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

chrome/browser/ui/passwords/manage_passwords_bubble_model.h

 // Copyright 2013 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #ifndef CHROME_BROWSER_UI_PASSWORDS_MANAGE_PASSWORDS_BUBBLE_MODEL_H_
 #define CHROME_BROWSER_UI_PASSWORDS_MANAGE_PASSWORDS_BUBBLE_MODEL_H_
 
 #include "chrome/browser/ui/passwords/manage_passwords_bubble.h"
 #include "components/autofill/core/common/password_form.h"
 #include "components/password_manager/core/browser/password_manager_metrics_util.h"
@@ skipping 68 matching lines @@
     return best_matches_;
   }
   const base::string16& manage_link() const { return manage_link_; }
   const base::string16& save_confirmation_text() const {
     return save_confirmation_text_;
   }
   const gfx::Range& save_confirmation_link_range() const {
     return save_confirmation_link_range_;
   }
 
+  // Make the accessor public.
+  using WebContentsObserver::web_contents;

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

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.

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

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.

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

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]

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.

+
 #if defined(UNIT_TEST)
   // Gets and sets the reason the bubble was displayed.
   password_manager::metrics_util::UIDisplayDisposition display_disposition()
       const {
     return display_disposition_;
   }
 
   // Gets the reason the bubble was dismissed.
   password_manager::metrics_util::UIDismissalReason dismissal_reason() const {
     return dismissal_reason_;
@@ skipping 12 matching lines @@
   base::string16 save_confirmation_text_;
   gfx::Range save_confirmation_link_range_;
 
   password_manager::metrics_util::UIDisplayDisposition display_disposition_;
   password_manager::metrics_util::UIDismissalReason dismissal_reason_;
 
   DISALLOW_COPY_AND_ASSIGN(ManagePasswordsBubbleModel);
 };
 
 #endif  // CHROME_BROWSER_UI_PASSWORDS_MANAGE_PASSWORDS_BUBBLE_MODEL_H_