Reland Change minimum length of Aura overlay scrollbars.

Reland Change minimum length of Aura overlay scrollbars.

It turns out Aura (and Android) overlay scrollbars weren't using the minimum
defined in NativeThemeAura. Instead, the minimum was set from
ScrollbarTheme::minimumThumbLength which just returns the thumb thickness.

After adding an override for ScrollbarThemeOverlay which correctly uses the
value form NativeThemeAura, the only consumers of
ScrollbarTheme::minimumThumbLength are the mock classes so I moved the base
implementation into the Mock class and made the base pure virtual.

BUG=678595
Review-Url: https://codereview.chromium.org/2734483002
Cr-Original-Commit-Position: refs/heads/master@{#460199}
Committed: https://chromium.googlesource.com/chromium/src/+/151a2db45f5fdbca8973988dd51d2bf6c86fbc8c
Review-Url: https://codereview.chromium.org/2734483002
Cr-Commit-Position: refs/heads/master@{#460367}
Committed: https://chromium.googlesource.com/chromium/src/+/bca2d029eea47a55324d505a88329cd4112de03d

[bokan, 2017-03-03 18:42:31]

The CQ bit was checked by bokan@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-03-03 18:42:49]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-03-03 20:02:35]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-03-03 20:02:36]

Dry run: Try jobs failed on following builders:
  linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[bokan, 2017-03-23 15:49:34]

The CQ bit was checked by bokan@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-03-23 15:50:02]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[bokan, 2017-03-23 16:45:08]

Description was changed from

==========
Change minimum length of Aura overlay scrollbars.

It turns out Aura overlay scrollbars weren't using the minimum defined in
NativeThemeAura. Instead, the minimum was set from
ScrollbarTheme::minimumThumbLength which just returns the thumb thickness.

After adding an override for ScrollbarThemeOverlay which correctly uses the
value form NativeThemeAura, the only user of ScrollbarTheme::minimumThumbLength
is ScrollbarThemeMock so I moved the base implementation into the Mock class
and made the base pure virtual.

BUG=678595
==========

to

==========
Change minimum length of Aura overlay scrollbars.

It turns out Aura overlay scrollbars weren't using the minimum defined in
NativeThemeAura. Instead, the minimum was set from
ScrollbarTheme::minimumThumbLength which just returns the thumb thickness.

After adding an override for ScrollbarThemeOverlay which correctly uses the
value form NativeThemeAura, the only user of ScrollbarTheme::minimumThumbLength
is ScrollbarThemeMock so I moved the base implementation into the Mock class
and made the base pure virtual.

BUG=678595
==========

[bokan, 2017-03-23 16:45:08]

bokan@chromium.org changed reviewers:
+ estade@chromium.org

[bokan, 2017-03-23 16:46:15]

Description was changed from

==========
Change minimum length of Aura overlay scrollbars.

It turns out Aura overlay scrollbars weren't using the minimum defined in
NativeThemeAura. Instead, the minimum was set from
ScrollbarTheme::minimumThumbLength which just returns the thumb thickness.

After adding an override for ScrollbarThemeOverlay which correctly uses the
value form NativeThemeAura, the only user of ScrollbarTheme::minimumThumbLength
is ScrollbarThemeMock so I moved the base implementation into the Mock class
and made the base pure virtual.

BUG=678595
==========

to

==========
Change minimum length of Aura overlay scrollbars.

It turns out Aura overlay scrollbars weren't using the minimum defined in
NativeThemeAura. Instead, the minimum was set from
ScrollbarTheme::minimumThumbLength which just returns the thumb thickness.

After adding an override for ScrollbarThemeOverlay which correctly uses the
value form NativeThemeAura, the only consumers of
ScrollbarTheme::minimumThumbLength are the mock classes so I moved the base
implementation into the Mock class and made the base pure virtual.

BUG=678595
==========

[bokan, 2017-03-23 16:47:56]

Hey Evan,

There's no good way to test this from Blink as we basically want to test a
platform preference so it should really be tested at a higher level.

Are there existing browser tests or similar for Aura UI like this?

[Evan Stade, 2017-03-23 23:16:24]

On 2017/03/23 16:47:56, bokan wrote:
> Hey Evan,
> 
> There's no good way to test this from Blink as we basically want to test a
> platform preference so it should really be tested at a higher level.
> 
> Are there existing browser tests or similar for Aura UI like this?

