Add ManagePasswordsDecoration and unit tests.

Add ManagePasswordsDecoration and unit tests.

This CL also moves the common decoration grit ID computation
logic up into ManagePasswordsIcon.

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

[dconnelly, 2014-07-25 16:56:52]

Hi mkwst and rohitrao, please review:

mkwst: chrome/browser/ui/views/passwords and chrome/browser/passwords
rohitrao: chrome/browser/ui/cocoa/location_bar

[Mike West, 2014-07-26 07:06:13]

chrome/browser/ui/views/passwords and chrome/browser/passwords LGTM. The
contours of the mac-specific code look reasonable, but I'm _really_
inexperienced with ObjC; I'm not even going to try a detailed review.

[Mike West, 2014-07-26 07:06:53]

On 2014/07/26 07:06:13, Mike West wrote:
> chrome/browser/ui/views/passwords and chrome/browser/passwords LGTM. The
> contours of the mac-specific code look reasonable, but I'm _really_
> inexperienced with ObjC; I'm not even going to try a detailed review.

(I would suggest creating a ui/cocoa/passwords/ directory, however, and stuffing
all the password things there for clarity)

[dconnelly, 2014-07-28 16:17:46]

On 2014/07/26 07:06:53, Mike West wrote:
> On 2014/07/26 07:06:13, Mike West wrote:
> > chrome/browser/ui/views/passwords and chrome/browser/passwords LGTM. The
> > contours of the mac-specific code look reasonable, but I'm _really_
> > inexperienced with ObjC; I'm not even going to try a detailed review.
> 
> (I would suggest creating a ui/cocoa/passwords/ directory, however, and
stuffing
> all the password things there for clarity)

The existing Mac location bar decorations all live in a common folder, in
contrast to the views side. Rohit: can you provide some feedback on this?

[dconnelly, 2014-07-29 08:21:59]

rohitrao: PTAL. Sent to wrong address before.

[dconnelly, 2014-07-30 15:35:46]

rohitrao: friendly ping

[rohitrao (ping after 24h), 2014-07-31 12:35:20]

https://codereview.chromium.org/419263002/diff/1/chrome/browser/ui/cocoa/loca...
File chrome/browser/ui/cocoa/location_bar/location_bar_view_mac.h (right):

https://codereview.chromium.org/419263002/diff/1/chrome/browser/ui/cocoa/loca...
chrome/browser/ui/cocoa/location_bar/location_bar_view_mac.h:117: NSPoint
GetManagePasswordsBubblePoint() const;
Is this function actually called anywhere?  I would leave it out if it's never
used.

https://codereview.chromium.org/419263002/diff/1/chrome/browser/ui/cocoa/loca...
File chrome/browser/ui/cocoa/location_bar/manage_passwords_decoration.h (right):

