Bug 28359

Summary: Cleanup: Undo some accidental changes around DOMApplicationCache
Product: WebKit Reporter: Michael Nordman <michaeln>
Component: WebCore Misc.Assignee: Nobody <webkit-unassigned>
Status: RESOLVED FIXED    
Severity: Normal CC: ap, eric
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: All   
OS: All   
Attachments:
Description Flags
simple cleanup
eric: review-
cleanup with layout test none

Description Michael Nordman 2009-08-16 10:31:34 PDT
Two recent revision made two inavdertent changes around DOMApplicationCache. Those changes should be undone.

r46609 made it a programming error to construct a DOMApplicationCache instance with a null frame ptr.
r47018 made a change to return 'null' for window.applicationCache when the feature is disabled in the preferences.
Comment 1 Michael Nordman 2009-08-16 10:39:35 PDT
Created attachment 34928 [details]
simple cleanup
Comment 2 Eric Seidel (no email) 2009-08-17 17:15:14 PDT
Comment on attachment 34928 [details]
simple cleanup

The DOMWindow change needs a test.  It's possible to test preference changes in DumpRenderTree these days.
Comment 3 Michael Nordman 2009-08-17 17:49:26 PDT
(In reply to comment #2)
> It's possible to test preference changes in DumpRenderTree these days.

What does that mean?
Comment 4 Eric Seidel (no email) 2009-08-17 22:46:55 PDT
You can use things like:
        layoutTestController.overridePreference("WebKitDefaultFontSize", "24");

to override preferences for the duration of a single test.  Thus you can test that toggling this preference no longer affects the visibility of window.applicaitonCache.

Since we can test it, a test is required. :)
Comment 5 Michael Nordman 2009-09-03 20:25:45 PDT
Created attachment 39035 [details]
cleanup with layout test
Comment 6 Alexey Proskuryakov 2009-09-04 13:42:31 PDT
Comment on attachment 39035 [details]
cleanup with layout test

r=me

For the record, the rationale behind this behavior is:

- it's slightly more consistent for a frameless window to be able to construct a DOMApplicaitonCache, because window.applicationCache would be non-null if constructed before detaching;
- it's unlikely that any browser will have ApplicationCache compiled in, but disabled; run-time disabling is needed for clients like Dashboard. So, knowing that it's non-null can simplify code a little, and will do no harm.

Neither is particularly important, just minor cleanup.

+         layoutTestController.overridePreference("WebKitOfflineWebApplicationCacheEnabled", false);

Does this really work on all platforms? You may need to add this new test to Skipped list for some (or maybe the platforms that don't have it already skip all tests http/tests/in appcache?)
Comment 7 Michael Nordman 2009-09-04 13:58:42 PDT
> layoutTestController.overridePreference("WebKitOfflineWebApplicationCacheEnabled",
> false);
> 
> Does this really work on all platforms? You may need to add this new test to
> Skipped list for some (or maybe the platforms that don't have it already skip
> all tests http/tests/in appcache?)

I'm not able to run the tests on all other platforms.

layoutTestController seems to be very platform specific. For example, this particular preference name was not being respected in chrome's test_shell for example. I have a seperate CL to add support for this key in chrome land.

The others impls I looked at did seem to support this pref name in their overridePreference method.

Worth pointing out that the test does not confirm if the pref is actually disabled by the overridePreference call, nor does passage depend on the pref actually being disabled. The test will PASS even if a feature is still enabled.
Comment 8 Eric Seidel (no email) 2009-09-05 15:44:35 PDT
Exception: Unknown committer: michaeln@google.com

Sigh.  I need to fix the commit queue to not wedge itself when a non-committer sets commit-queue+. :(
Comment 9 Eric Seidel (no email) 2009-09-06 22:14:58 PDT
Comment on attachment 39035 [details]
cleanup with layout test

Rejecting patch 39035 from commit-queue.  This patch will require manual commit.

['WebKitTools/Scripts/run-webkit-tests', '--no-launch-safari', '--quiet', '--exit-after-n-failures=1'] failed with exit code 1
Comment 10 Eric Seidel (no email) 2009-09-07 00:17:02 PDT
Comment on attachment 39035 [details]
cleanup with layout test

media/video-played-ranges-1.html -> failed

Looks like yet another flakey media test. :(
Comment 11 Eric Seidel (no email) 2009-09-07 00:28:30 PDT
Comment on attachment 39035 [details]
cleanup with layout test

Clearing flags on attachment: 39035

Committed r48108: <http://trac.webkit.org/changeset/48108>
Comment 12 Eric Seidel (no email) 2009-09-07 00:28:34 PDT
All reviewed patches have been landed.  Closing bug.
Comment 13 Michael Nordman 2009-09-07 09:29:22 PDT
(In reply to comment #12)
> All reviewed patches have been landed.  Closing bug.

Thank you Eric!