Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
878743007:
Implement CredentialItemView and tests for the Mac account chooser. (Closed)
|
Created:
5 years, 10 months ago by dconnelly Modified:
5 years, 9 months ago Reviewers:
groby-ooo-7-16 CC:
chromium-reviews Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionImplement CredentialItemView and tests for the Mac account chooser.
Depends on https://codereview.chromium.org/901493003/
BUG=448011
Committed: https://crrev.com/45b4df1826bad452cad3f122db85fc3040d0bc3d
Cr-Commit-Position: refs/heads/master@{#319084}
Patch Set 1 #
Total comments: 28
Patch Set 2 : cleanup and remove ListView #Patch Set 3 : fix order #
Total comments: 8
Patch Set 4 : memory #
Total comments: 2
Messages
Total messages: 14 (2 generated)
dconnelly@chromium.org changed reviewers: + groby@chromium.org
I'm currently swamped with other tasks, will get to this tonight/Monday morning
friendly ping :)
https://codereview.chromium.org/878743007/diff/1/chrome/browser/ui/cocoa/pass... File chrome/browser/ui/cocoa/passwords/credential_item_view.h (right): https://codereview.chromium.org/878743007/diff/1/chrome/browser/ui/cocoa/pass... chrome/browser/ui/cocoa/passwords/credential_item_view.h:18: // Handles user interaction with and image fetching for a CredentialItemView. Question: This functionality is presumably shared with non-Cocoa platforms, correct? In that case, wouldn't it make sense to have a cross-platform delegate API? Or does this need to be platform-specific? https://codereview.chromium.org/878743007/diff/1/chrome/browser/ui/cocoa/pass... chrome/browser/ui/cocoa/passwords/credential_item_view.h:57: @interface CredentialItemView(Testing) Any reason this can't just live in the test file? https://codereview.chromium.org/878743007/diff/1/chrome/browser/ui/cocoa/pass... chrome/browser/ui/cocoa/passwords/credential_item_view.h:72: @property(readonly) NSArray* items; Why not just use subviews? https://codereview.chromium.org/878743007/diff/1/chrome/browser/ui/cocoa/pass... File chrome/browser/ui/cocoa/passwords/credential_item_view.mm (right): https://codereview.chromium.org/878743007/diff/1/chrome/browser/ui/cocoa/pass... chrome/browser/ui/cocoa/passwords/credential_item_view.mm:26: @interface CredentialItemViewCell : NSButtonCell Are you sure you need an NSButton at all? :) https://codereview.chromium.org/878743007/diff/1/chrome/browser/ui/cocoa/pass... chrome/browser/ui/cocoa/passwords/credential_item_view.mm:60: avatarView_.reset([[NSImageView alloc] initWithFrame:CGRectZero]); I vaguely recall the profile switcher does similar magic for rounded frames - worth sharing code? (Not because it's tremendous accounts of code, but because the concept of applying that round frame should look identical everywhere. And I think profile switcher doesn't just use cornerRadius, but I might be wrong) https://codereview.chromium.org/878743007/diff/1/chrome/browser/ui/cocoa/pass... chrome/browser/ui/cocoa/passwords/credential_item_view.mm:121: nameFrame.origin = CGPointMake(nameX, nameLabelY); NSMakePoint https://codereview.chromium.org/878743007/diff/1/chrome/browser/ui/cocoa/pass... chrome/browser/ui/cocoa/passwords/credential_item_view.mm:128: CGRectGetWidth([usernameLabel_ frame]) NSWidth - you *do* love CG functions, don't you? ;) (here and elsewhere) https://codereview.chromium.org/878743007/diff/1/chrome/browser/ui/cocoa/pass... chrome/browser/ui/cocoa/passwords/credential_item_view.mm:192: @implementation CredentialItemListView Question: Would this be better off as an NSTableView? https://codereview.chromium.org/878743007/diff/1/chrome/browser/ui/cocoa/pass... chrome/browser/ui/cocoa/passwords/credential_item_view.mm:205: // Stack the credentials on top of each other. Shorter (and hopefully clearer) version: NSRect viewFrame = [self frame]; if ([[self subviews] count]) viewFrame.size.height += kVerticalPaddingBetweenCredentials; NSRect itemFrame = [item frame]; itemFrame.origin = NSMakePoint(0, NSHeight(viewFrame)); viewFrame = NSUnionRect(viewFrame, itemFrame); itemFrame.size.width = NSWidth(viewFrame); [item setFrame:itemFrame]; [self setFrameSize:viewFrame.size]; [self addSubview:item]; https://codereview.chromium.org/878743007/diff/1/chrome/browser/ui/cocoa/pass... chrome/browser/ui/cocoa/passwords/credential_item_view.mm:216: const CGFloat x = 0; You could replace the entire code block with [item setFrame:NSMakeRect(0, y, width, NSHeight([item frame])] (Obvs ignore if you follow the above suggestion) https://codereview.chromium.org/878743007/diff/1/chrome/browser/ui/cocoa/pass... File chrome/browser/ui/cocoa/passwords/credential_item_view_unittest.mm (right): https://codereview.chromium.org/878743007/diff/1/chrome/browser/ui/cocoa/pass... chrome/browser/ui/cocoa/passwords/credential_item_view_unittest.mm:54: return ResourceBundle::GetSharedInstance() Why not just use an empty NSImage and skip using the ResourceBundle? https://codereview.chromium.org/878743007/diff/1/chrome/browser/ui/cocoa/pass... chrome/browser/ui/cocoa/passwords/credential_item_view_unittest.mm:60: bool ImagesEqual(NSImage* left, NSImage* right) { Do you need this byte-by-byte comparison? Isn't EXPECT_NSEQ enough? (AFAICT, we only retain, but never copy/clone avatar images) https://codereview.chromium.org/878743007/diff/1/chrome/browser/ui/cocoa/pass... chrome/browser/ui/cocoa/passwords/credential_item_view_unittest.mm:70: autofill::PasswordForm credential; I vaguely recall autofill has various test helpers that create pre-filled structures. Should this be added there? https://codereview.chromium.org/878743007/diff/1/chrome/browser/ui/cocoa/pass... chrome/browser/ui/cocoa/passwords/credential_item_view_unittest.mm:121: }; Please add a TEST_VIEW test
https://codereview.chromium.org/878743007/diff/1/chrome/browser/ui/cocoa/pass... File chrome/browser/ui/cocoa/passwords/credential_item_view.h (right): https://codereview.chromium.org/878743007/diff/1/chrome/browser/ui/cocoa/pass... chrome/browser/ui/cocoa/passwords/credential_item_view.h:18: // Handles user interaction with and image fetching for a CredentialItemView. On 2015/02/19 17:01:33, groby wrote: > Question: This functionality is presumably shared with non-Cocoa platforms, > correct? > > In that case, wouldn't it make sense to have a cross-platform delegate API? Or > does this need to be platform-specific? ManagePasswordsBubbleModel is the cross-platform delegate, but it only handles stuff related to interacting with the model. This delegate is for platform-specific UI cleanup stuff -- see the implementations in the ManagePasswordsBubbleAccountChooserViewController. They mostly dismiss the view and delegate to the BubbleModel. I guess there could be a cross-platform UI delegate API, but it wouldn't be in ObjC, so I'd have to do an embedded-bridge-dance thing to implement it with a view controller. It's possible, though. WDYT? https://codereview.chromium.org/878743007/diff/1/chrome/browser/ui/cocoa/pass... chrome/browser/ui/cocoa/passwords/credential_item_view.h:57: @interface CredentialItemView(Testing) On 2015/02/19 17:01:33, groby wrote: > Any reason this can't just live in the test file? Done. https://codereview.chromium.org/878743007/diff/1/chrome/browser/ui/cocoa/pass... chrome/browser/ui/cocoa/passwords/credential_item_view.h:72: @property(readonly) NSArray* items; On 2015/02/19 17:01:33, groby wrote: > Why not just use subviews? Done. https://codereview.chromium.org/878743007/diff/1/chrome/browser/ui/cocoa/pass... File chrome/browser/ui/cocoa/passwords/credential_item_view.mm (right): https://codereview.chromium.org/878743007/diff/1/chrome/browser/ui/cocoa/pass... chrome/browser/ui/cocoa/passwords/credential_item_view.mm:26: @interface CredentialItemViewCell : NSButtonCell On 2015/02/19 17:01:33, groby wrote: > Are you sure you need an NSButton at all? :) Replaced all this handling and ListView stuff with an NSTableView in the view controller - removed https://codereview.chromium.org/878743007/diff/1/chrome/browser/ui/cocoa/pass... chrome/browser/ui/cocoa/passwords/credential_item_view.mm:60: avatarView_.reset([[NSImageView alloc] initWithFrame:CGRectZero]); On 2015/02/19 17:01:33, groby wrote: > I vaguely recall the profile switcher does similar magic for rounded frames - > worth sharing code? (Not because it's tremendous accounts of code, but because > the concept of applying that round frame should look identical everywhere. And I > think profile switcher doesn't just use cornerRadius, but I might be wrong) The profile switchers I see on trunk Chromium and Chrome Beta have square avatars -- can you point me to the dialog you mean? https://codereview.chromium.org/878743007/diff/1/chrome/browser/ui/cocoa/pass... chrome/browser/ui/cocoa/passwords/credential_item_view.mm:121: nameFrame.origin = CGPointMake(nameX, nameLabelY); On 2015/02/19 17:01:33, groby wrote: > NSMakePoint Done. https://codereview.chromium.org/878743007/diff/1/chrome/browser/ui/cocoa/pass... chrome/browser/ui/cocoa/passwords/credential_item_view.mm:128: CGRectGetWidth([usernameLabel_ frame]) On 2015/02/19 17:01:33, groby wrote: > NSWidth - you *do* love CG functions, don't you? ;) > > (here and elsewhere) Sorry, yeah, the iOS idioms are just stickier in my brain. https://codereview.chromium.org/878743007/diff/1/chrome/browser/ui/cocoa/pass... chrome/browser/ui/cocoa/passwords/credential_item_view.mm:192: @implementation CredentialItemListView On 2015/02/19 17:01:33, groby wrote: > Question: Would this be better off as an NSTableView? You know, I've never written an NSTableView before. Done. https://codereview.chromium.org/878743007/diff/1/chrome/browser/ui/cocoa/pass... chrome/browser/ui/cocoa/passwords/credential_item_view.mm:205: // Stack the credentials on top of each other. On 2015/02/19 17:01:33, groby wrote: > Shorter (and hopefully clearer) version: > > NSRect viewFrame = [self frame]; > if ([[self subviews] count]) > viewFrame.size.height += kVerticalPaddingBetweenCredentials; > > NSRect itemFrame = [item frame]; > itemFrame.origin = NSMakePoint(0, NSHeight(viewFrame)); > viewFrame = NSUnionRect(viewFrame, itemFrame); > itemFrame.size.width = NSWidth(viewFrame); > > [item setFrame:itemFrame]; > [self setFrameSize:viewFrame.size]; > [self addSubview:item]; TableView - removed all this https://codereview.chromium.org/878743007/diff/1/chrome/browser/ui/cocoa/pass... chrome/browser/ui/cocoa/passwords/credential_item_view.mm:216: const CGFloat x = 0; On 2015/02/19 17:01:33, groby wrote: > You could replace the entire code block with > [item setFrame:NSMakeRect(0, y, width, NSHeight([item frame])] > > (Obvs ignore if you follow the above suggestion) TableView - removed all this https://codereview.chromium.org/878743007/diff/1/chrome/browser/ui/cocoa/pass... File chrome/browser/ui/cocoa/passwords/credential_item_view_unittest.mm (right): https://codereview.chromium.org/878743007/diff/1/chrome/browser/ui/cocoa/pass... chrome/browser/ui/cocoa/passwords/credential_item_view_unittest.mm:54: return ResourceBundle::GetSharedInstance() On 2015/02/19 17:01:33, groby wrote: > Why not just use an empty NSImage and skip using the ResourceBundle? Done. https://codereview.chromium.org/878743007/diff/1/chrome/browser/ui/cocoa/pass... chrome/browser/ui/cocoa/passwords/credential_item_view_unittest.mm:60: bool ImagesEqual(NSImage* left, NSImage* right) { On 2015/02/19 17:01:33, groby wrote: > Do you need this byte-by-byte comparison? Isn't EXPECT_NSEQ enough? (AFAICT, we > only retain, but never copy/clone avatar images) Done. https://codereview.chromium.org/878743007/diff/1/chrome/browser/ui/cocoa/pass... chrome/browser/ui/cocoa/passwords/credential_item_view_unittest.mm:70: autofill::PasswordForm credential; On 2015/02/19 17:01:33, groby wrote: > I vaguely recall autofill has various test helpers that create pre-filled > structures. Should this be added there? These are pretty basic and skip everything that actual autofill would need, like element names and so on. I guess we could have a generic PasswordForm builder method, but is it worth it? These have basically no logic. https://codereview.chromium.org/878743007/diff/1/chrome/browser/ui/cocoa/pass... chrome/browser/ui/cocoa/passwords/credential_item_view_unittest.mm:121: }; On 2015/02/19 17:01:33, groby wrote: > Please add a TEST_VIEW test CredentialItemView can't |display| itself. Should I add this instead to the outermost view controller's tests?
just one more :)
Before I dive in further - is there any consumer for CredentialItemView? Can we add that to this CL? https://codereview.chromium.org/878743007/diff/40001/chrome/browser/ui/cocoa/... File chrome/browser/ui/cocoa/passwords/credential_item_view.h (right): https://codereview.chromium.org/878743007/diff/40001/chrome/browser/ui/cocoa/... chrome/browser/ui/cocoa/passwords/credential_item_view.h:16: @class CredentialItemView; We usually combine class/@class forward decls into one list (alphabetized) https://codereview.chromium.org/878743007/diff/40001/chrome/browser/ui/cocoa/... chrome/browser/ui/cocoa/passwords/credential_item_view.h:31: base::scoped_nsobject<NSTextField> nameLabel_; Do we want to refcount here? (I know, I contradict myself on the id<> further down - it's a general question which items actually should be refcounted/owned) https://codereview.chromium.org/878743007/diff/40001/chrome/browser/ui/cocoa/... chrome/browser/ui/cocoa/passwords/credential_item_view.h:34: id<CredentialItemDelegate> delegate_; Do you want scoped_nsprotocol here? (I.e. do we want to refcount?)
> Before I dive in further - is there any consumer for CredentialItemView? Can we add that to this CL? There is: ManagePasswordsBubbleAccountChooserViewController, which you already reviewed. There's another one coming, but it depends on another CL that you already reviewed which depends on this CL. I can send that one to someone else if that's helpful -- you seem to be very busy. https://codereview.chromium.org/878743007/diff/40001/chrome/browser/ui/cocoa/... File chrome/browser/ui/cocoa/passwords/credential_item_view.h (right): https://codereview.chromium.org/878743007/diff/40001/chrome/browser/ui/cocoa/... chrome/browser/ui/cocoa/passwords/credential_item_view.h:16: @class CredentialItemView; On 2015/03/03 17:34:33, groby wrote: > We usually combine class/@class forward decls into one list (alphabetized) Done. https://codereview.chromium.org/878743007/diff/40001/chrome/browser/ui/cocoa/... chrome/browser/ui/cocoa/passwords/credential_item_view.h:31: base::scoped_nsobject<NSTextField> nameLabel_; On 2015/03/03 17:34:33, groby wrote: > Do we want to refcount here? (I know, I contradict myself on the id<> further > down - it's a general question which items actually should be refcounted/owned) I've removed the scoping to match the other reviews you've done for me, but in general this makes me nervous. It relies on |subviews| maintaining references to these objects, and if |subviews| is changed, they're all blown away and these pointers will be invalid. I don't see the problem with some extra defensive-programming refs. https://codereview.chromium.org/878743007/diff/40001/chrome/browser/ui/cocoa/... chrome/browser/ui/cocoa/passwords/credential_item_view.h:34: id<CredentialItemDelegate> delegate_; On 2015/03/03 17:34:33, groby wrote: > Do you want scoped_nsprotocol here? (I.e. do we want to refcount?) ManagePasswordsBubbleAccountChooserViewController, which you already reviewed, both uses a CredentialItemView and is its delegate, so refcounting here would lead to a reference cycle. In Bling we use WeakNSObject/WeakNSProtocol to store auto-nilling references to delegates, but it's not available here in Mac-land. https://codereview.chromium.org/878743007/diff/60001/chrome/browser/ui/cocoa/... File chrome/browser/ui/cocoa/passwords/credential_item_view_unittest.mm (right): https://codereview.chromium.org/878743007/diff/60001/chrome/browser/ui/cocoa/... chrome/browser/ui/cocoa/passwords/credential_item_view_unittest.mm:60: bool ImagesEqual(NSImage* left, NSImage* right) { This had to come back. See comment.
lgtm https://codereview.chromium.org/878743007/diff/40001/chrome/browser/ui/cocoa/... File chrome/browser/ui/cocoa/passwords/credential_item_view.h (right): https://codereview.chromium.org/878743007/diff/40001/chrome/browser/ui/cocoa/... chrome/browser/ui/cocoa/passwords/credential_item_view.h:31: base::scoped_nsobject<NSTextField> nameLabel_; On 2015/03/04 12:30:54, dconnelly wrote: > On 2015/03/03 17:34:33, groby wrote: > > Do we want to refcount here? (I know, I contradict myself on the id<> further > > down - it's a general question which items actually should be > refcounted/owned) > > I've removed the scoping to match the other reviews you've done for me, but in > general this makes me nervous. It relies on |subviews| maintaining references to > these objects, and if |subviews| is changed, they're all blown away and these > pointers will be invalid. I don't see the problem with some extra > defensive-programming refs. Well, they're not entirely free :) I don't have a strong preference either way, though. I just wanted to make sure I understand the ownership reasoning. https://codereview.chromium.org/878743007/diff/40001/chrome/browser/ui/cocoa/... chrome/browser/ui/cocoa/passwords/credential_item_view.h:34: id<CredentialItemDelegate> delegate_; On 2015/03/04 12:30:54, dconnelly wrote: > On 2015/03/03 17:34:33, groby wrote: > > Do you want scoped_nsprotocol here? (I.e. do we want to refcount?) > > ManagePasswordsBubbleAccountChooserViewController, which you already reviewed, > both uses a CredentialItemView and is its delegate, so refcounting here would > lead to a reference cycle. In Bling we use WeakNSObject/WeakNSProtocol to store > auto-nilling references to delegates, but it's not available here in Mac-land. Acknowledged. https://codereview.chromium.org/878743007/diff/60001/chrome/browser/ui/cocoa/... File chrome/browser/ui/cocoa/passwords/credential_item_view_unittest.mm (right): https://codereview.chromium.org/878743007/diff/60001/chrome/browser/ui/cocoa/... chrome/browser/ui/cocoa/passwords/credential_item_view_unittest.mm:60: bool ImagesEqual(NSImage* left, NSImage* right) { On 2015/03/04 12:30:54, dconnelly wrote: > This had to come back. See comment. Ah well - it was worth a try :)
The CQ bit was checked by dconnelly@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/878743007/60001
Message was sent while issue was closed.
Committed patchset #4 (id:60001)
Message was sent while issue was closed.
Patchset 4 (id:??) landed as https://crrev.com/45b4df1826bad452cad3f122db85fc3040d0bc3d Cr-Commit-Position: refs/heads/master@{#319084} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
