Password bubble: Introduce a command to open the bubble.

Password bubble: ManagePasswordsIconView is now a BubbleIconView.

This CL introduces a new browser command to open the Manage Passwords bubble,
and converts the ManagePasswordsIconView class into a subclass of
BubbleIconView, which uses the new command to control the bubble's state.
This allows us to more easily test the view and the UI controller, as each
object's job is now more clearly defined (and the view is now doing a good
deal less work), and to independently verify that the command is doing the
right thing.

After this CL, we'll (finally!) have something approaching reasonable test
coverage for the core of the views code.

BUG=365678
TBR=cpu@chromium.org

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

[Mike West, 2014-04-22 14:18:21]

Hey Markus and Vaclav!

Can you take a look at this WIP CL? I have some linker errors locally that I'll
diagnose tomorrow, but I'd like to get this into your queue sooner rather than
later so you can take a look before I send it out for the various OWNERS who
I'll need approval from.

Thanks!

-mike

[Mike West, 2014-04-22 14:19:17]

On 2014/04/22 14:18:21, Mike West wrote:
> Hey Markus and Vaclav!
> 
> Can you take a look at this WIP CL? I have some linker errors locally that
I'll
> diagnose tomorrow, but I'd like to get this into your queue sooner rather than
> later so you can take a look before I send it out for the various OWNERS who
> I'll need approval from.
> 
> Thanks!
> 
> -mike

