Edit button makes username editable in the password manager bubble.

chrome/browser/ui/views/passwords/manage_passwords_bubble_view.cc

Index: chrome/browser/ui/views/passwords/manage_passwords_bubble_view.cc
diff --git a/chrome/browser/ui/views/passwords/manage_passwords_bubble_view.cc b/chrome/browser/ui/views/passwords/manage_passwords_bubble_view.cc
index 5fa6f306ca69d4440ff04aba49a7ea65ee3c3ac6..c7eabca255229be0e37291b000fe3fc5699c4fa3 100644
--- a/chrome/browser/ui/views/passwords/manage_passwords_bubble_view.cc
+++ b/chrome/browser/ui/views/passwords/manage_passwords_bubble_view.cc
@@ -64,6 +64,10 @@ enum ColumnSetType {
   // messages like "No passwords".
   SINGLE_VIEW_COLUMN_SET,
 
+  // | | (FILL, FILL) | | (FILL, FILL) | |
+  // Used for the credentials line of the bubble, for the pending view.
+  DOUBLE_VIEW_COLUMN_SET,
+
   // | | (TRAILING, CENTER) | | (TRAILING, CENTER) | |
   // Used for buttons at the bottom of the bubble which should nest at the
   // bottom-right corner.
@@ -93,6 +97,8 @@ void BuildColumnSet(views::GridLayout* layout, ColumnSetType type) {
   int full_width = ManagePasswordsBubbleView::kDesiredBubbleWidth;
   const int button_divider = ChromeLayoutProvider::Get()->GetDistanceMetric(
       views::DISTANCE_RELATED_BUTTON_HORIZONTAL);
+  const int column_divider = ChromeLayoutProvider::Get()->GetDistanceMetric(
+      views::DISTANCE_RELATED_CONTROL_HORIZONTAL);
   switch (type) {
     case SINGLE_VIEW_COLUMN_SET:
       column_set->AddColumn(views::GridLayout::FILL,
@@ -102,6 +108,13 @@ void BuildColumnSet(views::GridLayout* layout, ColumnSetType type) {
                             full_width,
                             0);
       break;
+    case DOUBLE_VIEW_COLUMN_SET:
+      column_set->AddColumn(views::GridLayout::FILL, views::GridLayout::FILL, 1,
+                            views::GridLayout::USE_PREF, 0, 0);
+      column_set->AddPaddingColumn(0, column_divider);
+      column_set->AddColumn(views::GridLayout::FILL, views::GridLayout::FILL, 1,
+                            views::GridLayout::USE_PREF, 0, 0);
+      break;
     case DOUBLE_BUTTON_COLUMN_SET:
       column_set->AddColumn(views::GridLayout::TRAILING,
                             views::GridLayout::CENTER,
@@ -269,61 +282,94 @@ void ManagePasswordsBubbleView::AutoSigninView::OnTimer() {
 // A view offering the user the ability to save credentials. Contains a
 // single ManagePasswordItemsView, along with a "Save Passwords" button,
 // a "Never" button and an "Edit" button to edit username field.
-class ManagePasswordsBubbleView::PendingView : public views::View,
-                                               public views::ButtonListener {
+class ManagePasswordsBubbleView::PendingView
+    : public views::View,
+      public views::ButtonListener,
+      public views::FocusChangeListener {
  public:
   explicit PendingView(ManagePasswordsBubbleView* parent);
   ~PendingView() override;
 
  private:
+  void CreateAndSetLayout();
   // views::ButtonListener:
   void ButtonPressed(views::Button* sender, const ui::Event& event) override;
+  // views::FocusChangeListener:
+  void OnWillChangeFocus(View* focused_before, View* focused_now) override;
+  void OnDidChangeFocus(View* focused_before, View* focused_now) override;
+  // views::View:
+  bool OnKeyPressed(const ui::KeyEvent& event) override;
 
   ManagePasswordsBubbleView* parent_;
 
   views::Button* edit_button_;
   views::Button* save_button_;
   views::Button* never_button_;
+  views::View* username_field_;
+  views::View* password_field_;
+
+  bool editing_;
 
   DISALLOW_COPY_AND_ASSIGN(PendingView);
 };
 
 ManagePasswordsBubbleView::PendingView::PendingView(
     ManagePasswordsBubbleView* parent)
-    : parent_(parent), edit_button_(nullptr) {
+    : parent_(parent),
+      edit_button_(nullptr),
+      save_button_(nullptr),
+      never_button_(nullptr),
+      username_field_(nullptr),
+      password_field_(nullptr),
+      editing_(false) {
+  CreateAndSetLayout();
+}
+
+void ManagePasswordsBubbleView::PendingView::CreateAndSetLayout() {
   views::GridLayout* layout = new views::GridLayout(this);
   layout->set_minimum_size(gfx::Size(kDesiredBubbleWidth, 0));
   SetLayoutManager(layout);
 
-  // Create the pending credential item, save button and refusal combobox.
-  ManagePasswordItemsView* item = nullptr;
-  if (!parent->model()->pending_password().username_value.empty()) {
-    item = new ManagePasswordItemsView(parent_->model(),
-                                       &parent->model()->pending_password());
-  }
-  if (base::FeatureList::IsEnabled(
+  // Create the edit button, save button and refusal
+  // combobox.

[vasilii, 2017/07/03 17:15:31]

There is no combobox for many years :)

[irmakk, 2017/07/06 10:42:46]

Done.

+  if (!edit_button_ &&
+      base::FeatureList::IsEnabled(
           password_manager::features::kEnableUsernameCorrection)) {
     edit_button_ = views::MdTextButton::CreateSecondaryUiButton(
         this, l10n_util::GetStringUTF16(IDS_PASSWORD_MANAGER_EDIT_BUTTON));
   }
-  save_button_ = views::MdTextButton::CreateSecondaryUiBlueButton(
-      this, l10n_util::GetStringUTF16(IDS_PASSWORD_MANAGER_SAVE_BUTTON));
-  never_button_ = views::MdTextButton::CreateSecondaryUiButton(
-      this,
-      l10n_util::GetStringUTF16(IDS_PASSWORD_MANAGER_BUBBLE_BLACKLIST_BUTTON));
-
-  BuildColumnSet(layout, SINGLE_VIEW_COLUMN_SET);
+  if (!save_button_) {
+    save_button_ = views::MdTextButton::CreateSecondaryUiBlueButton(
+        this, l10n_util::GetStringUTF16(IDS_PASSWORD_MANAGER_SAVE_BUTTON));
+  }
+  if (!never_button_) {
+    never_button_ = views::MdTextButton::CreateSecondaryUiButton(
+        this, l10n_util::GetStringUTF16(
+                  IDS_PASSWORD_MANAGER_BUBBLE_BLACKLIST_BUTTON));
+  }
 
-  // Credential row.
-  if (item) {
-    layout->StartRow(0, SINGLE_VIEW_COLUMN_SET);
-    layout->AddView(item);
+  // Credentials row.
+  BuildColumnSet(layout, DOUBLE_VIEW_COLUMN_SET);
+  if (!parent_->model()->pending_password().username_value.empty() ||
+      edit_button_) {
+    layout->StartRow(0, DOUBLE_VIEW_COLUMN_SET);
+    const autofill::PasswordForm* password_form =
+        &parent_->model()->pending_password();
+    if (editing_) {

[vasilii, 2017/07/03 17:15:31]

DCHECK that username_field_ is 0.

[irmakk, 2017/07/06 10:42:46]

Done.

+      username_field_ = GenerateUsernameEditable(*password_form).release();
+    } else {
+      username_field_ = GenerateUsernameLabel(*password_form).release();
+    }
+    if (!password_field_) {
+      password_field_ = GeneratePasswordLabel(*password_form).release();
+    }
+    layout->AddView(username_field_);
+    layout->AddView(password_field_);
     layout->AddPaddingRow(0,
                           ChromeLayoutProvider::Get()
                               ->GetInsetsMetric(views::INSETS_DIALOG_CONTENTS)
                               .bottom());
   }
-
   // Button row.
   ColumnSetType column_set_type =
       edit_button_ ? TRIPLE_BUTTON_COLUMN_SET : DOUBLE_BUTTON_COLUMN_SET;
@@ -335,7 +381,11 @@ ManagePasswordsBubbleView::PendingView::PendingView(
   layout->AddView(save_button_);
   layout->AddView(never_button_);
 
-  parent_->set_initially_focused_view(save_button_);
+  if (!parent_->initially_focused_view_) {
+    parent_->set_initially_focused_view(save_button_);

[vasilii, 2017/07/03 17:15:31]

I'd move it to the constructor.

[irmakk, 2017/07/06 10:42:46]

Done.

+  }
+
+  layout->Layout(this);
 }
 
 ManagePasswordsBubbleView::PendingView::~PendingView() {
@@ -346,6 +396,13 @@ void ManagePasswordsBubbleView::PendingView::ButtonPressed(
     const ui::Event& event) {
   // TODO(https://crbug.com/734965): Implement edit button logic.
   if (sender == edit_button_) {
+    edit_button_->SetEnabled(false);
+    editing_ = true;
+    this->RemoveChildView(username_field_);

[vasilii, 2017/07/03 17:15:31]

username_field_ = nullptr;

[irmakk, 2017/07/06 10:42:46]

Done.

+    CreateAndSetLayout();
+    this->GetFocusManager()->SetFocusedView(username_field_);
+    this->GetFocusManager()->AddFocusChangeListener(this);
+    parent_->SizeToContents();
     return;
   } else if (sender == save_button_) {

[vasilii, 2017/07/03 17:15:31]

No 'else' after 'return'

[irmakk, 2017/07/06 10:42:46]

Done.

     parent_->model()->OnSaveClicked();
@@ -362,6 +419,42 @@ void ManagePasswordsBubbleView::PendingView::ButtonPressed(
   parent_->CloseBubble();
 }
 
+void ManagePasswordsBubbleView::PendingView::OnWillChangeFocus(
+    View* focused_before,
+    View* focused_now) {
+  // Nothing to do here.
+}
+
+void ManagePasswordsBubbleView::PendingView::OnDidChangeFocus(
+    View* focused_before,
+    View* focused_now) {
+  if (editing_ && focused_before == username_field_) {
+    editing_ = false;
+    this->RemoveChildView(username_field_);
+    CreateAndSetLayout();
+    edit_button_->SetEnabled(true);
+    this->GetFocusManager()->SetFocusedView(save_button_);

[vasilii, 2017/07/03 17:15:31]

What happens if I focus another window? Does it steal focus from there?

[irmakk, 2017/07/06 10:42:46]

If I click anywhere outside of the bubble, the bubble closes and there is no
focus stealing.
If I use tab/esc/enter to change the focus, focus moves onto the save button.
If I do an alt+tab, bubble closes.

+    parent_->SizeToContents();
+    this->GetFocusManager()->RemoveFocusChangeListener(this);

[vasilii, 2017/07/03 17:15:31]

I'm concerned that we don't call this method in destructor. I'd
register/deregister in the constructor and destructor

[irmakk, 2017/07/06 10:42:46]

Tried that, but in the constructor when i try to get focus manager, it returns
null. I assume this is because the view is not fully constructed yet so the
focus manager is not ready.

[vasilii, 2017/07/06 11:59:32]

What if you user parent_->GetFocusManager()?

[irmakk, 2017/07/06 12:53:13]

Tried that too, it is also null. Since the bubble's initial status is the
PENDING_PASSWORD_STATUS, there isn't really a difference in their creation time
i think.

[vasilii, 2017/07/06 13:46:43]

Then just add RemoveFocusChangeListener to the destructor to be on the  safe
side. It's OK to unregister twice.

[irmakk, 2017/07/06 14:19:12]

Uhm. I tried, and the browser crashed upon destruction. I did a bit of research
and I think the reason to that is this line:

https://cs.chromium.org/chromium/src/base/observer_list.h?rcl=31d3264bf520dac...

Probably, by the time that focus manager actually handles the removing request,
the PendingView is already destructed and the check fails.

[vasilii, 2017/07/06 15:33:20]

It crashes because GetFocusManager() is nullptr

+  }
+}
+
+bool ManagePasswordsBubbleView::PendingView::OnKeyPressed(
+    const ui::KeyEvent& event) {
+  if (editing_ && (event.key_code() == ui::KeyboardCode::VKEY_RETURN ||
+                   event.key_code() == ui::KeyboardCode::VKEY_ESCAPE)) {
+    editing_ = false;
+    this->RemoveChildView(username_field_);

[vasilii, 2017/07/03 17:15:31]

username_field_ = nullptr

[irmakk, 2017/07/06 10:42:46]

Done.

+    CreateAndSetLayout();
+    edit_button_->SetEnabled(true);
+    this->GetFocusManager()->SetFocusedView(save_button_);
+    parent_->SizeToContents();
+    this->GetFocusManager()->RemoveFocusChangeListener(this);
+    return true;
+  }
+  return false;
+}
+
 // ManagePasswordsBubbleView::ManageView --------------------------------------
 
 // A view offering the user a list of their currently saved credentials