Password bubble: refactor ManagePasswordsIconView.

Password bubble: refactor ManagePasswordsIconView.

As another step towards a platform-agnostic test suite for the password
management UI, this CL refactors ManagePasswordsIconView in a few ways:

1. It introduces a new 'ManagePasswordsIcon' abstract base class
   to provide a platform-agnostic interface to the Omnibox icon.

2. The ManagePasswordsBubbleUIController is now responsible for
   determining the Omnibox's icon's state, and also responsible
   for triggering automatic display of the bubble UI. The actual
   functionality is still sitting on the icon for the moment, but
   will be moved out to a platform-agnostic layer in a future CL.

3. Most importantly, some basic unit tests verify the functionality
   added above.

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

[Mike West, 2014-04-24 11:37:17]

Hi Vaclav, Markus, and Peter.

Would you mind taking a look at this CL, which intends to extract some of the
platform-agnostic bits of ManagePasswordsIconView out of Views and into
something we can more easily test. This is another step along the path outlined
in the discussion on https://codereview.chromium.org/246393004/, and should make
it possible to test more of the framework than we're currently doing.

Thanks for your time!

vabr@ and markusheintz@: Can you review the ui/passwords and ui/views/passwords
bits?
pkasting@: Can you review the ui/views/location_bar change?

-mike

[Mike West, 2014-04-24 12:00:58]

Actually +pkasting@ this time. Hi!

[vabr (Chromium), 2014-04-24 12:56:11]

Hi Mike,

I'm looking forward to stealing your test-writing know-how from you. :)

LGTM with 2 copies of 1 comment.

Cheers,
Vaclav

https://codereview.chromium.org/250353003/diff/1/chrome/browser/ui/passwords/...
File chrome/browser/ui/passwords/manage_passwords_bubble_ui_controller.cc
(right):

