Issue 2426793002: Aura overlay scrollbars adjust color for dark backgrounds

Issue 2426793002: Aura overlay scrollbars adjust color for dark backgrounds (Closed)

Created:
4 years, 2 months ago by chaopeng

Modified:
4 years, 1 month ago

Reviewers:
Ian Vollick, bokan, Charlie Reis, Peter Kasting, Rick Byers

CC:
blink-reviews, blink-reviews-api_chromium.org, blink-reviews-layout_chromium.org, blink-reviews-paint_chromium.org, cc-bugs_chromium.org, chromium-reviews, creis+watch_chromium.org, darin-cc_chromium.org, dglazkov+blink, dshwang, eae+blinkwatch, jam, jchaffraix+rendering, leviw+renderwatch, nasko+codewatch_chromium.org, pdr+renderingwatchlist_chromium.org, szager+layoutwatch_chromium.org, zoltan1

Target Ref:
refs/pending/heads/master

Project:
chromium

Visibility:
Public.

More Reviews

Description

Aura overlay scrollbars adjust color for dark backgrounds Since we already save the overlay scrollbar color theme in ScrollableArea for Mac, this patch plumbs it through for Aura overlay scrollbars. Also we rename ScrollbarOverlayStyle to ScrollbarOverlayColorTheme to avoid misleading since we already have ScrollbarStyle WebThemeEngine.h. BUG=636662 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2 Committed: https://crrev.com/3386732f6b5e6959698b6697e799a5eb3be6e821 Cr-Commit-Position: refs/heads/master@{#428367}

Patch Set 1 #

Patch Set 2 : update #

Total comments: 3

Patch Set 3 : fix #

Patch Set 4 : rename to ScrollbarOverlayColorTheme #

Total comments: 9

Patch Set 5 : fix #

Total comments: 4

Patch Set 6 : fix #

Total comments: 1

Patch Set 7 : style #

Total comments: 8

Patch Set 8 : remove deadcode(WebThemeEngineImpl::paintStateTransition), remove ScrollbarOverlayColorThemeDefault #

Total comments: 1

Patch Set 9 : add test and remove dead code #

Total comments: 1

Patch Set 10 : remove IntValueBetween #

Total comments: 4

Patch Set 11 : fix for Peter's nit #

Total comments: 1

Patch Set 12 : fix for Peter's nit #

Patch Set 13 : Merge remote-tracking branch 'origin/master' into fix-636662 #

Patch Set 14 : fix test #

Created: 4 years, 1 month ago

Download [raw] [tar.bz2]

	Unified diffs	Side-by-side diffs	Delta from patch set	Stats (+208 lines, -186 lines)			Patch
