Ignore OnBubbleHidden() event when the password bubble is reopened.

Ignore OnBubbleHidden() event when the password bubble is reopened.

- ManagePasswordsUIController talks to ManagePasswordsBubbleModel though a proxy class. It can be destroyed by the controller so the bubble can't reach the controller with OnBubbleHidden message.
- ManagePasswordsBubbleModel doesn't need to be WebContentsObserver. It should take a pointer to the delegate directly.
- Most unit test can use a new lightweight PasswordsModelDelegateMock. There is no reason to instantiate ManagePasswordsUIControllerMock.

BUG=632408
Committed: https://crrev.com/2d3f7345020554fbf2c0ab10d3c5171cbd36687a
Cr-Commit-Position: refs/heads/master@{#411709}

[vasilii, 2016-08-02 18:43:03]

Description was changed from

==========
Ignore OnBubbleHidden() event when the password bubble is reopened.

BUG=632408
==========

to

==========
Ignore OnBubbleHidden() event when the password bubble is reopened.

- ManagePasswordsUIController talks to ManagePasswordsBubbleModel though a proxy
class. It can be destroyed by the controller so the bubble can't reach the
controller with OnBubbleHidden message.
- ManagePasswordsBubbleModel doesn't need to be WebContentsObserver. It should
take a pointer to the delegate directly.
- Most unit test can use a new lightweight PasswordsModelDelegateMock. There is
no reason to instantiate ManagePasswordsUIControllerMock.

BUG=632408
==========

[vasilii, 2016-08-02 18:44:30]

The CQ bit was checked by vasilii@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-08-02 18:45:13]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[vasilii, 2016-08-02 18:46:47]

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

[vasilii, 2016-08-02 18:46:48]

Hi Scott,

please review chrome/browser/ui/BUILD.gn

[commit-bot: I haz the power, 2016-08-02 19:52:34]

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

[commit-bot: I haz the power, 2016-08-02 19:52:35]

Dry run: This issue passed the CQ dry run.

[sky, 2016-08-02 20:37:47]

You only have myself listed as the reviewer and you ask for a review of one
file. Who is reviewing the rest of the cl?

[vasilii, 2016-08-03 08:31:43]

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

[vasilii, 2016-08-03 08:31:43]

Hi Tanya,

please review the CL.

[melandory, 2016-08-03 09:45:49]

On 2016/08/03 08:31:43, vasilii wrote:
> Hi Tanya,
> 
> please review the CL.

lgtm

[sky, 2016-08-03 15:34:57]

On 2016/08/03 08:31:43, vasilii wrote:
> Hi Tanya,
> 
> please review the CL.

AFAICT melandory@ isn't an owner of any of these files.

[vasilii, 2016-08-03 16:49:35]

On 2016/08/03 15:34:57, sky wrote:
> On 2016/08/03 08:31:43, vasilii wrote:
> > Hi Tanya,
> > 
> > please review the CL.
> 
> AFAICT melandory@ isn't an owner of any of these files.

For the core changes in chrome/browser/ui/passwords/ there are two owners. vabr@
is on vacation. melandory@ being a team member has contributed to the folder.
If you prefer to review the whole CL, please do it.

[sky, 2016-08-03 19:42:01]

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

https://codereview.chromium.org/2202373002/diff/20001/chrome/browser/ui/passw...
chrome/browser/ui/passwords/manage_passwords_ui_controller.h:179: // A proxy for
the bubble. The bubbles of different types can pop up
Forgive me if I am misunderstanding the problem, but it seems like you are
working around the problem. If you are going to show a bubble and there is one
already open, why don't you close the existing one first and ignore anything
from it?

https://codereview.chromium.org/2202373002/diff/20001/chrome/browser/ui/passw...
File chrome/browser/ui/passwords/passwords_model_delegate_proxy.cc (right):

https://codereview.chromium.org/2202373002/diff/20001/chrome/browser/ui/passw...
chrome/browser/ui/passwords/passwords_model_delegate_proxy.cc:77: };
nit: no ;

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

https://codereview.chromium.org/2202373002/diff/20001/chrome/browser/ui/passw...
chrome/browser/ui/passwords/passwords_model_delegate_proxy.h:21:
content::WebContents* GetWebContents() const override;
Prefix with where overriden from, eg:
// PasswordsModelDelegate:

[vasilii, 2016-08-04 09:27:47]

The CQ bit was checked by vasilii@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-08-04 09:28:03]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[vasilii, 2016-08-04 09:29:13]

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

