Password bubble: Deplatformify ManagePasswordsBubbleView.

Password bubble: Deplatformify ManagePasswordsBubbleView.

This CL does a few things in the hopes of reducing the responsibility of
ManagePasswordsBubbleView, working towards making it possible to create
bubbles in a cross-platform way:

1. Logging functionality moves out of ManagePasswordsBubbleView, into
   ManagePasswordsBubbleModel.

2. Model creation is moved out of ManagePasswordsBubbleView into a new
   superclass, ManagePasswordsBubble.

3. Metrics bits are moved up into the password manager component.

BUG=261628
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=265243

[Mike West, 2014-04-10 09:02:53]

Hi Markus, Vaclav!

Can you take a moment to review this CL?

Thanks!

-mike

[vabr (Chromium), 2014-04-10 09:30:24]

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

https://codereview.chromium.org/232753002/diff/1/chrome/browser/ui/passwords/...
chrome/browser/ui/passwords/manage_passwords_bubble.h:17: // Interface for
platform-specific bubble code.
nit: It's not purely an interface, because it also creates the model. I suggest
mentioning this, e.g., by saying "Interface ... code, which also takes care of
keeping the model."

https://codereview.chromium.org/232753002/diff/1/chrome/browser/ui/passwords/...
chrome/browser/ui/passwords/manage_passwords_bubble.h:22:
ManagePasswordsBubbleModel* model() const { return model_; }
Is the |model_| owned by the ManagePasswordBubble? If so, then this accessor
should not be const, IMO, as it allows you to modify a part of the bubble. If
not, then it's fine to keep it const.

https://codereview.chromium.org/232753002/diff/1/chrome/browser/ui/passwords/...
chrome/browser/ui/passwords/manage_passwords_bubble.h:22:
ManagePasswordsBubbleModel* model() const { return model_; }
Could this be protected?

https://codereview.chromium.org/232753002/diff/1/chrome/browser/ui/passwords/...
chrome/browser/ui/passwords/manage_passwords_bubble.h:25:
ManagePasswordsBubble(content::WebContents* contents, DisplayReason reason);
nit: comment on the use of |contents|, i.e., that it only serves to create the
model.

https://codereview.chromium.org/232753002/diff/1/chrome/browser/ui/passwords/...
chrome/browser/ui/passwords/manage_passwords_bubble.h:29:
ManagePasswordsBubbleModel* model_;
Please account for idiots like me, and comment on ownership and lifetime
expectancy. :)
I know almost nothing about models, and to me it seems strange: the
ManagePasswordBubble creates the model, but then keeps it in a raw pointer and
does not care. Does it self-destruct? When?

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

https://codereview.chromium.org/232753002/diff/1/chrome/browser/ui/passwords/...
chrome/browser/ui/passwords/manage_passwords_bubble_model.h:23: explicit
ManagePasswordsBubbleModel(content::WebContents* web_contents);
Would it make sense to comment on the default values for dismissal and creation
reasons assigned by the constructor?

https://codereview.chromium.org/232753002/diff/1/chrome/browser/ui/passwords/...
chrome/browser/ui/passwords/manage_passwords_bubble_model.h:62: // Close the
bubble without logging.
nit: This does not really say more than the function name. Culd you elaborate on
the "logging"? (Is it UMA?)

