Win: Display a native bubble (instead of the JS one) after the web signin flow.

chrome/browser/ui/views/sync/one_click_signin_bubble_view.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/sync/one_click_signin_bubble_view.h"
 
 #include "base/callback_helpers.h"
 #include "base/logging.h"
 #include "base/message_loop.h"
 #include "base/utf_string_conversions.h"
@@ skipping 10 matching lines @@
 #include "ui/base/resource/resource_bundle.h"
 #include "ui/views/controls/button/image_button.h"
 #include "ui/views/controls/button/label_button.h"
 #include "ui/views/controls/image_view.h"
 #include "ui/views/controls/label.h"
 #include "ui/views/controls/link.h"
 #include "ui/views/layout/grid_layout.h"
 #include "ui/views/layout/layout_constants.h"
 #include "ui/views/widget/widget.h"
 
-// Minimum width for the mutli-line label.
+// Minimum width for the multi-line label.
 const int kMinimumDialogLabelWidth = 400;
 const int kMinimumLabelWidth = 240;
 const int kDialogMargin = 16;
 
 namespace {
 
 // The column set constants that can be used in the InitContent() function
 // to layout views.
 enum OneClickSigninBubbleColumnTypes {
   COLUMN_SET_FILL_ALIGN,
   COLUMN_SET_CONTROLS,
   COLUMN_SET_TITLE_BAR
 };
 
 class OneClickSigninDialogView : public OneClickSigninBubbleView {
  public:
   OneClickSigninDialogView(
       content::WebContents* web_content,
       views::View* anchor_view,
       const string16& email,
       const BrowserWindow::StartSyncCallback& start_sync_callback);
 
  private:
   // Overridden from views::WidgetDelegate:
   virtual ui::ModalType GetModalType() const OVERRIDE;
 
   // Overridden from OneClickSigninBubbleView:
   virtual void InitContent(views::GridLayout* layout) OVERRIDE;
+
+  virtual void InitButtons(views::GridLayout* layout) OVERRIDE;
   virtual void GetButtons(views::LabelButton** ok_button,
                           views::LabelButton** undo_button) OVERRIDE;
-  virtual views::Link* GetAdvancedLink() OVERRIDE;
+
+  // Overridden from views::ButtonListener:
+  virtual void ButtonPressed(views::Button* sender,
+                             const ui::Event& event) OVERRIDE;
 
   // Overridden from views::LinkListener:
   virtual void LinkClicked(views::Link* source, int event_flags) OVERRIDE;
 
+  // views::View method:
+  virtual bool AcceleratorPressed(const ui::Accelerator& accelerator) OVERRIDE;
+
+  // views::WidgetDelegate method:
+  virtual void WindowClosing() OVERRIDE;
+
   const string16 email_;
-  content::WebContents* web_content_;
-  views::Link* learn_more_link_;
+
+  // This callback is nulled once its called, so that it is called only once.
+  // It will be called when the bubble is closed if it has not been called
+  // and nulled earlier.
+  BrowserWindow::StartSyncCallback start_sync_callback_;
+
   views::ImageButton* close_button_;
 
   DISALLOW_COPY_AND_ASSIGN(OneClickSigninDialogView);
 };
 
 OneClickSigninDialogView::OneClickSigninDialogView(
     content::WebContents* web_content,
     views::View* anchor_view,
     const string16& email,
     const BrowserWindow::StartSyncCallback& start_sync_callback)
-    : OneClickSigninBubbleView(anchor_view, start_sync_callback),
+    : OneClickSigninBubbleView(web_content, anchor_view, std::string()),
       email_(email),
-      web_content_(web_content),
-      learn_more_link_(NULL),
-      close_button_(NULL) {
+      close_button_(NULL),
+      start_sync_callback_(start_sync_callback) {
+  DCHECK(!start_sync_callback_.is_null());
   set_arrow(views::BubbleBorder::NONE);
   set_anchor_view_insets(gfx::Insets(0, 0, anchor_view->height() / 2, 0));
   set_close_on_deactivate(false);
   set_margins(gfx::Insets(kDialogMargin, kDialogMargin, kDialogMargin,
                           kDialogMargin));
 }
 
 ui::ModalType OneClickSigninDialogView::GetModalType() const {
   return ui::MODAL_TYPE_CHILD;
 }
@@ skipping 20 matching lines @@
     label->SetFont(label->font().DeriveFont(3, gfx::Font::BOLD));
     layout->AddView(label);
 
     close_button_ = new views::ImageButton(this);
     close_button_->SetImage(views::ImageButton::STATE_NORMAL,
                             rb.GetImageNamed(IDR_CLOSE_2).ToImageSkia());
     close_button_->SetImage(views::ImageButton::STATE_HOVERED,
                             rb.GetImageNamed(IDR_CLOSE_2_H).ToImageSkia());
     close_button_->SetImage(views::ImageButton::STATE_PRESSED,
                             rb.GetImageNamed(IDR_CLOSE_2_P).ToImageSkia());
+
     layout->AddView(close_button_);
   }
 
   layout->AddPaddingRow(0, views::kUnrelatedControlVerticalSpacing);
 
   {
     layout->StartRow(0, COLUMN_SET_FILL_ALIGN);
 
     views::Label* label = new views::Label(email_.empty() ?
         l10n_util::GetStringUTF16(IDS_ONE_CLICK_SIGNIN_DIALOG_MESSAGE) :
         l10n_util::GetStringFUTF16(IDS_ONE_CLICK_SIGNIN_DIALOG_MESSAGE_NEW,
                                    email_));
     label->SetMultiLine(true);
     label->SetHorizontalAlignment(gfx::ALIGN_LEFT);
     label->SizeToFit(kMinimumDialogLabelWidth);
     layout->AddView(label);
 
     layout->StartRow(0, COLUMN_SET_FILL_ALIGN);
 
-    learn_more_link_ = new views::Link(
-        l10n_util::GetStringUTF16(IDS_LEARN_MORE));
-    learn_more_link_->set_listener(this);
-    learn_more_link_->SetHorizontalAlignment(gfx::ALIGN_LEFT);
+    learn_more_link_ = GetLearnMoreLink();
     layout->AddView(learn_more_link_, 1, 1, views::GridLayout::TRAILING,
                     views::GridLayout::CENTER);
   }
 
   layout->AddPaddingRow(0, views::kUnrelatedControlVerticalSpacing);
 }
 
