Bookmark sync promo for Views.

chrome/browser/ui/views/bookmarks/bookmark_bubble_view.cc

Index: chrome/browser/ui/views/bookmarks/bookmark_bubble_view.cc
diff --git a/chrome/browser/ui/views/bookmarks/bookmark_bubble_view.cc b/chrome/browser/ui/views/bookmarks/bookmark_bubble_view.cc
index ca52a4507855165aeda92a43ae2258eb3510d169..dcc47d5c29018b33550da4db70324d1d85bb0f54 100644
--- a/chrome/browser/ui/views/bookmarks/bookmark_bubble_view.cc
+++ b/chrome/browser/ui/views/bookmarks/bookmark_bubble_view.cc
@@ -4,6 +4,7 @@
 
 #include "chrome/browser/ui/views/bookmarks/bookmark_bubble_view.h"
 
+#include "base/command_line.h"
 #include "base/strings/string16.h"
 #include "base/strings/string_util.h"
 #include "base/strings/utf_string_conversions.h"
@@ -14,8 +15,12 @@
 #include "chrome/browser/profiles/profile.h"
 #include "chrome/browser/ui/bookmarks/bookmark_editor.h"
 #include "chrome/browser/ui/browser.h"
+#include "chrome/browser/ui/browser_finder.h"
 #include "chrome/browser/ui/browser_list.h"
+#include "chrome/browser/ui/sync/sync_promo_ui.h"
 #include "chrome/browser/ui/views/bookmarks/bookmark_bubble_view_observer.h"
+#include "chrome/browser/ui/views/bookmarks/bookmark_sync_promo_view.h"
+#include "chrome/common/chrome_switches.h"
 #include "content/public/browser/user_metrics.h"
 #include "grit/generated_resources.h"
 #include "grit/theme_resources.h"
@@ -41,6 +46,15 @@ namespace {
 // Minimum width of the the bubble.
 const int kMinBubbleWidth = 350;
 
+// Top padding of the bubble.

[sky, 2013/07/15 18:14:10]

Can we use the standard constants we have instead of one offs? I'm thinking of
the constants in layout_constants.

[fdoray, 2013/07/16 17:49:58]

Done for kBubblePadding. But there is no relevant constant to replace
kBubbleTopPadding, unless I don't respect the mock crbug.com/231694.

[fdoray, 2013/07/17 15:01:59]

Done.

+const int kBubbleTopPadding = 12;
+
+// Horizontal and bottom padding of the bubble.
+const int kBubblePadding = 19;
+
+// Width of the border of a button or text field.
+const int kControlBorderWidth = 1;
+
 }  // namespace
 
 // Declared in browser_dialogs.h so callers don't have to depend on our header.
