IndexedDB: Add intVersion to chromium/public/WebIDBMetadata.h
Created attachment 152570 [details] Patch
Josh or Alec, could you look at this?
Please wait for approval from abarth@webkit.org, dglazkov@chromium.org, fishd@chromium.org, jamesr@chromium.org or tkent@chromium.org before submitting, as this patch contains changes to the Chromium public API. See also https://trac.webkit.org/wiki/ChromiumWebKitAPI.
Comment on attachment 152570 [details] Patch LGTM
Created attachment 152573 [details] Patch
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.
Created attachment 152609 [details] Patch
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.
Comment on attachment 152609 [details] Patch ping fishd / other WebKit API reviewers
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?
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.
Created attachment 152844 [details] Patch for landing
Comment on attachment 152844 [details] Patch for landing Clearing flags on attachment: 152844 Committed r122884: <http://trac.webkit.org/changeset/122884>
All reviewed patches have been landed. Closing bug.
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
Reverted r122884 for reason: Broke the chromium-win build. Committed r122898: <http://trac.webkit.org/changeset/122898>
Created attachment 153115 [details] Patch
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
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.
(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
Comment on attachment 153115 [details] Patch Adam, could you review this patch (again)? Thanks.
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.
Looks like we use "long long" for 64 bit ints.
e.g., WebFrame::identifier()
Created attachment 153147 [details] Patch
(In reply to comment #23) > Looks like we use "long long" for 64 bit ints. Switched to long long. Another look?
Comment on attachment 153147 [details] Patch Clearing flags on attachment: 153147 Committed r123059: <http://trac.webkit.org/changeset/123059>