+void OneClickSigninDialogView::InitButtons(views::GridLayout* layout) {
+  GetButtons(&ok_button_, &undo_button_);
+  layout->AddView(ok_button_);
+  layout->AddView(undo_button_);
+}
+
 void OneClickSigninDialogView::GetButtons(views::LabelButton** ok_button,
                                           views::LabelButton** undo_button) {
   *ok_button = new views::LabelButton(this, string16());
   (*ok_button)->SetStyle(views::Button::STYLE_NATIVE_TEXTBUTTON);
 
   *undo_button = new views::LabelButton(this, string16());
   (*undo_button)->SetStyle(views::Button::STYLE_NATIVE_TEXTBUTTON);
 
   // The default size of the buttons is too large.  To allow them to be smaller
   // ignore the minimum default size.  Furthermore, to make sure they are the
   // same size, SetText() is called with both strings on both buttons.
   (*ok_button)->set_min_size(gfx::Size());
   (*undo_button)->set_min_size(gfx::Size());
   string16 ok_label =
       l10n_util::GetStringUTF16(IDS_ONE_CLICK_SIGNIN_DIALOG_OK_BUTTON);
   string16 undo_label =
       l10n_util::GetStringUTF16(IDS_ONE_CLICK_SIGNIN_DIALOG_UNDO_BUTTON);
   (*ok_button)->SetText(undo_label);
   (*ok_button)->SetText(ok_label);
   (*undo_button)->SetText(ok_label);
   (*undo_button)->SetText(undo_label);
 }
 
