Summary: | [WKTR] Allow changing the WKContextConfiguration between successive tests | ||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Myles C. Maxfield <mmaxfield> | ||||||||
Component: | New Bugs | Assignee: | Myles C. Maxfield <mmaxfield> | ||||||||
Status: | RESOLVED FIXED | ||||||||||
Severity: | Normal | CC: | andersca, ap, commit-queue, sam, thorton | ||||||||
Priority: | P2 | ||||||||||
Version: | WebKit Nightly Build | ||||||||||
Hardware: | Unspecified | ||||||||||
OS: | Unspecified | ||||||||||
Bug Depends on: | 148992, 148996 | ||||||||||
Bug Blocks: | |||||||||||
Attachments: |
|
Description
Myles C. Maxfield
2015-09-04 15:12:00 PDT
Created attachment 260630 [details]
Patch
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. 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. (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. (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? Created attachment 260826 [details]
Patch for committing
Committed r189530: <http://trac.webkit.org/changeset/189530> This caused multiple assertions on WK2. Rolling out. Re-opened since this is blocked by bug 148992 Test fix: http://trac.webkit.org/changeset/189534 Re-opened since this is blocked by bug 148996 Created attachment 260910 [details]
Patch
Committed r189668: <http://trac.webkit.org/changeset/189668> |