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

chrome/browser/ui/views/autofill/autofill_dialog_views.cc

+// 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.
+
+#include "chrome/browser/ui/views/autofill/autofill_dialog_views.h"
+
+#include "base/utf_string_conversions.h"
+#include "chrome/browser/ui/views/constrained_window_views.h"
+#include "ui/views/controls/label.h"
+
+// static
+AutofillDialogView* AutofillDialogView::CreateDialog(
+    AutofillDialogController* controller) {
+  return new AutofillDialogViews(controller);
+}
+
+AutofillDialogViews::AutofillDialogViews(AutofillDialogController* controller)
+    : controller_(controller),
+      window_(NULL),
+      contents_(NULL) {

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

nit: DCHECK(controller)?

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

Done.

+}
+
+AutofillDialogViews::~AutofillDialogViews() {}
+
+void AutofillDialogViews::Show() {
+  InitChildViews();
+
+  // Ownership of |contents_| is handed off by this call. |window_| will take
+  // care of deleting itself after calling DeleteDelegate().

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

How does this pass ownership of |contents_|?  |contents_| isn't one of the
parameters to this method, is it?

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

Is DeleteDelegate() guaranteed to be called in all cases?  The memory management
of all this code seems way too involved :(

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

because constrainedwindowviews calls GetContentsView() and then owns the result.

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

yes it is, and this memory management model is common to all constrained
windows. The fact that it's not the most straightforward code ever is why there
is documentation here.

+  window_ = new ConstrainedWindowViews(
+      controller_->web_contents(), this,
+      true, ConstrainedWindowViews::DEFAULT_INSETS);
+}
+
+string16 AutofillDialogViews::GetWindowTitle() const {
+  return controller_->DialogTitle();
+}
+
+void AutofillDialogViews::DeleteDelegate() {

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

Should window_ be set to |null|? (Perhaps even earlier, as part of
WindowClosing()?)

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

what is WindowClosing? Actually it seems we don't even need window_, so I've
removed it.

+  // |this| belongs to |controller_|.
+  controller_->ViewClosed(false);
+}
+
+views::Widget* AutofillDialogViews::GetWidget() {
+  return contents_->GetWidget();
+}
+
+const views::Widget* AutofillDialogViews::GetWidget() const {
+  return contents_->GetWidget();
+}
+
+views::View* AutofillDialogViews::GetContentsView() {
+  return contents_;
+}
+
+string16 AutofillDialogViews::GetDialogButtonLabel(ui::DialogButton button)
+    const {
+  return button == ui::DIALOG_BUTTON_OK ?
+      controller_->ConfirmButtonText() : controller_->CancelButtonText();
+}
+
+bool AutofillDialogViews::IsDialogButtonEnabled(ui::DialogButton button) const {
+  return button == ui::DIALOG_BUTTON_OK ?
+      controller_->ConfirmButtonEnabled() : true;
+}
+
+bool AutofillDialogViews::UseChromeStyle() const {
+  return true;
+}
+
+bool AutofillDialogViews::Cancel() {
+  return true;
+}
+
+bool AutofillDialogViews::Accept() {
+  NOTREACHED();

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

nit: Please add TODOs to implement this and other methods in this class.

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

I'm not sure a TODO in every function that is not in its final form is very
useful.

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

I think it's likely to be useful when some of the functions are fully
implemented, whereas others still need to be finished -- especially since the
class design will likely evolve somewhat, resulting in some functions that
should actually be deleted rather than implemented.  Up to you, but I think
including some sort of documentation indicating that the code isn't finished is
helpful for keeping track of what needs to be done/cleaned up as the code
evolves.

+  return true;
+}
+
+void AutofillDialogViews::InitChildViews() {
+  contents_ = new views::View();

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

It looks like this can leak if Show() is never called.  Is there something I'm
missing?

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

This function was supposed to be private and only called from Show(). Fixed.

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

Ah, ok, that makes more sense.  Since it's only two lines and relies on the rest
of Show() to take ownership of the allocated memory, I think it might be simpler
to just inline this code in Show(), rather than having the extra function.

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

right now it is two lines. In the future it will be many lines.

+  contents_->AddChildView(new views::Label(ASCIIToUTF16("Hello, world.")));

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

Does this pass ownership of the child view?  It doesn't really look to me like
it does...

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

yes

+}