https://codereview.chromium.org/2202373002/diff/20001/chrome/browser/ui/passw...
chrome/browser/ui/passwords/manage_passwords_ui_controller.h:179: // A proxy for
the bubble. The bubbles of different types can pop up
On 2016/08/03 19:42:00, sky wrote:
> Forgive me if I am misunderstanding the problem, but it seems like you are
> working around the problem. If you are going to show a bubble and there is one
> already open, why don't you close the existing one first and ignore anything
> from it?

I do close the existing bubble and ignore anything from it. Though it's done in
the platform specific code, not here. Let's say I introduce a method
CloseExistingBubble here. Still on Windows it will be destroyed asynchronously
and I'll get OnBubbleHidden later when the new bubble is open.

There is another benefit of the current approach vs. bool ignore_close_.
ManagePasswordsBubbleModel extends WebContentsObserver for no reason. It just
needs web_contents(). Thus, it makes sense to pass some pointer to it in the
constructor. It can't be WebContents because it can be destroyed. It shouldn't
be ManagePasswordsUIController* either. If I close the tab by Ctrl-W then the
bubble outlives the controller. The bubble on Windows is  destroyed
asynchronously. The bubble on Mac is destroyed after animation finishes. Thus,
there is a good chance of use after free.

https://codereview.chromium.org/2202373002/diff/20001/chrome/browser/ui/passw...
File chrome/browser/ui/passwords/passwords_model_delegate_proxy.cc (right):

https://codereview.chromium.org/2202373002/diff/20001/chrome/browser/ui/passw...
chrome/browser/ui/passwords/passwords_model_delegate_proxy.cc:77: };
On 2016/08/03 19:42:00, sky wrote:
> nit: no ;

Done.

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

https://codereview.chromium.org/2202373002/diff/20001/chrome/browser/ui/passw...
chrome/browser/ui/passwords/passwords_model_delegate_proxy.h:21:
content::WebContents* GetWebContents() const override;
On 2016/08/03 19:42:00, sky wrote:
> Prefix with where overriden from, eg:
> // PasswordsModelDelegate:

Done.

[commit-bot: I haz the power, 2016-08-04 10:29:03]

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

[commit-bot: I haz the power, 2016-08-04 10:29:04]

Dry run: This issue passed the CQ dry run.

[sky, 2016-08-04 20:18:11]

On 2016/08/04 09:29:13, vasilii wrote:
>
https://codereview.chromium.org/2202373002/diff/20001/chrome/browser/ui/passw...
> File chrome/browser/ui/passwords/manage_passwords_ui_controller.h (right):
> 
>
https://codereview.chromium.org/2202373002/diff/20001/chrome/browser/ui/passw...
> chrome/browser/ui/passwords/manage_passwords_ui_controller.h:179: // A proxy
for
> the bubble. The bubbles of different types can pop up
> On 2016/08/03 19:42:00, sky wrote:
> > Forgive me if I am misunderstanding the problem, but it seems like you are
> > working around the problem. If you are going to show a bubble and there is
one
> > already open, why don't you close the existing one first and ignore anything
> > from it?
> 
> I do close the existing bubble and ignore anything from it. Though it's done
in
> the platform specific code, not here. Let's say I introduce a method
> CloseExistingBubble here. Still on Windows it will be destroyed asynchronously
> and I'll get OnBubbleHidden later when the new bubble is open.
> 
> There is another benefit of the current approach vs. bool ignore_close_.
> ManagePasswordsBubbleModel extends WebContentsObserver for no reason. It just
> needs web_contents(). Thus, it makes sense to pass some pointer to it in the
> constructor. It can't be WebContents because it can be destroyed. It shouldn't
> be ManagePasswordsUIController* either. If I close the tab by Ctrl-W then the
> bubble outlives the controller. The bubble on Windows is  destroyed
> asynchronously. The bubble on Mac is destroyed after animation finishes. Thus,
> there is a good chance of use after free.
> 
>
https://codereview.chromium.org/2202373002/diff/20001/chrome/browser/ui/passw...
> File chrome/browser/ui/passwords/passwords_model_delegate_proxy.cc (right):
> 
>
https://codereview.chromium.org/2202373002/diff/20001/chrome/browser/ui/passw...
> chrome/browser/ui/passwords/passwords_model_delegate_proxy.cc:77: };
> On 2016/08/03 19:42:00, sky wrote:
> > nit: no ;
> 
> Done.
> 
>
https://codereview.chromium.org/2202373002/diff/20001/chrome/browser/ui/passw...
> File chrome/browser/ui/passwords/passwords_model_delegate_proxy.h (right):
> 
>
https://codereview.chromium.org/2202373002/diff/20001/chrome/browser/ui/passw...
> chrome/browser/ui/passwords/passwords_model_delegate_proxy.h:21:
> content::WebContents* GetWebContents() const override;
> On 2016/08/03 19:42:00, sky wrote:
> > Prefix with where overriden from, eg:
> > // PasswordsModelDelegate:
> 
> Done.

