Add Views implementation of ProfileSigninConfirmationDialog.

chrome/browser/ui/views/sync/profile_signin_confirmation_dialog_views.cc

Index: chrome/browser/ui/views/sync/profile_signin_confirmation_dialog_views.cc
diff --git a/chrome/browser/ui/views/sync/profile_signin_confirmation_dialog_views.cc b/chrome/browser/ui/views/sync/profile_signin_confirmation_dialog_views.cc
new file mode 100644
index 0000000000000000000000000000000000000000..5afc6809621f54cfeaec64749e82f6d9966100dd
--- /dev/null
+++ b/chrome/browser/ui/views/sync/profile_signin_confirmation_dialog_views.cc
@@ -0,0 +1,265 @@
+// Copyright 2013 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/sync/profile_signin_confirmation_dialog_views.h"
+
+#include "base/utf_string_conversions.h"
+#include "chrome/browser/ui/browser.h"
+#include "chrome/browser/ui/browser_dialogs.h"
+#include "chrome/browser/ui/browser_navigator.h"
+#include "chrome/browser/ui/browser_window.h"
+#include "chrome/browser/ui/host_desktop.h"
+#include "chrome/browser/ui/views/constrained_window_views.h"
+#include "chrome/browser/ui/web_contents_modal_dialog_manager.h"
+#include "chrome/browser/ui/web_contents_modal_dialog_manager_delegate.h"
+#include "content/public/browser/web_contents.h"
+#include "content/public/browser/web_contents_view.h"
+#include "google_apis/gaia/gaia_auth_util.h"
+#include "grit/chromium_strings.h"
+#include "grit/generated_resources.h"
+#include "third_party/skia/include/core/SkColor.h"
+#include "ui/base/l10n/l10n_util.h"
+#include "ui/base/range/range.h"
+#include "ui/gfx/font.h"
+#include "ui/gfx/native_widget_types.h"
+#include "ui/views/background.h"
+#include "ui/views/controls/label.h"
+#include "ui/views/controls/link.h"
+#include "ui/views/controls/styled_label.h"
+#include "ui/views/layout/box_layout.h"
+#include "ui/views/layout/grid_layout.h"
+#include "ui/views/layout/layout_constants.h"
+#include "ui/views/widget/widget.h"
+
+namespace {
+
+// Wrap a view in a fixed-width container.
+views::View* MakeFixedWidth(views::View* view, int width) {
+  views::View* container = new views::View;
+  views::GridLayout* layout = views::GridLayout::CreatePanel(container);
+  container->SetLayoutManager(layout);
+  layout->AddColumnSet(0)->AddColumn(
+      views::GridLayout::LEADING, views::GridLayout::CENTER, 0,
+      views::GridLayout::FIXED, width, false);
+  layout->StartRow(0, 0);
+  layout->AddView(view, 1, 1, views::GridLayout::FILL, views::GridLayout::FILL);
+  return container;
+}
+
+}  // namespace
+
+namespace chrome {
+// Declared in browser_dialogs.h
+void ShowProfileSigninConfirmationDialog(
+    Browser* browser,
+    content::WebContents* web_contents,
+    Profile* profile,
+    const std::string& username,
+    const base::Closure& cancel_signin,
+    const base::Closure& signin_with_new_profile,
+    const base::Closure& continue_signin) {
+  ProfileSigninConfirmationDialogViews* dialog =
+      new ProfileSigninConfirmationDialogViews(
+          browser,
+          profile,
+          username,
+          cancel_signin,
+          signin_with_new_profile,
+          continue_signin);
+  ui::CheckShouldPromptForNewProfile(
+      profile,
+      // This callback is guaranteed to be invoked, and once it is, the dialog
+      // owns itself.
+      base::Bind(&ProfileSigninConfirmationDialogViews::Show,
+                 base::Unretained(dialog)));
+}
+}  // namespace chrome
+
+ProfileSigninConfirmationDialogViews::ProfileSigninConfirmationDialogViews(
+    Browser* browser,
+    Profile* profile,
+    const std::string& username,
+    const base::Closure& cancel_signin,
+    const base::Closure& signin_with_new_profile,
+    const base::Closure& continue_signin)
+  : browser_(browser),
+    profile_(profile),
+    username_(username),
+    cancel_signin_(cancel_signin),
+    signin_with_new_profile_(signin_with_new_profile),
+    continue_signin_(continue_signin),
+    link_(NULL) {
+  // Layout the labels in a single fixed-width column.
+  SetLayoutManager(new views::BoxLayout(views::BoxLayout::kVertical, 0, 0, 0));
+  const int kDialogWidth = 440;

[Peter Kasting, 2013/05/15 00:11:03]

Nit: Declare just above first use.

[dconnelly, 2013/05/17 13:01:55]

Done.

+
+  // Create the prompt label.
+  std::vector<size_t> offsets;
+  const string16 domain = ASCIIToUTF16(gaia::ExtractDomainName(username_));
+  const string16 prompt_text =
+      l10n_util::GetStringFUTF16(
+          IDS_ENTERPRISE_SIGNIN_CREATE_NEW_PROFILE_PROMPT_NEW_STYLE,
+          ASCIIToUTF16(username_), domain, &offsets);
+  views::StyledLabel* prompt_label = new views::StyledLabel(prompt_text, this);
+  views::StyledLabel::RangeStyleInfo bold_style;
+  bold_style.font_style = gfx::Font::BOLD;
+  prompt_label->AddStyleRange(
+      ui::Range(offsets[1], offsets[1] + domain.size()), bold_style);
+
+  // Add the prompt label with a darker background and border.
+  views::View* prompt_container = MakeFixedWidth(prompt_label, kDialogWidth);

[Peter Kasting, 2013/05/15 00:11:03]

Why are these MakeFixedWidth() calls necessary?

[dconnelly, 2013/05/17 13:01:55]

Was trying to reduce the amount of code to write.  GridLayout::CreatePanel will
set up a container with the right margins and spacing for the "new dialog
style", and if that container has a fixed width specified then the StyledLabel
will automatically resize properly.  MakeFixedWidth does both of these.

I think to get rid of this I would need to use GridLayout for this entire view,
and manually set the margins for the relevant child labels.  I think that's not
necessarily more clear, and is more code, but if you think it's better then I
can do it.

[Peter Kasting, 2013/05/17 18:45:53]

I think using a GridLayout for everything is slightly better in the sense that
nested layout managers are somewhat difficult to understand.

I also think that the vertical padding in the screenshot looks too large -- e.g.
around the "account is managed by X" label.  I'm surprised this padding results
from using the "panel" constants, because supposedly kPanelVertMargin = 13, and
that looks like WAY more than 13 px from text to border.  Are we somehow getting
more padding in here than that?  (The horizontal padding looks way bigger than
13 px on each side, too.)

I ask this because with one layout for everything it's easier to use different
layout constants as padding to customize the within-dialog spacing of
everything.

[dconnelly, 2013/05/27 12:49:57]

After some investigation I don't think I can avoid using nested layout managers.
 The gray bar needs insets of zero with respect to the dialog view, the label it
contains needs positive insets, and the explanation text beneath the gray bar
needs positive insets.  GridLayout only supports insets for the entire container
that it lays out.  So I can replace the BoxLayout of the dialog view with a
GridLayout, but not much will change.  What do you think?

(Still waiting for feedback from UI on the paddings)

+  prompt_container->set_border(
+      views::Border::CreateSolidSidedBorder(
+          1, 0, 1, 0, SkColorSetRGB(0xE1, 0xE1, 0xE1)));

[Peter Kasting, 2013/05/15 00:11:03]

You can't hardcode colors.  Please use appropriate colors from the theme service
or other globally defined/flexible constants (even if this means you can't use
the precise color in whatever mocks you've been given).

As a last resort, add new colors to the theming system that are suitably general
for others to use as well.

(2 places)

[dconnelly, 2013/05/17 13:01:55]

"git grep SkColorSetRGB -- chrome/browser/ui" shows quite a few files that
define color constants for specific views.  Is that not an acceptable approach?

[Peter Kasting, 2013/05/17 18:45:53]

Not in general.  Some specific cases might be OK, e.g. if we have an image
resource and there is code that declares what the color of that resource is (but
even then it would be better to extract the color from the resource than to
hardcode).

+  prompt_container->set_background(
+      views::Background::CreateSolidBackground(
+          SkColorSetRGB(0xF6, 0xF6, 0xF6)));
+  AddChildView(prompt_container);

[Peter Kasting, 2013/05/15 00:11:03]

Normally we have views not create or add any children except in response to
being added themselves in ViewHierarchyChanged().  This prevents any possibility
of children leaking if the view never gets properly added to a hierarchy.

I believe doing this would also eliminate your need to have things like
UpdateExplanationLabel() since you could just take those actions directly while
creating the children.

[dconnelly, 2013/05/17 13:01:55]

Done.

+
+  // Create the explanation label.
+  explanation_label_ = new views::StyledLabel(string16(), this);
+
+  // Add the explanation label.
+  AddChildView(MakeFixedWidth(explanation_label_, kDialogWidth));
+}
+
+ProfileSigninConfirmationDialogViews::~ProfileSigninConfirmationDialogViews() {}
+
+void ProfileSigninConfirmationDialogViews::UpdateExplanationLabel(
+    ui::DisplayCreateProfilePrompt prompt_for_new_profile) {
+  const string16 domain = ASCIIToUTF16(gaia::ExtractDomainName(username_));
+  const string16 learn_more_text =
+      l10n_util::GetStringUTF16(
+          IDS_ENTERPRISE_SIGNIN_PROFILE_LINK_LEARN_MORE);
+  const string16 create_explanation_text =
+      l10n_util::GetStringUTF16(
+          IDS_ENTERPRISE_SIGNIN_PROFILE_LINK_CREATE_MESSAGE_NEW_STYLE);
+  const string16 signin_explanation_text =
+      l10n_util::GetStringFUTF16(
+          IDS_ENTERPRISE_SIGNIN_PROFILE_LINK_MESSAGE_NEW_STYLE,
+          domain);
+
+  // Setup the message text, measuring the offset for the embedded link.
+  string16 label_text = signin_explanation_text;
+  int link_offset = signin_explanation_text.size();
+  if (prompt_for_new_profile == ui::PROMPT_TO_CREATE_PROFILE) {
+    label_text += ASCIIToUTF16(" ") + create_explanation_text;

[Peter Kasting, 2013/05/15 00:11:03]

You can't paste together strings this way, it's not localizable.

You need to have one big string, or have a "wrapper string" that you substitute
other strings into, or something else that's entirely .grd-controlled and
doesn't involve manual concatenation.

[dconnelly, 2013/05/17 13:01:55]

Done.

+    link_offset += 1 + create_explanation_text.size();
+  }
+  label_text += ASCIIToUTF16(" ") + learn_more_text;
+  link_offset += 1;
+
+  // Add the message text and linkify the "Learn More"
+  explanation_label_->SetText(label_text);
+  views::StyledLabel::RangeStyleInfo link_style =
+      views::StyledLabel::RangeStyleInfo::CreateForLink();
+  link_style.font_style = gfx::Font::NORMAL;
+  explanation_label_->AddStyleRange(
+      ui::Range(link_offset,
+                link_offset + learn_more_text.size()),
+                link_style);
+}
+
+void ProfileSigninConfirmationDialogViews::UpdateCreateProfileLink(

[Peter Kasting, 2013/05/15 00:11:03]

Seems like we could just do all this in CreateExtraView() directly.

[dconnelly, 2013/05/17 13:01:55]

Done.

+    ui::DisplayCreateProfilePrompt prompt_for_new_profile) {
+  if (prompt_for_new_profile == ui::PROMPT_TO_CREATE_PROFILE) {
+    const string16 create_profile_text =
+        l10n_util::GetStringUTF16(
+            IDS_ENTERPRISE_SIGNIN_CREATE_NEW_PROFILE_YES_NEW_STYLE);
+    link_ = new views::Link(create_profile_text);
+    link_->SetUnderline(false);
+    link_->set_listener(this);
+    link_->SetHorizontalAlignment(gfx::ALIGN_LEFT);
+  }
+}
+
+void ProfileSigninConfirmationDialogViews::Show(
+    ui::DisplayCreateProfilePrompt prompt_for_new_profile) {
+  UpdateExplanationLabel(prompt_for_new_profile);
+  UpdateCreateProfileLink(prompt_for_new_profile);
+  CreateDialogWidget(this, NULL, browser_->window()->GetNativeWindow())->Show();
+}
+
+string16 ProfileSigninConfirmationDialogViews::GetWindowTitle() const {
+  return l10n_util::GetStringUTF16(
+      IDS_ENTERPRISE_SIGNIN_PROFILE_LINK_DIALOG_TITLE_NEW_STYLE);
+}
+
+int ProfileSigninConfirmationDialogViews::GetDialogButtons() const {
+  return ui::DIALOG_BUTTON_OK | ui::DIALOG_BUTTON_CANCEL;

[Peter Kasting, 2013/05/15 00:11:03]

Isn't this just what the base class method already does?

[dconnelly, 2013/05/17 13:01:55]

Done.

+}
+
+string16 ProfileSigninConfirmationDialogViews::GetDialogButtonLabel(
+    ui::DialogButton button) const {
+  switch (button) {

[Peter Kasting, 2013/05/15 00:11:03]

Nit: Simpler and more readable:

  return l10n_util::GetStringUTF16((button == ui::DIALOG_BUTTON_OK) ?
      IDS_ENTERPRISE_SIGNIN_CREATE_NEW_PROFILE_NO_NEW_STYLE :
      IDS_ENTERPRISE_SIGNIN_CREATE_NEW_PROFILE_CANCEL);

[dconnelly, 2013/05/17 13:01:55]

Done.

+    case ui::DIALOG_BUTTON_OK:
+      return l10n_util::GetStringUTF16(
+          IDS_ENTERPRISE_SIGNIN_CREATE_NEW_PROFILE_NO_NEW_STYLE);
+    case ui::DIALOG_BUTTON_CANCEL:
+      return l10n_util::GetStringUTF16(
+          IDS_ENTERPRISE_SIGNIN_CREATE_NEW_PROFILE_CANCEL);
+    default:
+      NOTREACHED();
+      return DialogDelegateView::GetDialogButtonLabel(button);
+  }
+}
+
+views::View* ProfileSigninConfirmationDialogViews::CreateExtraView() {
+  return link_;
+}
+
+void ProfileSigninConfirmationDialogViews::DeleteDelegate() {
+  delete this;

[Peter Kasting, 2013/05/15 00:11:03]

Isn't this just what the base class method already does?

[dconnelly, 2013/05/17 13:01:55]

Done.

+}
+
+bool ProfileSigninConfirmationDialogViews::Accept() {
+  if (!continue_signin_.is_null()) {
+    continue_signin_.Run();
+    ResetCallbacks();
+  }
+  return true;
+}
+
+bool ProfileSigninConfirmationDialogViews::Cancel() {
+  if (!cancel_signin_.is_null()) {
+    cancel_signin_.Run();
+    ResetCallbacks();
+  }
+  return true;
+}
+
+void ProfileSigninConfirmationDialogViews::OnClose() {
+  Cancel();
+}
+
+ui::ModalType ProfileSigninConfirmationDialogViews::GetModalType() const {
+  return ui::MODAL_TYPE_WINDOW;
+}
+
+void ProfileSigninConfirmationDialogViews::LinkClicked(views::Link* source,
+                                                       int event_flags) {
+  if (!signin_with_new_profile_.is_null()) {
+    signin_with_new_profile_.Run();
+    ResetCallbacks();
+  }
+  GetWidget()->Close();
+}
+
+void ProfileSigninConfirmationDialogViews::StyledLabelLinkClicked(
+    const ui::Range& range,
+    int event_flags) {
+  const std::string url =
+      "http://support.google.com/chromeos/bin/answer.py?hl=en&answer=1331549";

[Peter Kasting, 2013/05/15 00:11:03]

Nit: Just inline this directly.

[dconnelly, 2013/05/17 13:01:55]

Done.

+  chrome::NavigateParams params(
+      browser_, GURL(url), content::PAGE_TRANSITION_AUTO_TOPLEVEL);

[Peter Kasting, 2013/05/15 00:11:03]

Why AUTO_TOPLEVEL?  Why is this not just a LINK?

[dconnelly, 2013/05/17 13:01:55]

From the descriptions of the transition types:

// This is a toplevel navigation. This is any content that is automatically
// loaded in a toplevel frame.  For example, opening a tab to show the ASH
// screen saver, opening the devtools window, opening the NTP after the safe
// browsing warning, opening web-based dialog boxes are examples of
// AUTO_TOPLEVEL navigations.

[Peter Kasting, 2013/05/17 18:45:53]

But this content isn't automatically loaded, is the point.  This is explicitly
loaded based on a user's click of a link.

AUTO_TOPLEVEL is for when we just open something without a user having taken any
action, or in response to doing something very unlike clicking a link (e.g.
pressing "OK" in a dialog box).

+  params.disposition = NEW_POPUP;
+  params.window_action = chrome::NavigateParams::SHOW_WINDOW;
+  chrome::Navigate(&params);
+}
+
+void ProfileSigninConfirmationDialogViews::ResetCallbacks() {
+  cancel_signin_.Reset();
+  continue_signin_.Reset();
+  signin_with_new_profile_.Reset();
+}