New issue
Advanced search Search tips

Issue 775541 link

Starred by 1 user

Issue metadata

Status: Started
Owner:
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: ----
Pri: 3
Type: Bug

Blocking:
issue 661783



Sign in to add a comment

Native ScrollbarTheme should not be a static singleton

Project Member Reported by chaopeng@chromium.org, Oct 17 2017

Issue description

Currently native ScrollbarTheme is a singleton make scrollbars wrong in mobile emulator. 
 
Status: Started (was: Untriaged)
Project Member

Comment 2 by bugdroid1@chromium.org, Oct 20 2017

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

commit 882844e3e270328f473ba1573d7e45662b9fe3a0
Author: chaopeng <chaopeng@chromium.org>
Date: Fri Oct 20 03:11:43 2017

[Refactor] Move ScrollbarTheme::GetTheme calls to Page::GetScrollbarTheme.

This patch moves ScrollbarTheme::GetTheme calls to Page::GetScrollbarTheme.
After this change, we allow change GetScrollbarTheme based on page settings
which we need to change scrollbar theme from native (Aura/Mac) to mobile for
Mobile Emulator toggle.

We still have some ScrollbarTheme::GetTheme calls:
- LayoutScrollbarTheme::*
- WebScrollbarThemeClientImpl::(~)WebScrollbarThemeClientImpl
- WebScrollbarTheme::UpdateScrollbarsWithNSDefaults

We still have some ScrollbarTheme::GetTheme calls in tests:
- MockScrollableArea::GetPageScrollbarTheme (in ScrollbarTestSuite.h)

