Created attachment 313615 [details] Patch

Comment on attachment 313615 [details] Patch Attachment 313615 [details] did not pass mac-ews (mac): Output: http://webkit-queues.webkit.org/results/3978184 Number of test failures exceeded the failure limit.

Created attachment 313621 [details] Archive of layout-test-results from ews101 for mac-elcapitan The attached test failures were seen while running run-webkit-tests on the mac-ews. Bot: ews101 Port: mac-elcapitan Platform: Mac OS X 10.11.6

Comment on attachment 313615 [details] Patch Attachment 313615 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://webkit-queues.webkit.org/results/3978179 Number of test failures exceeded the failure limit.

Created attachment 313622 [details] Archive of layout-test-results from ews105 for mac-elcapitan-wk2 The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews. Bot: ews105 Port: mac-elcapitan-wk2 Platform: Mac OS X 10.11.6

Comment on attachment 313615 [details] Patch Attachment 313615 [details] did not pass mac-debug-ews (mac): Output: http://webkit-queues.webkit.org/results/3978168 Number of test failures exceeded the failure limit.

Created attachment 313624 [details] Archive of layout-test-results from ews112 for mac-elcapitan The attached test failures were seen while running run-webkit-tests on the mac-debug-ews. Bot: ews112 Port: mac-elcapitan Platform: Mac OS X 10.11.6

Comment on attachment 313615 [details] Patch Attachment 313615 [details] did not pass ios-sim-ews (ios-simulator-wk2): Output: http://webkit-queues.webkit.org/results/3978245 New failing tests: css3/filters/filter-repaint.html fast/images/exif-orientation-composited.html fast/images/exif-orientation.html editing/selection/caret-mode-document-begin-end.html http/tests/security/mixedContent/insecure-script-in-data-iframe-in-main-frame-blocked.html fast/canvas/canvas-blend-image.html css3/filters/effect-brightness-clamping.html fast/canvas/drawImage-with-small-values.html css3/blending/effect-background-blend-mode.html fast/canvas/canvas-imageSmoothingQuality.html fast/images/exif-orientation-css.html fast/images/image-controls-basic.html fast/canvas/canvas-blend-solid.html http/tests/appcache/disabled.html loader/meta-refresh-disabled.html

Created attachment 313629 [details] Archive of layout-test-results from ews121 for ios-simulator-wk2 The attached test failures were seen while running run-webkit-tests on the ios-sim-ews. Bot: ews121 Port: ios-simulator-wk2 Platform: Mac OS X 10.12.5

Comment on attachment 313615 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=313615&action=review > LayoutTests/ChangeLog:4 > + Use window.internals instead of overridePreference to set WebCore settings in tests > + https://bugs.webkit.org/show_bug.cgi?id=173714 With this change you should be able to eliminate any no longer used settings from InjectedBundle::overrideBoolPreferenceForTestRunner. > LayoutTests/ChangeLog:7 > + We try to include a description of the change here in the ChangeLog if it is not immediately obvious from the title. > LayoutTests/accessibility/gtk/caret-browsing-select-focus.html:17 > +if (window.testRunner && window.internals) { > + internals.setEnableCaretBrowsing(true); I'm a bit confused by these changes. I don't think `internals.setEnableCaretBrowsing` exists. Perhaps `internals.settings.setEnableCaretBrowsing` does? To clarify my earlier comment from: https://bugs.webkit.org/show_bug.cgi?id=128594#c3 I think, where possible, we should move: testRunner.overridePreference("WebKitFooEnabled", true); to: internals.settings.setFooEnabled(true) Since using internals.settings is more direct, would fail immediately and obviously if its a function that does not exist, and these settings are restored properly between tests.

