Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1157733005:
Make 'Google Smart Lock' a hyperlink in the password infobar on Mac. (Closed)
|
Created:
5 years, 6 months ago by vasilii Modified:
5 years, 6 months ago CC:
chromium-reviews, gcasto+watchlist_chromium.org, vabr+watchlist_chromium.org, mkwst+watchlist-passwords_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionMake 'Google Smart Lock' a hyperlink in the password infobar on Mac.
The infobar supports adding a hyperlink in the end. This patch allows to make any part of the infobar text a hyperlink.
BUG=493620
Committed: https://crrev.com/274026e6def29bab1cbdf2d626f8154e4cbcbdae
Cr-Commit-Position: refs/heads/master@{#334182}
Patch Set 1 #
Total comments: 2
Patch Set 2 : link color #
Total comments: 4
Patch Set 3 : gcasto@ comment #Patch Set 4 : rebase #
Total comments: 1
Patch Set 5 : implement SavePasswordBarController #
Total comments: 2
Patch Set 6 : vabr@ #
Messages
Total messages: 35 (11 generated)
vasilii@chromium.org changed reviewers: + gcasto@chromium.org, groby@chromium.org, sdefresne@chromium.org
Hi, groby@chromium.org: Please review changes in confirm_infobar_controller.mm gcasto@chromium.org: Please review changes in save_password_infobar_delegate.* sdefresne@chromium.org: Please review changes in confirm_infobar_delegate.*
https://codereview.chromium.org/1157733005/diff/1/chrome/browser/ui/cocoa/inf... File chrome/browser/ui/cocoa/infobars/confirm_infobar_controller.mm (right): https://codereview.chromium.org/1157733005/diff/1/chrome/browser/ui/cocoa/inf... chrome/browser/ui/cocoa/infobars/confirm_infobar_controller.mm:134: linkColor:[NSColor blueColor]]; Please use chrome_theme instead: NSColor* linkColor = gfx::SkColorToCalibratedNSColor(chrome_style::GetLinkColor());
https://codereview.chromium.org/1157733005/diff/1/chrome/browser/ui/cocoa/inf... File chrome/browser/ui/cocoa/infobars/confirm_infobar_controller.mm (right): https://codereview.chromium.org/1157733005/diff/1/chrome/browser/ui/cocoa/inf... chrome/browser/ui/cocoa/infobars/confirm_infobar_controller.mm:134: linkColor:[NSColor blueColor]]; On 2015/06/03 18:27:40, groby wrote: > Please use chrome_theme instead: > NSColor* linkColor = > gfx::SkColorToCalibratedNSColor(chrome_style::GetLinkColor()); Done.
c/b/ui/cocoa LGTM pending nit https://codereview.chromium.org/1157733005/diff/20001/chrome/browser/ui/cocoa... File chrome/browser/ui/cocoa/infobars/confirm_infobar_controller.mm (right): https://codereview.chromium.org/1157733005/diff/20001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/infobars/confirm_infobar_controller.mm:134: NSColor* linkColor = I hate to be *that* reviewer, but can you add this to the other -addLinkRange: call above, too?
https://codereview.chromium.org/1157733005/diff/20001/chrome/browser/password... File chrome/browser/password_manager/save_password_infobar_delegate.h (right): https://codereview.chromium.org/1157733005/diff/20001/chrome/browser/password... chrome/browser/password_manager/save_password_infobar_delegate.h:59: gfx::Range GetMessageLinkRange() const override; Is there any reason to keep |title_link_range()| after adding this?
vasilii@chromium.org changed reviewers: + miguelg@chromium.org
miguelg@chromium.org: Please review changes in chrome/browser/ui/android/infobars/save_password_infobar.cc sdefresne@chromium.org: Please review changes in confirm_infobar_delegate.* gcasto@chromium.org: Please review changes in save_password_infobar_delegate.* https://codereview.chromium.org/1157733005/diff/20001/chrome/browser/password... File chrome/browser/password_manager/save_password_infobar_delegate.h (right): https://codereview.chromium.org/1157733005/diff/20001/chrome/browser/password... chrome/browser/password_manager/save_password_infobar_delegate.h:59: gfx::Range GetMessageLinkRange() const override; On 2015/06/03 21:59:30, Garrett Casto wrote: > Is there any reason to keep |title_link_range()| after adding this? Done. The reason was to avoid 4 reviewers for 6 files. https://codereview.chromium.org/1157733005/diff/20001/chrome/browser/ui/cocoa... File chrome/browser/ui/cocoa/infobars/confirm_infobar_controller.mm (right): https://codereview.chromium.org/1157733005/diff/20001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/infobars/confirm_infobar_controller.mm:134: NSColor* linkColor = On 2015/06/03 18:57:22, groby wrote: > I hate to be *that* reviewer, but can you add this to the other -addLinkRange: > call above, too? Done.
vasilii@chromium.org changed reviewers: + dfalcantara@chromium.org, pkasting@chromium.org
Nobody wants to review my changes :-( Peter, please review confirm_infobar_delegate.* Dan, pleaser review save_password_infobar.cc
lgtm
lgtm Sorry, don't think I saw this in my review queue until just now? https://codereview.chromium.org/1157733005/diff/60001/components/infobars/cor... File components/infobars/core/confirm_infobar_delegate.h (right): https://codereview.chromium.org/1157733005/diff/60001/components/infobars/cor... components/infobars/core/confirm_infobar_delegate.h:62: // any. Otherwise returns and empty string. nit: while you're here, and -> an
(1) This patch doesn't fully implement support -- in particular, views infobars have not been updated to support this. I'd really like to see such support added before signing off. (2) If we're going to add flexibility to confirm infobars to allow links to appear anywhere, then: (a) There are a couple existing special-case InfobarDelegate subclasses who exist solely because of the need for this, which should be found and eliminated (b) Rather than hardcoding the concept of a single link range, I wonder if we should instead use a more generic "styled ranges" system a la the one in RenderText. (Maybe we should just use RenderTexts directly?) This might be overkill, though, as I think having normal text + 0 or 1 links is about all we need to do today, and if so I'd rather not overdesign. Consider asking msw, who's worked on RnderText as well as some consumers of it like views::Label, whether some of this stuff makes sense to be more generic.
vasilii@chromium.org changed reviewers: - miguelg@chromium.org, sdefresne@chromium.org
On 2015/06/06 00:31:37, Peter Kasting wrote: > (1) This patch doesn't fully implement support -- in particular, views infobars > have not been updated to support this. I'd really like to see such support > added before signing off. > > (2) If we're going to add flexibility to confirm infobars to allow links to > appear anywhere, then: > (a) There are a couple existing special-case InfobarDelegate subclasses who > exist solely because of the need for this, which should be found and eliminated > (b) Rather than hardcoding the concept of a single link range, I wonder if we > should instead use a more generic "styled ranges" system a la the one in > RenderText. (Maybe we should just use RenderTexts directly?) This might be > overkill, though, as I think having normal text + 0 or 1 links is about all we > need to do today, and if so I'd rather not overdesign. > > Consider asking msw, who's worked on RnderText as well as some consumers of it > like views::Label, whether some of this stuff makes sense to be more generic. The context for this change is that we want to rebrand the password manager and include a link. On views we are using the bubble for a long time. We are to start rolling out the bubble on Mac as well. Therefore, soon this patch will not be needed if other infobars don't use the functionality. Do you still think that the feature should exist on Views now?
On 2015/06/08 09:34:54, vasilii wrote: > On 2015/06/06 00:31:37, Peter Kasting wrote: > > (1) This patch doesn't fully implement support -- in particular, views > infobars > > have not been updated to support this. I'd really like to see such support > > added before signing off. > > > > (2) If we're going to add flexibility to confirm infobars to allow links to > > appear anywhere, then: > > (a) There are a couple existing special-case InfobarDelegate subclasses who > > exist solely because of the need for this, which should be found and > eliminated > > (b) Rather than hardcoding the concept of a single link range, I wonder if we > > should instead use a more generic "styled ranges" system a la the one in > > RenderText. (Maybe we should just use RenderTexts directly?) This might be > > overkill, though, as I think having normal text + 0 or 1 links is about all we > > need to do today, and if so I'd rather not overdesign. > > > > Consider asking msw, who's worked on RnderText as well as some consumers of it > > like views::Label, whether some of this stuff makes sense to be more generic. > > The context for this change is that we want to rebrand the password manager and > include a link. On views we are using the bubble for a long time. We are to > start rolling out the bubble on Mac as well. Therefore, soon this patch will not > be needed if other infobars don't use the functionality. Do you still think that > the feature should exist on Views now? If you never intend to use anything like this on views, and there aren't enough other such consumers to make it make sense regardless, then I'd tend to prefer you modify the save password delegate to inherit from InfoBarDelegate instead and do things the harder way, so that you don't have functionality in some platforms' implementation of core things like confirm infobars that doesn't exist in other platforms. So it's really a question of whether there are enough other infobars today that could be simplified by this to justify adding it everywhere. If not, then I'd add it nowhere.
Patchset #5 (id:80001) has been deleted
Patchset #5 (id:100001) has been deleted
vasilii@chromium.org changed reviewers: - dfalcantara@chromium.org, pkasting@chromium.org
Hi guys, I implemented this in a different way. PTAL.
pkasting@chromium.org changed reviewers: + pkasting@chromium.org
Non-cocoa code LGTM
Rachel, Garrett: please review.
vabr@chromium.org changed reviewers: + vabr@chromium.org
Passwords stuff LGTM so far, but how is the new save password infobar controller used in the current code? Cheers, Vaclav https://codereview.chromium.org/1157733005/diff/120001/chrome/browser/ui/coco... File chrome/browser/ui/cocoa/infobars/save_password_infobar_controller.mm (right): https://codereview.chromium.org/1157733005/diff/120001/chrome/browser/ui/coco... chrome/browser/ui/cocoa/infobars/save_password_infobar_controller.mm:15: @interface SavePasswordBarController : ConfirmInfoBarController nit: The file name says "infobar", while the class name just "Bar". Please unify.
On 2015/06/11 08:46:49, vabr (Chromium) wrote: > Passwords stuff LGTM so far, but how is the new save password infobar controller > used in the current code? > > Cheers, > Vaclav > > https://codereview.chromium.org/1157733005/diff/120001/chrome/browser/ui/coco... > File chrome/browser/ui/cocoa/infobars/save_password_infobar_controller.mm > (right): > > https://codereview.chromium.org/1157733005/diff/120001/chrome/browser/ui/coco... > chrome/browser/ui/cocoa/infobars/save_password_infobar_controller.mm:15: > @interface SavePasswordBarController : ConfirmInfoBarController > nit: The file name says "infobar", while the class name just "Bar". Please > unify. It's instantiated in SavePasswordInfoBarDelegate::Create via CreateSavePasswordInfoBar().
Rachel, please review. https://codereview.chromium.org/1157733005/diff/120001/chrome/browser/ui/coco... File chrome/browser/ui/cocoa/infobars/save_password_infobar_controller.mm (right): https://codereview.chromium.org/1157733005/diff/120001/chrome/browser/ui/coco... chrome/browser/ui/cocoa/infobars/save_password_infobar_controller.mm:15: @interface SavePasswordBarController : ConfirmInfoBarController On 2015/06/11 08:46:49, vabr (Chromium) wrote: > nit: The file name says "infobar", while the class name just "Bar". Please > unify. Done.
On 2015/06/11 14:34:18, vasilii wrote: > On 2015/06/11 08:46:49, vabr (Chromium) wrote: > > Passwords stuff LGTM so far, but how is the new save password infobar > controller > > used in the current code? > > > > Cheers, > > Vaclav > > > > > https://codereview.chromium.org/1157733005/diff/120001/chrome/browser/ui/coco... > > File chrome/browser/ui/cocoa/infobars/save_password_infobar_controller.mm > > (right): > > > > > https://codereview.chromium.org/1157733005/diff/120001/chrome/browser/ui/coco... > > chrome/browser/ui/cocoa/infobars/save_password_infobar_controller.mm:15: > > @interface SavePasswordBarController : ConfirmInfoBarController > > nit: The file name says "infobar", while the class name just "Bar". Please > > unify. > > It's instantiated in SavePasswordInfoBarDelegate::Create via > CreateSavePasswordInfoBar(). Right, I don't know what I was looking at the first time. :) LGTM, thanks for this CL! Vaclav
LGTM Looks like vabr@ already got to this, but looks good to me as well.
c/b/ui/cocoa LGTM
The CQ bit was checked by vasilii@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from dfalcantara@chromium.org, pkasting@chromium.org Link to the patchset: https://codereview.chromium.org/1157733005/#ps140001 (title: "vabr@")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1157733005/140001
Message was sent while issue was closed.
Committed patchset #6 (id:140001)
Message was sent while issue was closed.
Patchset 6 (id:??) landed as https://crrev.com/274026e6def29bab1cbdf2d626f8154e4cbcbdae Cr-Commit-Position: refs/heads/master@{#334182} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
