Save password functionality added to the save password bubble

chrome/browser/ui/views/content_setting_bubble_contents.cc

Index: chrome/browser/ui/views/content_setting_bubble_contents.cc
diff --git a/chrome/browser/ui/views/content_setting_bubble_contents.cc b/chrome/browser/ui/views/content_setting_bubble_contents.cc
index 87f56c898b1129c549a814d27faee7fb1e06e7f2..db1ac258bd28bea5bf964c7450e397f5f9773736 100644
--- a/chrome/browser/ui/views/content_setting_bubble_contents.cc
+++ b/chrome/browser/ui/views/content_setting_bubble_contents.cc
@@ -148,7 +148,9 @@ ContentSettingBubbleContents::ContentSettingBubbleContents(
       web_contents_(web_contents),
       custom_link_(NULL),
       manage_link_(NULL),
-      close_button_(NULL) {
+      close_button_(NULL),
+      never_button_(NULL),

[Peter Kasting, 2013/09/09 18:19:02]

Nit: This isn't a good name for this button.  Perhaps |cancel_button_|, or
|dont_save_button_|.

[npentrel, 2013/09/09 22:08:34]

Done.

+      save_button_(NULL) {
   // Compensate for built-in vertical padding in the anchor view's image.
   set_anchor_view_insets(gfx::Insets(5, 0, 5, 0));
 
@@ -389,22 +391,57 @@ void ContentSettingBubbleContents::Init() {
     layout->AddPaddingRow(0, views::kRelatedControlVerticalSpacing);
   }
 
-  const int kDoubleColumnSetId = 1;
-  views::ColumnSet* double_column_set =
-      layout->AddColumnSet(kDoubleColumnSetId);
-  double_column_set->AddColumn(GridLayout::LEADING, GridLayout::CENTER, 1,
-                        GridLayout::USE_PREF, 0, 0);
-  double_column_set->AddPaddingColumn(
-      0, views::kUnrelatedControlHorizontalSpacing);
-  double_column_set->AddColumn(GridLayout::TRAILING, GridLayout::CENTER, 0,
-                        GridLayout::USE_PREF, 0, 0);
-
-  layout->StartRow(0, kDoubleColumnSetId);
-  manage_link_ = new views::Link(UTF8ToUTF16(bubble_content.manage_link));
-  manage_link_->set_listener(this);
-  layout->AddView(manage_link_);
-  if (content_setting_bubble_model_->content_type() !=
+  if (content_setting_bubble_model_->content_type() ==
       CONTENT_SETTINGS_TYPE_SAVE_PASSWORD) {
+    const int kDoubleColumnSetId = 2;

[Peter Kasting, 2013/09/09 18:19:02]

Tiny nit: Why order these as (2, 1) instead of (1, 2)?  Plus if you did the
latter you could pull kDoubleColumnSet and the first AddColumn() call out above
the conditional.

[npentrel, 2013/09/09 22:08:34]

Changed the order. However it is not possible to pull out those two calls
because the ids used in initializing double_column_set differ.

[Peter Kasting, 2013/09/10 00:22:11]

You misunderstood the intent of my comment; sorry if I was unclear.

I did not mean "reverse the order of the conditional arms".  I meant "swap the
column set IDs used inside this arm, since there's no obvious reason we add
column set number 2 before we add set 1".  Once that is done, both arms will
begin with:

const int kDoubleColumnSetId = 1;
views::ColumnSet* double_column_set = layout->AddColumnSet(kDoubleColumnSetId);

...and then this code can be hoisted.

[npentrel, 2013/09/10 16:46:38]

Done. Gotcha.

+    views::ColumnSet* double_column_set =
+        layout->AddColumnSet(kDoubleColumnSetId);
+    double_column_set->AddColumn(GridLayout::TRAILING, GridLayout::CENTER, 1,
+                                 GridLayout::USE_PREF, 0, 0);
+    double_column_set->AddPaddingColumn(
+        0, views::kRelatedControlSmallVerticalSpacing);
+    double_column_set->AddColumn(GridLayout::TRAILING, GridLayout::CENTER, 0,
+                                 GridLayout::USE_PREF, 0, 0);
+
+    const int kSingleColumnRightSetId = 1;
+    views::ColumnSet* right_column_set =
+        layout->AddColumnSet(kSingleColumnRightSetId);
+    right_column_set->AddColumn(GridLayout::TRAILING, GridLayout::FILL, 1,
+                                GridLayout::USE_PREF, 0, 0);
+
+    never_button_ = new views::LabelButton(
+        this, l10n_util::GetStringUTF16(IDS_PASSWORD_MANAGER_BLACKLIST_BUTTON));
+    never_button_->SetStyle(views::Button::STYLE_NATIVE_TEXTBUTTON);
+    save_button_ = new views::LabelButton(
+        this, l10n_util::GetStringUTF16(IDS_PASSWORD_MANAGER_SAVE_BUTTON));
+    save_button_->SetStyle(views::Button::STYLE_NATIVE_TEXTBUTTON);
+    manage_link_ = new views::Link(UTF8ToUTF16(bubble_content.manage_link));
+    manage_link_->set_listener(this);
+
+    // Buttons row

[Peter Kasting, 2013/09/09 18:19:02]

Nit: These comments add nothing to the code (plus, don't have punctuation);
remove them.

[npentrel, 2013/09/09 22:08:34]

Done.

+    layout->StartRow(0, kDoubleColumnSetId);
+    layout->AddView(never_button_);
+    layout->AddView(save_button_);
+
+    // Manage link row
+    layout->StartRow(0, kSingleColumnRightSetId);
+    layout->AddView(manage_link_);
+  } else {
+    const int kDoubleColumnSetId = 1;
+    views::ColumnSet* double_column_set =
+        layout->AddColumnSet(kDoubleColumnSetId);
+    double_column_set->AddColumn(GridLayout::LEADING, GridLayout::CENTER, 1,
+                          GridLayout::USE_PREF, 0, 0);
+    double_column_set->AddPaddingColumn(
+        0, views::kUnrelatedControlHorizontalSpacing);
+    double_column_set->AddColumn(GridLayout::TRAILING, GridLayout::CENTER, 0,
+                          GridLayout::USE_PREF, 0, 0);
+
+    layout->StartRow(0, kDoubleColumnSetId);
+    manage_link_ = new views::Link(UTF8ToUTF16(bubble_content.manage_link));
+    manage_link_->set_listener(this);
+    layout->AddView(manage_link_);
+
     close_button_ =
         new views::LabelButton(this, l10n_util::GetStringUTF16(IDS_DONE));
     close_button_->SetStyle(views::Button::STYLE_NATIVE_TEXTBUTTON);
@@ -414,6 +451,16 @@ void ContentSettingBubbleContents::Init() {
 
 void ContentSettingBubbleContents::ButtonPressed(views::Button* sender,
                                                  const ui::Event& event) {
+  if (sender == save_button_) {

[Peter Kasting, 2013/09/09 18:19:02]

Nit: You can simplify this function by handling the radio buttons first and then
collapsing the duplicate StartFade() calls:

  /* Code to handle radio buttons here */

  if (sender == save_button_)
    content_setting_bubble_model_->OnSaveClicked();
  else if (sender == cancel_button_)
    content_setting_bubble_model_->OnCancelClicked();
  else if (sender == close_button_)
    content_setting_bubble_model_->OnDoneClicked();
  StartFade(false);
  // No "return;" necessary

[npentrel, 2013/09/09 22:08:34]

That does not quite work because of the NOTREACHED(). I cannot put that call
anywhere else and therefore I need to call return in each if branch.

[Peter Kasting, 2013/09/10 00:22:11]

My intent was for you to discard the NOTREACHED.  I don't think it's critical.

However, you could preserve it if you wanted by e.g.:

...
else if (sender == close_button_)
  content_setting_bubble_model_->OnDoneClicked();
else
  NOTREACHED();
StartFade(false);

...or, identically, by adding {} to the conditional arms and converting the last
to:

...
} else {
  DCHECK_EQ(close_button_, sender);
  content_setting_bubble_model_->OnDoneClicked();
}

[npentrel, 2013/09/10 16:46:38]

Done.

+    content_setting_bubble_model_->OnSaveClicked();
+    StartFade(false);
+    return;
+  }
+  if (sender == never_button_) {
+    content_setting_bubble_model_->OnCancelClicked();
+    StartFade(false);
+    return;
+  }
   if (sender == close_button_) {
     content_setting_bubble_model_->OnDoneClicked();
     StartFade(false);