Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2632293002:
macOS: Adding tooltip in the profile chooser (Closed)
DescriptionmacOS: Adding tooltip in the profile chooser
This CL adds a tooltip for the profile name and the email address
in the profile chooser on macOS.
BUG=674462
Review-Url: https://codereview.chromium.org/2632293002
Cr-Commit-Position: refs/heads/master@{#448579}
Committed: https://chromium.googlesource.com/chromium/src/+/c50e876921535e569078385a0d022cdd8b15a8e7
Patch Set 1 #
Total comments: 6
Patch Set 2 : Mihai's comments #
Total comments: 4
Patch Set 3 : Mihai's comments #Messages
Total messages: 18 (8 generated)
jlebel@chromium.org changed reviewers: + msarda@chromium.org
Hello Miha, Can you review this patch to add back the tooltips in the profile chooser in macOS? Thanks,
Same comment as in CL https://codereview.chromium.org/2633183002/ for the CL description. https://codereview.chromium.org/2632293002/diff/1/chrome/browser/ui/cocoa/pro... File chrome/browser/ui/cocoa/profiles/profile_chooser_controller.mm (right): https://codereview.chromium.org/2632293002/diff/1/chrome/browser/ui/cocoa/pro... chrome/browser/ui/cocoa/profiles/profile_chooser_controller.mm:2153: profileName.toolTip = base::SysUTF16ToNSString(profileNameString); profileNameString is converted from NSString to UTF16 string 2 times - please use an local variable for it. https://codereview.chromium.org/2632293002/diff/1/chrome/browser/ui/cocoa/pro... chrome/browser/ui/cocoa/profiles/profile_chooser_controller.mm:2159: Have you tried using the same test as for the username below here (and removing the test above)? if (![[profileName stringValue] isEqualToString:base::SysUTF16ToNSString(profileNameString)]) profileName.toolTip = base::SysUTF16ToNSString(profileNameString); } https://codereview.chromium.org/2632293002/diff/1/chrome/browser/ui/cocoa/pro... chrome/browser/ui/cocoa/profiles/profile_chooser_controller.mm:2174: username.toolTip = base::SysUTF16ToNSString(item.username); base::SysUTF16ToNSString(item.username) is converted from a NSString to a UTF16 string 3 times. Please add a local variable for it.
Done. https://codereview.chromium.org/2632293002/diff/1/chrome/browser/ui/cocoa/pro... File chrome/browser/ui/cocoa/profiles/profile_chooser_controller.mm (right): https://codereview.chromium.org/2632293002/diff/1/chrome/browser/ui/cocoa/pro... chrome/browser/ui/cocoa/profiles/profile_chooser_controller.mm:2153: profileName.toolTip = base::SysUTF16ToNSString(profileNameString); On 2017/01/17 13:26:44, msarda wrote: > profileNameString is converted from NSString to UTF16 string 2 times - please > use an local variable for it. Done. https://codereview.chromium.org/2632293002/diff/1/chrome/browser/ui/cocoa/pro... chrome/browser/ui/cocoa/profiles/profile_chooser_controller.mm:2159: On 2017/01/17 13:26:44, msarda wrote: > Have you tried using the same test as for the username below here (and removing > the test above)? > if (![[profileName stringValue] > isEqualToString:base::SysUTF16ToNSString(profileNameString)]) > profileName.toolTip = base::SysUTF16ToNSString(profileNameString); > } -[NSTextField stringValue] returns the value given, and not the string displayed. I was not able to find any API to know if the text has been elide. For the version below, we elide ourselves the string. So we know if it has been done. But it is better to let the OS do it, since it tries first change the spaces between letters before eliding the string, so I think it is better to leave the profile name the way it is. https://codereview.chromium.org/2632293002/diff/1/chrome/browser/ui/cocoa/pro... chrome/browser/ui/cocoa/profiles/profile_chooser_controller.mm:2174: username.toolTip = base::SysUTF16ToNSString(item.username); On 2017/01/17 13:26:44, msarda wrote: > base::SysUTF16ToNSString(item.username) is converted from a NSString to a UTF16 > string 3 times. Please add a local variable for it. Done.
Description was changed from ========== macOS: Adding tooltip for the profile name and the email address in the profile chooser (only if the string is too long). BUG=674462 ========== to ========== Adding tooltip for the profile name and the email address in the profile chooser (only if the string is too long). BUG=674462 ==========
1. The CL description in Chromium have the following structure: CL Title (less than 72 characters) CL Body Title: Add tooltip in profile chooser on macOS. Body: Add tooltip in profile chooser on macOS. This CL adds a tooltip for the profile name and the email address in the profile chooser on macOS. https://codereview.chromium.org/2632293002/diff/20001/chrome/browser/ui/cocoa... File chrome/browser/ui/cocoa/profiles/profile_chooser_controller.mm (right): https://codereview.chromium.org/2632293002/diff/20001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/profiles/profile_chooser_controller.mm:2152: // can be reduced without truncated). s/can be reduced without truncated/may be reduced to avoid truncation https://codereview.chromium.org/2632293002/diff/20001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/profiles/profile_chooser_controller.mm:2167: NSString* elideEmail = Nit: s/elideEmail/elidedEmail
Description was changed from ========== Adding tooltip for the profile name and the email address in the profile chooser (only if the string is too long). BUG=674462 ========== to ========== macOS: Adding tooltip in the profile chooser Adding tooltip for the profile name and the email address in the profile chooser (only if the string is too long). BUG=674462 ==========
Description was changed from ========== macOS: Adding tooltip in the profile chooser Adding tooltip for the profile name and the email address in the profile chooser (only if the string is too long). BUG=674462 ========== to ========== macOS: Adding tooltip in the profile chooser This CL adds a tooltip for the profile name and the email address in the profile chooser on macOS. BUG=674462 ==========
Done. https://codereview.chromium.org/2632293002/diff/20001/chrome/browser/ui/cocoa... File chrome/browser/ui/cocoa/profiles/profile_chooser_controller.mm (right): https://codereview.chromium.org/2632293002/diff/20001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/profiles/profile_chooser_controller.mm:2152: // can be reduced without truncated). On 2017/01/17 14:01:33, msarda wrote: > s/can be reduced without truncated/may be reduced to avoid truncation Done. https://codereview.chromium.org/2632293002/diff/20001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/profiles/profile_chooser_controller.mm:2167: NSString* elideEmail = On 2017/01/17 14:01:33, msarda wrote: > Nit: s/elideEmail/elidedEmail Done.
lgtm
jlebel@chromium.org changed reviewers: + mark@chromium.org
Hello Mark, Can you review this patch to add tool tip for the main profile (when the name is too long) in the profile switch menu? Thanks,
LGTM
The CQ bit was checked by jlebel@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 40001, "attempt_start_ts": 1486460534735650,
"parent_rev": "11654a90a3b307ec2178f9a3d03fb02d3c06d656", "commit_rev":
"c50e876921535e569078385a0d022cdd8b15a8e7"}
Message was sent while issue was closed.
Description was changed from ========== macOS: Adding tooltip in the profile chooser This CL adds a tooltip for the profile name and the email address in the profile chooser on macOS. BUG=674462 ========== to ========== macOS: Adding tooltip in the profile chooser This CL adds a tooltip for the profile name and the email address in the profile chooser on macOS. BUG=674462 Review-Url: https://codereview.chromium.org/2632293002 Cr-Commit-Position: refs/heads/master@{#448579} Committed: https://chromium.googlesource.com/chromium/src/+/c50e876921535e569078385a0d02... ==========
Message was sent while issue was closed.
Committed patchset #3 (id:40001) as https://chromium.googlesource.com/chromium/src/+/c50e876921535e569078385a0d02... |