(This is based on https://codereview.chromium.org/230463002/, which means it
doesn't apply, which I forgot before I threw it at the bots. Oops!)

[vabr (Chromium), 2014-04-22 14:32:06]

https://codereview.chromium.org/246393004/diff/1/chrome/browser/ui/passwords/...
File chrome/browser/ui/passwords/manage_passwords_bubble.h (right):

https://codereview.chromium.org/246393004/diff/1/chrome/browser/ui/passwords/...
chrome/browser/ui/passwords/manage_passwords_bubble.h:30: virtual
~ManagePasswordsBubble();
Seems like the destructor is the only virtual method here (and this class does
not inherit from anywhere).
Why did you make the destructor virtual?

https://codereview.chromium.org/246393004/diff/1/chrome/browser/ui/views/pass...
File chrome/browser/ui/views/passwords/manage_passwords_icon_view.h (right):

https://codereview.chromium.org/246393004/diff/1/chrome/browser/ui/views/pass...
chrome/browser/ui/views/passwords/manage_passwords_icon_view.h:23: explicit
ManagePasswordsIconView(
You can drop the "explicit".

[Mike West, 2014-04-23 10:01:46]

I've added a simple test to ensure that the command is correctly tied to the
BrowserWindow method that needs to be called.

Mind taking (another) look, Markus/Vaclav? If neither of you see terrible
problems, I'll kick this out to UI OWNERs.

[vabr (Chromium), 2014-04-23 10:22:31]

LGTM, thanks for the test!

Vaclav

https://codereview.chromium.org/246393004/diff/30001/chrome/browser/ui/passwo...
File
chrome/browser/ui/passwords/manage_passwords_bubble_ui_controller_unittest.cc
(right):

https://codereview.chromium.org/246393004/diff/30001/chrome/browser/ui/passwo...
chrome/browser/ui/passwords/manage_passwords_bubble_ui_controller_unittest.cc:50:
show_bubble_count_++;
optional nit:
++show_bubble_count_ seems more appropriate, because you don't care about the
value of the statement, only about its side-effect (incrementing the counter).
Also prefix increment seems to be prevalent in the code (unless there is
actually the need to have the value of the statement be pre-increment).

[Mike West, 2014-04-23 10:59:12]

Thanks for taking a pass over the code, Vaclav.

Hello, OWNERS I'm just adding to this CL!

This change is the first step towards making ManagePasswordsIconView a slim,
trim BubbleIconView, as opposed to the confused muddle it is today. This CL
moves manage password bubble creation out to a browser-level command, and future
CLs will refactor the code so that the ManagePasswordsIconView is no longer
responsible for opening the bubble (via 'ShowBubbleIfNeeded').

cpu@: Can you review app/chrome_command_ids.h?

pkasting@: Can you take a look at browser/ui and
browser/ui/views/{frame,location_bar}?

markusheintz@: Would you review browser/ui/passwords and
browser/ui/views/passwords?

sky@: Would you mind stamping the change to TestBrowserWindow, assuming that the
change to BrowserWindow, et al. is acceptable?

Thanks all!

-mike

[sky, 2014-04-23 17:14:04]

https://codereview.chromium.org/246393004/diff/30001/chrome/browser/ui/browse...
File chrome/browser/ui/browser_window.h (right):

https://codereview.chromium.org/246393004/diff/30001/chrome/browser/ui/browse...
chrome/browser/ui/browser_window.h:239: virtual void
ShowManagePasswordsBubble(content::WebContents* contents) = 0;
Do you really need this on BrowserWindow? The views implementation at least
doesn't seem to need anything from Browser/BrowserView to service this. In other
words, could this be a standalone function in browser_dialogs?

[Mike West, 2014-04-23 17:35:21]

Thanks for taking a look!

https://codereview.chromium.org/246393004/diff/30001/chrome/browser/ui/browse...
File chrome/browser/ui/browser_window.h (right):

https://codereview.chromium.org/246393004/diff/30001/chrome/browser/ui/browse...
chrome/browser/ui/browser_window.h:239: virtual void
ShowManagePasswordsBubble(content::WebContents* contents) = 0;
On 2014/04/23 17:14:04, sky wrote:
> Do you really need this on BrowserWindow? The views implementation at least
> doesn't seem to need anything from Browser/BrowserView to service this. In
other
> words, could this be a standalone function in browser_dialogs?

I was following the Translate and Bookmark examples; that seemed to be the way
this sort of thing is done. If you'd prefer that I move the method off the
Window and into browser_dialogs, I'm happy to do so, but this seems more
consistent with existing code.

There's certainly no dependency on Browser to make this work, and I wouldn't
anticipate an eventual Cocoa implementation adding such a dependency.

[Peter Kasting, 2014-04-23 17:59:50]

On 2014/04/23 17:35:21, Mike West wrote:
> I was following the Translate and Bookmark examples; that seemed to be the way
> this sort of thing is done.

Assume those are wrong.

Almost everything that has to do with BrowserWindow, and most things that have
to do with browser_dialogs as well, have problems.

Indeed, it's not clear from the bug what motivates this change to begin with. 
"The views are doing too much work" in what sense?  That you need to share some
code with other platforms?  Something else?  Please provide more background
here, otherwise I'm going to be skeptical that a browser command is an
improvement.

[Mike West, 2014-04-24 07:30:38]

On 2014/04/23 17:59:50, Peter Kasting wrote:
> On 2014/04/23 17:35:21, Mike West wrote:
> > I was following the Translate and Bookmark examples; that seemed to be the
way
> > this sort of thing is done.
> 
> Assume those are wrong.

Hrm. :(

> Almost everything that has to do with BrowserWindow, and most things that have
> to do with browser_dialogs as well, have problems.

Are there any examples of bubbles that are doing things the way you'd like to
see them done?

> Indeed, it's not clear from the bug what motivates this change to begin with. 
> "The views are doing too much work" in what sense?  That you need to share
some
> code with other platforms?  Something else?  Please provide more background
> here, otherwise I'm going to be skeptical that a browser command is an
> improvement.

You're entirely correct. I should have been more clear in the bug.

Currently, the ManagePassword*View classes intermingle logic and presentation,
and have basically no test coverage. My main goal at the moment is to extract
logic into platform-agnostic bits of the platform so that I can write reasonable
tests. Right now, the ManagePasswordsIconView is responsible both for
hiding/showing a BubbleDelegateView, and for managing some of that bubble's
state (e.g. ManagePasswordsIconView::ShowBubbleIfNeeded). I'd like to strip the
ManagePasswordsIconView down to remove the latter responsibility completely, and
to delegate the former responsibility to something cross-platform.

Migrating to a BubbleIconView, as StarIconView and TranslateIconView have done,
looks like a clean way of building a minimal thing that is only responsible for
displaying UI in the Omnibox and sending a command to the browser in order to
delegate hiding/showing the bubble. The command structure would also make it
possible for the ManagePasswordsUIController to directly show the bubble,
removing the necessity for the ManagePasswordsIconView to understand anything
about whether or not the bubble should be automatically displayed without user
interaction.

Does the goal sound reasonable? Is moving to BubbleIconView a good way to get to
that goal? If so, this CL seems like the right first step.

Thanks!

[Peter Kasting, 2014-04-24 07:48:01]

Your description sounds like an overall improvement.

My biggest issue with a lot of the browser code is that we have a number of
cases where we've added global (or effectively global) functions to trigger
behaviors that we could have plumbed more directly.  Basically, a case of more
layers than we need, most seemingly just for consistency's sake with other
similar code.  Over time I've stripped out a few of these cases but I am always
suspicious of anything that sounds like more.  When I said "Assume those are
wrong", what I really meant was less "definitely don't do it that way" and more
"don't try to copy any existing code from anywhere; figure out what the simplest
and most maintainable implementation would be, in isolation".

As for using BubbleIconView, it's probably OK.  I don't think I've looked enough
at that code recently to know for sure.

[Mike West, 2014-04-24 08:10:03]

On 2014/04/24 07:48:01, Peter Kasting wrote:
> Your description sounds like an overall improvement.
> 
> My biggest issue with a lot of the browser code is that we have a number of
> cases where we've added global (or effectively global) functions to trigger
> behaviors that we could have plumbed more directly.  Basically, a case of more
> layers than we need, most seemingly just for consistency's sake with other
> similar code.  Over time I've stripped out a few of these cases but I am
always
> suspicious of anything that sounds like more.  When I said "Assume those are
> wrong", what I really meant was less "definitely don't do it that way" and
more
> "don't try to copy any existing code from anywhere; figure out what the
simplest
> and most maintainable implementation would be, in isolation".

OK, that makes sense. I agree that blind cargo-culting is not a great way to
improve the quality of the codebase, but it's also the case that following
existing conventions improves our ability to quickly understand code.

In any event, I think a command is probably appropriate here, but it's tough to
evaluate that just from this CL. Moreover, you and Scott are probably both
correct in pushing back against putting something on BrowserWindow. I'll take
another look at that piece.

> As for using BubbleIconView, it's probably OK.  I don't think I've looked
enough
> at that code recently to know for sure.

Why don't I flesh out this CL with the BubbleIconView implementation so that the
end state I'm aiming for is clear, and you can evaluate whether you're happy
with it. I'll probably have to land one or two other refactoring CLs to make
that possible first, so let's put this CL on ice for a day or so.

[Mike West, 2014-04-25 14:24:31]

Hi Peter, would you mind taking another look at this CL? I've fleshed out the
BubbleIconView transition, as well as started poking at the kinds of tests that
moving to the command makes more possible. This CL is still a bit raw, but I'd
appreciate your input while I clean it up on Monday.

Vaclav & Markus, please also take a look for the same reasons; the CL has
morphed quite a bit from the initial thing you reviewed. :)

Thanks!

-mike

[vabr (Chromium), 2014-04-25 15:26:16]

The current state of the CL makes sense to me.
A couple of nits and a sanity check below.

Cheers,
Vaclav

https://codereview.chromium.org/246393004/diff/70001/chrome/browser/ui/passwo...
File
chrome/browser/ui/passwords/manage_passwords_bubble_ui_controller_unittest.cc
(right):

https://codereview.chromium.org/246393004/diff/70001/chrome/browser/ui/passwo...
chrome/browser/ui/passwords/manage_passwords_bubble_ui_controller_unittest.cc:9:
#include "chrome/app/chrome_command_ids.h"
I'm a bit confused about these new includes -- are they already needed in the
original file, and just included indirectly until now, or did you forget to
either clean them up, or put in some new code?

(Changing the .cc to .h is obviously a plus, though!)

https://codereview.chromium.org/246393004/diff/70001/chrome/browser/ui/views/...
File chrome/browser/ui/views/passwords/manage_passwords_bubble_view.cc (right):

https://codereview.chromium.org/246393004/diff/70001/chrome/browser/ui/views/...
chrome/browser/ui/views/passwords/manage_passwords_bubble_view.cc:78: //
browser_dialog -------------------------------------------------------------
nit: This is not entirely clear to me -- it does not look like a class name, and
no similarly named variables are in sight. Please elaborate.

https://codereview.chromium.org/246393004/diff/70001/chrome/browser/ui/views/...
File chrome/browser/ui/views/passwords/manage_passwords_view_test.cc (right):

https://codereview.chromium.org/246393004/diff/70001/chrome/browser/ui/views/...
chrome/browser/ui/views/passwords/manage_passwords_view_test.cc:42: BrowserView*
browser_view = reinterpret_cast<BrowserView*>(
nit: This is casting a class to its subclass, and on the assumption that
window() actually is an instance of BrowserView, not reinterpreting the pointer.
By the same argument I used last time for casting the UIController, this should
be a static_cast.

https://codereview.chromium.org/246393004/diff/70001/chrome/browser/ui/views/...
File chrome/browser/ui/views/passwords/manage_passwords_view_test.h (right):

https://codereview.chromium.org/246393004/diff/70001/chrome/browser/ui/views/...
chrome/browser/ui/views/passwords/manage_passwords_view_test.h:34: void
ExecuteManagePasswordsCommand(Expectation expectation = EXPECT_SUCCESS);
optional nit: This read a bit strangely in the test above, when the
EXPECT_FAILURE was made explicit, and the EXPECT_SUCCESS was not mentioned.
I would suggest either
1) not using implicit arguments, and always write EXPECT_SUCCESS, or
2) (if the above would make lines too long) provide two versions of this method
(distinguished, e.g., by a short suffix -Success, -Error), which would
internally call the original method with the right argument.

https://codereview.chromium.org/246393004/diff/70001/chrome/chrome_tests.gypi
File chrome/chrome_tests.gypi (right):

https://codereview.chromium.org/246393004/diff/70001/chrome/chrome_tests.gypi...
chrome/chrome_tests.gypi:105:
'browser/ui/passwords/manage_passwords_bubble_ui_controller_mock.cc',
Is this file missing from the patch set?

[Peter Kasting, 2014-04-25 22:12:01]

https://codereview.chromium.org/246393004/diff/70001/chrome/browser/ui/passwo...
File
chrome/browser/ui/passwords/manage_passwords_bubble_ui_controller_unittest.cc
(right):

https://codereview.chromium.org/246393004/diff/70001/chrome/browser/ui/passwo...
chrome/browser/ui/passwords/manage_passwords_bubble_ui_controller_unittest.cc:7:
#include "base/strings/utf_string_conversions.h"
Woooooww.  Surprised this ever compiled...

https://codereview.chromium.org/246393004/diff/70001/chrome/browser/ui/views/...
File chrome/browser/ui/views/location_bar/location_bar_view.cc (right):

https://codereview.chromium.org/246393004/diff/70001/chrome/browser/ui/views/...
chrome/browser/ui/views/location_bar/location_bar_view.cc:393:
manage_passwords_icon_view_->SetState(ManagePasswordsIcon::INACTIVE_STATE);
Nit: This line doesn't seem necessary

https://codereview.chromium.org/246393004/diff/70001/chrome/browser/ui/views/...
File chrome/browser/ui/views/passwords/manage_passwords_bubble_view.cc (right):

https://codereview.chromium.org/246393004/diff/70001/chrome/browser/ui/views/...
chrome/browser/ui/views/passwords/manage_passwords_bubble_view.cc:77: 
Nit: One more newline here, and above the "ManagePasswordsBubbleView" divider
below

https://codereview.chromium.org/246393004/diff/70001/chrome/browser/ui/views/...
chrome/browser/ui/views/passwords/manage_passwords_bubble_view.cc:78: //
browser_dialog -------------------------------------------------------------
On 2014/04/25 15:26:17, vabr (Chromium) wrote:
> nit: This is not entirely clear to me -- it does not look like a class name,
and
> no similarly named variables are in sight. Please elaborate.

I'd probably call the divider "Globals" instead.

https://codereview.chromium.org/246393004/diff/70001/chrome/browser/ui/views/...
chrome/browser/ui/views/passwords/manage_passwords_bubble_view.cc:83:
ManagePasswordsBubbleUIController* controller =
Nit: Whole function should be indented 2, not 4

https://codereview.chromium.org/246393004/diff/70001/chrome/browser/ui/views/...
chrome/browser/ui/views/passwords/manage_passwords_bubble_view.cc:90: :
ManagePasswordsBubbleView::USER_ACTION);
Nit: Wrap/indent like:

  ManagePasswordsBubbleView::ShowBubble(
      web_contents,
      is_automatic_display ?
          ManagePasswordsBubbleView::AUTOMATIC :
          ManagePasswordsBubbleView::USER_ACTION);

