Close the password bubble when navigating between tabs.

Close the password bubble when navigating between tabs.

BUG=412247
TBR=ben@chromium.org

Committed: https://crrev.com/7c53f8636e7d2d343f2c706d7bee9abc60c36d03
Cr-Commit-Position: refs/heads/master@{#294803}

[vasilii, 2014-09-11 14:15:29]

vasilii@chromium.org changed reviewers:
+ mkwst@chromium.org

[vasilii, 2014-09-11 14:15:30]

Hi Mike,

please review.

[Mike West, 2014-09-12 11:03:50]

Thanks Vasilii! +dconnelly

Comments inline.

https://codereview.chromium.org/565673002/diff/20001/chrome/browser/ui/views/...
File chrome/browser/ui/views/passwords/manage_passwords_bubble_view.h (right):

https://codereview.chromium.org/565673002/diff/20001/chrome/browser/ui/views/...
chrome/browser/ui/views/passwords/manage_passwords_bubble_view.h:36: static void
CloseBubbleForInactiveTab();
Nit: How about 'CloseBubbleIfTabIsInactive()'.

https://codereview.chromium.org/565673002/diff/20001/chrome/browser/ui/views/...
File chrome/browser/ui/views/passwords/manage_passwords_icon_view.cc (left):

https://codereview.chromium.org/565673002/diff/20001/chrome/browser/ui/views/...
chrome/browser/ui/views/passwords/manage_passwords_icon_view.cc:20:
UpdateVisibleUI();
Did this call cause problems?

https://codereview.chromium.org/565673002/diff/20001/chrome/browser/ui/views/...
File chrome/browser/ui/views/passwords/manage_passwords_icon_view.cc (right):

https://codereview.chromium.org/565673002/diff/20001/chrome/browser/ui/views/...
chrome/browser/ui/views/passwords/manage_passwords_icon_view.cc:32:
ManagePasswordsBubbleView::CloseBubbleForInactiveTab();
This seems a bit roundabout, and platform-specific. ManagePasswordsUIController
is a WebContentsObserver. Could you instead override
WebContentsObserver::WasHidden in order to drive this behavior? This has the
advantage of forcing you to do something in the platform-agnostic bits that
Daniel could hook into for Mac.

[dconnelly, 2014-09-12 11:15:25]

dconnelly@chromium.org changed reviewers:
+ dconnelly@chromium.org

[dconnelly, 2014-09-12 11:15:28]

https://codereview.chromium.org/565673002/diff/20001/chrome/browser/ui/views/...
File chrome/browser/ui/views/passwords/manage_passwords_icon_view.cc (right):

https://codereview.chromium.org/565673002/diff/20001/chrome/browser/ui/views/...
chrome/browser/ui/views/passwords/manage_passwords_icon_view.cc:32:
ManagePasswordsBubbleView::CloseBubbleForInactiveTab();
On 2014/09/12 11:03:50, Mike West wrote:
> This seems a bit roundabout, and platform-specific.
ManagePasswordsUIController
> is a WebContentsObserver. Could you instead override
> WebContentsObserver::WasHidden in order to drive this behavior? This has the
> advantage of forcing you to do something in the platform-agnostic bits that
> Daniel could hook into for Mac.

This bug doesn't appear on Mac
(https://code.google.com/p/chromium/issues/detail?id=412247#c2), so I'm not that
worried. Of course, if it simplifies other logic or prevents Mac bugs in the
future I'm all for it :)

[vasilii, 2014-09-12 12:29:58]

https://codereview.chromium.org/565673002/diff/20001/chrome/browser/ui/views/...
File chrome/browser/ui/views/passwords/manage_passwords_bubble_view.h (right):

https://codereview.chromium.org/565673002/diff/20001/chrome/browser/ui/views/...
chrome/browser/ui/views/passwords/manage_passwords_bubble_view.h:36: static void
CloseBubbleForInactiveTab();
On 2014/09/12 11:03:50, Mike West wrote:
> Nit: How about 'CloseBubbleIfTabIsInactive()'.

Done.

https://codereview.chromium.org/565673002/diff/20001/chrome/browser/ui/views/...
File chrome/browser/ui/views/passwords/manage_passwords_icon_view.cc (left):

https://codereview.chromium.org/565673002/diff/20001/chrome/browser/ui/views/...
chrome/browser/ui/views/passwords/manage_passwords_icon_view.cc:20:
UpdateVisibleUI();
On 2014/09/12 11:03:50, Mike West wrote:
> Did this call cause problems?

No, but it doesn't belong here. Setting visibility and calling layout are not
right things for the constructor.

https://codereview.chromium.org/565673002/diff/20001/chrome/browser/ui/views/...
File chrome/browser/ui/views/passwords/manage_passwords_icon_view.cc (right):

https://codereview.chromium.org/565673002/diff/20001/chrome/browser/ui/views/...
chrome/browser/ui/views/passwords/manage_passwords_icon_view.cc:32:
ManagePasswordsBubbleView::CloseBubbleForInactiveTab();
On 2014/09/12 11:15:26, dconnelly wrote:
> On 2014/09/12 11:03:50, Mike West wrote:
> > This seems a bit roundabout, and platform-specific.
> ManagePasswordsUIController
> > is a WebContentsObserver. Could you instead override
> > WebContentsObserver::WasHidden in order to drive this behavior? This has the
> > advantage of forcing you to do something in the platform-agnostic bits that
> > Daniel could hook into for Mac.
> 
> This bug doesn't appear on Mac
> (https://code.google.com/p/chromium/issues/detail?id=412247#c2), so I'm not
that
> worried. Of course, if it simplifies other logic or prevents Mac bugs in the
> future I'm all for it :)

Mike, to make it cross platform we need cross platform methods CloseBubble() and
IsShowing(). We don't have them now.

[dconnelly, 2014-09-12 12:31:41]

https://codereview.chromium.org/565673002/diff/20001/chrome/browser/ui/views/...
File chrome/browser/ui/views/passwords/manage_passwords_icon_view.cc (right):

https://codereview.chromium.org/565673002/diff/20001/chrome/browser/ui/views/...
chrome/browser/ui/views/passwords/manage_passwords_icon_view.cc:32:
ManagePasswordsBubbleView::CloseBubbleForInactiveTab();
On 2014/09/12 12:29:58, vasilii wrote:
> On 2014/09/12 11:15:26, dconnelly wrote:
> > On 2014/09/12 11:03:50, Mike West wrote:
> > > This seems a bit roundabout, and platform-specific.
> > ManagePasswordsUIController
> > > is a WebContentsObserver. Could you instead override
> > > WebContentsObserver::WasHidden in order to drive this behavior? This has
the
> > > advantage of forcing you to do something in the platform-agnostic bits
that
> > > Daniel could hook into for Mac.
> > 
> > This bug doesn't appear on Mac
> > (https://code.google.com/p/chromium/issues/detail?id=412247#c2), so I'm not
> that
> > worried. Of course, if it simplifies other logic or prevents Mac bugs in the
> > future I'm all for it :)
> 
> Mike, to make it cross platform we need cross platform methods CloseBubble()
and
> IsShowing(). We don't have them now.

This would be pretty straightforward to add to the cross-platform bubble
interface, though.

[Mike West, 2014-09-12 13:10:18]

On 2014/09/12 12:31:41, dconnelly wrote:
>
https://codereview.chromium.org/565673002/diff/20001/chrome/browser/ui/views/...
> File chrome/browser/ui/views/passwords/manage_passwords_icon_view.cc (right):
> 
>
https://codereview.chromium.org/565673002/diff/20001/chrome/browser/ui/views/...
> chrome/browser/ui/views/passwords/manage_passwords_icon_view.cc:32:
> ManagePasswordsBubbleView::CloseBubbleForInactiveTab();
> On 2014/09/12 12:29:58, vasilii wrote:
> > On 2014/09/12 11:15:26, dconnelly wrote:
> > > On 2014/09/12 11:03:50, Mike West wrote:
> > > > This seems a bit roundabout, and platform-specific.
> > > ManagePasswordsUIController
> > > > is a WebContentsObserver. Could you instead override
> > > > WebContentsObserver::WasHidden in order to drive this behavior? This has
> the
> > > > advantage of forcing you to do something in the platform-agnostic bits
> that
> > > > Daniel could hook into for Mac.
> > > 
> > > This bug doesn't appear on Mac
> > > (https://code.google.com/p/chromium/issues/detail?id=412247#c2), so I'm
not
> > that
> > > worried. Of course, if it simplifies other logic or prevents Mac bugs in
the
> > > future I'm all for it :)
> > 
> > Mike, to make it cross platform we need cross platform methods CloseBubble()
> and
> > IsShowing(). We don't have them now.

We have a browser command, don't we?

> This would be pretty straightforward to add to the cross-platform bubble
> interface, though.

Indeed.

You could, for example, just pass the WasHidden event into the platform-specific
code to handle in some platform specific way. I think that would be cleaner than
checking every time you update the UI.

-mike

[vasilii, 2014-09-12 16:21:11]

Done.

[Mike West, 2014-09-12 18:46:03]

LGTM, thanks for doing another pass.

[vasilii, 2014-09-15 09:44:06]

Mike, the test was failing. Now I'm using another approach that is neater.
Please review again.

[Mike West, 2014-09-15 09:47:15]

On 2014/09/15 09:44:06, vasilii wrote:
> Mike, the test was failing. Now I'm using another approach that is neater.
> Please review again.

SLGTM.

[vasilii, 2014-09-15 09:58:46]

The CQ bit was checked by vasilii@chromium.org

[commit-bot: I haz the power, 2014-09-15 09:59:47]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patchset/565673002/80001

[commit-bot: I haz the power, 2014-09-15 10:30:42]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-09-15 10:30:43]

Try jobs failed on following builders:
  android_dbg_tests_recipe on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/android_dbg_tes...)

[vasilii, 2014-09-15 10:44:36]

The CQ bit was checked by vasilii@chromium.org

[commit-bot: I haz the power, 2014-09-15 10:45:12]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patchset/565673002/100001

[commit-bot: I haz the power, 2014-09-15 11:45:56]

Committed patchset #6 (id:100001) as 9d6933fd9a28f21a713f49751180f4c690c98849

[commit-bot: I haz the power, 2014-09-15 11:49:48]

Patchset 6 (id:??) landed as
https://crrev.com/7c53f8636e7d2d343f2c706d7bee9abc60c36d03
Cr-Commit-Position: refs/heads/master@{#294803}