-views::Link* OneClickSigninDialogView::GetAdvancedLink() {
-  views::Link* advanced_link= new views::Link(
-      l10n_util::GetStringUTF16(IDS_ONE_CLICK_SIGNIN_DIALOG_ADVANCED));
-  advanced_link->set_listener(this);
-  advanced_link->SetHorizontalAlignment(gfx::ALIGN_LEFT);
-  return advanced_link;
+void OneClickSigninDialogView::ButtonPressed(views::Button* sender,
+                                             const ui::Event& event) {
+  OneClickSigninBubbleView::ButtonPressed(sender, event);
+
+  base::ResetAndReturn(&start_sync_callback_).Run((sender == ok_button_) ?
+      OneClickSigninSyncStarter::SYNC_WITH_DEFAULT_SETTINGS :
+      OneClickSigninSyncStarter::UNDO_SYNC);
 }
 
 void OneClickSigninDialogView::LinkClicked(views::Link* source,
                                            int event_flags) {
   if (source == learn_more_link_) {
     content::OpenURLParams params(
         GURL(chrome::kChromeSyncLearnMoreURL), content::Referrer(),
         NEW_WINDOW, content::PAGE_TRANSITION_LINK, false);
     web_content_->OpenURL(params);

[Roger Tawa OOO till Jul 10th, 2013/04/17 17:53:25]

This is duplicated in the base class.  I wonder if this can be refactored
somehow.  What if there was a method in the base class like HandleLinkClicked(),
which returned something to say if the learn more was clicked or not?  Then this
code could still invoke the callback only when needed.

     return;
   }
 
-  OneClickSigninBubbleView::LinkClicked(source, event_flags);
+  base::ResetAndReturn(&start_sync_callback_).Run(
+      OneClickSigninSyncStarter::CONFIGURE_SYNC_FIRST);
+
+  OneClickSigninBubbleView::Hide();
 }
 
+bool OneClickSigninDialogView::AcceleratorPressed(
+  const ui::Accelerator& accelerator) {
+  bool isAcceleratorPressed =

[Roger Tawa OOO till Jul 10th, 2013/04/17 17:53:25]

variable naming should be:

is_accelerator_pressed

[noms (inactive), 2013/04/17 20:32:37]

Done.

+          OneClickSigninBubbleView::AcceleratorPressed(accelerator);
+
+  if (accelerator.key_code() == ui::VKEY_RETURN) {
+      base::ResetAndReturn(&start_sync_callback_).Run(
+          OneClickSigninSyncStarter::SYNC_WITH_DEFAULT_SETTINGS);
+  } else {
+    start_sync_callback_.Reset();
+  }
+
+  return isAcceleratorPressed;
+}
+
+void OneClickSigninDialogView::WindowClosing() {
+  OneClickSigninBubbleView::WindowClosing();
+
+  if (!start_sync_callback_.is_null()) {
+    base::ResetAndReturn(&start_sync_callback_).Run(
+        OneClickSigninSyncStarter::SYNC_WITH_DEFAULT_SETTINGS);
+  }
+}
 }  // namespace
 
 // OneClickSigninBubbleView ----------------------------------------------------
 
 // static
 OneClickSigninBubbleView* OneClickSigninBubbleView::bubble_view_ = NULL;
 
 // static
 void OneClickSigninBubbleView::ShowBubble(
     BrowserWindow::OneClickSigninBubbleType type,
     const string16& email,
     ToolbarView* toolbar_view,
-    const BrowserWindow::StartSyncCallback& start_sync) {
+    const BrowserWindow::StartSyncCallback& start_sync,
+    const std::string& error_message) {
   if (IsShowing())
     return;
 
   switch (type) {
     case BrowserWindow::ONE_CLICK_SIGNIN_BUBBLE_TYPE_BUBBLE:
-      bubble_view_ = new OneClickSigninBubbleView(toolbar_view->app_menu(),
-                                                  start_sync);
+      bubble_view_ = new OneClickSigninBubbleView(
+        toolbar_view->GetWebContents(),

[Roger Tawa OOO till Jul 10th, 2013/04/17 17:53:25]

I'm concerned about storing a contents pointer as a member of the bubble view
class.  I suspect it could be possible for the bubble to outlive the contents. 
This does not concern me too much with the dialogs since they are modal, but it
*may* be a problem there too.

The contents is used to show the a tab when the user clicks on a link in the
bubble or dialog.  I wonder if it would be better to save a pointer the Browser
object instead (which will certainly outlive the bubble or dialog) and then if a
link is clicked the code opens a new tab instead via Browser::OpenURL() ?

[noms (inactive), 2013/04/17 20:32:37]

Added a comment that both the dialog and the bubble outlive the tab (the bubble
registers the click and closes, and the tab is modal)

On 2013/04/17 17:53:25, Roger Tawa wrote:

+        toolbar_view->app_menu(), error_message);

[Roger Tawa OOO till Jul 10th, 2013/04/17 17:53:25]

ident lines 265 and 266 by 2 more.

[noms (inactive), 2013/04/17 20:32:37]

Done.

       break;
     case BrowserWindow::ONE_CLICK_SIGNIN_BUBBLE_TYPE_MODAL_DIALOG:
       bubble_view_ = new OneClickSigninDialogView(
           toolbar_view->GetWebContents(), toolbar_view->location_bar(),
           string16(), start_sync);
       break;
     case BrowserWindow::ONE_CLICK_SIGNIN_BUBBLE_TYPE_SAML_MODAL_DIALOG:
       bubble_view_ = new OneClickSigninDialogView(
           toolbar_view->GetWebContents(), toolbar_view->location_bar(),
           email, start_sync);
       break;
   }
 
   views::BubbleDelegateView::CreateBubble(bubble_view_)->Show();
 }
 
 // static
 bool OneClickSigninBubbleView::IsShowing() {
   return bubble_view_ != NULL;
 }
 
 // static
 void OneClickSigninBubbleView::Hide() {
   if (IsShowing())
     bubble_view_->GetWidget()->Close();
 }
 
 OneClickSigninBubbleView::OneClickSigninBubbleView(
+  content::WebContents* web_content,

[Roger Tawa OOO till Jul 10th, 2013/04/17 17:53:25]

indent 2 more.

[noms (inactive), 2013/04/17 20:32:37]

Done.

     views::View* anchor_view,
-    const BrowserWindow::StartSyncCallback& start_sync_callback)
+    const std::string& error_message)
     : BubbleDelegateView(anchor_view, views::BubbleBorder::TOP_RIGHT),