https://codereview.chromium.org/250353003/diff/1/chrome/browser/ui/passwords/...
chrome/browser/ui/passwords/manage_passwords_bubble_ui_controller.cc:63: if
(form_manager) {
nit: I'm always a bit uneasy about modifying production code just for use in
tests.
I believe a better solution would be to make this method virtual and override it
in ManagePasswordsBubbleUIControllerMock (which would probably need to be
renamed to TestManagePasswordsBubbleUIController) in a way that would allow
passing a NULL form manager. E.g., OnPasswordSubmitted could have the second
half (after if(form_Manager){...}) put into a protected method, and the
overriding test-class OnPasswordSubmitted could call that protected method.

I'm not insisting on this, if you believe this would make things more complex, I
won't fight too long. :)

https://codereview.chromium.org/250353003/diff/1/chrome/browser/ui/passwords/...
chrome/browser/ui/passwords/manage_passwords_bubble_ui_controller.cc:78: if
(!password_form_map_.empty()) {
Ditto about putting test-only provisions into production code.

[Mike West, 2014-04-24 14:27:58]

On 2014/04/24 12:56:11, vabr (Chromium) wrote:
> Hi Mike,
> 
> I'm looking forward to stealing your test-writing know-how from you. :)

Happy to chat, though my know-how basically boils down to cargo-culting bits of
existing tests. :)

https://codereview.chromium.org/250353003/diff/1/chrome/browser/ui/passwords/...
File chrome/browser/ui/passwords/manage_passwords_bubble_ui_controller.cc
(right):

https://codereview.chromium.org/250353003/diff/1/chrome/browser/ui/passwords/...
chrome/browser/ui/passwords/manage_passwords_bubble_ui_controller.cc:63: if
(form_manager) {
On 2014/04/24 12:56:12, vabr (Chromium) wrote:
> nit: I'm always a bit uneasy about modifying production code just for use in
> tests.
> I believe a better solution would be to make this method virtual and override
it
> in ManagePasswordsBubbleUIControllerMock (which would probably need to be
> renamed to TestManagePasswordsBubbleUIController) in a way that would allow
> passing a NULL form manager. E.g., OnPasswordSubmitted could have the second
> half (after if(form_Manager){...}) put into a protected method, and the
> overriding test-class OnPasswordSubmitted could call that protected method.
> 
> I'm not insisting on this, if you believe this would make things more complex,
I
> won't fight too long. :)

Naah, you're right. I shouldn't have taken the easy way out. Updated the tests
to create all the necessary bits.

https://codereview.chromium.org/250353003/diff/1/chrome/browser/ui/passwords/...
chrome/browser/ui/passwords/manage_passwords_bubble_ui_controller.cc:78: if
(!password_form_map_.empty()) {
On 2014/04/24 12:56:12, vabr (Chromium) wrote:
> Ditto about putting test-only provisions into production code.

Done.

[vabr (Chromium), 2014-04-24 14:46:33]

LGTM!

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

https://codereview.chromium.org/250353003/diff/20001/chrome/browser/ui/passwo...
chrome/browser/ui/passwords/manage_passwords_bubble_ui_controller_unittest.cc:104:
EXPECT_EQ(test_form().origin.spec(), controller()->origin().spec());
optional nit: Do you compare specs instead of the GURLs themselves? The
operator== for GURL does in fact compare by spec, and I believe the operator<<
for GURL is overloaded to output the spec, so the result should be identical if
your remove .spec().
Ditto on the similar lines in this test.

[vabr (Chromium), 2014-04-24 16:01:57]

Still LGTM, but one more optional nit.

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

https://codereview.chromium.org/250353003/diff/20001/chrome/browser/ui/passwo...
chrome/browser/ui/passwords/manage_passwords_bubble_ui_controller_unittest.cc:61:
return reinterpret_cast<ManagePasswordsBubbleUIControllerMock*>(
optional nit: This could be a static_cast -- you are not re-interpreting the
memory content in the sense of double* to int* conversion. You are about to
access the same controller, only more of it.
(Similar use of static_cast is for example in
RenderViewObserver::RenderViewObserver.)

[Peter Kasting, 2014-04-24 20:54:56]

c/b/ui/views/location_bar LGTM

[Mike West, 2014-04-25 08:01:26]

Thanks Vaclav and Peter.

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

https://codereview.chromium.org/250353003/diff/20001/chrome/browser/ui/passwo...
chrome/browser/ui/passwords/manage_passwords_bubble_ui_controller_unittest.cc:24:
class MockPasswordManagerDriver
Moved this into a separate file, since it's now used in three places.

https://codereview.chromium.org/250353003/diff/20001/chrome/browser/ui/passwo...
chrome/browser/ui/passwords/manage_passwords_bubble_ui_controller_unittest.cc:61:
return reinterpret_cast<ManagePasswordsBubbleUIControllerMock*>(
On 2014/04/24 16:01:57, vabr (Chromium) wrote:
> optional nit: This could be a static_cast -- you are not re-interpreting the
> memory content in the sense of double* to int* conversion. You are about to
> access the same controller, only more of it.
> (Similar use of static_cast is for example in
> RenderViewObserver::RenderViewObserver.)

Done.

https://codereview.chromium.org/250353003/diff/20001/chrome/browser/ui/passwo...
chrome/browser/ui/passwords/manage_passwords_bubble_ui_controller_unittest.cc:104:
EXPECT_EQ(test_form().origin.spec(), controller()->origin().spec());
On 2014/04/24 14:46:34, vabr (Chromium) wrote:
> optional nit: Do you compare specs instead of the GURLs themselves? The
> operator== for GURL does in fact compare by spec, and I believe the operator<<
> for GURL is overloaded to output the spec, so the result should be identical
if
> your remove .spec().
> Ditto on the similar lines in this test.

Hrm. I've been doing that wrong for a while. :) Thanks!

[markusheintz_, 2014-04-25 13:25:09]

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

https://codereview.chromium.org/250353003/diff/60001/chrome/browser/ui/views/...
chrome/browser/ui/views/passwords/manage_passwords_icon_view.cc:52: // Suppress
the bubble if the user is working in the omnibox.
What does working mean?

Is the bubble displayed after the user is done?

[Mike West, 2014-04-25 13:31:41]

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

https://codereview.chromium.org/250353003/diff/60001/chrome/browser/ui/views/...
chrome/browser/ui/views/passwords/manage_passwords_icon_view.cc:52: // Suppress
the bubble if the user is working in the omnibox.
On 2014/04/25 13:25:10, markusheintz_ wrote:
> What does working mean?

I believe this method returns true if the user has a caret in the Omnibox;
either via selection of the URL or because she's typing directly.

> Is the bubble displayed after the user is done?

No. If the user starts working in the Omnibox, we drop automatic requests to
display the bubble on the floor.

This isn't a behavioral change; this is moved from line 29 in the old code.

[markusheintz_, 2014-04-25 13:33:43]

On 2014/04/25 13:31:41, Mike West wrote:
>
https://codereview.chromium.org/250353003/diff/60001/chrome/browser/ui/views/...
> File chrome/browser/ui/views/passwords/manage_passwords_icon_view.cc (right):
> 
>
https://codereview.chromium.org/250353003/diff/60001/chrome/browser/ui/views/...
> chrome/browser/ui/views/passwords/manage_passwords_icon_view.cc:52: //
Suppress
> the bubble if the user is working in the omnibox.
> On 2014/04/25 13:25:10, markusheintz_ wrote:
> > What does working mean?
> 
> I believe this method returns true if the user has a caret in the Omnibox;
> either via selection of the URL or because she's typing directly.
> 
> > Is the bubble displayed after the user is done?
> 
> No. If the user starts working in the Omnibox, we drop automatic requests to
> display the bubble on the floor.
> 
> This isn't a behavioral change; this is moved from line 29 in the old code.

LGTM

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

Great, thanks Markus. I'll land this Monday morning, given that the tree's
permared today. :)

Hey Garett, can you rubberstamp the gypi change in the meantime? Thanks!

[Garrett Casto, 2014-04-25 23:32:48]

lgtm

[Mike West, 2014-04-26 06:25:26]

The CQ bit was checked by mkwst@chromium.org

[commit-bot: I haz the power, 2014-04-26 06:27:41]

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

[commit-bot: I haz the power, 2014-04-26 06:27:54]

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

[commit-bot: I haz the power, 2014-04-26 06:27:55]

Failed to apply patch for
chrome/browser/ui/views/passwords/manage_passwords_icon_view.cc:
While running patch -p1 --forward --force --no-backup-if-mismatch;
  patching file chrome/browser/ui/views/passwords/manage_passwords_icon_view.cc
  Hunk #2 FAILED at 15.
  1 out of 2 hunks FAILED -- saving rejects to file
chrome/browser/ui/views/passwords/manage_passwords_icon_view.cc.rej

Patch:       chrome/browser/ui/views/passwords/manage_passwords_icon_view.cc
Index: chrome/browser/ui/views/passwords/manage_passwords_icon_view.cc
diff --git a/chrome/browser/ui/views/passwords/manage_passwords_icon_view.cc
b/chrome/browser/ui/views/passwords/manage_passwords_icon_view.cc
index
aa99be4b8b9ae00f395a1a058e661f03bbad2e92..116e7c0f59deab45baa7c434347c4033263a252f
100644
--- a/chrome/browser/ui/views/passwords/manage_passwords_icon_view.cc
+++ b/chrome/browser/ui/views/passwords/manage_passwords_icon_view.cc
@@ -5,6 +5,7 @@
 #include "chrome/browser/ui/views/passwords/manage_passwords_icon_view.h"
 
 #include "chrome/browser/ui/passwords/manage_passwords_bubble_ui_controller.h"
+#include "chrome/browser/ui/view_ids.h"
 #include "chrome/browser/ui/views/passwords/manage_passwords_bubble_view.h"
 #include "grit/generated_resources.h"
 #include "grit/theme_resources.h"
@@ -14,50 +15,47 @@
 ManagePasswordsIconView::ManagePasswordsIconView(
     LocationBarView::Delegate* location_bar_delegate)
     : location_bar_delegate_(location_bar_delegate) {
+  set_id(VIEW_ID_MANAGE_PASSWORDS_ICON_BUTTON);
   SetAccessibilityFocusable(true);
-  Update(NULL);
   LocationBarView::InitTouchableLocationBarChildView(this);
+  SetState(ManagePasswordsIcon::INACTIVE_STATE);
 }
 
 ManagePasswordsIconView::~ManagePasswordsIconView() {}
 
-void ManagePasswordsIconView::Update(
-    ManagePasswordsBubbleUIController* manage_passwords_bubble_ui_controller) {
-  SetVisible(manage_passwords_bubble_ui_controller &&
-      manage_passwords_bubble_ui_controller->
-          manage_passwords_icon_to_be_shown() &&
-      !location_bar_delegate_->GetToolbarModel()->input_in_progress());
-  if (!visible()) {
-    ManagePasswordsBubbleView::CloseBubble();
+void ManagePasswordsIconView::SetStateInternal(
+    ManagePasswordsIcon::State state) {
+  if (state == ManagePasswordsIcon::INACTIVE_STATE) {
+    SetVisible(false);
+    if (ManagePasswordsBubbleView::IsShowing())
+      ManagePasswordsBubbleView::CloseBubble();
     return;
   }
-  int icon_to_display =
-      manage_passwords_bubble_ui_controller->autofill_blocked()
-          ? IDR_SAVE_PASSWORD_BLACKLISTED
-          : IDR_SAVE_PASSWORD;
-  SetImage(ui::ResourceBundle::GetSharedInstance().GetImageSkiaNamed(
-      icon_to_display));
-  SetTooltip(manage_passwords_bubble_ui_controller->password_to_be_saved());
-}
 
-void ManagePasswordsIconView::ShowBubbleIfNeeded(
-    ManagePasswordsBubbleUIController* manage_passwords_bubble_ui_controller) {
-  if (manage_passwords_bubble_ui_controller->
-          manage_passwords_bubble_needs_showing() &&
-      visible() &&
-      !ManagePasswordsBubbleView::IsShowing()) {
-    ManagePasswordsBubbleView::ShowBubble(
-        location_bar_delegate_->GetWebContents(),
-        ManagePasswordsBubbleView::AUTOMATIC);
-    manage_passwords_bubble_ui_controller->OnBubbleShown();
-  }
+  // Start with the correct values for ManagePasswordsIcon::MANAGE_STATE, and
+  // adjust things accordingly if we're either in BLACKLISTED_STATE or
+  // PENDING_STATE.
+  int which_icon = IDR_SAVE_PASSWORD;
+  int which_text = IDS_PASSWORD_MANAGER_TOOLTIP_MANAGE;
+  if (state == ManagePasswordsIcon::BLACKLISTED_STATE)
+    which_icon = IDR_SAVE_PASSWORD_BLACKLISTED;
+  else if (state == ManagePasswordsIcon::PENDING_STATE)
+    which_text = IDS_PASSWORD_MANAGER_TOOLTIP_SAVE;
+
+  SetVisible(true);
+  SetImage(
+      ui::ResourceBundle::GetSharedInstance().GetImageSkiaNamed(which_icon));
+  SetTooltipText(l10n_util::GetStringUTF16(which_text));
 }
 
-void ManagePasswordsIconView::SetTooltip(bool password_to_be_saved) {
-  SetTooltipText(l10n_util::GetStringUTF16(
-      (password_to_be_saved ?
-          IDS_PASSWORD_MANAGER_TOOLTIP_SAVE :
-          IDS_PASSWORD_MANAGER_TOOLTIP_MANAGE)));
+void ManagePasswordsIconView::ShowBubbleWithoutUserInteraction() {
+  // Suppress the bubble if the user is working in the omnibox.
+  if (location_bar_delegate_->GetToolbarModel()->input_in_progress())
+    return;
+
+  ManagePasswordsBubbleView::ShowBubble(
+      location_bar_delegate_->GetWebContents(),
+      ManagePasswordsBubbleView::AUTOMATIC);
 }
 
 bool ManagePasswordsIconView::GetTooltipText(const gfx::Point& p,

[Mike West, 2014-04-28 06:40:59]

The CQ bit was checked by mkwst@chromium.org

[commit-bot: I haz the power, 2014-04-28 06:42:18]

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

[Mike West, 2014-04-28 06:48:53]

The CQ bit was checked by mkwst@chromium.org

[commit-bot: I haz the power, 2014-04-28 06:51:24]

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

[commit-bot: I haz the power, 2014-04-28 06:54:24]

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

[commit-bot: I haz the power, 2014-04-28 06:54:24]

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

[Mike West, 2014-04-28 07:55:56]

The CQ bit was checked by mkwst@chromium.org

[commit-bot: I haz the power, 2014-04-28 07:58:26]

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

[commit-bot: I haz the power, 2014-04-28 12:21:08]

Change committed as 266520