Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2960843002:
Edit button makes username editable in the password manager bubble. (Closed)
DescriptionEdit button makes username editable in the password manager bubble.
When user clicks on the edit button in the password manager bubble;
- Username becomes editable
- Focus is on editable field
- Edit button becomes disabled
While username is editable, if user hits tab/enter/esc keys;
- Username field becomes a label again
- Focus is on save button
- Edit button is enabled again
Known issue:
Since this cl doesn't contain any communication with the username in the background and currently only has the ui part of the project, edited username will be restored to its original state after editing is done. This problem will be solved with the next cl sending the information to back-end and communication with the manager itself.
BUG=734965
Review-Url: https://codereview.chromium.org/2960843002
Cr-Commit-Position: refs/heads/master@{#484635}
Committed: https://chromium.googlesource.com/chromium/src/+/2fd1706822e4bd350826788f822d9a738ffca034
Patch Set 1 : Implemented clickable edit button #Patch Set 2 : Implemented delegate logic #Patch Set 3 : Removed unnecessary function. #Patch Set 4 : Label/Editable conversion upon focus changes are implemented. #Patch Set 5 : Minor improvements. #Patch Set 6 : Merge with ToT #
Total comments: 30
Patch Set 7 : Moved all manage password items view functionality inside pending view. #Patch Set 8 : Reverted RowStatus enum and made label generators stand-alone. #Patch Set 9 : Minor fixes. #
Total comments: 32
Patch Set 10 : Moved standalone functions out of anonymous namespace & did some more fixing. #
Total comments: 10
Patch Set 11 : Updated comments. #Patch Set 12 : Added new helper function. #
Total comments: 6
Patch Set 13 : Renamed helper function and did forward declaration for view generator functions. #
Messages
Total messages: 36 (21 generated)
Description was changed from ========== Edit button makes username editable in the password manager bubble. Edit button makes username editable in the password manager bubble. Initialized edit_button_ in the constructor Initialized edit_button_ in the constructor Fixed format Added experiment checks Checking for experiment flags to add edit button. Removed unnecessary declaration. Fixed typo. Fixed crushing on edit button click Added edit button. Created feature tags. BUG= ========== to ========== Edit button makes username editable in the password manager bubble. When user clicks on the edit button in the password manager bubble, it makes the username editable. Upon click, the edit button is disabled and the focus gets set on the editable field. BUG=734965 ==========
Description was changed from ========== Edit button makes username editable in the password manager bubble. When user clicks on the edit button in the password manager bubble, it makes the username editable. Upon click, the edit button is disabled and the focus gets set on the editable field. BUG=734965 ========== to ========== Edit button makes username editable in the password manager bubble. When user clicks on the edit button in the password manager bubble; - Username becomes editable - Focus is on editable field - Edit button becomes disabled While username is editable, if user hits tab/enter/esc keys; - Username field becomes a label again - Focus is on save button - Edit button is enabled again Known issue: Since this cl doesn't contain any communication with the username in the background and currently only has the ui part of the project, edited username will be restored to its original state after editing is done. This problem will be solved with the next cl sending the information to back-end and communication with the manager itself. BUG=734965 ==========
irmakk@google.com changed reviewers: + vasilii@chromium.org
Implemented ui side of the editable username view.
irmakk@google.com changed reviewers: + melandory@chromium.org
The CQ bit was checked by vasilii@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
I suspect you didn't merge with the top of the tree.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: ios-device-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-xcode-...) ios-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...) mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...) mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
On 2017/06/30 08:51:12, vasilii wrote: > I suspect you didn't merge with the top of the tree. PTAL
The CQ bit was checked by vasilii@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
I feel like we introduce too much complexity. The current state with PendingView wrapping ManagePasswordItemsView is a border line. But with the new changes ManagePasswordItemsView handles too many things which it shouldn't. Let's move everything related into PendingView. ManagePasswordItemsView can handle only the grid of credentials in the Manage state. I'm sure the code will become way simpler. I'm sorry for the radical suggestion. It wasn't obvious to me from the beginning :-) https://codereview.chromium.org/2960843002/diff/100001/chrome/browser/ui/view... File chrome/browser/ui/views/passwords/manage_password_items_view.cc (right): https://codereview.chromium.org/2960843002/diff/100001/chrome/browser/ui/view... chrome/browser/ui/views/passwords/manage_password_items_view.cc:16: #include "chrome/browser/ui/views/passwords/manage_passwords_bubble_view.h" Remove https://codereview.chromium.org/2960843002/diff/100001/chrome/browser/ui/view... chrome/browser/ui/views/passwords/manage_password_items_view.cc:84: const ::autofill::PasswordForm& form) { While it's totally correct, we should be consistent in the file. Thus, :: everywhere for PasswordForm or never. I prefer the latter :) https://codereview.chromium.org/2960843002/diff/100001/chrome/browser/ui/view... chrome/browser/ui/views/passwords/manage_password_items_view.cc:87: editable->SetHorizontalAlignment(gfx::ALIGN_LEFT); I'm pretty sure that by default views::Textfield does the right thing. This line, however, handles the RTL languages wrong. Just remove it. https://codereview.chromium.org/2960843002/diff/100001/chrome/browser/ui/view... chrome/browser/ui/views/passwords/manage_password_items_view.cc:156: RowStatus row_status_; Should be private https://codereview.chromium.org/2960843002/diff/100001/chrome/browser/ui/view... chrome/browser/ui/views/passwords/manage_password_items_view.cc:238: } else { DCHECK_EQ(EDITING, row_status_)? https://codereview.chromium.org/2960843002/diff/100001/chrome/browser/ui/view... chrome/browser/ui/views/passwords/manage_password_items_view.cc:289: undo_link_ = nullptr; editable_ = nullptr; https://codereview.chromium.org/2960843002/diff/100001/chrome/browser/ui/view... chrome/browser/ui/views/passwords/manage_password_items_view.cc:293: void ManagePasswordItemsView::PasswordFormRow::OnWillChangeFocus( The definition order should match the declaration order. Same for the ManagePasswordItemsView's methods. https://codereview.chromium.org/2960843002/diff/100001/chrome/browser/ui/view... chrome/browser/ui/views/passwords/manage_password_items_view.cc:363: bool ManagePasswordItemsView::OnKeyPressed(const ui::KeyEvent& event) { How does it work? The edit doesn't handle ENTER and ESC, therefore, we got the event here? https://codereview.chromium.org/2960843002/diff/100001/chrome/browser/ui/view... chrome/browser/ui/views/passwords/manage_password_items_view.cc:382: GetFocusManager()->RemoveFocusChangeListener(row.get()); 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. https://codereview.chromium.org/2960843002/diff/100001/chrome/browser/ui/view... File chrome/browser/ui/views/passwords/manage_password_items_view.h (right): https://codereview.chromium.org/2960843002/diff/100001/chrome/browser/ui/view... chrome/browser/ui/views/passwords/manage_password_items_view.h:13: #include "ui/views/focus/focus_manager.h" Seems like it's not used in the header? https://codereview.chromium.org/2960843002/diff/100001/chrome/browser/ui/view... chrome/browser/ui/views/passwords/manage_password_items_view.h:26: class ManagePasswordItemsDelegate { I'd declare it outside of this class because it's not an internal thing. Also don't forget to declare a protected virtual destructor. https://codereview.chromium.org/2960843002/diff/100001/chrome/browser/ui/view... chrome/browser/ui/views/passwords/manage_password_items_view.h:31: enum RowStatus { DEFAULT, DELETED, EDITING }; "DEFAULT" isn't clear. I don't understand if it's username,password or username,password delete https://codereview.chromium.org/2960843002/diff/100001/chrome/browser/ui/view... chrome/browser/ui/views/passwords/manage_password_items_view.h:38: ManagePasswordItemsDelegate* manage_password_items_delegate = nullptr); I don't think we win anything with the default argument here. https://codereview.chromium.org/2960843002/diff/100001/chrome/browser/ui/view... chrome/browser/ui/views/passwords/manage_password_items_view.h:39: void SetRowStatusForPendingViewRow(RowStatus row_status); A blank line before please. https://codereview.chromium.org/2960843002/diff/100001/chrome/browser/ui/view... chrome/browser/ui/views/passwords/manage_password_items_view.h:46: bool OnKeyPressed(const ui::KeyEvent& event) override; Add a comment // View: https://codereview.chromium.org/2960843002/diff/100001/chrome/browser/ui/view... File chrome/browser/ui/views/passwords/manage_passwords_bubble_view.cc (right): https://codereview.chromium.org/2960843002/diff/100001/chrome/browser/ui/view... chrome/browser/ui/views/passwords/manage_passwords_bubble_view.cc:309: if (!parent->model()->pending_password().username_value.empty()) { Actually if it's empty then we should allow to edit it. So if your feature is enabled we should execute this block. https://codereview.chromium.org/2960843002/diff/100001/chrome/browser/ui/view... chrome/browser/ui/views/passwords/manage_passwords_bubble_view.cc:315: edit_button_ = views::MdTextButton::CreateSecondaryUiButton( Broken merge? https://codereview.chromium.org/2960843002/diff/100001/chrome/browser/ui/view... chrome/browser/ui/views/passwords/manage_passwords_bubble_view.cc:382: void ManagePasswordsBubbleView::PendingView::FocusedOnEditable() { We don't need this method. We can call SizeToContents when we switch to the editing mode which only happens on edit click. https://codereview.chromium.org/2960843002/diff/100001/chrome/browser/ui/view... chrome/browser/ui/views/passwords/manage_passwords_bubble_view.cc:388: GetFocusManager()->SetFocusedView(save_button_); What happens if you click something outside of the bubble? Let's say in the omnibox. Does this code steal the focus from there?
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
I have finished refactoring. Some of the comments became irrelevant, therefore I did not reply them. PTAL. https://codereview.chromium.org/2960843002/diff/100001/chrome/browser/ui/view... File chrome/browser/ui/views/passwords/manage_password_items_view.cc (right): https://codereview.chromium.org/2960843002/diff/100001/chrome/browser/ui/view... chrome/browser/ui/views/passwords/manage_password_items_view.cc:16: #include "chrome/browser/ui/views/passwords/manage_passwords_bubble_view.h" On 2017/06/30 15:28:05, vasilii wrote: > Remove Done. https://codereview.chromium.org/2960843002/diff/100001/chrome/browser/ui/view... chrome/browser/ui/views/passwords/manage_password_items_view.cc:84: const ::autofill::PasswordForm& form) { On 2017/06/30 15:28:06, vasilii wrote: > While it's totally correct, we should be consistent in the file. Thus, :: > everywhere for PasswordForm or never. I prefer the latter :) Done. https://codereview.chromium.org/2960843002/diff/100001/chrome/browser/ui/view... chrome/browser/ui/views/passwords/manage_password_items_view.cc:87: editable->SetHorizontalAlignment(gfx::ALIGN_LEFT); On 2017/06/30 15:28:05, vasilii wrote: > I'm pretty sure that by default views::Textfield does the right thing. This > line, however, handles the RTL languages wrong. > > Just remove it. Removing. But please see line #79, the username label sets its alignment to left also, do you think this inconsistency will cause any problems? https://codereview.chromium.org/2960843002/diff/100001/chrome/browser/ui/view... chrome/browser/ui/views/passwords/manage_password_items_view.cc:238: } else { On 2017/06/30 15:28:05, vasilii wrote: > DCHECK_EQ(EDITING, row_status_)? Done. https://codereview.chromium.org/2960843002/diff/100001/chrome/browser/ui/view... chrome/browser/ui/views/passwords/manage_password_items_view.cc:289: undo_link_ = nullptr; On 2017/06/30 15:28:06, vasilii wrote: > editable_ = nullptr; Done. https://codereview.chromium.org/2960843002/diff/100001/chrome/browser/ui/view... File chrome/browser/ui/views/passwords/manage_password_items_view.h (right): https://codereview.chromium.org/2960843002/diff/100001/chrome/browser/ui/view... chrome/browser/ui/views/passwords/manage_password_items_view.h:13: #include "ui/views/focus/focus_manager.h" On 2017/06/30 15:28:06, vasilii wrote: > Seems like it's not used in the header? Done. https://codereview.chromium.org/2960843002/diff/100001/chrome/browser/ui/view... chrome/browser/ui/views/passwords/manage_password_items_view.h:38: ManagePasswordItemsDelegate* manage_password_items_delegate = nullptr); On 2017/06/30 15:28:06, vasilii wrote: > I don't think we win anything with the default argument here. This constructor is also used for the update pending view, but we decided that our changes will only cover the pending view. I can send the delegate from update pending view as well but it is not used in that case so i thought it is better to set a default null ptr. See https://cs.chromium.org/chromium/src/chrome/browser/ui/views/passwords/manage... https://codereview.chromium.org/2960843002/diff/100001/chrome/browser/ui/view... chrome/browser/ui/views/passwords/manage_password_items_view.h:39: void SetRowStatusForPendingViewRow(RowStatus row_status); On 2017/06/30 15:28:07, vasilii wrote: > A blank line before please. Done. https://codereview.chromium.org/2960843002/diff/100001/chrome/browser/ui/view... chrome/browser/ui/views/passwords/manage_password_items_view.h:46: bool OnKeyPressed(const ui::KeyEvent& event) override; On 2017/06/30 15:28:06, vasilii wrote: > Add a comment // View: Done. https://codereview.chromium.org/2960843002/diff/100001/chrome/browser/ui/view... File chrome/browser/ui/views/passwords/manage_passwords_bubble_view.cc (right): https://codereview.chromium.org/2960843002/diff/100001/chrome/browser/ui/view... chrome/browser/ui/views/passwords/manage_passwords_bubble_view.cc:315: edit_button_ = views::MdTextButton::CreateSecondaryUiButton( On 2017/06/30 15:28:07, vasilii wrote: > Broken merge? Broken merge.
The CQ bit was checked by vasilii@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: ios-device-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-xcode-...)
How about the new screenshots? https://codereview.chromium.org/2960843002/diff/100001/chrome/browser/ui/view... File chrome/browser/ui/views/passwords/manage_password_items_view.cc (right): https://codereview.chromium.org/2960843002/diff/100001/chrome/browser/ui/view... chrome/browser/ui/views/passwords/manage_password_items_view.cc:87: editable->SetHorizontalAlignment(gfx::ALIGN_LEFT); On 2017/07/03 14:56:53, irmakk wrote: > On 2017/06/30 15:28:05, vasilii wrote: > > I'm pretty sure that by default views::Textfield does the right thing. This > > line, however, handles the RTL languages wrong. > > > > Just remove it. > > Removing. > But please see line #79, the username label sets its alignment to left also, do > you think this inconsistency will cause any problems? Label::SetHorizontalAlignment swaps it for RTL https://codereview.chromium.org/2960843002/diff/160001/chrome/browser/ui/view... File chrome/browser/ui/views/passwords/manage_password_items_view.cc (right): https://codereview.chromium.org/2960843002/diff/160001/chrome/browser/ui/view... chrome/browser/ui/views/passwords/manage_password_items_view.cc:38: std::unique_ptr<views::Label> GenerateUsernameLabel( You should move them out of the anonymous namespace https://codereview.chromium.org/2960843002/diff/160001/chrome/browser/ui/view... chrome/browser/ui/views/passwords/manage_password_items_view.cc:49: editable->SetText(GetDisplayUsername(form)); I don't think we should fill <no username> there. Thus, form.username_value should be used. https://codereview.chromium.org/2960843002/diff/160001/chrome/browser/ui/view... File chrome/browser/ui/views/passwords/manage_password_items_view.h (right): https://codereview.chromium.org/2960843002/diff/160001/chrome/browser/ui/view... chrome/browser/ui/views/passwords/manage_password_items_view.h:14: #include "ui/views/controls/textfield/textfield.h" Label and Textfield can be just forward declared. https://codereview.chromium.org/2960843002/diff/160001/chrome/browser/ui/view... chrome/browser/ui/views/passwords/manage_password_items_view.h:30: // * Present credentials the user may choose to save. Obsolete https://codereview.chromium.org/2960843002/diff/160001/chrome/browser/ui/view... chrome/browser/ui/views/passwords/manage_password_items_view.h:40: const autofill::PasswordForm* password_form); I think this is not used anymore. Probably there is other code which is dead now like some ColumnSets. We should clean it up. https://codereview.chromium.org/2960843002/diff/160001/chrome/browser/ui/view... File chrome/browser/ui/views/passwords/manage_passwords_bubble_view.cc (right): https://codereview.chromium.org/2960843002/diff/160001/chrome/browser/ui/view... chrome/browser/ui/views/passwords/manage_passwords_bubble_view.cc:334: // combobox. There is no combobox for many years :) https://codereview.chromium.org/2960843002/diff/160001/chrome/browser/ui/view... chrome/browser/ui/views/passwords/manage_passwords_bubble_view.cc:358: if (editing_) { DCHECK that username_field_ is 0. https://codereview.chromium.org/2960843002/diff/160001/chrome/browser/ui/view... chrome/browser/ui/views/passwords/manage_passwords_bubble_view.cc:385: parent_->set_initially_focused_view(save_button_); I'd move it to the constructor. https://codereview.chromium.org/2960843002/diff/160001/chrome/browser/ui/view... chrome/browser/ui/views/passwords/manage_passwords_bubble_view.cc:401: this->RemoveChildView(username_field_); username_field_ = nullptr; https://codereview.chromium.org/2960843002/diff/160001/chrome/browser/ui/view... chrome/browser/ui/views/passwords/manage_passwords_bubble_view.cc:407: } else if (sender == save_button_) { No 'else' after 'return' https://codereview.chromium.org/2960843002/diff/160001/chrome/browser/ui/view... chrome/browser/ui/views/passwords/manage_passwords_bubble_view.cc:436: this->GetFocusManager()->SetFocusedView(save_button_); What happens if I focus another window? Does it steal focus from there? https://codereview.chromium.org/2960843002/diff/160001/chrome/browser/ui/view... chrome/browser/ui/views/passwords/manage_passwords_bubble_view.cc:438: this->GetFocusManager()->RemoveFocusChangeListener(this); I'm concerned that we don't call this method in destructor. I'd register/deregister in the constructor and destructor https://codereview.chromium.org/2960843002/diff/160001/chrome/browser/ui/view... chrome/browser/ui/views/passwords/manage_passwords_bubble_view.cc:447: this->RemoveChildView(username_field_); username_field_ = nullptr
About the screenshots, we only moved the functionality from one class to another, so I didn't update the latest screenshots (which are at the comments of the bug). https://codereview.chromium.org/2960843002/diff/160001/chrome/browser/ui/view... File chrome/browser/ui/views/passwords/manage_password_items_view.cc (right): https://codereview.chromium.org/2960843002/diff/160001/chrome/browser/ui/view... chrome/browser/ui/views/passwords/manage_password_items_view.cc:38: std::unique_ptr<views::Label> GenerateUsernameLabel( On 2017/07/03 17:15:30, vasilii wrote: > You should move them out of the anonymous namespace Done. https://codereview.chromium.org/2960843002/diff/160001/chrome/browser/ui/view... chrome/browser/ui/views/passwords/manage_password_items_view.cc:49: editable->SetText(GetDisplayUsername(form)); On 2017/07/03 17:15:31, vasilii wrote: > I don't think we should fill <no username> there. Thus, form.username_value > should be used. Done. https://codereview.chromium.org/2960843002/diff/160001/chrome/browser/ui/view... File chrome/browser/ui/views/passwords/manage_passwords_bubble_view.cc (right): https://codereview.chromium.org/2960843002/diff/160001/chrome/browser/ui/view... chrome/browser/ui/views/passwords/manage_passwords_bubble_view.cc:334: // combobox. On 2017/07/03 17:15:31, vasilii wrote: > There is no combobox for many years :) Done. https://codereview.chromium.org/2960843002/diff/160001/chrome/browser/ui/view... chrome/browser/ui/views/passwords/manage_passwords_bubble_view.cc:358: if (editing_) { On 2017/07/03 17:15:31, vasilii wrote: > DCHECK that username_field_ is 0. Done. https://codereview.chromium.org/2960843002/diff/160001/chrome/browser/ui/view... chrome/browser/ui/views/passwords/manage_passwords_bubble_view.cc:385: parent_->set_initially_focused_view(save_button_); On 2017/07/03 17:15:31, vasilii wrote: > I'd move it to the constructor. Done. https://codereview.chromium.org/2960843002/diff/160001/chrome/browser/ui/view... chrome/browser/ui/views/passwords/manage_passwords_bubble_view.cc:401: this->RemoveChildView(username_field_); On 2017/07/03 17:15:31, vasilii wrote: > username_field_ = nullptr; Done. https://codereview.chromium.org/2960843002/diff/160001/chrome/browser/ui/view... chrome/browser/ui/views/passwords/manage_passwords_bubble_view.cc:407: } else if (sender == save_button_) { On 2017/07/03 17:15:31, vasilii wrote: > No 'else' after 'return' Done. https://codereview.chromium.org/2960843002/diff/160001/chrome/browser/ui/view... chrome/browser/ui/views/passwords/manage_passwords_bubble_view.cc:436: this->GetFocusManager()->SetFocusedView(save_button_); On 2017/07/03 17:15:31, vasilii wrote: > What happens if I focus another window? Does it steal focus from there? 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. https://codereview.chromium.org/2960843002/diff/160001/chrome/browser/ui/view... chrome/browser/ui/views/passwords/manage_passwords_bubble_view.cc:438: this->GetFocusManager()->RemoveFocusChangeListener(this); On 2017/07/03 17:15:31, vasilii wrote: > I'm concerned that we don't call this method in destructor. I'd > register/deregister in the constructor and destructor 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. https://codereview.chromium.org/2960843002/diff/160001/chrome/browser/ui/view... chrome/browser/ui/views/passwords/manage_passwords_bubble_view.cc:447: this->RemoveChildView(username_field_); On 2017/07/03 17:15:31, vasilii wrote: > username_field_ = nullptr Done.
The CQ bit was checked by vasilii@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Comments on manage_password_items_view.h aren't addressed. I realized that UpdatePendingView still uses ManagePasswordItemsView. It would be nice to fix in a follow-up CL
https://codereview.chromium.org/2960843002/diff/160001/chrome/browser/ui/view... File chrome/browser/ui/views/passwords/manage_passwords_bubble_view.cc (right): https://codereview.chromium.org/2960843002/diff/160001/chrome/browser/ui/view... chrome/browser/ui/views/passwords/manage_passwords_bubble_view.cc:438: this->GetFocusManager()->RemoveFocusChangeListener(this); On 2017/07/06 10:42:46, irmakk wrote: > On 2017/07/03 17:15:31, vasilii wrote: > > I'm concerned that we don't call this method in destructor. I'd > > register/deregister in the constructor and destructor > > 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. What if you user parent_->GetFocusManager()? https://codereview.chromium.org/2960843002/diff/180001/chrome/browser/ui/view... File chrome/browser/ui/views/passwords/manage_passwords_bubble_view.cc (right): https://codereview.chromium.org/2960843002/diff/180001/chrome/browser/ui/view... chrome/browser/ui/views/passwords/manage_passwords_bubble_view.cc:359: DCHECK_EQ(username_field_, nullptr); Seems like it's true for both editing_ and !editing_ https://codereview.chromium.org/2960843002/diff/180001/chrome/browser/ui/view... chrome/browser/ui/views/passwords/manage_passwords_bubble_view.cc:385: layout->Layout(this); Move it to where you switch the state. There is no point in calling Layout from the constructor https://codereview.chromium.org/2960843002/diff/180001/chrome/browser/ui/view... chrome/browser/ui/views/passwords/manage_passwords_bubble_view.cc:398: this->RemoveChildView(username_field_); Is there a good reason to use "this->"? https://codereview.chromium.org/2960843002/diff/180001/chrome/browser/ui/view... chrome/browser/ui/views/passwords/manage_passwords_bubble_view.cc:402: parent_->GetFocusManager()->AddFocusChangeListener(this); It's confusing because this->GetFocusManager() and parent_->GetFocusManager() are the same. Thus, just use GetFocusManager(). https://codereview.chromium.org/2960843002/diff/180001/chrome/browser/ui/view... chrome/browser/ui/views/passwords/manage_passwords_bubble_view.cc:452: this->GetFocusManager()->RemoveFocusChangeListener(this); This is a copy/paste from OnDidChangeFocus(). There should be one helper method doing this transition.
PTAL https://codereview.chromium.org/2960843002/diff/160001/chrome/browser/ui/view... File chrome/browser/ui/views/passwords/manage_passwords_bubble_view.cc (right): https://codereview.chromium.org/2960843002/diff/160001/chrome/browser/ui/view... chrome/browser/ui/views/passwords/manage_passwords_bubble_view.cc:438: this->GetFocusManager()->RemoveFocusChangeListener(this); On 2017/07/06 11:59:32, vasilii wrote: > On 2017/07/06 10:42:46, irmakk wrote: > > On 2017/07/03 17:15:31, vasilii wrote: > > > I'm concerned that we don't call this method in destructor. I'd > > > register/deregister in the constructor and destructor > > > > 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. > > What if you user parent_->GetFocusManager()? 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. https://codereview.chromium.org/2960843002/diff/180001/chrome/browser/ui/view... File chrome/browser/ui/views/passwords/manage_passwords_bubble_view.cc (right): https://codereview.chromium.org/2960843002/diff/180001/chrome/browser/ui/view... chrome/browser/ui/views/passwords/manage_passwords_bubble_view.cc:359: DCHECK_EQ(username_field_, nullptr); On 2017/07/06 11:59:32, vasilii wrote: > Seems like it's true for both editing_ and !editing_ Done. https://codereview.chromium.org/2960843002/diff/180001/chrome/browser/ui/view... chrome/browser/ui/views/passwords/manage_passwords_bubble_view.cc:385: layout->Layout(this); On 2017/07/06 11:59:32, vasilii wrote: > Move it to where you switch the state. There is no point in calling Layout from > the constructor Done. https://codereview.chromium.org/2960843002/diff/180001/chrome/browser/ui/view... chrome/browser/ui/views/passwords/manage_passwords_bubble_view.cc:398: this->RemoveChildView(username_field_); On 2017/07/06 11:59:32, vasilii wrote: > Is there a good reason to use "this->"? Done. https://codereview.chromium.org/2960843002/diff/180001/chrome/browser/ui/view... chrome/browser/ui/views/passwords/manage_passwords_bubble_view.cc:402: parent_->GetFocusManager()->AddFocusChangeListener(this); On 2017/07/06 11:59:32, vasilii wrote: > It's confusing because this->GetFocusManager() and parent_->GetFocusManager() > are the same. Thus, just use GetFocusManager(). Oh, i didnt know it was inherited. Removing, thanks. https://codereview.chromium.org/2960843002/diff/180001/chrome/browser/ui/view... chrome/browser/ui/views/passwords/manage_passwords_bubble_view.cc:452: this->GetFocusManager()->RemoveFocusChangeListener(this); On 2017/07/06 11:59:32, vasilii wrote: > This is a copy/paste from OnDidChangeFocus(). There should be one helper method > doing this transition. Done.
https://codereview.chromium.org/2960843002/diff/160001/chrome/browser/ui/view... File chrome/browser/ui/views/passwords/manage_password_items_view.h (right): https://codereview.chromium.org/2960843002/diff/160001/chrome/browser/ui/view... chrome/browser/ui/views/passwords/manage_password_items_view.h:14: #include "ui/views/controls/textfield/textfield.h" On 2017/07/03 17:15:31, vasilii wrote: > Label and Textfield can be just forward declared. Not addressed. https://codereview.chromium.org/2960843002/diff/160001/chrome/browser/ui/view... File chrome/browser/ui/views/passwords/manage_passwords_bubble_view.cc (right): https://codereview.chromium.org/2960843002/diff/160001/chrome/browser/ui/view... chrome/browser/ui/views/passwords/manage_passwords_bubble_view.cc:438: this->GetFocusManager()->RemoveFocusChangeListener(this); On 2017/07/06 12:53:13, irmakk wrote: > On 2017/07/06 11:59:32, vasilii wrote: > > On 2017/07/06 10:42:46, irmakk wrote: > > > On 2017/07/03 17:15:31, vasilii wrote: > > > > I'm concerned that we don't call this method in destructor. I'd > > > > register/deregister in the constructor and destructor > > > > > > 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. > > > > What if you user parent_->GetFocusManager()? > > 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. Then just add RemoveFocusChangeListener to the destructor to be on the safe side. It's OK to unregister twice. https://codereview.chromium.org/2960843002/diff/220001/chrome/browser/ui/view... File chrome/browser/ui/views/passwords/manage_passwords_bubble_view.cc (right): https://codereview.chromium.org/2960843002/diff/220001/chrome/browser/ui/view... chrome/browser/ui/views/passwords/manage_passwords_bubble_view.cc:303: void UpdateUsernameField(); Call it "ToggleEditingState" or something https://codereview.chromium.org/2960843002/diff/220001/chrome/browser/ui/view... chrome/browser/ui/views/passwords/manage_passwords_bubble_view.cc:360: DCHECK_EQ(username_field_, nullptr); DCHECK_EQ(nullptr, username_field_) or DCHECK(!username_field_) https://codereview.chromium.org/2960843002/diff/220001/chrome/browser/ui/view... chrome/browser/ui/views/passwords/manage_passwords_bubble_view.cc:450: GetFocusManager()->RemoveFocusChangeListener(this); I'd swap these lines to avoid unnecessary notification.
We had a discussion about adding edit button to the update pending state as well and in the end we decided to not touch that class at all, that is why I didn't move the functionality of update pending view. I can move it with a following cl and clean up the itemsview class if you want. https://codereview.chromium.org/2960843002/diff/160001/chrome/browser/ui/view... File chrome/browser/ui/views/passwords/manage_password_items_view.h (right): https://codereview.chromium.org/2960843002/diff/160001/chrome/browser/ui/view... chrome/browser/ui/views/passwords/manage_password_items_view.h:14: #include "ui/views/controls/textfield/textfield.h" On 2017/07/06 13:46:42, vasilii wrote: > On 2017/07/03 17:15:31, vasilii wrote: > > Label and Textfield can be just forward declared. > > Not addressed. Done. https://codereview.chromium.org/2960843002/diff/160001/chrome/browser/ui/view... chrome/browser/ui/views/passwords/manage_password_items_view.h:30: // * Present credentials the user may choose to save. On 2017/07/03 17:15:31, vasilii wrote: > Obsolete Done. https://codereview.chromium.org/2960843002/diff/160001/chrome/browser/ui/view... chrome/browser/ui/views/passwords/manage_password_items_view.h:40: const autofill::PasswordForm* password_form); On 2017/07/03 17:15:31, vasilii wrote: > I think this is not used anymore. Probably there is other code which is dead now > like some ColumnSets. We should clean it up. This is still used in the Update Pending View case. Though we can move that case inside the bubble view with the next cl and i'll remove this constructor then. https://codereview.chromium.org/2960843002/diff/160001/chrome/browser/ui/view... File chrome/browser/ui/views/passwords/manage_passwords_bubble_view.cc (right): https://codereview.chromium.org/2960843002/diff/160001/chrome/browser/ui/view... chrome/browser/ui/views/passwords/manage_passwords_bubble_view.cc:438: this->GetFocusManager()->RemoveFocusChangeListener(this); On 2017/07/06 13:46:43, vasilii wrote: > On 2017/07/06 12:53:13, irmakk wrote: > > On 2017/07/06 11:59:32, vasilii wrote: > > > On 2017/07/06 10:42:46, irmakk wrote: > > > > On 2017/07/03 17:15:31, vasilii wrote: > > > > > I'm concerned that we don't call this method in destructor. I'd > > > > > register/deregister in the constructor and destructor > > > > > > > > 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. > > > > > > What if you user parent_->GetFocusManager()? > > > > 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. > > Then just add RemoveFocusChangeListener to the destructor to be on the safe > side. It's OK to unregister twice. 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. https://codereview.chromium.org/2960843002/diff/220001/chrome/browser/ui/view... File chrome/browser/ui/views/passwords/manage_passwords_bubble_view.cc (right): https://codereview.chromium.org/2960843002/diff/220001/chrome/browser/ui/view... chrome/browser/ui/views/passwords/manage_passwords_bubble_view.cc:303: void UpdateUsernameField(); On 2017/07/06 13:46:43, vasilii wrote: > Call it "ToggleEditingState" or something Done. https://codereview.chromium.org/2960843002/diff/220001/chrome/browser/ui/view... chrome/browser/ui/views/passwords/manage_passwords_bubble_view.cc:360: DCHECK_EQ(username_field_, nullptr); On 2017/07/06 13:46:43, vasilii wrote: > DCHECK_EQ(nullptr, username_field_) or DCHECK(!username_field_) Done. https://codereview.chromium.org/2960843002/diff/220001/chrome/browser/ui/view... chrome/browser/ui/views/passwords/manage_passwords_bubble_view.cc:450: GetFocusManager()->RemoveFocusChangeListener(this); On 2017/07/06 13:46:43, vasilii wrote: > I'd swap these lines to avoid unnecessary notification. Done.
lgtm https://codereview.chromium.org/2960843002/diff/160001/chrome/browser/ui/view... File chrome/browser/ui/views/passwords/manage_passwords_bubble_view.cc (right): https://codereview.chromium.org/2960843002/diff/160001/chrome/browser/ui/view... chrome/browser/ui/views/passwords/manage_passwords_bubble_view.cc:438: this->GetFocusManager()->RemoveFocusChangeListener(this); On 2017/07/06 14:19:12, irmakk wrote: > On 2017/07/06 13:46:43, vasilii wrote: > > On 2017/07/06 12:53:13, irmakk wrote: > > > On 2017/07/06 11:59:32, vasilii wrote: > > > > On 2017/07/06 10:42:46, irmakk wrote: > > > > > On 2017/07/03 17:15:31, vasilii wrote: > > > > > > I'm concerned that we don't call this method in destructor. I'd > > > > > > register/deregister in the constructor and destructor > > > > > > > > > > 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. > > > > > > > > What if you user parent_->GetFocusManager()? > > > > > > 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. > > > > Then just add RemoveFocusChangeListener to the destructor to be on the safe > > side. It's OK to unregister twice. > > 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. It crashes because GetFocusManager() is nullptr
The CQ bit was checked by irmakk@google.com
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 240001, "attempt_start_ts": 1499355304110270,
"parent_rev": "7c1793f8422af75eba7e3fc7170d15423e809675", "commit_rev":
"2fd1706822e4bd350826788f822d9a738ffca034"}
Message was sent while issue was closed.
Description was changed from ========== Edit button makes username editable in the password manager bubble. When user clicks on the edit button in the password manager bubble; - Username becomes editable - Focus is on editable field - Edit button becomes disabled While username is editable, if user hits tab/enter/esc keys; - Username field becomes a label again - Focus is on save button - Edit button is enabled again Known issue: Since this cl doesn't contain any communication with the username in the background and currently only has the ui part of the project, edited username will be restored to its original state after editing is done. This problem will be solved with the next cl sending the information to back-end and communication with the manager itself. BUG=734965 ========== to ========== Edit button makes username editable in the password manager bubble. When user clicks on the edit button in the password manager bubble; - Username becomes editable - Focus is on editable field - Edit button becomes disabled While username is editable, if user hits tab/enter/esc keys; - Username field becomes a label again - Focus is on save button - Edit button is enabled again Known issue: Since this cl doesn't contain any communication with the username in the background and currently only has the ui part of the project, edited username will be restored to its original state after editing is done. This problem will be solved with the next cl sending the information to back-end and communication with the manager itself. BUG=734965 Review-Url: https://codereview.chromium.org/2960843002 Cr-Commit-Position: refs/heads/master@{#484635} Committed: https://chromium.googlesource.com/chromium/src/+/2fd1706822e4bd350826788f822d... ==========
Message was sent while issue was closed.
Committed patchset #13 (id:240001) as https://chromium.googlesource.com/chromium/src/+/2fd1706822e4bd350826788f822d... |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