https://codereview.chromium.org/232753002/diff/1/chrome/browser/ui/passwords/...
chrome/browser/ui/passwords/manage_passwords_bubble_model.h:92: void
set_display_disposition(
I might be mistaken, but all these 4 accessors only used within the class? If
so, then I suggest getting rid of them, or making them private.

[Mike West, 2014-04-10 09:53:46]

Thanks Vaclav! I'll work through these while I write tests(!!!) for the next
patchset.

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

https://codereview.chromium.org/232753002/diff/1/chrome/browser/ui/passwords/...
chrome/browser/ui/passwords/manage_passwords_bubble.h:17: // Interface for
platform-specific bubble code.
On 2014/04/10 09:30:24, vabr (Chromium) wrote:
> nit: It's not purely an interface, because it also creates the model. I
suggest
> mentioning this, e.g., by saying "Interface ... code, which also takes care of
> keeping the model."

Yeah, this was initially a pure interface. It grew, and I didn't update the
comments. I'll do that in the next patchset.

https://codereview.chromium.org/232753002/diff/1/chrome/browser/ui/passwords/...
chrome/browser/ui/passwords/manage_passwords_bubble.h:22:
ManagePasswordsBubbleModel* model() const { return model_; }
On 2014/04/10 09:30:24, vabr (Chromium) wrote:
> Could this be protected?

It could!

https://codereview.chromium.org/232753002/diff/1/chrome/browser/ui/passwords/...
chrome/browser/ui/passwords/manage_passwords_bubble.h:22:
ManagePasswordsBubbleModel* model() const { return model_; }
On 2014/04/10 09:30:24, vabr (Chromium) wrote:
> Is the |model_| owned by the ManagePasswordBubble? If so, then this accessor
> should not be const, IMO, as it allows you to modify a part of the bubble. If
> not, then it's fine to keep it const.

Well, it's const, in that the _bubble_ isn't modified as a side-effect of
grabbing the model. The model might be modified, but not the bubble.

*shrug* I'll drop the const, it's not particularly important.

https://codereview.chromium.org/232753002/diff/1/chrome/browser/ui/passwords/...
chrome/browser/ui/passwords/manage_passwords_bubble.h:25:
ManagePasswordsBubble(content::WebContents* contents, DisplayReason reason);
On 2014/04/10 09:30:24, vabr (Chromium) wrote:
> nit: comment on the use of |contents|, i.e., that it only serves to create the
> model.

I'll comment it for now, but I'm trying to get rid of WebContents from this
class in https://codereview.chromium.org/228603003/.

https://codereview.chromium.org/232753002/diff/1/chrome/browser/ui/passwords/...
chrome/browser/ui/passwords/manage_passwords_bubble.h:29:
ManagePasswordsBubbleModel* model_;
On 2014/04/10 09:30:24, vabr (Chromium) wrote:
> Please account for idiots like me, and comment on ownership and lifetime
> expectancy. :)
> I know almost nothing about models, and to me it seems strange: the
> ManagePasswordBubble creates the model, but then keeps it in a raw pointer and
> does not care. Does it self-destruct? When?

You're right. This should be a scoped_ptr. It should be a scoped_ptr in the
existing code, of course, so I blame whoever I copied/pasted from. :)

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

https://codereview.chromium.org/232753002/diff/1/chrome/browser/ui/passwords/...
chrome/browser/ui/passwords/manage_passwords_bubble_model.h:23: explicit
ManagePasswordsBubbleModel(content::WebContents* web_contents);
On 2014/04/10 09:30:24, vabr (Chromium) wrote:
> Would it make sense to comment on the default values for dismissal and
creation
> reasons assigned by the constructor?

Done.

https://codereview.chromium.org/232753002/diff/1/chrome/browser/ui/passwords/...
chrome/browser/ui/passwords/manage_passwords_bubble_model.h:92: void
set_display_disposition(
On 2014/04/10 09:30:24, vabr (Chromium) wrote:
> I might be mistaken, but all these 4 accessors only used within the class? If
> so, then I suggest getting rid of them, or making them private.

They're also used in the tests I haven't finished writing yet.

[vabr (Chromium), 2014-04-10 10:24:44]

Thanks, Mike!
Ah, yes, indeed, the tests! I somehow always forget to comment on the important
("once you call this sometimes" -> "I should also call it!" etc.) :)

Just ping me once you add the tests and want me to take a new look.

Thanks!
Vaclav

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

https://codereview.chromium.org/232753002/diff/1/chrome/browser/ui/passwords/...
chrome/browser/ui/passwords/manage_passwords_bubble.h:22:
ManagePasswordsBubbleModel* model() const { return model_; }
On 2014/04/10 09:53:46, Mike West wrote:
> On 2014/04/10 09:30:24, vabr (Chromium) wrote:
> > Is the |model_| owned by the ManagePasswordBubble? If so, then this accessor
> > should not be const, IMO, as it allows you to modify a part of the bubble.
If
> > not, then it's fine to keep it const.
> 
> Well, it's const, in that the _bubble_ isn't modified as a side-effect of
> grabbing the model. The model might be modified, but not the bubble.
> 
> *shrug* I'll drop the const, it's not particularly important.