M	content/child/webthemeengine_impl_default.h	View	1 2 3 4 5 6 7	1 chunk	+0 lines, -6 lines	0 comments	Download
M	content/child/webthemeengine_impl_default.cc	View	1 2 3 4 5 6 7	5 chunks	+22 lines, -11 lines	0 comments	Download
M	third_party/WebKit/Source/core/frame/FrameView.cpp	View	1 2 3 4 5 6 7 8 9 10 11 12	1 chunk	+1 line, -1 line	0 comments	Download
M	third_party/WebKit/Source/core/layout/LayoutBox.cpp	View	1 2 3 4 5 6 7 8 9 10 11 12	1 chunk	+1 line, -1 line	0 comments	Download
M	third_party/WebKit/Source/core/paint/PaintLayerScrollableArea.cpp	View	1 2 3 4 5 6 7 8 9 10 11 12	2 chunks	+16 lines, -14 lines	0 comments	Download
M	third_party/WebKit/Source/core/paint/PaintLayerScrollableAreaTest.cpp	View	1 2 3 4 5 6 7 8	2 chunks	+36 lines, -0 lines	0 comments	Download
M	third_party/WebKit/Source/platform/exported/WebScrollbarImpl.h	View	1 2 3 4	1 chunk	+1 line, -1 line	0 comments	Download
M	third_party/WebKit/Source/platform/exported/WebScrollbarImpl.cpp	View	1 2 3 4	1 chunk	+3 lines, -3 lines	0 comments	Download
M	third_party/WebKit/Source/platform/exported/WebScrollbarThemeClientImpl.h	View	1 2 3	1 chunk	+1 line, -1 line	0 comments	Download
M	third_party/WebKit/Source/platform/exported/WebScrollbarThemeClientImpl.cpp	View	1 2 3	1 chunk	+4 lines, -4 lines	0 comments	Download
M	third_party/WebKit/Source/platform/scroll/ScrollTypes.h	View	1 2 3 4 5 6 7 8 9 10 11 12	1 chunk	+3 lines, -4 lines	0 comments	Download
M	third_party/WebKit/Source/platform/scroll/ScrollableArea.h	View	1 2 3 4	2 chunks	+6 lines, -5 lines	0 comments	Download
M	third_party/WebKit/Source/platform/scroll/ScrollableArea.cpp	View	1 2 3 4 5 6 7 8 9 10 11 12	4 chunks	+14 lines, -13 lines	0 comments	Download
M	third_party/WebKit/Source/platform/scroll/ScrollableAreaTest.cpp	View	1 2 3 4 5 6 7 8 9 10 11 12 13	1 chunk	+8 lines, -8 lines	0 comments	Download
M	third_party/WebKit/Source/platform/scroll/Scrollbar.h	View	1 2 3	1 chunk	+1 line, -1 line	0 comments	Download
M	third_party/WebKit/Source/platform/scroll/Scrollbar.cpp	View	1 2 3 4 5 6 7	1 chunk	+3 lines, -3 lines	0 comments	Download
M	third_party/WebKit/Source/platform/scroll/ScrollbarTheme.h	View	1 2 3	1 chunk	+1 line, -1 line	0 comments	Download
M	third_party/WebKit/Source/platform/scroll/ScrollbarThemeAura.cpp	View	1 2 3 4 5 6	2 chunks	+4 lines, -3 lines	0 comments	Download
M	third_party/WebKit/Source/platform/scroll/ScrollbarThemeClient.h	View	1 2 3	1 chunk	+1 line, -1 line	0 comments	Download
M	third_party/WebKit/Source/platform/scroll/ScrollbarThemeMac.h	View	1 2 3	1 chunk	+1 line, -1 line	0 comments	Download
M	third_party/WebKit/Source/platform/scroll/ScrollbarThemeMac.mm	View	1 2 3 4 5 6 7	3 chunks	+6 lines, -9 lines	0 comments	Download
M	third_party/WebKit/Source/platform/scroll/ScrollbarThemeOverlay.cpp	View	1 2 3 4	1 chunk	+6 lines, -1 line	0 comments	Download
M	third_party/WebKit/Source/web/AssertMatchingEnums.cpp	View	1 2 3 4 5 6 7	1 chunk	+4 lines, -6 lines	0 comments	Download
M	third_party/WebKit/public/BUILD.gn	View	1 2 3 4 5 6 7	1 chunk	+1 line, -0 lines	0 comments	Download
M	third_party/WebKit/public/platform/WebScrollbar.h	View	1 2 3 4	3 chunks	+2 lines, -7 lines	0 comments	Download
A	third_party/WebKit/public/platform/WebScrollbarOverlayColorTheme.h	View	1 2 3 4 5 6 7	1 chunk	+15 lines, -0 lines	0 comments	Download
M	third_party/WebKit/public/platform/WebThemeEngine.h	View	1 2 3 4 5	3 chunks	+7 lines, -0 lines	0 comments	Download
M	ui/native_theme/native_theme.h	View	1 2 3 4 5 6 7	2 chunks	+8 lines, -1 line	0 comments	Download
M	ui/native_theme/native_theme_aura.h	View	1 2 3 4 5 6 7 8	1 chunk	+2 lines, -8 lines	0 comments	Download
M	ui/native_theme/native_theme_aura.cc	View	1 2 3 4 5 6 7 8 9 10 11	6 chunks	+22 lines, -27 lines	0 comments	Download
M	ui/native_theme/native_theme_base.h	View	1 2 3 4 5 6 7 8	3 chunks	+2 lines, -16 lines	0 comments	Download
M	ui/native_theme/native_theme_base.cc	View	1 2 3 4 5 6 7 8	3 chunks	+6 lines, -29 lines	0 comments	Download