I'm not sure how you'd test this sensibly at the native theme level either
(checking that the NativeThemeAura returns a certain part size wouldn't have
caught the bug that the scrollbar wasn't consulting the theme). You want to test
that overlay scrollbars respect the size given by the WebThemeEngine. Can you
inject a mock/testing WebThemeEngine?

https://codereview.chromium.org/2734483002/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/platform/scroll/ScrollbarThemeOverlay.cpp
(right):

https://codereview.chromium.org/2734483002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/scroll/ScrollbarThemeOverlay.cpp:276: IntSize
size = Platform::current()->themeEngine()->getSize(
nit: return
Platform::current()->themeEngine()->getSize(WebThemeEngine::PartScrollbarVerticalThumb).height()?

https://codereview.chromium.org/2734483002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/scroll/ScrollbarThemeOverlay.cpp:283: return
size.width();
ditto

[bokan, 2017-03-24 20:19:52]

The CQ bit was checked by bokan@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-03-24 20:20:46]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[bokan, 2017-03-24 20:30:27]

I guess my question was more along the lines of the same one I just asked you in
chaopeng@'s CL. In any case, I added a test to make sure the ScrollbarThemes are
using the minimum defined in WebThemeEngine, is that enough?

https://codereview.chromium.org/2734483002/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/platform/scroll/ScrollbarThemeOverlay.cpp
(right):

https://codereview.chromium.org/2734483002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/scroll/ScrollbarThemeOverlay.cpp:276: IntSize
size = Platform::current()->themeEngine()->getSize(
On 2017/03/23 23:16:24, Evan Stade wrote:
> nit: return
>
Platform::current()->themeEngine()->getSize(WebThemeEngine::PartScrollbarVerticalThumb).height()?

Done (in ScrollbarThemeAura too).

https://codereview.chromium.org/2734483002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/scroll/ScrollbarThemeOverlay.cpp:283: return
size.width();
On 2017/03/23 23:16:24, Evan Stade wrote:
> ditto

Done.

[bokan, 2017-03-24 22:17:12]

The CQ bit was checked by bokan@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-03-24 22:18:14]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-03-24 23:33:57]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-03-24 23:33:57]

Dry run: Try jobs failed on following builders:
  linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)

[bokan, 2017-03-27 15:58:47]

The CQ bit was checked by bokan@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-03-27 15:59:39]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-03-27 17:26:59]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-03-27 17:27:01]

Dry run: This issue passed the CQ dry run.

[bokan, 2017-03-27 18:52:17]

FYI: I had to add some implementation in Android's native_theme since Android
uses ScrollbarThemeOverlay to get scrollbar geometry which now reaches into
NativeTheme/WebThemeEngine.

[Evan Stade, 2017-03-28 00:11:36]

test and patch lgtm

https://codereview.chromium.org/2734483002/diff/140001/third_party/WebKit/Sou...
File third_party/WebKit/Source/web/tests/ScrollbarsTest.cpp (right):