Could you add a parameter to close that indicates the delegate doesn't want any
other calls. The platform specific code would null out the delegate at that
point.

[vasilii, 2016-08-05 12:46:45]

On 2016/08/04 20:18:11, sky wrote:
> On 2016/08/04 09:29:13, vasilii wrote:
> >
>
https://codereview.chromium.org/2202373002/diff/20001/chrome/browser/ui/passw...
> > File chrome/browser/ui/passwords/manage_passwords_ui_controller.h (right):
> > 
> >
>
https://codereview.chromium.org/2202373002/diff/20001/chrome/browser/ui/passw...
> > chrome/browser/ui/passwords/manage_passwords_ui_controller.h:179: // A proxy
> for
> > the bubble. The bubbles of different types can pop up
> > On 2016/08/03 19:42:00, sky wrote:
> > > Forgive me if I am misunderstanding the problem, but it seems like you are
> > > working around the problem. If you are going to show a bubble and there is
> one
> > > already open, why don't you close the existing one first and ignore
anything
> > > from it?
> > 
> > I do close the existing bubble and ignore anything from it. Though it's done
> in
> > the platform specific code, not here. Let's say I introduce a method
> > CloseExistingBubble here. Still on Windows it will be destroyed
asynchronously
> > and I'll get OnBubbleHidden later when the new bubble is open.
> > 
> > There is another benefit of the current approach vs. bool ignore_close_.
> > ManagePasswordsBubbleModel extends WebContentsObserver for no reason. It
just
> > needs web_contents(). Thus, it makes sense to pass some pointer to it in the
> > constructor. It can't be WebContents because it can be destroyed. It
shouldn't
> > be ManagePasswordsUIController* either. If I close the tab by Ctrl-W then
the
> > bubble outlives the controller. The bubble on Windows is  destroyed
> > asynchronously. The bubble on Mac is destroyed after animation finishes.
Thus,
> > there is a good chance of use after free.
> > 
> >
>
https://codereview.chromium.org/2202373002/diff/20001/chrome/browser/ui/passw...
> > File chrome/browser/ui/passwords/passwords_model_delegate_proxy.cc (right):
> > 
> >
>
https://codereview.chromium.org/2202373002/diff/20001/chrome/browser/ui/passw...
> > chrome/browser/ui/passwords/passwords_model_delegate_proxy.cc:77: };
> > On 2016/08/03 19:42:00, sky wrote:
> > > nit: no ;
> > 
> > Done.
> > 
> >
>
https://codereview.chromium.org/2202373002/diff/20001/chrome/browser/ui/passw...
> > File chrome/browser/ui/passwords/passwords_model_delegate_proxy.h (right):
> > 
> >
>
https://codereview.chromium.org/2202373002/diff/20001/chrome/browser/ui/passw...
> > chrome/browser/ui/passwords/passwords_model_delegate_proxy.h:21:
> > content::WebContents* GetWebContents() const override;
> > On 2016/08/03 19:42:00, sky wrote:
> > > Prefix with where overriden from, eg:
> > > // PasswordsModelDelegate:
> > 
> > Done.
> 
> Could you add a parameter to close that indicates the delegate doesn't want
any
> other calls. The platform specific code would null out the delegate at that
> point.

That's possible but it leaves the lifetime problem (bubble vs. controller)
unsolved.

[vabr (Chromium), 2016-08-05 14:14:03]

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

[vabr (Chromium), 2016-08-05 14:14:04]

Vasilii: LGTM with some nits, but obviously please let's clarify any concerns
sky@ has first.

sky@: As for your earlier note on the OWNERS, I believe that vasilii asking
melandory to review the passwords files is in accordance to the rules, with
Vasilii being an OWNER of them already. To be precise,
https://www.chromium.org/developers/owners-files states: "If you are an owner,
another committer must still review the change, but they need not be an owner."

Cheers,
Vaclav

https://codereview.chromium.org/2202373002/diff/40001/chrome/browser/ui/cocoa...
File chrome/browser/ui/cocoa/passwords/base_passwords_controller_test.h (right):

https://codereview.chromium.org/2202373002/diff/40001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/passwords/base_passwords_controller_test.h:51:
std::unique_ptr<PasswordsModelDelegateMock> ui_controller_;
nit: #include <memory> for unique_ptr

