Bug 87838 – Expose setApplicationCacheOriginQuota via window.internals

• Status: RESOLVED
• Resolution: FIXED
• Component: Tools / Tests

[Ravi Phaneendra Kasibhatla]

Created attachment 144773 [details]
Initial patch. 

Move setApplicationCacheOriginQuota API to  WebCore::Internals instead of LayoutTestController interface. This is from the set of functions that were identified in the contributor meeting's hackaton. http://trac.webkit.org/wiki/Internals_Hackathon.

[Ravi Phaneendra Kasibhatla]

I have attached just the basic patch (even with change log) since I am facing some issues with building chromium port. I will be soon uploading final patch after checking & resolving build issues in other ports (if any) complete with Change log also. Hence I haven't marked any review flags.

Chromium doesn't use the files in loader/appcache (with a few exceptions).

Maybe Michael (added) can advise how to mimic this for chromium?

[Alexey Proskuryakov]

Hackathon notes about this function appear wrong - this function is certainly implemented at least on Mac, so it's not just Qt.

Perhaps we should institute a policy that any time WebKit test coverage through LayoutTestContoller is removes, similar coverage should be added in TestWebKitAPI?

[Michael Nordman]

Comment on attachment 144773 [details]
Initial patch. 

View in context: https://bugs.webkit.org/attachment.cgi?id=144773&action=review

> Source/WebCore/testing/Internals.cpp:980
> +    cacheStorage().storeUpdatedQuotaForOrigin(document->securityOrigin(), quota);

This won't build in the chromium port because it doesn't use the ApplicationCacheStorage class, that file isn't compiled or linked into the project. Please just conditionally compile this method body out #if PLATFORM(CHROMIUM).

[Ravi Phaneendra Kasibhatla]

Created attachment 145463 [details]
Moving of setApplicationCacheOriginQuota from LayoutTestController to Internals

[Ravi Phaneendra Kasibhatla]