Messages

Total messages: 68 (29 generated)

Expand Messages | Collapse Messages | Show Generated Messages | Hide Generated Messages

bokan

First pass. Title isn't very clear, perhaps "Aura overlay scrollbars adjust color for dark backgrounds" ...

4 years, 2 months ago (2016-10-18 19:55:26 UTC) #4

First pass.

Title isn't very clear, perhaps "Aura overlay scrollbars adjust color for dark
backgrounds"

Also, this CL is mixed up with my patch which makes it harder to review. You can
split it out of the review but still build on top of it:

1. Create a local git branch (ex: davidsPatch) and apply my patch on it.
2. (Optional I think) git checkout davidsPatch && git cl issue 2421913002 to set
that branch to track my review
2. Create another local git branch (ex: chaosPatch) off of davidsPatch, apply
your own changes here.
3. git checkout chaosPatch && git branch --set-upstream-to davidsPatch

So the branches and commits look like this:

origin/master  davidsPatch   chaosPatch
    |              |             |
  #3ef45e ----> #2f2f2f2 ----> #2eqceg

https://codereview.chromium.org/2426793002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/frame/FrameView.cpp (right):

https://codereview.chromium.org/2426793002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/frame/FrameView.cpp:2421: while
(view->isTransparent() && view->parentFrameView())
I don't think this is doing what you want it to. For example, if you have:

<html>
  <body style="background-color: red">
    <div style="background-color: blue">
        <iframe ...>
    </div>
   </body>
</html>

And you call this on the iframe, the color you'll calculate will be red, but the
iframe is inside a blue background object.

What you want is to walk up the LayoutObject tree, rather than the frame tree.
You can go from a FrameView to the containing LayoutObject using
m_frame->document()->localOwner(), and then walk up LayoutObject->parent() until
you hit the next FrameView (you know it's a FrameView because it's
isLayoutView() returns true).

(I'd guess this kind of tree walk is already encoded somewhere as it is for the
DOM tree in NodeTraversal.h, but I can't remember seeing something similar for
the layout tree, you may want to take a look first before reimplementing)

https://codereview.chromium.org/2426793002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/paint/PaintLayerScrollableArea.cpp (right):

