Bug 87680 – Restore original value of mock scrollbars enabled in InternalSettings

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

[Caio Marcelo de Oliveira Filho]

Restore original value of mock scrollbars enabled in InternalSettings

[Caio Marcelo de Oliveira Filho]

Created attachment 144404 [details]
Patch

[Caio Marcelo de Oliveira Filho]

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.

[Caio Marcelo de Oliveira Filho]

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.

[Caio Marcelo de Oliveira Filho]

Created attachment 144405 [details]
Patch

[Caio Marcelo de Oliveira Filho]

Fixing the export files for GTK and Mac. I'll upload a new patch soon.

[Caio Marcelo de Oliveira Filho]

Fixing the export files for GTK and Win. I'll upload a new patch soon.

[Build Bot]

Comment on attachment 144405 [details]
Patch

Attachment 144405 [details] did not pass win-ews (win):
Output: http://queues.webkit.org/results/12850263

[Caio Marcelo de Oliveira Filho]

Created attachment 144408 [details]
Patch

[WebKit Review Bot]

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

[WebKit Review Bot]

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

[Csaba Osztrogonác]

*** Bug 87700 has been marked as a duplicate of this bug. ***

[Caio Marcelo de Oliveira Filho]

Created attachment 144503 [details]
Patch

[Caio Marcelo de Oliveira Filho]

Comment on attachment 144503 [details]
Patch

Getting some feedback from Chromium EWS while I try to make sure I can test this locally.

[Caio Marcelo de Oliveira Filho]

(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

[WebKit Review Bot]

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

[WebKit Review Bot]

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

[Caio Marcelo de Oliveira Filho]

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?

[Csaba Osztrogonác]

*** Bug 90091 has been marked as a duplicate of this bug. ***

[Csaba Osztrogonác]

(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

[Csaba Osztrogonác]

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.

[Marcelo Lira]

Created attachment 161943 [details]
Patch

[Adam Barth]

Is there non-Qt code that should change in this patch as well?

[Caio Marcelo de Oliveira Filho]

(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.

[Adam Barth]

Perhaps we should change them all at the same time.

[Marcelo Lira]

(In reply to comment #24)
> Perhaps we should change them all at the same time.

I'll try.

[Marcelo Lira]

(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.

[Marcelo Lira]

Created attachment 163173 [details]
Patch

[Adam Barth]

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.

[Marcelo Lira]

(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);

[Adam Barth]

Do all the compositing tests do that or does this patch change the behavior?

[Marcelo Lira]

(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.

[James Robinson]

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.

[Marcelo Lira]

(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.

[Marcelo Lira]

Created attachment 163366 [details]
Patch

[Adam Barth]

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?

[Marcelo Lira]

Created attachment 163393 [details]
Patch

[Adam Barth]

Comment on attachment 163393 [details]
Patch

Thanks!

[WebKit Review Bot]

Comment on attachment 163393 [details]
Patch

Clearing flags on attachment: 163393

Committed r128210: <http://trac.webkit.org/changeset/128210>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.