Password bubble should close if user clicks on the web page.

Password bubble should close if user clicks on the web page.

BUG=394287
Committed: https://crrev.com/5db647c6d8a9200bc5ad980fa6cfc9bc688f7deb
Cr-Commit-Position: refs/heads/master@{#292381}

[vasilii, 2014-08-27 16:27:06]

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

[vasilii, 2014-08-27 16:27:07]

Vaclav, please review that one.

[vabr (Chromium), 2014-08-27 16:43:05]

What I saw so far looks good, but what about a test?

Also -- it's awesome, that the bubble will became closable by clicking in a page
again.

BTW., what happens, if the user clicks on the tab strip instead (to change to
another tab)?

Cheers,
Vaclav

https://codereview.chromium.org/512683003/diff/1/chrome/browser/ui/passwords/...
File chrome/browser/ui/passwords/manage_passwords_bubble_model.h (right):

https://codereview.chromium.org/512683003/diff/1/chrome/browser/ui/passwords/...
chrome/browser/ui/passwords/manage_passwords_bubble_model.h:90: using
WebContentsObserver::web_contents;
Instead of making the whole accessor public, could you instead expose just a
public method to access the aura::Window*?
Sounds like that would be enough, and keeping access restricted in general is a
recommendable practice.

https://codereview.chromium.org/512683003/diff/1/chrome/browser/ui/views/pass...
File chrome/browser/ui/views/passwords/manage_passwords_bubble_view.cc (right):

https://codereview.chromium.org/512683003/diff/1/chrome/browser/ui/views/pass...
chrome/browser/ui/views/passwords/manage_passwords_bubble_view.cc:519: // The
class listens WebContentsView for mouse clicks.
nit: listens -> listens to

https://codereview.chromium.org/512683003/diff/1/chrome/browser/ui/views/pass...
chrome/browser/ui/views/passwords/manage_passwords_bubble_view.cc:519: // The
class listens WebContentsView for mouse clicks.
nit: Could you extend the comment to say what the class does about the clicks?
For example:
The class listens to WebContentsView and notifies the bubble if the view was
clicked on.

[vasilii, 2014-08-27 17:24:24]

If the user changes the tab the bubble is closed but through the different
mechanism (LocationBarView::RefreshManagePasswordsIconView()). If the user
clicks on the Chrome frame then nothing happens.

https://codereview.chromium.org/512683003/diff/1/chrome/browser/ui/passwords/...
File chrome/browser/ui/passwords/manage_passwords_bubble_model.h (right):

https://codereview.chromium.org/512683003/diff/1/chrome/browser/ui/passwords/...
chrome/browser/ui/passwords/manage_passwords_bubble_model.h:90: using
WebContentsObserver::web_contents;
On 2014/08/27 16:43:05, vabr (Chromium) wrote:
> Instead of making the whole accessor public, could you instead expose just a
> public method to access the aura::Window*?
> Sounds like that would be enough, and keeping access restricted in general is
a
> recommendable practice.

aura::Window* is a very strange type to return from this class. This is very
bottom layer ui primitive. Additionally the bubble model has nothing to do with
any of the windows in the WebContents hierarchy.
This is the second time I get resistance for adding this accessor. No idea why.
ManagePasswordsBubbleView is created with content::WebContents*. During its
lifetime it's perfectly valid to request from inside the bubble which
WebContents it was created for.

https://codereview.chromium.org/512683003/diff/1/chrome/browser/ui/views/pass...
File chrome/browser/ui/views/passwords/manage_passwords_bubble_view.cc (right):

https://codereview.chromium.org/512683003/diff/1/chrome/browser/ui/views/pass...
chrome/browser/ui/views/passwords/manage_passwords_bubble_view.cc:519: // The
class listens WebContentsView for mouse clicks.
On 2014/08/27 16:43:05, vabr (Chromium) wrote:
> nit: listens -> listens to

Done.

https://codereview.chromium.org/512683003/diff/1/chrome/browser/ui/views/pass...
chrome/browser/ui/views/passwords/manage_passwords_bubble_view.cc:519: // The
class listens WebContentsView for mouse clicks.
On 2014/08/27 16:43:05, vabr (Chromium) wrote:
> nit: Could you extend the comment to say what the class does about the clicks?
> For example:
> The class listens to WebContentsView and notifies the bubble if the view was
> clicked on.

Done.

[vabr (Chromium), 2014-08-28 11:42:37]

LGTM with a comment, thanks for the test.

I'm of course happy to also talk about the accessor in person if you want to.

Cheers,
Vaclav

https://codereview.chromium.org/512683003/diff/1/chrome/browser/ui/passwords/...
File chrome/browser/ui/passwords/manage_passwords_bubble_model.h (right):

https://codereview.chromium.org/512683003/diff/1/chrome/browser/ui/passwords/...
chrome/browser/ui/passwords/manage_passwords_bubble_model.h:90: using
WebContentsObserver::web_contents;
On 2014/08/27 17:24:24, vasilii wrote:
> On 2014/08/27 16:43:05, vabr (Chromium) wrote:
> > Instead of making the whole accessor public, could you instead expose just a
> > public method to access the aura::Window*?
> > Sounds like that would be enough, and keeping access restricted in general
is
> a
> > recommendable practice.
> 
> aura::Window* is a very strange type to return from this class. This is very
> bottom layer ui primitive. Additionally the bubble model has nothing to do
with
> any of the windows in the WebContents hierarchy.
> This is the second time I get resistance for adding this accessor. No idea
why.
> ManagePasswordsBubbleView is created with content::WebContents*. During its
> lifetime it's perfectly valid to request from inside the bubble which
> WebContents it was created for.

Why I don't like exposing the WebContents (WC :)) accessor in the model -- not
only, as you said, has the model nothing to do with the WC window hierarchy,
it's API also has nothing to do with the WC. It observes the WC, but that's an
implementation detail. The model gets the WC from the bubble, and that's where
you need to get it from in the bubbleview code. In other words, the model is to
provide password-related information to the bubble, and execute associated
actions, not to store the WC.

My suggestion here is: store the WC given to ManagePasswordsBubble on creation
in a protected member variable. Use that in ManagePasswordsBubbleView when you
need to create the WebContentMouseHandler to retrieve the aura::Window. If you
must, retrieve the window even first in the WebContentMouseHandler. You can also
obviously do the other thing and remember the aura::Window intead of the WC in
ManagePasswordsBubble. I don't care here too much, as long as retrieving the WC
does not become the public API of the model.

[vasilii, 2014-08-28 11:53:55]

https://codereview.chromium.org/512683003/diff/1/chrome/browser/ui/passwords/...
File chrome/browser/ui/passwords/manage_passwords_bubble_model.h (right):

https://codereview.chromium.org/512683003/diff/1/chrome/browser/ui/passwords/...
chrome/browser/ui/passwords/manage_passwords_bubble_model.h:90: using
WebContentsObserver::web_contents;
On 2014/08/28 11:42:37, vabr (Chromium) wrote:
> On 2014/08/27 17:24:24, vasilii wrote:
> > On 2014/08/27 16:43:05, vabr (Chromium) wrote:
> > > Instead of making the whole accessor public, could you instead expose just
a
> > > public method to access the aura::Window*?
> > > Sounds like that would be enough, and keeping access restricted in general
> is
> > a
> > > recommendable practice.
> > 
> > aura::Window* is a very strange type to return from this class. This is very
> > bottom layer ui primitive. Additionally the bubble model has nothing to do
> with
> > any of the windows in the WebContents hierarchy.
> > This is the second time I get resistance for adding this accessor. No idea
> why.
> > ManagePasswordsBubbleView is created with content::WebContents*. During its
> > lifetime it's perfectly valid to request from inside the bubble which
> > WebContents it was created for.
> 
> Why I don't like exposing the WebContents (WC :)) accessor in the model -- not
> only, as you said, has the model nothing to do with the WC window hierarchy,
> it's API also has nothing to do with the WC. It observes the WC, but that's an
> implementation detail. The model gets the WC from the bubble, and that's where
> you need to get it from in the bubbleview code. In other words, the model is
to
> provide password-related information to the bubble, and execute associated
> actions, not to store the WC.
> 
> My suggestion here is: store the WC given to ManagePasswordsBubble on creation
> in a protected member variable. Use that in ManagePasswordsBubbleView when you
> need to create the WebContentMouseHandler to retrieve the aura::Window. If you
> must, retrieve the window even first in the WebContentMouseHandler. You can
also
> obviously do the other thing and remember the aura::Window intead of the WC in
> ManagePasswordsBubble. I don't care here too much, as long as retrieving the
WC
> does not become the public API of the model.

None of the solutions will work. ManagePasswordsBubbleModel listens to
WebContents and zeros the pointer in WebContentsObserver::ResetWebContents().
Making the copy of WebContents* or aura::Window* will result in dangling pointer
when the user closes the tab. This is the reason why ~WebContentMouseHandler()
checks for NULL pointer before unsubscribing.

[vabr (Chromium), 2014-08-28 12:11:46]

On 2014/08/28 11:53:55, vasilii wrote:
>
https://codereview.chromium.org/512683003/diff/1/chrome/browser/ui/passwords/...
> File chrome/browser/ui/passwords/manage_passwords_bubble_model.h (right):
> 
>
https://codereview.chromium.org/512683003/diff/1/chrome/browser/ui/passwords/...
> chrome/browser/ui/passwords/manage_passwords_bubble_model.h:90: using
> WebContentsObserver::web_contents;
> On 2014/08/28 11:42:37, vabr (Chromium) wrote:
> > On 2014/08/27 17:24:24, vasilii wrote:
> > > On 2014/08/27 16:43:05, vabr (Chromium) wrote:
> > > > Instead of making the whole accessor public, could you instead expose
just
> a
> > > > public method to access the aura::Window*?
> > > > Sounds like that would be enough, and keeping access restricted in
general
> > is
> > > a
> > > > recommendable practice.
> > > 
> > > aura::Window* is a very strange type to return from this class. This is
very
> > > bottom layer ui primitive. Additionally the bubble model has nothing to do
> > with
> > > any of the windows in the WebContents hierarchy.
> > > This is the second time I get resistance for adding this accessor. No idea
> > why.
> > > ManagePasswordsBubbleView is created with content::WebContents*. During
its
> > > lifetime it's perfectly valid to request from inside the bubble which
> > > WebContents it was created for.
> > 
> > Why I don't like exposing the WebContents (WC :)) accessor in the model --
not
> > only, as you said, has the model nothing to do with the WC window hierarchy,
> > it's API also has nothing to do with the WC. It observes the WC, but that's
an
> > implementation detail. The model gets the WC from the bubble, and that's
where
> > you need to get it from in the bubbleview code. In other words, the model is
> to
> > provide password-related information to the bubble, and execute associated
> > actions, not to store the WC.
> > 
> > My suggestion here is: store the WC given to ManagePasswordsBubble on
creation
> > in a protected member variable. Use that in ManagePasswordsBubbleView when
you
> > need to create the WebContentMouseHandler to retrieve the aura::Window. If
you
> > must, retrieve the window even first in the WebContentMouseHandler. You can
> also
> > obviously do the other thing and remember the aura::Window intead of the WC
in
> > ManagePasswordsBubble. I don't care here too much, as long as retrieving the
> WC
> > does not become the public API of the model.
> 
> None of the solutions will work. ManagePasswordsBubbleModel listens to
> WebContents and zeros the pointer in WebContentsObserver::ResetWebContents().
> Making the copy of WebContents* or aura::Window* will result in dangling
pointer
> when the user closes the tab. This is the reason why ~WebContentMouseHandler()
> checks for NULL pointer before unsubscribing.

I'm surprised to hear that the bubble lives on after the WC for which it was
created is destroyed. Would it make sense to override WebContentsDestroyed() in
the model, and signal that to the bubble, so that it can close itself?

[vasilii, 2014-08-28 13:36:02]

On 2014/08/28 12:11:46, vabr (Chromium) wrote:
> On 2014/08/28 11:53:55, vasilii wrote:
> >
>
https://codereview.chromium.org/512683003/diff/1/chrome/browser/ui/passwords/...
> > File chrome/browser/ui/passwords/manage_passwords_bubble_model.h (right):
> > 
> >
>
https://codereview.chromium.org/512683003/diff/1/chrome/browser/ui/passwords/...
> > chrome/browser/ui/passwords/manage_passwords_bubble_model.h:90: using
> > WebContentsObserver::web_contents;
> > On 2014/08/28 11:42:37, vabr (Chromium) wrote:
> > > On 2014/08/27 17:24:24, vasilii wrote:
> > > > On 2014/08/27 16:43:05, vabr (Chromium) wrote:
> > > > > Instead of making the whole accessor public, could you instead expose
> just
> > a
> > > > > public method to access the aura::Window*?
> > > > > Sounds like that would be enough, and keeping access restricted in
> general
> > > is
> > > > a
> > > > > recommendable practice.
> > > > 
> > > > aura::Window* is a very strange type to return from this class. This is
> very
> > > > bottom layer ui primitive. Additionally the bubble model has nothing to
do
> > > with
> > > > any of the windows in the WebContents hierarchy.
> > > > This is the second time I get resistance for adding this accessor. No
idea
> > > why.
> > > > ManagePasswordsBubbleView is created with content::WebContents*. During
> its
> > > > lifetime it's perfectly valid to request from inside the bubble which
> > > > WebContents it was created for.
> > > 
> > > Why I don't like exposing the WebContents (WC :)) accessor in the model --
> not
> > > only, as you said, has the model nothing to do with the WC window
hierarchy,
> > > it's API also has nothing to do with the WC. It observes the WC, but
that's
> an
> > > implementation detail. The model gets the WC from the bubble, and that's
> where
> > > you need to get it from in the bubbleview code. In other words, the model
is
> > to
> > > provide password-related information to the bubble, and execute associated
> > > actions, not to store the WC.
> > > 
> > > My suggestion here is: store the WC given to ManagePasswordsBubble on
> creation
> > > in a protected member variable. Use that in ManagePasswordsBubbleView when
> you
> > > need to create the WebContentMouseHandler to retrieve the aura::Window. If
> you
> > > must, retrieve the window even first in the WebContentMouseHandler. You
can
> > also
> > > obviously do the other thing and remember the aura::Window intead of the
WC
> in
> > > ManagePasswordsBubble. I don't care here too much, as long as retrieving
the
> > WC
> > > does not become the public API of the model.
> > 
> > None of the solutions will work. ManagePasswordsBubbleModel listens to
> > WebContents and zeros the pointer in
WebContentsObserver::ResetWebContents().
> > Making the copy of WebContents* or aura::Window* will result in dangling
> pointer
> > when the user closes the tab. This is the reason why
~WebContentMouseHandler()
> > checks for NULL pointer before unsubscribing.
> 
> I'm surprised to hear that the bubble lives on after the WC for which it was
> created is destroyed. Would it make sense to override WebContentsDestroyed()
in
> the model, and signal that to the bubble, so that it can close itself?

No, the bubble already closes through a different code path in this case.
The bubble isn't owned by WebContents. It's destroyed when the actual window is
destroyed.

[vabr (Chromium), 2014-08-28 13:58:57]

On 2014/08/28 13:36:02, vasilii wrote:
> On 2014/08/28 12:11:46, vabr (Chromium) wrote:
> > On 2014/08/28 11:53:55, vasilii wrote:
> > >
> >
>
https://codereview.chromium.org/512683003/diff/1/chrome/browser/ui/passwords/...
> > > File chrome/browser/ui/passwords/manage_passwords_bubble_model.h (right):
> > > 
> > >
> >
>
https://codereview.chromium.org/512683003/diff/1/chrome/browser/ui/passwords/...
> > > chrome/browser/ui/passwords/manage_passwords_bubble_model.h:90: using
> > > WebContentsObserver::web_contents;
> > > On 2014/08/28 11:42:37, vabr (Chromium) wrote:
> > > > On 2014/08/27 17:24:24, vasilii wrote:
> > > > > On 2014/08/27 16:43:05, vabr (Chromium) wrote:
> > > > > > Instead of making the whole accessor public, could you instead
expose
> > just
> > > a
> > > > > > public method to access the aura::Window*?
> > > > > > Sounds like that would be enough, and keeping access restricted in
> > general
> > > > is
> > > > > a
> > > > > > recommendable practice.
> > > > > 
> > > > > aura::Window* is a very strange type to return from this class. This
is
> > very
> > > > > bottom layer ui primitive. Additionally the bubble model has nothing
to
> do
> > > > with
> > > > > any of the windows in the WebContents hierarchy.
> > > > > This is the second time I get resistance for adding this accessor. No
> idea
> > > > why.
> > > > > ManagePasswordsBubbleView is created with content::WebContents*.
During
> > its
> > > > > lifetime it's perfectly valid to request from inside the bubble which
> > > > > WebContents it was created for.
> > > > 
> > > > Why I don't like exposing the WebContents (WC :)) accessor in the model
--
> > not
> > > > only, as you said, has the model nothing to do with the WC window
> hierarchy,
> > > > it's API also has nothing to do with the WC. It observes the WC, but
> that's
> > an
> > > > implementation detail. The model gets the WC from the bubble, and that's
> > where
> > > > you need to get it from in the bubbleview code. In other words, the
model
> is
> > > to
> > > > provide password-related information to the bubble, and execute
associated
> > > > actions, not to store the WC.
> > > > 
> > > > My suggestion here is: store the WC given to ManagePasswordsBubble on
> > creation
> > > > in a protected member variable. Use that in ManagePasswordsBubbleView
when
> > you
> > > > need to create the WebContentMouseHandler to retrieve the aura::Window.
If
> > you
> > > > must, retrieve the window even first in the WebContentMouseHandler. You
> can
> > > also
> > > > obviously do the other thing and remember the aura::Window intead of the
> WC
> > in
> > > > ManagePasswordsBubble. I don't care here too much, as long as retrieving
> the
> > > WC
> > > > does not become the public API of the model.
> > > 
> > > None of the solutions will work. ManagePasswordsBubbleModel listens to
> > > WebContents and zeros the pointer in
> WebContentsObserver::ResetWebContents().
> > > Making the copy of WebContents* or aura::Window* will result in dangling
> > pointer
> > > when the user closes the tab. This is the reason why
> ~WebContentMouseHandler()
> > > checks for NULL pointer before unsubscribing.
> > 
> > I'm surprised to hear that the bubble lives on after the WC for which it was
> > created is destroyed. Would it make sense to override WebContentsDestroyed()
> in
> > the model, and signal that to the bubble, so that it can close itself?
> 
> No, the bubble already closes through a different code path in this case.
> The bubble isn't owned by WebContents. It's destroyed when the actual window
is
> destroyed.

Vasilii silenced me :) with the argument that the model is by far not the first
class to make the WC accessor public. I'll file a bug to consider making the
accessor public already on the WCObserver.

LGTM.

Vaclav

[vasilii, 2014-08-28 14:00:26]

The CQ bit was checked by vasilii@chromium.org

[commit-bot: I haz the power, 2014-08-28 14:01:17]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/vasilii@chromium.org/512683003/20001

[commit-bot: I haz the power, 2014-08-28 14:51:25]

Committed patchset #2 (id:20001) as b3b4be39b1a5eed23d1c7d93d402cd371bb9b78a

[commit-bot: I haz the power, 2014-09-10 02:59:32]

Patchset 2 (id:??) landed as
https://crrev.com/5db647c6d8a9200bc5ad980fa6cfc9bc688f7deb
Cr-Commit-Position: refs/heads/master@{#292381}