https://codereview.chromium.org/419263002/diff/1/chrome/browser/ui/cocoa/loca...
chrome/browser/ui/cocoa/location_bar/manage_passwords_decoration.h:19: public
ManagePasswordsIcon {
The multiple inheritance bothers me (I think it's disallowed by the style
guide?).  Is there any way to do this through composition instead?  Can
|ManagePasswordsDecoration| own a |ManagePasswordsIcon| instead of subclassing,
without too much disruption?

https://codereview.chromium.org/419263002/diff/1/chrome/browser/ui/cocoa/loca...
File chrome/browser/ui/cocoa/location_bar/manage_passwords_decoration.mm
(right):

https://codereview.chromium.org/419263002/diff/1/chrome/browser/ui/cocoa/loca...
chrome/browser/ui/cocoa/location_bar/manage_passwords_decoration.mm:9: #import
"chrome/browser/ui/cocoa/omnibox/omnibox_view_mac.h"
OmniboxViewMac is a C++ class, so this should be #include too.

https://codereview.chromium.org/419263002/diff/1/chrome/browser/ui/cocoa/loca...
File
chrome/browser/ui/cocoa/location_bar/manage_passwords_decoration_unittest.mm
(right):

https://codereview.chromium.org/419263002/diff/1/chrome/browser/ui/cocoa/loca...
chrome/browser/ui/cocoa/location_bar/manage_passwords_decoration_unittest.mm:18:
#import "testing/gtest_mac.h"
#include is appropriate for every header in this block (4 changes in total).

https://codereview.chromium.org/419263002/diff/1/chrome/browser/ui/cocoa/tool...
File chrome/browser/ui/cocoa/toolbar/toolbar_controller.mm (right):

https://codereview.chromium.org/419263002/diff/1/chrome/browser/ui/cocoa/tool...
chrome/browser/ui/cocoa/toolbar/toolbar_controller.mm:738: -
(NSPoint)managePasswordsBubblePoint {
Is this called by anyone?

[dconnelly, 2014-08-01 09:11:47]

https://codereview.chromium.org/419263002/diff/1/chrome/browser/ui/cocoa/loca...
File chrome/browser/ui/cocoa/location_bar/location_bar_view_mac.h (right):

https://codereview.chromium.org/419263002/diff/1/chrome/browser/ui/cocoa/loca...
chrome/browser/ui/cocoa/location_bar/location_bar_view_mac.h:117: NSPoint
GetManagePasswordsBubblePoint() const;
On 2014/07/31 12:35:19, rohitrao wrote:
> Is this function actually called anywhere?  I would leave it out if it's never
> used.

It's used, but not in this CL. I'll move it to a follow-up CL.

https://codereview.chromium.org/419263002/diff/1/chrome/browser/ui/cocoa/loca...
File chrome/browser/ui/cocoa/location_bar/manage_passwords_decoration.h (right):

https://codereview.chromium.org/419263002/diff/1/chrome/browser/ui/cocoa/loca...
chrome/browser/ui/cocoa/location_bar/manage_passwords_decoration.h:19: public
ManagePasswordsIcon {
On 2014/07/31 12:35:19, rohitrao wrote:
> The multiple inheritance bothers me (I think it's disallowed by the style
> guide?).  Is there any way to do this through composition instead?  Can
> |ManagePasswordsDecoration| own a |ManagePasswordsIcon| instead of
subclassing,
> without too much disruption?

Done. But this is a good example of the style guide being too harsh, as
ManagePasswordDecoration really is-a both of these things and involves creating
a new class that only delegates to this one :)

https://codereview.chromium.org/419263002/diff/1/chrome/browser/ui/cocoa/loca...
File chrome/browser/ui/cocoa/location_bar/manage_passwords_decoration.mm
(right):

https://codereview.chromium.org/419263002/diff/1/chrome/browser/ui/cocoa/loca...
chrome/browser/ui/cocoa/location_bar/manage_passwords_decoration.mm:9: #import
"chrome/browser/ui/cocoa/omnibox/omnibox_view_mac.h"
On 2014/07/31 12:35:19, rohitrao wrote:
> OmniboxViewMac is a C++ class, so this should be #include too.

Done.

https://codereview.chromium.org/419263002/diff/1/chrome/browser/ui/cocoa/loca...
File
chrome/browser/ui/cocoa/location_bar/manage_passwords_decoration_unittest.mm
(right):

https://codereview.chromium.org/419263002/diff/1/chrome/browser/ui/cocoa/loca...
chrome/browser/ui/cocoa/location_bar/manage_passwords_decoration_unittest.mm:18:
#import "testing/gtest_mac.h"
On 2014/07/31 12:35:20, rohitrao wrote:
> #include is appropriate for every header in this block (4 changes in total).

Done.

https://codereview.chromium.org/419263002/diff/1/chrome/browser/ui/cocoa/tool...
File chrome/browser/ui/cocoa/toolbar/toolbar_controller.mm (right):

https://codereview.chromium.org/419263002/diff/1/chrome/browser/ui/cocoa/tool...
chrome/browser/ui/cocoa/toolbar/toolbar_controller.mm:738: -
(NSPoint)managePasswordsBubblePoint {
On 2014/07/31 12:35:20, rohitrao wrote:
> Is this called by anyone?

Not currently. Moved to a follow-up CL.

[rohitrao (ping after 24h), 2014-08-04 13:59:56]

lgtm

[dconnelly, 2014-08-04 14:02:29]

The CQ bit was checked by dconnelly@chromium.org

[commit-bot: I haz the power, 2014-08-04 14:02:50]

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

[commit-bot: I haz the power, 2014-08-04 15:51:19]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  android_clang_dbg on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/android_clang_d...)
  android_dbg on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/android_dbg/bui...)

[dconnelly, 2014-08-04 15:52:46]

The CQ bit was unchecked by dconnelly@chromium.org

[dconnelly, 2014-08-05 12:50:17]

The CQ bit was checked by dconnelly@chromium.org

[commit-bot: I haz the power, 2014-08-05 12:51:37]

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

[commit-bot: I haz the power, 2014-08-05 18:06:59]

Change committed as 287562