Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
943453003:
Add tooltips to profile name when truncated (Closed)
|
Created:
5 years, 10 months ago by wesleylancel Modified:
5 years, 9 months ago CC:
chromium-reviews Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionAdd tooltips to profile name when truncated
This adds tooltips to profile names in both the old and new profile
switcher when they are truncated on OSX.
This lines up with behaviour on Windows.
Also added myself to the AUTHORS file seeing this is my first contribution.
BUG=459572
Committed: https://crrev.com/30f4cc56a384a941fffe7de277c80c3088cce4e3
Cr-Commit-Position: refs/heads/master@{#318769}
Patch Set 1 #
Total comments: 4
Patch Set 2 : Incorporate feedback for profile name tooltips #Patch Set 3 : Add setAllowsExpansionToolTips in sdk_forward_declarations #
Total comments: 4
Patch Set 4 : Fix allowsExpansionToolTips in sdk_forward_declarations #Patch Set 5 : Actually include the sdk_forward_declarations #
Total comments: 8
Patch Set 6 : Fix code style issues #
Total comments: 7
Patch Set 7 : Styling issues #
Messages
Total messages: 40 (7 generated)
wesleylancel@gmail.com changed reviewers: + asvitkine@chromium.org, avi@chromium.org, groby@chromium.org
wesleylancel@gmail.com changed reviewers: + mlerman@chromium.org, noms@chromium.org
noms@chromium.org changed reviewers: - asvitkine@chromium.org, avi@chromium.org, mlerman@chromium.org
Oh! You only need one owner and maybe one other reviewer at most if the patch is complicated :) I've left Rachel on, because she's awesome, and kicked everyone else off to clear their plate. Hurray!
groby@chromium.org changed reviewers: + asvitkine@chromium.org, avi@chromium.org, mlerman@chromium.org
Congrats on your first patch! I do have a little bit of feedback, though... :) https://codereview.chromium.org/943453003/diff/1/chrome/browser/ui/cocoa/prof... File chrome/browser/ui/cocoa/profiles/avatar_menu_bubble_controller.mm (right): https://codereview.chromium.org/943453003/diff/1/chrome/browser/ui/cocoa/prof... chrome/browser/ui/cocoa/profiles/avatar_menu_bubble_controller.mm:162: [nameField setAllowsExpansionToolTips:YES]; This is only available in 10.8 or later. You'll need to check respondsToSelector:, since we're building against a 10.6 base SDK https://codereview.chromium.org/943453003/diff/1/chrome/browser/ui/cocoa/prof... File chrome/browser/ui/cocoa/profiles/profile_chooser_controller.mm (right): https://codereview.chromium.org/943453003/diff/1/chrome/browser/ui/cocoa/prof... chrome/browser/ui/cocoa/profiles/profile_chooser_controller.mm:704: Question: Why not allow expansion tooltips on |profileNameTextField_| instead? (I assume the containing control intercepts those, but I want to make sure we can't do that) https://codereview.chromium.org/943453003/diff/1/chrome/browser/ui/cocoa/prof... chrome/browser/ui/cocoa/profiles/profile_chooser_controller.mm:706: @{NSFontAttributeName: [NSFont labelFontOfSize:kTitleFontSize]}]; [profileNameTextField_ font] instead, please. https://codereview.chromium.org/943453003/diff/1/chrome/browser/ui/cocoa/prof... chrome/browser/ui/cocoa/profiles/profile_chooser_controller.mm:708: if (textSize.width > frameRect.size.width - [hoverImage size].width * 2) { ... > NSWidth(frameRect) (We're usually avoiding reaching into NSRect members, if we can)
noms@chromium.org changed reviewers: - asvitkine@chromium.org, avi@chromium.org, mlerman@chromium.org
The new bubble in ProfileChooserController also has a list of other profiles (behind the --enable-fast-user-switching flag in about://flags) that I think could get truncated (see -createOtherProfileView:). Would you mind adding the tooltip to that as well, please? :)
On 2015/02/19 19:00:10, groby wrote: > Congrats on your first patch! Thanks! > > I do have a little bit of feedback, though... :) I was expecting this :) > > https://codereview.chromium.org/943453003/diff/1/chrome/browser/ui/cocoa/prof... > File chrome/browser/ui/cocoa/profiles/avatar_menu_bubble_controller.mm (right): > > https://codereview.chromium.org/943453003/diff/1/chrome/browser/ui/cocoa/prof... > chrome/browser/ui/cocoa/profiles/avatar_menu_bubble_controller.mm:162: > [nameField setAllowsExpansionToolTips:YES]; > This is only available in 10.8 or later. You'll need to check > respondsToSelector:, since we're building against a 10.6 base SDK I'll look into this. > > https://codereview.chromium.org/943453003/diff/1/chrome/browser/ui/cocoa/prof... > File chrome/browser/ui/cocoa/profiles/profile_chooser_controller.mm (right): > > https://codereview.chromium.org/943453003/diff/1/chrome/browser/ui/cocoa/prof... > chrome/browser/ui/cocoa/profiles/profile_chooser_controller.mm:704: > Question: Why not allow expansion tooltips on |profileNameTextField_| instead? > (I assume the containing control intercepts those, but I want to make sure we > can't do that) The textfield is only showing after clicking the button to edit the name, adding a tooltip to that won't show it when not editing. > > https://codereview.chromium.org/943453003/diff/1/chrome/browser/ui/cocoa/prof... > chrome/browser/ui/cocoa/profiles/profile_chooser_controller.mm:706: > @{NSFontAttributeName: [NSFont labelFontOfSize:kTitleFontSize]}]; > [profileNameTextField_ font] instead, please. Will change. > > https://codereview.chromium.org/943453003/diff/1/chrome/browser/ui/cocoa/prof... > chrome/browser/ui/cocoa/profiles/profile_chooser_controller.mm:708: if > (textSize.width > frameRect.size.width - [hoverImage size].width * 2) { > ... > NSWidth(frameRect) > > (We're usually avoiding reaching into NSRect members, if we can) Any suggestions on how I could prevent that here?
On 2015/02/19 19:07:47, Monica Dinculescu wrote: > The new bubble in ProfileChooserController also has a list of other profiles > (behind the --enable-fast-user-switching flag in about://flags) that I think > could get truncated (see -createOtherProfileView:). Would you mind adding the > tooltip to that as well, please? :) Will take a look too.
I've added a new patchset that should contain most of the feedback.
The build error is due to the message being unknown. You might want to add it to base/mac/sdk_forward_declarations.h (in a MountainLionSDK category, inside the 10.8 section)
On 2015/02/19 22:52:54, groby wrote: > The build error is due to the message being unknown. You might want to add it to > base/mac/sdk_forward_declarations.h (in a MountainLionSDK category, inside the > 10.8 section) Would that be something like this? @interface NSObject (MountainLionSDK) - (void)setAllowsExpansionToolTips; @end
On 2015/02/19 23:10:10, wesleylancel wrote: > On 2015/02/19 22:52:54, groby wrote: > > The build error is due to the message being unknown. You might want to add it > to > > base/mac/sdk_forward_declarations.h (in a MountainLionSDK category, inside the > > 10.8 section) > > Would that be something like this? > > @interface NSObject (MountainLionSDK) > - (void)setAllowsExpansionToolTips; > @end Yep.
On 2015/02/19 23:54:02, groby wrote: > On 2015/02/19 23:10:10, wesleylancel wrote: > > On 2015/02/19 22:52:54, groby wrote: > > > The build error is due to the message being unknown. You might want to add > it > > to > > > base/mac/sdk_forward_declarations.h (in a MountainLionSDK category, inside > the > > > 10.8 section) > > > > Would that be something like this? > > > > @interface NSObject (MountainLionSDK) > > - (void)setAllowsExpansionToolTips; > > @end > > Yep. Tried adding that, but unfortunately it's still failing. Any hints on what's wrong?
Hope this helps fixing the issue. https://codereview.chromium.org/943453003/diff/40001/base/mac/sdk_forward_dec... File base/mac/sdk_forward_declarations.h (right): https://codereview.chromium.org/943453003/diff/40001/base/mac/sdk_forward_dec... base/mac/sdk_forward_declarations.h:288: @interface NSObject (MountainLionSDK) You want NSControl here. https://codereview.chromium.org/943453003/diff/40001/base/mac/sdk_forward_dec... base/mac/sdk_forward_declarations.h:289: - (void)setAllowsExpansionToolTips; And here, you want @property BOOL allowsExpansionToolTips (It would also works with a method declaration, but the one you added here is wrong - it doesn't take a BOOL param) https://codereview.chromium.org/943453003/diff/40001/chrome/browser/ui/cocoa/... File chrome/browser/ui/cocoa/profiles/profile_chooser_controller.mm (right): https://codereview.chromium.org/943453003/diff/40001/chrome/browser/ui/cocoa/... chrome/browser/ui/cocoa/profiles/profile_chooser_controller.mm:1818: if (lroundf(textSize.width) > Why round here?
On 2015/02/20 17:01:07, groby wrote: > Hope this helps fixing the issue. > > https://codereview.chromium.org/943453003/diff/40001/base/mac/sdk_forward_dec... > File base/mac/sdk_forward_declarations.h (right): > > https://codereview.chromium.org/943453003/diff/40001/base/mac/sdk_forward_dec... > base/mac/sdk_forward_declarations.h:288: @interface NSObject (MountainLionSDK) > You want NSControl here. > > https://codereview.chromium.org/943453003/diff/40001/base/mac/sdk_forward_dec... > base/mac/sdk_forward_declarations.h:289: - (void)setAllowsExpansionToolTips; > And here, you want > @property BOOL allowsExpansionToolTips > > (It would also works with a method declaration, but the one you added here is > wrong - it doesn't take a BOOL param) Thanks, will submit a new (and hopefully final) patchset fixing this. > > https://codereview.chromium.org/943453003/diff/40001/chrome/browser/ui/cocoa/... > File chrome/browser/ui/cocoa/profiles/profile_chooser_controller.mm (right): > > https://codereview.chromium.org/943453003/diff/40001/chrome/browser/ui/cocoa/... > chrome/browser/ui/cocoa/profiles/profile_chooser_controller.mm:1818: if > (lroundf(textSize.width) > > Why round here? I'm not really sure why, but the built in truncation doesn't seem to happen when for example the text width is 100.307 pixels and there's 100 pixels space, it probably rounds too.
https://codereview.chromium.org/943453003/diff/40001/base/mac/sdk_forward_dec... File base/mac/sdk_forward_declarations.h (right): https://codereview.chromium.org/943453003/diff/40001/base/mac/sdk_forward_dec... base/mac/sdk_forward_declarations.h:289: - (void)setAllowsExpansionToolTips; On 2015/02/20 17:01:07, groby wrote: > And here, you want > @property BOOL allowsExpansionToolTips > > (It would also works with a method declaration, but the one you added here is > wrong - it doesn't take a BOOL param) Changed it to this, unfortunately still giving errors on compile.
On 2015/02/21 18:51:41, wesleylancel wrote: > https://codereview.chromium.org/943453003/diff/40001/base/mac/sdk_forward_dec... > File base/mac/sdk_forward_declarations.h (right): > > https://codereview.chromium.org/943453003/diff/40001/base/mac/sdk_forward_dec... > base/mac/sdk_forward_declarations.h:289: - (void)setAllowsExpansionToolTips; > On 2015/02/20 17:01:07, groby wrote: > > And here, you want > > @property BOOL allowsExpansionToolTips > > > > (It would also works with a method declaration, but the one you added here is > > wrong - it doesn't take a BOOL param) > > Changed it to this, unfortunately still giving errors on compile. @groby: Finally managed to resolve the issue!
i'm just here with drive-by nits :) https://codereview.chromium.org/943453003/diff/80001/chrome/browser/ui/cocoa/... File chrome/browser/ui/cocoa/profiles/avatar_menu_bubble_controller.mm (right): https://codereview.chromium.org/943453003/diff/80001/chrome/browser/ui/cocoa/... chrome/browser/ui/cocoa/profiles/avatar_menu_bubble_controller.mm:163: if ([nameField respondsToSelector:@selector(setAllowsExpansionToolTips:)]) { nit: no {} for one-line if statements. https://codereview.chromium.org/943453003/diff/80001/chrome/browser/ui/cocoa/... File chrome/browser/ui/cocoa/profiles/profile_chooser_controller.mm (right): https://codereview.chromium.org/943453003/diff/80001/chrome/browser/ui/cocoa/... chrome/browser/ui/cocoa/profiles/profile_chooser_controller.mm:708: if (textSize.width > frameRect.size.width - [hoverImage size].width * 2) { nit: no {}, only 2 spaces indent on the second line https://codereview.chromium.org/943453003/diff/80001/chrome/browser/ui/cocoa/... chrome/browser/ui/cocoa/profiles/profile_chooser_controller.mm:1819: rect.size.width - kSmallImageSide - nit: move the thing on the right hand side of the > to its own local variable. it's pretty unreadable :) https://codereview.chromium.org/943453003/diff/80001/chrome/browser/ui/cocoa/... chrome/browser/ui/cocoa/profiles/profile_chooser_controller.mm:1821: [profileButton setToolTip:[profileButton title]]; nit: only indent 2 spaces
https://codereview.chromium.org/943453003/diff/80001/chrome/browser/ui/cocoa/... File chrome/browser/ui/cocoa/profiles/avatar_menu_bubble_controller.mm (right): https://codereview.chromium.org/943453003/diff/80001/chrome/browser/ui/cocoa/... chrome/browser/ui/cocoa/profiles/avatar_menu_bubble_controller.mm:163: if ([nameField respondsToSelector:@selector(setAllowsExpansionToolTips:)]) { On 2015/02/23 22:16:34, Monica Dinculescu wrote: > nit: no {} for one-line if statements. Done. https://codereview.chromium.org/943453003/diff/80001/chrome/browser/ui/cocoa/... File chrome/browser/ui/cocoa/profiles/profile_chooser_controller.mm (right): https://codereview.chromium.org/943453003/diff/80001/chrome/browser/ui/cocoa/... chrome/browser/ui/cocoa/profiles/profile_chooser_controller.mm:708: if (textSize.width > frameRect.size.width - [hoverImage size].width * 2) { On 2015/02/23 22:16:34, Monica Dinculescu wrote: > nit: no {}, only 2 spaces indent on the second line Done. https://codereview.chromium.org/943453003/diff/80001/chrome/browser/ui/cocoa/... chrome/browser/ui/cocoa/profiles/profile_chooser_controller.mm:1819: rect.size.width - kSmallImageSide - On 2015/02/23 22:16:34, Monica Dinculescu wrote: > nit: move the thing on the right hand side of the > to its own local variable. > it's pretty unreadable :) Done. https://codereview.chromium.org/943453003/diff/80001/chrome/browser/ui/cocoa/... chrome/browser/ui/cocoa/profiles/profile_chooser_controller.mm:1821: [profileButton setToolTip:[profileButton title]]; On 2015/02/23 22:16:34, Monica Dinculescu wrote: > nit: only indent 2 spaces Done.
One last one... (Also, if you want to avoid formatting issues 'git cl format' does a lot of the formatting automatically) https://codereview.chromium.org/943453003/diff/100001/chrome/browser/ui/cocoa... File chrome/browser/ui/cocoa/profiles/profile_chooser_controller.mm (right): https://codereview.chromium.org/943453003/diff/100001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/profiles/profile_chooser_controller.mm:1820: if (lroundf(textSize.width) > availableWidth) Please use std::ceil instead.
https://codereview.chromium.org/943453003/diff/100001/chrome/browser/ui/cocoa... File chrome/browser/ui/cocoa/profiles/profile_chooser_controller.mm (right): https://codereview.chromium.org/943453003/diff/100001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/profiles/profile_chooser_controller.mm:1820: if (lroundf(textSize.width) > availableWidth) On 2015/02/24 00:37:06, groby wrote: > Please use std::ceil instead. I actually need rounding and not ceiling here otherwise it won't work and still show the tooltip even though the text is not truncated. Want me to replace it with something else then?
On 2015/02/24 08:39:32, wesleylancel wrote: > https://codereview.chromium.org/943453003/diff/100001/chrome/browser/ui/cocoa... > File chrome/browser/ui/cocoa/profiles/profile_chooser_controller.mm (right): > > https://codereview.chromium.org/943453003/diff/100001/chrome/browser/ui/cocoa... > chrome/browser/ui/cocoa/profiles/profile_chooser_controller.mm:1820: if > (lroundf(textSize.width) > availableWidth) > On 2015/02/24 00:37:06, groby wrote: > > Please use std::ceil instead. > > I actually need rounding and not ceiling here otherwise it won't work and still > show the tooltip even though the text is not truncated. Want me to replace it > with something else then? groby: what can I do to make this commitable? :)
Sorry about the delay - was in class for the last week. Reviewing now.
https://codereview.chromium.org/943453003/diff/100001/chrome/browser/ui/cocoa... File chrome/browser/ui/cocoa/profiles/profile_chooser_controller.mm (right): https://codereview.chromium.org/943453003/diff/100001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/profiles/profile_chooser_controller.mm:1820: if (lroundf(textSize.width) > availableWidth) On 2015/02/24 08:39:31, wesleylancel wrote: > On 2015/02/24 00:37:06, groby wrote: > > Please use std::ceil instead. > > I actually need rounding and not ceiling here otherwise it won't work and still > show the tooltip even though the text is not truncated. Want me to replace it > with something else then? Maybe I misunderstand, but isn't ceil(width) strictly greater than lroundf(width)? I.e ceil: 1.0 -> 1.0, anything >1.0 and <2.0 ->2.0 round: anything >1.0 and <=1.5 ->1, >1.5 and <2.0 ->2 What am I missing?
avi@chromium.org changed reviewers: + avi@chromium.org
https://codereview.chromium.org/943453003/diff/100001/chrome/browser/ui/cocoa... File chrome/browser/ui/cocoa/profiles/profile_chooser_controller.mm (right): https://codereview.chromium.org/943453003/diff/100001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/profiles/profile_chooser_controller.mm:706: @{NSFontAttributeName: [profileNameTextField_ font]}]; Bad code style for the literal. http://google-styleguide.googlecode.com/svn/trunk/objcguide.xml#Container_Lit...
https://codereview.chromium.org/943453003/diff/100001/chrome/browser/ui/cocoa... File chrome/browser/ui/cocoa/profiles/profile_chooser_controller.mm (right): https://codereview.chromium.org/943453003/diff/100001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/profiles/profile_chooser_controller.mm:706: @{NSFontAttributeName: [profileNameTextField_ font]}]; On 2015/02/27 23:14:07, Avi wrote: > Bad code style for the literal. > > http://google-styleguide.googlecode.com/svn/trunk/objcguide.xml#Container_Lit... @AvI: needs to be 2 spaces instead of 4 then I guess? https://codereview.chromium.org/943453003/diff/100001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/profiles/profile_chooser_controller.mm:1820: if (lroundf(textSize.width) > availableWidth) On 2015/02/27 23:02:24, groby wrote: > On 2015/02/24 08:39:31, wesleylancel wrote: > > On 2015/02/24 00:37:06, groby wrote: > > > Please use std::ceil instead. > > > > I actually need rounding and not ceiling here otherwise it won't work and > still > > show the tooltip even though the text is not truncated. Want me to replace it > > with something else then? > > Maybe I misunderstand, but isn't ceil(width) strictly greater than > lroundf(width)? I.e > ceil: 1.0 -> 1.0, anything >1.0 and <2.0 ->2.0 > round: anything >1.0 and <=1.5 ->1, >1.5 and <2.0 ->2 > > What am I missing? > > Oh! I'm sorry. I totally thought I was doing something else here. Will change.
https://codereview.chromium.org/943453003/diff/100001/chrome/browser/ui/cocoa... File chrome/browser/ui/cocoa/profiles/profile_chooser_controller.mm (right): https://codereview.chromium.org/943453003/diff/100001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/profiles/profile_chooser_controller.mm:706: @{NSFontAttributeName: [profileNameTextField_ font]}]; On 2015/02/27 23:20:40, wesleylancel wrote: > On 2015/02/27 23:14:07, Avi wrote: > > Bad code style for the literal. > > > > > http://google-styleguide.googlecode.com/svn/trunk/objcguide.xml#Container_Lit... > > @AvI: needs to be 2 spaces instead of 4 then I guess? No. The literal is formatted incorrectly. The literal itself: spaces inside the braces, and around the colon.
On 2015/02/27 23:22:51, Avi wrote: > https://codereview.chromium.org/943453003/diff/100001/chrome/browser/ui/cocoa... > File chrome/browser/ui/cocoa/profiles/profile_chooser_controller.mm (right): > > https://codereview.chromium.org/943453003/diff/100001/chrome/browser/ui/cocoa... > chrome/browser/ui/cocoa/profiles/profile_chooser_controller.mm:706: > @{NSFontAttributeName: [profileNameTextField_ font]}]; > On 2015/02/27 23:20:40, wesleylancel wrote: > > On 2015/02/27 23:14:07, Avi wrote: > > > Bad code style for the literal. > > > > > > > > > http://google-styleguide.googlecode.com/svn/trunk/objcguide.xml#Container_Lit... > > > > @AvI: needs to be 2 spaces instead of 4 then I guess? > > No. The literal is formatted incorrectly. The literal itself: spaces inside the > braces, and around the colon. So: NSSize textSize = [[profileButton title] sizeWithAttributes: @{ NSFontAttributeName : [profileButton font] }];
On 2015/02/27 23:24:48, wesleylancel wrote: > On 2015/02/27 23:22:51, Avi wrote: > > > https://codereview.chromium.org/943453003/diff/100001/chrome/browser/ui/cocoa... > > File chrome/browser/ui/cocoa/profiles/profile_chooser_controller.mm (right): > > > > > https://codereview.chromium.org/943453003/diff/100001/chrome/browser/ui/cocoa... > > chrome/browser/ui/cocoa/profiles/profile_chooser_controller.mm:706: > > @{NSFontAttributeName: [profileNameTextField_ font]}]; > > On 2015/02/27 23:20:40, wesleylancel wrote: > > > On 2015/02/27 23:14:07, Avi wrote: > > > > Bad code style for the literal. > > > > > > > > > > > > > > http://google-styleguide.googlecode.com/svn/trunk/objcguide.xml#Container_Lit... > > > > > > @AvI: needs to be 2 spaces instead of 4 then I guess? > > > > No. The literal is formatted incorrectly. The literal itself: spaces inside > the > > braces, and around the colon. > > So: > > NSSize textSize = [[profileButton title] sizeWithAttributes: > @{ NSFontAttributeName : [profileButton font] }]; Appended a patch for the styling issues (mostly used `git cl format`).
On 2015/02/27 23:52:24, wesleylancel wrote: > On 2015/02/27 23:24:48, wesleylancel wrote: > > On 2015/02/27 23:22:51, Avi wrote: > > > > > > https://codereview.chromium.org/943453003/diff/100001/chrome/browser/ui/cocoa... > > > File chrome/browser/ui/cocoa/profiles/profile_chooser_controller.mm (right): > > > > > > > > > https://codereview.chromium.org/943453003/diff/100001/chrome/browser/ui/cocoa... > > > chrome/browser/ui/cocoa/profiles/profile_chooser_controller.mm:706: > > > @{NSFontAttributeName: [profileNameTextField_ font]}]; > > > On 2015/02/27 23:20:40, wesleylancel wrote: > > > > On 2015/02/27 23:14:07, Avi wrote: > > > > > Bad code style for the literal. > > > > > > > > > > > > > > > > > > > > http://google-styleguide.googlecode.com/svn/trunk/objcguide.xml#Container_Lit... > > > > > > > > @AvI: needs to be 2 spaces instead of 4 then I guess? > > > > > > No. The literal is formatted incorrectly. The literal itself: spaces inside > > the > > > braces, and around the colon. > > > > So: > > > > NSSize textSize = [[profileButton title] sizeWithAttributes: > > @{ NSFontAttributeName : [profileButton font] }]; > > Appended a patch for the styling issues (mostly used `git cl format`). That's one way to do it. Styling LGTM; leaving the rest to groby.
On 2015/02/28 03:23:24, Avi wrote: > On 2015/02/27 23:52:24, wesleylancel wrote: > > On 2015/02/27 23:24:48, wesleylancel wrote: > > > On 2015/02/27 23:22:51, Avi wrote: > > > > > > > > > > https://codereview.chromium.org/943453003/diff/100001/chrome/browser/ui/cocoa... > > > > File chrome/browser/ui/cocoa/profiles/profile_chooser_controller.mm > (right): > > > > > > > > > > > > > > https://codereview.chromium.org/943453003/diff/100001/chrome/browser/ui/cocoa... > > > > chrome/browser/ui/cocoa/profiles/profile_chooser_controller.mm:706: > > > > @{NSFontAttributeName: [profileNameTextField_ font]}]; > > > > On 2015/02/27 23:20:40, wesleylancel wrote: > > > > > On 2015/02/27 23:14:07, Avi wrote: > > > > > > Bad code style for the literal. > > > > > > > > > > > > > > > > > > > > > > > > > > > http://google-styleguide.googlecode.com/svn/trunk/objcguide.xml#Container_Lit... > > > > > > > > > > @AvI: needs to be 2 spaces instead of 4 then I guess? > > > > > > > > No. The literal is formatted incorrectly. The literal itself: spaces > inside > > > the > > > > braces, and around the colon. > > > > > > So: > > > > > > NSSize textSize = [[profileButton title] sizeWithAttributes: > > > @{ NSFontAttributeName : [profileButton font] }]; > > > > Appended a patch for the styling issues (mostly used `git cl format`). > > That's one way to do it. > > Styling LGTM; leaving the rest to groby. Thanks! @groby: The ceil is also in that patch. Good to go?
lgtm
The CQ bit was checked by wesleylancel@gmail.com
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/943453003/120001
Message was sent while issue was closed.
Committed patchset #7 (id:120001)
Message was sent while issue was closed.
Patchset 7 (id:??) landed as https://crrev.com/30f4cc56a384a941fffe7de277c80c3088cce4e3 Cr-Commit-Position: refs/heads/master@{#318769} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