https://codereview.chromium.org/2202373002/diff/40001/chrome/browser/ui/passw...
File chrome/browser/ui/passwords/manage_passwords_ui_controller.h (right):

https://codereview.chromium.org/2202373002/diff/40001/chrome/browser/ui/passw...
chrome/browser/ui/passwords/manage_passwords_ui_controller.h:186: // nothing to
do with the old bubble.
optional nit: The comment explains the issue, but not completely how it is
solved by having a proxy. Could you add a sentence about that?

https://codereview.chromium.org/2202373002/diff/40001/chrome/browser/ui/passw...
chrome/browser/ui/passwords/manage_passwords_ui_controller.h:187:
std::unique_ptr<PasswordsModelDelegateProxy> bubble_delegate_proxy_;
nit: #include <memory> for unique_ptr

https://codereview.chromium.org/2202373002/diff/40001/chrome/browser/ui/passw...
File chrome/browser/ui/passwords/manage_passwords_ui_controller_unittest.cc
(right):

https://codereview.chromium.org/2202373002/diff/40001/chrome/browser/ui/passw...
chrome/browser/ui/passwords/manage_passwords_ui_controller_unittest.cc:751: //
Open the autosign bubble.
typo:
autosign -> autosignin

[vasilii, 2016-08-05 16:44:20]

The CQ bit was checked by vasilii@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-08-05 16:44:41]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[vasilii, 2016-08-05 16:45:20]

https://codereview.chromium.org/2202373002/diff/40001/chrome/browser/ui/cocoa...
File chrome/browser/ui/cocoa/passwords/base_passwords_controller_test.h (right):

https://codereview.chromium.org/2202373002/diff/40001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/passwords/base_passwords_controller_test.h:51:
std::unique_ptr<PasswordsModelDelegateMock> ui_controller_;
On 2016/08/05 14:14:04, vabr (holiday until 8 Aug) wrote:
> nit: #include <memory> for unique_ptr

Done.

https://codereview.chromium.org/2202373002/diff/40001/chrome/browser/ui/passw...
File chrome/browser/ui/passwords/manage_passwords_ui_controller.h (right):

https://codereview.chromium.org/2202373002/diff/40001/chrome/browser/ui/passw...
chrome/browser/ui/passwords/manage_passwords_ui_controller.h:186: // nothing to
do with the old bubble.
On 2016/08/05 14:14:04, vabr (holiday until 8 Aug) wrote:
> optional nit: The comment explains the issue, but not completely how it is
> solved by having a proxy. Could you add a sentence about that?

Done.

https://codereview.chromium.org/2202373002/diff/40001/chrome/browser/ui/passw...
chrome/browser/ui/passwords/manage_passwords_ui_controller.h:187:
std::unique_ptr<PasswordsModelDelegateProxy> bubble_delegate_proxy_;
On 2016/08/05 14:14:04, vabr (holiday until 8 Aug) wrote:
> nit: #include <memory> for unique_ptr

Done.

https://codereview.chromium.org/2202373002/diff/40001/chrome/browser/ui/passw...
File chrome/browser/ui/passwords/manage_passwords_ui_controller_unittest.cc
(right):

https://codereview.chromium.org/2202373002/diff/40001/chrome/browser/ui/passw...
chrome/browser/ui/passwords/manage_passwords_ui_controller_unittest.cc:751: //
Open the autosign bubble.
On 2016/08/05 14:14:04, vabr (holiday until 8 Aug) wrote:
> typo:
> autosign -> autosignin

Done.

[commit-bot: I haz the power, 2016-08-05 17:39:24]

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

[commit-bot: I haz the power, 2016-08-05 17:39:25]

Dry run: This issue passed the CQ dry run.

[sky, 2016-08-05 17:58:26]