+      web_content_(web_content),
+      error_message_(error_message),
       advanced_link_(NULL),
+      learn_more_link_(NULL),
       ok_button_(NULL),
       undo_button_(NULL),
-      start_sync_callback_(start_sync_callback),
       message_loop_for_testing_(NULL) {
-  DCHECK(!start_sync_callback_.is_null());
 }
 
 OneClickSigninBubbleView::~OneClickSigninBubbleView() {
 }
 
 void OneClickSigninBubbleView::AnimationEnded(const ui::Animation* animation) {
   views::BubbleDelegateView::AnimationEnded(animation);
   if (message_loop_for_testing_)
     message_loop_for_testing_->Quit();
 }
@@ skipping 16 matching lines @@
   cs->AddColumn(views::GridLayout::TRAILING, views::GridLayout::CENTER, 0,
                 views::GridLayout::USE_PREF, 0, 0);
   cs->AddPaddingColumn(0, views::kRelatedButtonHSpacing);
   cs->AddColumn(views::GridLayout::TRAILING, views::GridLayout::CENTER, 0,
                 views::GridLayout::USE_PREF, 0, 0);
 
   InitContent(layout);
 
   // Add controls at the bottom.
   advanced_link_= GetAdvancedLink();
-  GetButtons(&ok_button_, &undo_button_);
-  ok_button_->SetIsDefault(true);
 
   layout->StartRow(0, COLUMN_SET_CONTROLS);
   layout->AddView(advanced_link_);
