Restore original value of mock scrollbars enabled in InternalSettings
Created attachment 144404 [details] Patch
In Qt port this setting was leaking causing other tests to FAIL because sizes of the elements were wrong. Some ports do reset this setting in their DRTs, but Qt didn't. In general, I think that InternalSettings should properly restore all its properties to their original values.
One example of problem was to run: Tools/Scripts/run-webkit-tests fast/dom/window-scroll-scaling.html fast/dynamic/008.html 008.html fails without the patch, but when run-webkit-tests retry it, it passes, marking it as flaky.
Created attachment 144405 [details] Patch
Fixing the export files for GTK and Mac. I'll upload a new patch soon.
Fixing the export files for GTK and Win. I'll upload a new patch soon.
Comment on attachment 144405 [details] Patch Attachment 144405 [details] did not pass win-ews (win): Output: http://queues.webkit.org/results/12850263
Created attachment 144408 [details] Patch
Comment on attachment 144408 [details] Patch Attachment 144408 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/12846330 New failing tests: css1/font_properties/font_size.html css1/box_properties/border_left_width.html css1/basic/class_as_selector.html css1/font_properties/font_family.html css1/box_properties/border_bottom.html css1/box_properties/border_bottom_width.html css1/box_properties/border_right_width.html css1/basic/id_as_selector.html css1/basic/inheritance.html css1/box_properties/border.html css1/color_and_background/background_position.html css1/box_properties/border_right_inline.html css1/box_properties/border_left.html css1/cascade/cascade_order.html css1/color_and_background/background.html css1/conformance/forward_compatible_parsing.html css1/box_properties/border_top.html css1/box_properties/border_style.html css1/formatting_model/height_of_lines.html css1/basic/containment.html css1/color_and_background/background_attachment.html css1/basic/comments.html css1/classification/white_space.html css1/color_and_background/background_repeat.html css1/font_properties/font.html css1/formatting_model/floating_elements.html css1/classification/display.html css1/classification/list_style_type.html
Created attachment 144424 [details] Archive of layout-test-results from ec2-cr-linux-01 The attached test failures were seen while running run-webkit-tests on the chromium-ews. Bot: ec2-cr-linux-01 Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'> Platform: Linux-2.6.35-28-virtual-x86_64-with-Ubuntu-10.10-maverick
*** Bug 87700 has been marked as a duplicate of this bug. ***
Created attachment 144503 [details] Patch
Comment on attachment 144503 [details] Patch Getting some feedback from Chromium EWS while I try to make sure I can test this locally.
(In reply to comment #13) > (From update of attachment 144503 [details]) > Getting some feedback from Chromium EWS while I try to make sure I can test this locally. this = chromium failures
Comment on attachment 144503 [details] Patch Attachment 144503 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/12843564 New failing tests: css1/font_properties/font_size.html css1/box_properties/border_left_width.html css1/basic/class_as_selector.html css1/font_properties/font_family.html css1/box_properties/border_bottom.html css1/box_properties/border_bottom_width.html css1/box_properties/border_right_width.html css1/basic/id_as_selector.html css1/basic/inheritance.html css1/box_properties/border.html css1/color_and_background/background_position.html css1/box_properties/border_right_inline.html css1/box_properties/border_left.html css1/cascade/cascade_order.html css1/color_and_background/background.html css1/conformance/forward_compatible_parsing.html css1/box_properties/border_top.html css1/box_properties/border_style.html css1/formatting_model/height_of_lines.html css1/basic/containment.html css1/color_and_background/background_attachment.html css1/basic/comments.html css1/classification/white_space.html css1/color_and_background/background_repeat.html css1/font_properties/font.html css1/formatting_model/floating_elements.html css1/classification/display.html css1/classification/list_style_type.html
Created attachment 144573 [details] Archive of layout-test-results from ec2-cr-linux-04 The attached test failures were seen while running run-webkit-tests on the chromium-ews. Bot: ec2-cr-linux-04 Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'> Platform: Linux-2.6.35-28-virtual-x86_64-with-Ubuntu-10.10-maverick
I couldn't reproduce the problems locally. In my pixel tests I get the scrollbar images just fine. Chromium port developers, any idea of what could be going on?
*** Bug 90091 has been marked as a duplicate of this bug. ***
(In reply to comment #18) > *** Bug 90091 has been marked as a duplicate of this bug. *** One more problem because of this bug. ^^^^^^^^^^ I had to skip a test to hide this bug (and make testing work on Qt port ... ) http://trac.webkit.org/changeset/121414
Here is a solution for Qt port only: https://bugs.webkit.org/show_bug.cgi?id=90155 We can revert that out once this general fix lands.
Created attachment 161943 [details] Patch
Is there non-Qt code that should change in this patch as well?
(In reply to comment #22) > Is there non-Qt code that should change in this patch as well? It seems to me DRT implementations of other ports that are resetting this shouldn't need anymore. Similar change than what was done for Qt DRT.
Perhaps we should change them all at the same time.
(In reply to comment #24) > Perhaps we should change them all at the same time. I'll try.
(In reply to comment #24) > Perhaps we should change them all at the same time. Taking a closer look, I would rather have this patch as it is and fill bugs for the other ports to follow. I prefer not to mess around with ports I can't perfectly understand, build or test (mac for instance). Since the cr-linux build machine is green, and it run the tests, it's safe that the patch will not interfere with the ones still using mock scrollbars in their DRTs.
Created attachment 163173 [details] Patch
Comment on attachment 163173 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=163173&action=review > Tools/DumpRenderTree/chromium/TestShell.cpp:-252 > - m_prefs.mockScrollbarsEnabled = true; Why did you remove this line? It seems that will stop compositing/ tests from having mock scrollbars.
(In reply to comment #28) > (From update of attachment 163173 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=163173&action=review > > > Tools/DumpRenderTree/chromium/TestShell.cpp:-252 > > - m_prefs.mockScrollbarsEnabled = true; > > Why did you remove this line? It seems that will stop compositing/ tests from having mock scrollbars. The initial state of the mock scroll bars should be set by the tests via javascript. For example, some tests include LayoutTests/compositing/resources/mock_scrollbars.js that sets the mock scroll bars like this: window.internals.settings.setMockScrollbarsEnabled(true);
Do all the compositing tests do that or does this patch change the behavior?
(In reply to comment #30) > Do all the compositing tests do that or does this patch change the behavior? The compositing tests that deal with geometry include the javascript that set the initial value of the mock scroll bars. It's reasonable to expect that the chromium DRT settings gives the same results as the one in the mentioned javascript, since all ports must have similar results in the tests. Besides, the cr-linux buildbot will run the chromium tests and we will see.
Comment on attachment 163173 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=163173&action=review >>> Tools/DumpRenderTree/chromium/TestShell.cpp:-252 >>> - m_prefs.mockScrollbarsEnabled = true; >> >> Why did you remove this line? It seems that will stop compositing/ tests from having mock scrollbars. > > The initial state of the mock scroll bars should be set by the tests via javascript. > For example, some tests include LayoutTests/compositing/resources/mock_scrollbars.js > that sets the mock scroll bars like this: > > window.internals.settings.setMockScrollbarsEnabled(true); No, that's incorrect. We want to explicitly override the mock scrollbar setting for this directory without setting the value in javascript. If you want to patch this please preserve this behavior for chromium DRT.
(In reply to comment #32) > (From update of attachment 163173 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=163173&action=review > > >>> Tools/DumpRenderTree/chromium/TestShell.cpp:-252 > >>> - m_prefs.mockScrollbarsEnabled = true; > >> > >> Why did you remove this line? It seems that will stop compositing/ tests from having mock scrollbars. > > > > The initial state of the mock scroll bars should be set by the tests via javascript. > > For example, some tests include LayoutTests/compositing/resources/mock_scrollbars.js > > that sets the mock scroll bars like this: > > > > window.internals.settings.setMockScrollbarsEnabled(true); > > No, that's incorrect. We want to explicitly override the mock scrollbar setting for this directory without setting the value in javascript. If you want to patch this please preserve this behavior for chromium DRT. Ok, I'll remove the changes to chromium DRT.
Created attachment 163366 [details] Patch
Comment on attachment 163366 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=163366&action=review > Tools/DumpRenderTree/efl/DumpRenderTreeChrome.cpp:-194 > - DumpRenderTreeSupportEfl::setMockScrollbarsEnabled(true); Everything in this patch looks fine except this line. It seems EFL will no longer use mock scrollbars everywhere after this patch. Perhaps we should leave this line unchanged?
Created attachment 163393 [details] Patch
Comment on attachment 163393 [details] Patch Thanks!
Comment on attachment 163393 [details] Patch Clearing flags on attachment: 163393 Committed r128210: <http://trac.webkit.org/changeset/128210>
All reviewed patches have been landed. Closing bug.