Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2006643004:
Fix scrollbar buttons on ChromeOS. (Closed)
|
Created:
4 years, 7 months ago by Bret Modified:
4 years, 7 months ago CC:
blink-reviews, blink-reviews-api_chromium.org, chromium-reviews, darin-cc_chromium.org, dglazkov+blink, jam, kinuko+watch Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionFix scrollbar buttons on ChromeOS.
Scrollbar buttons aren't supposed to draw on ChromeOS, so they need to
return a size of 0.
BUG=613668
Committed: https://crrev.com/57decb6dda19498ceec3b3cbdb9d6c6fd0fed001
Cr-Commit-Position: refs/heads/master@{#396012}
Patch Set 1 #Patch Set 2 : test #
Total comments: 11
Patch Set 3 : native_theme_base has button size again #Patch Set 4 : comments #
Total comments: 1
Patch Set 5 : revert deleting scrollbar_button_length_ #Patch Set 6 : remove function in themeEngine #
Messages
Total messages: 35 (11 generated)
The CQ bit was checked by bsep@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/2006643004/20001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/2006643004/20001
bsep@chromium.org changed reviewers: + esprehn@chromium.org, estade@chromium.org
I'm worried that this breaks native scrollbars (native_scroll_bar_views.cc) https://codereview.chromium.org/2006643004/diff/20001/ui/native_theme/native_... File ui/native_theme/native_theme_base.cc (left): https://codereview.chromium.org/2006643004/diff/20001/ui/native_theme/native_... ui/native_theme/native_theme_base.cc:121: return gfx::Size(scrollbar_button_length_, scrollbar_width_); why not leave most stuff alone, but change scrollbar_width_ to 0 if length is 0?
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: mac_chromium_compile_dbg_ng on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
https://codereview.chromium.org/2006643004/diff/20001/ui/native_theme/native_... File ui/native_theme/native_theme_base.cc (left): https://codereview.chromium.org/2006643004/diff/20001/ui/native_theme/native_... ui/native_theme/native_theme_base.cc:121: return gfx::Size(scrollbar_button_length_, scrollbar_width_); On 2016/05/23 22:30:59, Evan Stade wrote: > why not leave most stuff alone, but change scrollbar_width_ to 0 if length is 0? That wasn't actually the problem. I thought this was unused which is why I removed it, but you're right it looks like native_scroll_bar_views.cc is using it. I'll change this back.
https://codereview.chromium.org/2006643004/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/platform/scroll/ScrollbarThemeAuraTest.cpp (right): https://codereview.chromium.org/2006643004/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/scroll/ScrollbarThemeAuraTest.cpp:15: class ScrollbarThemeAuraButtonOverride : public ScrollbarThemeAura { final https://codereview.chromium.org/2006643004/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/scroll/ScrollbarThemeAuraTest.cpp:22: bool m_hasScrollbarButtons; no m_ if it's public, but can we just use a setter to match the getter? https://codereview.chromium.org/2006643004/diff/20001/ui/native_theme/native_... File ui/native_theme/native_theme_aura.cc (right): https://codereview.chromium.org/2006643004/diff/20001/ui/native_theme/native_... ui/native_theme/native_theme_aura.cc:223: const int extra_padding = (HasScrollbarButtons()) ? 0 : kThumbPadding; remove parens https://codereview.chromium.org/2006643004/diff/20001/ui/native_theme/native_... File ui/native_theme/native_theme_base.cc (right): https://codereview.chromium.org/2006643004/diff/20001/ui/native_theme/native_... ui/native_theme/native_theme_base.cc:223: if (HasScrollbarButtons()) how did we get here if it was zero size?
https://codereview.chromium.org/2006643004/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/platform/scroll/ScrollbarThemeAuraTest.cpp (right): https://codereview.chromium.org/2006643004/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/scroll/ScrollbarThemeAuraTest.cpp:15: class ScrollbarThemeAuraButtonOverride : public ScrollbarThemeAura { On 2016/05/23 23:11:19, esprehn wrote: > final Done. https://codereview.chromium.org/2006643004/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/scroll/ScrollbarThemeAuraTest.cpp:22: bool m_hasScrollbarButtons; On 2016/05/23 23:11:19, esprehn wrote: > no m_ if it's public, but can we just use a setter to match the getter? Sure. Done. https://codereview.chromium.org/2006643004/diff/20001/ui/native_theme/native_... File ui/native_theme/native_theme_aura.cc (right): https://codereview.chromium.org/2006643004/diff/20001/ui/native_theme/native_... ui/native_theme/native_theme_aura.cc:223: const int extra_padding = (HasScrollbarButtons()) ? 0 : kThumbPadding; On 2016/05/23 23:11:19, esprehn wrote: > remove parens Done. https://codereview.chromium.org/2006643004/diff/20001/ui/native_theme/native_... File ui/native_theme/native_theme_base.cc (left): https://codereview.chromium.org/2006643004/diff/20001/ui/native_theme/native_... ui/native_theme/native_theme_base.cc:121: return gfx::Size(scrollbar_button_length_, scrollbar_width_); On 2016/05/23 22:53:15, Bret Sepulveda wrote: > On 2016/05/23 22:30:59, Evan Stade wrote: > > why not leave most stuff alone, but change scrollbar_width_ to 0 if length is > 0? > > That wasn't actually the problem. I thought this was unused which is why I > removed it, but you're right it looks like native_scroll_bar_views.cc is using > it. I'll change this back. Done. https://codereview.chromium.org/2006643004/diff/20001/ui/native_theme/native_... File ui/native_theme/native_theme_base.cc (right): https://codereview.chromium.org/2006643004/diff/20001/ui/native_theme/native_... ui/native_theme/native_theme_base.cc:223: if (HasScrollbarButtons()) On 2016/05/23 23:11:19, esprehn wrote: > how did we get here if it was zero size? It looks like ScrollbarThemeAura just calls paint for every piece of the scrollbar without checking their sizes.
The CQ bit was checked by bsep@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/2006643004/60001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/2006643004/60001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
On 2016/05/24 01:10:12, commit-bot: I haz the power wrote: > Dry run: This issue passed the CQ dry run. Any more comments on this?
https://codereview.chromium.org/2006643004/diff/60001/ui/native_theme/native_... File ui/native_theme/native_theme_aura.cc (left): https://codereview.chromium.org/2006643004/diff/60001/ui/native_theme/native_... ui/native_theme/native_theme_aura.cc:86: set_scrollbar_button_length(0); I still don't understand why you're changing this
On 2016/05/24 22:03:55, Evan Stade wrote: > https://codereview.chromium.org/2006643004/diff/60001/ui/native_theme/native_... > File ui/native_theme/native_theme_aura.cc (left): > > https://codereview.chromium.org/2006643004/diff/60001/ui/native_theme/native_... > ui/native_theme/native_theme_aura.cc:86: set_scrollbar_button_length(0); > I still don't understand why you're changing this Oops sorry, here's more context: When I wrote https://codereview.chromium.org/1911973002 I made ScrollbarThemeAura the authority on what the scrollbar button sizes should be, because it has access to the requested thickness of the scrollbar, whereas native_theme_base was just returning a constant size (g_vertical_scroll_bar_width/height, which I removed). But I didn't realize that ChromeOS doesn't have any scrollbar buttons, so ScrollbarThemeAura was leaving space for buttons that never actually got drawn later because scrollbar_button_length_ was set to 0. When I looked at how scrollbar_button_length_ was used though, it was only ever two values, kDefaultScrollbarButtonLength and 0. So now I'm exposing a HasScrollbarButtons so ScrollbarThemeAura knows to return a size of 0 when that's false, and removing scrollbar_button_length_ for simplicity.
On 2016/05/24 22:15:29, Bret Sepulveda wrote: > On 2016/05/24 22:03:55, Evan Stade wrote: > > > https://codereview.chromium.org/2006643004/diff/60001/ui/native_theme/native_... > > File ui/native_theme/native_theme_aura.cc (left): > > > > > https://codereview.chromium.org/2006643004/diff/60001/ui/native_theme/native_... > > ui/native_theme/native_theme_aura.cc:86: set_scrollbar_button_length(0); > > I still don't understand why you're changing this > > Oops sorry, here's more context: > > When I wrote https://codereview.chromium.org/1911973002 I made > ScrollbarThemeAura the authority on what the scrollbar button sizes should be, > because it has access to the requested thickness of the scrollbar, whereas > native_theme_base was just returning a constant size > (g_vertical_scroll_bar_width/height, which I removed). But I didn't realize that > ChromeOS doesn't have any scrollbar buttons, so ScrollbarThemeAura was leaving > space for buttons that never actually got drawn later because > scrollbar_button_length_ was set to 0. When I looked at how > scrollbar_button_length_ was used though, it was only ever two values, > kDefaultScrollbarButtonLength and 0. So now I'm exposing a HasScrollbarButtons > so ScrollbarThemeAura knows to return a size of 0 when that's false, and > removing scrollbar_button_length_ for simplicity. Right. It doesn't seem simpler to me. Let sleeping dogs lie imo. And if you feel the need to make some change, keep the setter and just change the value to a boolean.
On 2016/05/24 22:17:58, Evan Stade wrote: > On 2016/05/24 22:15:29, Bret Sepulveda wrote: > > On 2016/05/24 22:03:55, Evan Stade wrote: > > > > > > https://codereview.chromium.org/2006643004/diff/60001/ui/native_theme/native_... > > > File ui/native_theme/native_theme_aura.cc (left): > > > > > > > > > https://codereview.chromium.org/2006643004/diff/60001/ui/native_theme/native_... > > > ui/native_theme/native_theme_aura.cc:86: set_scrollbar_button_length(0); > > > I still don't understand why you're changing this > > > > Oops sorry, here's more context: > > > > When I wrote https://codereview.chromium.org/1911973002 I made > > ScrollbarThemeAura the authority on what the scrollbar button sizes should be, > > because it has access to the requested thickness of the scrollbar, whereas > > native_theme_base was just returning a constant size > > (g_vertical_scroll_bar_width/height, which I removed). But I didn't realize > that > > ChromeOS doesn't have any scrollbar buttons, so ScrollbarThemeAura was leaving > > space for buttons that never actually got drawn later because > > scrollbar_button_length_ was set to 0. When I looked at how > > scrollbar_button_length_ was used though, it was only ever two values, > > kDefaultScrollbarButtonLength and 0. So now I'm exposing a HasScrollbarButtons > > so ScrollbarThemeAura knows to return a size of 0 when that's false, and > > removing scrollbar_button_length_ for simplicity. > > Right. It doesn't seem simpler to me. Let sleeping dogs lie imo. And if you feel > the need to make some change, keep the setter and just change the value to a > boolean. Refactoring native_theme was never really the point of this CL so I've reverted all changes to scrollbar_button_length_ and made HasScrollbarButtons just return scrollbar_button_length_>0
On 2016/05/24 23:30:26, Bret Sepulveda wrote: > On 2016/05/24 22:17:58, Evan Stade wrote: > > On 2016/05/24 22:15:29, Bret Sepulveda wrote: > > > On 2016/05/24 22:03:55, Evan Stade wrote: > > > > > > > > > > https://codereview.chromium.org/2006643004/diff/60001/ui/native_theme/native_... > > > > File ui/native_theme/native_theme_aura.cc (left): > > > > > > > > > > > > > > https://codereview.chromium.org/2006643004/diff/60001/ui/native_theme/native_... > > > > ui/native_theme/native_theme_aura.cc:86: set_scrollbar_button_length(0); > > > > I still don't understand why you're changing this > > > > > > Oops sorry, here's more context: > > > > > > When I wrote https://codereview.chromium.org/1911973002 I made > > > ScrollbarThemeAura the authority on what the scrollbar button sizes should > be, > > > because it has access to the requested thickness of the scrollbar, whereas > > > native_theme_base was just returning a constant size > > > (g_vertical_scroll_bar_width/height, which I removed). But I didn't realize > > that > > > ChromeOS doesn't have any scrollbar buttons, so ScrollbarThemeAura was > leaving > > > space for buttons that never actually got drawn later because > > > scrollbar_button_length_ was set to 0. When I looked at how > > > scrollbar_button_length_ was used though, it was only ever two values, > > > kDefaultScrollbarButtonLength and 0. So now I'm exposing a > HasScrollbarButtons > > > so ScrollbarThemeAura knows to return a size of 0 when that's false, and > > > removing scrollbar_button_length_ for simplicity. > > > > Right. It doesn't seem simpler to me. Let sleeping dogs lie imo. And if you > feel > > the need to make some change, keep the setter and just change the value to a > > boolean. > > Refactoring native_theme was never really the point of this CL so I've reverted > all changes to scrollbar_button_length_ and made HasScrollbarButtons just return > scrollbar_button_length_>0 can we just do theme->GetPartSize().IsEmpty() instead of adding a new function?
On 2016/05/24 23:41:35, Evan Stade wrote: > On 2016/05/24 23:30:26, Bret Sepulveda wrote: > > On 2016/05/24 22:17:58, Evan Stade wrote: > > > On 2016/05/24 22:15:29, Bret Sepulveda wrote: > > > > On 2016/05/24 22:03:55, Evan Stade wrote: > > > > > > > > > > > > > > > https://codereview.chromium.org/2006643004/diff/60001/ui/native_theme/native_... > > > > > File ui/native_theme/native_theme_aura.cc (left): > > > > > > > > > > > > > > > > > > > > https://codereview.chromium.org/2006643004/diff/60001/ui/native_theme/native_... > > > > > ui/native_theme/native_theme_aura.cc:86: set_scrollbar_button_length(0); > > > > > I still don't understand why you're changing this > > > > > > > > Oops sorry, here's more context: > > > > > > > > When I wrote https://codereview.chromium.org/1911973002 I made > > > > ScrollbarThemeAura the authority on what the scrollbar button sizes should > > be, > > > > because it has access to the requested thickness of the scrollbar, whereas > > > > native_theme_base was just returning a constant size > > > > (g_vertical_scroll_bar_width/height, which I removed). But I didn't > realize > > > that > > > > ChromeOS doesn't have any scrollbar buttons, so ScrollbarThemeAura was > > leaving > > > > space for buttons that never actually got drawn later because > > > > scrollbar_button_length_ was set to 0. When I looked at how > > > > scrollbar_button_length_ was used though, it was only ever two values, > > > > kDefaultScrollbarButtonLength and 0. So now I'm exposing a > > HasScrollbarButtons > > > > so ScrollbarThemeAura knows to return a size of 0 when that's false, and > > > > removing scrollbar_button_length_ for simplicity. > > > > > > Right. It doesn't seem simpler to me. Let sleeping dogs lie imo. And if you > > feel > > > the need to make some change, keep the setter and just change the value to a > > > boolean. > > > > Refactoring native_theme was never really the point of this CL so I've > reverted > > all changes to scrollbar_button_length_ and made HasScrollbarButtons just > return > > scrollbar_button_length_>0 > > can we just do theme->GetPartSize().IsEmpty() instead of adding a new function? I'm... not sure why I didn't think of that in the first place. Yes, I'll rewrite the CL to do that.
On 2016/05/24 23:50:04, Bret Sepulveda wrote: > On 2016/05/24 23:41:35, Evan Stade wrote: > > On 2016/05/24 23:30:26, Bret Sepulveda wrote: > > > On 2016/05/24 22:17:58, Evan Stade wrote: > > > > On 2016/05/24 22:15:29, Bret Sepulveda wrote: > > > > > On 2016/05/24 22:03:55, Evan Stade wrote: > > > > > > > > > > > > > > > > > > > > > https://codereview.chromium.org/2006643004/diff/60001/ui/native_theme/native_... > > > > > > File ui/native_theme/native_theme_aura.cc (left): > > > > > > > > > > > > > > > > > > > > > > > > > > > https://codereview.chromium.org/2006643004/diff/60001/ui/native_theme/native_... > > > > > > ui/native_theme/native_theme_aura.cc:86: > set_scrollbar_button_length(0); > > > > > > I still don't understand why you're changing this > > > > > > > > > > Oops sorry, here's more context: > > > > > > > > > > When I wrote https://codereview.chromium.org/1911973002 I made > > > > > ScrollbarThemeAura the authority on what the scrollbar button sizes > should > > > be, > > > > > because it has access to the requested thickness of the scrollbar, > whereas > > > > > native_theme_base was just returning a constant size > > > > > (g_vertical_scroll_bar_width/height, which I removed). But I didn't > > realize > > > > that > > > > > ChromeOS doesn't have any scrollbar buttons, so ScrollbarThemeAura was > > > leaving > > > > > space for buttons that never actually got drawn later because > > > > > scrollbar_button_length_ was set to 0. When I looked at how > > > > > scrollbar_button_length_ was used though, it was only ever two values, > > > > > kDefaultScrollbarButtonLength and 0. So now I'm exposing a > > > HasScrollbarButtons > > > > > so ScrollbarThemeAura knows to return a size of 0 when that's false, and > > > > > removing scrollbar_button_length_ for simplicity. > > > > > > > > Right. It doesn't seem simpler to me. Let sleeping dogs lie imo. And if > you > > > feel > > > > the need to make some change, keep the setter and just change the value to > a > > > > boolean. > > > > > > Refactoring native_theme was never really the point of this CL so I've > > reverted > > > all changes to scrollbar_button_length_ and made HasScrollbarButtons just > > return > > > scrollbar_button_length_>0 > > > > can we just do theme->GetPartSize().IsEmpty() instead of adding a new > function? > > I'm... not sure why I didn't think of that in the first place. Yes, I'll rewrite > the CL to do that. This is done.
On 2016/05/25 01:23:16, Bret Sepulveda wrote: > On 2016/05/24 23:50:04, Bret Sepulveda wrote: > > On 2016/05/24 23:41:35, Evan Stade wrote: > > > On 2016/05/24 23:30:26, Bret Sepulveda wrote: > > > > On 2016/05/24 22:17:58, Evan Stade wrote: > > > > > On 2016/05/24 22:15:29, Bret Sepulveda wrote: > > > > > > On 2016/05/24 22:03:55, Evan Stade wrote: > > > > > > > > > > > > > > > > > > > > > > > > > > > > https://codereview.chromium.org/2006643004/diff/60001/ui/native_theme/native_... > > > > > > > File ui/native_theme/native_theme_aura.cc (left): > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > https://codereview.chromium.org/2006643004/diff/60001/ui/native_theme/native_... > > > > > > > ui/native_theme/native_theme_aura.cc:86: > > set_scrollbar_button_length(0); > > > > > > > I still don't understand why you're changing this > > > > > > > > > > > > Oops sorry, here's more context: > > > > > > > > > > > > When I wrote https://codereview.chromium.org/1911973002 I made > > > > > > ScrollbarThemeAura the authority on what the scrollbar button sizes > > should > > > > be, > > > > > > because it has access to the requested thickness of the scrollbar, > > whereas > > > > > > native_theme_base was just returning a constant size > > > > > > (g_vertical_scroll_bar_width/height, which I removed). But I didn't > > > realize > > > > > that > > > > > > ChromeOS doesn't have any scrollbar buttons, so ScrollbarThemeAura was > > > > leaving > > > > > > space for buttons that never actually got drawn later because > > > > > > scrollbar_button_length_ was set to 0. When I looked at how > > > > > > scrollbar_button_length_ was used though, it was only ever two values, > > > > > > kDefaultScrollbarButtonLength and 0. So now I'm exposing a > > > > HasScrollbarButtons > > > > > > so ScrollbarThemeAura knows to return a size of 0 when that's false, > and > > > > > > removing scrollbar_button_length_ for simplicity. > > > > > > > > > > Right. It doesn't seem simpler to me. Let sleeping dogs lie imo. And if > > you > > > > feel > > > > > the need to make some change, keep the setter and just change the value > to > > a > > > > > boolean. > > > > > > > > Refactoring native_theme was never really the point of this CL so I've > > > reverted > > > > all changes to scrollbar_button_length_ and made HasScrollbarButtons just > > > return > > > > scrollbar_button_length_>0 > > > > > > can we just do theme->GetPartSize().IsEmpty() instead of adding a new > > function? > > > > I'm... not sure why I didn't think of that in the first place. Yes, I'll > rewrite > > the CL to do that. > > This is done. hurray, I no longer need to review this cl. :)
On 2016/05/25 17:11:23, Evan Stade wrote: > On 2016/05/25 01:23:16, Bret Sepulveda wrote: > > On 2016/05/24 23:50:04, Bret Sepulveda wrote: > > > On 2016/05/24 23:41:35, Evan Stade wrote: > > > > On 2016/05/24 23:30:26, Bret Sepulveda wrote: > > > > > On 2016/05/24 22:17:58, Evan Stade wrote: > > > > > > On 2016/05/24 22:15:29, Bret Sepulveda wrote: > > > > > > > On 2016/05/24 22:03:55, Evan Stade wrote: > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > https://codereview.chromium.org/2006643004/diff/60001/ui/native_theme/native_... > > > > > > > > File ui/native_theme/native_theme_aura.cc (left): > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > https://codereview.chromium.org/2006643004/diff/60001/ui/native_theme/native_... > > > > > > > > ui/native_theme/native_theme_aura.cc:86: > > > set_scrollbar_button_length(0); > > > > > > > > I still don't understand why you're changing this > > > > > > > > > > > > > > Oops sorry, here's more context: > > > > > > > > > > > > > > When I wrote https://codereview.chromium.org/1911973002 I made > > > > > > > ScrollbarThemeAura the authority on what the scrollbar button sizes > > > should > > > > > be, > > > > > > > because it has access to the requested thickness of the scrollbar, > > > whereas > > > > > > > native_theme_base was just returning a constant size > > > > > > > (g_vertical_scroll_bar_width/height, which I removed). But I didn't > > > > realize > > > > > > that > > > > > > > ChromeOS doesn't have any scrollbar buttons, so ScrollbarThemeAura > was > > > > > leaving > > > > > > > space for buttons that never actually got drawn later because > > > > > > > scrollbar_button_length_ was set to 0. When I looked at how > > > > > > > scrollbar_button_length_ was used though, it was only ever two > values, > > > > > > > kDefaultScrollbarButtonLength and 0. So now I'm exposing a > > > > > HasScrollbarButtons > > > > > > > so ScrollbarThemeAura knows to return a size of 0 when that's false, > > and > > > > > > > removing scrollbar_button_length_ for simplicity. > > > > > > > > > > > > Right. It doesn't seem simpler to me. Let sleeping dogs lie imo. And > if > > > you > > > > > feel > > > > > > the need to make some change, keep the setter and just change the > value > > to > > > a > > > > > > boolean. > > > > > > > > > > Refactoring native_theme was never really the point of this CL so I've > > > > reverted > > > > > all changes to scrollbar_button_length_ and made HasScrollbarButtons > just > > > > return > > > > > scrollbar_button_length_>0 > > > > > > > > can we just do theme->GetPartSize().IsEmpty() instead of adding a new > > > function? > > > > > > I'm... not sure why I didn't think of that in the first place. Yes, I'll > > rewrite > > > the CL to do that. > > > > This is done. > > hurray, I no longer need to review this cl. :) Thank you for your service. esprehn@ could you please review this today, or let me know if you can't?
Description was changed from ========== Fix scrollbar buttons on ChromeOS. Scrollbar buttons aren't supposed to draw on ChromeOS, so they need to return a size of 0. BUG=613668 ========== to ========== Fix scrollbar buttons on ChromeOS. Scrollbar buttons aren't supposed to draw on ChromeOS, so they need to return a size of 0. BUG=613668 ==========
bsep@chromium.org changed reviewers: - estade@chromium.org
lgtm
oshima@chromium.org changed reviewers: + oshima@chromium.org
confirmed the fix on chromeos lgtm
The CQ bit was checked by bsep@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/2006643004/100001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/2006643004/100001
Message was sent while issue was closed.
Description was changed from ========== Fix scrollbar buttons on ChromeOS. Scrollbar buttons aren't supposed to draw on ChromeOS, so they need to return a size of 0. BUG=613668 ========== to ========== Fix scrollbar buttons on ChromeOS. Scrollbar buttons aren't supposed to draw on ChromeOS, so they need to return a size of 0. BUG=613668 ==========
Message was sent while issue was closed.
Committed patchset #6 (id:100001)
Message was sent while issue was closed.
Description was changed from ========== Fix scrollbar buttons on ChromeOS. Scrollbar buttons aren't supposed to draw on ChromeOS, so they need to return a size of 0. BUG=613668 ========== to ========== Fix scrollbar buttons on ChromeOS. Scrollbar buttons aren't supposed to draw on ChromeOS, so they need to return a size of 0. BUG=613668 Committed: https://crrev.com/57decb6dda19498ceec3b3cbdb9d6c6fd0fed001 Cr-Commit-Position: refs/heads/master@{#396012} ==========
Message was sent while issue was closed.
Patchset 6 (id:??) landed as https://crrev.com/57decb6dda19498ceec3b3cbdb9d6c6fd0fed001 Cr-Commit-Position: refs/heads/master@{#396012} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