-  layout->AddView(ok_button_);
-  layout->AddView(undo_button_);
+
+  InitButtons(layout);
+  ok_button_->SetIsDefault(true);
 
   AddAccelerator(ui::Accelerator(ui::VKEY_RETURN, 0));
 }
 
 void OneClickSigninBubbleView::InitContent(views::GridLayout* layout) {
   // Add main text description.
-  views::Label* label = new views::Label(
-      l10n_util::GetStringUTF16(IDS_ONE_CLICK_SIGNIN_BUBBLE_MESSAGE));
+  layout->StartRow(0, COLUMN_SET_FILL_ALIGN);
+
+  views::Label* label = !error_message_.empty() ?
+      new views::Label(UTF8ToUTF16(error_message_)) :
+      new views::Label(
+        l10n_util::GetStringUTF16(IDS_ONE_CLICK_SIGNIN_BUBBLE_MESSAGE));

[Roger Tawa OOO till Jul 10th, 2013/04/17 17:53:25]

indent 2 more.

[noms (inactive), 2013/04/17 20:32:37]

Done.

+
   label->SetMultiLine(true);
   label->SetHorizontalAlignment(gfx::ALIGN_LEFT);
   label->SizeToFit(kMinimumLabelWidth);
-
-  layout->StartRow(0, COLUMN_SET_FILL_ALIGN);
   layout->AddView(label);
 
-  layout->AddPaddingRow(0, views::kRelatedControlSmallVerticalSpacing);
+  layout->StartRow(0, COLUMN_SET_CONTROLS);
+
+  // and learn more link
+  learn_more_link_ = GetLearnMoreLink();
+
+  layout->AddView(learn_more_link_);
+}
+
+void OneClickSigninBubbleView::InitButtons(views::GridLayout* layout) {
+  GetButtons(&ok_button_, &undo_button_);
+  layout->AddView(ok_button_);

[Roger Tawa OOO till Jul 10th, 2013/04/17 17:53:25]

I'm not sure InitButtons() needs to be virtual or reimplemented/overridden in
the derived class.  If its implemented only in the base class, and it checks the
returned pointers for null before adding them to the layout, I think it can be
made generic.

Also, since the bubble no longer needs an undo button (only the dialog does)
then probably need to be creating it anyway.  Might be able to remove a lot of
code in OneClickSigninBubbleView::GetButtons().

[noms (inactive), 2013/04/17 20:32:37]

The two InitButtons functions got merged, so this isn't an issue anymre

On 2013/04/17 17:53:25, Roger Tawa wrote:

 }
 
 void OneClickSigninBubbleView::GetButtons(views::LabelButton** ok_button,
                                           views::LabelButton** undo_button) {
   *ok_button = new views::LabelButton(this, string16());
   (*ok_button)->SetStyle(views::Button::STYLE_NATIVE_TEXTBUTTON);
 
   *undo_button = new views::LabelButton(this, string16());
   (*undo_button)->SetStyle(views::Button::STYLE_NATIVE_TEXTBUTTON);
 
   // The default size of the buttons is too large.  To allow them to be smaller
   // ignore the minimum default size.  Furthermore, to make sure they are the
   // same size, SetText() is called with both strings on both buttons.
   (*ok_button)->set_min_size(gfx::Size());
   (*undo_button)->set_min_size(gfx::Size());
+
   string16 ok_label = l10n_util::GetStringUTF16(IDS_OK);
-  string16 undo_label = l10n_util::GetStringUTF16(IDS_ONE_CLICK_BUBBLE_UNDO);
+  string16 undo_label =
+      l10n_util::GetStringUTF16(IDS_ONE_CLICK_SIGNIN_DIALOG_UNDO_BUTTON);

[Roger Tawa OOO till Jul 10th, 2013/04/17 17:53:25]

Just want to double check this is the correct string.  The IDS contains the word
"DIALOG", but this is not the dialog implementation.  Same for line 405 below.

[noms (inactive), 2013/04/17 20:32:37]

Done.

+
   (*ok_button)->SetText(undo_label);
   (*ok_button)->SetText(ok_label);
   (*undo_button)->SetText(ok_label);
   (*undo_button)->SetText(undo_label);
 }
 
 views::Link* OneClickSigninBubbleView::GetAdvancedLink() {
-  views::Link* advanced_link= new views::Link(
-      l10n_util::GetStringUTF16(IDS_SYNC_PROMO_NTP_BUBBLE_ADVANCED));
+  views::Link* advanced_link = new views::Link(
+      l10n_util::GetStringUTF16(IDS_ONE_CLICK_SIGNIN_DIALOG_ADVANCED));
   advanced_link->set_listener(this);
   advanced_link->SetHorizontalAlignment(gfx::ALIGN_LEFT);
   return advanced_link;
 }
 