https://codereview.chromium.org/2426793002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/paint/PaintLayerScrollableArea.cpp:1280: if
(style == ScrollbarOverlayStyleDefault) {
ScrollableArea has a layoutBox() method that returns the LayoutBox for the
ScrollableArea. You should be able to use that to implement
getScrollbarOverlayStyle once in ScrollableArea rather than separately for
PaintLayerScrollableArea and FrameView.

https://codereview.chromium.org/2426793002/diff/20001/third_party/WebKit/publ...
File third_party/WebKit/public/platform/WebThemeEngine.h (right):

https://codereview.chromium.org/2426793002/diff/20001/third_party/WebKit/publ...
third_party/WebKit/public/platform/WebThemeEngine.h:141: enum
ScrollbarOverlayStyle {
Style is misleading in this context. How about ScrollbarOverlayColorTheme?

chaopeng

On 2016/10/18 19:55:26, bokan wrote: > First pass. > > Title isn't very clear, perhaps ...

4 years, 2 months ago (2016-10-20 17:36:45 UTC) #8

On 2016/10/18 19:55:26, bokan wrote:
> First pass.
> 
> Title isn't very clear, perhaps "Aura overlay scrollbars adjust color for dark
> backgrounds"
> 
> Also, this CL is mixed up with my patch which makes it harder to review. You
can
> split it out of the review but still build on top of it:
> 
> 1. Create a local git branch (ex: davidsPatch) and apply my patch on it.
> 2. (Optional I think) git checkout davidsPatch && git cl issue 2421913002 to
set
> that branch to track my review
> 2. Create another local git branch (ex: chaosPatch) off of davidsPatch, apply
> your own changes here.
> 3. git checkout chaosPatch && git branch --set-upstream-to davidsPatch
> 
> So the branches and commits look like this:
> 
> origin/master  davidsPatch   chaosPatch
>     |              |             |
>   #3ef45e ----> #2f2f2f2 ----> #2eqceg
> 
>
https://codereview.chromium.org/2426793002/diff/20001/third_party/WebKit/Sour...
> File third_party/WebKit/Source/core/frame/FrameView.cpp (right):
> 
>
https://codereview.chromium.org/2426793002/diff/20001/third_party/WebKit/Sour...
> third_party/WebKit/Source/core/frame/FrameView.cpp:2421: while
> (view->isTransparent() && view->parentFrameView())
> I don't think this is doing what you want it to. For example, if you have:
> 
> <html>
>   <body style="background-color: red">
>     <div style="background-color: blue">
>         <iframe ...>
>     </div>
>    </body>
> </html>
> 
> And you call this on the iframe, the color you'll calculate will be red, but
the
> iframe is inside a blue background object.
> 
> What you want is to walk up the LayoutObject tree, rather than the frame tree.
> You can go from a FrameView to the containing LayoutObject using
> m_frame->document()->localOwner(), and then walk up LayoutObject->parent()
until
> you hit the next FrameView (you know it's a FrameView because it's
> isLayoutView() returns true).
> 
> (I'd guess this kind of tree walk is already encoded somewhere as it is for
the
> DOM tree in NodeTraversal.h, but I can't remember seeing something similar for
> the layout tree, you may want to take a look first before reimplementing)
> 
>
https://codereview.chromium.org/2426793002/diff/20001/third_party/WebKit/Sour...
> File third_party/WebKit/Source/core/paint/PaintLayerScrollableArea.cpp
(right):
> 
>
https://codereview.chromium.org/2426793002/diff/20001/third_party/WebKit/Sour...
> third_party/WebKit/Source/core/paint/PaintLayerScrollableArea.cpp:1280: if
> (style == ScrollbarOverlayStyleDefault) {
> ScrollableArea has a layoutBox() method that returns the LayoutBox for the
> ScrollableArea. You should be able to use that to implement
> getScrollbarOverlayStyle once in ScrollableArea rather than separately for
> PaintLayerScrollableArea and FrameView.
> 
>
https://codereview.chromium.org/2426793002/diff/20001/third_party/WebKit/publ...
> File third_party/WebKit/public/platform/WebThemeEngine.h (right):
> 
>
https://codereview.chromium.org/2426793002/diff/20001/third_party/WebKit/publ...
> third_party/WebKit/public/platform/WebThemeEngine.h:141: enum
> ScrollbarOverlayStyle {
> Style is misleading in this context. How about ScrollbarOverlayColorTheme?

PTAL. Thank you.

bokan

https://codereview.chromium.org/2426793002/diff/60001/content/child/webthemeengine_impl_default.cc File content/child/webthemeengine_impl_default.cc (right): https://codereview.chromium.org/2426793002/diff/60001/content/child/webthemeengine_impl_default.cc#newcode182 content/child/webthemeengine_impl_default.cc:182: native_theme_extra_params->scrollbar_theme = These should be |native_theme_extra_params->scrollbar_thumb.scrollbar_theme|. See my comment ...

4 years, 2 months ago (2016-10-21 16:57:14 UTC) #9

chaopeng

On 2016/10/21 16:57:14, bokan wrote: > https://codereview.chromium.org/2426793002/diff/60001/content/child/webthemeengine_impl_default.cc > File content/child/webthemeengine_impl_default.cc (right): > > https://codereview.chromium.org/2426793002/diff/60001/content/child/webthemeengine_impl_default.cc#newcode182 > ...

4 years, 2 months ago (2016-10-21 19:52:55 UTC) #12

bokan

https://codereview.chromium.org/2426793002/diff/80001/content/child/webthemeengine_impl_default.cc File content/child/webthemeengine_impl_default.cc (right): https://codereview.chromium.org/2426793002/diff/80001/content/child/webthemeengine_impl_default.cc#newcode197 content/child/webthemeengine_impl_default.cc:197: static_cast<ui::NativeTheme::ScrollbarOverlayColorTheme>( use the NativeThemeScrollbarOverlayColorTheme function rather than casting. https://codereview.chromium.org/2426793002/diff/80001/third_party/WebKit/Source/platform/scroll/ScrollbarThemeAura.cpp ...

4 years, 2 months ago (2016-10-21 20:11:39 UTC) #13

chaopeng

On 2016/10/21 20:11:39, bokan wrote: > https://codereview.chromium.org/2426793002/diff/80001/content/child/webthemeengine_impl_default.cc > File content/child/webthemeengine_impl_default.cc (right): > > https://codereview.chromium.org/2426793002/diff/80001/content/child/webthemeengine_impl_default.cc#newcode197 > ...

4 years, 2 months ago (2016-10-21 22:00:49 UTC) #14

bokan

lgtm % nit https://codereview.chromium.org/2426793002/diff/100001/third_party/WebKit/Source/platform/scroll/ScrollbarThemeAura.cpp File third_party/WebKit/Source/platform/scroll/ScrollbarThemeAura.cpp (right): https://codereview.chromium.org/2426793002/diff/100001/third_party/WebKit/Source/platform/scroll/ScrollbarThemeAura.cpp#newcode348 third_party/WebKit/Source/platform/scroll/ScrollbarThemeAura.cpp:348: state, WebRect(rect), 0); Nit: nullptr

4 years, 2 months ago (2016-10-21 22:46:44 UTC) #15

bokan

Also, please update the description to mention the rename. And change: "Since we already save ...

4 years, 2 months ago (2016-10-21 22:48:26 UTC) #16

bokan

+pkasting@ for ui/native_theme +creis@ for content/child +rbyers@ for WebKit/public +jbroman@ for WebKit/platform

4 years, 1 month ago (2016-10-24 13:55:13 UTC) #22

Rick Byers

On 2016/10/24 13:55:13, bokan wrote: > +pkasting@ for ui/native_theme > +creis@ for content/child > +rbyers@ ...

4 years, 1 month ago (2016-10-24 14:23:56 UTC) #23

Charlie Reis

https://codereview.chromium.org/2426793002/diff/120001/content/child/webthemeengine_impl_default.h File content/child/webthemeengine_impl_default.h (right): https://codereview.chromium.org/2426793002/diff/120001/content/child/webthemeengine_impl_default.h#newcode12 content/child/webthemeengine_impl_default.h:12: #include "ui/native_theme/native_theme.h" This declares ScrollbarOverlayColorTheme, but that doesn't appear ...

4 years, 1 month ago (2016-10-24 19:22:19 UTC) #24

chaopeng

https://codereview.chromium.org/2426793002/diff/120001/content/child/webthemeengine_impl_default.h File content/child/webthemeengine_impl_default.h (right): https://codereview.chromium.org/2426793002/diff/120001/content/child/webthemeengine_impl_default.h#newcode25 content/child/webthemeengine_impl_default.h:25: virtual void paintStateTransition( On 2016/10/24 19:22:19, Charlie Reis wrote: ...

4 years, 1 month ago (2016-10-24 19:51:29 UTC) #25

Charlie Reis

[+weiliangc, who added PaintStateTransition] https://codereview.chromium.org/2426793002/diff/120001/content/child/webthemeengine_impl_default.h File content/child/webthemeengine_impl_default.h (right): https://codereview.chromium.org/2426793002/diff/120001/content/child/webthemeengine_impl_default.h#newcode25 content/child/webthemeengine_impl_default.h:25: virtual void paintStateTransition( On 2016/10/24 ...

4 years, 1 month ago (2016-10-24 20:30:29 UTC) #27

Peter Kasting

https://codereview.chromium.org/2426793002/diff/120001/ui/native_theme/native_theme_aura.cc File ui/native_theme/native_theme_aura.cc (right): https://codereview.chromium.org/2426793002/diff/120001/ui/native_theme/native_theme_aura.cc#newcode210 ui/native_theme/native_theme_aura.cc:210: if (theme == ScrollbarOverlayColorThemeLight) { This code has only ...

4 years, 1 month ago (2016-10-24 23:33:42 UTC) #28

jbroman

I'm on vacation and probably won't be able to give this a good review until ...

4 years, 1 month ago (2016-10-25 00:33:49 UTC) #29

Ian Vollick

On 2016/10/25 00:55:28, chaopeng wrote: > mailto:vollick@chromium.org: Please review changes in lgtm

4 years, 1 month ago (2016-10-25 01:37:11 UTC) #33

bokan

https://codereview.chromium.org/2426793002/diff/120001/content/child/webthemeengine_impl_default.h File content/child/webthemeengine_impl_default.h (right): https://codereview.chromium.org/2426793002/diff/120001/content/child/webthemeengine_impl_default.h#newcode25 content/child/webthemeengine_impl_default.h:25: virtual void paintStateTransition( On 2016/10/24 20:30:29, Charlie Reis wrote: ...

4 years, 1 month ago (2016-10-25 18:45:52 UTC) #34

https://codereview.chromium.org/2426793002/diff/120001/content/child/webtheme...
File content/child/webthemeengine_impl_default.h (right):

https://codereview.chromium.org/2426793002/diff/120001/content/child/webtheme...
content/child/webthemeengine_impl_default.h:25: virtual void
paintStateTransition(
On 2016/10/24 20:30:29, Charlie Reis wrote:
> On 2016/10/24 19:51:29, chaopeng wrote:
> > On 2016/10/24 19:22:19, Charlie Reis wrote:
> > > I'm a bit confused about this method-- it's listed with the overrides of
> > > WebThemeEngine methods, but it's not an override (and there's no matching
> > method
> > > in WebThemeEngine).  I also don't see any call sites to it in code search.
> > > 
> > > When is it called?  Was it meant to be an override?  If not, we should add
a
> > > blank line to move it to a separate block from the // WebThemeEngine
> methods.
> > 
> > This method looks never get called and no override. Should I delete it?
> 
> That's a good question-- I might have to defer to someone who knows that code
a
> little more.
> 
> It looks like it was introduced in https://codereview.chromium.org/238723004/
> and a call was added in ScrollbarThemeOverlay in
> https://codereview.chromium.org/246293006.  That call site is no longer there,
> so maybe it's dead code?
> 
> If so, it looks like this is the only place that calls
> NativeTheme::PaintStateTransition, so maybe that can be removed as well?

I believe the original intention was for the painting code to animate transition
states (e.g. changing the opacity from idle to hovered state). The current
requirements don't call for animated opacity and any animated properties are
done on the compositor for performance reasons so I think we can get rid of
this. Similarly, NativeThemeAura has a "transition" version of the
PaintScrollbarThumb that can be merged and removed as well.

https://codereview.chromium.org/2426793002/diff/120001/ui/native_theme/native...
File ui/native_theme/native_theme_aura.cc (right):

https://codereview.chromium.org/2426793002/diff/120001/ui/native_theme/native...
ui/native_theme/native_theme_aura.cc:210: if (theme ==
ScrollbarOverlayColorThemeLight) {
On 2016/10/24 23:33:42, Peter Kasting wrote:
> This code has only two styles, while on Mac we have three, which Apple's
> developer docs describe as "dark", "light", and "dark with a light border".  I
> don't have screenshots of those (can I see some?), but it seems like the code
> here merely implements "dark with a light border" and "light with a dark
> border".
> 
> If we want the Mac meanings, then I would implement three distinct states
here;
> make the colors be reasonable facsimiles of the Mac colors (probably not just
> white/black based, but using shades for the dark/light cases; again,
screenshots
> would help); and rename the enums to not use "default" but explicitly describe
> each case (e.g. "dark thumb light border").
> 
> If we want to have two types of thumbs based on contrasting with light or dark
> backgrounds, then I would change the enums to only have two states, and make
the
> behavior consistent across Mac/views.

Taking a look, the Mac Dark/Default seems to produce exactly the same scrollbar
(I changed the painting code in ScrollbarThemeMac to pass Dark or Default
unconditionally), here's screen shots:

http://bokan.ca/light.png
http://bokan.ca/dark.png
http://bokan.ca/default.png

In any case, that's moot since we start with "Default" and if the background is
a dark color use "Light" scrollbars, so we never actually instantiate "Dark"
scrollbars. Given that, I'd suggest we remove "Default" from our ColorTheme enum
and just have "Light" and "Dark", defaulting to Dark as we do right now.
(ScrollbarThemeMac can use NSScrollerKnobStyleDefault for our
ScrollbarOverlayColorThemeDark).

Regarding matching colors, I think we already do. Although the the color
specified in the Aura painting code is White/Black, the shading comes from the
opacity so they're quite similar. See the spec sheet:
https://drive.google.com/a/google.com/file/d/0B6x6iYCtKinEbVBwQVNtN2tWTXc/vie...

Peter Kasting

4 years, 1 month ago (2016-10-25 19:40:48 UTC) #35

bokan

On 2016/10/25 19:40:48, Peter Kasting wrote: > > (ScrollbarThemeMac can use NSScrollerKnobStyleDefault for our > ...

4 years, 1 month ago (2016-10-25 19:49:34 UTC) #36

bokan

https://codereview.chromium.org/2426793002/diff/140001/ui/native_theme/native_theme_aura.h File ui/native_theme/native_theme_aura.h (right): https://codereview.chromium.org/2426793002/diff/140001/ui/native_theme/native_theme_aura.h#newcode51 ui/native_theme/native_theme_aura.h:51: void PaintScrollbarThumbStateTransition( The only actual caller of this method ...

4 years, 1 month ago (2016-10-27 15:14:49 UTC) #38

chaopeng

On 2016/10/27 15:14:49, bokan wrote: > https://codereview.chromium.org/2426793002/diff/140001/ui/native_theme/native_theme_aura.h > File ui/native_theme/native_theme_aura.h (right): > > https://codereview.chromium.org/2426793002/diff/140001/ui/native_theme/native_theme_aura.h#newcode51 > ...

4 years, 1 month ago (2016-10-27 18:26:26 UTC) #39

bokan

https://codereview.chromium.org/2426793002/diff/160001/ui/native_theme/native_theme_aura.cc File ui/native_theme/native_theme_aura.cc (right): https://codereview.chromium.org/2426793002/diff/160001/ui/native_theme/native_theme_aura.cc#newcode197 ui/native_theme/native_theme_aura.cc:197: double progress = 1.0; Remove this and the IntValueBetween ...

4 years, 1 month ago (2016-10-27 18:29:53 UTC) #40

chaopeng

On 2016/10/27 18:29:53, bokan wrote: > https://codereview.chromium.org/2426793002/diff/160001/ui/native_theme/native_theme_aura.cc > File ui/native_theme/native_theme_aura.cc (right): > > https://codereview.chromium.org/2426793002/diff/160001/ui/native_theme/native_theme_aura.cc#newcode197 > ...

4 years, 1 month ago (2016-10-27 18:42:17 UTC) #41

bokan

Thanks, still lgtm. Please wait for creis@'s and pkasting@'s stamps though.

4 years, 1 month ago (2016-10-27 18:44:57 UTC) #42

Peter Kasting

ui/native_theme LGTM https://codereview.chromium.org/2426793002/diff/180001/ui/native_theme/native_theme_aura.cc File ui/native_theme/native_theme_aura.cc (right): https://codereview.chromium.org/2426793002/diff/180001/ui/native_theme/native_theme_aura.cc#newcode41 ui/native_theme/native_theme_aura.cc:41: constexpr SkColor kOverlayScrollbarStrokeLightColor = SK_ColorBLACK; Nit: If ...

4 years, 1 month ago (2016-10-27 22:48:50 UTC) #44

chaopeng

On 2016/10/27 22:48:50, Peter Kasting wrote: > ui/native_theme LGTM > > https://codereview.chromium.org/2426793002/diff/180001/ui/native_theme/native_theme_aura.cc > File ui/native_theme/native_theme_aura.cc ...

4 years, 1 month ago (2016-10-28 00:59:23 UTC) #46

Peter Kasting

Still LGTM Note: You do not need to get re-review from someone after their LGTM ...

4 years, 1 month ago (2016-10-28 01:03:07 UTC) #47