Bug 43794 – Some settings are linked to the PageGroup not the Page. Create a new class for those.

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Jeremy Orlow]

Some settings are linked to the PageGroup not the Page.  Create a new class for those.

[Jeremy Orlow]

Created attachment 64015 [details]
Patch

[Jeremy Orlow]

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.

[Jeremy Orlow]

Created attachment 64016 [details]
Patch

[Steve Block]

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.

[Jeremy Orlow]

(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.

[Jeremy Orlow]

Created attachment 64090 [details]
Patch

[Early Warning System Bot]

Attachment 64090 [details] did not build on qt:
Build output: http://queues.webkit.org/results/3748052

[Steve Block]

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?!

[Jeremy Orlow]

Chromium uses it the quota param.  Just haven't updated that part yet.

[Jeremy Orlow]

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.

[Jeremy Orlow]

Created attachment 64095 [details]
Patch

[Early Warning System Bot]

Attachment 64095 [details] did not build on qt:
Build output: http://queues.webkit.org/results/3714055

[Jeremy Orlow]

Created attachment 64104 [details]
Patch

[Early Warning System Bot]

Attachment 64104 [details] did not build on qt:
Build output: http://queues.webkit.org/results/3777058

[Jeremy Orlow]

Created attachment 64108 [details]
Patch

[Jeremy Orlow]

K...looks like it builds now.  Will commit tomorrow morning (BST) if no one else has comments.

[Adam Barth]

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.

[Jeremy Orlow]

Committed r65228: <http://trac.webkit.org/changeset/65228>

[WebKit Review Bot]

http://trac.webkit.org/changeset/65228 might have broken Qt Linux Release minimal

[Jeremy Orlow]

Committed r65230: <http://trac.webkit.org/changeset/65230>

[Jeremy Orlow]

Committed r65234: <http://trac.webkit.org/changeset/65234>

[Jeremy Orlow]

Saw some weird breakage.  Reverting while I investigate.

[Jeremy Orlow]

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.

[WebKit Review Bot]

http://trac.webkit.org/changeset/65234 might have broken Qt Linux Release

[Csaba Osztrogonác]

(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.

[Jeremy Orlow]

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.

[Jeremy Orlow]

Created attachment 64325 [details]
Patch

[Jeremy Orlow]

Committed r65405: <http://trac.webkit.org/changeset/65405>

[WebKit Review Bot]

http://trac.webkit.org/changeset/65405 might have broken Qt Linux Release minimal and Qt Linux ARMv5 Release

[Lyon Chen]

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?

[Jeremy Orlow]

(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.