Summary: | IndexedDB: Add intVersion to chromium/public/WebIDBMetadata.h | ||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | David Grogan <dgrogan> | ||||||||||||||
Component: | New Bugs | Assignee: | David Grogan <dgrogan> | ||||||||||||||
Status: | RESOLVED FIXED | ||||||||||||||||
Severity: | Normal | CC: | abarth, alecflett, dglazkov, fishd, jamesr, jsbell, tkent+wkapi, tony, webkit.review.bot | ||||||||||||||
Priority: | P2 | ||||||||||||||||
Version: | 528+ (Nightly build) | ||||||||||||||||
Hardware: | Unspecified | ||||||||||||||||
OS: | Unspecified | ||||||||||||||||
Bug Depends on: | |||||||||||||||||
Bug Blocks: | 91414 | ||||||||||||||||
Attachments: |
|
Description
David Grogan
2012-07-16 11:15:51 PDT
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> All reviewed patches have been landed. Closing bug. |