Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
308833002:
[Mac] Password generation: Make "saved passwords" a hyperlink. (Closed)
|
Created:
6 years, 6 months ago by Patrick Dubroy Modified:
6 years, 6 months ago CC:
chromium-reviews, benquan, Dane Wallinga, dyu1, rouslan+autofillwatch_chromium.org, estade+watch_chromium.org, gcasto+watchlist_chromium.org, mkwst+watchlist_chromium.org, dconnelly, Garrett Casto Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
Description[Mac] Password generation: Make "saved passwords" a hyperlink.
BUG=114092
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=278017
Patch Set 1 #
Total comments: 8
Patch Set 2 : Address comments. #
Total comments: 6
Patch Set 3 : Address groby's comments. #
Messages
Total messages: 22 (0 generated)
groby, PTAL. dconnelly and gcasto: FYI
FYI, I think that you need an owner's review for chrome/browser/ui/autofill/password_generation_popup_controller_impl.cc. (No owner is in the reviewers list.)
isherman@ as autofill OWNER - PTAL https://codereview.chromium.org/308833002/diff/1/chrome/browser/ui/cocoa/auto... File chrome/browser/ui/cocoa/autofill/password_generation_popup_view_cocoa.mm (right): https://codereview.chromium.org/308833002/diff/1/chrome/browser/ui/cocoa/auto... chrome/browser/ui/cocoa/autofill/password_generation_popup_view_cocoa.mm:64: passwordView_ = [self textViewWithText:controller_->password() I'm not clear why you're switching the password field to NSTextView? Why not just change positionTextView:inRect: to take an NSView instead? (And call it positionView:inRect) https://codereview.chromium.org/308833002/diff/1/chrome/browser/ui/cocoa/auto... chrome/browser/ui/cocoa/autofill/password_generation_popup_view_cocoa.mm:74: helpTextView_ = IIRC, we prefer scoped_nsobject over autoreleased objects.
https://codereview.chromium.org/308833002/diff/1/chrome/browser/ui/autofill/p... File chrome/browser/ui/autofill/password_generation_popup_controller_impl.cc (right): https://codereview.chromium.org/308833002/diff/1/chrome/browser/ui/autofill/p... chrome/browser/ui/autofill/password_generation_popup_controller_impl.cc:103: {} This change doesn't seem relevant to the thrust of your CL. Please revert it. Then, you get the bonus of needing fewer reviewers :)
On 2014/05/30 19:44:22, Ilya Sherman wrote: > https://codereview.chromium.org/308833002/diff/1/chrome/browser/ui/autofill/p... > File chrome/browser/ui/autofill/password_generation_popup_controller_impl.cc > (right): > > https://codereview.chromium.org/308833002/diff/1/chrome/browser/ui/autofill/p... > chrome/browser/ui/autofill/password_generation_popup_controller_impl.cc:103: {} > This change doesn't seem relevant to the thrust of your CL. Please revert it. > Then, you get the bonus of needing fewer reviewers :) Serious?
On 2014/05/30 19:57:36, Patrick Dubroy wrote: > On 2014/05/30 19:44:22, Ilya Sherman wrote: > > > https://codereview.chromium.org/308833002/diff/1/chrome/browser/ui/autofill/p... > > File chrome/browser/ui/autofill/password_generation_popup_controller_impl.cc > > (right): > > > > > https://codereview.chromium.org/308833002/diff/1/chrome/browser/ui/autofill/p... > > chrome/browser/ui/autofill/password_generation_popup_controller_impl.cc:103: > {} > > This change doesn't seem relevant to the thrust of your CL. Please revert it. > > > Then, you get the bonus of needing fewer reviewers :) > > Serious? Yes, serious. More specifically, I don't think your change is quite right. I don't think it's really worth bikeshedding about such a minor thing as part of this CL, especially since there's no need to modify that file. Thus, I'd prefer that you simply revert the change.
https://codereview.chromium.org/308833002/diff/1/chrome/browser/ui/autofill/p... File chrome/browser/ui/autofill/password_generation_popup_controller_impl.cc (right): https://codereview.chromium.org/308833002/diff/1/chrome/browser/ui/autofill/p... chrome/browser/ui/autofill/password_generation_popup_controller_impl.cc:103: {} On 2014/05/30 19:44:22, Ilya Sherman wrote: > This change doesn't seem relevant to the thrust of your CL. Please revert it. > Then, you get the bonus of needing fewer reviewers :) FWIW clang format gives: PasswordGenerationPopupControllerImpl:: ~PasswordGenerationPopupControllerImpl() { } which is certainly novel.
https://codereview.chromium.org/308833002/diff/1/chrome/browser/ui/autofill/p... File chrome/browser/ui/autofill/password_generation_popup_controller_impl.cc (right): https://codereview.chromium.org/308833002/diff/1/chrome/browser/ui/autofill/p... chrome/browser/ui/autofill/password_generation_popup_controller_impl.cc:103: {} On 2014/05/30 22:50:59, Evan Stade wrote: > On 2014/05/30 19:44:22, Ilya Sherman wrote: > > This change doesn't seem relevant to the thrust of your CL. Please revert it. > > > Then, you get the bonus of needing fewer reviewers :) > > FWIW clang format gives: > > PasswordGenerationPopupControllerImpl:: > ~PasswordGenerationPopupControllerImpl() { > } > > which is certainly novel. (As long as we're bikeshedding, the clang format formatting is consistent with what I'd expect.)
https://codereview.chromium.org/308833002/diff/1/chrome/browser/ui/autofill/p... File chrome/browser/ui/autofill/password_generation_popup_controller_impl.cc (right): https://codereview.chromium.org/308833002/diff/1/chrome/browser/ui/autofill/p... chrome/browser/ui/autofill/password_generation_popup_controller_impl.cc:103: {} On 2014/05/30 19:44:22, Ilya Sherman wrote: > This change doesn't seem relevant to the thrust of your CL. Please revert it. > Then, you get the bonus of needing fewer reviewers :) Done. https://codereview.chromium.org/308833002/diff/1/chrome/browser/ui/cocoa/auto... File chrome/browser/ui/cocoa/autofill/password_generation_popup_view_cocoa.mm (right): https://codereview.chromium.org/308833002/diff/1/chrome/browser/ui/cocoa/auto... chrome/browser/ui/cocoa/autofill/password_generation_popup_view_cocoa.mm:64: passwordView_ = [self textViewWithText:controller_->password() On 2014/05/30 18:05:01, groby wrote: > I'm not clear why you're switching the password field to NSTextView? Why not > just change positionTextView:inRect: to take an NSView instead? (And call it > positionView:inRect) It's not really necessary, I just thought NSTextView was slightly more appropriate than NSTextField, and that future changes might be simpler if they were all NSTextViews. But, I've reverted these changes. https://codereview.chromium.org/308833002/diff/1/chrome/browser/ui/cocoa/auto... chrome/browser/ui/cocoa/autofill/password_generation_popup_view_cocoa.mm:74: helpTextView_ = On 2014/05/30 18:05:01, groby wrote: > IIRC, we prefer scoped_nsobject over autoreleased objects. Done. Though I'm not sure whether you meant that I should create a scoped_nsobject as a local, or rather make the member a scoped_nsobject.
groby: ping. PTAL and CQ if you are happy with it.
Sorry for the delay. LGTM w/ nits https://codereview.chromium.org/308833002/diff/30001/chrome/browser/ui/cocoa/... File chrome/browser/ui/cocoa/autofill/password_generation_popup_view_cocoa.mm (right): https://codereview.chromium.org/308833002/diff/30001/chrome/browser/ui/cocoa/... chrome/browser/ui/cocoa/autofill/password_generation_popup_view_cocoa.mm:81: linkColor:[NSColor blueColor]]; nit: You might want to get link color via gfx::SkColorToCalibratedNSColor(chrome_style::GetLinkColor()) https://codereview.chromium.org/308833002/diff/30001/chrome/browser/ui/cocoa/... chrome/browser/ui/cocoa/autofill/password_generation_popup_view_cocoa.mm:138: - (BOOL)textView:(NSTextView *)textView NSTextView* - remove space https://codereview.chromium.org/308833002/diff/30001/chrome/browser/ui/cocoa/... chrome/browser/ui/cocoa/autofill/password_generation_popup_view_cocoa.mm:140: atIndex:(NSUInteger)charIndex { if (controller_)? I don't _think_ this case can happen, but that relies on other classes doing the right thing.
I won't be able to land this for at least a week, as I'm currently traveling without my laptop. If someone wants to land it sooner, please feel free, otherwise I'll get to it when I'm back. On 2014/06/04 18:39:56, groby wrote: > Sorry for the delay. > > LGTM w/ nits > > https://codereview.chromium.org/308833002/diff/30001/chrome/browser/ui/cocoa/... > File chrome/browser/ui/cocoa/autofill/password_generation_popup_view_cocoa.mm > (right): > > https://codereview.chromium.org/308833002/diff/30001/chrome/browser/ui/cocoa/... > chrome/browser/ui/cocoa/autofill/password_generation_popup_view_cocoa.mm:81: > linkColor:[NSColor blueColor]]; > nit: You might want to get link color via > gfx::SkColorToCalibratedNSColor(chrome_style::GetLinkColor()) > > https://codereview.chromium.org/308833002/diff/30001/chrome/browser/ui/cocoa/... > chrome/browser/ui/cocoa/autofill/password_generation_popup_view_cocoa.mm:138: - > (BOOL)textView:(NSTextView *)textView > NSTextView* - remove space > > https://codereview.chromium.org/308833002/diff/30001/chrome/browser/ui/cocoa/... > chrome/browser/ui/cocoa/autofill/password_generation_popup_view_cocoa.mm:140: > atIndex:(NSUInteger)charIndex { > if (controller_)? > > I don't _think_ this case can happen, but that relies on other classes doing the > right thing.
Hey Patrick, will you be able to land this? If not, would you mind doing so Rachel?
The CQ bit was checked by dubroy@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/dubroy@chromium.org/308833002/50001
https://codereview.chromium.org/308833002/diff/30001/chrome/browser/ui/cocoa/... File chrome/browser/ui/cocoa/autofill/password_generation_popup_view_cocoa.mm (right): https://codereview.chromium.org/308833002/diff/30001/chrome/browser/ui/cocoa/... chrome/browser/ui/cocoa/autofill/password_generation_popup_view_cocoa.mm:81: linkColor:[NSColor blueColor]]; On 2014/06/04 18:39:56, groby wrote: > nit: You might want to get link color via > gfx::SkColorToCalibratedNSColor(chrome_style::GetLinkColor()) Done. https://codereview.chromium.org/308833002/diff/30001/chrome/browser/ui/cocoa/... chrome/browser/ui/cocoa/autofill/password_generation_popup_view_cocoa.mm:138: - (BOOL)textView:(NSTextView *)textView On 2014/06/04 18:39:56, groby wrote: > NSTextView* - remove space Done. https://codereview.chromium.org/308833002/diff/30001/chrome/browser/ui/cocoa/... chrome/browser/ui/cocoa/autofill/password_generation_popup_view_cocoa.mm:140: atIndex:(NSUInteger)charIndex { On 2014/06/04 18:39:56, groby wrote: > if (controller_)? > > I don't _think_ this case can happen, but that relies on other classes doing the > right thing. If it's ok with you, I'd rather not put the check in. It should not be possible, and if it is, we should crash rather than silently do nothing.
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: win_gpu_triggered_tests on tryserver.chromium.gpu (http://build.chromium.org/p/tryserver.chromium.gpu/builders/win_gpu_triggered...)
SLGTM
The CQ bit was checked by dubroy@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/dubroy@chromium.org/308833002/50001
Message was sent while issue was closed.
Change committed as 278017 |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
