[webkit-dev] Settings and Preferences in layout tests

[Ryosuke Niwa]

On Wed, Sep 26, 2012 at 1:44 PM, Simon Fraser <simon.fraser at apple.com>wrote:

> We have a lot of tests that poke internal settings, via testRunner:
>
>     testRunner.setFrameFlatteningEnabled(true);
>
> or window.internals:
>
>     internals.settings.setPageScaleFactor(0.5, 0, 0);
>
> and some that poke preferences, like:
>
>     testRunner.overridePreference("WebKitUsesPageCachePreferenceKey", 1);
>
> First, direct calls on testRunner that just set preferences should be
> migrated to internals.settings or testRunner.overridePreference calls, I
> think (I don't know if either is preferred).
>

I support the idea of unifying the approaches and just use
internals.settings. However, the last time I checked, Alexey had some
concerns about using internals due to settings may not be properly
propagated to WebKit2 layer. Has this concern been addressed?

Secondly, I see no guarantee that these settings and preferences are reset
> to their default values before the next test, and we don't seem to have a
> consistent strategy about how to do this.
>

That sounds really bad. We should fix that.

For internal settings, we have InternalSettings::Backup methods. However,
> there's no enforcement that when changing a setting in a test for the first
> time, developers also add code to reset it in InternalSettings.
>
> I looked at testRunner.overridePreference(), and it doesn't appear to
> reset the value at the end of the test.
>
> So I think we need clearer rules here. I suggest:
>
> * testRunner methods that just set preferences should migrate to
> internals.settings or testRunner.overridePreference
> * we should choose between internals.settings or
> testRunner.overridePreference if that makes sense.
> * we should enforce a policy that patches adding a settings/prefs toggle
> should, if necessary, add code to reset between tests.
>

Sounds good to me. I'm surprised that the third point isn't already
enforced. It seems like that'll be a review failure.

- Ryosuke
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.webkit.org/pipermail/webkit-dev/attachments/20120926/aad156d7/attachment.html>