This will also allow you to inline
"controller->manage_passwords_bubble_needs_showing()" into this and eliminate
the temp, without causing awkward wrapping issues.

https://codereview.chromium.org/246393004/diff/70001/chrome/browser/ui/views/...
File
chrome/browser/ui/views/passwords/manage_passwords_bubble_view_browsertest.cc
(right):

https://codereview.chromium.org/246393004/diff/70001/chrome/browser/ui/views/...
chrome/browser/ui/views/passwords/manage_passwords_bubble_view_browsertest.cc:16:
const char kDisplayDispositionMetric[] = "PasswordBubble.DisplayDisposition";
Nit: Place in anonymous namespace

https://codereview.chromium.org/246393004/diff/70001/chrome/browser/ui/views/...
chrome/browser/ui/views/passwords/manage_passwords_bubble_view_browsertest.cc:39:
CommandDisabledByDefault) {
Nit: Indent even

https://codereview.chromium.org/246393004/diff/70001/chrome/browser/ui/views/...
File chrome/browser/ui/views/passwords/manage_passwords_icon_view.cc (right):

https://codereview.chromium.org/246393004/diff/70001/chrome/browser/ui/views/...
chrome/browser/ui/views/passwords/manage_passwords_icon_view.cc:27: // call
would otherwise be deduped away.
This is confusing.  Your change must be built atop some other change that isn't
checked in, because I assume this refers to something in the ManagePasswordsIcon
class, which I can't find in ViewVC.

The issue is presumably that ManagePasswordsIcon() calls SetState() or
something, but because the subclass is not yet constructed, it doesn't wind up
calling SetStateInternal() here.  If so, consider moving the SetState() call to
an Init() method, or expecting subclasses to call it, or something.

https://codereview.chromium.org/246393004/diff/70001/chrome/browser/ui/views/...
chrome/browser/ui/views/passwords/manage_passwords_icon_view.cc:38:
IDC_MANAGE_PASSWORDS_FOR_PAGE, false);
Is it actually necessary to disable the command ever?  Unless the user can
trigger the command with a keyboard shortcut that needs to check the activation
state, or we switch UI based on the activation state, this command should just
be set to initially be enabled (we have a block that does this for many similar
browser commands) and never touched after this.

