Password bubble: Deplatformify ManagePasswordsBubbleView.

chrome/browser/ui/passwords/manage_passwords_bubble.h

+// Copyright 2014 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_H_
+#define CHROME_BROWSER_UI_PASSWORDS_MANAGE_PASSWORDS_BUBBLE_H_
+
+#include "base/basictypes.h"
+#include "components/password_manager/core/browser/password_manager_metrics_util.h"
+
+class ManagePasswordsBubbleModel;
+
+namespace content {
+class WebContents;
+}  // namespace content
+
+// Interface for platform-specific bubble code.

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

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."

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

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

+class ManagePasswordsBubble {
+ public:
+  enum DisplayReason { AUTOMATIC, USER_ACTION };
+
+  ManagePasswordsBubbleModel* model() const { return model_; }

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

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.

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

Could this be protected?

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

It could!

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

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.

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

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).

+
+ protected:
+  ManagePasswordsBubble(content::WebContents* contents, DisplayReason reason);

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

nit: comment on the use of |contents|, i.e., that it only serves to create the
model.

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

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

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

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.

+  ~ManagePasswordsBubble();
+
+ private:
+  ManagePasswordsBubbleModel* model_;

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

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?

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

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. :)

+
+  DISALLOW_COPY_AND_ASSIGN(ManagePasswordsBubble);
+};
+
+#endif  // CHROME_BROWSER_UI_PASSWORDS_MANAGE_PASSWORDS_BUBBLE_H_