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.
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?
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?
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).
Created attachment 145463 [details] Moving of setApplicationCacheOriginQuota from LayoutTestController to Internals
(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.
(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.
Please also note the new blocking bug. Perhaps it was just a bad idea to move any functions that change settings to window.internals.
(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.
Hi, I'd like to work on this issue. Could it be assigned to me?
Created attachment 214511 [details] WIP patch for EWS testing
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
Created attachment 214522 [details] WIP patch (v2)
Created attachment 214534 [details] Proposed patch
Comment on attachment 214534 [details] Proposed patch Looks good! r=me
Comment on attachment 214534 [details] Proposed patch What is going to undo the change after the current test finishes?
Created attachment 215424 [details] Proposed patch (v2) Patch was rebased with branch master.
(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().
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.
(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?
(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.
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.
(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.
So, do I need to update my patch or is it ok to be integrated?
Created attachment 215679 [details] Proposed patch (v3) Patch was updated: - reset the origin quota to default value (in Internals::resetToConsistentState) - rebased with branch master
(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? ?
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
Created attachment 215685 [details] Patch Updated patch with missing symbols (according to Win build)
(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.
Comment on attachment 215685 [details] Patch Clearing flags on attachment: 215685 Committed r158450: <http://trac.webkit.org/changeset/158450>
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.
Looks like this was landed, but commit-queue couldn't resolve the bug.