Bug 60985 - [Chromium] WebKit/WebCore changes to support WebSQLDatabase integration with Chrome's unified quota management system.
Summary: [Chromium] WebKit/WebCore changes to support WebSQLDatabase integration with ...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit API (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Michael Nordman
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2011-05-17 13:49 PDT by Michael Nordman
Modified: 2011-05-20 16:49 PDT (History)
6 users (show)

See Also:


Attachments
split (6.10 KB, patch)
2011-05-17 14:05 PDT, Michael Nordman
no flags Details | Formatted Diff | Diff
split (5.92 KB, patch)
2011-05-17 16:06 PDT, Michael Nordman
no flags Details | Formatted Diff | Diff
unified (11.67 KB, patch)
2011-05-19 14:40 PDT, Michael Nordman
no flags Details | Formatted Diff | Diff
unified (11.67 KB, patch)
2011-05-19 14:48 PDT, Michael Nordman
no flags Details | Formatted Diff | Diff
unified (11.66 KB, patch)
2011-05-20 12:42 PDT, Michael Nordman
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
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]
split
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]
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 6 Michael Nordman 2011-05-19 14:40:37 PDT
Created attachment 94119 [details]
unified
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]
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: 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]
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.