https://codereview.chromium.org/246393004/diff/70001/chrome/browser/ui/views/...
File chrome/browser/ui/views/passwords/manage_passwords_icon_view.h (right):

https://codereview.chromium.org/246393004/diff/70001/chrome/browser/ui/views/...
chrome/browser/ui/views/passwords/manage_passwords_icon_view.h:39: protected:
Nit: Newline above this

https://codereview.chromium.org/246393004/diff/70001/chrome/browser/ui/views/...
File chrome/browser/ui/views/passwords/manage_passwords_icon_view_browsertest.cc
(right):

https://codereview.chromium.org/246393004/diff/70001/chrome/browser/ui/views/...
chrome/browser/ui/views/passwords/manage_passwords_icon_view_browsertest.cc:14:
IN_PROC_BROWSER_TEST_F(ManagePasswordsIconViewTest, InactiveIsInvisible) {
Nit: Inactive -> Default?

https://codereview.chromium.org/246393004/diff/70001/chrome/browser/ui/views/...
File chrome/browser/ui/views/passwords/manage_passwords_view_test.cc (right):

https://codereview.chromium.org/246393004/diff/70001/chrome/browser/ui/views/...
chrome/browser/ui/views/passwords/manage_passwords_view_test.cc:42: BrowserView*
browser_view = reinterpret_cast<BrowserView*>(
On 2014/04/25 15:26:17, vabr (Chromium) wrote:
> nit: This is casting a class to its subclass, and on the assumption that
> window() actually is an instance of BrowserView, not reinterpreting the
pointer.
> By the same argument I used last time for casting the UIController, this
should
> be a static_cast.

Indeed, a cast between different points of an inheritance hierarchy must be a
static_cast so the pointer is adjusted properly if necessary; a reinterpret_cast
will do the wrong thing depending on your inheritance setup (e.g. with virtual
inheritance).

It probably "happens to work" here, but please fix.

https://codereview.chromium.org/246393004/diff/70001/chrome/browser/ui/views/...
chrome/browser/ui/views/passwords/manage_passwords_view_test.cc:45:
->location_bar()
Nit: Can go on previous line.

Also, put operators on ends of lines.

https://codereview.chromium.org/246393004/diff/70001/chrome/browser/ui/views/...
chrome/browser/ui/views/passwords/manage_passwords_view_test.cc:70:
password_manager::PasswordFormManager* test_form_manager =
If the call below takes ownership of this (not 100% clear from the class
comments... consider clarifying), prefer to make this local a scoped_ptr and
then call release() below.  This makes it clear you're intentionally
transferring ownership.  (In theory, it's more exception-safe too -- if we used
exceptions.)

https://codereview.chromium.org/246393004/diff/70001/chrome/browser/ui/views/...
File chrome/browser/ui/views/passwords/manage_passwords_view_test.h (right):

https://codereview.chromium.org/246393004/diff/70001/chrome/browser/ui/views/...
chrome/browser/ui/views/passwords/manage_passwords_view_test.h:31:
ManagePasswordsIconView* view() const;
Const methods must not return non-const pointers.

https://codereview.chromium.org/246393004/diff/70001/chrome/browser/ui/views/...
chrome/browser/ui/views/passwords/manage_passwords_view_test.h:42:
autofill::PasswordForm& test_form() { return test_form_; }
Nit: Don't return by non-const ref.  Return a const ref (and make the function
const) or return a pointer.

[Mike West, 2014-04-28 10:52:55]

Thanks Peter and Vaclav. Another pass is up.

Markus, it'd be great if you could take a look today as well! :)

-mike

https://codereview.chromium.org/246393004/diff/70001/chrome/browser/ui/passwo...
File
chrome/browser/ui/passwords/manage_passwords_bubble_ui_controller_unittest.cc
(right):

https://codereview.chromium.org/246393004/diff/70001/chrome/browser/ui/passwo...
chrome/browser/ui/passwords/manage_passwords_bubble_ui_controller_unittest.cc:7:
#include "base/strings/utf_string_conversions.h"
On 2014/04/25 22:12:02, Peter Kasting wrote:
> Woooooww.  Surprised this ever compiled...

Done.

https://codereview.chromium.org/246393004/diff/70001/chrome/browser/ui/passwo...
chrome/browser/ui/passwords/manage_passwords_bubble_ui_controller_unittest.cc:9:
#include "chrome/app/chrome_command_ids.h"
On 2014/04/25 15:26:17, vabr (Chromium) wrote:
> I'm a bit confused about these new includes -- are they already needed in the
> original file, and just included indirectly until now, or did you forget to
> either clean them up, or put in some new code?
> 
> (Changing the .cc to .h is obviously a plus, though!)

This was overzealous copy/pasta. I'll trim these headers down to size in the
next patchset.

https://codereview.chromium.org/246393004/diff/70001/chrome/browser/ui/views/...
File chrome/browser/ui/views/location_bar/location_bar_view.cc (right):

https://codereview.chromium.org/246393004/diff/70001/chrome/browser/ui/views/...
chrome/browser/ui/views/location_bar/location_bar_view.cc:393:
manage_passwords_icon_view_->SetState(ManagePasswordsIcon::INACTIVE_STATE);
On 2014/04/25 22:12:02, Peter Kasting wrote:
> Nit: This line doesn't seem necessary

Done.

https://codereview.chromium.org/246393004/diff/70001/chrome/browser/ui/views/...
File chrome/browser/ui/views/passwords/manage_passwords_bubble_view.cc (right):

https://codereview.chromium.org/246393004/diff/70001/chrome/browser/ui/views/...
chrome/browser/ui/views/passwords/manage_passwords_bubble_view.cc:77: 
On 2014/04/25 22:12:02, Peter Kasting wrote:
> Nit: One more newline here, and above the "ManagePasswordsBubbleView" divider
> below

Done.

https://codereview.chromium.org/246393004/diff/70001/chrome/browser/ui/views/...
chrome/browser/ui/views/passwords/manage_passwords_bubble_view.cc:78: //
browser_dialog -------------------------------------------------------------
On 2014/04/25 22:12:02, Peter Kasting wrote:
> On 2014/04/25 15:26:17, vabr (Chromium) wrote:
> > nit: This is not entirely clear to me -- it does not look like a class name,
> and
> > no similarly named variables are in sight. Please elaborate.
> 
> I'd probably call the divider "Globals" instead.

Done.

https://codereview.chromium.org/246393004/diff/70001/chrome/browser/ui/views/...
chrome/browser/ui/views/passwords/manage_passwords_bubble_view.cc:83:
ManagePasswordsBubbleUIController* controller =
On 2014/04/25 22:12:02, Peter Kasting wrote:
> Nit: Whole function should be indented 2, not 4

Done.

https://codereview.chromium.org/246393004/diff/70001/chrome/browser/ui/views/...
chrome/browser/ui/views/passwords/manage_passwords_bubble_view.cc:90: :
ManagePasswordsBubbleView::USER_ACTION);
On 2014/04/25 22:12:02, Peter Kasting wrote:
> Nit: Wrap/indent like:
> 
>   ManagePasswordsBubbleView::ShowBubble(
>       web_contents,
>       is_automatic_display ?
>           ManagePasswordsBubbleView::AUTOMATIC :
>           ManagePasswordsBubbleView::USER_ACTION);
> 
> This will also allow you to inline
> "controller->manage_passwords_bubble_needs_showing()" into this and eliminate
> the temp, without causing awkward wrapping issues.

