Add ManagePasswordItemViewController and unit tests.

Add ManagePasswordItemViewController and unit tests.

Currently only implements the Pending item view.

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

[dconnelly, 2014-08-01 09:30:07]

Still WIP. Sorry for the mail spam.

[dconnelly, 2014-08-01 11:02:43]

This is very small. Other states and transitions and therefore more
sophisticated tests will come in later CLs.

[dconnelly, 2014-08-01 11:03:45]

+vabr for components/password_manager

[vabr (Chromium), 2014-08-01 11:28:40]

On 2014/08/01 11:03:45, dconnelly wrote:
> +vabr for components/password_manager

Moving the PasswordItemPosition enum to the core of the p.m. component LGTM.

[Mike West, 2014-08-01 11:50:25]

c/b/ui/views/passwords LGTM, thanks!

[dconnelly, 2014-08-01 12:30:42]

FYI I added another unit test to ensure that the fields are populated properly
and the password is always obscured. No functional changes.

[dconnelly, 2014-08-04 15:53:31]

-rohitrao
+shess

[dconnelly, 2014-08-06 11:29:31]

Scott: PTAL. The CLs are all blocked in an order specified by their issue
descriptions. Thanks!

[chromium-reviews, 2014-08-06 11:30:16]

Also, it's not a WIP. Not sure why the subject line hasn't changed. Fixed
manually, I hope.


On Wed, Aug 6, 2014 at 1:29 PM, <dconnelly@chromium.org> wrote:

> Scott: PTAL. The CLs are all blocked in an order specified by their issue
> descriptions. Thanks!
>
> https://codereview.chromium.org/438893002/
>



-- 
Daniel Connelly | Software Engineer – Chrome | dconnelly@google.com |

Google Germany GmbH
Dienerstr. 12
80331 München

Registergericht und -nummer: Hamburg, HRB 86891
Sitz der Gesellschaft: Hamburg
Geschäftsführer: Graham Law, Christine Elizabeth Flores

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[Scott Hess - ex-Googler, 2014-08-06 17:56:38]

https://codereview.chromium.org/438893002/diff/80001/chrome/browser/ui/cocoa/...
File chrome/browser/ui/cocoa/passwords/OWNERS (right):

https://codereview.chromium.org/438893002/diff/80001/chrome/browser/ui/cocoa/...
chrome/browser/ui/cocoa/passwords/OWNERS:1: dconnelly@chromium.org
Is this intended to cause people to loop you in for reviews?  AFAICT you really
haven't made many CLs under chrome/browser/ui/cocoa, so I'm not entirely sure
it's appropriate.  But I don't know for sure what our metrics are for evaluating
things.

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

