Bug 28359 – Cleanup: Undo some accidental changes around DOMApplicationCache

• Status: RESOLVED
• Resolution: FIXED
• Component: WebCore Misc.

[Michael Nordman]

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.

[Michael Nordman]

Created attachment 34928 [details]
simple cleanup

[Eric Seidel (no email)]

Comment on attachment 34928 [details]
simple cleanup

The DOMWindow change needs a test.  It's possible to test preference changes in DumpRenderTree these days.

[Michael Nordman]

(In reply to comment #2)
> It's possible to test preference changes in DumpRenderTree these days.

What does that mean?

[Eric Seidel (no email)]

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. :)

[Michael Nordman]

Created attachment 39035 [details]
cleanup with layout test

[Alexey Proskuryakov]

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

[Michael Nordman]

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

[Eric Seidel (no email)]

Exception: Unknown committer: [e-mail]

Sigh.  I need to fix the commit queue to not wedge itself when a non-committer sets commit-queue+. :(

[Eric Seidel (no email)]

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

[Eric Seidel (no email)]

Comment on attachment 39035 [details]
cleanup with layout test

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

Looks like yet another flakey media test. :(

[Eric Seidel (no email)]

Comment on attachment 39035 [details]
cleanup with layout test

Clearing flags on attachment: 39035

Committed r48108: <http://trac.webkit.org/changeset/48108>

[Eric Seidel (no email)]

All reviewed patches have been landed.  Closing bug.

[Michael Nordman]

(In reply to comment #12)
> All reviewed patches have been landed.  Closing bug.

Thank you Eric!