Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1139913004:
[Smart Lock] Branding string (Google Smart Lock) in Mac save password infobar should be a link. (Closed)
Description[Smart Lock] Branding string (Google Smart Lock) in Mac save password infobar should be a link.
R=vabr@chromium.org
BUG=486739
Committed: https://crrev.com/a42db114bb2cf8ed04556e8755b7689a492d4b35
Cr-Commit-Position: refs/heads/master@{#329766}
Patch Set 1 #
Total comments: 4
Patch Set 2 : #
Total comments: 10
Patch Set 3 : #
Total comments: 6
Patch Set 4 : #
Messages
Total messages: 15 (4 generated)
Hi Vaclav, can you have a look first and then I submit to owner for review. P.S. You can find a screenshot in corresponding bug. Thanks in advance.
Thanks, Tanja, LGTM. And sorry for missing this yesterday. Cheers, Vaclav https://codereview.chromium.org/1139913004/diff/1/chrome/browser/ui/cocoa/pas... File chrome/browser/ui/cocoa/passwords/manage_passwords_bubble_pending_view_controller.mm (right): https://codereview.chromium.org/1139913004/diff/1/chrome/browser/ui/cocoa/pas... chrome/browser/ui/cocoa/passwords/manage_passwords_bubble_pending_view_controller.mm:10: #include "skia/ext/skia_utils_mac.h" nit: Should this be after the current line 16? https://codereview.chromium.org/1139913004/diff/1/chrome/browser/ui/cocoa/pas... chrome/browser/ui/cocoa/passwords/manage_passwords_bubble_pending_view_controller.mm:106: [titleView_ addLinkRange:model_->title_brand_link_range().ToNSRange() Just to be sure -- if the range is empty, this will just not show anything, at the link will not be clickable (because it will have 0 pixels), correct?
melandory@chromium.org changed reviewers: + avi@chromium.org
Hi Avi, do you have time to review this CL. Ideally we want to land it before M44 branch point. Thanks in advance. https://codereview.chromium.org/1139913004/diff/1/chrome/browser/ui/cocoa/pas... File chrome/browser/ui/cocoa/passwords/manage_passwords_bubble_pending_view_controller.mm (right): https://codereview.chromium.org/1139913004/diff/1/chrome/browser/ui/cocoa/pas... chrome/browser/ui/cocoa/passwords/manage_passwords_bubble_pending_view_controller.mm:10: #include "skia/ext/skia_utils_mac.h" On 2015/05/13 09:10:17, vabr (Chromium) wrote: > nit: Should this be after the current line 16? Yeah, my automatic sort is broken because of mixed #import and #include Thanks for catching. https://codereview.chromium.org/1139913004/diff/1/chrome/browser/ui/cocoa/pas... chrome/browser/ui/cocoa/passwords/manage_passwords_bubble_pending_view_controller.mm:106: [titleView_ addLinkRange:model_->title_brand_link_range().ToNSRange() On 2015/05/13 09:10:17, vabr (Chromium) wrote: > Just to be sure -- if the range is empty, this will just not show anything, at > the link will not be clickable (because it will have 0 pixels), correct? Yes.
https://codereview.chromium.org/1139913004/diff/20001/chrome/browser/ui/cocoa... File chrome/browser/ui/cocoa/passwords/manage_passwords_bubble_pending_view_controller.mm (right): https://codereview.chromium.org/1139913004/diff/20001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/passwords/manage_passwords_bubble_pending_view_controller.mm:72: atIndex:(NSUInteger)charIndex { please align on : https://codereview.chromium.org/1139913004/diff/20001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/passwords/manage_passwords_bubble_pending_view_controller.mm:76: } space after this function https://codereview.chromium.org/1139913004/diff/20001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/passwords/manage_passwords_bubble_pending_view_controller.mm:86: // ----------------------------------- Are you going to update this diagram with the new reality of what you're building below? https://codereview.chromium.org/1139913004/diff/20001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/passwords/manage_passwords_bubble_pending_view_controller.mm:94: // Title. You need to space this block of code out! Put a blank line before each comment line. https://codereview.chromium.org/1139913004/diff/20001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/passwords/manage_passwords_bubble_pending_view_controller.mm:120: value:[NSNumber numberWithInt:NSUnderlineStyleNone] @(NSUnderlineStyleNone)
Patchset #3 (id:40001) has been deleted
https://codereview.chromium.org/1139913004/diff/20001/chrome/browser/ui/cocoa... File chrome/browser/ui/cocoa/passwords/manage_passwords_bubble_pending_view_controller.mm (right): https://codereview.chromium.org/1139913004/diff/20001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/passwords/manage_passwords_bubble_pending_view_controller.mm:72: atIndex:(NSUInteger)charIndex { On 2015/05/13 15:48:57, Avi wrote: > please align on : Done. https://codereview.chromium.org/1139913004/diff/20001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/passwords/manage_passwords_bubble_pending_view_controller.mm:76: } On 2015/05/13 15:48:57, Avi wrote: > space after this function Done. https://codereview.chromium.org/1139913004/diff/20001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/passwords/manage_passwords_bubble_pending_view_controller.mm:86: // ----------------------------------- On 2015/05/13 15:48:57, Avi wrote: > Are you going to update this diagram with the new reality of what you're > building below? This diagram is correct. I can elaborate in comment that title might have a link. To make it more clear that link is optional I've added if clause to link creation. https://codereview.chromium.org/1139913004/diff/20001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/passwords/manage_passwords_bubble_pending_view_controller.mm:94: // Title. On 2015/05/13 15:48:57, Avi wrote: > You need to space this block of code out! Put a blank line before each comment > line. Done. https://codereview.chromium.org/1139913004/diff/20001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/passwords/manage_passwords_bubble_pending_view_controller.mm:120: value:[NSNumber numberWithInt:NSUnderlineStyleNone] On 2015/05/13 15:48:57, Avi wrote: > @(NSUnderlineStyleNone) Done.
https://codereview.chromium.org/1139913004/diff/60001/chrome/browser/ui/cocoa... File chrome/browser/ui/cocoa/passwords/manage_passwords_bubble_pending_view_controller.mm (right): https://codereview.chromium.org/1139913004/diff/60001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/passwords/manage_passwords_bubble_pending_view_controller.mm:89: // Title text depends on whenever user is signed in user who syncs paswword. This sentence is ungrammatical and strange. Perhaps: // The title text depends on whether the user is signed in and therefore syncs their password. https://codereview.chromium.org/1139913004/diff/60001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/passwords/manage_passwords_bubble_pending_view_controller.mm:102: gfx::SkColorToCalibratedNSColor(chrome_style::GetLinkColor()); linkColor is only used within the "if (titleBrandLinkRange.length) block below, so move it in there. https://codereview.chromium.org/1139913004/diff/60001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/passwords/manage_passwords_bubble_pending_view_controller.mm:123: range:titleBrandLinkRange]; align on :
https://codereview.chromium.org/1139913004/diff/60001/chrome/browser/ui/cocoa... File chrome/browser/ui/cocoa/passwords/manage_passwords_bubble_pending_view_controller.mm (right): https://codereview.chromium.org/1139913004/diff/60001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/passwords/manage_passwords_bubble_pending_view_controller.mm:89: // Title text depends on whenever user is signed in user who syncs paswword. On 2015/05/13 21:58:49, Avi wrote: > This sentence is ungrammatical and strange. Perhaps: > > // The title text depends on whether the user is signed in and therefore syncs > their password. Done. https://codereview.chromium.org/1139913004/diff/60001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/passwords/manage_passwords_bubble_pending_view_controller.mm:102: gfx::SkColorToCalibratedNSColor(chrome_style::GetLinkColor()); On 2015/05/13 21:58:49, Avi wrote: > linkColor is only used within the "if (titleBrandLinkRange.length) block below, > so move it in there. Done. https://codereview.chromium.org/1139913004/diff/60001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/passwords/manage_passwords_bubble_pending_view_controller.mm:123: range:titleBrandLinkRange]; On 2015/05/13 21:58:49, Avi wrote: > align on : Done.
lgtm
The CQ bit was checked by melandory@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from vabr@chromium.org Link to the patchset: https://codereview.chromium.org/1139913004/#ps80001 (title: " ")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1139913004/80001
Message was sent while issue was closed.
Committed patchset #4 (id:80001)
Message was sent while issue was closed.
Patchset 4 (id:??) landed as https://crrev.com/a42db114bb2cf8ed04556e8755b7689a492d4b35 Cr-Commit-Position: refs/heads/master@{#329766} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
