Bug 60985 – [Chromium] WebKit/WebCore changes to support WebSQLDatabase integration with Chrome's unified quota management system.

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKit API

[Michael Nordman]

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.

[Michael Nordman]

Created attachment 93814 [details]
split

[WebKit Review Bot]

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.

[Michael Nordman]

Created attachment 93841 [details]
split

fix style

[Kinuko Yasuda]

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?

[Michael Nordman]

I have some other changes to make in this area before this is ready. Removing review flag for now.

[Michael Nordman]

Created attachment 94119 [details]
unified

[WebKit Review Bot]

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.

[Michael Nordman]

Created attachment 94122 [details]
unified

Fixed style.

The chrome-side can be seen here...
http://codereview.chromium.org/7037018/

[Michael Nordman]

adding some reviewers to the cc list... we need get this in for M13

[Kinuko Yasuda]

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/

[Darin Fisher (:fishd, Google)]

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

[Michael Nordman]

Created attachment 94261 [details]
unified

made all suggested changes

[David Levin]

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.

[WebKit Commit Bot]

The commit-queue encountered the following flaky tests while processing attachment 94261 [details]:

http/tests/websocket/tests/multiple-connections.html bug 53825 (author: [e-mail])
The commit-queue is continuing to process your patch.

[WebKit Commit Bot]

Comment on attachment 94261 [details]
unified

Clearing flags on attachment: 94261

Committed r87001: <http://trac.webkit.org/changeset/87001>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.