I agree this is a fuzzy area. And different people have different opinions
(https://codereview.chromium.org/228263004/diff/110001/components/password_man...).
If you reconsider, we can talk, but I'm obviously fine with your last offer
(dropping the const).

https://codereview.chromium.org/232753002/diff/1/chrome/browser/ui/passwords/...
chrome/browser/ui/passwords/manage_passwords_bubble.h:25:
ManagePasswordsBubble(content::WebContents* contents, DisplayReason reason);
On 2014/04/10 09:53:46, Mike West wrote:
> On 2014/04/10 09:30:24, vabr (Chromium) wrote:
> > nit: comment on the use of |contents|, i.e., that it only serves to create
the
> > model.
> 
> I'll comment it for now, but I'm trying to get rid of WebContents from this
> class in https://codereview.chromium.org/228603003/. 

Fair enough. All these three ways of landing this CL seem OK to me:
1) Commenting on the use of the WC (not meaning the use of restrooms).
2) Adding a TODO(mkwst) to remove the WC.
3) Actually having the WC removed.

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

https://codereview.chromium.org/232753002/diff/1/chrome/browser/ui/passwords/...
chrome/browser/ui/passwords/manage_passwords_bubble_model.h:92: void
set_display_disposition(
On 2014/04/10 09:53:46, Mike West wrote:
> On 2014/04/10 09:30:24, vabr (Chromium) wrote:
> > I might be mistaken, but all these 4 accessors only used within the class?
If
> > so, then I suggest getting rid of them, or making them private.
> 
> They're also used in the tests I haven't finished writing yet.

But then please mark them as for testing only, and use direct access in the
class code. It will make the code clearer, this way it 1) looks like everybody
is welcome to change these members, and 2) inside the class it's confusing to
see the accessors (is it modifying something outside of the class? is there some
side-effect/processing?).

https://codereview.chromium.org/232753002/diff/1/components/password_manager/...
File components/password_manager/core/browser/password_manager_metrics_util.h
(right):

https://codereview.chromium.org/232753002/diff/1/components/password_manager/...
components/password_manager/core/browser/password_manager_metrics_util.h:25: //
Metrics: "PasswordBubble.DisplayDisposition"
Higher up in the CL, you talk about values of this type as "reasons". I'm not
sure I understand the word "disposition" properly, although it's one of those
cosmopolitans, which also happens to be a Czech word (and I'm sure has different
meaning from [define disposition]).

If you are introducing the name here, could you reconsider it?
If it already is used somewhere in this context, then it's fine to leave it.

[vabr (Chromium), 2014-04-10 10:47:53]

> I agree this is a fuzzy area. And different people have different opinions
>
(https://codereview.chromium.org/228263004/diff/110001/components/password_man...).
> If you reconsider, we can talk, but I'm obviously fine with your last offer
> (dropping the const).
Just to avoid confusion, I actually agree with Ilya on what he wrote in the
linked comment. And therefore I'm glad you removed the const, and would suggest
it now even if the model_ weren't owned by the bubble.

[Mike West, 2014-04-10 12:04:16]

Mind taking another look, Vaclav (and Markus :) )?

[vabr (Chromium), 2014-04-10 12:35:14]

Thanks, Mike. LGTM with comments.

https://codereview.chromium.org/232753002/diff/20001/chrome/browser/ui/passwo...
File chrome/browser/ui/passwords/manage_passwords_bubble_model_unittest.cc
(right):

