Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
220163007:
implement account removal card and add title to signin card on mac (Closed)
|
Created:
6 years, 8 months ago by guohui Modified:
6 years, 8 months ago CC:
chromium-reviews Base URL:
https://chromium.googlesource.com/chromium/src.git@master Visibility:
Public. |
Descriptionimplement account removal card and add title to signin card on mac
For mock please see slides 8-19 at https://docs.google.com/a/google.com/presentation/d/1UMAexroivw01osOXpnEqbQcE3FMyDkc9eZoIszm4DP4/edit#slide=id.g12716c62f_064
Snapshot at https://x20web.corp.google.com/~guohui/remove_secondary_mac.png
https://x20web.corp.google.com/~guohui/remove_primary_mac.png
https://x20web.corp.google.com/~guohui/signin_title_mac.png
BUG=345075, 354462
R=asvitkine@chromium.org, noms@chromium.org
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=261517
Patch Set 1 : #
Total comments: 16
Patch Set 2 : comments addressed and unit test fixed #
Total comments: 15
Patch Set 3 : leak and nits fixed #Patch Set 4 : expanded comments on accountIdToRemove_ #
Total comments: 6
Patch Set 5 : nits fixed #Patch Set 6 : tests fixed #
Messages
Total messages: 20 (0 generated)
Hey, could you please take a look at the CL? Thanks, Hui
https://codereview.chromium.org/220163007/diff/20001/chrome/browser/ui/cocoa/... File chrome/browser/ui/cocoa/browser/profile_chooser_controller.mm (right): https://codereview.chromium.org/220163007/diff/20001/chrome/browser/ui/cocoa/... chrome/browser/ui/cocoa/browser/profile_chooser_controller.mm:116: NSTextField* createLabel(NSString* title, NSPoint frameOrigin) { I think this is the same as labelWithTitle, at the end of the file. https://codereview.chromium.org/220163007/diff/20001/chrome/browser/ui/cocoa/... chrome/browser/ui/cocoa/browser/profile_chooser_controller.mm:132: NSView* createTitleCard(NSRect frameRect, int messageId, Same. I think I would move this out of the anonymous namespace and to the bottom of the file, since it's an Objective-C function, though Alexei would know best. Also, since this is Obj-C code, it should follow the Obj-C standards, so: - (NSView*)createTitleCard:(NSRect)frameRect etc.
https://codereview.chromium.org/220163007/diff/20001/chrome/browser/ui/cocoa/... File chrome/browser/ui/cocoa/browser/profile_chooser_controller.mm (right): https://codereview.chromium.org/220163007/diff/20001/chrome/browser/ui/cocoa/... chrome/browser/ui/cocoa/browser/profile_chooser_controller.mm:116: NSTextField* createLabel(NSString* title, NSPoint frameOrigin) { On 2014/04/01 21:53:14, Monica Dinculescu wrote: > I think this is the same as labelWithTitle, at the end of the file. yup, this is the same as labelWithTitle, i changed it from a method in ProfileChooserController to a function in the anonymous space so that i could reuse it in other local classes in this file. createTitleCard below used this function, and originally it was implemented as a local class, thus the change here. However because createTitleCard has no local state thus i converted it to a function, so we don't really need this change for now. Though still i think the function is general enough and not specifically related to ProfileChooserController, thus it makes sense to put it in anonoymous space so that it could be reused by other local classes in the future when needed. https://codereview.chromium.org/220163007/diff/20001/chrome/browser/ui/cocoa/... chrome/browser/ui/cocoa/browser/profile_chooser_controller.mm:132: NSView* createTitleCard(NSRect frameRect, int messageId, my knowledge of Obj-C is minimum so not sure if my comment is nonsense, but i thought - (NSView*)createTitleCard:(NSRect)frameRect declares a method inside ProfileCHooserController instead of a function. I think the function here is generic and could be potentially reused by other local classes in this file, thus i make it a function in anonymous space instead of a method in ProfileChooserController class. Though as i said, i knew nothing about obj-C and i don't have a strong opinion for this one, so i ll leave it to Alexei.
https://codereview.chromium.org/220163007/diff/20001/chrome/browser/ui/cocoa/... File chrome/browser/ui/cocoa/browser/profile_chooser_controller.mm (right): https://codereview.chromium.org/220163007/diff/20001/chrome/browser/ui/cocoa/... chrome/browser/ui/cocoa/browser/profile_chooser_controller.mm:116: NSTextField* createLabel(NSString* title, NSPoint frameOrigin) { On 2014/04/01 22:44:58, guohui wrote: > On 2014/04/01 21:53:14, Monica Dinculescu wrote: > > I think this is the same as labelWithTitle, at the end of the file. > > yup, this is the same as labelWithTitle, i changed it from a method in > ProfileChooserController to a function in the anonymous space so that i could > reuse it in other local classes in this file. > > createTitleCard below used this function, and originally it was implemented as a > local class, thus the change here. However because createTitleCard has no local > state thus i converted it to a function, so we don't really need this change for > now. > > Though still i think the function is general enough and not specifically related > to ProfileChooserController, thus it makes sense to put it in anonoymous space > so that it could be reused by other local classes in the future when needed. I don't have a problem with having such functions in the anon namespace. Though, please use C++ conventions for them (capitalize name, use hacker_style naming conventions, align params, etc) and make sure they have a short comment before them. https://codereview.chromium.org/220163007/diff/20001/chrome/browser/ui/cocoa/... chrome/browser/ui/cocoa/browser/profile_chooser_controller.mm:653: int tag = [sender tag]; Add a comment about this logic. https://codereview.chromium.org/220163007/diff/20001/chrome/browser/ui/cocoa/... chrome/browser/ui/cocoa/browser/profile_chooser_controller.mm:667: VLOG(1) << "removeAccountAndRelaunch called"; Are you planning to remove this line? https://codereview.chromium.org/220163007/diff/20001/chrome/browser/ui/cocoa/... chrome/browser/ui/cocoa/browser/profile_chooser_controller.mm:896: createLabel( Can you rename the function to BuildLabel() rather than CreateLabel()? The word "Create" in Cocoa/ObjC has a special meaning (i.e. that it returns a ref to the caller), which is not the case here - since it returns an autoreleased ref. I know there's some other code in this file that already uses the word "create" erroneously like this (which I hadn't caught in earlier reviews of this file). https://codereview.chromium.org/220163007/diff/20001/chrome/browser/ui/cocoa/... chrome/browser/ui/cocoa/browser/profile_chooser_controller.mm:1153: [accountButton setTag:-1]; This should have a comment. It might be good to make the -1 a constant so that other code can check for it explicitly rather than doing < 0. https://codereview.chromium.org/220163007/diff/20001/chrome/browser/ui/cocoa/... chrome/browser/ui/cocoa/browser/profile_chooser_controller.mm:1199: - (NSView*) createAccountRemovalView { Nit: No space after (NSView*). Also fix in the function above.
https://codereview.chromium.org/220163007/diff/20001/chrome/browser/ui/cocoa/... File chrome/browser/ui/cocoa/browser/profile_chooser_controller.mm (right): https://codereview.chromium.org/220163007/diff/20001/chrome/browser/ui/cocoa/... chrome/browser/ui/cocoa/browser/profile_chooser_controller.mm:116: NSTextField* createLabel(NSString* title, NSPoint frameOrigin) { On 2014/04/02 15:00:51, Alexei Svitkine wrote: > On 2014/04/01 22:44:58, guohui wrote: > > On 2014/04/01 21:53:14, Monica Dinculescu wrote: > > > I think this is the same as labelWithTitle, at the end of the file. > > > > yup, this is the same as labelWithTitle, i changed it from a method in > > ProfileChooserController to a function in the anonymous space so that i could > > reuse it in other local classes in this file. > > > > createTitleCard below used this function, and originally it was implemented as > a > > local class, thus the change here. However because createTitleCard has no > local > > state thus i converted it to a function, so we don't really need this change > for > > now. > > > > Though still i think the function is general enough and not specifically > related > > to ProfileChooserController, thus it makes sense to put it in anonoymous space > > so that it could be reused by other local classes in the future when needed. > > I don't have a problem with having such functions in the anon namespace. > > Though, please use C++ conventions for them (capitalize name, use hacker_style > naming conventions, align params, etc) and make sure they have a short comment > before them. Done. https://codereview.chromium.org/220163007/diff/20001/chrome/browser/ui/cocoa/... chrome/browser/ui/cocoa/browser/profile_chooser_controller.mm:653: int tag = [sender tag]; On 2014/04/02 15:00:51, Alexei Svitkine wrote: > Add a comment about this logic. Done. https://codereview.chromium.org/220163007/diff/20001/chrome/browser/ui/cocoa/... chrome/browser/ui/cocoa/browser/profile_chooser_controller.mm:667: VLOG(1) << "removeAccountAndRelaunch called"; On 2014/04/02 15:00:51, Alexei Svitkine wrote: > Are you planning to remove this line? Done. https://codereview.chromium.org/220163007/diff/20001/chrome/browser/ui/cocoa/... chrome/browser/ui/cocoa/browser/profile_chooser_controller.mm:896: createLabel( On 2014/04/02 15:00:51, Alexei Svitkine wrote: > Can you rename the function to BuildLabel() rather than CreateLabel()? > > The word "Create" in Cocoa/ObjC has a special meaning (i.e. that it returns a > ref to the caller), which is not the case here - since it returns an > autoreleased ref. I know there's some other code in this file that already uses > the word "create" erroneously like this (which I hadn't caught in earlier > reviews of this file). Done. https://codereview.chromium.org/220163007/diff/20001/chrome/browser/ui/cocoa/... chrome/browser/ui/cocoa/browser/profile_chooser_controller.mm:1153: [accountButton setTag:-1]; On 2014/04/02 15:00:51, Alexei Svitkine wrote: > This should have a comment. It might be good to make the -1 a constant so that > other code can check for it explicitly rather than doing < 0. Done. https://codereview.chromium.org/220163007/diff/20001/chrome/browser/ui/cocoa/... chrome/browser/ui/cocoa/browser/profile_chooser_controller.mm:1199: - (NSView*) createAccountRemovalView { On 2014/04/02 15:00:51, Alexei Svitkine wrote: > Nit: No space after (NSView*). Also fix in the function above. Done.
https://codereview.chromium.org/220163007/diff/60001/chrome/browser/ui/cocoa/... File chrome/browser/ui/cocoa/browser/profile_chooser_controller.h (right): https://codereview.chromium.org/220163007/diff/60001/chrome/browser/ui/cocoa/... chrome/browser/ui/cocoa/browser/profile_chooser_controller.h:53: // The account id to remove. Please expand this comment to talk about when this is set and what it's used for. https://codereview.chromium.org/220163007/diff/60001/chrome/browser/ui/cocoa/... File chrome/browser/ui/cocoa/browser/profile_chooser_controller.mm (right): https://codereview.chromium.org/220163007/diff/60001/chrome/browser/ui/cocoa/... chrome/browser/ui/cocoa/browser/profile_chooser_controller.mm:136: NSView* CreateTitleCard(NSRect frameRect, int messageId, This is named correctly in terms of the fact it's returning a ref to the caller, but from the way it's being used, it looks like the callers don't take care of freeing the ref. I suggest making this return an auto-released ref similar to the function above and then changing its name to BuildTitleCard. https://codereview.chromium.org/220163007/diff/60001/chrome/browser/ui/cocoa/... chrome/browser/ui/cocoa/browser/profile_chooser_controller.mm:137: id backButtonTarget, SEL backButtonAction) { Nit: 1 param per line. https://codereview.chromium.org/220163007/diff/60001/chrome/browser/ui/cocoa/... chrome/browser/ui/cocoa/browser/profile_chooser_controller.mm:141: [[HoverImageButton alloc] initWithFrame:frameRect]; You're leaking a ref to this. You're allocating it here and then the object has a ref count of 1. Then you pass it to -addSubview:, which will increment its ref count (i.e. you're not transferring "ownership"). Then, since you don't unref this, the object will have a ref count of 2 but only one place (the view its on) holding a ref to it. To fix it, you can use a scoped_nsobject here or call -autorelease on it. https://codereview.chromium.org/220163007/diff/60001/chrome/browser/ui/cocoa/... chrome/browser/ui/cocoa/browser/profile_chooser_controller.mm:668: LOG(WARNING) << "accountIdToRemove_ set to " << accountIdToRemove_; Are you planning to delete this too? https://codereview.chromium.org/220163007/diff/60001/chrome/browser/ui/cocoa/... chrome/browser/ui/cocoa/browser/profile_chooser_controller.mm:1204: - (NSView*)createAccountRemovalView { Rename this to -buildAccountRemovalView, since this is not transferring ownership. Also, same thing for createGaiaEmbeddedView. https://codereview.chromium.org/220163007/diff/60001/chrome/browser/ui/cocoa/... chrome/browser/ui/cocoa/browser/profile_chooser_controller.mm:1220: [[BlueLabelButton alloc] initWithFrame:NSZeroRect]; This is leaking a ref. Please fix. https://codereview.chromium.org/220163007/diff/60001/chrome/browser/ui/cocoa/... chrome/browser/ui/cocoa/browser/profile_chooser_controller.mm:1240: NSTextField* contentLabel = BuildLabel(contentStr, Nit: Just wrap after =
Thanks a lot Alexei! please take another look. https://codereview.chromium.org/220163007/diff/60001/chrome/browser/ui/cocoa/... File chrome/browser/ui/cocoa/browser/profile_chooser_controller.h (right): https://codereview.chromium.org/220163007/diff/60001/chrome/browser/ui/cocoa/... chrome/browser/ui/cocoa/browser/profile_chooser_controller.h:53: // The account id to remove. On 2014/04/02 19:22:44, Alexei Svitkine wrote: > Please expand this comment to talk about when this is set and what it's used > for. Done. https://codereview.chromium.org/220163007/diff/60001/chrome/browser/ui/cocoa/... File chrome/browser/ui/cocoa/browser/profile_chooser_controller.mm (right): https://codereview.chromium.org/220163007/diff/60001/chrome/browser/ui/cocoa/... chrome/browser/ui/cocoa/browser/profile_chooser_controller.mm:136: NSView* CreateTitleCard(NSRect frameRect, int messageId, On 2014/04/02 19:22:44, Alexei Svitkine wrote: > This is named correctly in terms of the fact it's returning a ref to the caller, > but from the way it's being used, it looks like the callers don't take care of > freeing the ref. > > I suggest making this return an auto-released ref similar to the function above > and then changing its name to BuildTitleCard. Done. https://codereview.chromium.org/220163007/diff/60001/chrome/browser/ui/cocoa/... chrome/browser/ui/cocoa/browser/profile_chooser_controller.mm:137: id backButtonTarget, SEL backButtonAction) { On 2014/04/02 19:22:44, Alexei Svitkine wrote: > Nit: 1 param per line. Done. https://codereview.chromium.org/220163007/diff/60001/chrome/browser/ui/cocoa/... chrome/browser/ui/cocoa/browser/profile_chooser_controller.mm:668: LOG(WARNING) << "accountIdToRemove_ set to " << accountIdToRemove_; On 2014/04/02 19:22:44, Alexei Svitkine wrote: > Are you planning to delete this too? Done. https://codereview.chromium.org/220163007/diff/60001/chrome/browser/ui/cocoa/... chrome/browser/ui/cocoa/browser/profile_chooser_controller.mm:1204: - (NSView*)createAccountRemovalView { On 2014/04/02 19:22:44, Alexei Svitkine wrote: > Rename this to -buildAccountRemovalView, since this is not transferring > ownership. Also, same thing for createGaiaEmbeddedView. Done. https://codereview.chromium.org/220163007/diff/60001/chrome/browser/ui/cocoa/... chrome/browser/ui/cocoa/browser/profile_chooser_controller.mm:1220: [[BlueLabelButton alloc] initWithFrame:NSZeroRect]; On 2014/04/02 19:22:44, Alexei Svitkine wrote: > This is leaking a ref. Please fix. Done. https://codereview.chromium.org/220163007/diff/60001/chrome/browser/ui/cocoa/... chrome/browser/ui/cocoa/browser/profile_chooser_controller.mm:1240: NSTextField* contentLabel = BuildLabel(contentStr, On 2014/04/02 19:22:44, Alexei Svitkine wrote: > Nit: Just wrap after = Done.
lgtm with nits from me https://codereview.chromium.org/220163007/diff/100001/chrome/browser/ui/cocoa... File chrome/browser/ui/cocoa/browser/profile_chooser_controller.mm (right): https://codereview.chromium.org/220163007/diff/100001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/browser/profile_chooser_controller.mm:119: // Bulids a label with the given |title| and anchored at |frame_origin|. nit: typo (Builds) https://codereview.chromium.org/220163007/diff/100001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/browser/profile_chooser_controller.mm:134: // Creates a title card with one back button right aligned and one label center nit: Should this be s/Creates/Builds as well?
FYI, I think the permissions on those screenshots are wrong, as I get a 403 when trying to get to them...:(
LGTM https://codereview.chromium.org/220163007/diff/100001/chrome/browser/ui/cocoa... File chrome/browser/ui/cocoa/browser/profile_chooser_controller.mm (right): https://codereview.chromium.org/220163007/diff/100001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/browser/profile_chooser_controller.mm:1207: CHECK(!accountIdToRemove_.empty()); Nit: CHECK -> DCHECK. Usually, we only use CHECK for things like security sensitive code.
https://codereview.chromium.org/220163007/diff/100001/chrome/browser/ui/cocoa... File chrome/browser/ui/cocoa/browser/profile_chooser_controller.mm (right): https://codereview.chromium.org/220163007/diff/100001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/browser/profile_chooser_controller.mm:119: // Bulids a label with the given |title| and anchored at |frame_origin|. On 2014/04/03 15:12:55, Monica Dinculescu wrote: > nit: typo (Builds) Done. https://codereview.chromium.org/220163007/diff/100001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/browser/profile_chooser_controller.mm:134: // Creates a title card with one back button right aligned and one label center On 2014/04/03 15:12:55, Monica Dinculescu wrote: > nit: Should this be s/Creates/Builds as well? Done. https://codereview.chromium.org/220163007/diff/100001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/browser/profile_chooser_controller.mm:1207: CHECK(!accountIdToRemove_.empty()); On 2014/04/03 15:20:53, Alexei Svitkine wrote: > Nit: CHECK -> DCHECK. > > Usually, we only use CHECK for things like security sensitive code. Done.
The CQ bit was checked by guohui@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/guohui@chromium.org/220163007/120001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: tryserver.chromium on mac_chromium_rel
The CQ bit was checked by guohui@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/guohui@chromium.org/220163007/140001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: tryserver.chromium on linux_chromium_chromeos_rel
Message was sent while issue was closed.
Committed patchset #6 manually as r261517 (presubmit successful). |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