-void OneClickSigninBubbleView::WindowClosing() {
-  // We have to reset |bubble_view_| here, not in our destructor, because
-  // we'll be destroyed asynchronously and the shown state will be checked
-  // before then.
-  DCHECK_EQ(bubble_view_, this);
-  bubble_view_ = NULL;
+views::Link* OneClickSigninBubbleView::GetLearnMoreLink() {
+  views::Link* learn_more_link_ = new views::Link(
+        l10n_util::GetStringUTF16(IDS_LEARN_MORE));

[Roger Tawa OOO till Jul 10th, 2013/04/17 17:53:25]

indent 2 less.

[noms (inactive), 2013/04/17 20:32:37]

Done.

+  learn_more_link_->set_listener(this);
+  learn_more_link_->SetHorizontalAlignment(gfx::ALIGN_LEFT);
 
-  if (!start_sync_callback_.is_null()) {
-    base::ResetAndReturn(&start_sync_callback_).Run(
-        OneClickSigninSyncStarter::SYNC_WITH_DEFAULT_SETTINGS);
-  }
+  return learn_more_link_;

[Roger Tawa OOO till Jul 10th, 2013/04/17 17:53:25]

Local var should not have a trailing _ in name.

However, if this method were renamed to something like CreateLearnMoreLink(), it
could the member variable as well as return the pointer.  Then you can make this
member variable private.

[noms (inactive), 2013/04/17 20:32:37]

Made private, renamed it to InitXXXLink and removed the return type, to be
consistent with InitContent/InitButtons etc.

On 2013/04/17 17:53:25, Roger Tawa wrote:

 }
 
 bool OneClickSigninBubbleView::AcceleratorPressed(
-    const ui::Accelerator& accelerator) {
+  const ui::Accelerator& accelerator) {
   if (accelerator.key_code() == ui::VKEY_RETURN ||
       accelerator.key_code() == ui::VKEY_ESCAPE) {
-    StartFade(false);
-    if (accelerator.key_code() == ui::VKEY_RETURN) {
-      base::ResetAndReturn(&start_sync_callback_).Run(
-          OneClickSigninSyncStarter::SYNC_WITH_DEFAULT_SETTINGS);
-    } else {
-      start_sync_callback_.Reset();
-    }
-
+    OneClickSigninBubbleView::Hide();

[Roger Tawa OOO till Jul 10th, 2013/04/17 17:53:25]

I don't think you need the class name here.  Same at line 448 below.

[noms (inactive), 2013/04/17 20:32:37]

Done.

     return true;
   }
 
   return BubbleDelegateView::AcceleratorPressed(accelerator);
 }
 
 void OneClickSigninBubbleView::LinkClicked(views::Link* source,
                                            int event_flags) {
-  StartFade(false);
-  base::ResetAndReturn(&start_sync_callback_).Run(
-      OneClickSigninSyncStarter::CONFIGURE_SYNC_FIRST);
+  if (source == learn_more_link_) {
+    content::OpenURLParams params(
+        GURL(chrome::kChromeSyncLearnMoreURL), content::Referrer(),
+        NEW_FOREGROUND_TAB, content::PAGE_TRANSITION_LINK, false);
+    web_content_->OpenURL(params);
+  } else if (source == advanced_link_) {
+    content::OpenURLParams params(
+        GURL(chrome::kChromeUISettingsURL), content::Referrer(),
+        CURRENT_TAB, content::PAGE_TRANSITION_LINK, false);
+    web_content_->OpenURL(params);
+  }
 }
 
 void OneClickSigninBubbleView::ButtonPressed(views::Button* sender,
                                              const ui::Event& event) {
-  StartFade(false);
-  base::ResetAndReturn(&start_sync_callback_).Run((sender == ok_button_) ?
-      OneClickSigninSyncStarter::SYNC_WITH_DEFAULT_SETTINGS :
-      OneClickSigninSyncStarter::UNDO_SYNC);
+  OneClickSigninBubbleView::Hide();
 }
+
+void OneClickSigninBubbleView::WindowClosing() {
+  // We have to reset |bubble_view_| here, not in our destructor, because
+  // we'll be destroyed asynchronously and the shown state will be checked
+  // before then.
+  DCHECK_EQ(bubble_view_, this);
+  bubble_view_ = NULL;
+}