On Fri, Aug 5, 2016 at 5:46 AM,  <vasilii@chromium.org> wrote:
> On 2016/08/04 20:18:11, sky wrote:
>> On 2016/08/04 09:29:13, vasilii wrote:
>> >
>>
>
https://codereview.chromium.org/2202373002/diff/20001/chrome/browser/ui/passw...
>> > File chrome/browser/ui/passwords/manage_passwords_ui_controller.h
>> > (right):
>> >
>> >
>>
>
https://codereview.chromium.org/2202373002/diff/20001/chrome/browser/ui/passw...
>> > chrome/browser/ui/passwords/manage_passwords_ui_controller.h:179: // A
>> > proxy
>> for
>> > the bubble. The bubbles of different types can pop up
>> > On 2016/08/03 19:42:00, sky wrote:
>> > > Forgive me if I am misunderstanding the problem, but it seems like you
>> > > are
>> > > working around the problem. If you are going to show a bubble and
>> > > there is
>> one
>> > > already open, why don't you close the existing one first and ignore
> anything
>> > > from it?
>> >
>> > I do close the existing bubble and ignore anything from it. Though it's
>> > done
>> in
>> > the platform specific code, not here. Let's say I introduce a method
>> > CloseExistingBubble here. Still on Windows it will be destroyed
> asynchronously
>> > and I'll get OnBubbleHidden later when the new bubble is open.
>> >
>> > There is another benefit of the current approach vs. bool ignore_close_.
>> > ManagePasswordsBubbleModel extends WebContentsObserver for no reason. It
> just
>> > needs web_contents(). Thus, it makes sense to pass some pointer to it in
>> > the
>> > constructor. It can't be WebContents because it can be destroyed. It
> shouldn't
>> > be ManagePasswordsUIController* either. If I close the tab by Ctrl-W
>> > then
> the
>> > bubble outlives the controller. The bubble on Windows is destroyed
>> > asynchronously. The bubble on Mac is destroyed after animation finishes.
> Thus,
>> > there is a good chance of use after free.
>> >
>> >
>>
>
https://codereview.chromium.org/2202373002/diff/20001/chrome/browser/ui/passw...
>> > File chrome/browser/ui/passwords/passwords_model_delegate_proxy.cc
>> > (right):
>> >
>> >
>>
>
https://codereview.chromium.org/2202373002/diff/20001/chrome/browser/ui/passw...
>> > chrome/browser/ui/passwords/passwords_model_delegate_proxy.cc:77: };
>> > On 2016/08/03 19:42:00, sky wrote:
>> > > nit: no ;
>> >
>> > Done.
>> >
>> >
>>
>
https://codereview.chromium.org/2202373002/diff/20001/chrome/browser/ui/passw...
>> > File chrome/browser/ui/passwords/passwords_model_delegate_proxy.h
>> > (right):
>> >
>> >
>>
>
https://codereview.chromium.org/2202373002/diff/20001/chrome/browser/ui/passw...
>> > chrome/browser/ui/passwords/passwords_model_delegate_proxy.h:21:
>> > content::WebContents* GetWebContents() const override;
>> > On 2016/08/03 19:42:00, sky wrote:
>> > > Prefix with where overriden from, eg:
>> > > // PasswordsModelDelegate:
>> >
>> > Done.
>>
>> Could you add a parameter to close that indicates the delegate doesn't
>> want
> any
>> other calls. The platform specific code would null out the delegate at
>> that
>> point.
>
> That's possible but it leaves the lifetime problem (bubble vs. controller)
> unsolved.

Could you please clarify the lifetime issue that leaves? Also, it
seems like what I'm proposing would be a lot less code. Am I mistaken
there?

  -Scott

> https://codereview.chromium.org/2202373002/

-- 
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[vasilii, 2016-08-08 10:02:52]

