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_rel_ng/builds/402305)
3 years, 9 months ago
(2017-03-03 20:02:36 UTC)
#4
Description was changed from ========== Change minimum length of Aura overlay scrollbars. It turns out ...
3 years, 9 months ago
(2017-03-23 16:45:08 UTC)
#7
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
==========
Description was changed from ========== Change minimum length of Aura overlay scrollbars. It turns out ...
3 years, 9 months ago
(2017-03-23 16:46:15 UTC)
#9
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
Hey Evan, There's no good way to test this from Blink as we basically want ...
3 years, 9 months ago
(2017-03-23 16:47:56 UTC)
#10
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
On 2017/03/23 16:47:56, bokan wrote: > Hey Evan, > > There's no good way to ...
3 years, 9 months ago
(2017-03-23 23:16:24 UTC)
#11
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
The CQ bit was checked by bokan@chromium.org to run a CQ dry run
3 years, 9 months ago
(2017-03-24 20:19:52 UTC)
#12
I guess my question was more along the lines of the same one I just ...
3 years, 9 months ago
(2017-03-24 20:30:27 UTC)
#14
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
The CQ bit was checked by bokan@chromium.org to run a CQ dry run
3 years, 9 months ago
(2017-03-24 22:17:12 UTC)
#15
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_android_rel_ng/builds/257641)
3 years, 9 months ago
(2017-03-24 23:33:57 UTC)
#18
3 years, 9 months ago
(2017-03-27 17:27:01 UTC)
#22
Dry run: This issue passed the CQ dry run.
bokan
FYI: I had to add some implementation in Android's native_theme since Android uses ScrollbarThemeOverlay to ...
3 years, 9 months ago
(2017-03-27 18:52:17 UTC)
#23
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
test and patch lgtm https://codereview.chromium.org/2734483002/diff/140001/third_party/WebKit/Source/web/tests/ScrollbarsTest.cpp File third_party/WebKit/Source/web/tests/ScrollbarsTest.cpp (right): https://codereview.chromium.org/2734483002/diff/140001/third_party/WebKit/Source/web/tests/ScrollbarsTest.cpp#newcode94 third_party/WebKit/Source/web/tests/ScrollbarsTest.cpp:94: TEST_P(ScrollbarAppearanceTest, DISABLED_ThemeEngineDefinesMinimumThumbLength) { nit: use ...
3 years, 9 months ago
(2017-03-28 00:11:36 UTC)
#24
Description was changed from ========== Change minimum length of Aura overlay scrollbars. It turns out ...
3 years, 8 months ago
(2017-03-28 13:58:07 UTC)
#26
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
+pdr@ for Source/platform +nasko@ for content/child https://codereview.chromium.org/2734483002/diff/140001/third_party/WebKit/Source/web/tests/ScrollbarsTest.cpp File third_party/WebKit/Source/web/tests/ScrollbarsTest.cpp (right): https://codereview.chromium.org/2734483002/diff/140001/third_party/WebKit/Source/web/tests/ScrollbarsTest.cpp#newcode94 third_party/WebKit/Source/web/tests/ScrollbarsTest.cpp:94: TEST_P(ScrollbarAppearanceTest, DISABLED_ThemeEngineDefinesMinimumThumbLength) { ...
3 years, 8 months ago
(2017-03-28 13:58:37 UTC)
#27
+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.
3 years, 8 months ago
(2017-03-28 15:33:30 UTC)
#28
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?
3 years, 8 months ago
(2017-03-28 15:48:12 UTC)
#29
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.
LGTM
3 years, 8 months ago
(2017-03-28 17:45:45 UTC)
#30
LGTM
nasko
content/ rubberstamp LGTM based on other reviewers.
3 years, 8 months ago
(2017-03-28 17:51:02 UTC)
#31
content/ rubberstamp LGTM based on other reviewers.
bokan
The CQ bit was checked by bokan@chromium.org
3 years, 8 months ago
(2017-03-28 18:06:05 UTC)
#32
CQ is committing da patch. Bot data: {"patchset_id": 160001, "attempt_start_ts": 1490724365475260, "parent_rev": "47b93d128610246960c032a00b67ec2083b2a05b", "commit_rev": "151a2db45f5fdbca8973988dd51d2bf6c86fbc8c"}
3 years, 8 months ago
(2017-03-28 20:15:07 UTC)
#35
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
Description was changed from ========== Change minimum length of Aura overlay scrollbars. It turns out ...
3 years, 8 months ago
(2017-03-28 20:15:55 UTC)
#36
Message was sent while issue was closed.
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
Committed patchset #9 (id:160001) as https://chromium.googlesource.com/chromium/src/+/151a2db45f5fdbca8973988dd51d2bf6c86fbc8c
3 years, 8 months ago
(2017-03-28 20:15:57 UTC)
#37
Description was changed from ========== Change minimum length of Aura overlay scrollbars. It turns out ...
3 years, 8 months ago
(2017-03-29 11:49:54 UTC)
#39
Message was sent while issue was closed.
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
Description was changed from ========== Change minimum length of Aura overlay scrollbars. It turns out ...
3 years, 8 months ago
(2017-03-29 11:52:09 UTC)
#40
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
The CQ bit was checked by bokan@chromium.org
3 years, 8 months ago
(2017-03-29 11:52:17 UTC)
#41
CQ is committing da patch. Bot data: {"patchset_id": 180001, "attempt_start_ts": 1490788337147460, "parent_rev": "88e50721bcd19d719d3417ac9c3f86dd87439268", "commit_rev": "bca2d029eea47a55324d505a88329cd4112de03d"}
3 years, 8 months ago
(2017-03-29 13:24:03 UTC)
#44
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
Description was changed from ========== Reland Change minimum length of Aura overlay scrollbars. It turns ...
3 years, 8 months ago
(2017-03-29 13:24:50 UTC)
#45
Message was sent while issue was closed.
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
Committed patchset #10 (id:180001) as https://chromium.googlesource.com/chromium/src/+/bca2d029eea47a55324d505a88329cd4112de03d
3 years, 8 months ago
(2017-03-29 13:24:52 UTC)
#46
Issue 2734483002: Reland Change minimum length of Aura overlay scrollbars.
(Closed)
Created 3 years, 9 months ago by bokan
Modified 3 years, 8 months ago
Reviewers: Evan Stade, nasko, pdr.
Base URL:
Comments: 7