|Summary:||[Chromium] WebKit/WebCore changes to support WebSQLDatabase integration with Chrome's unified quota management system.|
|Product:||WebKit||Reporter:||Michael Nordman <michaeln>|
|Component:||WebKit API||Assignee:||Michael Nordman <michaeln>|
|Severity:||Normal||CC:||commit-queue, dimich, fishd, kinuko, levin, webkit.review.bot|
|Version:||528+ (Nightly build)|
Description Michael Nordman 2011-05-17 13:49:10 PDT
Split WebDatabase::updateDatabaseSizeAndSpaceAvailable() into two methods to support unified quota management. Previously, in the silo'd quota system, the only changes to databases could change the space available. In the new system changes in other systems can affect how much space is available for databases. So it's useful to split this method into two.
Comment 2 WebKit Review Bot 2011-05-17 14:06:57 PDT
Attachment 93814 [details] did not pass style-queue: Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCor..." exit_code: 1 Source/WebCore/storage/chromium/QuotaTracker.h:54: Missing space after , [whitespace/comma]  Source/WebKit/chromium/public/WebDatabase.h:63: WEBKIT_API should not be used on a function with a body. [readability/webkit_api]  Total errors found: 2 in 6 files If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 3 Michael Nordman 2011-05-17 16:06:02 PDT
Created attachment 93841 [details] split fix style
Comment 4 Kinuko Yasuda 2011-05-17 22:51:22 PDT
Comment on attachment 93841 [details] split Looks good to me. View in context: https://bugs.webkit.org/attachment.cgi?id=93841&action=review > Source/WebCore/storage/chromium/QuotaTracker.h:52 > + unsigned long long databaseSize); style nit: do we format this file chromium-style since it's under chromium directory? (I often see files in chromium-style format under WebKit/chromium but not often under WebCore) > Source/WebKit/chromium/public/WebDatabase.h:59 > + // Deprecated This should go away after chrome side change? Maybe we should put a brief comment about that?
Comment 5 Michael Nordman 2011-05-18 12:05:23 PDT
I have some other changes to make in this area before this is ready. Removing review flag for now.
Comment 7 WebKit Review Bot 2011-05-19 14:43:28 PDT
Attachment 94119 [details] did not pass style-queue: Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCor..." exit_code: 1 Source/WebKit/chromium/public/WebKitClient.h:143: origin_identifier is incorrectly named. Don't use underscores in your identifier names. [readability/naming]  Total errors found: 1 in 9 files If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 8 Michael Nordman 2011-05-19 14:48:20 PDT
Created attachment 94122 [details] unified Fixed style. The chrome-side can be seen here... http://codereview.chromium.org/7037018/
Comment 9 Michael Nordman 2011-05-19 19:40:53 PDT
adding some reviewers to the cc list... we need get this in for M13
Comment 10 Kinuko Yasuda 2011-05-20 00:08:46 PDT
Comment on attachment 94122 [details] unified This looks good to me except for some nits in ChangeLogs. View in context: https://bugs.webkit.org/attachment.cgi?id=94122&action=review > Source/WebCore/ChangeLog:9 > + No new tests. (OOPS!) I think here we need to say 'No functionality change therefore no new tests.' or something like that. > Source/WebKit/chromium/ChangeLog:5 > + Changes to allow the WebDatabase system to participate in Chrome's unified quoata typo: s/quoata/quota/
Comment 11 Darin Fisher (:fishd, Google) 2011-05-20 11:01:04 PDT
Comment on attachment 94122 [details] unified View in context: https://bugs.webkit.org/attachment.cgi?id=94122&action=review > Source/WebKit/chromium/src/WebDatabase.cpp:98 > + WebCore::QuotaTracker::instance().updateDatabaseSize(originIdentifier, name, size); WebCore:: prefixes not needed in this .cpp file
Comment 12 Michael Nordman 2011-05-20 12:42:23 PDT
Created attachment 94261 [details] unified made all suggested changes
Comment 13 David Levin 2011-05-20 14:42:58 PDT
Comment on attachment 94261 [details] unified View in context: https://bugs.webkit.org/attachment.cgi?id=94261&action=review A few minor comments. Nothing that really needs to be fixed before submitting. (As I was writing this Darin r+'ed.) > Source/WebCore/platform/chromium/PlatformBridge.h:171 > + // Returns the space available for the origin Comment seems redundant with the method name. > Source/WebKit/chromium/ChangeLog:10 > + - WebDatabase::updateDatababaseSize() typo: updateDatababaseSize But ok. > Source/WebKit/chromium/public/WebKitClient.h:142 > + // Returns the space available for the given origin This comment seems redundant with the method name. > Source/WebKit/chromium/src/WebDatabase.cpp:99 > +#endif // ENABLE(DATABASE) You really don't need "// ENABLE(DATABASE)" in WebKit especially for such a small if/endif region.
Comment 14 WebKit Commit Bot 2011-05-20 16:47:20 PDT
The commit-queue encountered the following flaky tests while processing attachment 94261 [details]: http/tests/websocket/tests/multiple-connections.html bug 53825 (author: firstname.lastname@example.org) The commit-queue is continuing to process your patch.
Comment 15 WebKit Commit Bot 2011-05-20 16:48:58 PDT
Comment on attachment 94261 [details] unified Clearing flags on attachment: 94261 Committed r87001: <http://trac.webkit.org/changeset/87001>
Comment 16 WebKit Commit Bot 2011-05-20 16:49:04 PDT
All reviewed patches have been landed. Closing bug.