first run bubble using the views/bubble api.

chrome/browser/ui/views/first_run_bubble.cc

Index: chrome/browser/ui/views/first_run_bubble.cc
diff --git a/chrome/browser/ui/views/first_run_bubble.cc b/chrome/browser/ui/views/first_run_bubble.cc
index 349257a66e8f143a00a279ded5b6de2ad5e62db1..77871700130b359ee69400dc705fb125c54193db 100644
--- a/chrome/browser/ui/views/first_run_bubble.cc
+++ b/chrome/browser/ui/views/first_run_bubble.cc
@@ -24,8 +24,11 @@
 #include "views/controls/label.h"
 #include "views/events/event.h"
 #include "views/focus/focus_manager.h"
+#include "views/layout/fill_layout.h"
 #include "views/layout/layout_constants.h"
+#if defined(OS_WIN) && !defined(USE_AURA)
 #include "views/widget/native_widget_win.h"
+#endif
 #include "views/widget/widget.h"
 
 namespace {
@@ -38,22 +41,9 @@ const int kBubblePadding = 4;
 // provides in alternative OEM bubble.
 const int kOEMBubblePadding = 4;
 
-// Padding between parts of strings on the same line (for instance,
-// "New!" and "Search from the address bar!"
-const int kStringSeparationPadding = 2;
-
 // Margin around close button.
 const int kMarginRightOfCloseButton = 7;
 
-// The bubble's HWND is actually owned by the border widget, and it's the border
-// widget that's owned by the frame window the bubble is anchored to. This
-// function makes the two leaps necessary to go from the bubble contents HWND
-// to the frame HWND.
-HWND GetLogicalBubbleOwner(HWND bubble_hwnd) {
-  HWND border_widget_hwnd = GetWindow(bubble_hwnd, GW_OWNER);
-  return GetWindow(border_widget_hwnd, GW_OWNER);
-}
-
 }  // namespace
 
 // Base class for implementations of the client view which appears inside the
@@ -65,54 +55,69 @@ class FirstRunBubbleViewBase : public views::View,
   // Called by FirstRunBubble::Show to request focus for the proper button
   // in the FirstRunBubbleView when it is shown.
   virtual void BubbleShown() = 0;
+
+ protected:
+  FirstRunBubbleViewBase(FirstRunBubble* bubble_window, Profile* profile);
+  virtual ~FirstRunBubbleViewBase();
+  FirstRunBubble* bubble_window() { return bubble_window_; }
+  Profile* profile() { return profile_; }
+
+ private:
+  FirstRunBubble* bubble_window_;
+  Profile* profile_;
+  DISALLOW_COPY_AND_ASSIGN(FirstRunBubbleViewBase);
 };
 
-// FirstRunBubbleView ---------------------------------------------------------
+FirstRunBubbleViewBase::FirstRunBubbleViewBase(FirstRunBubble* bubble_window,
+                                       Profile* profile)

[msw, 2011/10/29 02:55:11]

Fix this indenting.

[alicet1, 2011/11/02 06:51:25]

Done.

+    : bubble_window_(bubble_window), profile_(profile) {}
 

[msw, 2011/10/29 02:55:11]

Replace this blank line.

[alicet1, 2011/11/02 06:51:25]

Done.

+FirstRunBubbleViewBase::~FirstRunBubbleViewBase() {
+  GetFocusManager()->RemoveFocusChangeListener(this);
+}
+
+// FirstRunBubbleView ---------------------------------------------------------
 class FirstRunBubbleView : public FirstRunBubbleViewBase {
  public:
   FirstRunBubbleView(FirstRunBubble* bubble_window, Profile* profile);
 
+  // Override from FirstRunBubbleViewBase:
+  void BubbleShown();

[msw, 2011/10/29 02:55:11]

Please use the OVERRIDE keyword for this and other overrides.

[alicet1, 2011/11/02 06:51:25]

removed.

+
  private:
   virtual ~FirstRunBubbleView() {}
 
-  // FirstRunBubbleViewBase:
-  virtual void BubbleShown();
-
-  // Overridden from View:
+  // Override from View:
   virtual void ButtonPressed(views::Button* sender, const views::Event& event);
-  virtual void Layout();
   virtual gfx::Size GetPreferredSize();
+  virtual void Layout();
 
-  // FocusChangeListener:
+  // Override from FocusChangeListener:
   virtual void FocusWillChange(View* focused_before, View* focused_now);
 
-  FirstRunBubble* bubble_window_;
   views::Label* label1_;
   views::Label* label2_;
   views::Label* label3_;
-  views::NativeTextButton* change_button_;
   views::NativeTextButton* keep_button_;
-  Profile* profile_;
+  views::NativeTextButton* change_button_;
 
   DISALLOW_COPY_AND_ASSIGN(FirstRunBubbleView);
 };
 
 FirstRunBubbleView::FirstRunBubbleView(FirstRunBubble* bubble_window,
                                        Profile* profile)
