Bug 43794 - Some settings are linked to the PageGroup not the Page. Create a new class for those.
Summary: Some settings are linked to the PageGroup not the Page. Create a new class f...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 528+ (Nightly build)
Hardware: Other OS X 10.5
: P2 Normal
Assignee: Jeremy Orlow
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2010-08-10 09:06 PDT by Jeremy Orlow
Modified: 2010-09-27 04:09 PDT (History)
13 users (show)

See Also:


Attachments
Patch (24.60 KB, patch)
2010-08-10 09:20 PDT, Jeremy Orlow
no flags Details | Formatted Diff | Diff
Patch (23.69 KB, patch)
2010-08-10 09:33 PDT, Jeremy Orlow
no flags Details | Formatted Diff | Diff
Patch (24.35 KB, patch)
2010-08-11 03:55 PDT, Jeremy Orlow
no flags Details | Formatted Diff | Diff
Patch (26.55 KB, patch)
2010-08-11 04:37 PDT, Jeremy Orlow
no flags Details | Formatted Diff | Diff
Patch (26.55 KB, patch)
2010-08-11 06:19 PDT, Jeremy Orlow
no flags Details | Formatted Diff | Diff
Patch (26.82 KB, patch)
2010-08-11 06:54 PDT, Jeremy Orlow
no flags Details | Formatted Diff | Diff
Patch (18.37 KB, patch)
2010-08-13 05:32 PDT, Jeremy Orlow
abarth: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Jeremy Orlow 2010-08-10 09:06:33 PDT
Some settings are linked to the PageGroup not the Page.  Create a new class for those.
Comment 1 Jeremy Orlow 2010-08-10 09:20:47 PDT
Created attachment 64015 [details]
Patch
Comment 2 Jeremy Orlow 2010-08-10 09:24:26 PDT
It's likely this will fail on some of the EWS bots.  Will watch them and fix other dependencies similar to the mac port one.  Please review assuming these will be fixed before landing.

ccing Steve since we discussed this together and mjs/darin/sam/ap in case they disagree that this is a move in the right direction.
Comment 3 Jeremy Orlow 2010-08-10 09:33:26 PDT
Created attachment 64016 [details]
Patch
Comment 4 Steve Block 2010-08-11 02:43:29 PDT
Comment on attachment 64016 [details]
Patch

WebCore/page/GroupSettings.cpp:13
 +   * THIS SOFTWARE IS PROVIDED BY APPLE COMPUTER, INC. ``AS IS'' AND ANY
Wrong copyright holder

WebCore/page/GroupSettings.h:57
 +      unsigned m_localStorageQuota;
I know you're just moving code, but it might be good while you're here to make the units clear in the variable name.

WebCore/page/GroupSettings.h:55
 +      PageGroup* m_pageGroup;
Why do we need this?

WebCore/page/GroupSettings.h:44
 +      unsigned localStorageQuota() const { return m_localStorageQuota; }
These were previously guarded with ENABLE(DOM_STORAGE). It looks safe, but did you mean to remove the guard?

WebCore/page/Settings.h:136
 +          unsigned sessionStorageQuota() const { return m_sessionStorageQuota; }
Ditto about removing guard

WebKit/mac/WebView/WebView.mm:690
 +      _private->page->group().groupSettings()->setLocalStorageDatabasePath([[self preferences] _localStorageDatabasePath]);
