Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1518543002:
Adds MD ink ripple animations to buttons within location bar (Closed)
|
Created:
5 years ago by varkha Modified:
4 years, 10 months ago CC:
chromium-reviews, tdanderson+views_chromium.org, msw+watch_chromium.org, tfarina, groby+bubble_chromium.org, hcarmona+bubble_chromium.org, rouslan+bubble_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionAdds MD ink ripple animations to buttons within location bar
BUG=518941
TEST=Click the possible icons in location bar:
Bookmark (Star) - always there
Zoom (Ctrl-+ to see it)
Translate (navigate to a page in foreign language)
Save Password - navigate to a secure page and enter credentials
Save Credit Card - navigate to a booking site and enter a bogus CC.
Allow popups and use http://www.popuptest.com/ to get a popup blocker view.
For all those buttons a click should produce an ink ripple animation.
TBR=sadrul@chromium.org
Committed: https://crrev.com/9b3dd66a8dce80938cb51578f128416d2d539595
Cr-Commit-Position: refs/heads/master@{#372279}
Patch Set 1 : Adds MD ink ripple animations to buttons within location bar (tweaks) #Patch Set 2 : Adds MD ink ripple animations to buttons within location bar (split gesture support into a separate… #
Total comments: 2
Patch Set 3 : Adds MD ink ripple animations to buttons within location bar (proper Z-order) #Patch Set 4 : Adds MD ink ripple animations to buttons within location bar (fixed test build) #
Total comments: 20
Patch Set 5 : Adds MD ink ripple animations to buttons within location bar (addressed most comments and rebased) #Patch Set 6 : Adds MD ink ripple animations to buttons within location bar (test fix) #Patch Set 7 : Adds MD ink ripple animations to buttons within location bar (with WidgetObserver) #
Total comments: 5
Patch Set 8 : Adds MD ink ripple animations to buttons within location bar (nits) #Patch Set 9 : Adds MD ink ripple animations to buttons within location bar (AddObserver in BubbleIconView) #Patch Set 10 : Adds MD ink ripple animations to buttons within location bar (reverted a bad change) #
Total comments: 6
Patch Set 11 : Adds MD ink ripple animations to buttons within location bar (Avoids multiple calls to observe) #Patch Set 12 : Adds MD ink ripple animations to buttons within location bar (sadrul's comments) #
Total comments: 3
Patch Set 13 : Adds MD ink ripple animations to buttons within location bar (fixes for immersive mode) #
Total comments: 6
Patch Set 14 : Adds MD ink ripple animations to buttons within location bar (added ContentSettingImageView) #
Total comments: 3
Patch Set 15 : Adds MD ink ripple animations to buttons within location bar (missing member init) #Messages
Total messages: 99 (41 generated)
varkha@chromium.org changed reviewers: + bruthig@chromium.org
bruthig@, early comments? I think it would be good time to move the size to LayoutConstants.
Patchset #1 (id:1) has been deleted
+1 To moving size constants to LayoutConstants! First off it would probably make sense to split this CL in to two. 1. Application of MD ink ripple to location bar buttons 2. Addition of InkDropHost::SupportsState() The work done towards 1 looks good with a few concerns inline. Part 2 stuff looks very promising but I think there are a few details we need to flush out. Namely, - Is SupportsState() using a white list or black list paradigm? In other words, should a child class call a parent classes SupportsState() and factor its return value in? And if so, with an && or an ||? - Will SupportState() be purely based off of the input parameter or will the return value ever depend on any object state? What consequences are there if this might depend off of object state? - How will this tie in to having ripples on Windows for mouse only and not touch? https://codereview.chromium.org/1518543002/diff/40001/chrome/browser/ui/views... File chrome/browser/ui/views/bar_control_button.cc (right): https://codereview.chromium.org/1518543002/diff/40001/chrome/browser/ui/views... chrome/browser/ui/views/bar_control_button.cc:92: return state == views::InkDropState::QUICK_ACTION || I would have expected this to return true for the following states as well: HIDDEN, ACTION_PENDING, SLOW_ACTION_PENDING, and DEACTIVATED. Similar point applies in other SupportsState() definitions. https://codereview.chromium.org/1518543002/diff/40001/chrome/browser/ui/views... File chrome/browser/ui/views/location_bar/bubble_icon_view.cc (right): https://codereview.chromium.org/1518543002/diff/40001/chrome/browser/ui/views... chrome/browser/ui/views/location_bar/bubble_icon_view.cc:67: ink_drop_delegate_->OnAction(views::InkDropState::ACTIVATED); Going to the ACTIVATED state on mouse pressed doesn't seem to be consistent with the comment below. i.e. // We want to show the bubble on mouse release; that is the standard behavior // for buttons. Should this be animating to ACTION_PENDING instead? https://codereview.chromium.org/1518543002/diff/40001/chrome/browser/ui/views... chrome/browser/ui/views/location_bar/bubble_icon_view.cc:86: ink_drop_delegate_->OnAction(views::InkDropState::HIDDEN); Will the button action be triggered in the following scenario? 1. Left mouse down 2. Right mouse down 3. Right mouse up 4. Left mouse up If so, then I suspect this line will cause the ripple to disappear after step 3. when the right mouse button is released which would be incorrect. https://codereview.chromium.org/1518543002/diff/40001/chrome/browser/ui/views... File chrome/browser/ui/views/location_bar/bubble_icon_view.h (right): https://codereview.chromium.org/1518543002/diff/40001/chrome/browser/ui/views... chrome/browser/ui/views/location_bar/bubble_icon_view.h:113: // Animation delegate for the ink drop ripple effect. It is owned by a Copy/paste error, the 2nd sentence in the comment is incorrect. https://codereview.chromium.org/1518543002/diff/40001/ui/views/animation/butt... File ui/views/animation/button_ink_drop_delegate.cc (right): https://codereview.chromium.org/1518543002/diff/40001/ui/views/animation/butt... ui/views/animation/button_ink_drop_delegate.cc:43: ink_drop_animation_controller_->AnimateToState(state); I wonder if we should have a D/CHECK(ink_drop_host_->SupportsState(state)) here... https://codereview.chromium.org/1518543002/diff/40001/ui/views/animation/butt... ui/views/animation/button_ink_drop_delegate.cc:60: return; This probably shouldn't be a quick return, this should animate to HIDDEN. Similar below.
On 2015/12/09 23:12:19, bruthig wrote: > +1 To moving size constants to LayoutConstants! > > First off it would probably make sense to split this CL in to two. > 1. Application of MD ink ripple to location bar buttons > 2. Addition of InkDropHost::SupportsState() > > The work done towards 1 looks good with a few concerns inline. > > Part 2 stuff looks very promising but I think there are a few details we need to > flush out. Namely, > - Is SupportsState() using a white list or black list paradigm? In other > words, should a child class call a parent classes SupportsState() and factor its > return value in? And if so, with an && or an ||? > - Will SupportState() be purely based off of the input parameter or will the > return value ever depend on any object state? What consequences are there if > this might depend off of object state? > - How will this tie in to having ripples on Windows for mouse only and not > touch? > One more point: - Will the SupportsState() method still live if we transition to a View-state based controller (as opposed to input events)?
Patchset #2 (id:40001) has been deleted
Patchset #2 (id:60001) has been deleted
Deleted the SupportsState patch by mistake, I will upload it separately. Please take a look at the rest of step 1.
Sorry I didn't notice this point in the first pass. https://codereview.chromium.org/1518543002/diff/80001/chrome/browser/ui/views... File chrome/browser/ui/views/location_bar/bubble_icon_view.cc (right): https://codereview.chromium.org/1518543002/diff/80001/chrome/browser/ui/views... chrome/browser/ui/views/location_bar/bubble_icon_view.cc:126: SetPaintToLayer(true); Does this configuration actually paint the ripple behind the icon? To me it seems like it won't because the ImageView::image_ is painted in the ImageView::OnPaint() method which I believe will be painting to the parent layer of the ink drop layer. We might need to make ImageView extend InkDropHost instead.
PTAL. https://codereview.chromium.org/1518543002/diff/80001/chrome/browser/ui/views... File chrome/browser/ui/views/location_bar/bubble_icon_view.cc (right): https://codereview.chromium.org/1518543002/diff/80001/chrome/browser/ui/views... chrome/browser/ui/views/location_bar/bubble_icon_view.cc:126: SetPaintToLayer(true); On 2015/12/11 17:43:38, bruthig wrote: > Does this configuration actually paint the ripple behind the icon? To me it > seems like it won't because the ImageView::image_ is painted in the > ImageView::OnPaint() method which I believe will be painting to the parent layer > of the ink drop layer. > We might need to make ImageView extend InkDropHost instead. I have changed BubbleIconView to have ImageView as a child rather than extend ImageView. This allows same mechanism as used for ToolbarButtons to work here.
The CQ bit was checked by varkha@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1518543002/100001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1518543002/100001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_chromeos_compile_dbg_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by varkha@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1518543002/120001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1518543002/120001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
varkha@chromium.org changed reviewers: + pkasting@chromium.org, sadrul@chromium.org
sadrul, pkasting, can you please take a look? I have experimented without having a BubbleIconView own an ImageView but having the ink drop ripple below the icon would force the ink drop host to be the location bar (since BubbleIconView used to just subclass painting). This CL doesn't yet deal with touch gestures, that will be a followup. Thanks!
Minor nit, otherwise lgtm https://codereview.chromium.org/1518543002/diff/120001/chrome/browser/ui/view... File chrome/browser/ui/views/location_bar/bubble_icon_view.h (right): https://codereview.chromium.org/1518543002/diff/120001/chrome/browser/ui/view... chrome/browser/ui/views/location_bar/bubble_icon_view.h:47: // Returns the image currently displayed or NULL of none is currently set. Can you update this comment, and the one in ImageView, because it can't actually return NULL?
https://codereview.chromium.org/1518543002/diff/120001/chrome/browser/ui/view... File chrome/browser/ui/views/location_bar/bubble_icon_view.cc (right): https://codereview.chromium.org/1518543002/diff/120001/chrome/browser/ui/view... chrome/browser/ui/views/location_bar/bubble_icon_view.cc:72: return image_->GetTooltipText(p, tooltip); Nit: Simpler: return IsBubbleShowing() && image_->GetTooltipText(p, tooltip); https://codereview.chromium.org/1518543002/diff/120001/chrome/browser/ui/view... chrome/browser/ui/views/location_bar/bubble_icon_view.cc:112: } Nit: Shorter: const bool activated = HitTestPoint(event.location()); ink_drop_delegate_->OnAction( activated ? views::InkDropState::ACTIVATED : views::InkDropState::HIDDEN); if (activated) ExecuteCommand(EXECUTE_SOURCE_MOUSE); https://codereview.chromium.org/1518543002/diff/120001/chrome/browser/ui/view... File chrome/browser/ui/views/location_bar/bubble_icon_view.h (right): https://codereview.chromium.org/1518543002/diff/120001/chrome/browser/ui/view... chrome/browser/ui/views/location_bar/bubble_icon_view.h:36: static const char kViewClassName[]; Why does this need to be a public static? Why can't it just be a string constant inside GetClassName()? https://codereview.chromium.org/1518543002/diff/120001/chrome/browser/ui/view... chrome/browser/ui/views/location_bar/bubble_icon_view.h:48: // The returned image is owned by the ImageView. Nit: "the ImageView" is vague. I would explicitly say "|image_|". https://codereview.chromium.org/1518543002/diff/120001/chrome/browser/ui/view... chrome/browser/ui/views/location_bar/bubble_icon_view.h:87: virtual gfx::VectorIconId GetVectorIcon() const; Why can't we leave this pure virtual? https://codereview.chromium.org/1518543002/diff/120001/chrome/browser/ui/view... chrome/browser/ui/views/location_bar/bubble_icon_view.h:95: const char* GetClassName() const override; Do we really need to override this? Seems like nothing checks for this view's particular class name. https://codereview.chromium.org/1518543002/diff/120001/chrome/browser/ui/view... File chrome/browser/ui/views/passwords/manage_passwords_icon_view_interactive_uitest.cc (right): https://codereview.chromium.org/1518543002/diff/120001/chrome/browser/ui/view... chrome/browser/ui/views/passwords/manage_passwords_icon_view_interactive_uitest.cc:46: const gfx::ImageSkia& GetImage() { Why bother factoring this out? https://codereview.chromium.org/1518543002/diff/120001/ui/views/view.h File ui/views/view.h (right): https://codereview.chromium.org/1518543002/diff/120001/ui/views/view.h#newcod... ui/views/view.h:1003: virtual void OnBubbleClosing() {} I'm not terribly keen on adding this sort of specific functionality to all views, just so we can call a method in one class. It seems like a better route would be for BubbleIconView to explicitly register itself as a listener on the bubble delegate, to be called when the bubble closed.
msw@chromium.org changed reviewers: + msw@chromium.org
https://codereview.chromium.org/1518543002/diff/120001/ui/views/view.h File ui/views/view.h (right): https://codereview.chromium.org/1518543002/diff/120001/ui/views/view.h#newcod... ui/views/view.h:1003: virtual void OnBubbleClosing() {} On 2016/01/26 16:59:35, Peter Kasting wrote: > I'm not terribly keen on adding this sort of specific functionality to all > views, just so we can call a method in one class. > > It seems like a better route would be for BubbleIconView to explicitly register > itself as a listener on the bubble delegate, to be called when the bubble > closed. Or observe the bubble as a widget observer.
Still working on the bubble closing but wanted to post the intermediate state. Thanks for the fast comments! https://codereview.chromium.org/1518543002/diff/120001/chrome/browser/ui/view... File chrome/browser/ui/views/location_bar/bubble_icon_view.cc (right): https://codereview.chromium.org/1518543002/diff/120001/chrome/browser/ui/view... chrome/browser/ui/views/location_bar/bubble_icon_view.cc:72: return image_->GetTooltipText(p, tooltip); On 2016/01/26 16:59:35, Peter Kasting wrote: > Nit: Simpler: > > return IsBubbleShowing() && image_->GetTooltipText(p, tooltip); Done. https://codereview.chromium.org/1518543002/diff/120001/chrome/browser/ui/view... chrome/browser/ui/views/location_bar/bubble_icon_view.cc:112: } On 2016/01/26 16:59:35, Peter Kasting wrote: > Nit: Shorter: > > const bool activated = HitTestPoint(event.location()); > ink_drop_delegate_->OnAction( > activated ? views::InkDropState::ACTIVATED : views::InkDropState::HIDDEN); > if (activated) > ExecuteCommand(EXECUTE_SOURCE_MOUSE); Done. https://codereview.chromium.org/1518543002/diff/120001/chrome/browser/ui/view... File chrome/browser/ui/views/location_bar/bubble_icon_view.h (right): https://codereview.chromium.org/1518543002/diff/120001/chrome/browser/ui/view... chrome/browser/ui/views/location_bar/bubble_icon_view.h:36: static const char kViewClassName[]; On 2016/01/26 16:59:35, Peter Kasting wrote: > Why does this need to be a public static? Why can't it just be a string > constant inside GetClassName()? Done. https://codereview.chromium.org/1518543002/diff/120001/chrome/browser/ui/view... chrome/browser/ui/views/location_bar/bubble_icon_view.h:47: // Returns the image currently displayed or NULL of none is currently set. On 2016/01/26 16:47:25, bruthig wrote: > Can you update this comment, and the one in ImageView, because it can't actually > return NULL? Done. https://codereview.chromium.org/1518543002/diff/120001/chrome/browser/ui/view... chrome/browser/ui/views/location_bar/bubble_icon_view.h:48: // The returned image is owned by the ImageView. On 2016/01/26 16:59:35, Peter Kasting wrote: > Nit: "the ImageView" is vague. I would explicitly say "|image_|". Done. https://codereview.chromium.org/1518543002/diff/120001/chrome/browser/ui/view... chrome/browser/ui/views/location_bar/bubble_icon_view.h:87: virtual gfx::VectorIconId GetVectorIcon() const; On 2016/01/26 16:59:35, Peter Kasting wrote: > Why can't we leave this pure virtual? Because it gets called for the first time from inside a derived class constructor (via UpdateIcon) and that would cause a pure virtual call. https://codereview.chromium.org/1518543002/diff/120001/chrome/browser/ui/view... chrome/browser/ui/views/location_bar/bubble_icon_view.h:95: const char* GetClassName() const override; On 2016/01/26 16:59:35, Peter Kasting wrote: > Do we really need to override this? Seems like nothing checks for this view's > particular class name. We don't. I used it for debugging. Removed. https://codereview.chromium.org/1518543002/diff/120001/chrome/browser/ui/view... File chrome/browser/ui/views/passwords/manage_passwords_icon_view_interactive_uitest.cc (right): https://codereview.chromium.org/1518543002/diff/120001/chrome/browser/ui/view... chrome/browser/ui/views/passwords/manage_passwords_icon_view_interactive_uitest.cc:46: const gfx::ImageSkia& GetImage() { On 2016/01/26 16:59:35, Peter Kasting wrote: > Why bother factoring this out? BubbleIconView::GetImage() is protected and this seemed like a typical pattern with having a test fixture class be a friend of the ManagePasswordsIconViews (which extends BubbleIconView). An alternative would be to override and make ManagePasswordsIconViews::GetImage() public but I think this is simpler. https://codereview.chromium.org/1518543002/diff/120001/ui/views/view.h File ui/views/view.h (right): https://codereview.chromium.org/1518543002/diff/120001/ui/views/view.h#newcod... ui/views/view.h:1003: virtual void OnBubbleClosing() {} On 2016/01/26 16:59:35, Peter Kasting wrote: > I'm not terribly keen on adding this sort of specific functionality to all > views, just so we can call a method in one class. > > It seems like a better route would be for BubbleIconView to explicitly register > itself as a listener on the bubble delegate, to be called when the bubble > closed. I will try to find a way around it. https://codereview.chromium.org/1518543002/diff/120001/ui/views/view.h#newcod... ui/views/view.h:1003: virtual void OnBubbleClosing() {} On 2016/01/26 17:42:32, msw wrote: > On 2016/01/26 16:59:35, Peter Kasting wrote: > > I'm not terribly keen on adding this sort of specific functionality to all > > views, just so we can call a method in one class. > > > > It seems like a better route would be for BubbleIconView to explicitly > register > > itself as a listener on the bubble delegate, to be called when the bubble > > closed. > > Or observe the bubble as a widget observer. Thanks, msw@, I will take a look at what's involved.
The CQ bit was checked by varkha@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1518543002/140001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1518543002/140001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_chromeos_rel_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
Patchset #7 (id:180001) has been deleted
Patchset #7 (id:200001) has been deleted
Patchset #7 (id:220001) has been deleted
Patchset #7 (id:240001) has been deleted
https://codereview.chromium.org/1518543002/diff/260001/ui/views/bubble/bubble... File ui/views/bubble/bubble_delegate.h (right): https://codereview.chromium.org/1518543002/diff/260001/ui/views/bubble/bubble... ui/views/bubble/bubble_delegate.h:41: BubbleDelegateView(WidgetObserverView* anchor_view, msw@, pkasting@, did you have in mind something like this?
The CQ bit was checked by varkha@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1518543002/260001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1518543002/260001
LGTM. The |has_widget_observer_view_| stuff is kinda ugly but I don't have a better idea. https://codereview.chromium.org/1518543002/diff/260001/chrome/browser/ui/view... File chrome/browser/ui/views/location_bar/bubble_icon_view.h (right): https://codereview.chromium.org/1518543002/diff/260001/chrome/browser/ui/view... chrome/browser/ui/views/location_bar/bubble_icon_view.h:46: // Returns the image currently displayed which can be empty if not set. Nit: Comma after "displayed" https://codereview.chromium.org/1518543002/diff/260001/ui/views/controls/imag... File ui/views/controls/image_view.h (right): https://codereview.chromium.org/1518543002/diff/260001/ui/views/controls/imag... ui/views/controls/image_view.h:53: // Returns the image currently displayed which can be empty if not set. Nit: Comma after "displayed"
The CQ bit was checked by varkha@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1518543002/280001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1518543002/280001
Thanks for quick turnaround! https://codereview.chromium.org/1518543002/diff/260001/chrome/browser/ui/view... File chrome/browser/ui/views/location_bar/bubble_icon_view.h (right): https://codereview.chromium.org/1518543002/diff/260001/chrome/browser/ui/view... chrome/browser/ui/views/location_bar/bubble_icon_view.h:46: // Returns the image currently displayed which can be empty if not set. On 2016/01/28 00:22:08, Peter Kasting wrote: > Nit: Comma after "displayed" Done. https://codereview.chromium.org/1518543002/diff/260001/ui/views/controls/imag... File ui/views/controls/image_view.h (right): https://codereview.chromium.org/1518543002/diff/260001/ui/views/controls/imag... ui/views/controls/image_view.h:53: // Returns the image currently displayed which can be empty if not set. On 2016/01/28 00:22:08, Peter Kasting wrote: > Nit: Comma after "displayed" Done.
NOT LGTM. I don't really like the direction of this change. widget_observer_view.h should not be necessary. BubbleDelegateView::CreateBubble returns a widget, BubbleIconView should just subclass View and WidgetObserver, and then add itself as an observer of that bubble widget, etc. Building logic into bubble delegate to manage who is observing it is weird...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_chromeos_compile_dbg_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by varkha@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1518543002/300001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1518543002/300001
On 2016/01/28 00:40:55, msw wrote: > NOT LGTM. I don't really like the direction of this change. > widget_observer_view.h should not be necessary. > BubbleDelegateView::CreateBubble returns a widget, BubbleIconView should just > subclass View and WidgetObserver, and then add itself as an observer of that > bubble widget, etc. Building logic into bubble delegate to manage who is > observing it is weird... Thanks, msw. PTAL, I've tried this in the last patch set. I am using the WidgetObserverView to get around ToolbarView::GetBookmarkBubbleAnchor() and such that may return a BubbleIconView (save_cc / star / translate) but may also retrun the app_menu_button so I need to catch both cases in a same class.
Patchset #9 (id:300001) has been deleted
The CQ bit was checked by varkha@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1518543002/320001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1518543002/320001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_chromium_rel_ng on tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
The CQ bit was checked by varkha@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1518543002/340001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1518543002/340001
sadrul@, ping for comments in ui/views/.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_chromeos_rel_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
https://codereview.chromium.org/1518543002/diff/340001/chrome/browser/ui/view... File chrome/browser/ui/views/frame/browser_view.cc (right): https://codereview.chromium.org/1518543002/diff/340001/chrome/browser/ui/view... chrome/browser/ui/views/frame/browser_view.cc:1298: BookmarkBubbleView::bookmark_bubble()->GetWidget()); I would do this differently: . BubbleIconView becomes a WidgetObserver, and BubbleIconView::ObserveBubble(Widget*) is added to notify it when there's a bubble anchored to it. . BrowserView notifies the ToolbarView when a bubble is created, e.g.: views::View* anchor = GetToolbarView()->GetBookmarkBubbleAnchor(); BookmarkBubbleView::ShowBubble(anchor, ....); // Can ShowBubble() return the Widget instead? views::Widget* bubble = BookmarkBubbleView::bookmark_bubble()->GetWidget(); GetToolbarView()->OnBubbleCreatedForAnchor(anchor, widget); . ToolbarView::OnBubbleCreatedForAnchor(views::View* anchor, views::Widget* bubble) looks like this: bool anchor_is_bubble_icon_view = anchor == location_bar()->star_view() || anchor == location_bar()->translate_icon_view() || anchor == location_bar()->save_credit_card_icon_view(); if (anchor_is_bubble_icon_view) static_cast<BubbleIconView*>(anchor)->ObserveBubble(bubble); This should take care of the issues, right? https://codereview.chromium.org/1518543002/diff/340001/ui/views/controls/butt... File ui/views/controls/button/button.h (right): https://codereview.chromium.org/1518543002/diff/340001/ui/views/controls/butt... ui/views/controls/button/button.h:30: class VIEWS_EXPORT Button : public WidgetObserverView { We shouldn't do this, of course. https://codereview.chromium.org/1518543002/diff/340001/ui/views/widget/widget... File ui/views/widget/widget_observer_view.h (right): https://codereview.chromium.org/1518543002/diff/340001/ui/views/widget/widget... ui/views/widget/widget_observer_view.h:21: #endif // UI_VIEWS_WIDGET_WIDGET_OBSERVER_VIEW_H_ I dislike this quite a bit. I don't think we should do this.
+1 to everything Sadrul said, but instead of adding BubbleIconView::ObserveBubble, just use Widget::AddObserver
Thanks, this ToolbarView handler is what I was missing. >> instead of adding BubbleIconView::ObserveBubble, just use Widget::AddObserver Done. https://codereview.chromium.org/1518543002/diff/340001/chrome/browser/ui/view... File chrome/browser/ui/views/frame/browser_view.cc (right): https://codereview.chromium.org/1518543002/diff/340001/chrome/browser/ui/view... chrome/browser/ui/views/frame/browser_view.cc:1298: BookmarkBubbleView::bookmark_bubble()->GetWidget()); On 2016/01/28 17:45:06, sadrul wrote: > I would do this differently: > > . BubbleIconView becomes a WidgetObserver, and > BubbleIconView::ObserveBubble(Widget*) is added to notify it when there's a > bubble anchored to it. > > . BrowserView notifies the ToolbarView when a bubble is created, e.g.: > > views::View* anchor = GetToolbarView()->GetBookmarkBubbleAnchor(); > BookmarkBubbleView::ShowBubble(anchor, ....); > // Can ShowBubble() return the Widget instead? > views::Widget* bubble = BookmarkBubbleView::bookmark_bubble()->GetWidget(); > GetToolbarView()->OnBubbleCreatedForAnchor(anchor, widget); > > > . ToolbarView::OnBubbleCreatedForAnchor(views::View* anchor, views::Widget* > bubble) looks like this: > > bool anchor_is_bubble_icon_view = > anchor == location_bar()->star_view() || > anchor == location_bar()->translate_icon_view() || > anchor == location_bar()->save_credit_card_icon_view(); > if (anchor_is_bubble_icon_view) > static_cast<BubbleIconView*>(anchor)->ObserveBubble(bubble); > > This should take care of the issues, right? Yes, this is precisely what I was looking for. Thanks for the idea! Done. https://codereview.chromium.org/1518543002/diff/340001/ui/views/controls/butt... File ui/views/controls/button/button.h (right): https://codereview.chromium.org/1518543002/diff/340001/ui/views/controls/butt... ui/views/controls/button/button.h:30: class VIEWS_EXPORT Button : public WidgetObserverView { On 2016/01/28 17:45:06, sadrul wrote: > We shouldn't do this, of course. Done. https://codereview.chromium.org/1518543002/diff/340001/ui/views/widget/widget... File ui/views/widget/widget_observer_view.h (right): https://codereview.chromium.org/1518543002/diff/340001/ui/views/widget/widget... ui/views/widget/widget_observer_view.h:21: #endif // UI_VIEWS_WIDGET_WIDGET_OBSERVER_VIEW_H_ On 2016/01/28 17:45:06, sadrul wrote: > I dislike this quite a bit. I don't think we should do this. Done.
Patchset #12 (id:380001) has been deleted
The CQ bit was checked by varkha@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1518543002/400001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1518543002/400001
Still LGTM https://codereview.chromium.org/1518543002/diff/400001/chrome/browser/ui/view... File chrome/browser/ui/views/toolbar/toolbar_view.cc (right): https://codereview.chromium.org/1518543002/diff/400001/chrome/browser/ui/view... chrome/browser/ui/views/toolbar/toolbar_view.cc:337: anchor_view == location_bar()->translate_icon_view())) { Nit: {} not necessary
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_chromium_rel_ng on tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
Some anchor views can be NULL when in immersive mode. Added DCHECK when this is not expected and condition when it is. https://codereview.chromium.org/1518543002/diff/400001/chrome/browser/ui/view... File chrome/browser/ui/views/toolbar/toolbar_view.cc (right): https://codereview.chromium.org/1518543002/diff/400001/chrome/browser/ui/view... chrome/browser/ui/views/toolbar/toolbar_view.cc:337: anchor_view == location_bar()->translate_icon_view())) { On 2016/01/28 20:42:12, Peter Kasting wrote: > Nit: {} not necessary I am used to the style guide allowing this[1] for complex conditions: "conditional or loop statements with complex conditions or statements may be more readable with curly braces." I think it helps keep track of the indentation. [1] https://google.github.io/styleguide/cppguide.html#Conditionals
The CQ bit was checked by varkha@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1518543002/410001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1518543002/410001
https://codereview.chromium.org/1518543002/diff/400001/chrome/browser/ui/view... File chrome/browser/ui/views/toolbar/toolbar_view.cc (right): https://codereview.chromium.org/1518543002/diff/400001/chrome/browser/ui/view... chrome/browser/ui/views/toolbar/toolbar_view.cc:337: anchor_view == location_bar()->translate_icon_view())) { On 2016/01/28 22:04:52, varkha wrote: > On 2016/01/28 20:42:12, Peter Kasting wrote: > > Nit: {} not necessary > > I am used to the style guide allowing this[1] for complex conditions: > "conditional or loop statements with complex conditions or statements may be > more readable with curly braces." I think it helps keep track of the > indentation. > > [1] https://google.github.io/styleguide/cppguide.html#Conditionals Right, it's allowed if you want it. Probably more c/b/ui/views/ code omits it than has it but there's no real consistency to mandate.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
lgtm with a nit and a q. static_cast isn't wonderful, but this is a much better change; thanks for addressing our comments. https://codereview.chromium.org/1518543002/diff/410001/chrome/browser/ui/view... File chrome/browser/ui/views/passwords/manage_passwords_icon_view_interactive_uitest.cc (right): https://codereview.chromium.org/1518543002/diff/410001/chrome/browser/ui/view... chrome/browser/ui/views/passwords/manage_passwords_icon_view_interactive_uitest.cc:46: const gfx::ImageSkia& GetImage() { nit: this change isn't necessary, afaict https://codereview.chromium.org/1518543002/diff/410001/chrome/browser/ui/view... File chrome/browser/ui/views/toolbar/toolbar_view.cc (right): https://codereview.chromium.org/1518543002/diff/410001/chrome/browser/ui/view... chrome/browser/ui/views/toolbar/toolbar_view.cc:339: bubble_widget->AddObserver(static_cast<BubbleIconView*>(anchor_view)); q: Did you want the app list view to actually show a ripple? Doesn't seem like this will handle that case.
https://codereview.chromium.org/1518543002/diff/410001/chrome/browser/ui/view... File chrome/browser/ui/views/passwords/manage_passwords_icon_view_interactive_uitest.cc (right): https://codereview.chromium.org/1518543002/diff/410001/chrome/browser/ui/view... chrome/browser/ui/views/passwords/manage_passwords_icon_view_interactive_uitest.cc:46: const gfx::ImageSkia& GetImage() { On 2016/01/28 23:27:42, msw wrote: > nit: this change isn't necessary, afaict No, I need this or some other solution like making GetImage public in BubbleIconView (and I like this more since nobody else needs it and this test class (but not the PendingState test fixture below) is already a friend of ManagePasswordsIconViews). https://codereview.chromium.org/1518543002/diff/410001/chrome/browser/ui/view... File chrome/browser/ui/views/toolbar/toolbar_view.cc (right): https://codereview.chromium.org/1518543002/diff/410001/chrome/browser/ui/view... chrome/browser/ui/views/toolbar/toolbar_view.cc:339: bubble_widget->AddObserver(static_cast<BubbleIconView*>(anchor_view)); On 2016/01/28 23:27:42, msw wrote: > q: Did you want the app list view to actually show a ripple? > Doesn't seem like this will handle that case. Do you mean |app_menu_button_|? If so then no, we don't want a ripple update in this case, only if the anchor is a BubbleIconView (which is where the ripple would be shown on click and so needs to be hidden when the bubble widget is hidden).
lgtm https://codereview.chromium.org/1518543002/diff/410001/chrome/browser/ui/view... File chrome/browser/ui/views/passwords/manage_passwords_icon_view_interactive_uitest.cc (right): https://codereview.chromium.org/1518543002/diff/410001/chrome/browser/ui/view... chrome/browser/ui/views/passwords/manage_passwords_icon_view_interactive_uitest.cc:46: const gfx::ImageSkia& GetImage() { On 2016/01/29 00:00:40, varkha wrote: > On 2016/01/28 23:27:42, msw wrote: > > nit: this change isn't necessary, afaict > > No, I need this or some other solution like making GetImage public in > BubbleIconView (and I like this more since nobody else needs it and this test > class (but not the PendingState test fixture below) is already a friend of > ManagePasswordsIconViews). Acknowledged. https://codereview.chromium.org/1518543002/diff/410001/chrome/browser/ui/view... File chrome/browser/ui/views/toolbar/toolbar_view.cc (right): https://codereview.chromium.org/1518543002/diff/410001/chrome/browser/ui/view... chrome/browser/ui/views/toolbar/toolbar_view.cc:339: bubble_widget->AddObserver(static_cast<BubbleIconView*>(anchor_view)); On 2016/01/29 00:00:40, varkha wrote: > On 2016/01/28 23:27:42, msw wrote: > > q: Did you want the app list view to actually show a ripple? > > Doesn't seem like this will handle that case. > > Do you mean |app_menu_button_|? If so then no, we don't want a ripple update in > this case, only if the anchor is a BubbleIconView (which is where the ripple > would be shown on click and so needs to be hidden when the bubble widget is > hidden). Sorry, yeah that's what I meant; acknowledged.
One more case added that I didn't remember was there until I saw crbug.com/582196. https://codereview.chromium.org/1518543002/diff/430001/chrome/browser/ui/view... File chrome/browser/ui/views/location_bar/content_setting_image_view.h (right): https://codereview.chromium.org/1518543002/diff/430001/chrome/browser/ui/view... chrome/browser/ui/views/location_bar/content_setting_image_view.h:43: public views::InkDropHost, I have noticed that this class controls more views to the right of the URL in location bar. Those views should also have a ripple. I have added the changes here to have them working at the same time but I will see if some refactoring of commonality is possible. Interestingly this class is already being the WidgetObserver, have I seen this earlier it would have been easier with the rest.
The CQ bit was checked by varkha@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1518543002/430001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1518543002/430001
Description was changed from ========== Adds MD ink ripple animations to buttons within location bar BUG=518941 ========== to ========== Adds MD ink ripple animations to buttons within location bar BUG=518941 TEST=Click the possible icons in location bar: Bookmark (Star) - always there Zoom (Ctrl-+ to see it) Translate (navigate to a page in foreign language) Save Password - navigate to a secure page and enter credentials Save Credit Card - navigate to a booking site and enter a bogus CC. Allow popups and use http://www.popuptest.com/ to get a popup blocker view. For all those buttons a click should produce an ink ripple animation. ==========
https://codereview.chromium.org/1518543002/diff/430001/chrome/browser/ui/view... File chrome/browser/ui/views/location_bar/content_setting_image_view.cc (right): https://codereview.chromium.org/1518543002/diff/430001/chrome/browser/ui/view... chrome/browser/ui/views/location_bar/content_setting_image_view.cc:50: bubble_widget_(nullptr), you can probably remove the |bubble_widget_| member if you're adding |bubble_view_|, and you should definitely initialize |bubble_view_| here.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_chromium_x64_rel_ng on tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)
The CQ bit was checked by varkha@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1518543002/450001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1518543002/450001
https://codereview.chromium.org/1518543002/diff/430001/chrome/browser/ui/view... File chrome/browser/ui/views/location_bar/content_setting_image_view.cc (right): https://codereview.chromium.org/1518543002/diff/430001/chrome/browser/ui/view... chrome/browser/ui/views/location_bar/content_setting_image_view.cc:50: bubble_widget_(nullptr), On 2016/01/29 01:10:10, msw wrote: > you can probably remove the |bubble_widget_| member if you're adding > |bubble_view_|, and you should definitely initialize |bubble_view_| here. Ouch. Done.
lgtm, but I idly wonder if the whole ink ripple thing could be done [more simply] at the location bar level (painting beneath the various bubble icon views).
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
> if the whole ink ripple thing could be done [more simply] at the location bar level. I will explore that. We still plan to simplify it a bit by pulling more ink ripple common functionality aside and making it state-driven as opposed to input event driven as now. Our plan was to get the visible pieces in so that they can be interacted with to gain time for feedback and refactor once the visuals are stable. In this case location bar could be or own an InkDropHost and delegate positioning to the child views. A challenge here is that different variations of buttons / views have slightly different event model and need tweaks to make the ripple consistent with the button state. E.g. content views (popup blocker icon) respond to any mouse buttons while zoom / bookmark and such only work on left mouse buttons. Little things like that may need to be stabilized first.
Description was changed from ========== Adds MD ink ripple animations to buttons within location bar BUG=518941 TEST=Click the possible icons in location bar: Bookmark (Star) - always there Zoom (Ctrl-+ to see it) Translate (navigate to a page in foreign language) Save Password - navigate to a secure page and enter credentials Save Credit Card - navigate to a booking site and enter a bogus CC. Allow popups and use http://www.popuptest.com/ to get a popup blocker view. For all those buttons a click should produce an ink ripple animation. ========== to ========== Adds MD ink ripple animations to buttons within location bar BUG=518941 TEST=Click the possible icons in location bar: Bookmark (Star) - always there Zoom (Ctrl-+ to see it) Translate (navigate to a page in foreign language) Save Password - navigate to a secure page and enter credentials Save Credit Card - navigate to a booking site and enter a bogus CC. Allow popups and use http://www.popuptest.com/ to get a popup blocker view. For all those buttons a click should produce an ink ripple animation. TBR=sadrul ==========
Description was changed from ========== Adds MD ink ripple animations to buttons within location bar BUG=518941 TEST=Click the possible icons in location bar: Bookmark (Star) - always there Zoom (Ctrl-+ to see it) Translate (navigate to a page in foreign language) Save Password - navigate to a secure page and enter credentials Save Credit Card - navigate to a booking site and enter a bogus CC. Allow popups and use http://www.popuptest.com/ to get a popup blocker view. For all those buttons a click should produce an ink ripple animation. TBR=sadrul ========== to ========== Adds MD ink ripple animations to buttons within location bar BUG=518941 TEST=Click the possible icons in location bar: Bookmark (Star) - always there Zoom (Ctrl-+ to see it) Translate (navigate to a page in foreign language) Save Password - navigate to a secure page and enter credentials Save Credit Card - navigate to a booking site and enter a bogus CC. Allow popups and use http://www.popuptest.com/ to get a popup blocker view. For all those buttons a click should produce an ink ripple animation. TBR=sadrul@chromium.org ==========
The changes in ui/views/ are now trivial, adding TBR=sadrul.
The CQ bit was checked by varkha@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from bruthig@chromium.org, pkasting@chromium.org Link to the patchset: https://codereview.chromium.org/1518543002/#ps450001 (title: "Adds MD ink ripple animations to buttons within location bar (missing member init)")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1518543002/450001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1518543002/450001
Message was sent while issue was closed.
Description was changed from ========== Adds MD ink ripple animations to buttons within location bar BUG=518941 TEST=Click the possible icons in location bar: Bookmark (Star) - always there Zoom (Ctrl-+ to see it) Translate (navigate to a page in foreign language) Save Password - navigate to a secure page and enter credentials Save Credit Card - navigate to a booking site and enter a bogus CC. Allow popups and use http://www.popuptest.com/ to get a popup blocker view. For all those buttons a click should produce an ink ripple animation. TBR=sadrul@chromium.org ========== to ========== Adds MD ink ripple animations to buttons within location bar BUG=518941 TEST=Click the possible icons in location bar: Bookmark (Star) - always there Zoom (Ctrl-+ to see it) Translate (navigate to a page in foreign language) Save Password - navigate to a secure page and enter credentials Save Credit Card - navigate to a booking site and enter a bogus CC. Allow popups and use http://www.popuptest.com/ to get a popup blocker view. For all those buttons a click should produce an ink ripple animation. TBR=sadrul@chromium.org ==========
Message was sent while issue was closed.
Committed patchset #15 (id:450001)
Message was sent while issue was closed.
Description was changed from ========== Adds MD ink ripple animations to buttons within location bar BUG=518941 TEST=Click the possible icons in location bar: Bookmark (Star) - always there Zoom (Ctrl-+ to see it) Translate (navigate to a page in foreign language) Save Password - navigate to a secure page and enter credentials Save Credit Card - navigate to a booking site and enter a bogus CC. Allow popups and use http://www.popuptest.com/ to get a popup blocker view. For all those buttons a click should produce an ink ripple animation. TBR=sadrul@chromium.org ========== to ========== Adds MD ink ripple animations to buttons within location bar BUG=518941 TEST=Click the possible icons in location bar: Bookmark (Star) - always there Zoom (Ctrl-+ to see it) Translate (navigate to a page in foreign language) Save Password - navigate to a secure page and enter credentials Save Credit Card - navigate to a booking site and enter a bogus CC. Allow popups and use http://www.popuptest.com/ to get a popup blocker view. For all those buttons a click should produce an ink ripple animation. TBR=sadrul@chromium.org Committed: https://crrev.com/9b3dd66a8dce80938cb51578f128416d2d539595 Cr-Commit-Position: refs/heads/master@{#372279} ==========
Message was sent while issue was closed.
Patchset 15 (id:??) landed as https://crrev.com/9b3dd66a8dce80938cb51578f128416d2d539595 Cr-Commit-Position: refs/heads/master@{#372279}
Message was sent while issue was closed.
On 2016/01/29 03:31:15, varkha wrote: > > if the whole ink ripple thing could be done [more simply] at the location bar > level. > I will explore that. We still plan to simplify it a bit by pulling more ink > ripple common functionality aside and making it state-driven as opposed to input > event driven as now. Our plan was to get the visible pieces in so that they can > be interacted with to gain time for feedback and refactor once the visuals are > stable. > In this case location bar could be or own an InkDropHost and delegate > positioning to the child views. A challenge here is that different variations of > buttons / views have slightly different event model and need tweaks to make the > ripple consistent with the button state. E.g. content views (popup blocker icon) > respond to any mouse buttons while zoom / bookmark and such only work on left > mouse buttons. Little things like that may need to be stabilized first. Sounds good. To elaborate on my thinking, I wonder if all these bubble showing functions on various buttons could just call into something like LocationBarView::BubbleOpened(gfx::Point location, views::Widget* bubble)... (or just pass the BubbleDelegate, which would know its anchor and its widget), and then the location bar could paint the ripple and observe the bubble widget. That said, it's probably more complicated than I imagine with this hand waving. |