(In reply to comment #4)
> (From update of attachment 144773 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=144773&action=review
> 
> > Source/WebCore/testing/Internals.cpp:980
> > +    cacheStorage().storeUpdatedQuotaForOrigin(document->securityOrigin(), quota);
> 
> This won't build in the chromium port because it doesn't use the ApplicationCacheStorage class, that file isn't compiled or linked into the project. Please just conditionally compile this method body out #if PLATFORM(CHROMIUM).

Done.

[Ravi Phaneendra Kasibhatla]

(In reply to comment #3)
> Hackathon notes about this function appear wrong - this function is certainly implemented at least on Mac, so it's not just Qt.
Yes I cross verified that it is only implemented for Qt and Mac, but the movement to internals still looks correct and it doesn't effect test behavior.
> 
> Perhaps we should institute a policy that any time WebKit test coverage through LayoutTestContoller is removes, similar coverage should be added in TestWebKitAPI?
Are you suggesting that I should add new test for TestWebKitAPI for this? I will check on this and try to update.

[Alexey Proskuryakov]

Please also note the new blocking bug. Perhaps it was just a bad idea to move any functions that change settings to window.internals.

[Ojan Vafai]

(In reply to comment #8)
> Please also note the new blocking bug. Perhaps it was just a bad idea to move any functions that change settings to window.internals.

That would be a shame. It avoids so much code duplication to have them on internals. We should try to see if we can resolve the issues with settings on internals if possible.

[Afonso Costa]

Hi,

I'd like to work on this issue. Could it be assigned to me?

[Afonso Costa]

Created attachment 214511 [details]
WIP patch for EWS testing

[Build Bot]

Comment on attachment 214511 [details]
WIP patch for EWS testing

Attachment 214511 [details] did not pass win-ews (win):
Output: http://webkit-queues.appspot.com/results/4108516

[Afonso Costa]

Created attachment 214522 [details]
WIP patch (v2)

[Afonso Costa]

Created attachment 214534 [details]
Proposed patch

[Joseph Pecoraro]

Comment on attachment 214534 [details]
Proposed patch

Looks good! r=me

[Alexey Proskuryakov]

Comment on attachment 214534 [details]
Proposed patch

What is going to undo the change after the current test finishes?

[Afonso Costa]

Created attachment 215424 [details]
Proposed patch (v2)

Patch was rebased with branch master.

[Afonso Costa]

(In reply to comment #16)
> (From update of attachment 214534 [details])
> What is going to undo the change after the current test finishes?

Hi Alexey, the function clearAllApplicationCaches() is called before to set the new cache quota in all startTest().

[Alexey Proskuryakov]

clearAllApplicationCaches() is a TestRunner feature, so it has to be a responsibility of the test to do this?

Ideally, any settings that are changed duting a test should be reset automatically.

[Afonso Costa]

(In reply to comment #19)
> clearAllApplicationCaches() is a TestRunner feature, so it has to be a responsibility of the test to do this?
> 
> Ideally, any settings that are changed duting a test should be reset automatically.

Maybe the test should: reset -> set -> test -> reset (at the end), just to keep the initial state of any configuration that is being tested.

@Joseph, what do you think?

[Joseph Pecoraro]

(In reply to comment #20)
> (In reply to comment #19)
> > clearAllApplicationCaches() is a TestRunner feature, so it has to be a responsibility of the test to do this?
> > 
> > Ideally, any settings that are changed duting a test should be reset automatically.
> 
> Maybe the test should: reset -> set -> test -> reset (at the end), just to keep the initial state of any configuration that is being tested.
> 
> @Joseph, what do you think?

What Alexey means, is that the test driver should reset to a consistent state before every test. We cannot rely on the test reseting state at the end, because the test might crash or produce an error half way through, and then affect later tests. E.g. putting something in DumpRenderTree.mm resetWebViewToConsistentStateBeforeTesting(), and likewise for other ports.

Your change doesn't change the existing behavior, so I'm still okay with it. But it would be nice to improve this even further and have DumpRenderTree reset the state between tests.

[Alexey Proskuryakov]

Logically, the code to reset should be in Internals, because that's what changes the setting.

There is a "Backup" in InternalSettings, but I'm not sure if Internals methods have an established pattern for doing the right thing.

[Tony Chang]

(In reply to comment #22)
> Logically, the code to reset should be in Internals, because that's what changes the setting.
> 
> There is a "Backup" in InternalSettings, but I'm not sure if Internals methods have an established pattern for doing the right thing.

This is what Internals::resetToConsistentState is for.

[Afonso Costa]

So, do I need to update my patch or is it ok to be integrated?

[Afonso Costa]

Created attachment 215679 [details]
Proposed patch (v3)

Patch was updated: 
- reset the origin quota to default value (in Internals::resetToConsistentState)
- rebased with branch master

[Joseph Pecoraro]

(In reply to comment #25)
> Created an attachment (id=215679) [details]
> Proposed patch (v3)
> 
> Patch was updated: 
> - reset the origin quota to default value (in Internals::resetToConsistentState)
> - rebased with branch master

Should this patch be r? ?

[Build Bot]

Comment on attachment 215679 [details]
Proposed patch (v3)

Attachment 215679 [details] did not pass win-ews (win):
Output: http://webkit-queues.appspot.com/results/19358002

[Afonso Costa]

Created attachment 215685 [details]
Patch

Updated patch with missing symbols (according to Win build)

[Afonso Costa]

(In reply to comment #26)
> (In reply to comment #25)
> > Created an attachment (id=215679) [details] [details]
> > Proposed patch (v3)
> > 
> > Patch was updated: 
> > - reset the origin quota to default value (in Internals::resetToConsistentState)
> > - rebased with branch master
> 
> Should this patch be r? ?

Hi Joseph,

I'll set this flag after the patch is ok on EWS. Sorry, but I don't have a win environment to test it.

[WebKit Commit Bot]

Comment on attachment 215685 [details]
Patch

Clearing flags on attachment: 215685

Committed r158450: <http://trac.webkit.org/changeset/158450>

[Csaba Osztrogonác]

Comment on attachment 214534 [details]
Proposed patch

Cleared Joseph Pecoraro's review+ from obsolete attachment 214534 [details] so that this bug does not appear in http://webkit.org/pending-commit.

[Alexey Proskuryakov]

Looks like this was landed, but commit-queue couldn't resolve the bug.