-    : bubble_window_(bubble_window),
+    : FirstRunBubbleViewBase(bubble_window, profile),
       label1_(NULL),
       label2_(NULL),
       label3_(NULL),
       keep_button_(NULL),
-      change_button_(NULL),
-      profile_(profile) {
+      change_button_(NULL) {
   const gfx::Font& font =
       ResourceBundle::GetSharedInstance().GetFont(ResourceBundle::MediumFont);
 
   label1_ = new views::Label(l10n_util::GetStringUTF16(IDS_FR_BUBBLE_TITLE));
   label1_->SetFont(font.DeriveFont(3, gfx::Font::BOLD));
-  label1_->SetBackgroundColor(Bubble::kBackgroundColor);
+  label1_->SetBackgroundColor(SK_ColorWHITE);

[msw, 2011/10/29 02:55:11]

Define a local constant as white to use instead of Bubble::kBackgroundColor here
and elsewhere, so fixing this later isn't as troublesome.

[msw, 2011/10/30 21:02:15]

Actually, I suspect all of these label backgrounds are unnecessary, you should
try removing them and let the bubble's background show. Otherwise, if possible,
you should use the delegate's GetColor() instead of a constant.

[alicet1, 2011/11/02 06:51:25]

removed.

   label1_->SetHorizontalAlignment(views::Label::ALIGN_LEFT);
   AddChildView(label1_);
 
@@ -121,7 +126,7 @@ FirstRunBubbleView::FirstRunBubbleView(FirstRunBubble* bubble_window,
   label2_ = new views::Label(l10n_util::GetStringUTF16(IDS_FR_BUBBLE_SUBTEXT));
   label2_->SetMultiLine(true);
   label2_->SetFont(font);
-  label2_->SetBackgroundColor(Bubble::kBackgroundColor);
+  label2_->SetBackgroundColor(SK_ColorWHITE);
   label2_->SetHorizontalAlignment(views::Label::ALIGN_LEFT);
   label2_->SizeToFit(ps.width() - kBubblePadding * 2);
   AddChildView(label2_);
@@ -132,42 +137,42 @@ FirstRunBubbleView::FirstRunBubbleView(FirstRunBubble* bubble_window,
   label3_ = new views::Label(question_str);
   label3_->SetMultiLine(true);
   label3_->SetFont(font);
-  label3_->SetBackgroundColor(Bubble::kBackgroundColor);
+  label3_->SetBackgroundColor(SK_ColorWHITE);
   label3_->SetHorizontalAlignment(views::Label::ALIGN_LEFT);
   label3_->SizeToFit(ps.width() - kBubblePadding * 2);
   AddChildView(label3_);
 
-  std::wstring keep_str = UTF16ToWide(l10n_util::GetStringFUTF16(
+  string16 keep_str = l10n_util::GetStringFUTF16(
       IDS_FR_BUBBLE_OK,
-      GetDefaultSearchEngineName(profile)));
+      GetDefaultSearchEngineName(profile));
   keep_button_ = new views::NativeTextButton(this, keep_str);
   keep_button_->SetIsDefault(true);
   AddChildView(keep_button_);
 
-  std::wstring change_str =
-      UTF16ToWide(l10n_util::GetStringUTF16(IDS_FR_BUBBLE_CHANGE));
+  string16 change_str = l10n_util::GetStringUTF16(IDS_FR_BUBBLE_CHANGE);
   change_button_ = new views::NativeTextButton(this, change_str);
   AddChildView(change_button_);
 }
 
-void FirstRunBubbleView::BubbleShown() {
-  keep_button_->RequestFocus();
-}
-
 void FirstRunBubbleView::ButtonPressed(views::Button* sender,
                                        const views::Event& event) {
   UserMetrics::RecordAction(UserMetricsAction("FirstRunBubbleView_Clicked"));
-  bubble_window_->set_fade_away_on_close(true);
-  bubble_window_->Close();
+  static_cast<views::BubbleDelegateView*>(GetWidget()->widget_delegate())
+      ->StartFade(false);
   if (change_button_ == sender) {

[msw, 2011/10/29 02:55:11]

Code after StartFade is technically in a race condition with the bubble closing
and this view being deleted, right? I wonder if this should be executed before
starting the fade, or be made robust to the destruction of |this|.

[alicet1, 2011/11/02 06:51:25]

this is removed as part of removing this bubble -- the large version of this
bubble is no longer shown.

     UserMetrics::RecordAction(
                     UserMetricsAction("FirstRunBubbleView_ChangeButton"));

[msw, 2011/10/29 02:55:11]

Fix this indenting.

[alicet1, 2011/11/02 06:51:25]

Done.

 
-    Browser* browser = BrowserList::GetLastActiveWithProfile(profile_);
+    Browser* browser = BrowserList::GetLastActiveWithProfile(profile());
     if (browser) {
       browser->OpenSearchEngineOptionsDialog();

[msw, 2011/10/29 02:55:11]

Single line if doesn't require braces.

[alicet1, 2011/11/02 06:51:25]

removed.

     }
   }
+  GetWidget()->Close();

[msw, 2011/10/29 02:55:11]

Why is this closing the bubble mid-fade?

[alicet1, 2011/11/02 06:51:25]

I changed it to close only. I'm not sure why a button press should be a fade
instead of a direct close.

+}
+
+void FirstRunBubbleView::BubbleShown() {
+  keep_button_->RequestFocus();
 }
 
 void FirstRunBubbleView::Layout() {
@@ -241,45 +246,42 @@ class FirstRunOEMBubbleView : public FirstRunBubbleViewBase {
  public:
   FirstRunOEMBubbleView(FirstRunBubble* bubble_window, Profile* profile);
 
- private:
-  virtual ~FirstRunOEMBubbleView() { }
+  // Override from FirstRunBubbleViewBase:
+  void BubbleShown();
 
-  // FirstRunBubbleViewBase:
-  virtual void BubbleShown();
+ private:
+  virtual ~FirstRunOEMBubbleView() {}
 
-  // Overridden from View:
+  // Override from View:
   virtual void ButtonPressed(views::Button* sender, const views::Event& event);
-  virtual void Layout();
   virtual gfx::Size GetPreferredSize();
+  virtual void Layout();
 
-  // FocusChangeListener:
+  // Override from FocusChangeListener:
   virtual void FocusWillChange(View* focused_before, View* focused_now);
 
-  FirstRunBubble* bubble_window_;
   views::Label* label1_;
   views::Label* label2_;
   views::Label* label3_;
   views::ImageButton* close_button_;
-  Profile* profile_;
 
   DISALLOW_COPY_AND_ASSIGN(FirstRunOEMBubbleView);
 };
 
 FirstRunOEMBubbleView::FirstRunOEMBubbleView(FirstRunBubble* bubble_window,
                                              Profile* profile)
-    : bubble_window_(bubble_window),
+    : FirstRunBubbleViewBase(bubble_window, profile),
       label1_(NULL),
       label2_(NULL),
       label3_(NULL),
-      close_button_(NULL),
-      profile_(profile) {
+      close_button_(NULL) {
   ResourceBundle& rb = ResourceBundle::GetSharedInstance();
   const gfx::Font& font = rb.GetFont(ResourceBundle::MediumFont);
 
   label1_ = new views::Label(
       l10n_util::GetStringUTF16(IDS_FR_OEM_BUBBLE_TITLE_1));
   label1_->SetFont(font.DeriveFont(3, gfx::Font::BOLD));
-  label1_->SetBackgroundColor(Bubble::kBackgroundColor);
+  label1_->SetBackgroundColor(SK_ColorWHITE);
   label1_->SetEnabledColor(SK_ColorRED);
   label1_->SetHorizontalAlignment(views::Label::ALIGN_LEFT);
   AddChildView(label1_);
@@ -287,7 +289,7 @@ FirstRunOEMBubbleView::FirstRunOEMBubbleView(FirstRunBubble* bubble_window,
   label2_ = new views::Label(
       l10n_util::GetStringUTF16(IDS_FR_OEM_BUBBLE_TITLE_2));
   label2_->SetFont(font.DeriveFont(3, gfx::Font::BOLD));
-  label2_->SetBackgroundColor(Bubble::kBackgroundColor);
+  label2_->SetBackgroundColor(SK_ColorWHITE);
   label2_->SetHorizontalAlignment(views::Label::ALIGN_LEFT);
   AddChildView(label2_);
 
@@ -297,7 +299,7 @@ FirstRunOEMBubbleView::FirstRunOEMBubbleView(FirstRunBubble* bubble_window,
       l10n_util::GetStringUTF16(IDS_FR_OEM_BUBBLE_SUBTEXT));
   label3_->SetMultiLine(true);
   label3_->SetFont(font);
-  label3_->SetBackgroundColor(Bubble::kBackgroundColor);
+  label3_->SetBackgroundColor(SK_ColorWHITE);
   label3_->SetHorizontalAlignment(views::Label::ALIGN_LEFT);
   label3_->SizeToFit(ps.width() - kOEMBubblePadding * 2);
   AddChildView(label3_);
@@ -315,15 +317,14 @@ FirstRunOEMBubbleView::FirstRunOEMBubbleView(FirstRunBubble* bubble_window,
 
 void FirstRunOEMBubbleView::BubbleShown() {
   RequestFocus();

[msw, 2011/10/29 02:55:11]

This probably isn't necessary.

[alicet1, 2011/11/02 06:51:25]

removed.

-  // No button in oem_bubble to request focus.
 }
 
 void FirstRunOEMBubbleView::ButtonPressed(views::Button* sender,
                                           const views::Event& event) {
   UserMetrics::RecordAction(
       UserMetricsAction("FirstRunOEMBubbleView_Clicked"));
-  bubble_window_->set_fade_away_on_close(true);
-  bubble_window_->Close();
+  static_cast<views::BubbleDelegateView*>(GetWidget()->widget_delegate())
+      ->StartFade(false);
 }
 
 void FirstRunOEMBubbleView::Layout() {
@@ -366,7 +367,7 @@ gfx::Size FirstRunOEMBubbleView::GetPreferredSize() {
           IDS_FIRSTRUNOEMBUBBLE_DIALOG_WIDTH_CHARS, font),
       ui::GetLocalizedContentsHeightForFont(
           IDS_FIRSTRUNOEMBUBBLE_DIALOG_HEIGHT_LINES, font));
-
+#if defined(OS_WIN) && !defined(USE_AURA)
   // WARNING: HACK. Vista and XP calculate font size differently; this means
   // that a dialog box correctly proportioned for XP will appear too large in
   // Vista. The correct thing to do is to change font size calculations in
@@ -378,6 +379,7 @@ gfx::Size FirstRunOEMBubbleView::GetPreferredSize() {
     size.set_width(static_cast<int>(size.width() * 0.85));
     size.set_height(static_cast<int>(size.height() * 0.85));
   }
+#endif
   return size;
 }
 
@@ -393,23 +395,21 @@ class FirstRunMinimalBubbleView : public FirstRunBubbleViewBase {
  public:
   FirstRunMinimalBubbleView(FirstRunBubble* bubble_window, Profile* profile);
 
- private:
-  virtual ~FirstRunMinimalBubbleView() { }
+  // Override from FirstRunBubbleViewBase:
+  void BubbleShown();
 
-  // FirstRunBubbleViewBase:
-  virtual void BubbleShown();
+ private:
+  virtual ~FirstRunMinimalBubbleView();
 
-  // Overridden from View:
+  // Override from View:
   virtual void ButtonPressed(views::Button* sender,
                              const views::Event& event) { }

[msw, 2011/10/29 02:55:11]

I think inline virtual overrides break clang.

[alicet1, 2011/11/02 06:51:25]

removed.

-  virtual void Layout();
   virtual gfx::Size GetPreferredSize();
+  virtual void Layout();
 
-  // FocusChangeListener:
+  // Override from FocusChangeListener:
   virtual void FocusWillChange(View* focused_before, View* focused_now);
 
-  FirstRunBubble* bubble_window_;
-  Profile* profile_;
   views::Label* label1_;
   views::Label* label2_;
 
@@ -419,8 +419,7 @@ class FirstRunMinimalBubbleView : public FirstRunBubbleViewBase {
 FirstRunMinimalBubbleView::FirstRunMinimalBubbleView(
     FirstRunBubble* bubble_window,
     Profile* profile)
-    : bubble_window_(bubble_window),
-      profile_(profile),
+    : FirstRunBubbleViewBase(bubble_window, profile),
       label1_(NULL),
       label2_(NULL) {
   const gfx::Font& font =
@@ -428,9 +427,9 @@ FirstRunMinimalBubbleView::FirstRunMinimalBubbleView(
 
   label1_ = new views::Label(l10n_util::GetStringFUTF16(
       IDS_FR_SE_BUBBLE_TITLE,
-      GetDefaultSearchEngineName(profile_)));
+      GetDefaultSearchEngineName(profile)));
   label1_->SetFont(font.DeriveFont(3, gfx::Font::BOLD));
-  label1_->SetBackgroundColor(Bubble::kBackgroundColor);
+  label1_->SetBackgroundColor(SK_ColorWHITE);
   label1_->SetHorizontalAlignment(views::Label::ALIGN_LEFT);
   AddChildView(label1_);
 
@@ -440,12 +439,16 @@ FirstRunMinimalBubbleView::FirstRunMinimalBubbleView(
       l10n_util::GetStringUTF16(IDS_FR_BUBBLE_SUBTEXT));
   label2_->SetMultiLine(true);
   label2_->SetFont(font);
-  label2_->SetBackgroundColor(Bubble::kBackgroundColor);
+  label2_->SetBackgroundColor(SK_ColorWHITE);
   label2_->SetHorizontalAlignment(views::Label::ALIGN_LEFT);
   label2_->SizeToFit(ps.width() - kBubblePadding * 2);
   AddChildView(label2_);
 }
 
+FirstRunMinimalBubbleView::~FirstRunMinimalBubbleView() {
+  GetFocusManager()->RemoveFocusChangeListener(this);
+}
+
 void FirstRunMinimalBubbleView::BubbleShown() {
   RequestFocus();
 }
@@ -486,47 +489,65 @@ void FirstRunMinimalBubbleView::FocusWillChange(View* focused_before,
 // FirstRunBubble -------------------------------------------------------------
 
 // static
-FirstRunBubble* FirstRunBubble::Show(
+views::Widget* FirstRunBubble::Show(
     Profile* profile,
     views::Widget* parent,
     const gfx::Rect& position_relative_to,
     views::BubbleBorder::ArrowLocation arrow_location,
     FirstRun::BubbleType bubble_type) {
-  FirstRunBubble* bubble = new FirstRunBubble();
-  FirstRunBubbleViewBase* view = NULL;
+  FirstRunBubble* delegate =
+      new FirstRunBubble(profile,
+                         parent,
+                         position_relative_to,
+                         arrow_location,
+                         bubble_type);
+  views::Widget* widget =
+      views::BubbleDelegateView::CreateBubble(delegate, parent);
+  delegate->StartFade(true);
+  return widget;
+}
 
-  switch (bubble_type) {
+void FirstRunBubble::Init() {
+  SetLayoutManager(new views::FillLayout());
+  FirstRunBubbleViewBase* view = NULL;
+  switch (bubble_type_) {
     case FirstRun::OEM_BUBBLE:
-      view = new FirstRunOEMBubbleView(bubble, profile);
+      view = new FirstRunOEMBubbleView(this, profile_);
       break;
     case FirstRun::LARGE_BUBBLE:
-      view = new FirstRunBubbleView(bubble, profile);
+      view = new FirstRunBubbleView(this, profile_);
       break;
     case FirstRun::MINIMAL_BUBBLE:
-      view = new FirstRunMinimalBubbleView(bubble, profile);
+      view = new FirstRunMinimalBubbleView(this, profile_);
       break;
     default:
       NOTREACHED();
   }
-  bubble->set_view(view);
-  bubble->InitBubble(
-      parent, position_relative_to, arrow_location, view, bubble);
-  bubble->GetWidget()->GetFocusManager()->AddFocusChangeListener(view);
+  AddChildView(view);
+  parent_->GetFocusManager()->AddFocusChangeListener(view);
   view->BubbleShown();
-  return bubble;
 }
 
-FirstRunBubble::FirstRunBubble()
-    : has_been_activated_(false),
-      ALLOW_THIS_IN_INITIALIZER_LIST(enable_window_method_factory_(this)),
-      view_(NULL) {
+FirstRunBubble::FirstRunBubble(
+    Profile* profile,
+    views::Widget* parent,
+    const gfx::Rect& position_relative_to,
+    views::BubbleBorder::ArrowLocation arrow_location,
+    FirstRun::BubbleType bubble_type)
+    : views::BubbleDelegateView(position_relative_to.origin(),
+                                arrow_location, SK_ColorWHITE),
+      profile_(profile),
+      parent_(parent),
+      bubble_type_(bubble_type),
+      has_been_activated_(false),
+      ALLOW_THIS_IN_INITIALIZER_LIST(enable_window_method_factory_(this)) {
 }
 
 FirstRunBubble::~FirstRunBubble() {
   enable_window_method_factory_.InvalidateWeakPtrs();
-  GetWidget()->GetFocusManager()->RemoveFocusChangeListener(view_);
 }
 
+#if defined(OS_WIN) && !defined(USE_AURA)
 void FirstRunBubble::EnableParent() {
   ::EnableWindow(GetParent(), true);
   // The EnableWindow() call above causes the parent to become active, which
@@ -534,16 +555,13 @@ void FirstRunBubble::EnableParent() {
   // have to call it again before activating the bubble to prevent the parent
   // window from rendering inactive.
   // TODO(beng): this only works in custom-frame mode, not glass-frame mode.
-  HWND bubble_owner = GetLogicalBubbleOwner(GetNativeView());
-  views::Widget* parent = views::Widget::GetWidgetForNativeView(bubble_owner);
-  if (parent)
-    parent->DisableInactiveRendering();
+  if (parent_)
+    parent_->DisableInactiveRendering();
   // Reactivate the FirstRunBubble so it responds to OnActivate messages.
   SetWindowPos(GetParent(), 0, 0, 0, 0,
                SWP_NOSIZE | SWP_NOMOVE | SWP_NOREDRAW | SWP_SHOWWINDOW);
 }
 
-#if defined(OS_WIN) && !defined(USE_AURA)
 void FirstRunBubble::OnActivate(UINT action, BOOL minimized, HWND window) {
   // Keep the bubble around for kLingerTime milliseconds, to prevent accidental
   // closure.
@@ -571,8 +589,7 @@ void FirstRunBubble::OnActivate(UINT action, BOOL minimized, HWND window) {
 }
 #endif
 
-void FirstRunBubble::BubbleClosing(Bubble* bubble, bool closed_by_escape) {
+void FirstRunBubble::WindowClosing() {
   // Make sure our parent window is re-enabled.
-  if (!IsWindowEnabled(GetParent()))
-    ::EnableWindow(GetParent(), true);
+  GetFocusManager()->FocusNativeView(parent_->GetNativeView());

[msw, 2011/10/29 02:55:11]

Can you check if this is necessary? I suspect it isn't.

[alicet1, 2011/11/02 06:51:25]

as in, when bubble widget is closing, parent should automatically get the focus?
 I'm actually not sure if L550 - 590 is needed.

[Miranda Callahan, 2011/11/02 15:36:14]

I remember there being bizarre focus problems that we had to work around when
the first run bubble was first introduced years ago; if these are no longer a
problem, it would be great if we could get rid of some of this code.

 }