Edit button makes username editable in the password manager bubble.

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

Index: chrome/browser/ui/views/passwords/manage_password_items_view.cc
diff --git a/chrome/browser/ui/views/passwords/manage_password_items_view.cc b/chrome/browser/ui/views/passwords/manage_password_items_view.cc
index 6ccdd2ccf2d92a7b38829bc03fbb36453b4dd270..a81cecdf5b274e9ca7f0bc9a9cccf43d48dde01c 100644
--- a/chrome/browser/ui/views/passwords/manage_password_items_view.cc
+++ b/chrome/browser/ui/views/passwords/manage_password_items_view.cc
@@ -13,6 +13,7 @@
 #include "chrome/browser/ui/passwords/manage_passwords_view_utils.h"
 #include "chrome/browser/ui/views/harmony/chrome_layout_provider.h"
 #include "chrome/browser/ui/views/harmony/chrome_typography.h"
+#include "chrome/browser/ui/views/passwords/manage_passwords_bubble_view.h"

[vasilii, 2017/06/30 15:28:05]

Remove

[irmakk, 2017/07/03 14:56:53]

Done.

 #include "chrome/grit/generated_resources.h"
 #include "ui/base/l10n/l10n_util.h"
 #include "ui/base/resource/resource_bundle.h"
@@ -22,6 +23,7 @@
 #include "ui/views/controls/label.h"
 #include "ui/views/controls/link.h"
 #include "ui/views/controls/link_listener.h"
+#include "ui/views/controls/textfield/textfield.h"
 #include "ui/views/layout/fill_layout.h"
 #include "ui/views/layout/grid_layout.h"
 
@@ -78,6 +80,14 @@ std::unique_ptr<views::Label> GenerateUsernameLabel(
   return label;
 }
 
+std::unique_ptr<views::Textfield> GenerateUsernameEditable(
+    const ::autofill::PasswordForm& form) {

[vasilii, 2017/06/30 15:28:06]

While it's totally correct, we should be consistent in the file. Thus, ::
everywhere for PasswordForm or never. I prefer the latter :)

[irmakk, 2017/07/03 14:56:54]

Done.

+  auto editable = base::MakeUnique<views::Textfield>();
+  editable->SetText(GetDisplayUsername(form));
+  editable->SetHorizontalAlignment(gfx::ALIGN_LEFT);

[vasilii, 2017/06/30 15:28:05]

I'm pretty sure that by default views::Textfield does the right thing. This
line, however, handles the RTL languages wrong.

Just remove it.

[irmakk, 2017/07/03 14:56:53]

Removing.
But please see line #79, the username label sets its alignment to left also, do
you think this inconsistency will cause any problems?

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

Label::SetHorizontalAlignment swaps it for RTL