@@ -123,7 +137,7 @@ void BookmarkBubbleView::WindowClosing() {
 
   if (observer_)
     observer_->OnBookmarkBubbleHidden();
- }
+}
 
 bool BookmarkBubbleView::AcceleratorPressed(
     const ui::Accelerator& accelerator) {
@@ -172,14 +186,22 @@ void BookmarkBubbleView::Init() {
   GridLayout* layout = new GridLayout(this);
   SetLayoutManager(layout);
 
-  const int kTitleColumnSetID = 0;
+  // Column sets used in the layout of the bubble.
+  enum ColumnSetID {

[sky, 2013/07/15 18:14:10]

Chrome style is ALL_CAPS for enums.

[fdoray, 2013/07/16 17:49:58]

Done.

+    kTitleColumnSetID,
+    kContentColumnSetID,
+    kSyncPromoColumnSetID
+  };
+
   ColumnSet* cs = layout->AddColumnSet(kTitleColumnSetID);
+  cs->AddPaddingColumn(0, kBubblePadding);
   cs->AddColumn(GridLayout::CENTER, GridLayout::CENTER, 0, GridLayout::USE_PREF,
                 0, 0);
+  cs->AddPaddingColumn(0, kBubblePadding);
 
   // The column layout used for middle and bottom rows.
-  const int kFirstColumnSetID = 1;
-  cs = layout->AddColumnSet(kFirstColumnSetID);
+  cs = layout->AddColumnSet(kContentColumnSetID);
+  cs->AddPaddingColumn(0, kBubblePadding);
   cs->AddColumn(GridLayout::LEADING, GridLayout::CENTER, 0,
                 GridLayout::USE_PREF, 0, 0);
   cs->AddPaddingColumn(0, views::kUnrelatedControlHorizontalSpacing);
@@ -193,12 +215,13 @@ void BookmarkBubbleView::Init() {
   cs->AddPaddingColumn(0, views::kRelatedButtonHSpacing);
   cs->AddColumn(GridLayout::LEADING, GridLayout::TRAILING, 0,
                 GridLayout::USE_PREF, 0, 0);
+  cs->AddPaddingColumn(0, kBubblePadding - kControlBorderWidth);
 
   layout->StartRow(0, kTitleColumnSetID);
   layout->AddView(title_label);
   layout->AddPaddingRow(0, views::kUnrelatedControlHorizontalSpacing);
 
-  layout->StartRow(0, kFirstColumnSetID);
+  layout->StartRow(0, kContentColumnSetID);
   views::Label* label = new views::Label(
       l10n_util::GetStringUTF16(IDS_BOOKMARK_BUBBLE_TITLE_TEXT));
   layout->AddView(label);
@@ -208,18 +231,41 @@ void BookmarkBubbleView::Init() {
 
   layout->AddPaddingRow(0, views::kUnrelatedControlHorizontalSpacing);
 
-  layout->StartRow(0, kFirstColumnSetID);
+  layout->StartRow(0, kContentColumnSetID);
   layout->AddView(combobox_label);
   layout->AddView(parent_combobox_, 5, 1);
 
   layout->AddPaddingRow(0, views::kRelatedControlVerticalSpacing);
 
-  layout->StartRow(0, kFirstColumnSetID);
+  layout->StartRow(0, kContentColumnSetID);
   layout->SkipColumns(2);
   layout->AddView(remove_button_);
   layout->AddView(edit_button_);
   layout->AddView(close_button_);
 
+  layout->AddPaddingRow(0, kBubblePadding - kControlBorderWidth);
+
+  const CommandLine* command_line = CommandLine::ForCurrentProcess();
+  if (command_line->HasSwitch(switches::kEnableBookmarkSyncPromo) &&
+      SyncPromoUI::ShouldShowSyncPromo(profile_)) {

[sky, 2013/07/15 18:14:10]

I don't like changing the meaning of ShouldShowSyncPromo to mean should show
sync promo or bookmark bubble promo. There should be a separate function for the
bookmarks and it shouldn't be on SyncPromoUI.

[fdoray, 2013/07/16 17:49:58]

ShouldShowSyncPromo is already used to determine if a sync promo should be
displayed at startup, on the NTP or in a bubble after an extension is installed.
Using it to determine if a sync promo should be displayed in the bookmark bubble
seems logical.

Is the problem that I removed "if (profile->IsOffTheRecord())" from the
function? This has no impact on existing code:

sync_promo_ui.cc(46): I copied the "if (profile->IsOffTheRecord())" check at the
beginning of the ShouldShowSyncPromoAtStartup function.

ntp_login_handler.cc(143): NTPLoginHandler is never instantiated for an
incognito NTP.

extension_installed_bubble.cc(137): The bubble is always shown in a normal
browser, even when the extension is installed from an incognito browser.


On 2013/07/15 18:14:10, sky wrote:

[sky, 2013/07/16 21:23:00]

My concern still stands. Isn't SyncPromoUI a specific thing and not a handful of
different UI bits?

[Roger Tawa OOO till Jul 10th, 2013/07/17 13:26:32]

Initially there was only one sync promo, but over time other sync promos have
been added.  They all use this static function to determine if they should show
themselves or not.  So its OK to reuse this function in different UI bits.

+    // The column layout used for the sync promo.
+    cs = layout->AddColumnSet(kSyncPromoColumnSetID);
+    cs->AddColumn(GridLayout::FILL,
+                  GridLayout::FILL,
+                  1,
+                  GridLayout::USE_PREF,
+                  0,
+                  0);
+
+    Browser* browser = chrome::FindBrowserWithProfile(

[sky, 2013/07/15 18:14:10]

There is nothing that forces BookmarkBubbleView to be used by a browser. I think
it would be better to put this logic in a delegate so that BookmarkBubbleView
isn't dependent on a browser. This would likely simplify your test code too.

[fdoray, 2013/07/16 17:49:58]

I couldn't figure out how a delegate would be useful here. I tried something
else to make BookmarkBubbleView independent from BookmarkSyncPromoView and from
any browser. I you prefer a delegate, what would its interface look like?

On 2013/07/15 18:14:10, sky wrote:

[sky, 2013/07/16 21:23:00]

class BookmarkBubbleViewDelegate {
 public:
  virtual void OnXXXClicked() = 0;
};

[fdoray, 2013/07/17 15:01:59]

Done. This is true on all platforms, so I wrote a reusable delegate.

+        profile_,
+        chrome::HOST_DESKTOP_TYPE_NATIVE);
+    sync_promo_view_ = new BookmarkSyncPromoView(browser);
+
+    layout->StartRow(0, kSyncPromoColumnSetID);
+    layout->AddView(sync_promo_view_);
+  }
+
   AddAccelerator(ui::Accelerator(ui::VKEY_RETURN, ui::EF_NONE));
 }
 
@@ -242,13 +288,14 @@ BookmarkBubbleView::BookmarkBubbleView(views::View* anchor_view,
       close_button_(NULL),
       title_tf_(NULL),
       parent_combobox_(NULL),
+      sync_promo_view_(NULL),
       remove_bookmark_(false),
       apply_edits_(true) {
   const SkColor background_color = GetNativeTheme()->GetSystemColor(
       ui::NativeTheme::kColorId_DialogBackground);
   set_color(background_color);
   set_background(views::Background::CreateSolidBackground(background_color));
-  set_margins(gfx::Insets(12, 19, 18, 18));
+  set_margins(gfx::Insets(kBubbleTopPadding, 0, 0, 0));
   // Compensate for built-in vertical padding in the anchor view's image.
   set_anchor_view_insets(gfx::Insets(7, 0, 7, 0));
 }