Done.

https://codereview.chromium.org/246393004/diff/70001/chrome/browser/ui/views/...
File
chrome/browser/ui/views/passwords/manage_passwords_bubble_view_browsertest.cc
(right):

https://codereview.chromium.org/246393004/diff/70001/chrome/browser/ui/views/...
chrome/browser/ui/views/passwords/manage_passwords_bubble_view_browsertest.cc:16:
const char kDisplayDispositionMetric[] = "PasswordBubble.DisplayDisposition";
On 2014/04/25 22:12:02, Peter Kasting wrote:
> Nit: Place in anonymous namespace

Done.

https://codereview.chromium.org/246393004/diff/70001/chrome/browser/ui/views/...
chrome/browser/ui/views/passwords/manage_passwords_bubble_view_browsertest.cc:39:
CommandDisabledByDefault) {
On 2014/04/25 22:12:02, Peter Kasting wrote:
> Nit: Indent even

Done.

https://codereview.chromium.org/246393004/diff/70001/chrome/browser/ui/views/...
File chrome/browser/ui/views/passwords/manage_passwords_icon_view.cc (right):

https://codereview.chromium.org/246393004/diff/70001/chrome/browser/ui/views/...
chrome/browser/ui/views/passwords/manage_passwords_icon_view.cc:27: // call
would otherwise be deduped away.
On 2014/04/25 22:12:02, Peter Kasting wrote:
> This is confusing.  Your change must be built atop some other change that
isn't
> checked in, because I assume this refers to something in the
ManagePasswordsIcon
> class, which I can't find in ViewVC.

This CL is based on https://codereview.chromium.org/250353003/, which I did a
terrible job of making clear when asking for reviews. Sorry about that.

> The issue is presumably that ManagePasswordsIcon() calls SetState() or
> something, but because the subclass is not yet constructed, it doesn't wind up
> calling SetStateInternal() here.  If so, consider moving the SetState() call
to
> an Init() method, or expecting subclasses to call it, or something.

The issue is that the default value of the internal 'state_' variable is
INACTIVE_STATE, and the side-effects of setting the state are wrapped up in the
SetStateInternal call. I'll rename this internal method to something more
appropriate, perhaps |UpdateVisibleUI()|.

https://codereview.chromium.org/246393004/diff/70001/chrome/browser/ui/views/...
chrome/browser/ui/views/passwords/manage_passwords_icon_view.cc:38:
IDC_MANAGE_PASSWORDS_FOR_PAGE, false);
On 2014/04/25 22:12:02, Peter Kasting wrote:
> Is it actually necessary to disable the command ever?  Unless the user can
> trigger the command with a keyboard shortcut that needs to check the
activation
> state, or we switch UI based on the activation state, this command should just
> be set to initially be enabled (we have a block that does this for many
similar
> browser commands) and never touched after this.

