New issue
Advanced search Search tips

Issue 661783 link

Starred by 2 users
Status: Fixed
Owner: ----
Closed: Dec 6
Components:
EstimatedDays: ----
NextAction: ----
OS: All
Pri: 3
Type: Bug

Blocked on:
issue 775541


Sign in to add a comment

Remove use of setOverlayScrollbarsEnabled in layout tests

Project Member Reported by bokan@chromium.org, Nov 2 2016 
Setting this flag mid test doesn't change the ScrollbarTheme or the existing scrollbars. That means most of these tests aren't running with overlays but will take overlay branches that depend on REF::overlayScrollbarsEnabled. There's already a FIXME in InternalSettings.h:

  // FIXME: The following are RuntimeEnabledFeatures and likely
  // cannot be changed after process start. These setters should
  // be removed or moved onto internals.runtimeFlags:

We should remove this flag and rewrite the tests as unit tests which can better control REFs or add a virtual test suite for overlay scrollbars.
Project Member

Comment 1 by sheriffbot@chromium.org, Nov 3 2017

Labels: Hotlist-Recharge-Cold
Status: Untriaged (was: Available)
This issue has been Available for over a year. If it's no longer important or seems unlikely to be fixed, please consider closing it out. If it is important, please re-triage the issue.

Sorry for the inconvenience if the bug really should have been left as Available. If you change it back, also remove the "Hotlist-Recharge-Cold" label.

For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot

Comment 2 by bokan@chromium.org, Nov 3 2017

Status: Available (was: Untriaged)
This is still an issue

Comment 3 by bokan@chromium.org, Nov 3 2017

Components: Blink>Scroll

Comment 4 by suneel.k...@samsung.com, Nov 7 2017 

Hi, i observed that setOverlayScrollbarsEnabled() is used in the initial stages of the tests, i found one test "fast/dom/root-client-size.html", where  internals.settings.setOverlayScrollbarsEnabled(true);

    runClientSizeTest("wide", false, false, 1);
    runClientSizeTest("tall", false, false, 1);

    internals.settings.setOverlayScrollbarsEnabled(false);

    runClientSizeTest("wide", false, true, 0);
    runClientSizeTest("tall", true, false, 0);

    internals.settings.setOverlayScrollbarsEnabled(originalOverlayScrollbars);

I think the state of OverlayScrollsbards enabled changes twice here and the test passes normally,

so what is the need to remove setOverlayScrollbarsEnabled() and replace with internals.runtimeFlags.overlayScrollbarsEnabled?

the test works fine with below modifications to the above mentioned test 
//internals.settings.setOverlayScrollbarsEnabled(true);
    internals.runtimeFlags.overlayScrollbarsEnabled = true;

    runClientSizeTest("wide", false, false, 1);
    runClientSizeTest("tall", false, false, 1);

    //internals.settings.setOverlayScrollbarsEnabled(false);
    internals.runtimeFlags.overlayScrollbarsEnabled = false;

    runClientSizeTest("wide", false, true, 0);
    runClientSizeTest("tall", true, false, 0);

    //internals.settings.setOverlayScrollbarsEnabled(originalOverlayScrollbars);
    internals.runtimeFlags.overlayScrollbarsEnabled = originalOverlayScrollbars;

Comment 5 by bokan@chromium.org, Nov 15 2017 