https://codereview.chromium.org/2734483002/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/web/tests/ScrollbarsTest.cpp:94:
TEST_P(ScrollbarAppearanceTest, DISABLED_ThemeEngineDefinesMinimumThumbLength) {
nit: use MAYBE_TestName pattern? Or just wrap entire test in macro (is there
some reason to make sure this compiles on mac?)

[bokan, 2017-03-28 13:57:21]

bokan@chromium.org changed reviewers:
+ nasko@chromium.org, pdr@chromium.org

[bokan, 2017-03-28 13:58:07]

Description was changed from

==========
Change minimum length of Aura overlay scrollbars.

It turns out Aura overlay scrollbars weren't using the minimum defined in
NativeThemeAura. Instead, the minimum was set from
ScrollbarTheme::minimumThumbLength which just returns the thumb thickness.

After adding an override for ScrollbarThemeOverlay which correctly uses the
value form NativeThemeAura, the only consumers of
ScrollbarTheme::minimumThumbLength are the mock classes so I moved the base
implementation into the Mock class and made the base pure virtual.

BUG=678595
==========

to

==========
Change minimum length of Aura overlay scrollbars.

It turns out Aura (and Android) overlay scrollbars weren't using the minimum
defined in NativeThemeAura. Instead, the minimum was set from
ScrollbarTheme::minimumThumbLength which just returns the thumb thickness.

After adding an override for ScrollbarThemeOverlay which correctly uses the
value form NativeThemeAura, the only consumers of
ScrollbarTheme::minimumThumbLength are the mock classes so I moved the base
implementation into the Mock class and made the base pure virtual.

BUG=678595
==========

[bokan, 2017-03-28 13:58:37]

+pdr@ for Source/platform
+nasko@ for content/child

https://codereview.chromium.org/2734483002/diff/140001/third_party/WebKit/Sou...
File third_party/WebKit/Source/web/tests/ScrollbarsTest.cpp (right):

https://codereview.chromium.org/2734483002/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/web/tests/ScrollbarsTest.cpp:94:
TEST_P(ScrollbarAppearanceTest, DISABLED_ThemeEngineDefinesMinimumThumbLength) {
On 2017/03/28 00:11:36, Evan Stade wrote:
> nit: use MAYBE_TestName pattern? Or just wrap entire test in macro (is there
> some reason to make sure this compiles on mac?)

I'd tried the MAYBE() macro but it looks like TEST_P doesn't support macro
expansion the same way TEST_F does. I just wrapped the whole test.

[Evan Stade, 2017-03-28 15:33:30]

https://codereview.chromium.org/2734483002/diff/140001/third_party/WebKit/Sou...
File third_party/WebKit/Source/web/tests/ScrollbarsTest.cpp (right):

https://codereview.chromium.org/2734483002/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/web/tests/ScrollbarsTest.cpp:94:
TEST_P(ScrollbarAppearanceTest, DISABLED_ThemeEngineDefinesMinimumThumbLength) {
On 2017/03/28 13:58:37, bokan wrote:
> On 2017/03/28 00:11:36, Evan Stade wrote:
> > nit: use MAYBE_TestName pattern? Or just wrap entire test in macro (is there
> > some reason to make sure this compiles on mac?)
> 
> I'd tried the MAYBE() macro but it looks like TEST_P doesn't support macro
> expansion the same way TEST_F does. I just wrapped the whole test.

#if OS(MACOSX)
#define MAYBE_TestName DISABLED_TestName
#else
#define MAYBE_TestName TestName
#endif
TEST_P(TestClass, MAYBE_TestName)

doesn't work?

[bokan, 2017-03-28 15:48:12]

On 2017/03/28 15:33:30, Evan Stade wrote:
>
https://codereview.chromium.org/2734483002/diff/140001/third_party/WebKit/Sou...
> File third_party/WebKit/Source/web/tests/ScrollbarsTest.cpp (right):
> 
>
https://codereview.chromium.org/2734483002/diff/140001/third_party/WebKit/Sou...
> third_party/WebKit/Source/web/tests/ScrollbarsTest.cpp:94:
> TEST_P(ScrollbarAppearanceTest, DISABLED_ThemeEngineDefinesMinimumThumbLength)
{
> On 2017/03/28 13:58:37, bokan wrote:
> > On 2017/03/28 00:11:36, Evan Stade wrote:
> > > nit: use MAYBE_TestName pattern? Or just wrap entire test in macro (is
there
> > > some reason to make sure this compiles on mac?)
> > 
> > I'd tried the MAYBE() macro but it looks like TEST_P doesn't support macro
> > expansion the same way TEST_F does. I just wrapped the whole test.
> 
> #if OS(MACOSX)
> #define MAYBE_TestName DISABLED_TestName
> #else
> #define MAYBE_TestName TestName
> #endif
> TEST_P(TestClass, MAYBE_TestName)
> 
> doesn't work?

I was using a function-style macro for MAYBE(TestName). But I tried the above
version just now and that doesn't work either, the macro isn't replaced.

[pdr., 2017-03-28 17:45:45]

LGTM

[nasko, 2017-03-28 17:51:02]

content/ rubberstamp LGTM based on other reviewers.

[bokan, 2017-03-28 18:06:05]

The CQ bit was checked by bokan@chromium.org

[bokan, 2017-03-28 18:06:05]

The patchset sent to the CQ was uploaded after l-g-t-m from estade@chromium.org
Link to the patchset: https://codereview.chromium.org/2734483002/#ps160001
(title: "#if-out whole test body on Mac")

[commit-bot: I haz the power, 2017-03-28 18:06:23]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-03-28 20:15:07]

CQ is committing da patch.

Bot data: {"patchset_id": 160001, "attempt_start_ts": 1490724365475260,
"parent_rev": "47b93d128610246960c032a00b67ec2083b2a05b", "commit_rev":
"151a2db45f5fdbca8973988dd51d2bf6c86fbc8c"}

[commit-bot: I haz the power, 2017-03-28 20:15:55]

Description was changed from

==========
Change minimum length of Aura overlay scrollbars.

It turns out Aura (and Android) overlay scrollbars weren't using the minimum
defined in NativeThemeAura. Instead, the minimum was set from
ScrollbarTheme::minimumThumbLength which just returns the thumb thickness.

After adding an override for ScrollbarThemeOverlay which correctly uses the
value form NativeThemeAura, the only consumers of
ScrollbarTheme::minimumThumbLength are the mock classes so I moved the base
implementation into the Mock class and made the base pure virtual.

BUG=678595
==========

to

==========
Change minimum length of Aura overlay scrollbars.

It turns out Aura (and Android) overlay scrollbars weren't using the minimum
defined in NativeThemeAura. Instead, the minimum was set from
ScrollbarTheme::minimumThumbLength which just returns the thumb thickness.

After adding an override for ScrollbarThemeOverlay which correctly uses the
value form NativeThemeAura, the only consumers of
ScrollbarTheme::minimumThumbLength are the mock classes so I moved the base
implementation into the Mock class and made the base pure virtual.

BUG=678595

Review-Url: https://codereview.chromium.org/2734483002
Cr-Commit-Position: refs/heads/master@{#460199}
Committed:
https://chromium.googlesource.com/chromium/src/+/151a2db45f5fdbca8973988dd51d...
==========

[commit-bot: I haz the power, 2017-03-28 20:15:57]

Committed patchset #9 (id:160001) as
https://chromium.googlesource.com/chromium/src/+/151a2db45f5fdbca8973988dd51d...

[bokan, 2017-03-28 22:23:14]

A revert of this CL (patchset #9 id:160001) has been created in
https://codereview.chromium.org/2780923003/ by bokan@chromium.org.

The reason for reverting is: Unit test failing on Android:
https://uberchromegw.corp.google.com/i/chromium.webkit/builders/WebKit%20Andr....

[bokan, 2017-03-29 11:49:54]

Description was changed from

==========
Change minimum length of Aura overlay scrollbars.

It turns out Aura (and Android) overlay scrollbars weren't using the minimum
defined in NativeThemeAura. Instead, the minimum was set from
ScrollbarTheme::minimumThumbLength which just returns the thumb thickness.

After adding an override for ScrollbarThemeOverlay which correctly uses the
value form NativeThemeAura, the only consumers of
ScrollbarTheme::minimumThumbLength are the mock classes so I moved the base
implementation into the Mock class and made the base pure virtual.

BUG=678595

Review-Url: https://codereview.chromium.org/2734483002
Cr-Commit-Position: refs/heads/master@{#460199}
Committed:
https://chromium.googlesource.com/chromium/src/+/151a2db45f5fdbca8973988dd51d...
==========

to

==========
Change minimum length of Aura overlay scrollbars.

It turns out Aura (and Android) overlay scrollbars weren't using the minimum
defined in NativeThemeAura. Instead, the minimum was set from
ScrollbarTheme::minimumThumbLength which just returns the thumb thickness.

After adding an override for ScrollbarThemeOverlay which correctly uses the
value form NativeThemeAura, the only consumers of
ScrollbarTheme::minimumThumbLength are the mock classes so I moved the base
implementation into the Mock class and made the base pure virtual.

BUG=678595

Review-Url: https://codereview.chromium.org/2734483002
Cr-Commit-Position: refs/heads/master@{#460199}
Committed:
https://chromium.googlesource.com/chromium/src/+/151a2db45f5fdbca8973988dd51d...
==========

[bokan, 2017-03-29 11:52:09]

Description was changed from

==========
Change minimum length of Aura overlay scrollbars.

It turns out Aura (and Android) overlay scrollbars weren't using the minimum
defined in NativeThemeAura. Instead, the minimum was set from
ScrollbarTheme::minimumThumbLength which just returns the thumb thickness.

After adding an override for ScrollbarThemeOverlay which correctly uses the
value form NativeThemeAura, the only consumers of
ScrollbarTheme::minimumThumbLength are the mock classes so I moved the base
implementation into the Mock class and made the base pure virtual.

BUG=678595

Review-Url: https://codereview.chromium.org/2734483002
Cr-Commit-Position: refs/heads/master@{#460199}
Committed:
https://chromium.googlesource.com/chromium/src/+/151a2db45f5fdbca8973988dd51d...
==========

to

==========
Reland Change minimum length of Aura overlay scrollbars.

It turns out Aura (and Android) overlay scrollbars weren't using the minimum
defined in NativeThemeAura. Instead, the minimum was set from
ScrollbarTheme::minimumThumbLength which just returns the thumb thickness.

After adding an override for ScrollbarThemeOverlay which correctly uses the
value form NativeThemeAura, the only consumers of
ScrollbarTheme::minimumThumbLength are the mock classes so I moved the base
implementation into the Mock class and made the base pure virtual.

BUG=678595

Review-Url: https://codereview.chromium.org/2734483002
Cr-Commit-Position: refs/heads/master@{#460199}
Committed:
https://chromium.googlesource.com/chromium/src/+/151a2db45f5fdbca8973988dd51d...
==========

[bokan, 2017-03-29 11:52:17]

The CQ bit was checked by bokan@chromium.org

[bokan, 2017-03-29 11:52:17]

The patchset sent to the CQ was uploaded after l-g-t-m from pdr@chromium.org,
nasko@chromium.org, estade@chromium.org
Link to the patchset: https://codereview.chromium.org/2734483002/#ps180001
(title: "Fix breakage on Android")

[commit-bot: I haz the power, 2017-03-29 11:52:34]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-03-29 13:24:03]

CQ is committing da patch.

Bot data: {"patchset_id": 180001, "attempt_start_ts": 1490788337147460,
"parent_rev": "88e50721bcd19d719d3417ac9c3f86dd87439268", "commit_rev":
"bca2d029eea47a55324d505a88329cd4112de03d"}

[commit-bot: I haz the power, 2017-03-29 13:24:50]

Description was changed from

==========
Reland Change minimum length of Aura overlay scrollbars.

It turns out Aura (and Android) overlay scrollbars weren't using the minimum
defined in NativeThemeAura. Instead, the minimum was set from
ScrollbarTheme::minimumThumbLength which just returns the thumb thickness.

After adding an override for ScrollbarThemeOverlay which correctly uses the
value form NativeThemeAura, the only consumers of
ScrollbarTheme::minimumThumbLength are the mock classes so I moved the base
implementation into the Mock class and made the base pure virtual.

BUG=678595

Review-Url: https://codereview.chromium.org/2734483002
Cr-Commit-Position: refs/heads/master@{#460199}
Committed:
https://chromium.googlesource.com/chromium/src/+/151a2db45f5fdbca8973988dd51d...
==========

to

==========
Reland Change minimum length of Aura overlay scrollbars.

It turns out Aura (and Android) overlay scrollbars weren't using the minimum
defined in NativeThemeAura. Instead, the minimum was set from
ScrollbarTheme::minimumThumbLength which just returns the thumb thickness.

After adding an override for ScrollbarThemeOverlay which correctly uses the
value form NativeThemeAura, the only consumers of
ScrollbarTheme::minimumThumbLength are the mock classes so I moved the base
implementation into the Mock class and made the base pure virtual.

BUG=678595

Review-Url: https://codereview.chromium.org/2734483002
Cr-Original-Commit-Position: refs/heads/master@{#460199}
Committed:
https://chromium.googlesource.com/chromium/src/+/151a2db45f5fdbca8973988dd51d...
Review-Url: https://codereview.chromium.org/2734483002
Cr-Commit-Position: refs/heads/master@{#460367}
Committed:
https://chromium.googlesource.com/chromium/src/+/bca2d029eea47a55324d505a8832...
==========

[commit-bot: I haz the power, 2017-03-29 13:24:52]

Committed patchset #10 (id:180001) as
https://chromium.googlesource.com/chromium/src/+/bca2d029eea47a55324d505a8832...