Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
394403003:
Close the password bubble on the icon click. (Closed)
|
Created:
6 years, 5 months ago by vasilii Modified:
6 years, 4 months ago CC:
chromium-reviews, tfarina, gcasto+watchlist_chromium.org, mkwst+watchlist_chromium.org Base URL:
svn://svn.chromium.org/chrome/trunk/src Project:
chromium Visibility:
Public. |
DescriptionClose the password bubble on the icon click.
After the password bubble became focusless we need additional code to close it by clicking the "key" icon.
BUG=394680
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=285641
Patch Set 1 #Patch Set 2 : Removed the unnecessary command #
Total comments: 9
Patch Set 3 : Added a test #
Total comments: 6
Patch Set 4 : Peter's comments #
Total comments: 6
Patch Set 5 : Don't touch bubble_icon_view.h #
Total comments: 6
Patch Set 6 : rename the getter #
Messages
Total messages: 20 (0 generated)
Hi guys, please review.
I don't like the indirection of adding a browser command for this and a whole framework to the BubbleIconView that adds a "hide command ID" no one else needs. Why can't we just have clicks on the key icon close the bubble directly like clicks on e.g. the star already do? Why do we have to route through the browser command handler system? Also, the change description says "locK" where I think you mean "key".
On 2014/07/21 19:57:49, Peter Kasting wrote: > Why can't we just have clicks on the key icon close the bubble directly like > clicks on e.g. the star already do? Clicks on the star close the bookmarking bubble because that bubble grabbed focus from the page in response to a user gesture. Since it's focused, clicking away from it onto some other focusable element closes the bubble. The star icon simply needs to not re-open the bubble in order to have the expected behavior. We're moving to a different model for bubbles that automatically pop up, as the focus-grabbing behavior has annoyed users. Since that changes the bubble's behavior, we need to be more explicit about closing the bubble. > Why do we have to route through the browser command handler system? I agree that we should be able to close the bubble without routing through the command system. The browser command is necessary for opening the bubble because we have to do so in a platform agnostic way that triggers for views, cocoa, etc. Since we're responding to a user's click from within the views code, we can call `ManagePasswordsBubbleView::CloseBubble()` directly, as we do now in `ManagePasswordsIconView::UpdateVisibleUI()`. I think `::OnExecuting` would be the right place for that.
I removed routing through the command system.
https://codereview.chromium.org/394403003/diff/20001/chrome/browser/ui/passwo... File chrome/browser/ui/passwords/manage_passwords_bubble_model.h (right): https://codereview.chromium.org/394403003/diff/20001/chrome/browser/ui/passwo... chrome/browser/ui/passwords/manage_passwords_bubble_model.h:91: using content::WebContentsObserver::web_contents; Nit: This seems unnecessary? https://codereview.chromium.org/394403003/diff/20001/chrome/browser/ui/views/... File chrome/browser/ui/views/location_bar/bubble_icon_view.cc (right): https://codereview.chromium.org/394403003/diff/20001/chrome/browser/ui/views/... chrome/browser/ui/views/location_bar/bubble_icon_view.cc:38: OnMousePressedIgnored(); Is the timing of this call important? If not, it seems reasonable to move it down to '::OnMouseReleased()' (and rename it accordingly). https://codereview.chromium.org/394403003/diff/20001/chrome/browser/ui/views/... File chrome/browser/ui/views/location_bar/bubble_icon_view.h (right): https://codereview.chromium.org/394403003/diff/20001/chrome/browser/ui/views/... chrome/browser/ui/views/location_bar/bubble_icon_view.h:31: virtual void OnMousePressedIgnored() {} Nit: How about 'OnMousePressSuppressed()' for consistency with the 'suppress_mouse_released_action_'. https://codereview.chromium.org/394403003/diff/20001/chrome/browser/ui/views/... File chrome/browser/ui/views/passwords/manage_passwords_bubble_view.cc (right): https://codereview.chromium.org/394403003/diff/20001/chrome/browser/ui/views/... chrome/browser/ui/views/passwords/manage_passwords_bubble_view.cc:130: DCHECK(ManagePasswordsBubbleView::Bubble()->web_contents() != web_contents); Can you add a comment about this case? It's not clear what this DCHECK is dchecking.
Also: can you add a test which ensures that the bubble is closed after interaction with the icon? You don't need to fake click events; just call the relevant methods on the IconView to fake them.
I added the test. https://codereview.chromium.org/394403003/diff/20001/chrome/browser/ui/passwo... File chrome/browser/ui/passwords/manage_passwords_bubble_model.h (right): https://codereview.chromium.org/394403003/diff/20001/chrome/browser/ui/passwo... chrome/browser/ui/passwords/manage_passwords_bubble_model.h:91: using content::WebContentsObserver::web_contents; On 2014/07/22 12:08:40, Mike West wrote: > Nit: This seems unnecessary? I'm bringing it to the public scope. I use it in ShowManagePasswordsBubble. https://codereview.chromium.org/394403003/diff/20001/chrome/browser/ui/views/... File chrome/browser/ui/views/location_bar/bubble_icon_view.cc (right): https://codereview.chromium.org/394403003/diff/20001/chrome/browser/ui/views/... chrome/browser/ui/views/location_bar/bubble_icon_view.cc:38: OnMousePressedIgnored(); On 2014/07/22 12:08:40, Mike West wrote: > Is the timing of this call important? If not, it seems reasonable to move it > down to '::OnMouseReleased()' (and rename it accordingly). All other bubbles lose the focus on mouse down event. Otherwise some interesting questions occur. If you click the icon, move the cursor outside of Chrome and release then do we want to close the bubble? https://codereview.chromium.org/394403003/diff/20001/chrome/browser/ui/views/... File chrome/browser/ui/views/location_bar/bubble_icon_view.h (right): https://codereview.chromium.org/394403003/diff/20001/chrome/browser/ui/views/... chrome/browser/ui/views/location_bar/bubble_icon_view.h:31: virtual void OnMousePressedIgnored() {} On 2014/07/22 12:08:40, Mike West wrote: > Nit: How about 'OnMousePressSuppressed()' for consistency with the > 'suppress_mouse_released_action_'. Done. https://codereview.chromium.org/394403003/diff/20001/chrome/browser/ui/views/... File chrome/browser/ui/views/passwords/manage_passwords_bubble_view.cc (right): https://codereview.chromium.org/394403003/diff/20001/chrome/browser/ui/views/... chrome/browser/ui/views/passwords/manage_passwords_bubble_view.cc:130: DCHECK(ManagePasswordsBubbleView::Bubble()->web_contents() != web_contents); On 2014/07/22 12:08:40, Mike West wrote: > Can you add a comment about this case? It's not clear what this DCHECK is > dchecking. Done.
chrome/browser/ui/passwords and chrome/browser/ui/views/passwords LGTM, thanks. https://codereview.chromium.org/394403003/diff/20001/chrome/browser/ui/views/... File chrome/browser/ui/views/location_bar/bubble_icon_view.cc (right): https://codereview.chromium.org/394403003/diff/20001/chrome/browser/ui/views/... chrome/browser/ui/views/location_bar/bubble_icon_view.cc:38: OnMousePressedIgnored(); On 2014/07/22 14:18:51, vasilii wrote: > On 2014/07/22 12:08:40, Mike West wrote: > > Is the timing of this call important? If not, it seems reasonable to move it > > down to '::OnMouseReleased()' (and rename it accordingly). > > All other bubbles lose the focus on mouse down event. Otherwise some interesting > questions occur. If you click the icon, move the cursor outside of Chrome and > release then do we want to close the bubble? Hrm. *shrug* Either way. If this is consistent with other bubbles, that's great.
https://codereview.chromium.org/394403003/diff/40001/chrome/browser/ui/views/... File chrome/browser/ui/views/passwords/manage_passwords_bubble_view.cc (right): https://codereview.chromium.org/394403003/diff/40001/chrome/browser/ui/views/... chrome/browser/ui/views/passwords/manage_passwords_bubble_view.cc:132: DCHECK(ManagePasswordsBubbleView::Bubble()->web_contents() != web_contents); If this DCHECK is the only reason to expose web_contents() accessors on two different classes, I think we should just kill the DCHECK and not expose those. https://codereview.chromium.org/394403003/diff/40001/chrome/browser/ui/views/... File chrome/browser/ui/views/passwords/manage_passwords_bubble_view.h (right): https://codereview.chromium.org/394403003/diff/40001/chrome/browser/ui/views/... chrome/browser/ui/views/passwords/manage_passwords_bubble_view.h:144: // Returns the web content for the bubble. Nit: content -> contents https://codereview.chromium.org/394403003/diff/40001/chrome/browser/ui/views/... chrome/browser/ui/views/passwords/manage_passwords_bubble_view.h:145: content::WebContents* web_contents() const; Const methods should not return non-const pointers. Either make the returned pointer const, or make the method non-const. https://codereview.chromium.org/394403003/diff/40001/chrome/browser/ui/views/... File chrome/browser/ui/views/passwords/manage_passwords_icon_view.cc (right): https://codereview.chromium.org/394403003/diff/40001/chrome/browser/ui/views/... chrome/browser/ui/views/passwords/manage_passwords_icon_view.cc:72: void ManagePasswordsIconView::OnMousePressSuppressed() { Instead of adding a new virtual method with the somewhat confusing name of OnMousePressSuppressed(), why not just override OnMousePressed() in this class, have it call the superclass method to set the |suppress_mouse_released_action_| variable correctly, then call CloseBubble()?
https://codereview.chromium.org/394403003/diff/40001/chrome/browser/ui/views/... File chrome/browser/ui/views/passwords/manage_passwords_bubble_view.cc (right): https://codereview.chromium.org/394403003/diff/40001/chrome/browser/ui/views/... chrome/browser/ui/views/passwords/manage_passwords_bubble_view.cc:132: DCHECK(ManagePasswordsBubbleView::Bubble()->web_contents() != web_contents); On 2014/07/22 18:55:18, Peter Kasting wrote: > If this DCHECK is the only reason to expose web_contents() accessors on two > different classes, I think we should just kill the DCHECK and not expose those. Done. https://codereview.chromium.org/394403003/diff/40001/chrome/browser/ui/views/... File chrome/browser/ui/views/passwords/manage_passwords_icon_view.cc (right): https://codereview.chromium.org/394403003/diff/40001/chrome/browser/ui/views/... chrome/browser/ui/views/passwords/manage_passwords_icon_view.cc:72: void ManagePasswordsIconView::OnMousePressSuppressed() { On 2014/07/22 18:55:18, Peter Kasting wrote: > Instead of adding a new virtual method with the somewhat confusing name of > OnMousePressSuppressed(), why not just override OnMousePressed() in this class, > have it call the superclass method to set the |suppress_mouse_released_action_| > variable correctly, then call CloseBubble()? Done.
https://codereview.chromium.org/394403003/diff/60001/chrome/browser/ui/views/... File chrome/browser/ui/views/location_bar/bubble_icon_view.h (right): https://codereview.chromium.org/394403003/diff/60001/chrome/browser/ui/views/... chrome/browser/ui/views/location_bar/bubble_icon_view.h:41: bool suppress_mouse_released_action() const { Rather than add this, we could do the following, which seems cleaner: * Move IsBubbleShowing() to ManagePasswordsIconView * Simplify the definition of BubbleIconView::OnMousePressed() to not check IsBubbleShowing(); instead, change ManagePasswordsIconView::OnMousePressed() to check that and handle that appropriately * Simplify the definition of BubbleIconView::GetTooltipText() to not check IsBubbleShowing(); instead, override GetTooltipText() in ManagePasswordsIconView() to check that and call the superclass method if it's false. Basically, this removes any knowledge of the manage passwords bubble from this class, and keeps it all in the subclass. Sorry for not thinking of this sooner. https://codereview.chromium.org/394403003/diff/60001/chrome/browser/ui/views/... File chrome/browser/ui/views/passwords/manage_passwords_bubble_view.cc (right): https://codereview.chromium.org/394403003/diff/60001/chrome/browser/ui/views/... chrome/browser/ui/views/passwords/manage_passwords_bubble_view.cc:129: if (ManagePasswordsBubbleView::IsShowing()) { Nit: No {} You might want a brief note about why we want to close the bubble here. https://codereview.chromium.org/394403003/diff/60001/chrome/browser/ui/views/... chrome/browser/ui/views/passwords/manage_passwords_bubble_view.cc:496: return ManagePasswordsBubbleView::manage_passwords_bubble_; Nit: This is a simple getter, so it should probably be inlined into the header and named "bubble()".
https://codereview.chromium.org/394403003/diff/60001/chrome/browser/ui/views/... File chrome/browser/ui/views/location_bar/bubble_icon_view.h (right): https://codereview.chromium.org/394403003/diff/60001/chrome/browser/ui/views/... chrome/browser/ui/views/location_bar/bubble_icon_view.h:41: bool suppress_mouse_released_action() const { On 2014/07/23 17:58:23, Peter Kasting wrote: > Rather than add this, we could do the following, which seems cleaner: > > * Move IsBubbleShowing() to ManagePasswordsIconView > * Simplify the definition of BubbleIconView::OnMousePressed() to not check > IsBubbleShowing(); instead, change ManagePasswordsIconView::OnMousePressed() to > check that and handle that appropriately > * Simplify the definition of BubbleIconView::GetTooltipText() to not check > IsBubbleShowing(); instead, override GetTooltipText() in > ManagePasswordsIconView() to check that and call the superclass method if it's > false. > > Basically, this removes any knowledge of the manage passwords bubble from this > class, and keeps it all in the subclass. > > Sorry for not thinking of this sooner. I didn't get your proposal. BubbleIconView is used by other bubbles too and for them it's important that we don't show the tooltip and ignore the click if the bubble is open. I don't want to copy/paste the same code into all the subclasses. Anyway I got rid of this method. https://codereview.chromium.org/394403003/diff/60001/chrome/browser/ui/views/... File chrome/browser/ui/views/passwords/manage_passwords_bubble_view.cc (right): https://codereview.chromium.org/394403003/diff/60001/chrome/browser/ui/views/... chrome/browser/ui/views/passwords/manage_passwords_bubble_view.cc:129: if (ManagePasswordsBubbleView::IsShowing()) { On 2014/07/23 17:58:23, Peter Kasting wrote: > Nit: No {} > > You might want a brief note about why we want to close the bubble here. Done. https://codereview.chromium.org/394403003/diff/60001/chrome/browser/ui/views/... chrome/browser/ui/views/passwords/manage_passwords_bubble_view.cc:496: return ManagePasswordsBubbleView::manage_passwords_bubble_; On 2014/07/23 17:58:23, Peter Kasting wrote: > Nit: This is a simple getter, so it should probably be inlined into the header > and named "bubble()". Done.
Don't worry, even if you didn't understand what I was suggesting, you did exactly what I was trying to say :) LGTM https://codereview.chromium.org/394403003/diff/80001/chrome/browser/ui/views/... File chrome/browser/ui/views/passwords/manage_passwords_bubble_view.h (right): https://codereview.chromium.org/394403003/diff/80001/chrome/browser/ui/views/... chrome/browser/ui/views/passwords/manage_passwords_bubble_view.h:142: static const ManagePasswordsBubbleView* Bubble() { This doesn't appear to have actually been moved into the header and renamed. (Also, I suggested the wrong name last time -- getters should be named like the member, so this should be "manage_password_bubble()".) https://codereview.chromium.org/394403003/diff/80001/chrome/browser/ui/views/... File chrome/browser/ui/views/passwords/manage_passwords_icon_view_browsertest.cc (right): https://codereview.chromium.org/394403003/diff/80001/chrome/browser/ui/views/... chrome/browser/ui/views/passwords/manage_passwords_icon_view_browsertest.cc:71: static_cast<views::View*>(view())->OnMousePressed(mouse_down); This cast surprises me; what's the type of view() right now? It can't be "const views::View*" since static_cast can't cast away const. So why is this actually needed?
https://codereview.chromium.org/394403003/diff/80001/chrome/browser/ui/views/... File chrome/browser/ui/views/passwords/manage_passwords_bubble_view.h (right): https://codereview.chromium.org/394403003/diff/80001/chrome/browser/ui/views/... chrome/browser/ui/views/passwords/manage_passwords_bubble_view.h:142: static const ManagePasswordsBubbleView* Bubble() { On 2014/07/24 18:18:08, Peter Kasting wrote: > This doesn't appear to have actually been moved into the header and renamed. > (Also, I suggested the wrong name last time -- getters should be named like the > member, so this should be "manage_password_bubble()".) Done. https://codereview.chromium.org/394403003/diff/80001/chrome/browser/ui/views/... File chrome/browser/ui/views/passwords/manage_passwords_icon_view_browsertest.cc (right): https://codereview.chromium.org/394403003/diff/80001/chrome/browser/ui/views/... chrome/browser/ui/views/passwords/manage_passwords_icon_view_browsertest.cc:71: static_cast<views::View*>(view())->OnMousePressed(mouse_down); On 2014/07/24 18:18:08, Peter Kasting wrote: > This cast surprises me; what's the type of view() right now? It can't be "const > views::View*" since static_cast can't cast away const. So why is this actually > needed? The type is ManagePasswordsIconView*. BubbleIconView::OnMousePressed is protected. So I cast it back to View* to be able to call it.
The CQ bit was checked by vasilii@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/vasilii@chromium.org/394403003/100001
FYI, CQ is re-trying this CL (attempt #1). The failing builders are: win_chromium_rel on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_rel/buil...)
Message was sent while issue was closed.
Change committed as 285641
Message was sent while issue was closed.
Sigh, somehow didn't send this. Could you deal with this as a followup? https://codereview.chromium.org/394403003/diff/80001/chrome/browser/ui/views/... File chrome/browser/ui/views/passwords/manage_passwords_icon_view_browsertest.cc (right): https://codereview.chromium.org/394403003/diff/80001/chrome/browser/ui/views/... chrome/browser/ui/views/passwords/manage_passwords_icon_view_browsertest.cc:71: static_cast<views::View*>(view())->OnMousePressed(mouse_down); On 2014/07/25 08:24:34, vasilii wrote: > On 2014/07/24 18:18:08, Peter Kasting wrote: > > This cast surprises me; what's the type of view() right now? It can't be > "const > > views::View*" since static_cast can't cast away const. So why is this > actually > > needed? > > The type is ManagePasswordsIconView*. BubbleIconView::OnMousePressed is > protected. So I cast it back to View* to be able to call it. Make OnMousePressed() public and remove this cast, then. It's only protected right now because no one else has needed to call it publicly; this is a legit usage, so we should unprotect it.
Message was sent while issue was closed.
https://codereview.chromium.org/394403003/diff/80001/chrome/browser/ui/views/... File chrome/browser/ui/views/passwords/manage_passwords_icon_view_browsertest.cc (right): https://codereview.chromium.org/394403003/diff/80001/chrome/browser/ui/views/... chrome/browser/ui/views/passwords/manage_passwords_icon_view_browsertest.cc:71: static_cast<views::View*>(view())->OnMousePressed(mouse_down); On 2014/07/25 20:34:28, Peter Kasting wrote: > On 2014/07/25 08:24:34, vasilii wrote: > > On 2014/07/24 18:18:08, Peter Kasting wrote: > > > This cast surprises me; what's the type of view() right now? It can't be > > "const > > > views::View*" since static_cast can't cast away const. So why is this > > actually > > > needed? > > > > The type is ManagePasswordsIconView*. BubbleIconView::OnMousePressed is > > protected. So I cast it back to View* to be able to call it. > > Make OnMousePressed() public and remove this cast, then. > > It's only protected right now because no one else has needed to call it > publicly; this is a legit usage, so we should unprotect it. Done. |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
