[webkit-reviews] review denied: [Bug 84208] [Chromium] IndexedDB: Prep for changing keyPath return type : [Attachment 137631] Patch

[bugzilla-daemon at webkit.org]

James Robinson <jamesr at chromium.org> has denied Joshua Bell
<jsbell at chromium.org>'s request for review:
Bug 84208: [Chromium] IndexedDB: Prep for changing keyPath return type
https://bugs.webkit.org/show_bug.cgi?id=84208

Attachment 137631: Patch
https://bugs.webkit.org/attachment.cgi?id=137631&action=review

------- Additional Comments from James Robinson <jamesr at chromium.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=137631&action=review


> Source/WebKit/chromium/public/WebIDBIndex.h:2
> + * Copyright (C) 2012 Google Inc. All rights reserved.

we don't do this in WebKit. leave the date alone

>> Source/WebKit/chromium/src/WebIDBIndexImpl.h:48
>> +	virtual WebString keyPathString() const OVERRIDE;
> 
> You probably already know, but you can leave off the OVERRIDEs for WebKit API
overrides if they make commit engineering difficult.

we typically don't use OVERRIDE for things in the WebKit API.  The hard rule is
that if the declaration you are overriding is in a different repository, do not
use OVERRIDE cos it'll make staging changes difficult. In this case,
keyPathString() is in the same repo so that shouldn't be an issue - but we also
say to favor consistency, which would be a strike against using OVERRIDE here. 
So on balance I think you should not add OVERRIDE here unless you want to do it
everywhere