Bug 91408 – IndexedDB: Add intVersion to chromium/public/WebIDBMetadata.h

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[David Grogan]

IndexedDB: Add intVersion to chromium/public/WebIDBMetadata.h

[David Grogan]

Created attachment 152570 [details]
Patch

[David Grogan]

Josh or Alec, could you look at this?

[WebKit Review Bot]

Please wait for approval from [e-mail], [e-mail], [e-mail], [e-mail] or [e-mail] before submitting, as this patch contains changes to the Chromium public API. See also https://trac.webkit.org/wiki/ChromiumWebKitAPI.

[Joshua Bell]

Comment on attachment 152570 [details]
Patch

LGTM

[David Grogan]

Created attachment 152573 [details]
Patch

[Darin Fisher (:fishd, Google)]

Comment on attachment 152573 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=152573&action=review

> Source/WebKit/chromium/public/WebIDBMetadata.h:48
>      WebIDBMetadata() { }

perhaps you should initialize intVersion?  it is good practice for
default constructors to initialize integer fields, etc.

[David Grogan]

Created attachment 152609 [details]
Patch

[David Grogan]

Comment on attachment 152573 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=152573&action=review

>> Source/WebKit/chromium/public/WebIDBMetadata.h:48
>>      WebIDBMetadata() { }
> 
> perhaps you should initialize intVersion?  it is good practice for
> default constructors to initialize integer fields, etc.

Sounds good.  Done.

[David Grogan]

Comment on attachment 152609 [details]
Patch

ping fishd / other WebKit API reviewers

[Adam Barth]

Comment on attachment 152609 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=152609&action=review

> Source/WebKit/chromium/public/WebIDBMetadata.h:49
>      WebString version;
> +    int64_t intVersion;

Is the plan to delete version and rename intVersion to version at some point?  I wonder if it make sense to add a macro for selecting between the two?  Maybe add a FIXME explaining your future plans?

[David Grogan]

Comment on attachment 152609 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=152609&action=review

>> Source/WebKit/chromium/public/WebIDBMetadata.h:49
>> +    int64_t intVersion;
> 
> Is the plan to delete version and rename intVersion to version at some point?  I wonder if it make sense to add a macro for selecting between the two?  Maybe add a FIXME explaining your future plans?

The plan is to delete version, but not necessarily to rename intVersion.  A macro seems like overkill, but a FIXME would be prudent.  Coming up.

[David Grogan]

Created attachment 152844 [details]
Patch for landing

[WebKit Review Bot]

Comment on attachment 152844 [details]
Patch for landing

Clearing flags on attachment: 152844

Committed r122884: <http://trac.webkit.org/changeset/122884>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Tony Chang]

This appears to be causing compile failures on Windows:
7>  WebIDBMetadata.cpp
7>C:\b\build\slave\webkit-win-latest-rel\build\src\third_party\WebKit\Source\WebKit\chromium\public\WebIDBMetadata.h(52): error C2146: syntax error : missing ';' before identifier 'intVersion'
7>C:\b\build\slave\webkit-win-latest-rel\build\src\third_party\WebKit\Source\WebKit\chromium\public\WebIDBMetadata.h(52): error C4430: missing type specifier - int assumed. Note: C++ does not support default-int
7>C:\b\build\slave\webkit-win-latest-rel\build\src\third_party\WebKit\Source\WebKit\chromium\public\WebIDBMetadata.h(52): error C4430: missing type specifier - int assumed. Note: C++ does not support default-int
7>C:\b\build\slave\webkit-win-latest-rel\build\src\third_party\WebKit\Source\WebKit\chromium\public\WebIDBMetadata.h(54): error C2614: 'WebKit::WebIDBMetadata' : illegal member initialization: 'intVersion' is not a base or member
7>c:\b\build\slave\webkit-win-latest-rel\build\src\third_party\webkit\source\webkit\chromium\public\WebIDBMetadata.h(52): error C2146: syntax error : missing ';' before identifier 'intVersion'
7>c:\b\build\slave\webkit-win-latest-rel\build\src\third_party\webkit\source\webkit\chromium\public\WebIDBMetadata.h(52): error C4430: missing type specifier - int assumed. Note: C++ does not support default-int
7>c:\b\build\slave\webkit-win-latest-rel\build\src\third_party\webkit\source\webkit\chromium\public\WebIDBMetadata.h(52): error C4430: missing type specifier - int assumed. Note: C++ does not support default-int
7>c:\b\build\slave\webkit-win-latest-rel\build\src\third_party\webkit\source\webkit\chromium\public\WebIDBMetadata.h(54): error C2614: 'WebKit::WebIDBMetadata' : illegal member initialization: 'intVersion' is not a base or member
7

[Tony Chang]

Reverted r122884 for reason:

Broke the chromium-win build.

Committed r122898: <http://trac.webkit.org/changeset/122898>

[David Grogan]

Created attachment 153115 [details]
Patch

[David Grogan]

Comment on attachment 153115 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=153115&action=review

> Source/Platform/chromium/public/WebCommon.h:77
> +typedef __int64 int64_t;

The windows try server suggests this will fix the problem:

original patch fails compile: http://build.chromium.org/p/tryserver.chromium/builders/win_layout_rel/builds/519
patch with typedef compiles fine: http://build.chromium.org/p/tryserver.chromium/builders/win_layout_rel/builds/520

[WebKit Review Bot]

Attachment 153115 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/Platform/ChangeLog', u'Source/Platf..." exit_code: 1
Source/Platform/chromium/public/WebCommon.h:77:  int64_t is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
Total errors found: 1 in 4 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[David Grogan]

(In reply to comment #19)
> Attachment 153115 [details] did not pass style-queue:
> 
> Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/Platform/ChangeLog', u'Source/Platf..." exit_code: 1
> Source/Platform/chromium/public/WebCommon.h:77:  int64_t is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
> Total errors found: 1 in 4 files
> 
> 
> If any of these errors are false positives, please file a bug against check-webkit-style.

Done: https://bugs.webkit.org/show_bug.cgi?id=91691

[David Grogan]

Comment on attachment 153115 [details]
Patch

Adam, could you review this patch (again)?  Thanks.

[Adam Barth]

Comment on attachment 153115 [details]
Patch

We must use 64-bit numbers in the WebKit API.  I'm surprised that we'd need this typedef.

[Adam Barth]

Looks like we use "long long" for 64 bit ints.

[Adam Barth]

e.g., WebFrame::identifier()

[David Grogan]

Created attachment 153147 [details]
Patch

[David Grogan]

(In reply to comment #23)
> Looks like we use "long long" for 64 bit ints.

Switched to long long.  Another look?

[WebKit Review Bot]

Comment on attachment 153147 [details]
Patch

Clearing flags on attachment: 153147

Committed r123059: <http://trac.webkit.org/changeset/123059>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.