+  return editable;
+}
+
 std::unique_ptr<views::Label> GeneratePasswordLabel(
     const autofill::PasswordForm& form) {
   base::string16 text =
@@ -132,8 +142,10 @@ std::unique_ptr<views::Link> GenerateUndoLink(views::LinkListener* listener) {
 
 // Manage credentials: stores credentials state and adds proper row to layout
 // based on credential state.
-class ManagePasswordItemsView::PasswordFormRow : public views::ButtonListener,
-                                                 public views::LinkListener {
+class ManagePasswordItemsView::PasswordFormRow
+    : public views::ButtonListener,
+      public views::LinkListener,
+      public views::FocusChangeListener {
  public:
   PasswordFormRow(ManagePasswordItemsView* host,
                   const autofill::PasswordForm* password_form,
@@ -141,6 +153,7 @@ class ManagePasswordItemsView::PasswordFormRow : public views::ButtonListener,
   ~PasswordFormRow() override = default;
 
   void AddRow(views::GridLayout* layout);
+  RowStatus row_status_;

[vasilii, 2017/06/30 15:28:06]

Should be private

 
   // Returns the fixed height for a row excluding padding. 0 means no fixed
   // height required.
@@ -156,6 +169,9 @@ class ManagePasswordItemsView::PasswordFormRow : public views::ButtonListener,
   void ButtonPressed(views::Button* sender, const ui::Event& event) override;
   // views::LinkListener:
   void LinkClicked(views::Link* source, int event_flags) override;
+  // views::FocusChangeListener:
+  void OnWillChangeFocus(View* focused_before, View* focused_now) override;
+  void OnDidChangeFocus(View* focused_before, View* focused_now) override;
 
   void ResetControls();
 
@@ -165,7 +181,7 @@ class ManagePasswordItemsView::PasswordFormRow : public views::ButtonListener,
   views::Link* undo_link_;
   views::ImageButton* delete_button_;
   const int fixed_height_;
-  bool deleted_;
+  views::Textfield* editable_;
 
   DISALLOW_COPY_AND_ASSIGN(PasswordFormRow);
 };
@@ -174,16 +190,17 @@ ManagePasswordItemsView::PasswordFormRow::PasswordFormRow(
     ManagePasswordItemsView* host,
     const autofill::PasswordForm* password_form,
     int fixed_height)
-    : host_(host),
+    : row_status_(RowStatus::DEFAULT),
+      host_(host),
       password_form_(password_form),
       undo_link_(nullptr),
       delete_button_(nullptr),
       fixed_height_(fixed_height),
-      deleted_(false) {}
+      editable_(nullptr) {}
 
 void ManagePasswordItemsView::PasswordFormRow::AddRow(
     views::GridLayout* layout) {
-  if (deleted_) {
+  if (row_status_ == RowStatus::DELETED) {
     AddUndoRow(layout);
   } else {
     AddCredentialsRow(layout);
@@ -214,9 +231,18 @@ void ManagePasswordItemsView::PasswordFormRow::AddCredentialsRow(
           : TWO_COLUMN_SET;
   BuildColumnSetIfNeeded(layout, column_set_id);
   layout->StartRow(0, column_set_id);
-  layout->AddView(GenerateUsernameLabel(*password_form_).release(), 1, 1,
-                  views::GridLayout::FILL, views::GridLayout::FILL,
-                  0, fixed_height_);
+  if (row_status_ == RowStatus::DEFAULT) {
+    layout->AddView(GenerateUsernameLabel(*password_form_).release(), 1, 1,
+                    views::GridLayout::FILL, views::GridLayout::FILL, 0,
+                    fixed_height_);
+  } else {

[vasilii, 2017/06/30 15:28:05]

DCHECK_EQ(EDITING, row_status_)?

[irmakk, 2017/07/03 14:56:54]

Done.

+    editable_ = GenerateUsernameEditable(*password_form_).release();
+    layout->AddView(editable_, 1, 1, views::GridLayout::FILL,
+                    views::GridLayout::FILL, 0, fixed_height_);
+    host_->GetFocusManager()->SetFocusedView(editable_);
+    host_->delegate_->FocusedOnEditable();
+    host_->GetFocusManager()->AddFocusChangeListener(this);
+  }
   layout->AddView(GeneratePasswordLabel(*password_form_).release(), 1, 1,
                   views::GridLayout::FILL, views::GridLayout::FILL,
                   0, fixed_height_);
@@ -247,15 +273,15 @@ void ManagePasswordItemsView::PasswordFormRow::AddUndoRow(
 void ManagePasswordItemsView::PasswordFormRow::ButtonPressed(
     views::Button* sender, const ui::Event& event) {
   DCHECK_EQ(delete_button_, sender);
-  deleted_ = true;
-  host_->NotifyPasswordFormStatusChanged(*password_form_, deleted_);
+  row_status_ = RowStatus::DELETED;
+  host_->NotifyPasswordFormStatusChanged(*password_form_, true);
 }
 
 void ManagePasswordItemsView::PasswordFormRow::LinkClicked(views::Link* sender,
                                                            int event_flags) {
   DCHECK_EQ(undo_link_, sender);
-  deleted_ = false;
-  host_->NotifyPasswordFormStatusChanged(*password_form_, deleted_);
+  row_status_ = RowStatus::DEFAULT;
+  host_->NotifyPasswordFormStatusChanged(*password_form_, false);
 }
 
 void ManagePasswordItemsView::PasswordFormRow::ResetControls() {
@@ -263,6 +289,21 @@ void ManagePasswordItemsView::PasswordFormRow::ResetControls() {
   undo_link_ = nullptr;

[vasilii, 2017/06/30 15:28:06]

editable_ = nullptr;

[irmakk, 2017/07/03 14:56:53]

Done.

 }
 
+// FocusChangeListener methods.
+void ManagePasswordItemsView::PasswordFormRow::OnWillChangeFocus(

[vasilii, 2017/06/30 15:28:06]

The definition order should match the declaration order.
Same for the ManagePasswordItemsView's methods.

+    View* focused_before,
+    View* focused_now) {
+  // Nothing to do here.
+}
+
+void ManagePasswordItemsView::PasswordFormRow::OnDidChangeFocus(
+    View* focused_before,
+    View* focused_now) {
+  if (focused_before == editable_) {
+    host_->SetRowStatusForPendingViewRow(RowStatus::DEFAULT);
+  }
+}
+
 // ManagePasswordItemsView
 ManagePasswordItemsView::ManagePasswordItemsView(
     ManagePasswordsBubbleModel* manage_passwords_bubble_model,
@@ -278,8 +319,10 @@ ManagePasswordItemsView::ManagePasswordItemsView(
 
 ManagePasswordItemsView::ManagePasswordItemsView(
     ManagePasswordsBubbleModel* manage_passwords_bubble_model,
-    const autofill::PasswordForm* password_form)
-    : model_(manage_passwords_bubble_model) {
+    const autofill::PasswordForm* password_form,
+    ManagePasswordItemsDelegate* manage_password_items_delegate)
+    : model_(manage_passwords_bubble_model),
+      delegate_(manage_password_items_delegate) {
   password_forms_rows_.push_back(
       base::MakeUnique<PasswordFormRow>(this, password_form, 0));
   AddRows();
@@ -316,3 +359,30 @@ void ManagePasswordItemsView::Refresh() {
   RemoveAllChildViews(true);
   AddRows();
 }
+
+bool ManagePasswordItemsView::OnKeyPressed(const ui::KeyEvent& event) {

[vasilii, 2017/06/30 15:28:05]

How does it work? The edit doesn't handle ENTER and ESC, therefore, we got the
event here?

+  // Handle key press if editable field is active, leave it to the parent
+  // otherwise.
+  if (password_manager::ui::PENDING_PASSWORD_STATE == model_->state() &&
+      password_forms_rows_[0]->row_status_ == RowStatus::EDITING &&
+      (event.key_code() == ui::KeyboardCode::VKEY_RETURN ||
+       event.key_code() == ui::KeyboardCode::VKEY_ESCAPE)) {
+    SetRowStatusForPendingViewRow(RowStatus::DEFAULT);
+    return true;
+  }
+  return false;
+}
+
+void ManagePasswordItemsView::SetRowStatusForPendingViewRow(
+    RowStatus row_status) {
+  DCHECK_EQ(password_manager::ui::PENDING_PASSWORD_STATE, model_->state());
+  const std::unique_ptr<PasswordFormRow>& row = password_forms_rows_[0];
+  if (row->row_status_ == RowStatus::EDITING &&
+      row_status == RowStatus::DEFAULT) {
+    GetFocusManager()->RemoveFocusChangeListener(row.get());

[vasilii, 2017/06/30 15:28:06]

I feel that it's wrong that it's not PasswordFormRow that unregisters itself.
PasswordFormRow should register once it enters the EDIT mode and unregister once
it leaves it or on destruction.

+    delegate_->FocusLostOnEditable();
+  }
+  row->row_status_ = row_status;
+  RemoveAllChildViews(true);
+  AddRows();
+}