On 2016/08/05 17:58:26, sky wrote:
> On Fri, Aug 5, 2016 at 5:46 AM,  <mailto:vasilii@chromium.org> wrote:
> > On 2016/08/04 20:18:11, sky wrote:
> >> On 2016/08/04 09:29:13, vasilii wrote:
> >> >
> >>
> >
>
https://codereview.chromium.org/2202373002/diff/20001/chrome/browser/ui/passw...
> >> > File chrome/browser/ui/passwords/manage_passwords_ui_controller.h
> >> > (right):
> >> >
> >> >
> >>
> >
>
https://codereview.chromium.org/2202373002/diff/20001/chrome/browser/ui/passw...
> >> > chrome/browser/ui/passwords/manage_passwords_ui_controller.h:179: // A
> >> > proxy
> >> for
> >> > the bubble. The bubbles of different types can pop up
> >> > On 2016/08/03 19:42:00, sky wrote:
> >> > > Forgive me if I am misunderstanding the problem, but it seems like you
> >> > > are
> >> > > working around the problem. If you are going to show a bubble and
> >> > > there is
> >> one
> >> > > already open, why don't you close the existing one first and ignore
> > anything
> >> > > from it?
> >> >
> >> > I do close the existing bubble and ignore anything from it. Though it's
> >> > done
> >> in
> >> > the platform specific code, not here. Let's say I introduce a method
> >> > CloseExistingBubble here. Still on Windows it will be destroyed
> > asynchronously
> >> > and I'll get OnBubbleHidden later when the new bubble is open.
> >> >
> >> > There is another benefit of the current approach vs. bool ignore_close_.
> >> > ManagePasswordsBubbleModel extends WebContentsObserver for no reason. It
> > just
> >> > needs web_contents(). Thus, it makes sense to pass some pointer to it in
> >> > the
> >> > constructor. It can't be WebContents because it can be destroyed. It
> > shouldn't
> >> > be ManagePasswordsUIController* either. If I close the tab by Ctrl-W
> >> > then
> > the
> >> > bubble outlives the controller. The bubble on Windows is destroyed
> >> > asynchronously. The bubble on Mac is destroyed after animation finishes.
> > Thus,
> >> > there is a good chance of use after free.
> >> >
> >> >
> >>
> >
>
https://codereview.chromium.org/2202373002/diff/20001/chrome/browser/ui/passw...
> >> > File chrome/browser/ui/passwords/passwords_model_delegate_proxy.cc
> >> > (right):
> >> >
> >> >
> >>
> >
>
https://codereview.chromium.org/2202373002/diff/20001/chrome/browser/ui/passw...
> >> > chrome/browser/ui/passwords/passwords_model_delegate_proxy.cc:77: };
> >> > On 2016/08/03 19:42:00, sky wrote:
> >> > > nit: no ;
> >> >
> >> > Done.
> >> >
> >> >
> >>
> >
>
https://codereview.chromium.org/2202373002/diff/20001/chrome/browser/ui/passw...
> >> > File chrome/browser/ui/passwords/passwords_model_delegate_proxy.h
> >> > (right):
> >> >
> >> >
> >>
> >
>
https://codereview.chromium.org/2202373002/diff/20001/chrome/browser/ui/passw...
> >> > chrome/browser/ui/passwords/passwords_model_delegate_proxy.h:21:
> >> > content::WebContents* GetWebContents() const override;
> >> > On 2016/08/03 19:42:00, sky wrote:
> >> > > Prefix with where overriden from, eg:
> >> > > // PasswordsModelDelegate:
> >> >
> >> > Done.
> >>
> >> Could you add a parameter to close that indicates the delegate doesn't
> >> want
> > any
> >> other calls. The platform specific code would null out the delegate at
> >> that
> >> point.
> >
> > That's possible but it leaves the lifetime problem (bubble vs. controller)
> > unsolved.
> 
> Could you please clarify the lifetime issue that leaves? Also, it
> seems like what I'm proposing would be a lot less code. Am I mistaken
> there?
> 
>   -Scott
> 
> > https://codereview.chromium.org/2202373002/
> 
> -- 
> You received this message because you are subscribed to the Google Groups
> "Chromium-reviews" group.
> To unsubscribe from this group and stop receiving emails from it, send an
email
> to mailto:chromium-reviews+unsubscribe@chromium.org.
> 

I described it in comment #17 second paragraph.

Less code maybe. Less robustness for sure. I'm not a fan of passing raw pointers
around while we know for sure that the lifetimes of the bubble and the
controller are not tied together. There are a few places (either platform
specific or nor) where the bubble is closed. It will still be alive for some
time and the controller may go away. This is my concern about passing
ManagePasswordsUIController* to the bubble.

[sky, 2016-08-08 15:14:22]

