Bug 60985

Summary: [Chromium] WebKit/WebCore changes to support WebSQLDatabase integration with Chrome's unified quota management system.
Product: WebKit Reporter: Michael Nordman <michaeln>
Component: WebKit APIAssignee: Michael Nordman <michaeln>
Severity: Normal CC: commit-queue, dimich, fishd, kinuko, levin, webkit.review.bot
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Description Flags
unified none

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 1 Michael Nordman 2011-05-17 14:05:17 PDT
Created attachment 93814 [details]
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] [3]
Source/WebKit/chromium/public/WebDatabase.h:63:  WEBKIT_API should not be used on a function with a body.  [readability/webkit_api] [5]
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]

fix style
Comment 4 Kinuko Yasuda 2011-05-17 22:51:22 PDT
Comment on attachment 93841 [details]

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 6 Michael Nordman 2011-05-19 14:40:37 PDT
Created attachment 94119 [details]
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] [4]
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]

Fixed style.

The chrome-side can be seen here...
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]

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]

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]

made all suggested changes
Comment 13 David Levin 2011-05-20 14:42:58 PDT
Comment on attachment 94261 [details]

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: abarth@webkit.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]

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.