Refactoring Manage Passwords Bubble Code to exclude TabSpecificContentSettings

This CL refactors the Manage Passwords Bubble Code which previously depended on TabSpecificContentSettings to directly interact with ManagePasswordsController.

BUG=261628
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=238400

[markusheintz_, 2013-11-26 12:44:36]

LGTM

Only nit and naming comments.
Please fix them before submitting

https://codereview.chromium.org/87853004/diff/1/chrome/browser/ui/passwords/m...
File chrome/browser/ui/passwords/manage_passwords_bubble_model.h (right):

https://codereview.chromium.org/87853004/diff/1/chrome/browser/ui/passwords/m...
chrome/browser/ui/passwords/manage_passwords_bubble_model.h:58: 
nit: Unrelated change. Please remove this empty line.

https://codereview.chromium.org/87853004/diff/20001/chrome/browser/ui/passwor...
File chrome/browser/ui/passwords/manage_passwords_bubble_model.cc (right):

https://codereview.chromium.org/87853004/diff/20001/chrome/browser/ui/passwor...
chrome/browser/ui/passwords/manage_passwords_bubble_model.cc:40: if
(password_submitted_) {
nit: no need to add the {. Please remove for the sake of consistency since you
don't use them above.

https://codereview.chromium.org/87853004/diff/20001/chrome/browser/ui/passwor...
File chrome/browser/ui/passwords/manage_passwords_bubble_model.h (right):

https://codereview.chromium.org/87853004/diff/20001/chrome/browser/ui/passwor...
chrome/browser/ui/passwords/manage_passwords_bubble_model.h:45: void
AddOrRemovePassword(autofill::PasswordForm password_form, bool remove);
"Add" or "Remove" are two contradicting verbs that describe opposite actions.
This makes the method name very confusing and hard to understand. It is
discouraged to form method names like this. (There was a nice tot episode about
this, but a couldn't find it on go/tot so it's probably very old).

How about:
A) "OnPasswordAction"
B) "HandlePasswordAction"
C) "UpdatePassword"

as this seems to be a user input/ ui event handler.

https://codereview.chromium.org/87853004/diff/20001/chrome/browser/ui/passwor...
chrome/browser/ui/passwords/manage_passwords_bubble_model.h:58: 
nit: remove this empty line

https://codereview.chromium.org/87853004/diff/20001/chrome/browser/ui/passwor...
File chrome/browser/ui/passwords/manage_passwords_controller.cc (right):

