Add ManagePasswordsBubbleManageViewController and unit tests.

Add ManagePasswordsBubbleManageViewController and unit tests.

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

[dconnelly, 2014-08-22 10:45:14]

+vabr

[vabr (Chromium), 2014-08-22 12:08:45]

chrome/browser/ui/views/passwords/*
and
chrome/browser/ui/passwords/*
LGTM.

[Scott Hess - ex-Googler, 2014-08-22 22:37:36]

https://codereview.chromium.org/462563002/diff/60001/chrome/browser/ui/cocoa/...
File chrome/browser/ui/cocoa/passwords/manage_password_item_view_controller.mm
(right):

https://codereview.chromium.org/462563002/diff/60001/chrome/browser/ui/cocoa/...
chrome/browser/ui/cocoa/passwords/manage_password_item_view_controller.mm:28:
static const SkColor kHoverColor = SkColorSetARGBInline(0xFF, 0xEB, 0xEB, 0xEB);
anonymous namespace doesn't need static.

https://codereview.chromium.org/462563002/diff/60001/chrome/browser/ui/cocoa/...
chrome/browser/ui/cocoa/passwords/manage_password_item_view_controller.mm:48:
static const CGFloat width = std::max(kUsernameWidth, undoExplanationWidth);
If these are not constructible at compile-time, then you're going to get a bot
failure for adding new non-POD statics, I think.  If they _are_ constructible at
compile time, then const is sufficient, I think.

If the static is so that things get cached for performance, I doubt people will
hit the password manager enough for it to be helpful.  But if they hit it once,
now some extra memory is used forever.

https://codereview.chromium.org/462563002/diff/60001/chrome/browser/ui/cocoa/...
chrome/browser/ui/cocoa/passwords/manage_password_item_view_controller.mm:57:
static const CGFloat width = std::max(kPasswordWidth, undoLinkWidth);
These statics should go, to.

https://codereview.chromium.org/462563002/diff/60001/chrome/browser/ui/cocoa/...
chrome/browser/ui/cocoa/passwords/manage_password_item_view_controller.mm:69:
kFramePadding;
And this one.

https://codereview.chromium.org/462563002/diff/60001/chrome/browser/ui/cocoa/...
chrome/browser/ui/cocoa/passwords/manage_password_item_view_controller.mm:247:
[self setFrameSize:NSMakeSize(ItemWidth(), curY)];
Why did you update curX, then?

https://codereview.chromium.org/462563002/diff/60001/chrome/browser/ui/cocoa/...
chrome/browser/ui/cocoa/passwords/manage_password_item_view_controller.mm:345:
[self.view setFrameSize:NSMakeSize(width, height)];
NSMakeSize() -> contentSize.

https://codereview.chromium.org/462563002/diff/60001/chrome/browser/ui/cocoa/...
chrome/browser/ui/cocoa/passwords/manage_password_item_view_controller.mm:425:
int options = NSTrackingMouseEnteredAndExited | NSTrackingActiveAlways;
|int|->|NSTrackingAreaOptions|.

NSTrackingAlways?  That means you'll track even if your window or app is in the
background.  Generally that is not preferred unless you are also handling first
clicks correctly.

https://codereview.chromium.org/462563002/diff/60001/chrome/browser/ui/cocoa/...
File
chrome/browser/ui/cocoa/passwords/manage_password_item_view_controller_unittest.mm
(right):

https://codereview.chromium.org/462563002/diff/60001/chrome/browser/ui/cocoa/...
chrome/browser/ui/cocoa/passwords/manage_password_item_view_controller_unittest.mm:27:
static NSString* const kItemTestPassword = @"bar";
No need for static inside anonymous namespace.

https://codereview.chromium.org/462563002/diff/60001/chrome/browser/ui/cocoa/...
chrome/browser/ui/cocoa/passwords/manage_password_item_view_controller_unittest.mm:73:
};
Apparently DISALLOW_COPY_AND_ASSIGN() is a thing in tests, too?  I never know if
I'm supposed to request it or not.

https://codereview.chromium.org/462563002/diff/60001/chrome/browser/ui/cocoa/...
File
chrome/browser/ui/cocoa/passwords/manage_passwords_bubble_manage_view_controller.mm
(right):

https://codereview.chromium.org/462563002/diff/60001/chrome/browser/ui/cocoa/...
chrome/browser/ui/cocoa/passwords/manage_passwords_bubble_manage_view_controller.mm:29:
for (auto i = model->best_matches().rbegin();
The chromium-dev thread "Clearance on C++11 Features?" recently indicated that
auto is being used on Mac, but then Nico said "Don't use C++11 features yet." 
He didn't say "Except auto, that's fine", so I'm inclined towards concern about
this.

https://codereview.chromium.org/462563002/diff/60001/chrome/browser/ui/cocoa/...
chrome/browser/ui/cocoa/passwords/manage_passwords_bubble_manage_view_controller.mm:49:
NSMaxY([itemView frame]))];
Can this go outside the loop?  I don't see an obvious place to calculate width,
but curY will do for the height.

https://codereview.chromium.org/462563002/diff/60001/chrome/browser/ui/cocoa/...
chrome/browser/ui/cocoa/passwords/manage_passwords_bubble_manage_view_controller.mm:51:
itemViews_.reset(items.release());
itemViews_.swap(items)

https://codereview.chromium.org/462563002/diff/60001/chrome/browser/ui/cocoa/...
File
chrome/browser/ui/cocoa/passwords/manage_passwords_bubble_manage_view_controller_unittest.mm
(right):

https://codereview.chromium.org/462563002/diff/60001/chrome/browser/ui/cocoa/...
chrome/browser/ui/cocoa/passwords/manage_passwords_bubble_manage_view_controller_unittest.mm:64:
};
DISALLOW_COPY_AND_ASSIGN().

https://codereview.chromium.org/462563002/diff/60001/chrome/browser/ui/cocoa/...
chrome/browser/ui/cocoa/passwords/manage_passwords_bubble_manage_view_controller_unittest.mm:111:
for (ManagePasswordItemViewController* item : items) {
for (Type* item in items).

[dconnelly, 2014-08-25 12:55:41]

https://codereview.chromium.org/462563002/diff/60001/chrome/browser/ui/cocoa/...
File chrome/browser/ui/cocoa/passwords/manage_password_item_view_controller.mm
(right):

https://codereview.chromium.org/462563002/diff/60001/chrome/browser/ui/cocoa/...
chrome/browser/ui/cocoa/passwords/manage_password_item_view_controller.mm:28:
static const SkColor kHoverColor = SkColorSetARGBInline(0xFF, 0xEB, 0xEB, 0xEB);
On 2014/08/22 22:37:36, Scott Hess wrote:
> anonymous namespace doesn't need static.

Done.

https://codereview.chromium.org/462563002/diff/60001/chrome/browser/ui/cocoa/...
chrome/browser/ui/cocoa/passwords/manage_password_item_view_controller.mm:48:
static const CGFloat width = std::max(kUsernameWidth, undoExplanationWidth);
On 2014/08/22 22:37:35, Scott Hess wrote:
> If these are not constructible at compile-time, then you're going to get a bot
> failure for adding new non-POD statics, I think.  If they _are_ constructible
at
> compile time, then const is sufficient, I think.
> 
> If the static is so that things get cached for performance, I doubt people
will
> hit the password manager enough for it to be helpful.  But if they hit it
once,
> now some extra memory is used forever.

Done.

https://codereview.chromium.org/462563002/diff/60001/chrome/browser/ui/cocoa/...
chrome/browser/ui/cocoa/passwords/manage_password_item_view_controller.mm:57:
static const CGFloat width = std::max(kPasswordWidth, undoLinkWidth);
On 2014/08/22 22:37:35, Scott Hess wrote:
> These statics should go, to.

Done.

https://codereview.chromium.org/462563002/diff/60001/chrome/browser/ui/cocoa/...
chrome/browser/ui/cocoa/passwords/manage_password_item_view_controller.mm:69:
kFramePadding;
On 2014/08/22 22:37:36, Scott Hess wrote:
> And this one.

Done.

https://codereview.chromium.org/462563002/diff/60001/chrome/browser/ui/cocoa/...
chrome/browser/ui/cocoa/passwords/manage_password_item_view_controller.mm:247:
[self setFrameSize:NSMakeSize(ItemWidth(), curY)];
On 2014/08/22 22:37:35, Scott Hess wrote:
> Why did you update curX, then?

Done.

https://codereview.chromium.org/462563002/diff/60001/chrome/browser/ui/cocoa/...
chrome/browser/ui/cocoa/passwords/manage_password_item_view_controller.mm:345:
[self.view setFrameSize:NSMakeSize(width, height)];
On 2014/08/22 22:37:35, Scott Hess wrote:
> NSMakeSize() -> contentSize.

Done.

https://codereview.chromium.org/462563002/diff/60001/chrome/browser/ui/cocoa/...
chrome/browser/ui/cocoa/passwords/manage_password_item_view_controller.mm:425:
int options = NSTrackingMouseEnteredAndExited | NSTrackingActiveAlways;
On 2014/08/22 22:37:35, Scott Hess wrote:
> |int|->|NSTrackingAreaOptions|.
> 
> NSTrackingAlways?  That means you'll track even if your window or app is in
the
> background.  Generally that is not preferred unless you are also handling
first
> clicks correctly.

Done.

https://codereview.chromium.org/462563002/diff/60001/chrome/browser/ui/cocoa/...
File
chrome/browser/ui/cocoa/passwords/manage_password_item_view_controller_unittest.mm
(right):

https://codereview.chromium.org/462563002/diff/60001/chrome/browser/ui/cocoa/...
chrome/browser/ui/cocoa/passwords/manage_password_item_view_controller_unittest.mm:27:
static NSString* const kItemTestPassword = @"bar";
On 2014/08/22 22:37:36, Scott Hess wrote:
> No need for static inside anonymous namespace.

Done.

https://codereview.chromium.org/462563002/diff/60001/chrome/browser/ui/cocoa/...
chrome/browser/ui/cocoa/passwords/manage_password_item_view_controller_unittest.mm:73:
};
On 2014/08/22 22:37:36, Scott Hess wrote:
> Apparently DISALLOW_COPY_AND_ASSIGN() is a thing in tests, too?  I never know
if
> I'm supposed to request it or not.

Done.

https://codereview.chromium.org/462563002/diff/60001/chrome/browser/ui/cocoa/...
File
chrome/browser/ui/cocoa/passwords/manage_passwords_bubble_manage_view_controller.mm
(right):

https://codereview.chromium.org/462563002/diff/60001/chrome/browser/ui/cocoa/...
chrome/browser/ui/cocoa/passwords/manage_passwords_bubble_manage_view_controller.mm:29:
for (auto i = model->best_matches().rbegin();
On 2014/08/22 22:37:36, Scott Hess wrote:
> The chromium-dev thread "Clearance on C++11 Features?" recently indicated that
> auto is being used on Mac, but then Nico said "Don't use C++11 features yet." 
> He didn't say "Except auto, that's fine", so I'm inclined towards concern
about
> this.

Done.

https://codereview.chromium.org/462563002/diff/60001/chrome/browser/ui/cocoa/...
chrome/browser/ui/cocoa/passwords/manage_passwords_bubble_manage_view_controller.mm:49:
NSMaxY([itemView frame]))];
On 2014/08/22 22:37:36, Scott Hess wrote:
> Can this go outside the loop?  I don't see an obvious place to calculate
width,
> but curY will do for the height.

Done.

https://codereview.chromium.org/462563002/diff/60001/chrome/browser/ui/cocoa/...
chrome/browser/ui/cocoa/passwords/manage_passwords_bubble_manage_view_controller.mm:51:
itemViews_.reset(items.release());
On 2014/08/22 22:37:36, Scott Hess wrote:
> itemViews_.swap(items)

The generic type differs; the member variable is a scoped_nsobject<NSArray>
while the local is a scoped_nsobject<NSMutableArray>. The compiler throws an
error about the argument type into swap().

https://codereview.chromium.org/462563002/diff/60001/chrome/browser/ui/cocoa/...
File
chrome/browser/ui/cocoa/passwords/manage_passwords_bubble_manage_view_controller_unittest.mm
(right):

https://codereview.chromium.org/462563002/diff/60001/chrome/browser/ui/cocoa/...
chrome/browser/ui/cocoa/passwords/manage_passwords_bubble_manage_view_controller_unittest.mm:64:
};
On 2014/08/22 22:37:36, Scott Hess wrote:
> DISALLOW_COPY_AND_ASSIGN().

Done.

https://codereview.chromium.org/462563002/diff/60001/chrome/browser/ui/cocoa/...
chrome/browser/ui/cocoa/passwords/manage_passwords_bubble_manage_view_controller_unittest.mm:111:
for (ManagePasswordItemViewController* item : items) {
On 2014/08/22 22:37:36, Scott Hess wrote:
> for (Type* item in items).

Done.

[Scott Hess - ex-Googler, 2014-08-25 22:21:21]

lgtm

[dconnelly, 2014-08-26 08:36:40]

The CQ bit was checked by dconnelly@chromium.org

[commit-bot: I haz the power, 2014-08-26 08:38:17]

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

[dconnelly, 2014-08-26 09:14:19]

The CQ bit was unchecked by dconnelly@chromium.org

[dconnelly, 2014-08-26 09:25:58]

The CQ bit was checked by dconnelly@chromium.org

[commit-bot: I haz the power, 2014-08-26 09:26:36]

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

[commit-bot: I haz the power, 2014-08-26 10:31:24]

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

[commit-bot: I haz the power, 2014-08-26 11:55:26]

Committed patchset #7 (120001) as c34d2eb0fffd348b6fd050f7e0ca5a035a5a75fd

[commit-bot: I haz the power, 2014-09-10 02:42:18]

Patchset 7 (id:??) landed as
https://crrev.com/ac419494095f23a3350c112439134448ae7de625
Cr-Commit-Position: refs/heads/master@{#291889}