[views] "Hello, world" autofill imperative API dialog.

chrome/browser/ui/autofill/autofill_dialog_controller.h

+// Copyright (c) 2012 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_AUTOFILL_AUTOFILL_DIALOG_CONTROLLER_H_
+#define CHROME_BROWSER_UI_AUTOFILL_AUTOFILL_DIALOG_CONTROLLER_H_
+
+#include "base/memory/scoped_ptr.h"
+#include "base/string16.h"
+
+namespace content {
+class WebContents;
+}
+
+class AutofillDialogController;
+
+class AutofillDialogView {

[Ilya Sherman, 2012/10/23 00:39:00]

nit: Please include a comment describing this class, especially since we also
have an AutofillPopupView class.

[Ilya Sherman, 2012/10/23 00:39:00]

nit: Please move this class into its own file.

[Evan Stade, 2012/10/23 01:52:04]

oh yea, forgot class level comments. I'll add all those.

[Evan Stade, 2012/10/23 01:52:04]

I don't believe it warrants its own file, but done.

+ public:
+  virtual ~AutofillDialogView;
+
+  // Shows the dialog.
+  virtual void Show() = 0;
+
+  static AutofillDialogView* CreateDialog(AutofillDialogController* controller);

[Ilya Sherman, 2012/10/23 00:39:00]

nit: Please include a comment describing this function, including a mention of
the fact that it's a static factory so that each platform can construct its own
flavor of view.

[Evan Stade, 2012/10/23 01:52:04]

Done.

+};
+
+class AutofillDialogController {

[Ilya Sherman, 2012/10/23 00:39:00]

nit: Please include a brief comment describing this class.

[Evan Stade, 2012/10/23 01:52:04]

Done.

+ public:
+  explicit AutofillDialogController(content::WebContents* contents);
+  ~AutofillDialogController() {}
+
+  void Show();
+
+  // Called by the view.
+  string16 DialogTitle();
+  string16 CancelButtonText();
+  string16 ConfirmButtonText();
+  bool ConfirmButtonEnabled();

[Ilya Sherman, 2012/10/23 00:39:00]

nit: If these are only called by the view, why not have them simply be defined
on the View class?  (Warning: I have never quite figured out what the point of
the controller in an MVC design is.  I might just be showing my ignorance here.)

[Ilya Sherman, 2012/10/23 00:39:00]

nit: These should all be const methods, and the string16 ones should return
const string16s.

[Evan Stade, 2012/10/23 01:52:04]

changed to const methods, but the return values cannot be const refs (and const
without ref on an object has no meaning)

[Evan Stade, 2012/10/23 01:52:04]

the controller contains the logic and state for the dialog. The clearest reason
for its separation from the view is that it contains stuff that's shared by all
views. If these were in each view class there would be duplication (bad). If we
put them in the AutofillDialogView base class, that base class would cease to be
pure virtual and we'd likely be dealing with multiple inheritance (also bad).
Sharing code via composition is preferable to inheritance.

Arguably, the functions which just look up a string from l10n_util don't need to
exist. However I expect that ConfirmButtonText will change based on state, so I
want that logic in the controller, and the others alongside it for consistency.

[Ilya Sherman, 2012/10/23 04:52:29]

Sadly, const without a ref does have a meaning.  Without const, you can do this:

controller.DialogTitle() = "foobar";  // Calls operator= on the result of
controller.DialogTitle()

This can then result in a typo from "==" to "=" resulting in really weird broken
behavior.  Thus, const is recommended even though you're returning a copy.

[Evan Stade, 2012/10/23 18:48:04]

Sorry, I'm not trying to be difficult, but...

if you tried to use controller.DialogTitle() = some_string16 as a truth value,
your code would not compile. gcc states: "error: cannot convert
'std::basic_string<short unsigned int, base::string16_char_traits>' to 'bool'"
and I strongly suspect other compilers would complain as well. Anyway I don't
think we really do this in chrome: I can find 5 instances of a const string
return value in autofill/, and 0 instances in the rest of chrome/browser.

+
+  // Called when the view has been closed. A false value for |accepted|
+  // indicates that the autofill operation should be aborted.

[Ilya Sherman, 2012/10/23 00:39:00]

nit: "autofill" -> "Autofill", since this is referring to the feature name, and
the feature name is capitalized.

[Evan Stade, 2012/10/23 01:52:04]

Done.

+  void ViewClosed(bool accepted);

[Ilya Sherman, 2012/10/23 00:39:00]

nit: This boolean's meaning will not be clear at the call site.  Please create a
two-valued enumeration that can be passed in instead.

[Evan Stade, 2012/10/23 01:52:04]

Done.

+
+  content::WebContents* web_contents() { return contents_; }

[Ilya Sherman, 2012/10/23 00:39:00]

nit: Can this be constified?

[Evan Stade, 2012/10/23 01:52:04]

Done.

+
+ private:
+  // The WebContents that initiated the autofill action.

[Ilya Sherman, 2012/10/23 00:39:00]

nit: The user initiates an Autofill action, not the WebContents.  Perhaps "The
WebContents that contains the form that is being filled"?  (Sorry to be super
pedantic, but I really did find this comment a little confusing.)

[Evan Stade, 2012/10/23 01:52:04]

well, it's initiated by a javascript call which is not necessarily a result of a
user action.

[Ilya Sherman, 2012/10/23 04:52:29]

Fair enough, but I still think that describing this as initiated *by* the
WebContents is weird.

[Evan Stade, 2012/10/23 18:48:04]

I tried to improve the wording.

+  content::WebContents* contents_;

[Ilya Sherman, 2012/10/23 00:39:00]

nit: content::WebContents* const contents_;

[Evan Stade, 2012/10/23 01:52:04]

Done.

+
+  scoped_ptr<AutofillDialogView> view_;
+};
+
+#endif  // CHROME_BROWSER_UI_AUTOFILL_AUTOFILL_DIALOG_CONTROLLER_H_