Bug: 775541
Cq-Include-Trybots: master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2
Change-Id: Iad26031e9582a5dfc3ef232439c03471ec7f7ad4
Reviewed-on: https://chromium-review.googlesource.com/661325
Commit-Queue: Jianpeng Chao <chaopeng@chromium.org>
Reviewed-by: Philip Rogers <pdr@chromium.org>
Reviewed-by: David Bokan <bokan@chromium.org>
Cr-Commit-Position: refs/heads/master@{#510314}
[modify] https://crrev.com/882844e3e270328f473ba1573d7e45662b9fe3a0/third_party/WebKit/Source/core/editing/commands/EditorCommand.cpp
[modify] https://crrev.com/882844e3e270328f473ba1573d7e45662b9fe3a0/third_party/WebKit/Source/core/frame/FrameTestHelpers.h
[modify] https://crrev.com/882844e3e270328f473ba1573d7e45662b9fe3a0/third_party/WebKit/Source/core/frame/LocalDOMWindow.cpp
[modify] https://crrev.com/882844e3e270328f473ba1573d7e45662b9fe3a0/third_party/WebKit/Source/core/frame/LocalFrameView.cpp
[modify] https://crrev.com/882844e3e270328f473ba1573d7e45662b9fe3a0/third_party/WebKit/Source/core/frame/LocalFrameView.h
[modify] https://crrev.com/882844e3e270328f473ba1573d7e45662b9fe3a0/third_party/WebKit/Source/core/frame/RootFrameViewport.cpp
[modify] https://crrev.com/882844e3e270328f473ba1573d7e45662b9fe3a0/third_party/WebKit/Source/core/frame/RootFrameViewport.h
[modify] https://crrev.com/882844e3e270328f473ba1573d7e45662b9fe3a0/third_party/WebKit/Source/core/frame/RootFrameViewportTest.cpp
[modify] https://crrev.com/882844e3e270328f473ba1573d7e45662b9fe3a0/third_party/WebKit/Source/core/frame/VisualViewport.cpp
[modify] https://crrev.com/882844e3e270328f473ba1573d7e45662b9fe3a0/third_party/WebKit/Source/core/frame/VisualViewport.h
[modify] https://crrev.com/882844e3e270328f473ba1573d7e45662b9fe3a0/third_party/WebKit/Source/core/html/forms/SpinButtonElement.cpp
[modify] https://crrev.com/882844e3e270328f473ba1573d7e45662b9fe3a0/third_party/WebKit/Source/core/layout/LayoutScrollbarPart.cpp
[modify] https://crrev.com/882844e3e270328f473ba1573d7e45662b9fe3a0/third_party/WebKit/Source/core/layout/LayoutScrollbarPart.h
[modify] https://crrev.com/882844e3e270328f473ba1573d7e45662b9fe3a0/third_party/WebKit/Source/core/layout/LayoutScrollbarTheme.cpp
[modify] https://crrev.com/882844e3e270328f473ba1573d7e45662b9fe3a0/third_party/WebKit/Source/core/layout/LayoutScrollbarTheme.h
[modify] https://crrev.com/882844e3e270328f473ba1573d7e45662b9fe3a0/third_party/WebKit/Source/core/layout/LayoutTestHelper.cpp
[modify] https://crrev.com/882844e3e270328f473ba1573d7e45662b9fe3a0/third_party/WebKit/Source/core/layout/LayoutTextControl.cpp
[modify] https://crrev.com/882844e3e270328f473ba1573d7e45662b9fe3a0/third_party/WebKit/Source/core/layout/ScrollbarsTest.cpp
[modify] https://crrev.com/882844e3e270328f473ba1573d7e45662b9fe3a0/third_party/WebKit/Source/core/page/Page.cpp
[modify] https://crrev.com/882844e3e270328f473ba1573d7e45662b9fe3a0/third_party/WebKit/Source/core/page/Page.h
[modify] https://crrev.com/882844e3e270328f473ba1573d7e45662b9fe3a0/third_party/WebKit/Source/core/paint/PaintLayerScrollableArea.cpp
[modify] https://crrev.com/882844e3e270328f473ba1573d7e45662b9fe3a0/third_party/WebKit/Source/core/paint/PaintLayerScrollableArea.h
[modify] https://crrev.com/882844e3e270328f473ba1573d7e45662b9fe3a0/third_party/WebKit/Source/core/testing/Internals.cpp
[modify] https://crrev.com/882844e3e270328f473ba1573d7e45662b9fe3a0/third_party/WebKit/Source/platform/exported/WebScrollbarThemeClientImpl.cpp
[modify] https://crrev.com/882844e3e270328f473ba1573d7e45662b9fe3a0/third_party/WebKit/Source/platform/graphics/test/FakeScrollableArea.h
[modify] https://crrev.com/882844e3e270328f473ba1573d7e45662b9fe3a0/third_party/WebKit/Source/platform/mac/ScrollAnimatorMac.mm
[modify] https://crrev.com/882844e3e270328f473ba1573d7e45662b9fe3a0/third_party/WebKit/Source/platform/scroll/ScrollAnimatorTest.cpp
[modify] https://crrev.com/882844e3e270328f473ba1573d7e45662b9fe3a0/third_party/WebKit/Source/platform/scroll/ScrollableArea.cpp
[modify] https://crrev.com/882844e3e270328f473ba1573d7e45662b9fe3a0/third_party/WebKit/Source/platform/scroll/ScrollableArea.h
[modify] https://crrev.com/882844e3e270328f473ba1573d7e45662b9fe3a0/third_party/WebKit/Source/platform/scroll/ScrollableAreaTest.cpp
[modify] https://crrev.com/882844e3e270328f473ba1573d7e45662b9fe3a0/third_party/WebKit/Source/platform/scroll/Scrollbar.cpp
[modify] https://crrev.com/882844e3e270328f473ba1573d7e45662b9fe3a0/third_party/WebKit/Source/platform/scroll/ScrollbarTestSuite.h
[modify] https://crrev.com/882844e3e270328f473ba1573d7e45662b9fe3a0/third_party/WebKit/Source/platform/scroll/ScrollbarTheme.cpp
[modify] https://crrev.com/882844e3e270328f473ba1573d7e45662b9fe3a0/third_party/WebKit/Source/platform/scroll/ScrollbarTheme.h
[modify] https://crrev.com/882844e3e270328f473ba1573d7e45662b9fe3a0/third_party/WebKit/Source/platform/scroll/WebScrollbarTheme.mm

Comment 3 by bokan@chromium.org, Oct 20 2017

Blocking: 740879
Cc: bokan@chromium.org
Labels: Hotlist-Input-Dev
Status: Fixed (was: Started)

Comment 5 by bokan@chromium.org, Oct 20 2017

Status: Started (was: Fixed)
I don't think this is done. ScrollbarTheme is still static and ScrollbarTheme::GetTheme is still used in some places. I would only call this done when Page itself is creating the Theme and we don't have any cases of static themes.
Blocking: -740879
Summary: Native ScrollbarTheme should not be a static singleton (was: Native ScrollbarTheme should determine by page (settings) to support devtools mobile emulator)

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

Blocking: 661783

Sign in to add a comment