Hide the password bubble when the underlying ManagePasswordsUIController changes its state.

Hide the password bubble when the underlying ManagePasswordsUIController changes its state.

BUG=468474
Committed: https://crrev.com/4a7c97ce6afccbc4b45997dad2a1b1c5d65c4c7f
Cr-Commit-Position: refs/heads/master@{#323882}

[vasilii, 2015-03-25 11:32:55]

vasilii@chromium.org changed reviewers:
+ groby@chromium.org, vabr@chromium.org

[vasilii, 2015-03-25 11:32:55]

Hi guys,

groby@chromium.org: Please review changes in
chrome/browser/ui/cocoa

vabr@chromium.org: Please review changes in
everything else

[vabr (Chromium), 2015-03-25 11:43:25]

LGTM with nits.
Thanks!
Vaclav

https://codereview.chromium.org/1028243004/diff/20001/chrome/browser/ui/cocoa...
File chrome/browser/ui/cocoa/location_bar/manage_passwords_decoration.h (right):

https://codereview.chromium.org/1028243004/diff/20001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/location_bar/manage_passwords_decoration.h:52: // Closes
the it's currently displayed.
grammar: This comment appears to have broken grammar.

https://codereview.chromium.org/1028243004/diff/20001/chrome/browser/ui/passw...
File chrome/browser/ui/passwords/manage_passwords_icon.h (right):

https://codereview.chromium.org/1028243004/diff/20001/chrome/browser/ui/passw...
chrome/browser/ui/passwords/manage_passwords_icon.h:35: // Called from
SetState() iff the icon's state has changed in order to close
nit: Is the "in order to close the current bubble" part necessary? Could leaving
it out actually make the comment on the purpose of this hook clearer?

[vasilii, 2015-03-25 11:48:55]

https://codereview.chromium.org/1028243004/diff/20001/chrome/browser/ui/cocoa...
File chrome/browser/ui/cocoa/location_bar/manage_passwords_decoration.h (right):

https://codereview.chromium.org/1028243004/diff/20001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/location_bar/manage_passwords_decoration.h:52: // Closes
the it's currently displayed.
On 2015/03/25 11:43:24, vabr (Chromium) wrote:
> grammar: This comment appears to have broken grammar.

Done.

https://codereview.chromium.org/1028243004/diff/20001/chrome/browser/ui/passw...
File chrome/browser/ui/passwords/manage_passwords_icon.h (right):

https://codereview.chromium.org/1028243004/diff/20001/chrome/browser/ui/passw...
chrome/browser/ui/passwords/manage_passwords_icon.h:35: // Called from
SetState() iff the icon's state has changed in order to close
On 2015/03/25 11:43:24, vabr (Chromium) wrote:
> nit: Is the "in order to close the current bubble" part necessary? Could
leaving
> it out actually make the comment on the purpose of this hook clearer?

Done.

[vabr (Chromium), 2015-03-25 13:13:18]

lgtm

[groby-ooo-7-16, 2015-03-25 18:18:48]

c/b/ui/cocoa looks good, but can we add a browsertest that covers Cocoa as well?

[vasilii, 2015-04-01 15:16:01]

On 2015/03/25 18:18:48, groby wrote:
> c/b/ui/cocoa looks good, but can we add a browsertest that covers Cocoa as
well?

I added a unit test!

[vasilii, 2015-04-02 08:26:13]

Rachel?

[groby-ooo-7-16, 2015-04-02 21:19:07]

I suppose a shared browsertest is out of the question? (Reluctant LGTM)

[vasilii, 2015-04-04 14:24:56]

On 2015/04/02 21:19:07, groby wrote:
> I suppose a shared browsertest is out of the question? (Reluctant LGTM)

Not that easy. There is no cross platform function IsBubbleShowing() for
example. For this particular test it would make sense. In general the Mac bubble
is always with focus while the Views bubble is without. So they are quite
different.

[vasilii, 2015-04-04 14:25:01]

The CQ bit was checked by vasilii@chromium.org

[vasilii, 2015-04-04 14:25:02]

The patchset sent to the CQ was uploaded after l-g-t-m from vabr@chromium.org
Link to the patchset: https://codereview.chromium.org/1028243004/#ps60001
(title: "add a Mac test")

[commit-bot: I haz the power, 2015-04-04 14:25:11]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1028243004/60001

[commit-bot: I haz the power, 2015-04-04 15:27:54]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2015-04-04 15:28:36]

Patchset 4 (id:??) landed as
https://crrev.com/4a7c97ce6afccbc4b45997dad2a1b1c5d65c4c7f
Cr-Commit-Position: refs/heads/master@{#323882}