Makes sense. There's no real need to disable the command in the current model;
the icon is the only way of triggering the UI, and the command is only called
via the icon. I'll drop this; we can always add it back in the future.

https://codereview.chromium.org/246393004/diff/70001/chrome/browser/ui/views/...
File chrome/browser/ui/views/passwords/manage_passwords_icon_view.h (right):

https://codereview.chromium.org/246393004/diff/70001/chrome/browser/ui/views/...
chrome/browser/ui/views/passwords/manage_passwords_icon_view.h:39: protected:
On 2014/04/25 22:12:02, Peter Kasting wrote:
> Nit: Newline above this

Done.

https://codereview.chromium.org/246393004/diff/70001/chrome/browser/ui/views/...
File chrome/browser/ui/views/passwords/manage_passwords_icon_view_browsertest.cc
(right):

https://codereview.chromium.org/246393004/diff/70001/chrome/browser/ui/views/...
chrome/browser/ui/views/passwords/manage_passwords_icon_view_browsertest.cc:14:
IN_PROC_BROWSER_TEST_F(ManagePasswordsIconViewTest, InactiveIsInvisible) {
On 2014/04/25 22:12:02, Peter Kasting wrote:
> Nit: Inactive -> Default?

Done.

https://codereview.chromium.org/246393004/diff/70001/chrome/browser/ui/views/...
File chrome/browser/ui/views/passwords/manage_passwords_view_test.cc (right):

https://codereview.chromium.org/246393004/diff/70001/chrome/browser/ui/views/...
chrome/browser/ui/views/passwords/manage_passwords_view_test.cc:42: BrowserView*
browser_view = reinterpret_cast<BrowserView*>(
On 2014/04/25 22:12:02, Peter Kasting wrote:
> On 2014/04/25 15:26:17, vabr (Chromium) wrote:
> > nit: This is casting a class to its subclass, and on the assumption that
> > window() actually is an instance of BrowserView, not reinterpreting the
> pointer.
> > By the same argument I used last time for casting the UIController, this
> should
> > be a static_cast.
> 
> Indeed, a cast between different points of an inheritance hierarchy must be a
> static_cast so the pointer is adjusted properly if necessary; a
reinterpret_cast
> will do the wrong thing depending on your inheritance setup (e.g. with virtual
> inheritance).
> 
> It probably "happens to work" here, but please fix.

Done.

https://codereview.chromium.org/246393004/diff/70001/chrome/browser/ui/views/...
chrome/browser/ui/views/passwords/manage_passwords_view_test.cc:45:
->location_bar()
On 2014/04/25 22:12:02, Peter Kasting wrote:
> Nit: Can go on previous line.
> 
> Also, put operators on ends of lines.

Done.

https://codereview.chromium.org/246393004/diff/70001/chrome/browser/ui/views/...
File chrome/browser/ui/views/passwords/manage_passwords_view_test.h (right):

https://codereview.chromium.org/246393004/diff/70001/chrome/browser/ui/views/...
chrome/browser/ui/views/passwords/manage_passwords_view_test.h:31:
ManagePasswordsIconView* view() const;
On 2014/04/25 22:12:02, Peter Kasting wrote:
> Const methods must not return non-const pointers.

Done.

https://codereview.chromium.org/246393004/diff/70001/chrome/browser/ui/views/...
chrome/browser/ui/views/passwords/manage_passwords_view_test.h:42:
autofill::PasswordForm& test_form() { return test_form_; }
On 2014/04/25 22:12:02, Peter Kasting wrote:
> Nit: Don't return by non-const ref.  Return a const ref (and make the function
> const) or return a pointer.

Done.

https://codereview.chromium.org/246393004/diff/70001/chrome/chrome_tests.gypi
File chrome/chrome_tests.gypi (right):

https://codereview.chromium.org/246393004/diff/70001/chrome/chrome_tests.gypi...
chrome/chrome_tests.gypi:105:
'browser/ui/passwords/manage_passwords_bubble_ui_controller_mock.cc',
On 2014/04/25 15:26:17, vabr (Chromium) wrote:
> Is this file missing from the patch set?

Included in the previous patch: https://codereview.chromium.org/250353003/

[vabr (Chromium), 2014-04-28 12:59:34]

Still LGTM, with one comment.

Cheers,
Vaclav

https://codereview.chromium.org/246393004/diff/70001/chrome/chrome_tests.gypi
File chrome/chrome_tests.gypi (right):

https://codereview.chromium.org/246393004/diff/70001/chrome/chrome_tests.gypi...
chrome/chrome_tests.gypi:105:
'browser/ui/passwords/manage_passwords_bubble_ui_controller_mock.cc',
On 2014/04/28 10:52:56, Mike West wrote:
> On 2014/04/25 15:26:17, vabr (Chromium) wrote:
> > Is this file missing from the patch set?
> 
> Included in the previous patch: https://codereview.chromium.org/250353003/

Ah, you meant https://codereview.chromium.org/232753002. :)
Did you also mean to add the header file for that?

Also, did you consider pulling that up into the test_support_common target in
chrome_tests_unit.gypi, which is currently a dependency of both
interactive_ui_tests here, and unit_tests there (which both contain the mock
controller)?

[Peter Kasting, 2014-04-28 20:07:14]

LGTM

https://codereview.chromium.org/246393004/diff/70001/chrome/browser/ui/views/...
File chrome/browser/ui/views/passwords/manage_passwords_bubble_view.cc (right):

https://codereview.chromium.org/246393004/diff/70001/chrome/browser/ui/views/...
chrome/browser/ui/views/passwords/manage_passwords_bubble_view.cc:77: 
On 2014/04/28 10:52:56, Mike West wrote:
> On 2014/04/25 22:12:02, Peter Kasting wrote:
> > Nit: One more newline here, and above the "ManagePasswordsBubbleView"
divider
> > below
> 
> Done.

Doesn't look like this actually happened.

https://codereview.chromium.org/246393004/diff/70001/chrome/browser/ui/views/...
chrome/browser/ui/views/passwords/manage_passwords_bubble_view.cc:90: :
ManagePasswordsBubbleView::USER_ACTION);
On 2014/04/28 10:52:56, Mike West wrote:
> On 2014/04/25 22:12:02, Peter Kasting wrote:
> > Nit: Wrap/indent like:
> > 
> >   ManagePasswordsBubbleView::ShowBubble(
> >       web_contents,
> >       is_automatic_display ?
> >           ManagePasswordsBubbleView::AUTOMATIC :
> >           ManagePasswordsBubbleView::USER_ACTION);
> > 
> > This will also allow you to inline
> > "controller->manage_passwords_bubble_needs_showing()" into this and
eliminate
> > the temp, without causing awkward wrapping issues.
> 
> Done.