On 2016/08/08 10:02:52, vasilii wrote:
> On 2016/08/05 17:58:26, sky wrote:
> > On Fri, Aug 5, 2016 at 5:46 AM,  <mailto:vasilii@chromium.org> wrote:
> > > On 2016/08/04 20:18:11, sky wrote:
> > >> On 2016/08/04 09:29:13, vasilii wrote:
> > >> >
> > >>
> > >
> >
>
https://codereview.chromium.org/2202373002/diff/20001/chrome/browser/ui/passw...
> > >> > File chrome/browser/ui/passwords/manage_passwords_ui_controller.h
> > >> > (right):
> > >> >
> > >> >
> > >>
> > >
> >
>
https://codereview.chromium.org/2202373002/diff/20001/chrome/browser/ui/passw...
> > >> > chrome/browser/ui/passwords/manage_passwords_ui_controller.h:179: // A
> > >> > proxy
> > >> for
> > >> > the bubble. The bubbles of different types can pop up
> > >> > On 2016/08/03 19:42:00, sky wrote:
> > >> > > Forgive me if I am misunderstanding the problem, but it seems like
you
> > >> > > are
> > >> > > working around the problem. If you are going to show a bubble and
> > >> > > there is
> > >> one
> > >> > > already open, why don't you close the existing one first and ignore
> > > anything
> > >> > > from it?
> > >> >
> > >> > I do close the existing bubble and ignore anything from it. Though it's
> > >> > done
> > >> in
> > >> > the platform specific code, not here. Let's say I introduce a method
> > >> > CloseExistingBubble here. Still on Windows it will be destroyed
> > > asynchronously
> > >> > and I'll get OnBubbleHidden later when the new bubble is open.
> > >> >
> > >> > There is another benefit of the current approach vs. bool
ignore_close_.
> > >> > ManagePasswordsBubbleModel extends WebContentsObserver for no reason.
It
> > > just
> > >> > needs web_contents(). Thus, it makes sense to pass some pointer to it
in
> > >> > the
> > >> > constructor. It can't be WebContents because it can be destroyed. It
> > > shouldn't
> > >> > be ManagePasswordsUIController* either. If I close the tab by Ctrl-W
> > >> > then
> > > the
> > >> > bubble outlives the controller. The bubble on Windows is destroyed
> > >> > asynchronously. The bubble on Mac is destroyed after animation
finishes.
> > > Thus,
> > >> > there is a good chance of use after free.
> > >> >
> > >> >
> > >>
> > >
> >
>
https://codereview.chromium.org/2202373002/diff/20001/chrome/browser/ui/passw...
> > >> > File chrome/browser/ui/passwords/passwords_model_delegate_proxy.cc
> > >> > (right):
> > >> >
> > >> >
> > >>
> > >
> >
>
https://codereview.chromium.org/2202373002/diff/20001/chrome/browser/ui/passw...
> > >> > chrome/browser/ui/passwords/passwords_model_delegate_proxy.cc:77: };
> > >> > On 2016/08/03 19:42:00, sky wrote:
> > >> > > nit: no ;
> > >> >
> > >> > Done.
> > >> >
> > >> >
> > >>
> > >
> >
>
https://codereview.chromium.org/2202373002/diff/20001/chrome/browser/ui/passw...
> > >> > File chrome/browser/ui/passwords/passwords_model_delegate_proxy.h
> > >> > (right):
> > >> >
> > >> >
> > >>
> > >
> >
>
https://codereview.chromium.org/2202373002/diff/20001/chrome/browser/ui/passw...
> > >> > chrome/browser/ui/passwords/passwords_model_delegate_proxy.h:21:
> > >> > content::WebContents* GetWebContents() const override;
> > >> > On 2016/08/03 19:42:00, sky wrote:
> > >> > > Prefix with where overriden from, eg:
> > >> > > // PasswordsModelDelegate:
> > >> >
> > >> > Done.
> > >>
> > >> Could you add a parameter to close that indicates the delegate doesn't
> > >> want
> > > any
> > >> other calls. The platform specific code would null out the delegate at
> > >> that
> > >> point.
> > >
> > > That's possible but it leaves the lifetime problem (bubble vs. controller)
> > > unsolved.
> > 
> > Could you please clarify the lifetime issue that leaves? Also, it
> > seems like what I'm proposing would be a lot less code. Am I mistaken
> > there?
> > 
> >   -Scott
> > 
> > > https://codereview.chromium.org/2202373002/
> > 
> > -- 
> > You received this message because you are subscribed to the Google Groups
> > "Chromium-reviews" group.
> > To unsubscribe from this group and stop receiving emails from it, send an
> email
> > to mailto:chromium-reviews+unsubscribe@chromium.org.
> > 
> 
> I described it in comment #17 second paragraph.
> 
> Less code maybe. Less robustness for sure. I'm not a fan of passing raw
pointers
> around while we know for sure that the lifetimes of the bubble and the
> controller are not tied together. There are a few places (either platform
> specific or nor) where the bubble is closed. It will still be alive for some
> time and the controller may go away. This is my concern about passing
> ManagePasswordsUIController* to the bubble.

I do agree that not dealing with raw pointers is advantageous. Can you clarify
why you need the proxy? Can't you get the same effect by making
ManagePasswordsUIController (and other delegates) have a weakref that they
invalidate when necessary?

[vasilii, 2016-08-12 15:37:10]

The CQ bit was checked by vasilii@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-08-12 15:37:31]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[vasilii, 2016-08-12 15:38:39]

Dropped the proxy.

[commit-bot: I haz the power, 2016-08-12 16:45:02]

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

[commit-bot: I haz the power, 2016-08-12 16:45:03]

Dry run: Try jobs failed on following builders:
  linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[sky, 2016-08-12 16:59:36]

