Hook up the Mac password bubble to the browser and add browser tests.

Hook up the Mac password bubble to the browser and add browser tests.

BUG=328847
Committed: https://crrev.com/895539fab3c573341f0c4e91653dcd32370bcc07
Cr-Commit-Position: refs/heads/master@{#291703}

[dconnelly, 2014-08-05 12:39:06]

There are only two browser tests because of the thorough unit test coverage
within each layer and between each pair of interacting layers. These tests only
ensure that the browser and the bubble and the location bar decoration are
properly hooked up.

vabr: chrome/browser/password_manager
mkwst: chrome/browser/ui/passwords and chrome/browser/ui/views/passwords
shess: chrome/browser/ui/cocoa

[dconnelly, 2014-08-05 12:39:51]

+sky for chrome/browser/ui

[vabr (Chromium), 2014-08-05 13:14:26]

chrome/browser/password_manager LGTM with a comment
Thanks for completing the desktop bubble coverage!
Vaclav

https://codereview.chromium.org/444603003/diff/20001/chrome/browser/password_...
File chrome/browser/password_manager/chrome_password_manager_client.cc (right):

https://codereview.chromium.org/444603003/diff/20001/chrome/browser/password_...
chrome/browser/password_manager/chrome_password_manager_client.cc:357: #if
!defined(USE_AURA) && !defined(OS_MACOSX)
This is within //chrome, which means desktop only. I believe it is safe to just
remove lines 357-359 altogether.

[sky, 2014-08-05 17:03:30]

I didn't review the cocoa side. Please get a more local owner for that.

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

https://codereview.chromium.org/444603003/diff/20001/chrome/browser/ui/views/...
chrome/browser/ui/views/passwords/manage_passwords_bubble_view_browsertest.cc:28:
virtual ManagePasswordsIcon* view() OVERRIDE {
public: along with a private: DISALLOW...

https://codereview.chromium.org/444603003/diff/20001/chrome/browser/ui/views/...
chrome/browser/ui/views/passwords/manage_passwords_bubble_view_browsertest.cc:30:
return browser_view->GetToolbarView()
Did the formatter do this? If not, I would put location_bar() on 30.

[Scott Hess - ex-Googler, 2014-08-05 17:41:01]

lgtm for cocoa/, with minor nit.

https://codereview.chromium.org/444603003/diff/20001/chrome/browser/ui/cocoa/...
File chrome/browser/ui/cocoa/passwords/manage_passwords_bubble_browsertest.mm
(right):

https://codereview.chromium.org/444603003/diff/20001/chrome/browser/ui/cocoa/...
chrome/browser/ui/cocoa/passwords/manage_passwords_bubble_browsertest.mm:22:
@interface AlwaysKeyNSWindow : NSObject
The use of this is kind of ... terrifying, but I can see that doing it a
different way might be kind of intrusive.  That said, this should subclass
NSWindow, rather than NSObject, or be a category of NSWindow with naming
something like -cr_alwaysIsKeyWindow.

Makes me wonder if we need a browser_tests version of CocoaTestHelperWindow,
where it synthetically pretends to be foreground even if everything is actually
background.

[dconnelly, 2014-08-06 12:46:12]

I've added two more browser tests, the second of which caught that tab changes
weren't always handled properly. That's now fixed.

https://codereview.chromium.org/444603003/diff/20001/chrome/browser/password_...
File chrome/browser/password_manager/chrome_password_manager_client.cc (right):

https://codereview.chromium.org/444603003/diff/20001/chrome/browser/password_...
chrome/browser/password_manager/chrome_password_manager_client.cc:357: #if
!defined(USE_AURA) && !defined(OS_MACOSX)
On 2014/08/05 13:14:26, vabr (Chromium) wrote:
> This is within //chrome, which means desktop only. I believe it is safe to
just
> remove lines 357-359 altogether.

Done.

https://codereview.chromium.org/444603003/diff/20001/chrome/browser/ui/cocoa/...
File chrome/browser/ui/cocoa/passwords/manage_passwords_bubble_browsertest.mm
(right):

https://codereview.chromium.org/444603003/diff/20001/chrome/browser/ui/cocoa/...
chrome/browser/ui/cocoa/passwords/manage_passwords_bubble_browsertest.mm:22:
@interface AlwaysKeyNSWindow : NSObject
On 2014/08/05 17:41:01, Scott Hess wrote:
> The use of this is kind of ... terrifying, but I can see that doing it a
> different way might be kind of intrusive.  That said, this should subclass
> NSWindow, rather than NSObject, or be a category of NSWindow with naming
> something like -cr_alwaysIsKeyWindow.

Done.

> 
> Makes me wonder if we need a browser_tests version of CocoaTestHelperWindow,
> where it synthetically pretends to be foreground even if everything is
actually
> background.

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

https://codereview.chromium.org/444603003/diff/20001/chrome/browser/ui/views/...
chrome/browser/ui/views/passwords/manage_passwords_bubble_view_browsertest.cc:28:
virtual ManagePasswordsIcon* view() OVERRIDE {
On 2014/08/05 17:03:30, sky wrote:
> public: along with a private: DISALLOW...

Done.

https://codereview.chromium.org/444603003/diff/20001/chrome/browser/ui/views/...
chrome/browser/ui/views/passwords/manage_passwords_bubble_view_browsertest.cc:30:
return browser_view->GetToolbarView()
On 2014/08/05 17:03:30, sky wrote:
> Did the formatter do this? If not, I would put location_bar() on 30.

It did.

[dconnelly, 2014-08-07 12:17:37]

sky: PTAL

[sky, 2014-08-07 16:18:57]

LGTM

[dconnelly, 2014-08-25 13:30:03]

The CQ bit was checked by dconnelly@chromium.org

[commit-bot: I haz the power, 2014-08-25 13:30:30]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/dconnelly@chromium.org/444603003/140001

[commit-bot: I haz the power, 2014-08-25 14:24:36]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  linux_chromium_chromeos_rel_swarming on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_rel_swarming on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  win8_chromium_rel on tryserver.chromium.win
(http://build.chromium.org/p/tryserver.chromium.win/builders/win8_chromium_rel...)
  win_chromium_compile_dbg on tryserver.chromium.win
(http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_comp...)
  win_chromium_rel_swarming on tryserver.chromium.win
(http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
  win_chromium_x64_rel_swarming on tryserver.chromium.win
(http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)

[dconnelly, 2014-08-25 14:27:58]

The CQ bit was unchecked by dconnelly@chromium.org

[dconnelly, 2014-08-25 14:56:34]

The CQ bit was checked by dconnelly@chromium.org

[commit-bot: I haz the power, 2014-08-25 14:57:05]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/dconnelly@chromium.org/444603003/160001

[commit-bot: I haz the power, 2014-08-25 16:41:21]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  mac_chromium_rel_swarming on tryserver.chromium.mac
(http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
  win_chromium_x64_rel_swarming on tryserver.chromium.win
(http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)

[commit-bot: I haz the power, 2014-08-25 17:27:23]

Committed patchset #8 (160001) as dae2a9adff280655fa9261b972fd62ae837b374d

[jam, 2014-08-25 20:37:28]

A revert of this CL (patchset #8) has been created in
https://codereview.chromium.org/473423005/ by jam@chromium.org.

The reason for reverting is: the browser tests this added are really flaky on
the trybots..

[commit-bot: I haz the power, 2014-09-10 02:35:16]

Patchset 8 (id:??) landed as
https://crrev.com/895539fab3c573341f0c4e91653dcd32370bcc07
Cr-Commit-Position: refs/heads/master@{#291703}