The test works but the page is in a really unrealistic state - the scrollbars are still likely non-overlay (since we don't recreate them) and the code will take some branches as if they were overlay and others as if they were. This is brittle and likely to break long term. Adding runtimeFlags.overlayScrollbarsEnabled would help to make more of these branches take the overlay path but we should also cause the page to recreate the scrollbars.

Comment 7 by bokan@chromium.org, Nov 29 2017

Blockedon: 775541
Rather than rewriting the tests, I think finishing bug 775541 so that we can swap ScrollbarTheme on the fly is a better way forward on this.
Project Member

Comment 8 by bugdroid1@chromium.org, Dec 4 2017 

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/fb1b81a2f5dede8186631bba7ef10a7419f78049

commit fb1b81a2f5dede8186631bba7ef10a7419f78049
Author: Suneel Kota <suneel.kota@samsung.com>
Date: Mon Dec 04 08:12:31 2017

Remove use of setOverlayScrollbarsEnabled in layout tests

Adding runtimeFlags.overlayScrollbarsEnabled would help to
make more of these branches take the overlay path

Bug:  661783 
Change-Id: I1d5523922fe3d6c8f671edd1c54789ef9aae399d
Reviewed-on: https://chromium-review.googlesource.com/792833
Reviewed-by: David Bokan <bokan@chromium.org>
Commit-Queue: srirama chandra sekhar <srirama.m@samsung.com>
Cr-Commit-Position: refs/heads/master@{#521289}
[modify] https://crrev.com/fb1b81a2f5dede8186631bba7ef10a7419f78049/third_party/WebKit/LayoutTests/compositing/overflow/non-reparented-overlay-scrollbars-expected.html
[modify] https://crrev.com/fb1b81a2f5dede8186631bba7ef10a7419f78049/third_party/WebKit/LayoutTests/compositing/overflow/non-reparented-overlay-scrollbars.html
[modify] https://crrev.com/fb1b81a2f5dede8186631bba7ef10a7419f78049/third_party/WebKit/LayoutTests/compositing/overflow/reflected-overlay-scrollbars-should-appear-without-compositing-expected.html
[modify] https://crrev.com/fb1b81a2f5dede8186631bba7ef10a7419f78049/third_party/WebKit/LayoutTests/compositing/overflow/reflected-overlay-scrollbars-should-appear-without-compositing.html
[modify] https://crrev.com/fb1b81a2f5dede8186631bba7ef10a7419f78049/third_party/WebKit/LayoutTests/compositing/overflow/reflected-overlay-scrollbars-should-respect-ancestor-clip-expected.html
[modify] https://crrev.com/fb1b81a2f5dede8186631bba7ef10a7419f78049/third_party/WebKit/LayoutTests/compositing/overflow/reflected-overlay-scrollbars-should-respect-ancestor-clip.html
[modify] https://crrev.com/fb1b81a2f5dede8186631bba7ef10a7419f78049/third_party/WebKit/LayoutTests/compositing/overflow/reparented-overlay-scrollbars-should-respect-ancestor-clip-expected.html
[modify] https://crrev.com/fb1b81a2f5dede8186631bba7ef10a7419f78049/third_party/WebKit/LayoutTests/compositing/overflow/reparented-overlay-scrollbars-should-respect-ancestor-clip.html
[modify] https://crrev.com/fb1b81a2f5dede8186631bba7ef10a7419f78049/third_party/WebKit/LayoutTests/compositing/overflow/reparented-scrollbars-non-sc-anc.html
[modify] https://crrev.com/fb1b81a2f5dede8186631bba7ef10a7419f78049/third_party/WebKit/LayoutTests/compositing/overflow/reparented-unclipped-overlay-scrollbars-with-offset-from-renderer-expected.html
[modify] https://crrev.com/fb1b81a2f5dede8186631bba7ef10a7419f78049/third_party/WebKit/LayoutTests/compositing/overflow/reparented-unclipped-overlay-scrollbars-with-offset-from-renderer.html
[modify] https://crrev.com/fb1b81a2f5dede8186631bba7ef10a7419f78049/third_party/WebKit/LayoutTests/compositing/overflow/scroll-parent-with-non-stacking-context-composited-ancestor.html
[modify] https://crrev.com/fb1b81a2f5dede8186631bba7ef10a7419f78049/third_party/WebKit/LayoutTests/compositing/overflow/scrollbar-layer-placement.html
[modify] https://crrev.com/fb1b81a2f5dede8186631bba7ef10a7419f78049/third_party/WebKit/LayoutTests/compositing/scrollbars/nested-overlay-scrollbars.html
[modify] https://crrev.com/fb1b81a2f5dede8186631bba7ef10a7419f78049/third_party/WebKit/LayoutTests/compositing/squashing/squashing-inside-perspective-with-reparented-scrolling-expected.html
[modify] https://crrev.com/fb1b81a2f5dede8186631bba7ef10a7419f78049/third_party/WebKit/LayoutTests/compositing/squashing/squashing-inside-perspective-with-reparented-scrolling.html
[modify] https://crrev.com/fb1b81a2f5dede8186631bba7ef10a7419f78049/third_party/WebKit/LayoutTests/css3/viewport-percentage-lengths/viewport-percentage-lengths-scaled-overlay-scrollbars.html
[modify] https://crrev.com/fb1b81a2f5dede8186631bba7ef10a7419f78049/third_party/WebKit/LayoutTests/fast/block/float/float-change-composited-scrolling.html
[modify] https://crrev.com/fb1b81a2f5dede8186631bba7ef10a7419f78049/third_party/WebKit/LayoutTests/fast/dom/partial-layout-block.html
[modify] https://crrev.com/fb1b81a2f5dede8186631bba7ef10a7419f78049/third_party/WebKit/LayoutTests/fast/dom/partial-layout-non-overlay-scrollbars.html
[modify] https://crrev.com/fb1b81a2f5dede8186631bba7ef10a7419f78049/third_party/WebKit/LayoutTests/fast/dom/partial-layout-overlay-scrollbars.html
[modify] https://crrev.com/fb1b81a2f5dede8186631bba7ef10a7419f78049/third_party/WebKit/LayoutTests/fast/dom/root-client-size-iframe.html
[modify] https://crrev.com/fb1b81a2f5dede8186631bba7ef10a7419f78049/third_party/WebKit/LayoutTests/fast/dom/root-client-size.html
[modify] https://crrev.com/fb1b81a2f5dede8186631bba7ef10a7419f78049/third_party/WebKit/LayoutTests/fast/events/hit-test-cache-scrollbar-no-crash.html
[modify] https://crrev.com/fb1b81a2f5dede8186631bba7ef10a7419f78049/third_party/WebKit/LayoutTests/fast/forms/select/listbox-overlay-scrollbar.html
[modify] https://crrev.com/fb1b81a2f5dede8186631bba7ef10a7419f78049/third_party/WebKit/LayoutTests/fast/scrolling/overlay-scrollbars.html
[modify] https://crrev.com/fb1b81a2f5dede8186631bba7ef10a7419f78049/third_party/WebKit/LayoutTests/fast/scrolling/scrollbar-tickmarks-hittest.html
[modify] https://crrev.com/fb1b81a2f5dede8186631bba7ef10a7419f78049/third_party/WebKit/LayoutTests/html/sections/body-quirk-client-size.html
[modify] https://crrev.com/fb1b81a2f5dede8186631bba7ef10a7419f78049/third_party/WebKit/LayoutTests/paint/invalidation/scroll/destroy-overlay-scrollbar.html
[modify] https://crrev.com/fb1b81a2f5dede8186631bba7ef10a7419f78049/third_party/WebKit/LayoutTests/plugins/overlay-scrollbar-mouse-capture.html
[modify] https://crrev.com/fb1b81a2f5dede8186631bba7ef10a7419f78049/third_party/WebKit/LayoutTests/scrollbars/auto-scrollbar-fades-out.html
[modify] https://crrev.com/fb1b81a2f5dede8186631bba7ef10a7419f78049/third_party/WebKit/LayoutTests/scrollbars/hidden-scrollbars-invisible.html
[modify] https://crrev.com/fb1b81a2f5dede8186631bba7ef10a7419f78049/third_party/WebKit/LayoutTests/scrollbars/overlay-scrollbars-within-overflow-scroll.html
[modify] https://crrev.com/fb1b81a2f5dede8186631bba7ef10a7419f78049/third_party/WebKit/Source/core/testing/InternalSettings.cpp
[modify] https://crrev.com/fb1b81a2f5dede8186631bba7ef10a7419f78049/third_party/WebKit/Source/core/testing/InternalSettings.h
[modify] https://crrev.com/fb1b81a2f5dede8186631bba7ef10a7419f78049/third_party/WebKit/Source/core/testing/InternalSettings.idl
Project Member

Comment 9 by sheriffbot@chromium.org, Dec 4

Status: Untriaged (was: Available)
This issue has been Available for over a year. If it's no longer important or seems unlikely to be fixed, please consider closing it out. If it is important, please re-triage the issue.

Sorry for the inconvenience if the bug really should have been left as Available.

For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot

Comment 10 by nzolghadr@chromium.org, Dec 6

Status: Fixed (was: Untriaged)
I'm closing this as the usage has been removed. We keep the other bug for dynamic switching of scrollbar themes open until that is addressed.
►

Sign in to add a comment