This probably warrants an comment explaining that this will overwrite the setting for the group if it's already been set by another WebView/Page. You could also add that we might want to expose a group setting in the WebKit API in the future.
Comment 5 Jeremy Orlow 2010-08-11 02:49:52 PDT
(In reply to comment #4)
> (From update of attachment 64016 [details])
> WebCore/page/GroupSettings.cpp:13
>  +   * THIS SOFTWARE IS PROVIDED BY APPLE COMPUTER, INC. ``AS IS'' AND ANY
> Wrong copyright holder

Copied in a copyright form another file that says "APPLE AND ITS CONTRIBUTORS"

> WebCore/page/GroupSettings.h:57
>  +      unsigned m_localStorageQuota;
> I know you're just moving code, but it might be good while you're here to make the units clear in the variable name.

Good idea.

> WebCore/page/GroupSettings.h:55
>  +      PageGroup* m_pageGroup;
> Why do we need this?

We don't.

> WebCore/page/GroupSettings.h:44
>  +      unsigned localStorageQuota() const { return m_localStorageQuota; }
> These were previously guarded with ENABLE(DOM_STORAGE). It looks safe, but did you mean to remove the guard?

Half the stuff was within guards, half wasn't.  I didn't see any reason why we needed the guards though, and it added a bunch of code clutter.  So I left them out (and took them out of Settings.h).

> WebKit/mac/WebView/WebView.mm:690
>  +      _private->page->group().groupSettings()->setLocalStorageDatabasePath([[self preferences] _localStorageDatabasePath]);
> This probably warrants an comment explaining that this will overwrite the setting for the group if it's already been set by another WebView/Page. You could also add that we might want to expose a group setting in the WebKit API in the future.

Good point.  Future additions to GroupSettings would hopefully get their own abstraction in various WebKit layers.
Comment 6 Jeremy Orlow 2010-08-11 03:55:00 PDT
Created attachment 64090 [details]
Patch
Comment 7 Early Warning System Bot 2010-08-11 04:06:09 PDT
Attachment 64090 [details] did not build on qt:
Build output: http://queues.webkit.org/results/3748052
Comment 8 Steve Block 2010-08-11 04:10:09 PDT
Comment on attachment 64090 [details]
Patch

WebCore/page/GroupSettings.h:44
 +      unsigned localStorageQuotaBytes() const { return m_localStorageQuotaBytes; }
I guess there are no other call sites of these methods to update?!
Comment 9 Jeremy Orlow 2010-08-11 04:21:03 PDT
Chromium uses it the quota param.  Just haven't updated that part yet.
Comment 10 Jeremy Orlow 2010-08-11 04:35:10 PDT
Actually, Chrome access it a different(In reply to comment #9)
> Chromium uses it the quota param.  Just haven't updated that part yet.

Nm, was mixed up.  Apparently no one overrides that default!

Fixed the QT build in the same way I did the mac one (with the same fixme).  WIll upload in a sec.
Comment 11 Jeremy Orlow 2010-08-11 04:37:31 PDT
Created attachment 64095 [details]
Patch
Comment 12 Early Warning System Bot 2010-08-11 04:51:50 PDT
Attachment 64095 [details] did not build on qt:
Build output: http://queues.webkit.org/results/3714055
Comment 13 Jeremy Orlow 2010-08-11 06:19:19 PDT
Created attachment 64104 [details]
Patch
Comment 14 Early Warning System Bot 2010-08-11 06:32:46 PDT
Attachment 64104 [details] did not build on qt:
Build output: http://queues.webkit.org/results/3777058
Comment 15 Jeremy Orlow 2010-08-11 06:54:18 PDT
Created attachment 64108 [details]
Patch
Comment 16 Jeremy Orlow 2010-08-11 07:16:08 PDT
K...looks like it builds now.  Will commit tomorrow morning (BST) if no one else has comments.
Comment 17 Adam Barth 2010-08-11 22:35:28 PDT
Comment on attachment 64108 [details]
Patch

lgtm.  It might be slightly confusing in the future which settings to put where, but I guess we'll sort that out.
Comment 18 Jeremy Orlow 2010-08-12 03:03:43 PDT
Committed r65228: <http://trac.webkit.org/changeset/65228>
Comment 19 WebKit Review Bot 2010-08-12 03:08:32 PDT
http://trac.webkit.org/changeset/65228 might have broken Qt Linux Release minimal
Comment 20 Jeremy Orlow 2010-08-12 03:21:32 PDT
Committed r65230: <http://trac.webkit.org/changeset/65230>
Comment 21 Jeremy Orlow 2010-08-12 04:11:08 PDT
Committed r65234: <http://trac.webkit.org/changeset/65234>
Comment 22 Jeremy Orlow 2010-08-12 04:16:08 PDT
Saw some weird breakage.  Reverting while I investigate.
Comment 23 Jeremy Orlow 2010-08-12 04:19:58 PDT
The two main failures: http://build.webkit.org/builders/Qt%20Linux%20Release%20minimal/builds/6852/steps/compile-webkit/logs/stdio
and
http://build.webkit.org/builders/SnowLeopard%20Intel%20Release%20(Tests)/builds/15327/steps/layout-test/logs/stdio

I also realized that there's a chance the page group name would be getting set after we're initializing the localStorageDatabasePath.

The correct solution might be to not move localStorageDatabasePath and instead just make the GroupSettings be the place for this stuff in the future.
Comment 24 WebKit Review Bot 2010-08-12 04:38:08 PDT
http://trac.webkit.org/changeset/65234 might have broken Qt Linux Release
Comment 25 Csaba Osztrogonác 2010-08-12 04:41:05 PDT
(In reply to comment #24)
> http://trac.webkit.org/changeset/65234 might have broken Qt Linux Release

Not at all, it was a roll-out patch.
Comment 26 Jeremy Orlow 2010-08-13 05:01:25 PDT
Yeah, I don't see an easy way to handle localStoragePath, but quota should still be able to move over.  Will roll another patch that doesn't try to move localStoragePath.
Comment 27 Jeremy Orlow 2010-08-13 05:32:13 PDT
Created attachment 64325 [details]
Patch
Comment 28 Jeremy Orlow 2010-08-16 03:03:14 PDT
Committed r65405: <http://trac.webkit.org/changeset/65405>
Comment 29 WebKit Review Bot 2010-08-16 03:08:19 PDT
http://trac.webkit.org/changeset/65405 might have broken Qt Linux Release minimal and Qt Linux ARMv5 Release
Comment 30 Lyon Chen 2010-09-24 07:51:16 PDT
Based on http://dev.w3.org/html5/webstorage/#disk-space, the local storage should also have total amount limit and per site limit. Make it per-PageGroup setting will prevent us from enforce per-site quota, also leave it as per-page setting will make it hard to enforce this quota when user open multiple pages for the same site.

So maybe we need a LocalStorageTracker like DatabaseTracker? Or combine them together?
Comment 31 Jeremy Orlow 2010-09-27 04:09:06 PDT
(In reply to comment #30)
> Based on http://dev.w3.org/html5/webstorage/#disk-space, the local storage should also have total amount limit and per site limit. Make it per-PageGroup setting will prevent us from enforce per-site quota, also leave it as per-page setting will make it hard to enforce this quota when user open multiple pages for the same site.
>
> So maybe we need a LocalStorageTracker like DatabaseTracker? Or combine them together?

Maybe open a bug for making it possible to set different quotas for different origins (cc me if so).  You may also want to add a FIXME to this effect (with a link to the bug) into the code so that someone exposing the setting from a WebKit layer would consider making their API compatible with such a change.