Bookmark sync promo for Views.

chrome/browser/ui/views/bookmarks/bookmark_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/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"
 #include "chrome/app/chrome_command_ids.h"
 #include "chrome/browser/bookmarks/bookmark_model.h"
 #include "chrome/browser/bookmarks/bookmark_model_factory.h"
 #include "chrome/browser/bookmarks/bookmark_utils.h"
 #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"
 #include "ui/base/keycodes/keyboard_codes.h"
 #include "ui/base/l10n/l10n_util.h"
 #include "ui/base/resource/resource_bundle.h"
 #include "ui/views/bubble/bubble_frame_view.h"
 #include "ui/views/controls/button/label_button.h"
 #include "ui/views/controls/combobox/combobox.h"
 #include "ui/views/controls/label.h"
 #include "ui/views/controls/link.h"
 #include "ui/views/controls/textfield/textfield.h"
 #include "ui/views/layout/grid_layout.h"
 #include "ui/views/layout/layout_constants.h"
 #include "ui/views/widget/widget.h"
 
 using content::UserMetricsAction;
 using views::ColumnSet;
 using views::GridLayout;
 
 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.
 
 namespace chrome {
 
 void ShowBookmarkBubbleView(views::View* anchor_view,
                             BookmarkBubbleViewObserver* observer,
                             Profile* profile,
                             const GURL& url,
@@ skipping 62 matching lines @@
 }
 
 void BookmarkBubbleView::WindowClosing() {
   // We have to reset |bubble_| here, not in our destructor, because we'll be
   // destroyed asynchronously and the shown state will be checked before then.
   DCHECK_EQ(bookmark_bubble_, this);
   bookmark_bubble_ = NULL;
 
   if (observer_)
     observer_->OnBookmarkBubbleHidden();
- }
+}
 
 bool BookmarkBubbleView::AcceleratorPressed(
     const ui::Accelerator& accelerator) {
   if (accelerator.key_code() == ui::VKEY_RETURN) {
      if (edit_button_->HasFocus())
        HandleButtonPressed(edit_button_);
      else
        HandleButtonPressed(close_button_);
      return true;
   } else if (accelerator.key_code() == ui::VKEY_ESCAPE) {
@@ skipping 28 matching lines @@
   views::Label* combobox_label = new views::Label(
       l10n_util::GetStringUTF16(IDS_BOOKMARK_BUBBLE_FOLDER_TEXT));
 
   parent_combobox_ = new views::Combobox(&parent_model_);
   parent_combobox_->set_listener(this);
   parent_combobox_->SetAccessibleName(combobox_label->text());
 
   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);
 
   cs->AddColumn(GridLayout::FILL, GridLayout::CENTER, 0,
                 GridLayout::USE_PREF, 0, 0);
   cs->AddPaddingColumn(1, views::kUnrelatedControlLargeHorizontalSpacing);
 
   cs->AddColumn(GridLayout::LEADING, GridLayout::TRAILING, 0,
                 GridLayout::USE_PREF, 0, 0);
   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);
   title_tf_ = new views::Textfield();
   title_tf_->SetText(GetTitle());
   layout->AddView(title_tf_, 5, 1);
 
   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));
 }
 
 BookmarkBubbleView::BookmarkBubbleView(views::View* anchor_view,
                                        BookmarkBubbleViewObserver* observer,
                                        Profile* profile,
                                        const GURL& url,
                                        bool newly_bookmarked)
     : BubbleDelegateView(anchor_view, views::BubbleBorder::TOP_RIGHT),
       observer_(observer),
       profile_(profile),
       url_(url),
       newly_bookmarked_(newly_bookmarked),
       parent_model_(
           BookmarkModelFactory::GetForProfile(profile_),
           BookmarkModelFactory::GetForProfile(profile_)->
               GetMostRecentlyAddedNodeForURL(url)),
       remove_button_(NULL),
       edit_button_(NULL),
       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));
 }
 
 string16 BookmarkBubbleView::GetTitle() {
   BookmarkModel* bookmark_model =
       BookmarkModelFactory::GetForProfile(profile_);
   const BookmarkNode* node =
       bookmark_model->GetMostRecentlyAddedNodeForURL(url_);
   if (node)
@@ skipping 62 matching lines @@
   if (node) {
     const string16 new_title = title_tf_->text();
     if (new_title != node->GetTitle()) {
       model->SetTitle(node, new_title);
       content::RecordAction(
           UserMetricsAction("BookmarkBubble_ChangeTitleInBubble"));
     }
     parent_model_.MaybeChangeParent(node, parent_combobox_->selected_index());
   }
 }