The wrapping is still wrong here.  Operators go on the ends of lines, not their
start.

https://codereview.chromium.org/246393004/diff/110001/chrome/browser/ui/brows...
File chrome/browser/ui/browser_dialogs.h (right):

https://codereview.chromium.org/246393004/diff/110001/chrome/browser/ui/brows...
chrome/browser/ui/browser_dialogs.h:114: content::WebContents* web_contents);
Nit: Don't move this block down here just to share the #if.  The first function
here makes sense where it was, next to ShowCreateChromeAppShortcutsDialog(), and
the views implementation of these functions is also in the old order.

If you're desperate to avoid having two #ifs, you could in theory add your new
function _above_ ShowCreateWebAppShortcutsDialog(), with both in the middle of
the file.

https://codereview.chromium.org/246393004/diff/110001/chrome/browser/ui/views...
File chrome/browser/ui/views/passwords/manage_passwords_bubble_view.cc (right):

https://codereview.chromium.org/246393004/diff/110001/chrome/browser/ui/views...
chrome/browser/ui/views/passwords/manage_passwords_bubble_view.cc:82: // Defined
in 'chrome/browser/ui/browser_dialogs.h'.
Nit: I'd omit this; if you want to keep it, change "Defined" to "Declared" and
remove the single quotes.

https://codereview.chromium.org/246393004/diff/110001/chrome/browser/ui/views...
File chrome/browser/ui/views/passwords/manage_passwords_icon_view.h (right):

https://codereview.chromium.org/246393004/diff/110001/chrome/browser/ui/views...
chrome/browser/ui/views/passwords/manage_passwords_icon_view.h:41: // for
testing.
Nit: Since these are defined here in the header, you can enforce this by
surrounding them with #if defined(UNIT_TEST) (and then remove the now-redundant
comment).

https://codereview.chromium.org/246393004/diff/110001/chrome/browser/ui/views...
File chrome/browser/ui/views/passwords/manage_passwords_icon_view_browsertest.cc
(right):

https://codereview.chromium.org/246393004/diff/110001/chrome/browser/ui/views...
chrome/browser/ui/views/passwords/manage_passwords_icon_view_browsertest.cc:16:
IN_PROC_BROWSER_TEST_F(ManagePasswordsIconViewTest, DefaultStateIsInavtive) {
Nit: Inavtive -> Inactive

[Mike West, 2014-04-29 06:47:18]

Thanks Peter!

Markus, friendly ping.

https://codereview.chromium.org/246393004/diff/70001/chrome/chrome_tests.gypi
File chrome/chrome_tests.gypi (right):

https://codereview.chromium.org/246393004/diff/70001/chrome/chrome_tests.gypi...
chrome/chrome_tests.gypi:105:
'browser/ui/passwords/manage_passwords_bubble_ui_controller_mock.cc',
On 2014/04/28 12:59:35, vabr (Chromium) wrote:
> On 2014/04/28 10:52:56, Mike West wrote:
> > On 2014/04/25 15:26:17, vabr (Chromium) wrote:
> > > Is this file missing from the patch set?
> > 
> > Included in the previous patch: https://codereview.chromium.org/250353003/
> 
> Ah, you meant https://codereview.chromium.org/232753002. :)
> Did you also mean to add the header file for that?
> 
> Also, did you consider pulling that up into the test_support_common target in
> chrome_tests_unit.gypi, which is currently a dependency of both
> interactive_ui_tests here, and unit_tests there (which both contain the mock
> controller)?

Makes sense to me. Done and done.

https://codereview.chromium.org/246393004/diff/110001/chrome/browser/ui/brows...
File chrome/browser/ui/browser_dialogs.h (right):

https://codereview.chromium.org/246393004/diff/110001/chrome/browser/ui/brows...
chrome/browser/ui/browser_dialogs.h:114: content::WebContents* web_contents);
On 2014/04/28 20:07:15, Peter Kasting wrote:
> Nit: Don't move this block down here just to share the #if.  The first
function
> here makes sense where it was, next to ShowCreateChromeAppShortcutsDialog(),
and
> the views implementation of these functions is also in the old order.
> 
> If you're desperate to avoid having two #ifs, you could in theory add your new
> function _above_ ShowCreateWebAppShortcutsDialog(), with both in the middle of
> the file.