https://codereview.chromium.org/232753002/diff/20001/chrome/browser/ui/passwo...
chrome/browser/ui/passwords/manage_passwords_bubble_model_unittest.cc:236:
TEST_F(ManagePasswordsBubbleModelTest, DismissalReasons) {
optional: these EXPECT_EQ calls could be included in the tests above. They seem
to logically fit each in the appropriate test above, rather than all together
here. It also creates an unnatural test scenario, where the model is notified of
5 ways of closing without a single way to open.

https://codereview.chromium.org/232753002/diff/20001/chrome/browser/ui/passwo...
chrome/browser/ui/passwords/manage_passwords_bubble_model_unittest.cc:237: 
nit: please remove the blank line

https://codereview.chromium.org/232753002/diff/20001/chrome/browser/ui/passwo...
chrome/browser/ui/passwords/manage_passwords_bubble_model_unittest.cc:267: /*
Did you mean to remove this?
If you meant to disable, then please use the DISABLED_ prefix and explain. Also,
remove the blank lines in the test body.

https://codereview.chromium.org/232753002/diff/20001/chrome/browser/ui/passwo...
File chrome/browser/ui/passwords/manage_passwords_bubble_ui_controller.h
(right):

https://codereview.chromium.org/232753002/diff/20001/chrome/browser/ui/passwo...
chrome/browser/ui/passwords/manage_passwords_bubble_ui_controller.h:104: friend
class TestManagePasswordsBubbleUIController;
This does not seem right. TestManagePasswordsBubbleUIController is a child of
ManagePasswordsBubbleUIController, so please just make the needed private parts
of it protected, and get rid of the friend declaration. Relatives should not be
friends! :)

[Mike West, 2014-04-10 12:46:42]

https://codereview.chromium.org/232753002/diff/20001/chrome/browser/ui/passwo...
File chrome/browser/ui/passwords/manage_passwords_bubble_model_unittest.cc
(right):

https://codereview.chromium.org/232753002/diff/20001/chrome/browser/ui/passwo...
chrome/browser/ui/passwords/manage_passwords_bubble_model_unittest.cc:236:
TEST_F(ManagePasswordsBubbleModelTest, DismissalReasons) {
On 2014/04/10 12:35:14, vabr (Chromium) wrote:
> optional: these EXPECT_EQ calls could be included in the tests above. They
seem
> to logically fit each in the appropriate test above, rather than all together
> here. It also creates an unnatural test scenario, where the model is notified
of
> 5 ways of closing without a single way to open.

Indeed. I meant to remove this test after creating the individual method-based
tests. Thanks for reminding me. :)

https://codereview.chromium.org/232753002/diff/20001/chrome/browser/ui/passwo...
chrome/browser/ui/passwords/manage_passwords_bubble_model_unittest.cc:237: 
On 2014/04/10 12:35:14, vabr (Chromium) wrote:
> nit: please remove the blank line

Done.

https://codereview.chromium.org/232753002/diff/20001/chrome/browser/ui/passwo...
chrome/browser/ui/passwords/manage_passwords_bubble_model_unittest.cc:267: /*
On 2014/04/10 12:35:14, vabr (Chromium) wrote:
> Did you mean to remove this?
> If you meant to disable, then please use the DISABLED_ prefix and explain.
Also,
> remove the blank lines in the test body.

Meant to kill this, thanks!

https://codereview.chromium.org/232753002/diff/20001/chrome/browser/ui/passwo...
File chrome/browser/ui/passwords/manage_passwords_bubble_ui_controller.h
(right):

https://codereview.chromium.org/232753002/diff/20001/chrome/browser/ui/passwo...
chrome/browser/ui/passwords/manage_passwords_bubble_ui_controller.h:104: friend
class TestManagePasswordsBubbleUIController;
On 2014/04/10 12:35:14, vabr (Chromium) wrote:
> This does not seem right. TestManagePasswordsBubbleUIController is a child of
> ManagePasswordsBubbleUIController, so please just make the needed private
parts
> of it protected, and get rid of the friend declaration. Relatives should not
be
> friends! :)

You're right, of course. Not sure what I was thinking here (other than "Just
compile, damnit!" :) ).

[Mike West, 2014-04-22 06:30:22]

Markus: Ping! :)

[markusheintz_, 2014-04-22 11:00:53]

LGTM

https://codereview.chromium.org/232753002/diff/110001/chrome/browser/ui/passw...
File chrome/browser/ui/passwords/manage_passwords_bubble_model.h (right):

https://codereview.chromium.org/232753002/diff/110001/chrome/browser/ui/passw...
chrome/browser/ui/passwords/manage_passwords_bubble_model.h:21: class
ManagePasswordsBubbleModel : public content::WebContentsObserver {
For future CLs: This class should not be called model any more. It is more a
controller.

https://codereview.chromium.org/232753002/diff/110001/chrome/browser/ui/passw...
File chrome/browser/ui/passwords/test_manage_passwords_bubble_ui_controller.h
(right):

https://codereview.chromium.org/232753002/diff/110001/chrome/browser/ui/passw...
chrome/browser/ui/passwords/test_manage_passwords_bubble_ui_controller.h:15:
class TestManagePasswordsBubbleUIController
nit: I know we use TestABC in other places but I think this should be called
ManagedPasswordsBubbleUIControllerStub or Mock.

https://codereview.chromium.org/232753002/diff/110001/chrome/browser/ui/passw...
chrome/browser/ui/passwords/test_manage_passwords_bubble_ui_controller.h:16: :
public ManagePasswordsBubbleUIController {
I wonder whether there should be a common interface that can be implemented
instead of extending this class.

https://codereview.chromium.org/232753002/diff/110001/chrome/browser/ui/passw...
chrome/browser/ui/passwords/test_manage_passwords_bubble_ui_controller.h:22: //
Don't _actually_ navigate in a test. That would be silly.
Why would it be silly. Please explain this in the comment.

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

https://codereview.chromium.org/232753002/diff/110001/chrome/browser/ui/views...
chrome/browser/ui/views/passwords/manage_passwords_bubble_view.h:82: void
CloseWithoutLogging();
Why is this needed?

[markusheintz_, 2014-04-22 11:00:56]

LGTM

[Mike West, 2014-04-22 11:30:06]

The CQ bit was checked by mkwst@chromium.org

[commit-bot: I haz the power, 2014-04-22 11:30:22]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/mkwst@chromium.org/232753002/130001

[Mike West, 2014-04-22 11:57:39]

https://codereview.chromium.org/232753002/diff/110001/chrome/browser/ui/passw...
File chrome/browser/ui/passwords/manage_passwords_bubble_model.h (right):

https://codereview.chromium.org/232753002/diff/110001/chrome/browser/ui/passw...
chrome/browser/ui/passwords/manage_passwords_bubble_model.h:21: class
ManagePasswordsBubbleModel : public content::WebContentsObserver {
On 2014/04/22 11:00:54, markusheintz_ wrote:
> For future CLs: This class should not be called model any more. It is more a
> controller.

Duly noted. :)

https://codereview.chromium.org/232753002/diff/110001/chrome/browser/ui/passw...
File chrome/browser/ui/passwords/test_manage_passwords_bubble_ui_controller.h
(right):

https://codereview.chromium.org/232753002/diff/110001/chrome/browser/ui/passw...
chrome/browser/ui/passwords/test_manage_passwords_bubble_ui_controller.h:15:
class TestManagePasswordsBubbleUIController
On 2014/04/22 11:00:54, markusheintz_ wrote:
> nit: I know we use TestABC in other places but I think this should be called
> ManagedPasswordsBubbleUIControllerStub or Mock.
> 
> 
> 

Done.

https://codereview.chromium.org/232753002/diff/110001/chrome/browser/ui/passw...
chrome/browser/ui/passwords/test_manage_passwords_bubble_ui_controller.h:16: :
public ManagePasswordsBubbleUIController {
On 2014/04/22 11:00:54, markusheintz_ wrote:
> I wonder whether there should be a common interface that can be implemented
> instead of extending this class.

Extending the class means that I'm actually testing the class' logic. Perhaps
I'm misunderstanding your suggestion?

I'm happy to change this in a future CL if there's a good reason to do so.

https://codereview.chromium.org/232753002/diff/110001/chrome/browser/ui/passw...
chrome/browser/ui/passwords/test_manage_passwords_bubble_ui_controller.h:22: //
Don't _actually_ navigate in a test. That would be silly.
On 2014/04/22 11:00:54, markusheintz_ wrote:
> Why would it be silly. Please explain this in the comment.

Done.

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

https://codereview.chromium.org/232753002/diff/110001/chrome/browser/ui/views...
chrome/browser/ui/views/passwords/manage_passwords_bubble_view.h:82: void
CloseWithoutLogging();
On 2014/04/22 11:00:54, markusheintz_ wrote:
> Why is this needed?

Done.

[commit-bot: I haz the power, 2014-04-22 12:49:33]

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

[commit-bot: I haz the power, 2014-04-22 12:49:34]

Try jobs failed on following builders:
  tryserver.chromium on linux_chromium_chromeos_rel

[Mike West, 2014-04-22 12:50:33]

The CQ bit was checked by mkwst@chromium.org

[commit-bot: I haz the power, 2014-04-22 12:50:39]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/mkwst@chromium.org/232753002/130001

[commit-bot: I haz the power, 2014-04-22 14:04:26]

Change committed as 265243