Bug 148833 – [WKTR] Allow changing the WKContextConfiguration between successive tests

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Myles C. Maxfield]

[WKTR] Allow changing the WKContextConfiguration between successive tests

[Myles C. Maxfield]

Created attachment 260630 [details]
Patch

[Myles C. Maxfield]

Comment on attachment 260630 [details]
Patch

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

> Tools/WebKitTestRunner/TestController.cpp:466
> +    // Modify contextConfiguration here.

I might as well do this modification inside generateContextConfiguration.

[Tim Horton]

Comment on attachment 260630 [details]
Patch

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

I might be confused.

> Tools/WebKitTestRunner/ContextConfigurationOptions.h:32
> +    bool useThreadedScrolling { false };

This is not a "context configuration option", it's something that gets written to WKPreferences (which we can mutate without recreating the WKContext).

> Tools/WebKitTestRunner/ContextConfigurationOptions.h:33
> +    bool useRemoteLayerTree { false };

This is not a "context configuration option", it's something that gets turned into a NSUserDefault that's read at view creation time. Possible that it *should* be, though. This seems like the only valid one.

> Tools/WebKitTestRunner/ContextConfigurationOptions.h:34
> +    bool shouldShowWebView { false };

This is not a "context configuration option", it's something that gets read at view/window creation time and determines where to place the window (and whether to make it frontmost).

> Tools/WebKitTestRunner/ContextConfigurationOptions.h:35
> +    bool useTiledDrawing { false };

Please get rid of this one. Nothing reads nor writes it.

> Tools/WebKitTestRunner/ContextConfigurationOptions.h:37
> +    bool useFixedLayout { false };

This is not a "context configuration option", it's something that gets pushed (in EFL only) to the WKPage (and IIRC can be mutated at run time, it's not a one-time thing).

> Tools/WebKitTestRunner/TestController.cpp:470
> +    // FIXME: Migrate these preferences to WKContextConfigurationRef.

Agreed on this point.

[Myles C. Maxfield]

(In reply to comment #3)
> Comment on attachment 260630 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=260630&action=review
> 
> I might be confused.
> 
> > Tools/WebKitTestRunner/ContextConfigurationOptions.h:32
> > +    bool useThreadedScrolling { false };
> 
> This is not a "context configuration option", it's something that gets
> written to WKPreferences (which we can mutate without recreating the
> WKContext).
> 
> > Tools/WebKitTestRunner/ContextConfigurationOptions.h:33
> > +    bool useRemoteLayerTree { false };
> 
> This is not a "context configuration option", it's something that gets
> turned into a NSUserDefault that's read at view creation time. Possible that
> it *should* be, though. This seems like the only valid one.
> 
> > Tools/WebKitTestRunner/ContextConfigurationOptions.h:34
> > +    bool shouldShowWebView { false };
> 
> This is not a "context configuration option", it's something that gets read
> at view/window creation time and determines where to place the window (and
> whether to make it frontmost).
> 
> > Tools/WebKitTestRunner/ContextConfigurationOptions.h:35
> > +    bool useTiledDrawing { false };
> 
> Please get rid of this one. Nothing reads nor writes it.
> 
> > Tools/WebKitTestRunner/ContextConfigurationOptions.h:37
> > +    bool useFixedLayout { false };
> 
> This is not a "context configuration option", it's something that gets
> pushed (in EFL only) to the WKPage (and IIRC can be mutated at run time,
> it's not a one-time thing).
> 
> > Tools/WebKitTestRunner/TestController.cpp:470
> > +    // FIXME: Migrate these preferences to WKContextConfigurationRef.
> 
> Agreed on this point.

I was talking with Anders about this. Rather than discriminating between options which require rebuilding the WebView and options which require rebuilding the entire WKContextConfiguration, we decided that it isn't worth the added complexity. Therefore, we are making all cases which previously caused us to rebuild the WebView to now rebuild the ContextConfiguration.

[Tim Horton]

(In reply to comment #4)
> I was talking with Anders about this. Rather than discriminating between
> options which require rebuilding the WebView and options which require
> rebuilding the entire WKContextConfiguration, we decided that it isn't worth
> the added complexity. Therefore, we are making all cases which previously
> caused us to rebuild the WebView to now rebuild the ContextConfiguration.

OK, but the name is going from being accurate in at best 1 case out of 5 to 0/5. Can it be "TestOptions" or "TestRunnerConfiguration" or something?

[Myles C. Maxfield]

Created attachment 260826 [details]
Patch for committing

[Myles C. Maxfield]

Committed r189530: <http://trac.webkit.org/changeset/189530>

[Alexey Proskuryakov]

This caused multiple assertions on WK2. Rolling out.

[WebKit Commit Bot]

Re-opened since this is blocked by bug 148992

[Myles C. Maxfield]

Test fix: http://trac.webkit.org/changeset/189534

[WebKit Commit Bot]

Re-opened since this is blocked by bug 148996

[Myles C. Maxfield]

Created attachment 260910 [details]
Patch

[Myles C. Maxfield]

Committed r189668: <http://trac.webkit.org/changeset/189668>