Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1671313002:
MacViews: Overlay Scrollbars with Show/Hide Animations (Closed)
DescriptionMacViews: Overlay Scrollbars with Show/Hide Animations
The scrollbars are implemented in CocoaScrollBar. They switch
between overlay and non-overlay style, depending on the
system's preference
If using the overlay style, the scrollbars will:
- Hide if unused
- Display the scrolltrack if the user hovers over it
- Currently hovering will not adjust the width. That will come later.
In addition, since we are switching to CocoaScrollbars on OSX, dead
code is cleaned up in native_theme_mac. Tests are modified and
written for the overlay scrollbars. Changes are made to
scoped_preferred_scroller_style_legacy_mac so that we can force
the scroller style to be non-overlay and overlay in our tests.
BUG=568409
Committed: https://crrev.com/83534ab036f32a6aa7417e64fa3bc37a0b61be9d
Cr-Commit-Position: refs/heads/master@{#377052}
Patch Set 1 #Patch Set 2 : Added comments and fixed nits #
Total comments: 39
Patch Set 3 : Switched to animating using layers #Patch Set 4 : nit #
Total comments: 13
Patch Set 5 : #
Total comments: 32
Patch Set 6 : Switched to CocoaScrollbar #Patch Set 7 : #
Total comments: 34
Patch Set 8 : Remove dead native theme code, fixed overlapping scrollbars #Patch Set 9 : #Patch Set 10 : nit #
Total comments: 4
Patch Set 11 : Update layout according to scroller style #
Total comments: 22
Patch Set 12 : Added hover thumb color and fixed dragging issue #Patch Set 13 : Fixed scrollview.scrollbar test #Patch Set 14 : Added test for Overlay scrollers #Patch Set 15 : nit #Patch Set 16 : #
Total comments: 16
Patch Set 17 : Tests and nits #Patch Set 18 : #
Total comments: 6
Patch Set 19 : #
Total comments: 11
Patch Set 20 : #
Total comments: 8
Patch Set 21 : Rebased and fixed test #Patch Set 22 : #Patch Set 23 : nit #
Total comments: 7
Patch Set 24 : nits #Messages
Total messages: 55 (14 generated)
spqchan@chromium.org changed reviewers: + tapted@chromium.org
On 2016/02/06 03:38:41, spqchan wrote: > mailto:spqchan@chromium.org changed reviewers: > + mailto:tapted@chromium.org Hey, can you review this? My CL is getting to get too big so I'm going to submit what I have for show/hide animations. This is to make sure that the existing code structure is in the right direction before I write more code for the hover animation. Anyway, here is a quick breakdown on the code structure: I created the Overlay Scrollbar in the form of the NativeCocoaScrollBar class which inherits from the NativeScrollBar. This way we can get a scrollbar with the NativeTheme NativeCocoaScrollBar will handle the show/hide animation logic by applying the OverlayParams to the parts of the ScrollBar. The OverlayParams is then used by the NativeThemeMac to change the opacity of the views. To get the scroller style, I have ViewsScrollbarBridge as a bridge to NSScroller Thanks!
Using GetLayer()->GetAnimator()->SetOpacity(0.0) might simplify a lot of the NativeTheme changes and other decisions around NativeScrollBarWrapper and friends. Also.... NativeScrollBar and friends might be in need of a bit of refactoring first. Looking at http://crbug.com/93103 I think NativeScrollBar came about to solve a problem on Windows that no longer exists, but it was never cleaned up. i.e. *all* scrollers (including the one we want for Mac) are "bitmap" scrollbars -- i.e. we do all the drawing rather than some native library. The `Wrapper` was I think originally to Wrap some native library resource (like a HWND or similar) and is now obsolete. So, there might be a way to do this without adjustments to NativeScrollBarWrapper (but maybe first we need to explore getting rid of that entirely, and `flattening` the scrollbar class hierarchy a bit). But! with the layer-animator() changes that might become tangential anyway. Let me know if that's enough to go on - I'd need to paw through the scrollbar code a bit more to figure out what the right refactor is, (but I'm pretty sure one is long overdue..). https://codereview.chromium.org/1671313002/diff/20001/ui/native_theme/native_... File ui/native_theme/native_theme.h (right): https://codereview.chromium.org/1671313002/diff/20001/ui/native_theme/native_... ui/native_theme/native_theme.h:105: double is_overlay; double -> bool? https://codereview.chromium.org/1671313002/diff/20001/ui/views/cocoa/views_sc... File ui/views/cocoa/views_scrollbar_bridge.h (right): https://codereview.chromium.org/1671313002/diff/20001/ui/views/cocoa/views_sc... ui/views/cocoa/views_scrollbar_bridge.h:31: - (void)onScrollerStyleChanged:(NSNotification*)notification; nit: move to private `@interface ViewsScrollbarBridge ()` in the .mm? https://codereview.chromium.org/1671313002/diff/20001/ui/views/cocoa/views_sc... File ui/views/cocoa/views_scrollbar_bridge.mm (right): https://codereview.chromium.org/1671313002/diff/20001/ui/views/cocoa/views_sc... ui/views/cocoa/views_scrollbar_bridge.mm:16: addObserver:self usually `dealloc` should remove this, but since this is owned by a C++ instance, which has a tighter lifetime, there should be a method to clear the delegate, which also removes this observer. There are many ways to structure this... my favourite would be to scrap the `initWithDelegate` method, and just have a - (void)setDelegate:(ViewsScrollbarBridgeDelegate*)delegate { DCHECK_NE(delegate, delegate_); delegate_ = delegate; if (!delegate_) { [[NSNotificationCenter ..] removeObserver:self]; return; } [[NSNotificationCenter ..] addObserver:self]; } then optional would be a dealloc override which just does { DCHECK(!delegate_); [super dealloc]; } https://codereview.chromium.org/1671313002/diff/20001/ui/views/cocoa/views_sc... ui/views/cocoa/views_scrollbar_bridge.mm:29: if (![NSScroller respondsToSelector:@selector(preferredScrollerStyle)]) this shouldn't be needed much longer, since it is 10.6 only... optional: `DCHECK(IsOSSnowLeopard) inside the `if`. That would remind us that we can remove the check when we remove the `IsOSSnowLeopard` declaration once it becomes a tautology. But probably we will just git-grep for preferredScrollerStyle since lots of things do this. https://codereview.chromium.org/1671313002/diff/20001/ui/views/controls/scrol... File ui/views/controls/scrollbar/native_cocoa_scroll_bar.h (right): https://codereview.chromium.org/1671313002/diff/20001/ui/views/controls/scrol... ui/views/controls/scrollbar/native_cocoa_scroll_bar.h:34: class VIEWS_EXPORT NativeCocoaScrollBarViews nit: needs a comment also I'm not sure about `Native` :). Usually that means we're using some native drawing code, but we're actually emulating it. So perhaps `Emulated`? Or something else - I'm not fully sold on Emulated either Note there is http://crbug.com/93103 "Get rid of NativeScrollBar" (or, just rename/merge it really) https://codereview.chromium.org/1671313002/diff/20001/ui/views/controls/scrol... ui/views/controls/scrollbar/native_cocoa_scroll_bar.h:61: NSScrollerStyle scroller_style_; needs an initializer, since UpdateScrollStyle checks the current value. NSScrollerStyle is typedef NSInteger, so perhaps just initialize to `-1` to force the setup the first time UpdateScrollStyle is called. Then you can probably get rid of the separate |init_| data member. (But comment here like // The current scroller style, or -1 before it is initialized. NSScrollerStyle scroller_style_ = -1; But! The initialization flow might really need to be tweaked a bit. E.g. it should never animate. Perhaps the constructor should just call [bridge_ getPreferredScrollerStyle] and set properties appropriately without animating https://codereview.chromium.org/1671313002/diff/20001/ui/views/controls/scrol... ui/views/controls/scrollbar/native_cocoa_scroll_bar.h:70: double scroller_opacity_; This should probably be initialized too https://codereview.chromium.org/1671313002/diff/20001/ui/views/controls/scrol... ui/views/controls/scrollbar/native_cocoa_scroll_bar.h:76: bool init_; (remove[ or initialize]) https://codereview.chromium.org/1671313002/diff/20001/ui/views/controls/scrol... File ui/views/controls/scrollbar/native_cocoa_scroll_bar.mm (right): https://codereview.chromium.org/1671313002/diff/20001/ui/views/controls/scrol... ui/views/controls/scrollbar/native_cocoa_scroll_bar.mm:14: // How long we should wait before hiding the scrollbar. nit: blank line before https://codereview.chromium.org/1671313002/diff/20001/ui/views/controls/scrol... ui/views/controls/scrollbar/native_cocoa_scroll_bar.mm:22: } nit: } // namespace (also, blank line before) https://codereview.chromium.org/1671313002/diff/20001/ui/views/controls/scrol... ui/views/controls/scrollbar/native_cocoa_scroll_bar.mm:46: UpdateScrollStyle(); So I think we can simplify things a bit and get a much smoother animation by using a ui::Layer. Here, you would call SetPaintsToLayer(true); (invoking the method on views::View). Then GetLayer()->SetAnimator()->( new ui::LayerAnimator(base::TimeDelta::FromMilliseconds(kFadeDurationMs)); (the gfx::LinearAnimation shouldn't be needed) Then, see other comments. However, there is a tricky part, and that's what to do with the gutter with legacy scrollbars -- it shouldn't fade out, but the thumb should (partially). I think the best way to do it would be to add the scrollbar thumb as a subview with its own layer. https://codereview.chromium.org/1671313002/diff/20001/ui/views/controls/scrol... ui/views/controls/scrollbar/native_cocoa_scroll_bar.mm:100: animation_.Start(); Then here, you would just call GetLayer()->GetAnimator()->SetOpacity(0.0); You shouldn't need the SchedulePaint call https://codereview.chromium.org/1671313002/diff/20001/ui/views/controls/scrol... ui/views/controls/scrollbar/native_cocoa_scroll_bar.mm:105: animation_.Stop(); And here, GetLayer()->SetOpacity(1.0); https://codereview.chromium.org/1671313002/diff/20001/ui/views/controls/scrol... File ui/views/controls/scrollbar/native_scroll_bar.cc (right): https://codereview.chromium.org/1671313002/diff/20001/ui/views/controls/scrol... ui/views/controls/scrollbar/native_scroll_bar.cc:67: // Create a method to CreateWrapper and then override that shit (lol) https://codereview.chromium.org/1671313002/diff/20001/ui/views/controls/scrol... File ui/views/controls/scrollbar/native_scroll_bar.h (right): https://codereview.chromium.org/1671313002/diff/20001/ui/views/controls/scrol... ui/views/controls/scrollbar/native_scroll_bar.h:66: NativeScrollBarWrapper* native_wrapper_; hrm - should this be scoped_ptr? I can't figure out who owns the returned pointer from CreateWrapper.. https://codereview.chromium.org/1671313002/diff/20001/ui/views/controls/scrol... File ui/views/controls/scrollbar/native_scroll_bar_views.cc (right): https://codereview.chromium.org/1671313002/diff/20001/ui/views/controls/scrol... ui/views/controls/scrollbar/native_scroll_bar_views.cc:55: explicit ScrollBarThumb(BaseScrollBar* scroll_bar, nit: explicit not needed https://codereview.chromium.org/1671313002/diff/20001/ui/views/controls/scrol... ui/views/controls/scrollbar/native_scroll_bar_views.cc:71: NativeScrollBarWrapper* wrapper_; hm. I wonder if ScrollBar should expose a GetNativeWrapper virtual method. Or, does anything other than NativeScrollBarViews call the constructor? (can you just pass a NativeScrollBar, add a NativeScrollBar::native_wrapper() accessor, and change the type of |scroll_bar_| to NativeScrollBar? https://codereview.chromium.org/1671313002/diff/20001/ui/views/controls/scrol... ui/views/controls/scrollbar/native_scroll_bar_views.cc:114: params.scrollbar_arrow.overlay = wrapper_->GetOverlayParams(); check for a null |wrapper_|? https://codereview.chromium.org/1671313002/diff/20001/ui/views/controls/scrol... ui/views/controls/scrollbar/native_scroll_bar_views.cc:197: params.scrollbar_thumb.overlay = wrapper_->GetOverlayParams(); null check?
Wow, I had no idea about GetLayer()->GetAnimator()->SetOpacity(0.0). That would really simplify things. I'll give that a try and will let you how it goes. Afterwards, let’s figure out what to do with NativeScrollBar. It would make sense to flatten that, especially since it's needlessly complicated right now
On 2016/02/08 22:12:50, spqchan wrote: > Wow, I had no idea about GetLayer()->GetAnimator()->SetOpacity(0.0). That would > really simplify things. > I'll give that a try and will let you how it goes. Afterwards, let’s figure out > what to do with NativeScrollBar. > It would make sense to flatten that, especially since it's needlessly > complicated right now I just changed the code to use the layer animation and it simplified a large portion of the code. The next step would be to figure out what to NativeScrollBar. I’m a bit confused about BitmapScrollbar. Are NativeScrollBars replaced with them? Anyway, if we’re simplifying NativeScrollBars, I should probably do it in a separate CL first. We can start this progress by flattening NativeScrollbarWrapper into NativeScrollbarViews first. Afterwards, NativeScrollbarViews can be flattened into NativeScrollbar. I’m a bit concerned that doing that might make the code in NativeScrollbar too bulky. Thoughts?
I just changed the code to use the layer animation and it simplified a large portion of the code. The next step would be to figure out what to NativeScrollBar. I’m a bit confused about BitmapScrollbar. Are NativeScrollBars replaced with them? Anyway, if we’re simplifying NativeScrollBars, I should probably do it in a separate CL first. We can start this progress by flattening NativeScrollbarWrapper into NativeScrollbarViews first. Afterwards, NativeScrollbarViews can be flattened into NativeScrollbar. I’m a bit concerned that doing that might make the code in NativeScrollbar too bulky. Thoughts? https://codereview.chromium.org/1671313002/diff/20001/ui/native_theme/native_... File ui/native_theme/native_theme.h (right): https://codereview.chromium.org/1671313002/diff/20001/ui/native_theme/native_... ui/native_theme/native_theme.h:105: double is_overlay; On 2016/02/08 00:31:15, tapted wrote: > double -> bool? Whoops, done https://codereview.chromium.org/1671313002/diff/20001/ui/views/cocoa/views_sc... File ui/views/cocoa/views_scrollbar_bridge.h (right): https://codereview.chromium.org/1671313002/diff/20001/ui/views/cocoa/views_sc... ui/views/cocoa/views_scrollbar_bridge.h:31: - (void)onScrollerStyleChanged:(NSNotification*)notification; On 2016/02/08 00:31:15, tapted wrote: > nit: move to private `@interface ViewsScrollbarBridge ()` in the .mm? Done. https://codereview.chromium.org/1671313002/diff/20001/ui/views/cocoa/views_sc... File ui/views/cocoa/views_scrollbar_bridge.mm (right): https://codereview.chromium.org/1671313002/diff/20001/ui/views/cocoa/views_sc... ui/views/cocoa/views_scrollbar_bridge.mm:16: addObserver:self On 2016/02/08 00:31:15, tapted wrote: > usually `dealloc` should remove this, but since this is owned by a C++ instance, > which has a tighter lifetime, there should be a method to clear the delegate, > which also removes this observer. There are many ways to structure this... my > favourite would be to scrap the `initWithDelegate` method, and just have a > > - (void)setDelegate:(ViewsScrollbarBridgeDelegate*)delegate { > DCHECK_NE(delegate, delegate_); > delegate_ = delegate; > if (!delegate_) { > [[NSNotificationCenter ..] removeObserver:self]; > return; > } > > [[NSNotificationCenter ..] addObserver:self]; > } > > > then optional would be a dealloc override which just does > > { > DCHECK(!delegate_); > [super dealloc]; > } Done. https://codereview.chromium.org/1671313002/diff/20001/ui/views/cocoa/views_sc... ui/views/cocoa/views_scrollbar_bridge.mm:29: if (![NSScroller respondsToSelector:@selector(preferredScrollerStyle)]) On 2016/02/08 00:31:15, tapted wrote: > this shouldn't be needed much longer, since it is 10.6 only... optional: > `DCHECK(IsOSSnowLeopard) inside the `if`. That would remind us that we can > remove the check when we remove the `IsOSSnowLeopard` declaration once it > becomes a tautology. But probably we will just git-grep for > preferredScrollerStyle since lots of things do this. Done. https://codereview.chromium.org/1671313002/diff/20001/ui/views/controls/scrol... File ui/views/controls/scrollbar/native_cocoa_scroll_bar.h (right): https://codereview.chromium.org/1671313002/diff/20001/ui/views/controls/scrol... ui/views/controls/scrollbar/native_cocoa_scroll_bar.h:34: class VIEWS_EXPORT NativeCocoaScrollBarViews On 2016/02/08 00:31:15, tapted wrote: > nit: needs a comment > > also I'm not sure about `Native` :). Usually that means we're using some native > drawing code, but we're actually emulating it. So perhaps `Emulated`? Or > something else - I'm not fully sold on Emulated either > > Note there is http://crbug.com/93103 "Get rid of NativeScrollBar" (or, just > rename/merge it really) That's true, I'm not sure what the best name it is atm. Let's rename it after we figure out what to do with NativeScrollBar https://codereview.chromium.org/1671313002/diff/20001/ui/views/controls/scrol... ui/views/controls/scrollbar/native_cocoa_scroll_bar.h:61: NSScrollerStyle scroller_style_; On 2016/02/08 00:31:15, tapted wrote: > needs an initializer, since UpdateScrollStyle checks the current value. > NSScrollerStyle is typedef NSInteger, so perhaps just initialize to `-1` to > force the setup the first time UpdateScrollStyle is called. Then you can > probably get rid of the separate |init_| data member. (But comment here like > > // The current scroller style, or -1 before it is initialized. > NSScrollerStyle scroller_style_ = -1; > > > But! The initialization flow might really need to be tweaked a bit. E.g. it > should never animate. Perhaps the constructor should just call [bridge_ > getPreferredScrollerStyle] and set properties appropriately without animating Hm, that is true. I can't set it to -1 since it's outside the enum range However, I just ended up calling getPreferredScrollerStyle instead https://codereview.chromium.org/1671313002/diff/20001/ui/views/controls/scrol... ui/views/controls/scrollbar/native_cocoa_scroll_bar.h:70: double scroller_opacity_; On 2016/02/08 00:31:15, tapted wrote: > This should probably be initialized too Done. https://codereview.chromium.org/1671313002/diff/20001/ui/views/controls/scrol... ui/views/controls/scrollbar/native_cocoa_scroll_bar.h:76: bool init_; On 2016/02/08 00:31:15, tapted wrote: > (remove[ or initialize]) Done. https://codereview.chromium.org/1671313002/diff/20001/ui/views/controls/scrol... File ui/views/controls/scrollbar/native_cocoa_scroll_bar.mm (right): https://codereview.chromium.org/1671313002/diff/20001/ui/views/controls/scrol... ui/views/controls/scrollbar/native_cocoa_scroll_bar.mm:14: // How long we should wait before hiding the scrollbar. On 2016/02/08 00:31:15, tapted wrote: > nit: blank line before Done. https://codereview.chromium.org/1671313002/diff/20001/ui/views/controls/scrol... ui/views/controls/scrollbar/native_cocoa_scroll_bar.mm:22: } On 2016/02/08 00:31:15, tapted wrote: > nit: } // namespace > > (also, blank line before) Done. https://codereview.chromium.org/1671313002/diff/20001/ui/views/controls/scrol... ui/views/controls/scrollbar/native_cocoa_scroll_bar.mm:46: UpdateScrollStyle(); On 2016/02/08 00:31:15, tapted wrote: > So I think we can simplify things a bit and get a much smoother animation by > using a ui::Layer. > > Here, you would call > > SetPaintsToLayer(true); > > (invoking the method on views::View). > > Then > > GetLayer()->SetAnimator()->( > new ui::LayerAnimator(base::TimeDelta::FromMilliseconds(kFadeDurationMs)); > > > (the gfx::LinearAnimation shouldn't be needed) > > Then, see other comments. > > > However, there is a tricky part, and that's what to do with the gutter with > legacy scrollbars -- it shouldn't fade out, but the thumb should (partially). I > think the best way to do it would be to add the scrollbar thumb as a subview > with its own layer. Done. https://codereview.chromium.org/1671313002/diff/20001/ui/views/controls/scrol... ui/views/controls/scrollbar/native_cocoa_scroll_bar.mm:100: animation_.Start(); On 2016/02/08 00:31:15, tapted wrote: > Then here, you would just call > > GetLayer()->GetAnimator()->SetOpacity(0.0); > > You shouldn't need the SchedulePaint call Done. https://codereview.chromium.org/1671313002/diff/20001/ui/views/controls/scrol... ui/views/controls/scrollbar/native_cocoa_scroll_bar.mm:105: animation_.Stop(); On 2016/02/08 00:31:15, tapted wrote: > And here, > GetLayer()->SetOpacity(1.0); Done. https://codereview.chromium.org/1671313002/diff/20001/ui/views/controls/scrol... File ui/views/controls/scrollbar/native_scroll_bar.cc (right): https://codereview.chromium.org/1671313002/diff/20001/ui/views/controls/scrol... ui/views/controls/scrollbar/native_scroll_bar.cc:67: // Create a method to CreateWrapper and then override that shit On 2016/02/08 00:31:15, tapted wrote: > (lol) whoops. https://codereview.chromium.org/1671313002/diff/20001/ui/views/controls/scrol... File ui/views/controls/scrollbar/native_scroll_bar.h (right): https://codereview.chromium.org/1671313002/diff/20001/ui/views/controls/scrol... ui/views/controls/scrollbar/native_scroll_bar.h:66: NativeScrollBarWrapper* native_wrapper_; On 2016/02/08 00:31:15, tapted wrote: > hrm - should this be scoped_ptr? I can't figure out who owns the returned > pointer from CreateWrapper.. Yeah, the structure of the NativeScrollbar is quite confusing but basically NativeScrollBar owns native_wrapper_. https://codereview.chromium.org/1671313002/diff/20001/ui/views/controls/scrol... File ui/views/controls/scrollbar/native_scroll_bar_views.cc (right): https://codereview.chromium.org/1671313002/diff/20001/ui/views/controls/scrol... ui/views/controls/scrollbar/native_scroll_bar_views.cc:55: explicit ScrollBarThumb(BaseScrollBar* scroll_bar, On 2016/02/08 00:31:15, tapted wrote: > nit: explicit not needed Done. https://codereview.chromium.org/1671313002/diff/20001/ui/views/controls/scrol... ui/views/controls/scrollbar/native_scroll_bar_views.cc:71: NativeScrollBarWrapper* wrapper_; On 2016/02/08 00:31:15, tapted wrote: > hm. I wonder if ScrollBar should expose a GetNativeWrapper virtual method. Or, > does anything other than NativeScrollBarViews call the constructor? (can you > just pass a NativeScrollBar, add a NativeScrollBar::native_wrapper() accessor, > and change the type of |scroll_bar_| to NativeScrollBar? ScrollBar doesn't contain the native_wrapper so that wouldn't work. Changing it to NativeScrollBar would work though Anyway, I just removed the wrapper since we no longer need it https://codereview.chromium.org/1671313002/diff/20001/ui/views/controls/scrol... ui/views/controls/scrollbar/native_scroll_bar_views.cc:114: params.scrollbar_arrow.overlay = wrapper_->GetOverlayParams(); On 2016/02/08 00:31:15, tapted wrote: > check for a null |wrapper_|? Acknowledged. https://codereview.chromium.org/1671313002/diff/20001/ui/views/controls/scrol... ui/views/controls/scrollbar/native_scroll_bar_views.cc:197: params.scrollbar_thumb.overlay = wrapper_->GetOverlayParams(); On 2016/02/08 00:31:15, tapted wrote: > null check? Acknowledged. https://codereview.chromium.org/1671313002/diff/60001/ui/views/cocoa/views_sc... File ui/views/cocoa/views_scrollbar_bridge.mm (right): https://codereview.chromium.org/1671313002/diff/60001/ui/views/cocoa/views_sc... ui/views/cocoa/views_scrollbar_bridge.mm:37: [[NSNotificationCenter defaultCenter] removeObserver:self]; This might be unnecessary? Especially since we're removing it in setDelegate
Some initial feedback since I'm about to disappear into talks for a couple of hours. I'll need to dive a bit deeper to figure out what's needed for NativeScrollBar (if anything, at this stage) https://codereview.chromium.org/1671313002/diff/60001/ui/views/cocoa/views_sc... File ui/views/cocoa/views_scrollbar_bridge.mm (right): https://codereview.chromium.org/1671313002/diff/60001/ui/views/cocoa/views_sc... ui/views/cocoa/views_scrollbar_bridge.mm:37: [[NSNotificationCenter defaultCenter] removeObserver:self]; On 2016/02/09 21:21:26, spqchan wrote: > This might be unnecessary? Especially since we're removing it in setDelegate Yeah - I think it's redundant with the DCHECK above. But I think we can use this simpler form of removeObserver in setDelegate. https://codereview.chromium.org/1671313002/diff/60001/ui/views/controls/scrol... File ui/views/controls/scroll_view.h (right): https://codereview.chromium.org/1671313002/diff/60001/ui/views/controls/scrol... ui/views/controls/scroll_view.h:136: scoped_ptr<ScrollBar> horiz_sb_; I don't immediately grok this bit (I may just need to look closer), or why the same isn't done for vert_sb_; - it might just need a comment to clarify. I have a vague memory of looking at this in the past and wondering why these were not scoped_ptr, and then realising why. I think it was because ScrollView needed to `delete horiz_sb_;` to remove the view from the view hierarchy, but it was invalid for the the ~ScrollView() destructor to do that because the super-views::View (which *actually* owns the view) may have already done the `delete horiz_sb_`, and left ScrollView with an invalid pointer. https://codereview.chromium.org/1671313002/diff/60001/ui/views/controls/scrol... File ui/views/controls/scrollbar/native_cocoa_scroll_bar.mm (right): https://codereview.chromium.org/1671313002/diff/60001/ui/views/controls/scrol... ui/views/controls/scrollbar/native_cocoa_scroll_bar.mm:55: NativeCocoaScrollBarViews::~NativeCocoaScrollBarViews() {} [bridge_ setDelegate:nullptr]; https://codereview.chromium.org/1671313002/diff/60001/ui/views/controls/scrol... ui/views/controls/scrollbar/native_cocoa_scroll_bar.mm:94: } // end of views namespace -> // namespace views https://codereview.chromium.org/1671313002/diff/60001/ui/views/controls/scrol... File ui/views/controls/scrollbar/native_scroll_bar_views.cc (right): https://codereview.chromium.org/1671313002/diff/60001/ui/views/controls/scrol... ui/views/controls/scrollbar/native_scroll_bar_views.cc:52: ScrollBarThumb(BaseScrollBar* scroll_bar); nit: explicit needed back now :) https://codereview.chromium.org/1671313002/diff/60001/ui/views/style/platform... File ui/views/style/platform_style_mac.mm (right): https://codereview.chromium.org/1671313002/diff/60001/ui/views/style/platform... ui/views/style/platform_style_mac.mm:24: scoped_ptr<ScrollBar> PlatformStyle::CreateScrollBar(bool is_horizontal) { I think platform_style.cc will need an implementation of this for non-mac platforms.
Sounds good. I also have a new patch for your review https://codereview.chromium.org/1671313002/diff/60001/ui/views/cocoa/views_sc... File ui/views/cocoa/views_scrollbar_bridge.mm (right): https://codereview.chromium.org/1671313002/diff/60001/ui/views/cocoa/views_sc... ui/views/cocoa/views_scrollbar_bridge.mm:37: [[NSNotificationCenter defaultCenter] removeObserver:self]; On 2016/02/09 23:51:48, tapted wrote: > On 2016/02/09 21:21:26, spqchan wrote: > > This might be unnecessary? Especially since we're removing it in setDelegate > > Yeah - I think it's redundant with the DCHECK above. But I think we can use this > simpler form of removeObserver in setDelegate. Gotcha, removed it https://codereview.chromium.org/1671313002/diff/60001/ui/views/controls/scrol... File ui/views/controls/scroll_view.h (right): https://codereview.chromium.org/1671313002/diff/60001/ui/views/controls/scrol... ui/views/controls/scroll_view.h:136: scoped_ptr<ScrollBar> horiz_sb_; On 2016/02/09 23:51:48, tapted wrote: > I don't immediately grok this bit (I may just need to look closer), or why the > same isn't done for vert_sb_; - it might just need a comment to clarify. > > I have a vague memory of looking at this in the past and wondering why these > were not scoped_ptr, and then realising why. I think it was because ScrollView > needed to `delete horiz_sb_;` to remove the view from the view hierarchy, but it > was invalid for the the ~ScrollView() destructor to do that because the > super-views::View (which *actually* owns the view) may have already done the > `delete horiz_sb_`, and left ScrollView with an invalid pointer. Sorry, this wasn't supposed to be here. This is actually test code (I was changing horiz_sb to the NativeCocoaScrollbar so I could compare it with vert_sb). I was considering on replacing both of them with scoped_ptrs in the future, but it looks like I should leave them as just pointers. Anyway, I just removed it https://codereview.chromium.org/1671313002/diff/60001/ui/views/controls/scrol... File ui/views/controls/scrollbar/native_cocoa_scroll_bar.mm (right): https://codereview.chromium.org/1671313002/diff/60001/ui/views/controls/scrol... ui/views/controls/scrollbar/native_cocoa_scroll_bar.mm:55: NativeCocoaScrollBarViews::~NativeCocoaScrollBarViews() {} On 2016/02/09 23:51:48, tapted wrote: > [bridge_ setDelegate:nullptr]; Done. https://codereview.chromium.org/1671313002/diff/60001/ui/views/controls/scrol... ui/views/controls/scrollbar/native_cocoa_scroll_bar.mm:94: } // end of views namespace On 2016/02/09 23:51:48, tapted wrote: > -> // namespace views Done. https://codereview.chromium.org/1671313002/diff/60001/ui/views/controls/scrol... File ui/views/controls/scrollbar/native_scroll_bar_views.cc (right): https://codereview.chromium.org/1671313002/diff/60001/ui/views/controls/scrol... ui/views/controls/scrollbar/native_scroll_bar_views.cc:52: ScrollBarThumb(BaseScrollBar* scroll_bar); On 2016/02/09 23:51:48, tapted wrote: > nit: explicit needed back now :) Done. https://codereview.chromium.org/1671313002/diff/60001/ui/views/style/platform... File ui/views/style/platform_style_mac.mm (right): https://codereview.chromium.org/1671313002/diff/60001/ui/views/style/platform... ui/views/style/platform_style_mac.mm:24: scoped_ptr<ScrollBar> PlatformStyle::CreateScrollBar(bool is_horizontal) { On 2016/02/09 23:51:48, tapted wrote: > I think platform_style.cc will need an implementation of this for non-mac > platforms. Done.
I'm currently thinking NativeScrollBarViews might not actually be providing us with much. It might be better to inherit from OverlayScrollBar (or perhaps even BaseScrollBar) and implement just what we need. For example we would need a ScrollBarThumb, but not a ScrollBarButton, and the Thumb doesn't have a "gripper", so that part of NativeScrollBarViews's (private) ScrollBarThumb class is unsued on Mac. There might be a little bit of repetition, but it would decouple Mac better for the future changes we still need, and remove the confusion about exactly how `NativeScrollBar` may need to be refactored. https://codereview.chromium.org/1671313002/diff/20001/ui/views/controls/scrol... File ui/views/controls/scrollbar/native_scroll_bar.h (right): https://codereview.chromium.org/1671313002/diff/20001/ui/views/controls/scrol... ui/views/controls/scrollbar/native_scroll_bar.h:66: NativeScrollBarWrapper* native_wrapper_; On 2016/02/09 21:21:26, spqchan wrote: > On 2016/02/08 00:31:15, tapted wrote: > > hrm - should this be scoped_ptr? I can't figure out who owns the returned > > pointer from CreateWrapper.. > > Yeah, the structure of the NativeScrollbar is quite confusing but basically > NativeScrollBar owns native_wrapper_. Ahhhhhhhh. The wrapper is a views::View (i.e. GetView() *must* return |this|), the wrapper is then inserted into the view hierarchy and gets owned by that. Crazy. https://codereview.chromium.org/1671313002/diff/80001/ui/views/cocoa/views_sc... File ui/views/cocoa/views_scrollbar_bridge.h (right): https://codereview.chromium.org/1671313002/diff/80001/ui/views/cocoa/views_sc... ui/views/cocoa/views_scrollbar_bridge.h:16: virtual void OnScrollerStyleChanged() = 0; nit: needs a comment. I'd say something like, `Invoked when the system informs the process that the preferred scroller style has changed (e.g. external mouse plugged in, or changed in System Preferences). https://codereview.chromium.org/1671313002/diff/80001/ui/views/cocoa/views_sc... ui/views/cocoa/views_scrollbar_bridge.h:31: - (NSScrollerStyle)getPreferredScrollerStyle; + class function? https://codereview.chromium.org/1671313002/diff/80001/ui/views/cocoa/views_sc... File ui/views/cocoa/views_scrollbar_bridge.mm (right): https://codereview.chromium.org/1671313002/diff/80001/ui/views/cocoa/views_sc... ui/views/cocoa/views_scrollbar_bridge.mm:8: #include "base/mac/sdk_forward_declarations.h" nit: import https://codereview.chromium.org/1671313002/diff/80001/ui/views/cocoa/views_sc... ui/views/cocoa/views_scrollbar_bridge.mm:23: removeObserver:self i think we can omit the name and object arguments -- that seems more conventional in chrome https://codereview.chromium.org/1671313002/diff/80001/ui/views/cocoa/views_sc... ui/views/cocoa/views_scrollbar_bridge.mm:41: delegate_->OnScrollerStyleChanged(); this should check for a null delegate (which is fine - it's the usual way we solve the different lifetime models) https://codereview.chromium.org/1671313002/diff/80001/ui/views/controls/scrol... File ui/views/controls/scrollbar/native_cocoa_scroll_bar.h (right): https://codereview.chromium.org/1671313002/diff/80001/ui/views/controls/scrol... ui/views/controls/scrollbar/native_cocoa_scroll_bar.h:34: class VIEWS_EXPORT NativeCocoaScrollBarViews I think this should be NativeScrollBarViewsMac it should have a brief comment to describing what behaviours its responsible for https://codereview.chromium.org/1671313002/diff/80001/ui/views/controls/scrol... ui/views/controls/scrollbar/native_cocoa_scroll_bar.h:35: : public NativeScrollBarViews, We should override NativeScrollBarViews::OnPaint() so that the track isn't painted for the overlay style -- that should be straightforward (I think we just do nothing or call NativeScrollBarViews::OnPaint()). https://codereview.chromium.org/1671313002/diff/80001/ui/views/controls/scrol... ui/views/controls/scrollbar/native_cocoa_scroll_bar.h:41: // BaseScrollBar override nit: Lately views OWNERS have been asking me to go with just // BaseScrollBar: for these (I guess `override[s]` is kinda redundant now it's on every method and part of the C++11 language https://codereview.chromium.org/1671313002/diff/80001/ui/views/controls/scrol... ui/views/controls/scrollbar/native_cocoa_scroll_bar.h:43: Hm... how do we fix the layout so that the content renders underneath in proper overlay style? It might be really simple... I tried borrowing some ideas from OverlayScrollBar, like just overriding, // ScrollBar: int GetLayoutSize() const override; int GetContentOverlapSize() const override; to just return 0 for layoutsize and NativeScrollBarViews::GetLayoutSize() for GetContentOverlapSize(). But that didn't seem to be enough :/ https://codereview.chromium.org/1671313002/diff/80001/ui/views/controls/scrol... File ui/views/controls/scrollbar/native_cocoa_scroll_bar.mm (right): https://codereview.chromium.org/1671313002/diff/80001/ui/views/controls/scrol... ui/views/controls/scrollbar/native_cocoa_scroll_bar.mm:22: } nit: blank line before https://codereview.chromium.org/1671313002/diff/80001/ui/views/controls/scrol... ui/views/controls/scrollbar/native_cocoa_scroll_bar.mm:26: //////////////////////////////// The convention in views seems to be more commonly like //////////////////////////////////////////////////////////////////////////////// // NativeCocoaScrollBar for these. See e.g native_scroll_bar_views.cc and widget.cc https://codereview.chromium.org/1671313002/diff/80001/ui/views/controls/scrol... ui/views/controls/scrollbar/native_cocoa_scroll_bar.mm:71: bool NativeCocoaScrollBarViews::OnScroll(float dx, float dy) { we also need to prevent the scroller fading out while it's being dragged.. (more ideas to borrow from OverlayScrollBar) https://codereview.chromium.org/1671313002/diff/80001/ui/views/controls/scrol... ui/views/controls/scrollbar/native_cocoa_scroll_bar.mm:72: bool didScroll = BaseScrollBar::OnScroll(dx, dy); didScroll -> did_scroll https://codereview.chromium.org/1671313002/diff/80001/ui/views/controls/scrol... ui/views/controls/scrollbar/native_cocoa_scroll_bar.mm:89: layer()->GetAnimator()->SetOpacity(0.0); confusingly, I discovered that layer()->SetOpacity always goes through the animator, so the GetAnimator() bit here is redundant. Unfortunately it also means that when we *show* the scrollbar it's also using an animator. So I think we need to do (in both Hide and Show) layer()->GetAnimator()->SetTransitionDuration(base::TimeDelta() or base::TimeDelta::FromMilliseconds(kScrollbarHideTimeoutMs)); it also means the layer()->SetAnimator call in the constructor is obsolete, since the first call to GetAnimator creates one with a zero-duration automatically https://codereview.chromium.org/1671313002/diff/80001/ui/views/controls/scrol... File ui/views/controls/scrollbar/native_scroll_bar_views.cc (right): https://codereview.chromium.org/1671313002/diff/80001/ui/views/controls/scrol... ui/views/controls/scrollbar/native_scroll_bar_views.cc:32: explicit ScrollBarButton(ButtonListener* listener, Type type); oops - I think we can revert all the diffs in this file. (The ScrollBarThumb constructor should be explicit, but it's orthogonal to this CL.) https://codereview.chromium.org/1671313002/diff/80001/ui/views/style/platform... File ui/views/style/platform_style.h (right): https://codereview.chromium.org/1671313002/diff/80001/ui/views/style/platform... ui/views/style/platform_style.h:30: static scoped_ptr<ScrollBar> CreateScrollBar(bool is_horizontal); nit: needs a comment. (Also.. this should perhaps return a pointer, but I like this consistency. It means scroll_view.cc will need to do something like horiz_sb_(PlatformStyle::CreateScrollBar(true).release()), vert_sb_(PlatformStyle::CreateScrollBar(false).release()), which is fine - we should do that in this CL as well.
On 2016/02/11 08:46:18, tapted wrote: > I'm currently thinking NativeScrollBarViews might not actually be providing us > with much. It might be better to inherit from OverlayScrollBar (or perhaps even > BaseScrollBar) and implement just what we need. > > For example we would need a ScrollBarThumb, but not a ScrollBarButton, and the > Thumb doesn't have a "gripper", so that part of NativeScrollBarViews's (private) > ScrollBarThumb class is unsued on Mac. > > There might be a little bit of repetition, but it would decouple Mac better for > the future changes we still need, and remove the confusion about exactly how > `NativeScrollBar` may need to be refactored. > > https://codereview.chromium.org/1671313002/diff/20001/ui/views/controls/scrol... > File ui/views/controls/scrollbar/native_scroll_bar.h (right): > > https://codereview.chromium.org/1671313002/diff/20001/ui/views/controls/scrol... > ui/views/controls/scrollbar/native_scroll_bar.h:66: NativeScrollBarWrapper* > native_wrapper_; > On 2016/02/09 21:21:26, spqchan wrote: > > On 2016/02/08 00:31:15, tapted wrote: > > > hrm - should this be scoped_ptr? I can't figure out who owns the returned > > > pointer from CreateWrapper.. > > > > Yeah, the structure of the NativeScrollbar is quite confusing but basically > > NativeScrollBar owns native_wrapper_. > > Ahhhhhhhh. The wrapper is a views::View (i.e. GetView() *must* return |this|), > the wrapper is then inserted into the view hierarchy and gets owned by that. > Crazy. > > https://codereview.chromium.org/1671313002/diff/80001/ui/views/cocoa/views_sc... > File ui/views/cocoa/views_scrollbar_bridge.h (right): > > https://codereview.chromium.org/1671313002/diff/80001/ui/views/cocoa/views_sc... > ui/views/cocoa/views_scrollbar_bridge.h:16: virtual void > OnScrollerStyleChanged() = 0; > nit: needs a comment. I'd say something like, `Invoked when the system informs > the process that the preferred scroller style has changed (e.g. external mouse > plugged in, or changed in System Preferences). > > https://codereview.chromium.org/1671313002/diff/80001/ui/views/cocoa/views_sc... > ui/views/cocoa/views_scrollbar_bridge.h:31: - > (NSScrollerStyle)getPreferredScrollerStyle; > + class function? > > https://codereview.chromium.org/1671313002/diff/80001/ui/views/cocoa/views_sc... > File ui/views/cocoa/views_scrollbar_bridge.mm (right): > > https://codereview.chromium.org/1671313002/diff/80001/ui/views/cocoa/views_sc... > ui/views/cocoa/views_scrollbar_bridge.mm:8: #include > "base/mac/sdk_forward_declarations.h" > nit: import > > https://codereview.chromium.org/1671313002/diff/80001/ui/views/cocoa/views_sc... > ui/views/cocoa/views_scrollbar_bridge.mm:23: removeObserver:self > i think we can omit the name and object arguments -- that seems more > conventional in chrome > > https://codereview.chromium.org/1671313002/diff/80001/ui/views/cocoa/views_sc... > ui/views/cocoa/views_scrollbar_bridge.mm:41: > delegate_->OnScrollerStyleChanged(); > this should check for a null delegate (which is fine - it's the usual way we > solve the different lifetime models) > > https://codereview.chromium.org/1671313002/diff/80001/ui/views/controls/scrol... > File ui/views/controls/scrollbar/native_cocoa_scroll_bar.h (right): > > https://codereview.chromium.org/1671313002/diff/80001/ui/views/controls/scrol... > ui/views/controls/scrollbar/native_cocoa_scroll_bar.h:34: class VIEWS_EXPORT > NativeCocoaScrollBarViews > I think this should be > > NativeScrollBarViewsMac > > it should have a brief comment to describing what behaviours its responsible for > > https://codereview.chromium.org/1671313002/diff/80001/ui/views/controls/scrol... > ui/views/controls/scrollbar/native_cocoa_scroll_bar.h:35: : public > NativeScrollBarViews, > We should override NativeScrollBarViews::OnPaint() so that the track isn't > painted for the overlay style -- that should be straightforward (I think we just > do nothing or call NativeScrollBarViews::OnPaint()). > > https://codereview.chromium.org/1671313002/diff/80001/ui/views/controls/scrol... > ui/views/controls/scrollbar/native_cocoa_scroll_bar.h:41: // BaseScrollBar > override > nit: Lately views OWNERS have been asking me to go with just > > // BaseScrollBar: > > for these (I guess `override[s]` is kinda redundant now it's on every method and > part of the C++11 language > > https://codereview.chromium.org/1671313002/diff/80001/ui/views/controls/scrol... > ui/views/controls/scrollbar/native_cocoa_scroll_bar.h:43: > Hm... how do we fix the layout so that the content renders underneath in proper > overlay style? It might be really simple... I tried borrowing some ideas from > OverlayScrollBar, like just overriding, > > // ScrollBar: > int GetLayoutSize() const override; > int GetContentOverlapSize() const override; > > to just return 0 for layoutsize and NativeScrollBarViews::GetLayoutSize() for > GetContentOverlapSize(). But that didn't seem to be enough :/ > > https://codereview.chromium.org/1671313002/diff/80001/ui/views/controls/scrol... > File ui/views/controls/scrollbar/native_cocoa_scroll_bar.mm (right): > > https://codereview.chromium.org/1671313002/diff/80001/ui/views/controls/scrol... > ui/views/controls/scrollbar/native_cocoa_scroll_bar.mm:22: } > nit: blank line before > > https://codereview.chromium.org/1671313002/diff/80001/ui/views/controls/scrol... > ui/views/controls/scrollbar/native_cocoa_scroll_bar.mm:26: > //////////////////////////////// > The convention in views seems to be more commonly like > > //////////////////////////////////////////////////////////////////////////////// > // NativeCocoaScrollBar > > for these. See e.g native_scroll_bar_views.cc and widget.cc > > https://codereview.chromium.org/1671313002/diff/80001/ui/views/controls/scrol... > ui/views/controls/scrollbar/native_cocoa_scroll_bar.mm:71: bool > NativeCocoaScrollBarViews::OnScroll(float dx, float dy) { > we also need to prevent the scroller fading out while it's being dragged.. > (more ideas to borrow from OverlayScrollBar) > > https://codereview.chromium.org/1671313002/diff/80001/ui/views/controls/scrol... > ui/views/controls/scrollbar/native_cocoa_scroll_bar.mm:72: bool didScroll = > BaseScrollBar::OnScroll(dx, dy); > didScroll -> did_scroll > > https://codereview.chromium.org/1671313002/diff/80001/ui/views/controls/scrol... > ui/views/controls/scrollbar/native_cocoa_scroll_bar.mm:89: > layer()->GetAnimator()->SetOpacity(0.0); > confusingly, I discovered that layer()->SetOpacity always goes through the > animator, so the GetAnimator() bit here is redundant. Unfortunately it also > means that when we *show* the scrollbar it's also using an animator. So I think > we need to do (in both Hide and Show) > > layer()->GetAnimator()->SetTransitionDuration(base::TimeDelta() or > base::TimeDelta::FromMilliseconds(kScrollbarHideTimeoutMs)); > > it also means the layer()->SetAnimator call in the constructor is obsolete, > since the first call to GetAnimator creates one with a zero-duration > automatically > > https://codereview.chromium.org/1671313002/diff/80001/ui/views/controls/scrol... > File ui/views/controls/scrollbar/native_scroll_bar_views.cc (right): > > https://codereview.chromium.org/1671313002/diff/80001/ui/views/controls/scrol... > ui/views/controls/scrollbar/native_scroll_bar_views.cc:32: explicit > ScrollBarButton(ButtonListener* listener, Type type); > oops - I think we can revert all the diffs in this file. (The ScrollBarThumb > constructor should be explicit, but it's orthogonal to this CL.) > > https://codereview.chromium.org/1671313002/diff/80001/ui/views/style/platform... > File ui/views/style/platform_style.h (right): > > https://codereview.chromium.org/1671313002/diff/80001/ui/views/style/platform... > ui/views/style/platform_style.h:30: static scoped_ptr<ScrollBar> > CreateScrollBar(bool is_horizontal); > nit: needs a comment. (Also.. this should perhaps return a pointer, but I like > this consistency. It means scroll_view.cc will need to do something like > > horiz_sb_(PlatformStyle::CreateScrollBar(true).release()), > vert_sb_(PlatformStyle::CreateScrollBar(false).release()), > > which is fine - we should do that in this CL as well. Sure thing, I'll make a switch then. It should be simple, because I was inheriting from OverlayScrollbars in the beginning and I still have the code for that. I think I'll inherit from BaseScrollbars instead though, because OverlayScrollbars lack the scrolltrack and it doesn't use LayerAnimations. With BaseScrollbars, it'll be more flexible.
I made the switch to CocoaScrollbar, which inherits from BaseScrollBar. I migrated the code from NativeCocoaScrollbar into this one. I also copied a bit of drawing code from NativeThemeMac. Few other changes: - Scrollbar track now shows only if you're hovering the scrollbar thumb in overlay style. - Scrollbar will no longer disappear if you try to drag it. https://codereview.chromium.org/1671313002/diff/80001/ui/views/cocoa/views_sc... File ui/views/cocoa/views_scrollbar_bridge.h (right): https://codereview.chromium.org/1671313002/diff/80001/ui/views/cocoa/views_sc... ui/views/cocoa/views_scrollbar_bridge.h:16: virtual void OnScrollerStyleChanged() = 0; On 2016/02/11 08:46:17, tapted wrote: > nit: needs a comment. I'd say something like, `Invoked when the system informs > the process that the preferred scroller style has changed (e.g. external mouse > plugged in, or changed in System Preferences). Done. https://codereview.chromium.org/1671313002/diff/80001/ui/views/cocoa/views_sc... ui/views/cocoa/views_scrollbar_bridge.h:31: - (NSScrollerStyle)getPreferredScrollerStyle; On 2016/02/11 08:46:17, tapted wrote: > + class function? Done. https://codereview.chromium.org/1671313002/diff/80001/ui/views/cocoa/views_sc... File ui/views/cocoa/views_scrollbar_bridge.mm (right): https://codereview.chromium.org/1671313002/diff/80001/ui/views/cocoa/views_sc... ui/views/cocoa/views_scrollbar_bridge.mm:8: #include "base/mac/sdk_forward_declarations.h" On 2016/02/11 08:46:17, tapted wrote: > nit: import Done. https://codereview.chromium.org/1671313002/diff/80001/ui/views/cocoa/views_sc... ui/views/cocoa/views_scrollbar_bridge.mm:23: removeObserver:self On 2016/02/11 08:46:17, tapted wrote: > i think we can omit the name and object arguments -- that seems more > conventional in chrome Done. https://codereview.chromium.org/1671313002/diff/80001/ui/views/cocoa/views_sc... ui/views/cocoa/views_scrollbar_bridge.mm:41: delegate_->OnScrollerStyleChanged(); On 2016/02/11 08:46:17, tapted wrote: > this should check for a null delegate (which is fine - it's the usual way we > solve the different lifetime models) Done. https://codereview.chromium.org/1671313002/diff/80001/ui/views/controls/scrol... File ui/views/controls/scrollbar/native_cocoa_scroll_bar.h (right): https://codereview.chromium.org/1671313002/diff/80001/ui/views/controls/scrol... ui/views/controls/scrollbar/native_cocoa_scroll_bar.h:34: class VIEWS_EXPORT NativeCocoaScrollBarViews On 2016/02/11 08:46:17, tapted wrote: > I think this should be > > NativeScrollBarViewsMac > > it should have a brief comment to describing what behaviours its responsible for I just switched back to CocoaScrollBar, do you think that name is alright? If not, I can change it to NativeScrollBarViewsMac https://codereview.chromium.org/1671313002/diff/80001/ui/views/controls/scrol... ui/views/controls/scrollbar/native_cocoa_scroll_bar.h:35: : public NativeScrollBarViews, On 2016/02/11 08:46:17, tapted wrote: > We should override NativeScrollBarViews::OnPaint() so that the track isn't > painted for the overlay style -- that should be straightforward (I think we just > do nothing or call NativeScrollBarViews::OnPaint()). Did it for CocoaScrollBar. https://codereview.chromium.org/1671313002/diff/80001/ui/views/controls/scrol... ui/views/controls/scrollbar/native_cocoa_scroll_bar.h:41: // BaseScrollBar override On 2016/02/11 08:46:17, tapted wrote: > nit: Lately views OWNERS have been asking me to go with just > > // BaseScrollBar: > > for these (I guess `override[s]` is kinda redundant now it's on every method and > part of the C++11 language Done. https://codereview.chromium.org/1671313002/diff/80001/ui/views/controls/scrol... ui/views/controls/scrollbar/native_cocoa_scroll_bar.h:43: On 2016/02/11 08:46:17, tapted wrote: > Hm... how do we fix the layout so that the content renders underneath in proper > overlay style? It might be really simple... I tried borrowing some ideas from > OverlayScrollBar, like just overriding, > > // ScrollBar: > int GetLayoutSize() const override; > int GetContentOverlapSize() const override; > > to just return 0 for layoutsize and NativeScrollBarViews::GetLayoutSize() for > GetContentOverlapSize(). But that didn't seem to be enough :/ I used return 0 for GetLayoutSize() I used the scrollbar width instead (pretty much the same thing as GetLayoutSize()) and it looks like it's turning out okay. There's a corner issue right now that I'll have to investigate though. https://codereview.chromium.org/1671313002/diff/80001/ui/views/controls/scrol... File ui/views/controls/scrollbar/native_cocoa_scroll_bar.mm (right): https://codereview.chromium.org/1671313002/diff/80001/ui/views/controls/scrol... ui/views/controls/scrollbar/native_cocoa_scroll_bar.mm:22: } On 2016/02/11 08:46:18, tapted wrote: > nit: blank line before Done. https://codereview.chromium.org/1671313002/diff/80001/ui/views/controls/scrol... ui/views/controls/scrollbar/native_cocoa_scroll_bar.mm:26: //////////////////////////////// On 2016/02/11 08:46:17, tapted wrote: > The convention in views seems to be more commonly like > > //////////////////////////////////////////////////////////////////////////////// > // NativeCocoaScrollBar > > for these. See e.g native_scroll_bar_views.cc and widget.cc Done. https://codereview.chromium.org/1671313002/diff/80001/ui/views/controls/scrol... ui/views/controls/scrollbar/native_cocoa_scroll_bar.mm:71: bool NativeCocoaScrollBarViews::OnScroll(float dx, float dy) { On 2016/02/11 08:46:17, tapted wrote: > we also need to prevent the scroller fading out while it's being dragged.. > (more ideas to borrow from OverlayScrollBar) OverlayScrollBar is a bit different since it shows the scroller when the mouse is inside the scrollview. I played around for a bit and got it to work using MouseEnter and MouseExit handlers on the scrollbar thumb. I'm considering on setting OnThumbStateChanged in BaseScrollBar to virtual and then overriding instead though, since doing so might give us something a bit cleaner. https://codereview.chromium.org/1671313002/diff/80001/ui/views/controls/scrol... ui/views/controls/scrollbar/native_cocoa_scroll_bar.mm:72: bool didScroll = BaseScrollBar::OnScroll(dx, dy); On 2016/02/11 08:46:18, tapted wrote: > didScroll -> did_scroll Done. https://codereview.chromium.org/1671313002/diff/80001/ui/views/controls/scrol... ui/views/controls/scrollbar/native_cocoa_scroll_bar.mm:89: layer()->GetAnimator()->SetOpacity(0.0); On 2016/02/11 08:46:18, tapted wrote: > confusingly, I discovered that layer()->SetOpacity always goes through the > animator, so the GetAnimator() bit here is redundant. Unfortunately it also > means that when we *show* the scrollbar it's also using an animator. So I think > we need to do (in both Hide and Show) > > layer()->GetAnimator()->SetTransitionDuration(base::TimeDelta() or > base::TimeDelta::FromMilliseconds(kScrollbarHideTimeoutMs)); > > it also means the layer()->SetAnimator call in the constructor is obsolete, > since the first call to GetAnimator creates one with a zero-duration > automatically Heads up, SetTransitionDuration was private so I had to expose it. I considered setting the animator each time Show/Hide gets called, but it doesn't seem efficient https://codereview.chromium.org/1671313002/diff/80001/ui/views/controls/scrol... File ui/views/controls/scrollbar/native_scroll_bar_views.cc (right): https://codereview.chromium.org/1671313002/diff/80001/ui/views/controls/scrol... ui/views/controls/scrollbar/native_scroll_bar_views.cc:32: explicit ScrollBarButton(ButtonListener* listener, Type type); On 2016/02/11 08:46:18, tapted wrote: > oops - I think we can revert all the diffs in this file. (The ScrollBarThumb > constructor should be explicit, but it's orthogonal to this CL.) I removed all the diffs involving NativeScrollBar https://codereview.chromium.org/1671313002/diff/80001/ui/views/style/platform... File ui/views/style/platform_style.h (right): https://codereview.chromium.org/1671313002/diff/80001/ui/views/style/platform... ui/views/style/platform_style.h:30: static scoped_ptr<ScrollBar> CreateScrollBar(bool is_horizontal); On 2016/02/11 08:46:18, tapted wrote: > nit: needs a comment. (Also.. this should perhaps return a pointer, but I like > this consistency. It means scroll_view.cc will need to do something like > > horiz_sb_(PlatformStyle::CreateScrollBar(true).release()), > vert_sb_(PlatformStyle::CreateScrollBar(false).release()), > > which is fine - we should do that in this CL as well. Sounds good, I made the change in scroll_view
I think I have an idea on how to fix the corner issue with the CocoaScrollBar. I might have to adjust how the bounds are set in scroll_view. I'll see if I can have a fix up on Monday.
neato https://codereview.chromium.org/1671313002/diff/120001/ui/compositor/layer_an... File ui/compositor/layer_animator.h (right): https://codereview.chromium.org/1671313002/diff/120001/ui/compositor/layer_an... ui/compositor/layer_animator.h:109: void SetTransitionDuration(base::TimeDelta duration); Ah curious. So I think the way this is intended to be used is something like (in HideScrollbar()) ui::ScopedLayerAnimationSettings animation(layer()->GetAnimator()); animation.SetTransitionDuration(base::TimeDelta::FromMilliseconds(kFadeDurationMs)); then this doesn't need to be moved into the public interface https://codereview.chromium.org/1671313002/diff/120001/ui/views/controls/scro... File ui/views/controls/scrollbar/cocoa_scroll_bar.h (right): https://codereview.chromium.org/1671313002/diff/120001/ui/views/controls/scro... ui/views/controls/scrollbar/cocoa_scroll_bar.h:16: // The transparent scrollbar which overlays its contents. nit: mention Mac? https://codereview.chromium.org/1671313002/diff/120001/ui/views/controls/scro... ui/views/controls/scrollbar/cocoa_scroll_bar.h:27: // ScrollDelegate override nit: `// ScrollDelegate:` (i.e. since we've been putting `override` on all the method names, saying more than this in the comment seems redundant, and this is how views OWNERS have suggested it for new code (unless it's inconsistent with surrounding code in the same file) https://codereview.chromium.org/1671313002/diff/120001/ui/views/controls/scro... ui/views/controls/scrollbar/cocoa_scroll_bar.h:30: // ViewsScrollbarBridgeDelegate override // ViewsScrollbarBridgeDelegate: https://codereview.chromium.org/1671313002/diff/120001/ui/views/controls/scro... ui/views/controls/scrollbar/cocoa_scroll_bar.h:34: // BaseScrollBar override: // BaseScrollBar: https://codereview.chromium.org/1671313002/diff/120001/ui/views/controls/scro... ui/views/controls/scrollbar/cocoa_scroll_bar.h:37: // ScrollBar overrides: // ScrollBar: https://codereview.chromium.org/1671313002/diff/120001/ui/views/controls/scro... ui/views/controls/scrollbar/cocoa_scroll_bar.h:41: // View overrides: // View: https://codereview.chromium.org/1671313002/diff/120001/ui/views/controls/scro... ui/views/controls/scrollbar/cocoa_scroll_bar.h:69: DISALLOW_COPY_AND_ASSIGN(CocoaScrollBar); nit: blank line before https://codereview.chromium.org/1671313002/diff/120001/ui/views/controls/scro... File ui/views/controls/scrollbar/cocoa_scroll_bar.mm (right): https://codereview.chromium.org/1671313002/diff/120001/ui/views/controls/scro... ui/views/controls/scrollbar/cocoa_scroll_bar.mm:42: static const int kFadeDurationMs = 240; nit: not static Also perhaps move this up next to kScrollbarHideTimeoutMs https://codereview.chromium.org/1671313002/diff/120001/ui/views/controls/scro... ui/views/controls/scrollbar/cocoa_scroll_bar.mm:149: return gfx::Size(); Does this need to change based on scroller_style_? (same with GetLayoutSize() https://codereview.chromium.org/1671313002/diff/120001/ui/views/controls/scro... ui/views/controls/scrollbar/cocoa_scroll_bar.mm:152: void CocoaScrollBar::Layout() { I wonder if this could go in BaseScrollBar.... Of course NativeScrollBarViews doesn't match (it can stay overriding.. but maybe there's an equivalence there too) https://codereview.chromium.org/1671313002/diff/120001/ui/views/controls/scro... ui/views/controls/scrollbar/cocoa_scroll_bar.mm:196: left_border.set_width(kScrollerTrackBorderWidth); do these look any better merged? I think only the set_height/width set_x/y lines would change https://codereview.chromium.org/1671313002/diff/120001/ui/views/controls/scro... ui/views/controls/scrollbar/cocoa_scroll_bar.mm:220: return; nit: blank line after return https://codereview.chromium.org/1671313002/diff/120001/ui/views/controls/scro... ui/views/controls/scrollbar/cocoa_scroll_bar.mm:274: base::TimeDelta::FromMilliseconds(0)); this line probably won't be needed with the ScopedAnimationSettings thingo https://codereview.chromium.org/1671313002/diff/120001/ui/views/style/platfor... File ui/views/style/platform_style.cc (right): https://codereview.chromium.org/1671313002/diff/120001/ui/views/style/platfor... ui/views/style/platform_style.cc:11: you probably need a native_scroll_bar.h #include https://codereview.chromium.org/1671313002/diff/120001/ui/views/style/platfor... File ui/views/style/platform_style.h (right): https://codereview.chromium.org/1671313002/diff/120001/ui/views/style/platfor... ui/views/style/platform_style.h:11: #include "ui/views/controls/scrollbar/scroll_bar.h" nit: forward declare? (Button was just needed for the Button::ButtonStyle)
oh also if there's now some dead code in native_theme_mac we should remove that...
On 2016/02/16 07:05:37, tapted wrote: > oh also if there's now some dead code in native_theme_mac we should remove > that... Fixed the nits and other issues according to your comments. I also fixed the overlapping vertical and horizontal scrollbars issue in scroll_view.cc. Now when the scroll_view sets the scrollbar bounds, it will take in account of the other scrollbar's overlapping size. I also removed the dead native_theme_mac code.
Fixed the nits and other issues according to your comments. I also fixed the overlapping vertical and horizontal scrollbars issue in scroll_view.cc. Now when the scroll_view sets the scrollbar bounds, it will take in account of the other scrollbar's overlapping size. I also removed the dead native_theme_mac code. https://codereview.chromium.org/1671313002/diff/120001/ui/compositor/layer_an... File ui/compositor/layer_animator.h (right): https://codereview.chromium.org/1671313002/diff/120001/ui/compositor/layer_an... ui/compositor/layer_animator.h:109: void SetTransitionDuration(base::TimeDelta duration); On 2016/02/16 06:47:58, tapted wrote: > Ah curious. So I think the way this is intended to be used is something like (in > HideScrollbar()) > > ui::ScopedLayerAnimationSettings animation(layer()->GetAnimator()); > animation.SetTransitionDuration(base::TimeDelta::FromMilliseconds(kFadeDurationMs)); > > then this doesn't need to be moved into the public interface Ah, that makes more sense. I made the change for it https://codereview.chromium.org/1671313002/diff/120001/ui/views/controls/scro... File ui/views/controls/scrollbar/cocoa_scroll_bar.h (right): https://codereview.chromium.org/1671313002/diff/120001/ui/views/controls/scro... ui/views/controls/scrollbar/cocoa_scroll_bar.h:16: // The transparent scrollbar which overlays its contents. On 2016/02/16 06:47:58, tapted wrote: > nit: mention Mac? Done. https://codereview.chromium.org/1671313002/diff/120001/ui/views/controls/scro... ui/views/controls/scrollbar/cocoa_scroll_bar.h:27: // ScrollDelegate override On 2016/02/16 06:47:58, tapted wrote: > nit: `// ScrollDelegate:` > > (i.e. since we've been putting `override` on all the method names, saying more > than this in the comment seems redundant, and this is how views OWNERS have > suggested it for new code (unless it's inconsistent with surrounding code in the > same file) Done. https://codereview.chromium.org/1671313002/diff/120001/ui/views/controls/scro... ui/views/controls/scrollbar/cocoa_scroll_bar.h:30: // ViewsScrollbarBridgeDelegate override On 2016/02/16 06:47:58, tapted wrote: > // ViewsScrollbarBridgeDelegate: Done. https://codereview.chromium.org/1671313002/diff/120001/ui/views/controls/scro... ui/views/controls/scrollbar/cocoa_scroll_bar.h:34: // BaseScrollBar override: On 2016/02/16 06:47:58, tapted wrote: > // BaseScrollBar: Done. https://codereview.chromium.org/1671313002/diff/120001/ui/views/controls/scro... ui/views/controls/scrollbar/cocoa_scroll_bar.h:37: // ScrollBar overrides: On 2016/02/16 06:47:58, tapted wrote: > // ScrollBar: Done. https://codereview.chromium.org/1671313002/diff/120001/ui/views/controls/scro... ui/views/controls/scrollbar/cocoa_scroll_bar.h:41: // View overrides: On 2016/02/16 06:47:58, tapted wrote: > // View: Done. https://codereview.chromium.org/1671313002/diff/120001/ui/views/controls/scro... ui/views/controls/scrollbar/cocoa_scroll_bar.h:69: DISALLOW_COPY_AND_ASSIGN(CocoaScrollBar); On 2016/02/16 06:47:58, tapted wrote: > nit: blank line before Done. https://codereview.chromium.org/1671313002/diff/120001/ui/views/controls/scro... File ui/views/controls/scrollbar/cocoa_scroll_bar.mm (right): https://codereview.chromium.org/1671313002/diff/120001/ui/views/controls/scro... ui/views/controls/scrollbar/cocoa_scroll_bar.mm:42: static const int kFadeDurationMs = 240; On 2016/02/16 06:47:58, tapted wrote: > nit: not static > > Also perhaps move this up next to kScrollbarHideTimeoutMs Done. https://codereview.chromium.org/1671313002/diff/120001/ui/views/controls/scro... ui/views/controls/scrollbar/cocoa_scroll_bar.mm:149: return gfx::Size(); On 2016/02/16 06:47:58, tapted wrote: > Does this need to change based on scroller_style_? (same with GetLayoutSize() This most likely will have to change later, especially when the scrollbar changes size when it's hovered in overlay. For now, I want to just focus on getting the show/hide animation out. https://codereview.chromium.org/1671313002/diff/120001/ui/views/controls/scro... ui/views/controls/scrollbar/cocoa_scroll_bar.mm:152: void CocoaScrollBar::Layout() { On 2016/02/16 06:47:58, tapted wrote: > I wonder if this could go in BaseScrollBar.... Of course NativeScrollBarViews > doesn't match (it can stay overriding.. but maybe there's an equivalence there > too) Yeah, that would work. https://codereview.chromium.org/1671313002/diff/120001/ui/views/controls/scro... ui/views/controls/scrollbar/cocoa_scroll_bar.mm:196: left_border.set_width(kScrollerTrackBorderWidth); On 2016/02/16 06:47:58, tapted wrote: > do these look any better merged? I think only the set_height/width set_x/y lines > would change I kept it separate because I thought it would be easier to read and understand the code. I just merged it and it doesn't look as bad as I'd thought. https://codereview.chromium.org/1671313002/diff/120001/ui/views/controls/scro... ui/views/controls/scrollbar/cocoa_scroll_bar.mm:220: return; On 2016/02/16 06:47:58, tapted wrote: > nit: blank line after return We can't have blank lines after return? TIL, I'll go fix this in a few other places :) https://codereview.chromium.org/1671313002/diff/120001/ui/views/controls/scro... ui/views/controls/scrollbar/cocoa_scroll_bar.mm:274: base::TimeDelta::FromMilliseconds(0)); On 2016/02/16 06:47:58, tapted wrote: > this line probably won't be needed with the ScopedAnimationSettings thingo Done. https://codereview.chromium.org/1671313002/diff/120001/ui/views/style/platfor... File ui/views/style/platform_style.cc (right): https://codereview.chromium.org/1671313002/diff/120001/ui/views/style/platfor... ui/views/style/platform_style.cc:11: On 2016/02/16 06:47:58, tapted wrote: > you probably need a native_scroll_bar.h #include Done. https://codereview.chromium.org/1671313002/diff/120001/ui/views/style/platfor... File ui/views/style/platform_style.h (right): https://codereview.chromium.org/1671313002/diff/120001/ui/views/style/platfor... ui/views/style/platform_style.h:11: #include "ui/views/controls/scrollbar/scroll_bar.h" On 2016/02/16 06:47:58, tapted wrote: > nit: forward declare? (Button was just needed for the Button::ButtonStyle) Done.
Sorry, I just realized what you meant about this. The scrollbar shouldn't overlap for legacy scrollbars. Can you hold on the review? I'll add a fix for it. https://codereview.chromium.org/1671313002/diff/120001/ui/views/controls/scro... File ui/views/controls/scrollbar/cocoa_scroll_bar.mm (right): https://codereview.chromium.org/1671313002/diff/120001/ui/views/controls/scro... ui/views/controls/scrollbar/cocoa_scroll_bar.mm:149: return gfx::Size(); On 2016/02/16 23:18:18, spqchan wrote: > On 2016/02/16 06:47:58, tapted wrote: > > Does this need to change based on scroller_style_? (same with GetLayoutSize() > > This most likely will have to change later, especially when the scrollbar > changes size when it's hovered in overlay. > For now, I want to just focus on getting the show/hide animation out. Oh shoot, I see what you mean. I'll look into this
just did a quick pass, but it's looking really close https://codereview.chromium.org/1671313002/diff/120001/ui/views/controls/scro... File ui/views/controls/scrollbar/cocoa_scroll_bar.mm (right): https://codereview.chromium.org/1671313002/diff/120001/ui/views/controls/scro... ui/views/controls/scrollbar/cocoa_scroll_bar.mm:220: return; On 2016/02/16 23:18:18, spqchan wrote: > On 2016/02/16 06:47:58, tapted wrote: > > nit: blank line after return > > We can't have blank lines after return? > TIL, I'll go fix this in a few other places :) Ah, no the opposite :). There *should* be a line break. Sorry for being unclear. It's not really a rule, it just helps break up the code, since the cases are clearly separate. E.g. https://www.chromium.org/developers/coding-style#Code_Formatting says "Always linebreak after a conditional, even if the body is only a return or other simple action." but that's more to say don't do if (foo) return; do if (foo) return; instead. https://codereview.chromium.org/1671313002/diff/180001/ui/views/controls/scro... File ui/views/controls/scrollbar/base_scroll_bar.cc (right): https://codereview.chromium.org/1671313002/diff/180001/ui/views/controls/scro... ui/views/controls/scrollbar/base_scroll_bar.cc:132: gfx::Rect thumb_bounds = GetTrackBounds(); Yeah - I'm not sure what views/OWNERS will think about doing this.. so, to back it up, perhaps a comment here like, // Provide a default implementation that <summarize the layout approach>. https://codereview.chromium.org/1671313002/diff/180001/ui/views/controls/scro... File ui/views/controls/scrollbar/cocoa_scroll_bar.mm (right): https://codereview.chromium.org/1671313002/diff/180001/ui/views/controls/scro... ui/views/controls/scrollbar/cocoa_scroll_bar.mm:268: animation.SetTransitionDuration(base::TimeDelta::TimeDelta()); does it behave if we skip these lines? Since there is no default duration now, I think omitting the use of a ScopedLayerAnimationSettings is equivalent to saying "this is not animated", which is the effect we are after
Fixed the layout for scroller style. The scrollbar will only overlap the content if we're using the legacy style. https://codereview.chromium.org/1671313002/diff/180001/ui/views/controls/scro... File ui/views/controls/scrollbar/base_scroll_bar.cc (right): https://codereview.chromium.org/1671313002/diff/180001/ui/views/controls/scro... ui/views/controls/scrollbar/base_scroll_bar.cc:132: gfx::Rect thumb_bounds = GetTrackBounds(); On 2016/02/16 23:32:57, tapted wrote: > Yeah - I'm not sure what views/OWNERS will think about doing this.. so, to back > it up, perhaps a comment here like, > > // Provide a default implementation that <summarize the layout approach>. Done. https://codereview.chromium.org/1671313002/diff/180001/ui/views/controls/scro... File ui/views/controls/scrollbar/cocoa_scroll_bar.mm (right): https://codereview.chromium.org/1671313002/diff/180001/ui/views/controls/scro... ui/views/controls/scrollbar/cocoa_scroll_bar.mm:268: animation.SetTransitionDuration(base::TimeDelta::TimeDelta()); On 2016/02/16 23:32:57, tapted wrote: > does it behave if we skip these lines? Since there is no default duration now, I > think omitting the use of a ScopedLayerAnimationSettings is equivalent to saying > "this is not animated", which is the effect we are after Just checked, yes it can be omitted. Removed
I think a views_unittest will fail after this -- ScrollViewTest.ScrollBars (fails on Mac, not sure about other platforms, but views_unittests isn't yet part of the CQ, so we should make sure the test passes locally first). We also need to check whether ScrollViewTest.* is giving satisfactory coverage -- I think we should add some extra tests (possibly mac-specific). E.g., there don't seem to be any overlay scroller tests we can lean on :| Then before sending to OWNERS we should update the CL description with a bit more -- the BUG= and some rationale about why we're moving things out of NativeTheme and makin' our own scrollers, and also a summary of expected new behaviour. (and sorry - I may have missed some stuff - got buried in bug triage today - might need to play with this some more still) https://codereview.chromium.org/1671313002/diff/200001/ui/views/controls/scro... File ui/views/controls/scrollbar/cocoa_scroll_bar.h (right): https://codereview.chromium.org/1671313002/diff/200001/ui/views/controls/scro... ui/views/controls/scrollbar/cocoa_scroll_bar.h:58: base::OneShotTimer hide_scrollbar_timer_; I think it might be nicer just to use a base::Timer directly, and initialize it in the constructor with all the arguments to Start(..) Then we don't need ResetTimer() -- simply calling hide_scrollbar_timer_.Reset() seems to be enough. But I might not have groked the timer documentation entirely correctly. https://codereview.chromium.org/1671313002/diff/200001/ui/views/controls/scro... File ui/views/controls/scrollbar/cocoa_scroll_bar.mm (right): https://codereview.chromium.org/1671313002/diff/200001/ui/views/controls/scro... ui/views/controls/scrollbar/cocoa_scroll_bar.mm:1: // Copyright 2013 The Chromium Authors. All rights reserved. 2016 https://codereview.chromium.org/1671313002/diff/200001/ui/views/controls/scro... ui/views/controls/scrollbar/cocoa_scroll_bar.mm:5: #include "ui/views/controls/scrollbar/cocoa_scroll_bar.h" nit:import https://codereview.chromium.org/1671313002/diff/200001/ui/views/controls/scro... ui/views/controls/scrollbar/cocoa_scroll_bar.mm:39: const SkColor kScrollerThumbColor = SkColorSetARGB(0x38, 0, 0, 0); I think we've lost the Hover color - SkColorSetARGB(0x80, 0, 0, 0); This seems to be more noticeable for the overlay style, since thumbs in overlay style seem to always draw as if they are hovered (i.e. darker), currently they are too light. If you want to defer the hover-changes-colour logic to a follow-up we should maybe start with the darker style, since it seems quite light. https://codereview.chromium.org/1671313002/diff/200001/ui/views/controls/scro... ui/views/controls/scrollbar/cocoa_scroll_bar.mm:86: SkPaint paint; paint.setAntiAlias(true); after this (it's easier to spot on a non-retina screen :) https://codereview.chromium.org/1671313002/diff/200001/ui/views/controls/scro... ui/views/controls/scrollbar/cocoa_scroll_bar.mm:176: NULL, nit: nullptr https://codereview.chromium.org/1671313002/diff/200001/ui/views/controls/scro... ui/views/controls/scrollbar/cocoa_scroll_bar.mm:209: if (layer()->opacity()) { this is subtle.. probably needs a comment, like // If an overlay scroller has not yet faded out completely, reshow it when entering the scroll track to give more time for the user to drag it. I think we also need a call to ResetTimer() after showing it for this case too, otherwise it will stick around forever, but that only seems to be the case once a drag is initiated. https://codereview.chromium.org/1671313002/diff/200001/ui/views/controls/scro... ui/views/controls/scrollbar/cocoa_scroll_bar.mm:211: } nit: doesn't need curlies https://codereview.chromium.org/1671313002/diff/200001/ui/views/controls/scro... ui/views/controls/scrollbar/cocoa_scroll_bar.mm:228: if (layer()->opacity() < 1.0) is this check needed (can we just always ShowScrollbar()? https://codereview.chromium.org/1671313002/diff/200001/ui/views/controls/scro... ui/views/controls/scrollbar/cocoa_scroll_bar.mm:260: ui::ScopedLayerAnimationSettings animation(layer()->GetAnimator()); DCHECK in here that the scroller_style_ is overlay style https://codereview.chromium.org/1671313002/diff/200001/ui/views/controls/scro... ui/views/controls/scrollbar/cocoa_scroll_bar.mm:275: } I think we need to cancel the timer here. E.g. hide_scrollbar_timer_.Stop(). Otherwise it could trigger shortly after the scroller style changes (I could do this if I really quickly set `Show scroll bars` to `Always` shortly after scrolling an overlay scroller). There may be other races - I think Timer::Stop() here will cover them all.
ooh! I forgot to mention - a yandex contributor landed ui::test::ScopedPreferredScrollerStyleLegacy recently ( https://codereview.chromium.org/1642553004 ). This provides a neat way for a test to ensure it doesn't become flaky based on what a particular machine has set for its scroller preference. We may want to tweak it so that it can also force the style to be `overlay`, to ensure we get coverage of both styles for the views_unittests.
On 2016/02/17 22:55:09, tapted wrote: > ooh! I forgot to mention - a yandex contributor landed > ui::test::ScopedPreferredScrollerStyleLegacy recently ( > https://codereview.chromium.org/1642553004 ). This provides a neat way for a > test to ensure it doesn't become flaky based on what a particular machine has > set for its scroller preference. > > We may want to tweak it so that it can also force the style to be `overlay`, to > ensure we get coverage of both styles for the views_unittests. Thanks for the heads up on the Yandex change, it was really helpful! Several things since the last review: - Fixed the "scrollbar won't disappear if you drag it" issue. The logic is changed slightly for that one - Added back the hover thumb color. - Fixed the ScrollView unit test - Added a new scrollbar test for the overlay style and tweaked ScopedPreferredScrollerStyleLegacy so we can force the style to be overlay. The CL is rather large now, do you think I should move the Overlay style unit test changes (Patch 14-16) into a follow up CL? Doing that might make things easier for both of us.
Thanks for the heads up on the Yandex change, it was really helpful! Several things since the last review: - Fixed the "scrollbar won't disappear if you drag it" issue. The logic is changed slightly for that one - Added back the hover thumb color. - Fixed the ScrollView unit test - Added a new scrollbar test for the overlay style and tweaked ScopedPreferredScrollerStyleLegacy so we can force the style to be overlay. The CL is rather large now, do you think I should move the Overlay style unit test changes (Patch 14-16) into a follow up CL? Doing that might make things easier for both of us. https://codereview.chromium.org/1671313002/diff/200001/ui/views/controls/scro... File ui/views/controls/scrollbar/cocoa_scroll_bar.h (right): https://codereview.chromium.org/1671313002/diff/200001/ui/views/controls/scro... ui/views/controls/scrollbar/cocoa_scroll_bar.h:58: base::OneShotTimer hide_scrollbar_timer_; On 2016/02/17 08:46:20, tapted wrote: > I think it might be nicer just to use a base::Timer directly, and initialize it > in the constructor with all the arguments to Start(..) > > Then we don't need ResetTimer() -- simply calling hide_scrollbar_timer_.Reset() > seems to be enough. But I might not have groked the timer documentation entirely > correctly. Done https://codereview.chromium.org/1671313002/diff/200001/ui/views/controls/scro... File ui/views/controls/scrollbar/cocoa_scroll_bar.mm (right): https://codereview.chromium.org/1671313002/diff/200001/ui/views/controls/scro... ui/views/controls/scrollbar/cocoa_scroll_bar.mm:1: // Copyright 2013 The Chromium Authors. All rights reserved. On 2016/02/17 08:46:20, tapted wrote: > 2016 Done. https://codereview.chromium.org/1671313002/diff/200001/ui/views/controls/scro... ui/views/controls/scrollbar/cocoa_scroll_bar.mm:5: #include "ui/views/controls/scrollbar/cocoa_scroll_bar.h" On 2016/02/17 08:46:21, tapted wrote: > nit:import Done. https://codereview.chromium.org/1671313002/diff/200001/ui/views/controls/scro... ui/views/controls/scrollbar/cocoa_scroll_bar.mm:39: const SkColor kScrollerThumbColor = SkColorSetARGB(0x38, 0, 0, 0); On 2016/02/17 08:46:20, tapted wrote: > I think we've lost the Hover color - SkColorSetARGB(0x80, 0, 0, 0); This seems > to be more noticeable for the overlay style, since thumbs in overlay style seem > to always draw as if they are hovered (i.e. darker), currently they are too > light. > > If you want to defer the hover-changes-colour logic to a follow-up we should > maybe start with the darker style, since it seems quite light. Ah I see what you mean. The color is darker in Overlay style and if it's hovered in Legacy. The logic for it is fairly simple, so it shouldn't hurt adding it to this CL. I updated OnPaint on the CocoaScrollbarThumb https://codereview.chromium.org/1671313002/diff/200001/ui/views/controls/scro... ui/views/controls/scrollbar/cocoa_scroll_bar.mm:86: SkPaint paint; On 2016/02/17 08:46:20, tapted wrote: > paint.setAntiAlias(true); after this (it's easier to spot on a non-retina screen > :) Done. https://codereview.chromium.org/1671313002/diff/200001/ui/views/controls/scro... ui/views/controls/scrollbar/cocoa_scroll_bar.mm:176: NULL, On 2016/02/17 08:46:20, tapted wrote: > nit: nullptr Done. https://codereview.chromium.org/1671313002/diff/200001/ui/views/controls/scro... ui/views/controls/scrollbar/cocoa_scroll_bar.mm:209: if (layer()->opacity()) { On 2016/02/17 08:46:20, tapted wrote: > this is subtle.. probably needs a comment, like > // If an overlay scroller has not yet faded out completely, reshow it when > entering the scroll track to give more time for the user to drag it. > > I think we also need a call to ResetTimer() after showing it for this case too, > otherwise it will stick around forever, but that only seems to be the case once > a drag is initiated. Sure thing. The reason why I didn't put ResetTimer() there originally was because I didn't want the scrollbars to disappear when the mouse is still hovering over it. I managed to fix this issue just now by resetting the timer in HideScrollbar() if the thumb is in a hover or pressed state. https://codereview.chromium.org/1671313002/diff/200001/ui/views/controls/scro... ui/views/controls/scrollbar/cocoa_scroll_bar.mm:211: } On 2016/02/17 08:46:20, tapted wrote: > nit: doesn't need curlies Done. https://codereview.chromium.org/1671313002/diff/200001/ui/views/controls/scro... ui/views/controls/scrollbar/cocoa_scroll_bar.mm:228: if (layer()->opacity() < 1.0) On 2016/02/17 08:46:20, tapted wrote: > is this check needed (can we just always ShowScrollbar()? This check was needed earlier because of an edge case where the scrolltrack disappears before the rest of the scrollbar fades away. It's no longer necessary due to the recent changes though, so I'll remove it. https://codereview.chromium.org/1671313002/diff/200001/ui/views/controls/scro... ui/views/controls/scrollbar/cocoa_scroll_bar.mm:260: ui::ScopedLayerAnimationSettings animation(layer()->GetAnimator()); On 2016/02/17 08:46:20, tapted wrote: > DCHECK in here that the scroller_style_ is overlay style Done. https://codereview.chromium.org/1671313002/diff/200001/ui/views/controls/scro... ui/views/controls/scrollbar/cocoa_scroll_bar.mm:275: } On 2016/02/17 08:46:21, tapted wrote: > I think we need to cancel the timer here. E.g. hide_scrollbar_timer_.Stop(). > Otherwise it could trigger shortly after the scroller style changes (I could do > this if I really quickly set `Show scroll bars` to `Always` shortly after > scrolling an overlay scroller). There may be other races - I think Timer::Stop() > here will cover them all. Done.
hm, I guess it's tricky to test around the timers/animations. A few more suggestions below - I think then it's fine to send to owners. I don't think the CL is too large, and it's usually much preferred to land tests along with any functional changes, rather than do tests in a follow-up. https://codereview.chromium.org/1671313002/diff/300001/ui/base/test/scoped_pr... File ui/base/test/scoped_preferred_scroller_style_mac.h (right): https://codereview.chromium.org/1671313002/diff/300001/ui/base/test/scoped_pr... ui/base/test/scoped_preferred_scroller_style_mac.h:25: ScopedPreferredScrollerStyle(bool overlay); nit: explicit [aside: It's also common to use an enum like ScopedPreferredScrollerStyle::OVERLAY/LEGACY rather than a bool (but "legacy" isn't very descriptive anyway -- it's basically "not overlay" -- so I'm indifferent. Plus it would be silly to provide our own enum when Cocoa provides one we don't want to expose so this can be included in C++ files)] https://codereview.chromium.org/1671313002/diff/300001/ui/views/controls/scro... File ui/views/controls/scroll_view_unittest.cc (right): https://codereview.chromium.org/1671313002/diff/300001/ui/views/controls/scro... ui/views/controls/scroll_view_unittest.cc:25: enum ScrollBarOrientation { HORIZONTAL, VERTICAL }; `enum` -> `enum class`? (otherwise, I think HORIZONTAL/VERTICAL are accessible without the ScrollBarOrientation:: prefix https://codereview.chromium.org/1671313002/diff/300001/ui/views/controls/scro... ui/views/controls/scroll_view_unittest.cc:56: void CheckScrollbarVisibility(ScrollView& scroll_view, does const-reference work? (otherwise ScrollView* -> we aren't allowed non-const ref arguments in Chromium) https://codereview.chromium.org/1671313002/diff/300001/ui/views/controls/scro... ui/views/controls/scroll_view_unittest.cc:58: bool is_visible) { maybe is_visible -> should_be_visible? https://codereview.chromium.org/1671313002/diff/300001/ui/views/controls/scro... ui/views/controls/scroll_view_unittest.cc:63: ASSERT_TRUE(scrollbar != NULL); typically I just see ASSERT_TRUE(scrollbar); for non-null checks. (otherwise NULL -> nullptr) https://codereview.chromium.org/1671313002/diff/300001/ui/views/controls/scro... ui/views/controls/scroll_view_unittest.cc:83: // If on Mac, test the legacy scrollbars. nit: legacy -> non-overlay style https://codereview.chromium.org/1671313002/diff/300001/ui/views/controls/scro... ui/views/controls/scroll_view_unittest.cc:490: EXPECT_EQ(100 - scroll_view.GetScrollBarWidth(), contents->parent()->width()); should scroll_view.GetScrollBarWidth() be zero for this case? Maybe a separate check for that, and just do // Since it is overlaid, the ViewPort size should match the ScrollView. EXPECT_EQ(100, contents->parent()->width()); same for the both-scrollbars cases https://codereview.chromium.org/1671313002/diff/300001/ui/views/controls/scro... ui/views/controls/scroll_view_unittest.cc:516: } I think a good test would be to (here) make scroller_style_override a scoped_ptr and do scroller_style_override.reset(); scroller_style_override.reset(new ScopedPreferred..(false)); then do some spot checks to ensure the contents->parent() (the ViewPort?) has resized to be smaller, and the ScrollBarWidth is non-zero (both without requiring a separate call to scroll_view.Layout())
https://codereview.chromium.org/1671313002/diff/300001/ui/base/test/scoped_pr... File ui/base/test/scoped_preferred_scroller_style_mac.h (right): https://codereview.chromium.org/1671313002/diff/300001/ui/base/test/scoped_pr... ui/base/test/scoped_preferred_scroller_style_mac.h:25: ScopedPreferredScrollerStyle(bool overlay); On 2016/02/19 06:56:37, tapted wrote: > nit: explicit > > [aside: It's also common to use an enum like > ScopedPreferredScrollerStyle::OVERLAY/LEGACY rather than a bool (but "legacy" > isn't very descriptive anyway -- it's basically "not overlay" -- so I'm > indifferent. Plus it would be silly to provide our own enum when Cocoa provides > one we don't want to expose so this can be included in C++ files)] Done. https://codereview.chromium.org/1671313002/diff/300001/ui/views/controls/scro... File ui/views/controls/scroll_view_unittest.cc (right): https://codereview.chromium.org/1671313002/diff/300001/ui/views/controls/scro... ui/views/controls/scroll_view_unittest.cc:25: enum ScrollBarOrientation { HORIZONTAL, VERTICAL }; On 2016/02/19 06:56:37, tapted wrote: > `enum` -> `enum class`? (otherwise, I think HORIZONTAL/VERTICAL are accessible > without the ScrollBarOrientation:: prefix True, I removed the prefix. Is it okay if I stick with enum then? Or should I use enum class? https://codereview.chromium.org/1671313002/diff/300001/ui/views/controls/scro... ui/views/controls/scroll_view_unittest.cc:56: void CheckScrollbarVisibility(ScrollView& scroll_view, On 2016/02/19 06:56:37, tapted wrote: > does const-reference work? (otherwise ScrollView* -> we aren't allowed non-const > ref arguments in Chromium) Done. https://codereview.chromium.org/1671313002/diff/300001/ui/views/controls/scro... ui/views/controls/scroll_view_unittest.cc:58: bool is_visible) { On 2016/02/19 06:56:37, tapted wrote: > maybe is_visible -> should_be_visible? Done. https://codereview.chromium.org/1671313002/diff/300001/ui/views/controls/scro... ui/views/controls/scroll_view_unittest.cc:63: ASSERT_TRUE(scrollbar != NULL); On 2016/02/19 06:56:37, tapted wrote: > typically I just see ASSERT_TRUE(scrollbar); for non-null checks. (otherwise > NULL -> nullptr) Done. https://codereview.chromium.org/1671313002/diff/300001/ui/views/controls/scro... ui/views/controls/scroll_view_unittest.cc:83: // If on Mac, test the legacy scrollbars. On 2016/02/19 06:56:37, tapted wrote: > nit: legacy -> non-overlay style Done. https://codereview.chromium.org/1671313002/diff/300001/ui/views/controls/scro... ui/views/controls/scroll_view_unittest.cc:490: EXPECT_EQ(100 - scroll_view.GetScrollBarWidth(), contents->parent()->width()); On 2016/02/19 06:56:37, tapted wrote: > should scroll_view.GetScrollBarWidth() be zero for this case? Maybe a separate > check for that, and just do > > // Since it is overlaid, the ViewPort size should match the ScrollView. > EXPECT_EQ(100, contents->parent()->width()); > > > same for the both-scrollbars cases Done. https://codereview.chromium.org/1671313002/diff/300001/ui/views/controls/scro... ui/views/controls/scroll_view_unittest.cc:516: } On 2016/02/19 06:56:37, tapted wrote: > I think a good test would be to (here) make scroller_style_override a scoped_ptr > and do > > scroller_style_override.reset(); > scroller_style_override.reset(new ScopedPreferred..(false)); > > then do some spot checks to ensure the contents->parent() (the ViewPort?) has > resized to be smaller, and the ScrollBarWidth is non-zero (both without > requiring a separate call to scroll_view.Layout()) Done.
PTAL
lgtm with a couple of tweaks On 2016/02/17 08:46:21, tapted wrote: > Then before sending to OWNERS we should update the CL description with a bit > more -- the BUG= and some rationale about why we're moving things out of > NativeTheme and makin' our own scrollers, and also a summary of expected new > behaviour. (ping on ^^). The CL summary line should also start with `MacViews: ` And it would be good to say that we don't yet adjust the track/thumb width for overlay scrollers on hover/drag. (future-note: Chrome's actually seems more complicated than Safari - Safari never animates the width, never shows a "thin" track (only ever a thin thumb), and only shows a track at all if the mouse pointer is over the track when the thumb is first shown) https://codereview.chromium.org/1671313002/diff/340001/ui/views/controls/scro... File ui/views/controls/scrollbar/base_scroll_bar.cc (right): https://codereview.chromium.org/1671313002/diff/340001/ui/views/controls/scro... ui/views/controls/scrollbar/base_scroll_bar.cc:131: void BaseScrollBar::Layout() { If we keep this here, the identical logic can be removed from overlay_scroll_bar.cc However, I just tried it out, and I think this can be simplified to just `GetThumb()->SetBoundsRect(GetTrackBounds())` (in CocoaScrollBar) That's what NativeScrollBarViews does, but since it's just one line, I think it's fine to just put it in CocoaScrollBar, and remove the default-provided implementation (make BaseScrollBar::Layout() pure virtual again). (I think this will be more appealing to OWNERS too, but beforehand it was too much duplicated logic). https://codereview.chromium.org/1671313002/diff/340001/ui/views/controls/scro... File ui/views/controls/scrollbar/cocoa_scroll_bar.mm (right): https://codereview.chromium.org/1671313002/diff/340001/ui/views/controls/scro... ui/views/controls/scrollbar/cocoa_scroll_bar.mm:42: // Scroller track colors. // TODO(spqchan): Add an alpha channel for the overlay-style scroll track. https://codereview.chromium.org/1671313002/diff/340001/ui/views/controls/scro... ui/views/controls/scrollbar/cocoa_scroll_bar.mm:255: HideScrollbar(); This shouldn't fade out (to be consistent with AppKit). I think just layer()->SetOpacity(0.0); // Don't fade out. is fine, since the scrolltrack might need to be repainted anyway (user will just have to scroll some more). (doing this also avoids a glitch where the corner isn't painted when this fade-out occurs)
Description was changed from ========== Overlay Scrollbars with Show/Hide ========== to ========== Implemented scrollbars that follow the style of Cocoa. The scrollbars are implemented in CocoaScrollBar. They switch between overlay and non-overlay style, depending on the system's preference If using the overlay style, the scrollbars will: - Hide if unused - Display the scrolltrack if the user hovers over it - Currently hovering will not adjust the width. That will come later. In addition, since we are switching to CocoaScrollbars on OSX, dead code is cleaned up in native_theme_mac. Tests are also fixed and written for it. Changes are made to scoped_preferred_scroller_style_legacy_mac so that we can force the scroller style to be non-overlay and overlay in our tests. BUG=Issue 568409: ==========
Description was changed from ========== Implemented scrollbars that follow the style of Cocoa. The scrollbars are implemented in CocoaScrollBar. They switch between overlay and non-overlay style, depending on the system's preference If using the overlay style, the scrollbars will: - Hide if unused - Display the scrolltrack if the user hovers over it - Currently hovering will not adjust the width. That will come later. In addition, since we are switching to CocoaScrollbars on OSX, dead code is cleaned up in native_theme_mac. Tests are also fixed and written for it. Changes are made to scoped_preferred_scroller_style_legacy_mac so that we can force the scroller style to be non-overlay and overlay in our tests. BUG=Issue 568409: ========== to ========== Implemented scrollbars that follow the style of Cocoa. The scrollbars are implemented in CocoaScrollBar. They switch between overlay and non-overlay style, depending on the system's preference If using the overlay style, the scrollbars will: - Hide if unused - Display the scrolltrack if the user hovers over it - Currently hovering will not adjust the width. That will come later. In addition, since we are switching to CocoaScrollbars on OSX, dead code is cleaned up in native_theme_mac. Tests are modified and written for the overlay scrollbars. Changes are made to scoped_preferred_scroller_style_legacy_mac so that we can force the scroller style to be non-overlay and overlay in our tests. BUG=Issue 568409: ==========
Description was changed from ========== Implemented scrollbars that follow the style of Cocoa. The scrollbars are implemented in CocoaScrollBar. They switch between overlay and non-overlay style, depending on the system's preference If using the overlay style, the scrollbars will: - Hide if unused - Display the scrolltrack if the user hovers over it - Currently hovering will not adjust the width. That will come later. In addition, since we are switching to CocoaScrollbars on OSX, dead code is cleaned up in native_theme_mac. Tests are modified and written for the overlay scrollbars. Changes are made to scoped_preferred_scroller_style_legacy_mac so that we can force the scroller style to be non-overlay and overlay in our tests. BUG=Issue 568409: ========== to ========== Implemented scrollbars that follow the style of Cocoa. The scrollbars are implemented in CocoaScrollBar. They switch between overlay and non-overlay style, depending on the system's preference If using the overlay style, the scrollbars will: - Hide if unused - Display the scrolltrack if the user hovers over it - Currently hovering will not adjust the width. That will come later. In addition, since we are switching to CocoaScrollbars on OSX, dead code is cleaned up in native_theme_mac. Tests are modified and written for the overlay scrollbars. Changes are made to scoped_preferred_scroller_style_legacy_mac so that we can force the scroller style to be non-overlay and overlay in our tests. BUG=568409: ==========
spqchan@chromium.org changed reviewers: + avi@chromium.org
+avi for OWNERS https://codereview.chromium.org/1671313002/diff/340001/ui/views/controls/scro... File ui/views/controls/scrollbar/base_scroll_bar.cc (right): https://codereview.chromium.org/1671313002/diff/340001/ui/views/controls/scro... ui/views/controls/scrollbar/base_scroll_bar.cc:131: void BaseScrollBar::Layout() { On 2016/02/22 04:42:15, tapted wrote: > If we keep this here, the identical logic can be removed from > overlay_scroll_bar.cc > > However, I just tried it out, and I think this can be simplified to just > `GetThumb()->SetBoundsRect(GetTrackBounds())` (in CocoaScrollBar) > > That's what NativeScrollBarViews does, but since it's just one line, I think > it's fine to just put it in CocoaScrollBar, and remove the default-provided > implementation (make BaseScrollBar::Layout() pure virtual again). (I think this > will be more appealing to OWNERS too, but beforehand it was too much duplicated > logic). Done. https://codereview.chromium.org/1671313002/diff/340001/ui/views/controls/scro... File ui/views/controls/scrollbar/cocoa_scroll_bar.mm (right): https://codereview.chromium.org/1671313002/diff/340001/ui/views/controls/scro... ui/views/controls/scrollbar/cocoa_scroll_bar.mm:42: // Scroller track colors. On 2016/02/22 04:42:15, tapted wrote: > // TODO(spqchan): Add an alpha channel for the overlay-style scroll track. Done. https://codereview.chromium.org/1671313002/diff/340001/ui/views/controls/scro... ui/views/controls/scrollbar/cocoa_scroll_bar.mm:255: HideScrollbar(); On 2016/02/22 04:42:15, tapted wrote: > This shouldn't fade out (to be consistent with AppKit). I think just > > layer()->SetOpacity(0.0); // Don't fade out. > > is fine, since the scrolltrack might need to be repainted anyway (user will just > have to scroll some more). (doing this also avoids a glitch where the corner > isn't painted when this fade-out occurs) Done.
https://codereview.chromium.org/1671313002/diff/360001/ui/views/cocoa/views_s... File ui/views/cocoa/views_scrollbar_bridge.mm (right): https://codereview.chromium.org/1671313002/diff/360001/ui/views/cocoa/views_s... ui/views/cocoa/views_scrollbar_bridge.mm:30: object:nil]; Won't this end up with a double registration if called with a new delegate while already registered with one? https://codereview.chromium.org/1671313002/diff/360001/ui/views/controls/scro... File ui/views/controls/scroll_view.cc (right): https://codereview.chromium.org/1671313002/diff/360001/ui/views/controls/scro... ui/views/controls/scroll_view.cc:128: vert_sb_(PlatformStyle::CreateScrollBar(false).release()), o_O Make horiz_sb_ and vert_sb_ into scoped_ptrs in a prep CL. https://codereview.chromium.org/1671313002/diff/360001/ui/views/controls/scro... File ui/views/controls/scrollbar/cocoa_scroll_bar.mm (right): https://codereview.chromium.org/1671313002/diff/360001/ui/views/controls/scro... ui/views/controls/scrollbar/cocoa_scroll_bar.mm:100: thumb_color = kScrollerHoverThumbColor; {} because the if() is multiline https://codereview.chromium.org/1671313002/diff/360001/ui/views/controls/scro... ui/views/controls/scrollbar/cocoa_scroll_bar.mm:269: DCHECK(scroller_style_ == NSScrollerStyleOverlay); DCHECK_EQ?
PTAL https://codereview.chromium.org/1671313002/diff/360001/ui/views/cocoa/views_s... File ui/views/cocoa/views_scrollbar_bridge.mm (right): https://codereview.chromium.org/1671313002/diff/360001/ui/views/cocoa/views_s... ui/views/cocoa/views_scrollbar_bridge.mm:30: object:nil]; On 2016/02/22 19:37:07, Avi wrote: > Won't this end up with a double registration if called with a new delegate while > already registered with one? True, I changed it so it only sets the delegate when it initializes. https://codereview.chromium.org/1671313002/diff/360001/ui/views/controls/scro... File ui/views/controls/scroll_view.cc (right): https://codereview.chromium.org/1671313002/diff/360001/ui/views/controls/scro... ui/views/controls/scroll_view.cc:128: vert_sb_(PlatformStyle::CreateScrollBar(false).release()), On 2016/02/22 19:37:08, Avi wrote: > o_O > > Make horiz_sb_ and vert_sb_ into scoped_ptrs in a prep CL. What do you mean by a prep CL? https://codereview.chromium.org/1671313002/diff/360001/ui/views/controls/scro... File ui/views/controls/scrollbar/cocoa_scroll_bar.mm (right): https://codereview.chromium.org/1671313002/diff/360001/ui/views/controls/scro... ui/views/controls/scrollbar/cocoa_scroll_bar.mm:100: thumb_color = kScrollerHoverThumbColor; On 2016/02/22 19:37:08, Avi wrote: > {} because the if() is multiline Done. https://codereview.chromium.org/1671313002/diff/360001/ui/views/controls/scro... ui/views/controls/scrollbar/cocoa_scroll_bar.mm:269: DCHECK(scroller_style_ == NSScrollerStyleOverlay); On 2016/02/22 19:37:08, Avi wrote: > DCHECK_EQ? Done.
lgtm https://codereview.chromium.org/1671313002/diff/360001/ui/views/controls/scro... File ui/views/controls/scroll_view.cc (right): https://codereview.chromium.org/1671313002/diff/360001/ui/views/controls/scro... ui/views/controls/scroll_view.cc:128: vert_sb_(PlatformStyle::CreateScrollBar(false).release()), On 2016/02/22 20:49:25, spqchan wrote: > On 2016/02/22 19:37:08, Avi wrote: > > o_O > > > > Make horiz_sb_ and vert_sb_ into scoped_ptrs in a prep CL. > > What do you mean by a prep CL? I meant to make a new CL that makes horiz_sb_ and vert_sb_ into scoped_ptrs and land that first, rebasing this on top of that one. Or alternatively, land this as-is, but fix it in a follow-up. If horiz_sb_ and vert_sb_ are owned, we should indicate it, not drop the ownership here.
On 2016/02/22 20:54:15, Avi wrote: > lgtm > > https://codereview.chromium.org/1671313002/diff/360001/ui/views/controls/scro... > File ui/views/controls/scroll_view.cc (right): > > https://codereview.chromium.org/1671313002/diff/360001/ui/views/controls/scro... > ui/views/controls/scroll_view.cc:128: > vert_sb_(PlatformStyle::CreateScrollBar(false).release()), > On 2016/02/22 20:49:25, spqchan wrote: > > On 2016/02/22 19:37:08, Avi wrote: > > > o_O > > > > > > Make horiz_sb_ and vert_sb_ into scoped_ptrs in a prep CL. > > > > What do you mean by a prep CL? > > I meant to make a new CL that makes horiz_sb_ and vert_sb_ into scoped_ptrs and > land that first, rebasing this on top of that one. Or alternatively, land this > as-is, but fix it in a follow-up. > > If horiz_sb_ and vert_sb_ are owned, we should indicate it, not drop the > ownership here. Got it, thanks for the clarification! I'll fix it in a follow up CL
The CQ bit was checked by spqchan@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from tapted@chromium.org Link to the patchset: https://codereview.chromium.org/1671313002/#ps380001 (title: " ")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1671313002/380001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1671313002/380001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_chromium_chromeos_ozone_rel_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_chromeos_rel_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_compile_dbg_32_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_compile_dbg_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_rel_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
https://codereview.chromium.org/1671313002/diff/360001/ui/views/controls/scro... File ui/views/controls/scroll_view.cc (right): https://codereview.chromium.org/1671313002/diff/360001/ui/views/controls/scro... ui/views/controls/scroll_view.cc:128: vert_sb_(PlatformStyle::CreateScrollBar(false).release()), On 2016/02/22 20:54:15, Avi wrote: > On 2016/02/22 20:49:25, spqchan wrote: > > On 2016/02/22 19:37:08, Avi wrote: > > > o_O > > > > > > Make horiz_sb_ and vert_sb_ into scoped_ptrs in a prep CL. > > > > What do you mean by a prep CL? > > I meant to make a new CL that makes horiz_sb_ and vert_sb_ into scoped_ptrs and > land that first, rebasing this on top of that one. Or alternatively, land this > as-is, but fix it in a follow-up. > > If horiz_sb_ and vert_sb_ are owned, we should indicate it, not drop the > ownership here. I think the reason for this is that horiz_sb_ and vert_sb_ can be owned either by this class, or by "the view hierarchy" (i.e. the children_ vector in views::View). Calling View::AddChildView(horiz_sb_) is always a transfer of ownership, but since that's only "maybe" called in the lifetime of ScrollView, ScrollView still needs a handle on the view so that it can delete it -- the views::View destructor always first removes itself from any view hierarchy so `delete horiz_sb_` in the ScrollView destructor will either remove itself from the view hierarchy and delete the view, or just delete the view, depending on what happened. A scoped_ptr might still work for this, but `AddChildView(horiz_sb_.get())` could be misleading, since there becomes effectively two non-weak owners of the object.
https://codereview.chromium.org/1671313002/diff/360001/ui/views/cocoa/views_s... File ui/views/cocoa/views_scrollbar_bridge.mm (right): https://codereview.chromium.org/1671313002/diff/360001/ui/views/cocoa/views_s... ui/views/cocoa/views_scrollbar_bridge.mm:30: object:nil]; On 2016/02/22 20:49:25, spqchan wrote: > On 2016/02/22 19:37:07, Avi wrote: > > Won't this end up with a double registration if called with a new delegate > while > > already registered with one? > > True, I changed it so it only sets the delegate when it initializes. I think there's a nicer way to fix this, just by reordering the statements, like if (delegate_) [[NSNotificationCenter defaultCenter] removeObserver:self]; delegate_ = delegate; if (delegate_) { [[NSNotificationCenter defaultCenter] addObserver:self ...etc]; But we can alternatively keep the initializer approach too - see later comment https://codereview.chromium.org/1671313002/diff/380001/ui/views/cocoa/views_s... File ui/views/cocoa/views_scrollbar_bridge.mm (right): https://codereview.chromium.org/1671313002/diff/380001/ui/views/cocoa/views_s... ui/views/cocoa/views_scrollbar_bridge.mm:20: if (self = [super init]) { needs extra parens around assignment used as condition https://codereview.chromium.org/1671313002/diff/380001/ui/views/cocoa/views_s... ui/views/cocoa/views_scrollbar_bridge.mm:33: [[NSNotificationCenter defaultCenter] removeObserver:self]; I'm not really a fan of this approach. I guess here it's unlikely to come up, but ObjC objects have a different lifetime than C++ objects. If something creates an extra reference to a `ViewsScrollbarBridge`, dealloc won't be called, and it can still receive notifications; sending them on to a |delegate_| which could now be a freed object. I'd prefer an explicit `clearDelegate` method which removes the observer and sets the delegate to nullptr; Then keep the DCHECK in dealloc. https://codereview.chromium.org/1671313002/diff/380001/ui/views/controls/scro... File ui/views/controls/scrollbar/cocoa_scroll_bar.mm (right): https://codereview.chromium.org/1671313002/diff/380001/ui/views/controls/scro... ui/views/controls/scrollbar/cocoa_scroll_bar.mm:43: // Add an alpha channel for the overlay-style scroll track. Should say 'TODO' in the comment https://codereview.chromium.org/1671313002/diff/380001/ui/views/controls/scro... ui/views/controls/scrollbar/cocoa_scroll_bar.mm:259: layer()->SetOpacity(0.0); // Don't fade out nit: full stop after comment.
PTAL https://codereview.chromium.org/1671313002/diff/380001/ui/views/cocoa/views_s... File ui/views/cocoa/views_scrollbar_bridge.mm (right): https://codereview.chromium.org/1671313002/diff/380001/ui/views/cocoa/views_s... ui/views/cocoa/views_scrollbar_bridge.mm:20: if (self = [super init]) { On 2016/02/22 22:34:57, tapted wrote: > needs extra parens around assignment used as condition Done. https://codereview.chromium.org/1671313002/diff/380001/ui/views/cocoa/views_s... ui/views/cocoa/views_scrollbar_bridge.mm:33: [[NSNotificationCenter defaultCenter] removeObserver:self]; On 2016/02/22 22:34:57, tapted wrote: > I'm not really a fan of this approach. I guess here it's unlikely to come up, > but ObjC objects have a different lifetime than C++ objects. If something > creates an extra reference to a `ViewsScrollbarBridge`, dealloc won't be called, > and it can still receive notifications; sending them on to a |delegate_| which > could now be a freed object. > > I'd prefer an explicit `clearDelegate` method which removes the observer and > sets the delegate to nullptr; Then keep the DCHECK in dealloc. Added a clearDelegate method https://codereview.chromium.org/1671313002/diff/380001/ui/views/controls/scro... File ui/views/controls/scrollbar/cocoa_scroll_bar.mm (right): https://codereview.chromium.org/1671313002/diff/380001/ui/views/controls/scro... ui/views/controls/scrollbar/cocoa_scroll_bar.mm:43: // Add an alpha channel for the overlay-style scroll track. On 2016/02/22 22:34:57, tapted wrote: > Should say 'TODO' in the comment Done. https://codereview.chromium.org/1671313002/diff/380001/ui/views/controls/scro... ui/views/controls/scrollbar/cocoa_scroll_bar.mm:259: layer()->SetOpacity(0.0); // Don't fade out On 2016/02/22 22:34:57, tapted wrote: > nit: full stop after comment. Done.
lgtm % nits https://codereview.chromium.org/1671313002/diff/440001/content/browser/site_p... File content/browser/site_per_process_browsertest.cc (right): https://codereview.chromium.org/1671313002/diff/440001/content/browser/site_p... content/browser/site_per_process_browsertest.cc:1: // Copyright (c) 2012 The Chromium Authors. All rights reserved. CL Description: summary field and first line should always match. There's also a stray colon at the bottom (hehe) https://codereview.chromium.org/1671313002/diff/440001/ui/views/cocoa/views_s... File ui/views/cocoa/views_scrollbar_bridge.mm (right): https://codereview.chromium.org/1671313002/diff/440001/ui/views/cocoa/views_s... ui/views/cocoa/views_scrollbar_bridge.mm:31: -(void)clearDelegate { nit: space after `-` https://codereview.chromium.org/1671313002/diff/440001/ui/views/cocoa/views_s... ui/views/cocoa/views_scrollbar_bridge.mm:36: - (void)dealloc { nit: move dealloc up -- should always come after initializers https://codereview.chromium.org/1671313002/diff/440001/ui/views/controls/scro... File ui/views/controls/scrollbar/cocoa_scroll_bar.mm (right): https://codereview.chromium.org/1671313002/diff/440001/ui/views/controls/scro... ui/views/controls/scrollbar/cocoa_scroll_bar.mm:260: // Hide the scrollbar, but don't fade out. nit: comments count towards the needs-curlies rule, so the `if` and the `else` should get curlies (I like the clearer comment :)).
Description was changed from ========== Implemented scrollbars that follow the style of Cocoa. The scrollbars are implemented in CocoaScrollBar. They switch between overlay and non-overlay style, depending on the system's preference If using the overlay style, the scrollbars will: - Hide if unused - Display the scrolltrack if the user hovers over it - Currently hovering will not adjust the width. That will come later. In addition, since we are switching to CocoaScrollbars on OSX, dead code is cleaned up in native_theme_mac. Tests are modified and written for the overlay scrollbars. Changes are made to scoped_preferred_scroller_style_legacy_mac so that we can force the scroller style to be non-overlay and overlay in our tests. BUG=568409: ========== to ========== MacViews: Overlay Scrollbars with Show/Hide Animations The scrollbars are implemented in CocoaScrollBar. They switch between overlay and non-overlay style, depending on the system's preference If using the overlay style, the scrollbars will: - Hide if unused - Display the scrolltrack if the user hovers over it - Currently hovering will not adjust the width. That will come later. In addition, since we are switching to CocoaScrollbars on OSX, dead code is cleaned up in native_theme_mac. Tests are modified and written for the overlay scrollbars. Changes are made to scoped_preferred_scroller_style_legacy_mac so that we can force the scroller style to be non-overlay and overlay in our tests. BUG=568409: ==========
Description was changed from ========== MacViews: Overlay Scrollbars with Show/Hide Animations The scrollbars are implemented in CocoaScrollBar. They switch between overlay and non-overlay style, depending on the system's preference If using the overlay style, the scrollbars will: - Hide if unused - Display the scrolltrack if the user hovers over it - Currently hovering will not adjust the width. That will come later. In addition, since we are switching to CocoaScrollbars on OSX, dead code is cleaned up in native_theme_mac. Tests are modified and written for the overlay scrollbars. Changes are made to scoped_preferred_scroller_style_legacy_mac so that we can force the scroller style to be non-overlay and overlay in our tests. BUG=568409: ========== to ========== MacViews: Overlay Scrollbars with Show/Hide Animations The scrollbars are implemented in CocoaScrollBar. They switch between overlay and non-overlay style, depending on the system's preference If using the overlay style, the scrollbars will: - Hide if unused - Display the scrolltrack if the user hovers over it - Currently hovering will not adjust the width. That will come later. In addition, since we are switching to CocoaScrollbars on OSX, dead code is cleaned up in native_theme_mac. Tests are modified and written for the overlay scrollbars. Changes are made to scoped_preferred_scroller_style_legacy_mac so that we can force the scroller style to be non-overlay and overlay in our tests. BUG=568409 ==========
https://codereview.chromium.org/1671313002/diff/440001/ui/views/cocoa/views_s... File ui/views/cocoa/views_scrollbar_bridge.mm (right): https://codereview.chromium.org/1671313002/diff/440001/ui/views/cocoa/views_s... ui/views/cocoa/views_scrollbar_bridge.mm:31: -(void)clearDelegate { On 2016/02/22 23:41:55, tapted wrote: > nit: space after `-` Done. https://codereview.chromium.org/1671313002/diff/440001/ui/views/cocoa/views_s... ui/views/cocoa/views_scrollbar_bridge.mm:36: - (void)dealloc { On 2016/02/22 23:41:55, tapted wrote: > nit: move dealloc up -- should always come after initializers Done. https://codereview.chromium.org/1671313002/diff/440001/ui/views/controls/scro... File ui/views/controls/scrollbar/cocoa_scroll_bar.mm (right): https://codereview.chromium.org/1671313002/diff/440001/ui/views/controls/scro... ui/views/controls/scrollbar/cocoa_scroll_bar.mm:260: // Hide the scrollbar, but don't fade out. On 2016/02/22 23:41:56, tapted wrote: > nit: comments count towards the needs-curlies rule, so the `if` and the `else` > should get curlies (I like the clearer comment :)). Done.
The CQ bit was checked by spqchan@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from avi@chromium.org, tapted@chromium.org Link to the patchset: https://codereview.chromium.org/1671313002/#ps460001 (title: "nits")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1671313002/460001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1671313002/460001
Message was sent while issue was closed.
Description was changed from ========== MacViews: Overlay Scrollbars with Show/Hide Animations The scrollbars are implemented in CocoaScrollBar. They switch between overlay and non-overlay style, depending on the system's preference If using the overlay style, the scrollbars will: - Hide if unused - Display the scrolltrack if the user hovers over it - Currently hovering will not adjust the width. That will come later. In addition, since we are switching to CocoaScrollbars on OSX, dead code is cleaned up in native_theme_mac. Tests are modified and written for the overlay scrollbars. Changes are made to scoped_preferred_scroller_style_legacy_mac so that we can force the scroller style to be non-overlay and overlay in our tests. BUG=568409 ========== to ========== MacViews: Overlay Scrollbars with Show/Hide Animations The scrollbars are implemented in CocoaScrollBar. They switch between overlay and non-overlay style, depending on the system's preference If using the overlay style, the scrollbars will: - Hide if unused - Display the scrolltrack if the user hovers over it - Currently hovering will not adjust the width. That will come later. In addition, since we are switching to CocoaScrollbars on OSX, dead code is cleaned up in native_theme_mac. Tests are modified and written for the overlay scrollbars. Changes are made to scoped_preferred_scroller_style_legacy_mac so that we can force the scroller style to be non-overlay and overlay in our tests. BUG=568409 ==========
Message was sent while issue was closed.
Committed patchset #24 (id:460001)
Message was sent while issue was closed.
Description was changed from ========== MacViews: Overlay Scrollbars with Show/Hide Animations The scrollbars are implemented in CocoaScrollBar. They switch between overlay and non-overlay style, depending on the system's preference If using the overlay style, the scrollbars will: - Hide if unused - Display the scrolltrack if the user hovers over it - Currently hovering will not adjust the width. That will come later. In addition, since we are switching to CocoaScrollbars on OSX, dead code is cleaned up in native_theme_mac. Tests are modified and written for the overlay scrollbars. Changes are made to scoped_preferred_scroller_style_legacy_mac so that we can force the scroller style to be non-overlay and overlay in our tests. BUG=568409 ========== to ========== MacViews: Overlay Scrollbars with Show/Hide Animations The scrollbars are implemented in CocoaScrollBar. They switch between overlay and non-overlay style, depending on the system's preference If using the overlay style, the scrollbars will: - Hide if unused - Display the scrolltrack if the user hovers over it - Currently hovering will not adjust the width. That will come later. In addition, since we are switching to CocoaScrollbars on OSX, dead code is cleaned up in native_theme_mac. Tests are modified and written for the overlay scrollbars. Changes are made to scoped_preferred_scroller_style_legacy_mac so that we can force the scroller style to be non-overlay and overlay in our tests. BUG=568409 Committed: https://crrev.com/83534ab036f32a6aa7417e64fa3bc37a0b61be9d Cr-Commit-Position: refs/heads/master@{#377052} ==========
Message was sent while issue was closed.
Patchset 24 (id:??) landed as https://crrev.com/83534ab036f32a6aa7417e64fa3bc37a0b61be9d Cr-Commit-Position: refs/heads/master@{#377052} |