https://codereview.chromium.org/87853004/diff/20001/chrome/browser/ui/passwor...
chrome/browser/ui/passwords/manage_passwords_controller.cc:74: if
(!details.is_in_page) {
nit: I guess we could could change this to:

if (details.is_in_page)
  return;

...

This way you don't have to indent the lines below. It also makes it easy to
read: If (condition is true) return and don't do anything. I found this pattern
frequently in the code and it was also a few times suggested to me during code
reviews.

https://codereview.chromium.org/87853004/diff/20001/chrome/browser/ui/passwor...
File chrome/browser/ui/passwords/manage_passwords_controller.h (right):

https://codereview.chromium.org/87853004/diff/20001/chrome/browser/ui/passwor...
chrome/browser/ui/passwords/manage_passwords_controller.h:18: class
ManagePasswordsController
ManagePasswordsController is a very generic name that could be more specific. I
mean what's the controller for? settings page, bubble, icon, maybe something
else?

Since I have a hard time suggesting a better one you can keep it for now.

Alternative name:
ManagePasswordsBubbleController to emphasis that this is for the passwords
bubble of the omnibox. Unfortunately this does not mention the omnibox icon.

We could also rethink the relation of the model and the controller. Let's
discuss this offline how to proceed here.

https://codereview.chromium.org/87853004/diff/20001/chrome/browser/ui/passwor...
chrome/browser/ui/passwords/manage_passwords_controller.h:93: void
OnPasswordAction();
nit: I'll leave this name change up to you but "UpdateUi" or
"UpdateBubbleAndIconVisibility" seems more appropriate here since this is what
the method does.

OnPasswordAction is misleading since it does not suggest that the visibility
state is UI elements is changed and a Navigation is no passwords event.

https://codereview.chromium.org/87853004/diff/20001/chrome/browser/ui/passwor...
chrome/browser/ui/passwords/manage_passwords_controller.h:112: bool
manage_passwords_bubble_needs_showing_;
It would be great to add a comment for this field. Thanks

https://codereview.chromium.org/87853004/diff/20001/chrome/browser/ui/views/p...
File chrome/browser/ui/views/passwords/manage_passwords_bubble_view.cc (right):

https://codereview.chromium.org/87853004/diff/20001/chrome/browser/ui/views/p...
chrome/browser/ui/views/passwords/manage_passwords_bubble_view.cc:26: 
Please remove these 3 lines. No need for special ASCII separators. Same below.

https://codereview.chromium.org/87853004/diff/20001/chrome/browser/ui/views/p...
chrome/browser/ui/views/passwords/manage_passwords_bubble_view.cc:48: 
Please remove these 3 lines. (see comment above)

https://codereview.chromium.org/87853004/diff/20001/chrome/browser/ui/views/p...
chrome/browser/ui/views/passwords/manage_passwords_bubble_view.cc:163: //
bubble; we do not need to clamp the password field width because
nit: s/; we/. We/

https://codereview.chromium.org/87853004/diff/20001/chrome/browser/ui/views/p...
chrome/browser/ui/views/passwords/manage_passwords_bubble_view.cc:167: const int
predefined_username_field_max_width =
bad name format: Please follow the Google C++ style guide for naming consts
since the Chromium style guide does not deviate here:
https://engdoc.corp.google.com/eng/doc/devguide/cpp/styleguide.shtml?cl=head#...

https://codereview.chromium.org/87853004/diff/20001/chrome/browser/ui/views/p...
chrome/browser/ui/views/passwords/manage_passwords_bubble_view.cc:169: const int
max_username_or_password_width =
bad name format: Please follow the Google C++ style guide for naming consts
since the Chromium style guide does not deviate here:
https://engdoc.corp.google.com/eng/doc/devguide/cpp/styleguide.shtml?cl=head#...

https://codereview.chromium.org/87853004/diff/20001/chrome/browser/ui/views/p...
chrome/browser/ui/views/passwords/manage_passwords_bubble_view.cc:172: const int
first_field_width = std::max(max_username_or_password_width,
bad name format: Please follow the Google C++ style guide for naming consts
since the Chromium style guide does not deviate here:
https://engdoc.corp.google.com/eng/doc/devguide/cpp/styleguide.shtml?cl=head#...

https://codereview.chromium.org/87853004/diff/20001/chrome/browser/ui/views/p...
chrome/browser/ui/views/passwords/manage_passwords_bubble_view.cc:176: const int
second_field_width = std::max(
bad name format: Please follow the Google C++ style guide for naming consts
since the Chromium style guide does not deviate here:
https://engdoc.corp.google.com/eng/doc/devguide/cpp/styleguide.shtml?cl=head#...

[npentrel, 2013-11-26 14:25:54]

https://codereview.chromium.org/87853004/diff/1/chrome/browser/ui/passwords/m...
File chrome/browser/ui/passwords/manage_passwords_bubble_model.h (right):

https://codereview.chromium.org/87853004/diff/1/chrome/browser/ui/passwords/m...
chrome/browser/ui/passwords/manage_passwords_bubble_model.h:58: 
On 2013/11/26 12:44:36, markusheintz_ wrote:
> nit: Unrelated change. Please remove this empty line.

Done.

https://codereview.chromium.org/87853004/diff/20001/chrome/browser/ui/passwor...
File chrome/browser/ui/passwords/manage_passwords_bubble_model.cc (right):

https://codereview.chromium.org/87853004/diff/20001/chrome/browser/ui/passwor...
chrome/browser/ui/passwords/manage_passwords_bubble_model.cc:40: if
(password_submitted_) {
On 2013/11/26 12:44:37, markusheintz_ wrote:
> nit: no need to add the {. Please remove for the sake of consistency since you
> don't use them above.

Done.

https://codereview.chromium.org/87853004/diff/20001/chrome/browser/ui/passwor...
File chrome/browser/ui/passwords/manage_passwords_bubble_model.h (right):

https://codereview.chromium.org/87853004/diff/20001/chrome/browser/ui/passwor...
chrome/browser/ui/passwords/manage_passwords_bubble_model.h:45: void
AddOrRemovePassword(autofill::PasswordForm password_form, bool remove);
On 2013/11/26 12:44:37, markusheintz_ wrote:
> "Add" or "Remove" are two contradicting verbs that describe opposite actions.
> This makes the method name very confusing and hard to understand. It is
> discouraged to form method names like this. (There was a nice tot episode
about
> this, but a couldn't find it on go/tot so it's probably very old).
> 
> How about:
> A) "OnPasswordAction"
> B) "HandlePasswordAction"
> C) "UpdatePassword"
> 
> as this seems to be a user input/ ui event handler.

Done.

https://codereview.chromium.org/87853004/diff/20001/chrome/browser/ui/passwor...
chrome/browser/ui/passwords/manage_passwords_bubble_model.h:58: 
On 2013/11/26 12:44:37, markusheintz_ wrote:
> nit: remove this empty line

Done.

https://codereview.chromium.org/87853004/diff/20001/chrome/browser/ui/passwor...
File chrome/browser/ui/passwords/manage_passwords_controller.cc (right):

https://codereview.chromium.org/87853004/diff/20001/chrome/browser/ui/passwor...
chrome/browser/ui/passwords/manage_passwords_controller.cc:74: if
(!details.is_in_page) {
On 2013/11/26 12:44:37, markusheintz_ wrote:
> nit: I guess we could could change this to:
> 
> if (details.is_in_page)
>   return;
> 
> ...
> 
> This way you don't have to indent the lines below. It also makes it easy to
> read: If (condition is true) return and don't do anything. I found this
pattern
> frequently in the code and it was also a few times suggested to me during code
> reviews. 

Done.

https://codereview.chromium.org/87853004/diff/20001/chrome/browser/ui/passwor...
File chrome/browser/ui/passwords/manage_passwords_controller.h (right):

https://codereview.chromium.org/87853004/diff/20001/chrome/browser/ui/passwor...
chrome/browser/ui/passwords/manage_passwords_controller.h:18: class
ManagePasswordsController
On 2013/11/26 12:44:37, markusheintz_ wrote:
> ManagePasswordsController is a very generic name that could be more specific.
I
> mean what's the controller for? settings page, bubble, icon, maybe something
> else?
> 
> Since I have a hard time suggesting a better one you can keep it for now.
> 
> Alternative name:
> ManagePasswordsBubbleController to emphasis that this is for the passwords
> bubble of the omnibox. Unfortunately this does not mention the omnibox icon.
> 
> We could also rethink the relation of the model and the controller. Let's
> discuss this offline how to proceed here.

Done.

https://codereview.chromium.org/87853004/diff/20001/chrome/browser/ui/passwor...
chrome/browser/ui/passwords/manage_passwords_controller.h:93: void
OnPasswordAction();
On 2013/11/26 12:44:37, markusheintz_ wrote:
> nit: I'll leave this name change up to you but "UpdateUi" or
> "UpdateBubbleAndIconVisibility" seems more appropriate here since this is what
> the method does.
> 
> OnPasswordAction is misleading since it does not suggest that the visibility
> state is UI elements is changed and a Navigation is no passwords event.

Done.

https://codereview.chromium.org/87853004/diff/20001/chrome/browser/ui/passwor...
chrome/browser/ui/passwords/manage_passwords_controller.h:112: bool
manage_passwords_bubble_needs_showing_;
On 2013/11/26 12:44:37, markusheintz_ wrote:
> It would be great to add a comment for this field. Thanks

Done.

https://codereview.chromium.org/87853004/diff/20001/chrome/browser/ui/views/p...
File chrome/browser/ui/views/passwords/manage_passwords_bubble_view.cc (right):

https://codereview.chromium.org/87853004/diff/20001/chrome/browser/ui/views/p...
chrome/browser/ui/views/passwords/manage_passwords_bubble_view.cc:163: //
bubble; we do not need to clamp the password field width because
On 2013/11/26 12:44:37, markusheintz_ wrote:
> nit: s/; we/. We/

Done.

https://codereview.chromium.org/87853004/diff/20001/chrome/browser/ui/views/p...
chrome/browser/ui/views/passwords/manage_passwords_bubble_view.cc:167: const int
predefined_username_field_max_width =
On 2013/11/26 12:44:37, markusheintz_ wrote:
> bad name format: Please follow the Google C++ style guide for naming consts
> since the Chromium style guide does not deviate here:
>
https://engdoc.corp.google.com/eng/doc/devguide/cpp/styleguide.shtml?cl=head#...

Not a compile time const.

https://codereview.chromium.org/87853004/diff/20001/chrome/browser/ui/views/p...
chrome/browser/ui/views/passwords/manage_passwords_bubble_view.cc:169: const int
max_username_or_password_width =
On 2013/11/26 12:44:37, markusheintz_ wrote:
> bad name format: Please follow the Google C++ style guide for naming consts
> since the Chromium style guide does not deviate here:
>
https://engdoc.corp.google.com/eng/doc/devguide/cpp/styleguide.shtml?cl=head#...

Not a compile time const.

https://codereview.chromium.org/87853004/diff/20001/chrome/browser/ui/views/p...
chrome/browser/ui/views/passwords/manage_passwords_bubble_view.cc:172: const int
first_field_width = std::max(max_username_or_password_width,
On 2013/11/26 12:44:37, markusheintz_ wrote:
> bad name format: Please follow the Google C++ style guide for naming consts
> since the Chromium style guide does not deviate here:
>
https://engdoc.corp.google.com/eng/doc/devguide/cpp/styleguide.shtml?cl=head#...

Not a compile time const.

https://codereview.chromium.org/87853004/diff/20001/chrome/browser/ui/views/p...
chrome/browser/ui/views/passwords/manage_passwords_bubble_view.cc:176: const int
second_field_width = std::max(
On 2013/11/26 12:44:37, markusheintz_ wrote:
> bad name format: Please follow the Google C++ style guide for naming consts
> since the Chromium style guide does not deviate here:
>
https://engdoc.corp.google.com/eng/doc/devguide/cpp/styleguide.shtml?cl=head#...

Not a compile time const.

[npentrel, 2013-11-26 14:33:31]

Hi Peter and Garrett, 

Could you please review the following files?

gcasto: chrome/browser/password_manager/password_manager.cc
pkasting: 
chrome/browser/ui/browser_tab_contents.cc
chrome/browser/ui/gtk/*
chrome/browser/ui/views/location_bar/location_bar_view.cc

Thanks,
Naomi

[Peter Kasting, 2013-11-26 20:01:34]

LGTM

[npentrel, 2013-12-02 18:53:08]

Hi Garrett,

Can you please have a look at this CL?

Thanks,
Naomi

[Garrett Casto, 2013-12-02 22:28:49]

LGTM

I like it.

https://codereview.chromium.org/87853004/diff/60001/chrome/browser/ui/views/p...
File chrome/browser/ui/views/passwords/manage_passwords_icon_view.cc (right):

https://codereview.chromium.org/87853004/diff/60001/chrome/browser/ui/views/p...
chrome/browser/ui/views/passwords/manage_passwords_icon_view.cc:29: &&
!location_bar_delegate_->GetToolbarModel()->input_in_progress());
You should be consistent about how you break lines with "&&"
(http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Boolea...).
Personally I would rename "manage_passwords_bubble_ui_controller" to something
shorter ("bubble_ui_controller" maybe?), but if you don't want to do that, I
would think that you would indent like

manage_passwords_bubble_ui_controller->
    manage_passwords_icon_to_be_show() &&
!location_bar_delegate...

https://codereview.chromium.org/87853004/diff/60001/chrome/browser/ui/views/p...
chrome/browser/ui/views/passwords/manage_passwords_icon_view.cc:43:
!ManagePasswordsBubbleView::IsShowing()) {
Break after "&&" here as well. So 

manage_passwords_bubble_ui_controller->
   manage_passwords_bubble_needs_showing() &&
visible() &&
...

[npentrel, 2013-12-03 10:00:24]

https://codereview.chromium.org/87853004/diff/60001/chrome/browser/ui/views/p...
File chrome/browser/ui/views/passwords/manage_passwords_icon_view.cc (right):

https://codereview.chromium.org/87853004/diff/60001/chrome/browser/ui/views/p...
chrome/browser/ui/views/passwords/manage_passwords_icon_view.cc:29: &&
!location_bar_delegate_->GetToolbarModel()->input_in_progress());
On 2013/12/02 22:28:49, Garrett Casto wrote:
> You should be consistent about how you break lines with "&&"
>
(http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Boolea...).
> Personally I would rename "manage_passwords_bubble_ui_controller" to something
> shorter ("bubble_ui_controller" maybe?), but if you don't want to do that, I
> would think that you would indent like
> 
> manage_passwords_bubble_ui_controller->
>     manage_passwords_icon_to_be_show() &&
> !location_bar_delegate...

Done.

https://codereview.chromium.org/87853004/diff/60001/chrome/browser/ui/views/p...
chrome/browser/ui/views/passwords/manage_passwords_icon_view.cc:43:
!ManagePasswordsBubbleView::IsShowing()) {
On 2013/12/02 22:28:49, Garrett Casto wrote:
> Break after "&&" here as well. So 
> 
> manage_passwords_bubble_ui_controller->
>    manage_passwords_bubble_needs_showing() &&
> visible() &&
> ...

Done.

[commit-bot: I haz the power, 2013-12-03 10:39:06]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/npentrel@chromium.org/87853004/100001

[commit-bot: I haz the power, 2013-12-03 12:09:12]

Retried try job too often on android_clang_dbg for step(s) slave_steps
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_cl...

[commit-bot: I haz the power, 2013-12-03 13:23:47]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/npentrel@chromium.org/87853004/120001

[commit-bot: I haz the power, 2013-12-03 15:21:45]

Change committed as 238400