(In reply to Joseph Pecoraro from comment #10) > With this change you should be able to eliminate any no longer used settings > from InjectedBundle::overrideBoolPreferenceForTestRunner. > We try to include a description of the change here in the ChangeLog if it is > not immediately obvious from the title. Right. This was just an experimental patch to see the results. > > LayoutTests/accessibility/gtk/caret-browsing-select-focus.html:17 > > +if (window.testRunner && window.internals) { > > + internals.setEnableCaretBrowsing(true); > > I'm a bit confused by these changes. I don't think > `internals.setEnableCaretBrowsing` exists. Perhaps > `internals.settings.setEnableCaretBrowsing` does? Indeed, it was a mistake. However, I tried setEnableCaretBrowsing locally and it still failed. There are other similar failures with undefined function even when the settings are in Settings.in but I'm not exactly sure what is the rule to automatically generate setter for boolean settings...

(In reply to Frédéric Wang (:fredw) from comment #11) > > I'm a bit confused by these changes. I don't think > > `internals.setEnableCaretBrowsing` exists. Perhaps > > `internals.settings.setEnableCaretBrowsing` does? > > Indeed, it was a mistake. However, I tried setEnableCaretBrowsing locally > and it still failed. There are other similar failures with undefined > function even when the settings are in Settings.in but I'm not exactly sure > what is the rule to automatically generate setter for boolean settings... OK, I read too quickly your message yesterday. The name of the settings seems to be "caretBrowsingEnabled" which is what I tried locally yesterday. I did not notice that you were actually highlighting to the missing ".settings" which was indeed another error on my side. I'll try to upload an updated patch and see if it gives better results :-)

Created attachment 313705 [details] Patch

Comment on attachment 313705 [details] Patch Attachment 313705 [details] did not pass mac-ews (mac): Output: http://webkit-queues.webkit.org/results/3983385 New failing tests: loader/meta-refresh-disabled.html editing/selection/caret-mode-document-begin-end.html http/tests/security/mixedContent/insecure-script-in-data-iframe-in-main-frame-blocked.html

Created attachment 313707 [details] Archive of layout-test-results from ews101 for mac-elcapitan The attached test failures were seen while running run-webkit-tests on the mac-ews. Bot: ews101 Port: mac-elcapitan Platform: Mac OS X 10.11.6

Comment on attachment 313705 [details] Patch Attachment 313705 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://webkit-queues.webkit.org/results/3983390 New failing tests: loader/meta-refresh-disabled.html http/tests/security/mixedContent/insecure-script-in-data-iframe-in-main-frame-blocked.html

Created attachment 313708 [details] Archive of layout-test-results from ews104 for mac-elcapitan-wk2 The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews. Bot: ews104 Port: mac-elcapitan-wk2 Platform: Mac OS X 10.11.6

Comment on attachment 313705 [details] Patch Attachment 313705 [details] did not pass mac-debug-ews (mac): Output: http://webkit-queues.webkit.org/results/3983400 New failing tests: loader/meta-refresh-disabled.html editing/selection/caret-mode-document-begin-end.html http/tests/security/mixedContent/insecure-script-in-data-iframe-in-main-frame-blocked.html

Created attachment 313709 [details] Archive of layout-test-results from ews115 for mac-elcapitan The attached test failures were seen while running run-webkit-tests on the mac-debug-ews. Bot: ews115 Port: mac-elcapitan Platform: Mac OS X 10.11.6

Comment on attachment 313705 [details] Patch Attachment 313705 [details] did not pass ios-sim-ews (ios-simulator-wk2): Output: http://webkit-queues.webkit.org/results/3983408 New failing tests: loader/meta-refresh-disabled.html http/tests/security/mixedContent/insecure-script-in-data-iframe-in-main-frame-blocked.html

Created attachment 313710 [details] Archive of layout-test-results from ews122 for ios-simulator-wk2 The attached test failures were seen while running run-webkit-tests on the ios-sim-ews. Bot: ews122 Port: ios-simulator-wk2 Platform: Mac OS X 10.12.5

Created attachment 313720 [details] Patch

These are the remaining uses of overridePreference: WebKit2AsynchronousPluginInitializationEnabled WebKit2AsynchronousPluginInitializationEnabledForAllPlugins WebKitCSSRegionsEnabled WebKitDefaultFontSize WebKitDefaultTextEncodingName WebKitDisplayImagesKey WebKitHiddenPageDOMTimerThrottlingEnabled WebKitJavaScriptEnabled WebKitMinimumFontSize WebKitShouldInvertColors WebKitStorageBlockingPolicy WebKitTabToLinksPreferenceKey WebKitUsesPageCachePreferenceKey

Comment on attachment 313720 [details] Patch Nice!

Committed r218754: <http://trac.webkit.org/changeset/218754>

More analysis on the remaining properties: These have special handling in InjectedBundle::overrideBoolPreferenceForTestRunner and do not correspond to a WebCore settings ; I wonder if we could extend Internal.idl to expose them: WebKitTabToLinksPreferenceKey WebKit2AsynchronousPluginInitializationEnabled WebKit2AsynchronousPluginInitializationEnabledForAllPlugins This maps to some setters on Settings.h but not to a settings in Setting.in ; I wonder if we could extend Internal.idl to expose them: WebKitDisplayImagesKey => LoadsImagesAutomatically WebKitJavaScriptEnabled => ScriptEnabled WebKitUsesPageCachePreferenceKey => UsesPageCache These seem to be WebKit 1 preferences: WebKitDefaultFontSize WebKitDefaultTextEncodingName WebKitMinimumFontSize WebKitShouldInvertColors WebKitStorageBlockingPolicy WebKitCSSRegionsEnabled is only used in fast/regions/region-leak-js-information-when-disabled-at-runtime.html, but it is commented out. Anyway, it seems the test is no longer relevant after https://trac.webkit.org/changeset/200524/webkit WebKitHiddenPageDOMTimerThrottlingEnabled is used in ./fast/dom/timer-throttling-hidden-page-non-nested.html and ./fast/dom/timer-throttling-hidden-page.html ; for some reason it is separated from the rest of the boolean settings in InjectedBundle::overrideBoolPreferenceForTestRunner, but it looks like we could do the same replacements as done here.

(In reply to Frédéric Wang (:fredw) from comment #26) > More analysis on the remaining properties: > > These have special handling in > InjectedBundle::overrideBoolPreferenceForTestRunner and do not correspond to > a WebCore settings ; I wonder if we could extend Internal.idl to expose them: > WebKitTabToLinksPreferenceKey > WebKit2AsynchronousPluginInitializationEnabled > WebKit2AsynchronousPluginInitializationEnabledForAllPlugins > > This maps to some setters on Settings.h but not to a settings in Setting.in > ; I wonder if we could extend Internal.idl to expose them: > WebKitDisplayImagesKey => LoadsImagesAutomatically > WebKitJavaScriptEnabled => ScriptEnabled > WebKitUsesPageCachePreferenceKey => UsesPageCache Some of these need to be set before the test starts loading, presumably (you can't run script to disable JS in a sensible way). Our technique for such behaviors is test headers, see updateTestOptionsFromTestHeader() in WTR.

(In reply to Frédéric Wang (:fredw) from comment #26) > WebKitHiddenPageDOMTimerThrottlingEnabled is used in > ./fast/dom/timer-throttling-hidden-page-non-nested.html and > ./fast/dom/timer-throttling-hidden-page.html ; for some reason it is > separated from the rest of the boolean settings in > InjectedBundle::overrideBoolPreferenceForTestRunner, but it looks like we > could do the same replacements as done here. OK, I stand corrected. WebKitHiddenPageDOMTimerThrottlingEnabled is also in the category "This maps to some setters on Settings.h but not to a settings in Setting.in".