https://codereview.chromium.org/438893002/diff/80001/chrome/browser/ui/cocoa/...
chrome/browser/ui/cocoa/passwords/manage_password_item_view_controller.mm:21:
void InitLabel(NSTextField* textField, base::string16 text) {
const base::string16& seems in order.  Also in other uses below.  Compiler can
probably intuit this, but might as well lock it down...

https://codereview.chromium.org/438893002/diff/80001/chrome/browser/ui/cocoa/...
chrome/browser/ui/cocoa/passwords/manage_password_item_view_controller.mm:53: //
Layout the elements, starting at the bottom-left and moving up.
Elements are laid out horizontally, though?

https://codereview.chromium.org/438893002/diff/80001/chrome/browser/ui/cocoa/...
chrome/browser/ui/cocoa/passwords/manage_password_item_view_controller.mm:63:
curX += [usernameField_ frame].size.width + views::kItemLabelSpacing;
Use NSWidth() and NSHeight().

https://codereview.chromium.org/438893002/diff/80001/chrome/browser/ui/cocoa/...
chrome/browser/ui/cocoa/passwords/manage_password_item_view_controller.mm:70:
curY += [passwordField_ frame].size.height;
These might make sense as NSMaxX() and NSMaxY().  The comment mentions padding?

https://codereview.chromium.org/438893002/diff/80001/chrome/browser/ui/cocoa/...
chrome/browser/ui/cocoa/passwords/manage_password_item_view_controller.mm:112:
// TODO(dconnelly): Implement.
NOTIMPLEMENTED() for these?

https://codereview.chromium.org/438893002/diff/80001/chrome/browser/ui/cocoa/...
chrome/browser/ui/cocoa/passwords/manage_password_item_view_controller.mm:145:
// Mac views don't have backing layers by default.
I will freely admit that I have no idea what the subtleties of animation layers
are.

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

https://codereview.chromium.org/438893002/diff/80001/chrome/browser/ui/cocoa/...
chrome/browser/ui/cocoa/passwords/manage_password_item_view_controller_unittest.mm:38:
new ManagePasswordsUIControllerMock(test_web_contents_.get());
This is marked weak, so it's owned by test_web_contents_?

https://codereview.chromium.org/438893002/diff/80001/chrome/browser/ui/cocoa/...
chrome/browser/ui/cocoa/passwords/manage_password_item_view_controller_unittest.mm:43:
scoped_ptr<ManagePasswordsBubbleModel> model_;
Since they aren't setup by the test class, why are these two variables in here
rather than in the specific tests which use them?

https://codereview.chromium.org/438893002/diff/80001/chrome/browser/ui/cocoa/...
chrome/browser/ui/cocoa/passwords/manage_password_item_view_controller_unittest.mm:68:
NSString* password = @"bar";
NSString* const kUsername, etc.

[dconnelly, 2014-08-07 13:10:05]

I've also merged in a test refactoring that simplifies the fixture in this CL.

https://codereview.chromium.org/438893002/diff/80001/chrome/browser/ui/cocoa/...
File chrome/browser/ui/cocoa/passwords/OWNERS (right):

https://codereview.chromium.org/438893002/diff/80001/chrome/browser/ui/cocoa/...
chrome/browser/ui/cocoa/passwords/OWNERS:1: dconnelly@chromium.org
On 2014/08/06 17:56:38, Scott Hess wrote:
> Is this intended to cause people to loop you in for reviews?

Yes. Was duplicating the views/ side, where our team (Privacy) members who wrote
the views/ side own chrome/browser/ui/views/passwords.

>  AFAICT you really
> haven't made many CLs under chrome/browser/ui/cocoa, so I'm not entirely sure
> it's appropriate.  But I don't know for sure what our metrics are for
evaluating
> things.

No problem. Removed.

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

https://codereview.chromium.org/438893002/diff/80001/chrome/browser/ui/cocoa/...
chrome/browser/ui/cocoa/passwords/manage_password_item_view_controller.mm:21:
void InitLabel(NSTextField* textField, base::string16 text) {
On 2014/08/06 17:56:38, Scott Hess wrote:
> const base::string16& seems in order.  Also in other uses below.  Compiler can
> probably intuit this, but might as well lock it down...

Done.

https://codereview.chromium.org/438893002/diff/80001/chrome/browser/ui/cocoa/...
chrome/browser/ui/cocoa/passwords/manage_password_item_view_controller.mm:53: //
Layout the elements, starting at the bottom-left and moving up.
On 2014/08/06 17:56:38, Scott Hess wrote:
> Elements are laid out horizontally, though?

Comment updated.

https://codereview.chromium.org/438893002/diff/80001/chrome/browser/ui/cocoa/...
chrome/browser/ui/cocoa/passwords/manage_password_item_view_controller.mm:63:
curX += [usernameField_ frame].size.width + views::kItemLabelSpacing;
On 2014/08/06 17:56:38, Scott Hess wrote:
> Use NSWidth() and NSHeight().

Done.

https://codereview.chromium.org/438893002/diff/80001/chrome/browser/ui/cocoa/...
chrome/browser/ui/cocoa/passwords/manage_password_item_view_controller.mm:70:
curY += [passwordField_ frame].size.height;
On 2014/08/06 17:56:38, Scott Hess wrote:
> These might make sense as NSMaxX() and NSMaxY().  The comment mentions
padding?

Done.

Comment updated.

https://codereview.chromium.org/438893002/diff/80001/chrome/browser/ui/cocoa/...
chrome/browser/ui/cocoa/passwords/manage_password_item_view_controller.mm:112:
// TODO(dconnelly): Implement.
On 2014/08/06 17:56:38, Scott Hess wrote:
> NOTIMPLEMENTED() for these?

Done.

https://codereview.chromium.org/438893002/diff/80001/chrome/browser/ui/cocoa/...
chrome/browser/ui/cocoa/passwords/manage_password_item_view_controller.mm:145:
// Mac views don't have backing layers by default.
On 2014/08/06 17:56:38, Scott Hess wrote:
> I will freely admit that I have no idea what the subtleties of animation
layers
> are.

Neither do I. This took me two hours to figure out.

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

https://codereview.chromium.org/438893002/diff/80001/chrome/browser/ui/cocoa/...
chrome/browser/ui/cocoa/passwords/manage_password_item_view_controller_unittest.mm:38:
new ManagePasswordsUIControllerMock(test_web_contents_.get());
On 2014/08/06 17:56:38, Scott Hess wrote:
> This is marked weak, so it's owned by test_web_contents_?

Comment updated.

https://codereview.chromium.org/438893002/diff/80001/chrome/browser/ui/cocoa/...
chrome/browser/ui/cocoa/passwords/manage_password_item_view_controller_unittest.mm:43:
scoped_ptr<ManagePasswordsBubbleModel> model_;
On 2014/08/06 17:56:38, Scott Hess wrote:
> Since they aren't setup by the test class, why are these two variables in here
> rather than in the specific tests which use them?

Done.

https://codereview.chromium.org/438893002/diff/80001/chrome/browser/ui/cocoa/...
chrome/browser/ui/cocoa/passwords/manage_password_item_view_controller_unittest.mm:68:
NSString* password = @"bar";
On 2014/08/06 17:56:38, Scott Hess wrote:
> NSString* const kUsername, etc.

For some bizarre reason, SysNSStringToUTF16 doesn't accept const NSStrings.

[Scott Hess - ex-Googler, 2014-08-07 22:05:33]

LGTM, except I'm really just rubber-stamping the CALayer stuff.

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

https://codereview.chromium.org/438893002/diff/80001/chrome/browser/ui/cocoa/...
chrome/browser/ui/cocoa/passwords/manage_password_item_view_controller.mm:145:
// Mac views don't have backing layers by default.
On 2014/08/07 13:10:05, dconnelly wrote:
> On 2014/08/06 17:56:38, Scott Hess wrote:
> > I will freely admit that I have no idea what the subtleties of animation
> layers
> > are.
> 
> Neither do I. This took me two hours to figure out.

It might be worthwhile to loop in someone who has an idea about this.  Maybe
erikchen?

My main concern is that I have no idea if this is sensible, so in reviewing it
I'd be like "If it compiles and looks right, I guess it works", which is not
really satisfactory.

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

https://codereview.chromium.org/438893002/diff/80001/chrome/browser/ui/cocoa/...
chrome/browser/ui/cocoa/passwords/manage_password_item_view_controller_unittest.mm:68:
NSString* password = @"bar";
On 2014/08/07 13:10:05, dconnelly wrote:
> On 2014/08/06 17:56:38, Scott Hess wrote:
> > NSString* const kUsername, etc.
> 
> For some bizarre reason, SysNSStringToUTF16 doesn't accept const NSStrings.

Are you saying "const NSString* kUsername" (pointer to const NSString) or
"NSString* const kUsername" (const pointer to NSString)?  If the latter case,
that is bizarre, because the const should apply to whether the object the
variable points to can be changed, which should be gone by the time you pass it
as a parameter.

Regardless, kConstant naming seems in order to document the intent of the
variable, even if you cannot enforce it.

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

https://codereview.chromium.org/438893002/diff/120001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/passwords/manage_password_item_view_controller.mm:17: //
TODO(dconnelly): Figure out how to share all these constants.
I think you figured this out in a different review?

[dconnelly, 2014-08-08 08:25:53]

The CQ bit was checked by dconnelly@chromium.org

[dconnelly, 2014-08-08 08:25:59]

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

https://codereview.chromium.org/438893002/diff/80001/chrome/browser/ui/cocoa/...
chrome/browser/ui/cocoa/passwords/manage_password_item_view_controller.mm:145:
// Mac views don't have backing layers by default.
On 2014/08/07 22:05:33, Scott Hess wrote:
> On 2014/08/07 13:10:05, dconnelly wrote:
> > On 2014/08/06 17:56:38, Scott Hess wrote:
> > > I will freely admit that I have no idea what the subtleties of animation
> > layers
> > > are.
> > 
> > Neither do I. This took me two hours to figure out.
> 
> It might be worthwhile to loop in someone who has an idea about this.  Maybe
> erikchen?
> 
> My main concern is that I have no idea if this is sensible, so in reviewing it
> I'd be like "If it compiles and looks right, I guess it works", which is not
> really satisfactory.

Okay, I read the following resources and am now convinced that this is
reasonable:
-
http://www.raywenderlich.com/2502/calayers-tutorial-for-ios-introduction-to-c...
- http://stackoverflow.com/a/9552461 (Mac CALayers)

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

https://codereview.chromium.org/438893002/diff/80001/chrome/browser/ui/cocoa/...
chrome/browser/ui/cocoa/passwords/manage_password_item_view_controller_unittest.mm:68:
NSString* password = @"bar";
On 2014/08/07 22:05:33, Scott Hess wrote:
> On 2014/08/07 13:10:05, dconnelly wrote:
> > On 2014/08/06 17:56:38, Scott Hess wrote:
> > > NSString* const kUsername, etc.
> > 
> > For some bizarre reason, SysNSStringToUTF16 doesn't accept const NSStrings.
> 
> Are you saying "const NSString* kUsername" (pointer to const NSString) or
> "NSString* const kUsername" (const pointer to NSString)?  If the latter case,
> that is bizarre, because the const should apply to whether the object the
> variable points to can be changed, which should be gone by the time you pass
it
> as a parameter.
> 
> Regardless, kConstant naming seems in order to document the intent of the
> variable, even if you cannot enforce it.

I was being dumb. Done.

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

https://codereview.chromium.org/438893002/diff/120001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/passwords/manage_password_item_view_controller.mm:17: //
TODO(dconnelly): Figure out how to share all these constants.
On 2014/08/07 22:05:33, Scott Hess wrote:
> I think you figured this out in a different review?

That review (the one that follows this one) adds a new base class, moves these
constants, and removes them from this file. I'd prefer not to make this review
larger by adding new irrelevant files. I think these are fine to stay for one
CL.

[commit-bot: I haz the power, 2014-08-08 08:27:58]

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

[commit-bot: I haz the power, 2014-08-08 11:18:42]

Change committed as 288332