Much simpler, thanks! LGTM

https://codereview.chromium.org/2202373002/diff/80001/chrome/browser/ui/passw...
File chrome/browser/ui/passwords/manage_passwords_ui_controller_unittest.cc
(right):

https://codereview.chromium.org/2202373002/diff/80001/chrome/browser/ui/passw...
chrome/browser/ui/passwords/manage_passwords_ui_controller_unittest.cc:752:
ScopedVector<autofill::PasswordForm> local_credentials;
ScopedVector is deprecated, use std::vector<std::unique_ptr>. But I see you need
it for OnAutoSignin. It would be good if this code could be converted.

[vasilii, 2016-08-12 17:26:42]

Thank you!

https://codereview.chromium.org/2202373002/diff/80001/chrome/browser/ui/passw...
File chrome/browser/ui/passwords/manage_passwords_ui_controller_unittest.cc
(right):

https://codereview.chromium.org/2202373002/diff/80001/chrome/browser/ui/passw...
chrome/browser/ui/passwords/manage_passwords_ui_controller_unittest.cc:752:
ScopedVector<autofill::PasswordForm> local_credentials;
On 2016/08/12 16:59:35, sky wrote:
> ScopedVector is deprecated, use std::vector<std::unique_ptr>. But I see you
need
> it for OnAutoSignin. It would be good if this code could be converted.

We have a tracking bug for this http://crbug.com/555132
I'll convert this part of the code soon.

[vasilii, 2016-08-12 17:26:55]

The CQ bit was checked by vasilii@chromium.org

[vasilii, 2016-08-12 17:26:55]

The patchset sent to the CQ was uploaded after l-g-t-m from
melandory@chromium.org, vabr@chromium.org
Link to the patchset: https://codereview.chromium.org/2202373002/#ps80001
(title: "delete proxy")

[commit-bot: I haz the power, 2016-08-12 17:27:09]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-08-12 18:29:12]

Description was changed from

==========
Ignore OnBubbleHidden() event when the password bubble is reopened.

- ManagePasswordsUIController talks to ManagePasswordsBubbleModel though a proxy
class. It can be destroyed by the controller so the bubble can't reach the
controller with OnBubbleHidden message.
- ManagePasswordsBubbleModel doesn't need to be WebContentsObserver. It should
take a pointer to the delegate directly.
- Most unit test can use a new lightweight PasswordsModelDelegateMock. There is
no reason to instantiate ManagePasswordsUIControllerMock.

BUG=632408
==========

to

==========
Ignore OnBubbleHidden() event when the password bubble is reopened.

- ManagePasswordsUIController talks to ManagePasswordsBubbleModel though a proxy
class. It can be destroyed by the controller so the bubble can't reach the
controller with OnBubbleHidden message.
- ManagePasswordsBubbleModel doesn't need to be WebContentsObserver. It should
take a pointer to the delegate directly.
- Most unit test can use a new lightweight PasswordsModelDelegateMock. There is
no reason to instantiate ManagePasswordsUIControllerMock.

BUG=632408
==========

[commit-bot: I haz the power, 2016-08-12 18:29:15]

Committed patchset #5 (id:80001)

[commit-bot: I haz the power, 2016-08-12 18:31:04]

Description was changed from

==========
Ignore OnBubbleHidden() event when the password bubble is reopened.

- ManagePasswordsUIController talks to ManagePasswordsBubbleModel though a proxy
class. It can be destroyed by the controller so the bubble can't reach the
controller with OnBubbleHidden message.
- ManagePasswordsBubbleModel doesn't need to be WebContentsObserver. It should
take a pointer to the delegate directly.
- Most unit test can use a new lightweight PasswordsModelDelegateMock. There is
no reason to instantiate ManagePasswordsUIControllerMock.

BUG=632408
==========

to

==========
Ignore OnBubbleHidden() event when the password bubble is reopened.

- ManagePasswordsUIController talks to ManagePasswordsBubbleModel though a proxy
class. It can be destroyed by the controller so the bubble can't reach the
controller with OnBubbleHidden message.
- ManagePasswordsBubbleModel doesn't need to be WebContentsObserver. It should
take a pointer to the delegate directly.
- Most unit test can use a new lightweight PasswordsModelDelegateMock. There is
no reason to instantiate ManagePasswordsUIControllerMock.

BUG=632408

Committed: https://crrev.com/2d3f7345020554fbf2c0ab10d3c5171cbd36687a
Cr-Commit-Position: refs/heads/master@{#411709}
==========

[commit-bot: I haz the power, 2016-08-12 18:31:05]

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