Done.

https://codereview.chromium.org/246393004/diff/110001/chrome/browser/ui/views...
File chrome/browser/ui/views/passwords/manage_passwords_icon_view.h (right):

https://codereview.chromium.org/246393004/diff/110001/chrome/browser/ui/views...
chrome/browser/ui/views/passwords/manage_passwords_icon_view.h:41: // for
testing.
On 2014/04/28 20:07:15, Peter Kasting wrote:
> Nit: Since these are defined here in the header, you can enforce this by
> surrounding them with #if defined(UNIT_TEST) (and then remove the
now-redundant
> comment).

Done.

https://codereview.chromium.org/246393004/diff/110001/chrome/browser/ui/views...
File chrome/browser/ui/views/passwords/manage_passwords_icon_view_browsertest.cc
(right):

https://codereview.chromium.org/246393004/diff/110001/chrome/browser/ui/views...
chrome/browser/ui/views/passwords/manage_passwords_icon_view_browsertest.cc:16:
IN_PROC_BROWSER_TEST_F(ManagePasswordsIconViewTest, DefaultStateIsInavtive) {
On 2014/04/28 20:07:15, Peter Kasting wrote:
> Nit: Inavtive -> Inactive

Done.

[markusheintz_, 2014-04-29 08:01:35]

LGTM

[markusheintz_, 2014-04-29 08:09:16]

LGTM

[Mike West, 2014-04-29 08:16:03]

The CQ bit was checked by mkwst@chromium.org

[commit-bot: I haz the power, 2014-04-29 08:16:50]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/mkwst@chromium.org/246393004/130001

[commit-bot: I haz the power, 2014-04-29 08:29:34]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-04-29 08:29:34]

Try jobs failed on following builders:
  chromium_presubmit on tryserver.chromium

[Mike West, 2014-04-29 08:37:41]

On 2014/04/29 08:29:34, I haz the power (commit-bot) wrote:
> Try jobs failed on following builders:
>   chromium_presubmit on tryserver.chromium

cpu@: TBRing you for chrome/app/chrome_command_ids.h.

[Mike West, 2014-04-29 08:37:46]

The CQ bit was checked by mkwst@chromium.org

[commit-bot: I haz the power, 2014-04-29 08:38:35]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/mkwst@chromium.org/246393004/130001

[commit-bot: I haz the power, 2014-04-29 10:28:25]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-04-29 10:28:26]

Try jobs failed on following builders:
  win_chromium_rel on tryserver.chromium

[Mike West, 2014-04-29 10:58:55]

The CQ bit was checked by mkwst@chromium.org

[commit-bot: I haz the power, 2014-04-29 10:59:03]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/mkwst@chromium.org/246393004/130001

[commit-bot: I haz the power, 2014-04-29 12:39:48]

Change committed as 266859

[Michael Achenbach, 2014-04-29 13:10:34]

A revert of this CL has been created in
https://codereview.chromium.org/256333003/ by machenbach@chromium.org.

The reason for reverting is: This is causing leaks on linux asan lsan:
http://build.chromium.org/p/chromium.memory/builders/Linux%20ASan%20LSan%20Te....

[Mike West, 2014-04-30 07:45:46]

Vaclav, can you take a look at the fix in the latest patchset?
https://codereview.chromium.org/246393004/diff2/130001:150001/chrome/browser/...
is the only diff.

-mike

[Mike West, 2014-04-30 07:46:40]

On 2014/04/30 07:45:46, Mike West wrote:
> Vaclav, can you take a look at the fix in the latest patchset?
>
https://codereview.chromium.org/246393004/diff2/130001:150001/chrome/browser/...
> is the only diff.
> 
> -mike

Actually, ignore that. I'll upload a new CL rather than reusing this review.

[vabr (Chromium), 2014-04-30 07:56:06]

Thanks Mike,
Great that you discovered the leak!

I don't really like the current fix, though, please consider my comment below.

Thanks,
Vaclav

https://codereview.chromium.org/246393004/diff/150001/chrome/browser/ui/views...
File chrome/browser/ui/views/passwords/manage_passwords_bubble_view.cc (right):

https://codereview.chromium.org/246393004/diff/150001/chrome/browser/ui/views...
chrome/browser/ui/views/passwords/manage_passwords_bubble_view.cc:163: // The
combobox doesn't take ownership of it's model. If we created a combobox
The explanation in the comment is very helpful.
However, please use scoped_ptr instead of this deleting in the destructor. More
precisely,
1) Change the type of |refuse_combobox_| from a raw pointer to a scoped pointer.
2) Add a member variable scoped_ptr for the model, declared just before the
|refuse_combobox_|, and move this comment before the two declarations, to
emphasise the importance of their order.
3) In the constructor's initializer list, simply create the model in it's
scoped_ptr, and then use that to create the combobox in its scoped_ptr.

[vabr (Chromium), 2014-04-30 07:57:33]

On 2014/04/30 07:46:40, Mike West wrote:
> On 2014/04/30 07:45:46, Mike West wrote:
> > Vaclav, can you take a look at the fix in the latest patchset?
> >
>
https://codereview.chromium.org/246393004/diff2/130001:150001/chrome/browser/...
> > is the only diff.
> > 
> > -mike
> 
> Actually, ignore that. I'll upload a new CL rather than reusing this review.

Fair enough. Please just bear my comment from here in mind during doing that. :)

Vaclav