Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
487193003:
Update Mac password generation autofill popup to match latest mocks. (Closed)
|
Created:
6 years, 4 months ago by dconnelly Modified:
6 years, 4 months ago CC:
chromium-reviews, benquan, tfarina, Dane Wallinga, dyu1, rouslan+autofillwatch_chromium.org, estade+watch_chromium.org, gcasto+watchlist_chromium.org, Ilya Sherman, mkwst+watchlist_chromium.org Base URL:
svn://svn.chromium.org/chrome/trunk/src Project:
chromium Visibility:
Public. |
DescriptionUpdate Mac password generation autofill popup to match latest mocks.
Screenshots available on the bug.
BUG=394303
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=291389
Patch Set 1 #
Total comments: 62
Patch Set 2 : rewrite layout #
Total comments: 31
Patch Set 3 : cleanups #
Total comments: 6
Patch Set 4 : nits #Patch Set 5 : fix #
Messages
Total messages: 29 (0 generated)
+estade for chrome/browser/ui/views/autofill/password_generation_popup_view_views.cc
views lgtm
+Rachel for Cocoa review, as she's much more familiar with the Cocoa library than I am. https://codereview.chromium.org/487193003/diff/1/chrome/browser/ui/cocoa/auto... File chrome/browser/ui/cocoa/autofill/password_generation_popup_view_bridge.h (right): https://codereview.chromium.org/487193003/diff/1/chrome/browser/ui/cocoa/auto... chrome/browser/ui/cocoa/autofill/password_generation_popup_view_bridge.h:44: PasswordGenerationPopupController* controller_; // weak Where is this used? https://codereview.chromium.org/487193003/diff/1/chrome/browser/ui/cocoa/auto... File chrome/browser/ui/cocoa/autofill/password_generation_popup_view_cocoa.h (right): https://codereview.chromium.org/487193003/diff/1/chrome/browser/ui/cocoa/auto... chrome/browser/ui/cocoa/autofill/password_generation_popup_view_cocoa.h:37: - (BOOL)isPointInPasswordBounds:(CGPoint)point; nit: Can the |point| be passed as an NSPoint rather than as a CGPoint? https://codereview.chromium.org/487193003/diff/1/chrome/browser/ui/cocoa/auto... chrome/browser/ui/cocoa/autofill/password_generation_popup_view_cocoa.h:37: - (BOOL)isPointInPasswordBounds:(CGPoint)point; Please add documentation for this method. https://codereview.chromium.org/487193003/diff/1/chrome/browser/ui/cocoa/auto... File chrome/browser/ui/cocoa/autofill/password_generation_popup_view_cocoa.mm (right): https://codereview.chromium.org/487193003/diff/1/chrome/browser/ui/cocoa/auto... chrome/browser/ui/cocoa/autofill/password_generation_popup_view_cocoa.mm:35: const CGFloat kPasswordSectionVerticalSeparation = 5; nit: Please document these, including units. https://codereview.chromium.org/487193003/diff/1/chrome/browser/ui/cocoa/auto... chrome/browser/ui/cocoa/autofill/password_generation_popup_view_cocoa.mm:38: return (NSMaxY(frame) + NSMinY(frame)) / 2.0; Any particular reason not to just use NSMidY? https://codereview.chromium.org/487193003/diff/1/chrome/browser/ui/cocoa/auto... chrome/browser/ui/cocoa/autofill/password_generation_popup_view_cocoa.mm:103: [[helpTextView textContainer] setLineFragmentPadding:0.0f]; Is this still needed, in addition to the code you added below? https://codereview.chromium.org/487193003/diff/1/chrome/browser/ui/cocoa/auto... chrome/browser/ui/cocoa/autofill/password_generation_popup_view_cocoa.mm:172: forField:passwordField_]; Is it not possible to update the text without needing to redefine all of these attributes? https://codereview.chromium.org/487193003/diff/1/chrome/browser/ui/cocoa/auto... chrome/browser/ui/cocoa/autofill/password_generation_popup_view_cocoa.mm:178: // The password can change while the bubble is shown: if the user has nit: "if" -> "If" https://codereview.chromium.org/487193003/diff/1/chrome/browser/ui/cocoa/auto... chrome/browser/ui/cocoa/autofill/password_generation_popup_view_cocoa.mm:182: [self updatePassword]; nit: Can this be moved into the if-stmt below? https://codereview.chromium.org/487193003/diff/1/chrome/browser/ui/cocoa/auto... chrome/browser/ui/cocoa/autofill/password_generation_popup_view_cocoa.mm:197: NSRectToCGRect([self passwordSectionBoundsForWidth:width]), point); Can you use NSPointInRect instead? (In general, NS types and functions are preferred to CG ones). https://codereview.chromium.org/487193003/diff/1/chrome/browser/ui/cocoa/auto... chrome/browser/ui/cocoa/autofill/password_generation_popup_view_cocoa.mm:283: return textFrame.size; I feel like this code must already exist somewhere. Does https://developer.apple.com/library/mac/documentation/Cocoa/Reference/Applica...: do what you need? https://codereview.chromium.org/487193003/diff/1/chrome/browser/ui/cocoa/auto... chrome/browser/ui/cocoa/autofill/password_generation_popup_view_cocoa.mm:364: ResourceBundle::SmallFont).GetPrimaryFont().GetNativeFont(); nit: Perhaps format this to match the formatting above, so that a visual diff is easier.
https://codereview.chromium.org/487193003/diff/1/chrome/browser/ui/cocoa/auto... File chrome/browser/ui/cocoa/autofill/password_generation_popup_view_bridge.mm (right): https://codereview.chromium.org/487193003/diff/1/chrome/browser/ui/cocoa/auto... chrome/browser/ui/cocoa/autofill/password_generation_popup_view_bridge.mm:14: #include "ui/base/cocoa/window_size_constants.h" Do you need window_size_constants? https://codereview.chromium.org/487193003/diff/1/chrome/browser/ui/cocoa/auto... chrome/browser/ui/cocoa/autofill/password_generation_popup_view_bridge.mm:15: #include "ui/gfx/rect.h" Do you need gfx/rect.h? https://codereview.chromium.org/487193003/diff/1/chrome/browser/ui/cocoa/auto... File chrome/browser/ui/cocoa/autofill/password_generation_popup_view_cocoa.mm (right): https://codereview.chromium.org/487193003/diff/1/chrome/browser/ui/cocoa/auto... chrome/browser/ui/cocoa/autofill/password_generation_popup_view_cocoa.mm:89: keyIcon_ = ResourceBundle::GetSharedInstance() There's nothing retaining keyIcon_ - it can (and will) be autoreleased. I suggest scoped_nsobject. https://codereview.chromium.org/487193003/diff/1/chrome/browser/ui/cocoa/auto... chrome/browser/ui/cocoa/autofill/password_generation_popup_view_cocoa.mm:105: [helpTextView setLinkTextAttributes:nil]; setLinkTextAttributes:nil does nothing for HyperlinkTextView https://codereview.chromium.org/487193003/diff/1/chrome/browser/ui/cocoa/auto... chrome/browser/ui/cocoa/autofill/password_generation_popup_view_cocoa.mm:109: range:controller_->HelpTextLinkRange().ToNSRange()]; Is this cleared with UI review? HyperlinkTextView is meant to encapsulate a Chrome-wide style for links used in UI text. https://codereview.chromium.org/487193003/diff/1/chrome/browser/ui/cocoa/auto... chrome/browser/ui/cocoa/autofill/password_generation_popup_view_cocoa.mm:111: helpTextView_ = helpTextView.get(); Why? Why not make helpTextView_ a scoped_nsobject? https://codereview.chromium.org/487193003/diff/1/chrome/browser/ui/cocoa/auto... chrome/browser/ui/cocoa/autofill/password_generation_popup_view_cocoa.mm:119: - (void)drawRect:(NSRect)dirtyRect { This is missing a call to [super drawRect:] - intentional? https://codereview.chromium.org/487193003/diff/1/chrome/browser/ui/cocoa/auto... chrome/browser/ui/cocoa/autofill/password_generation_popup_view_cocoa.mm:138: [NSBezierPath fillRect:[self helpBoundsForWidth:width]]; Why not just set a background color on the help text? https://codereview.chromium.org/487193003/diff/1/chrome/browser/ui/cocoa/auto... chrome/browser/ui/cocoa/autofill/password_generation_popup_view_cocoa.mm:141: [DividerColor() set]; Usually, you draw dividers by just having an NSBox of type NSBoxSeparator. https://codereview.chromium.org/487193003/diff/1/chrome/browser/ui/cocoa/auto... chrome/browser/ui/cocoa/autofill/password_generation_popup_view_cocoa.mm:145: [keyIcon_ drawInRect:[self iconBoundsForWidth:width] Can you use an NSImageView instead? https://codereview.chromium.org/487193003/diff/1/chrome/browser/ui/cocoa/auto... chrome/browser/ui/cocoa/autofill/password_generation_popup_view_cocoa.mm:146: fromRect:{ NSZeroPoint, [keyIcon_ size] } Passing in NSZeroRect automatically specifies the whole image - no need to build your own rect :) https://codereview.chromium.org/487193003/diff/1/chrome/browser/ui/cocoa/auto... chrome/browser/ui/cocoa/autofill/password_generation_popup_view_cocoa.mm:177: NSWidth(NSRectFromCGRect(controller_->popup_bounds().ToCGRect())); Just width = controller->popup_bounds().width() Neither ToCGRect nor NSRectFromCGRect touch width. https://codereview.chromium.org/487193003/diff/1/chrome/browser/ui/cocoa/auto... chrome/browser/ui/cocoa/autofill/password_generation_popup_view_cocoa.mm:220: forField:(NSTextField*)field { This seem overkill, but I'm not sure how many different text styles you use. Ideally, just define the styles (as dictionaries) and use as needed. (If there are many different combinations of color/font/alignment, I suggest pinging UI peeps) https://codereview.chromium.org/487193003/diff/1/chrome/browser/ui/cocoa/auto... chrome/browser/ui/cocoa/autofill/password_generation_popup_view_cocoa.mm:267: - (NSRect)passwordSectionBoundsForWidth:(CGFloat)width { Using bounds in the name is confusing. Usually, [self bounds] is the local coordinate system - if the password section were a view, this would be a rect. I.e. passwordRectForWidth: https://codereview.chromium.org/487193003/diff/1/chrome/browser/ui/cocoa/auto... chrome/browser/ui/cocoa/autofill/password_generation_popup_view_cocoa.mm:293: (kPasswordSectionVerticalSeparation / 2.0) - Be careful with rounding. Currently, kPasswordSectionVerticalSeparation is odd, so chance are y might not be an integer coordinate. If you position controls using non-integer coordinates, you get a blurred view. I'd suggest std::ceil/std::floor https://codereview.chromium.org/487193003/diff/1/chrome/browser/ui/cocoa/auto... chrome/browser/ui/cocoa/autofill/password_generation_popup_view_cocoa.mm:294: passwordSize.height; You seem to just care about the height of the text - [NSFont defaultLineHeight] might be a better choice. https://codereview.chromium.org/487193003/diff/1/chrome/browser/ui/cocoa/auto... chrome/browser/ui/cocoa/autofill/password_generation_popup_view_cocoa.mm:316: kDividerWidth); Why is the last param kDividerWidth? Shouldn't that be height? https://codereview.chromium.org/487193003/diff/1/chrome/browser/ui/cocoa/auto... chrome/browser/ui/cocoa/autofill/password_generation_popup_view_cocoa.mm:328: attributes:attributes]; Ideally, you don't want to compute field sizes at all, but just use sizeToFit and let it figure out the perfect size. Would that approach work here? https://codereview.chromium.org/487193003/diff/1/chrome/browser/ui/cocoa/auto... chrome/browser/ui/cocoa/autofill/password_generation_popup_view_cocoa.mm:355: return ResourceBundle::GetSharedInstance() Fonts grabbed with DeriveWithStyle don't always look native. Are none of the system fonts suitable?
PTAL: I have significantly changed (improved? :) the layout logic. https://codereview.chromium.org/487193003/diff/1/chrome/browser/ui/cocoa/auto... File chrome/browser/ui/cocoa/autofill/password_generation_popup_view_bridge.h (right): https://codereview.chromium.org/487193003/diff/1/chrome/browser/ui/cocoa/auto... chrome/browser/ui/cocoa/autofill/password_generation_popup_view_bridge.h:44: PasswordGenerationPopupController* controller_; // weak On 2014/08/19 22:32:17, Ilya Sherman wrote: > Where is this used? It's not. Forgot to remove. Done. https://codereview.chromium.org/487193003/diff/1/chrome/browser/ui/cocoa/auto... File chrome/browser/ui/cocoa/autofill/password_generation_popup_view_bridge.mm (right): https://codereview.chromium.org/487193003/diff/1/chrome/browser/ui/cocoa/auto... chrome/browser/ui/cocoa/autofill/password_generation_popup_view_bridge.mm:14: #include "ui/base/cocoa/window_size_constants.h" On 2014/08/19 23:32:17, groby wrote: > Do you need window_size_constants? Done. https://codereview.chromium.org/487193003/diff/1/chrome/browser/ui/cocoa/auto... chrome/browser/ui/cocoa/autofill/password_generation_popup_view_bridge.mm:15: #include "ui/gfx/rect.h" On 2014/08/19 23:32:17, groby wrote: > Do you need gfx/rect.h? Done. https://codereview.chromium.org/487193003/diff/1/chrome/browser/ui/cocoa/auto... File chrome/browser/ui/cocoa/autofill/password_generation_popup_view_cocoa.h (right): https://codereview.chromium.org/487193003/diff/1/chrome/browser/ui/cocoa/auto... chrome/browser/ui/cocoa/autofill/password_generation_popup_view_cocoa.h:37: - (BOOL)isPointInPasswordBounds:(CGPoint)point; On 2014/08/19 22:32:17, Ilya Sherman wrote: > nit: Can the |point| be passed as an NSPoint rather than as a CGPoint? Done. https://codereview.chromium.org/487193003/diff/1/chrome/browser/ui/cocoa/auto... chrome/browser/ui/cocoa/autofill/password_generation_popup_view_cocoa.h:37: - (BOOL)isPointInPasswordBounds:(CGPoint)point; On 2014/08/19 22:32:17, Ilya Sherman wrote: > Please add documentation for this method. Done. https://codereview.chromium.org/487193003/diff/1/chrome/browser/ui/cocoa/auto... File chrome/browser/ui/cocoa/autofill/password_generation_popup_view_cocoa.mm (right): https://codereview.chromium.org/487193003/diff/1/chrome/browser/ui/cocoa/auto... chrome/browser/ui/cocoa/autofill/password_generation_popup_view_cocoa.mm:35: const CGFloat kPasswordSectionVerticalSeparation = 5; On 2014/08/19 22:32:17, Ilya Sherman wrote: > nit: Please document these, including units. Done. https://codereview.chromium.org/487193003/diff/1/chrome/browser/ui/cocoa/auto... chrome/browser/ui/cocoa/autofill/password_generation_popup_view_cocoa.mm:38: return (NSMaxY(frame) + NSMinY(frame)) / 2.0; On 2014/08/19 22:32:17, Ilya Sherman wrote: > Any particular reason not to just use NSMidY? Amusingly, I googled for it and only found crap, because apparently the Foundation docs for it weren't in the top half of the results, and so I assumed that no such function existed. Done. https://codereview.chromium.org/487193003/diff/1/chrome/browser/ui/cocoa/auto... chrome/browser/ui/cocoa/autofill/password_generation_popup_view_cocoa.mm:89: keyIcon_ = ResourceBundle::GetSharedInstance() On 2014/08/19 23:32:18, groby wrote: > There's nothing retaining keyIcon_ - it can (and will) be autoreleased. I > suggest scoped_nsobject. Done. https://codereview.chromium.org/487193003/diff/1/chrome/browser/ui/cocoa/auto... chrome/browser/ui/cocoa/autofill/password_generation_popup_view_cocoa.mm:103: [[helpTextView textContainer] setLineFragmentPadding:0.0f]; On 2014/08/19 22:32:17, Ilya Sherman wrote: > Is this still needed, in addition to the code you added below? Done. https://codereview.chromium.org/487193003/diff/1/chrome/browser/ui/cocoa/auto... chrome/browser/ui/cocoa/autofill/password_generation_popup_view_cocoa.mm:105: [helpTextView setLinkTextAttributes:nil]; On 2014/08/19 23:32:18, groby wrote: > setLinkTextAttributes:nil does nothing for HyperlinkTextView Done. https://codereview.chromium.org/487193003/diff/1/chrome/browser/ui/cocoa/auto... chrome/browser/ui/cocoa/autofill/password_generation_popup_view_cocoa.mm:109: range:controller_->HelpTextLinkRange().ToNSRange()]; On 2014/08/19 23:32:18, groby wrote: > Is this cleared with UI review? HyperlinkTextView is meant to encapsulate a > Chrome-wide style for links used in UI text. This came from UI mocks: https://code.google.com/p/chromium/issues/detail?id=318977#c35 There are many examples of non-underlined links in UI on both Views and Cocoa platforms. For example, the "sign in" link in the Bookmarks bubble. Not sure why that's not the default. https://codereview.chromium.org/487193003/diff/1/chrome/browser/ui/cocoa/auto... chrome/browser/ui/cocoa/autofill/password_generation_popup_view_cocoa.mm:111: helpTextView_ = helpTextView.get(); On 2014/08/19 23:32:18, groby wrote: > Why? Why not make helpTextView_ a scoped_nsobject? Agreed. I've now converted all the __weak pointer properties in the class to scoped_nsobjects. Additionally: I don't think adding __weak does anything in our codebase. https://codereview.chromium.org/487193003/diff/1/chrome/browser/ui/cocoa/auto... chrome/browser/ui/cocoa/autofill/password_generation_popup_view_cocoa.mm:119: - (void)drawRect:(NSRect)dirtyRect { On 2014/08/19 23:32:18, groby wrote: > This is missing a call to [super drawRect:] - intentional? I don't know; this is inherited code. I can't tell that adding it breaks anything, so I've added it. https://codereview.chromium.org/487193003/diff/1/chrome/browser/ui/cocoa/auto... chrome/browser/ui/cocoa/autofill/password_generation_popup_view_cocoa.mm:138: [NSBezierPath fillRect:[self helpBoundsForWidth:width]]; On 2014/08/19 23:32:18, groby wrote: > Why not just set a background color on the help text? Done. https://codereview.chromium.org/487193003/diff/1/chrome/browser/ui/cocoa/auto... chrome/browser/ui/cocoa/autofill/password_generation_popup_view_cocoa.mm:141: [DividerColor() set]; On 2014/08/19 23:32:17, groby wrote: > Usually, you draw dividers by just having an NSBox of type NSBoxSeparator. You can't change its color that way. I've replaced this with an NSBox modeled after the BookmarkSyncPromoController's separator. https://codereview.chromium.org/487193003/diff/1/chrome/browser/ui/cocoa/auto... chrome/browser/ui/cocoa/autofill/password_generation_popup_view_cocoa.mm:145: [keyIcon_ drawInRect:[self iconBoundsForWidth:width] On 2014/08/19 23:32:18, groby wrote: > Can you use an NSImageView instead? Done. https://codereview.chromium.org/487193003/diff/1/chrome/browser/ui/cocoa/auto... chrome/browser/ui/cocoa/autofill/password_generation_popup_view_cocoa.mm:146: fromRect:{ NSZeroPoint, [keyIcon_ size] } On 2014/08/19 23:32:18, groby wrote: > Passing in NSZeroRect automatically specifies the whole image - no need to build > your own rect :) Done. https://codereview.chromium.org/487193003/diff/1/chrome/browser/ui/cocoa/auto... chrome/browser/ui/cocoa/autofill/password_generation_popup_view_cocoa.mm:172: forField:passwordField_]; On 2014/08/19 22:32:17, Ilya Sherman wrote: > Is it not possible to update the text without needing to redefine all of these > attributes? I thought not, but I found a way. Done. https://codereview.chromium.org/487193003/diff/1/chrome/browser/ui/cocoa/auto... chrome/browser/ui/cocoa/autofill/password_generation_popup_view_cocoa.mm:177: NSWidth(NSRectFromCGRect(controller_->popup_bounds().ToCGRect())); On 2014/08/19 23:32:18, groby wrote: > Just width = controller->popup_bounds().width() > > Neither ToCGRect nor NSRectFromCGRect touch width. Done. https://codereview.chromium.org/487193003/diff/1/chrome/browser/ui/cocoa/auto... chrome/browser/ui/cocoa/autofill/password_generation_popup_view_cocoa.mm:178: // The password can change while the bubble is shown: if the user has On 2014/08/19 22:32:17, Ilya Sherman wrote: > nit: "if" -> "If" TIL. Done. https://codereview.chromium.org/487193003/diff/1/chrome/browser/ui/cocoa/auto... chrome/browser/ui/cocoa/autofill/password_generation_popup_view_cocoa.mm:182: [self updatePassword]; On 2014/08/19 22:32:17, Ilya Sherman wrote: > nit: Can this be moved into the if-stmt below? Done. https://codereview.chromium.org/487193003/diff/1/chrome/browser/ui/cocoa/auto... chrome/browser/ui/cocoa/autofill/password_generation_popup_view_cocoa.mm:197: NSRectToCGRect([self passwordSectionBoundsForWidth:width]), point); On 2014/08/19 22:32:17, Ilya Sherman wrote: > Can you use NSPointInRect instead? (In general, NS types and functions are > preferred to CG ones). Done. https://codereview.chromium.org/487193003/diff/1/chrome/browser/ui/cocoa/auto... chrome/browser/ui/cocoa/autofill/password_generation_popup_view_cocoa.mm:220: forField:(NSTextField*)field { On 2014/08/19 23:32:17, groby wrote: > This seem overkill, but I'm not sure how many different text styles you use. > Ideally, just define the styles (as dictionaries) and use as needed. > > (If there are many different combinations of color/font/alignment, I suggest > pinging UI peeps) Done. https://codereview.chromium.org/487193003/diff/1/chrome/browser/ui/cocoa/auto... chrome/browser/ui/cocoa/autofill/password_generation_popup_view_cocoa.mm:267: - (NSRect)passwordSectionBoundsForWidth:(CGFloat)width { On 2014/08/19 23:32:18, groby wrote: > Using bounds in the name is confusing. Usually, [self bounds] is the local > coordinate system - if the password section were a view, this would be a rect. > I.e. passwordRectForWidth: I think all this is confusing, so I've removed these methods and replaced both preferredSize and updateBoundsAndRedrawPopup with what I think are more straightforward implementations. https://codereview.chromium.org/487193003/diff/1/chrome/browser/ui/cocoa/auto... chrome/browser/ui/cocoa/autofill/password_generation_popup_view_cocoa.mm:283: return textFrame.size; On 2014/08/19 22:32:17, Ilya Sherman wrote: > I feel like this code must already exist somewhere. Does > https://developer.apple.com/library/mac/documentation/Cocoa/Reference/Applica...: > do what you need? Done. https://codereview.chromium.org/487193003/diff/1/chrome/browser/ui/cocoa/auto... chrome/browser/ui/cocoa/autofill/password_generation_popup_view_cocoa.mm:293: (kPasswordSectionVerticalSeparation / 2.0) - On 2014/08/19 23:32:18, groby wrote: > Be careful with rounding. Currently, kPasswordSectionVerticalSeparation is odd, > so chance are y might not be an integer coordinate. If you position controls > using non-integer coordinates, you get a blurred view. I'd suggest > std::ceil/std::floor Done. https://codereview.chromium.org/487193003/diff/1/chrome/browser/ui/cocoa/auto... chrome/browser/ui/cocoa/autofill/password_generation_popup_view_cocoa.mm:294: passwordSize.height; On 2014/08/19 23:32:18, groby wrote: > You seem to just care about the height of the text - [NSFont defaultLineHeight] > might be a better choice. It's deprecated. I think this should be fine, as the field fits the text. https://codereview.chromium.org/487193003/diff/1/chrome/browser/ui/cocoa/auto... chrome/browser/ui/cocoa/autofill/password_generation_popup_view_cocoa.mm:316: kDividerWidth); On 2014/08/19 23:32:18, groby wrote: > Why is the last param kDividerWidth? Shouldn't that be height? Poor naming. I was thinking of the line as having length and width, not width and height. Fixed. https://codereview.chromium.org/487193003/diff/1/chrome/browser/ui/cocoa/auto... chrome/browser/ui/cocoa/autofill/password_generation_popup_view_cocoa.mm:328: attributes:attributes]; On 2014/08/19 23:32:18, groby wrote: > Ideally, you don't want to compute field sizes at all, but just use sizeToFit > and let it figure out the perfect size. Would that approach work here? Done. https://codereview.chromium.org/487193003/diff/1/chrome/browser/ui/cocoa/auto... chrome/browser/ui/cocoa/autofill/password_generation_popup_view_cocoa.mm:355: return ResourceBundle::GetSharedInstance() On 2014/08/19 23:32:18, groby wrote: > Fonts grabbed with DeriveWithStyle don't always look native. Are none of the > system fonts suitable? Done. https://codereview.chromium.org/487193003/diff/1/chrome/browser/ui/cocoa/auto... chrome/browser/ui/cocoa/autofill/password_generation_popup_view_cocoa.mm:364: ResourceBundle::SmallFont).GetPrimaryFont().GetNativeFont(); On 2014/08/19 22:32:17, Ilya Sherman wrote: > nit: Perhaps format this to match the formatting above, so that a visual diff is > easier. Done.
In meeting mania, will get to this tonight :( Please ping me if this is blocking you.
Sorry for being so nitpicky :( https://codereview.chromium.org/487193003/diff/20001/chrome/browser/ui/cocoa/... File chrome/browser/ui/cocoa/autofill/password_generation_popup_view_bridge.mm (right): https://codereview.chromium.org/487193003/diff/20001/chrome/browser/ui/cocoa/... chrome/browser/ui/cocoa/autofill/password_generation_popup_view_bridge.mm:12: #include "chrome/browser/ui/autofill/popup_constants.h" I believe both of these are unnecessary? https://codereview.chromium.org/487193003/diff/20001/chrome/browser/ui/cocoa/... File chrome/browser/ui/cocoa/autofill/password_generation_popup_view_cocoa.h (right): https://codereview.chromium.org/487193003/diff/20001/chrome/browser/ui/cocoa/... chrome/browser/ui/cocoa/autofill/password_generation_popup_view_cocoa.h:41: // Determines whether |point| falls inside the password section of the popup. Do you need this function? You're not consuming it anywhere. If you do, please annotate that |point| needs to be in the popup's coordinate system. https://codereview.chromium.org/487193003/diff/20001/chrome/browser/ui/cocoa/... File chrome/browser/ui/cocoa/autofill/password_generation_popup_view_cocoa.mm (right): https://codereview.chromium.org/487193003/diff/20001/chrome/browser/ui/cocoa/... chrome/browser/ui/cocoa/autofill/password_generation_popup_view_cocoa.mm:5: #include <cmath> After password_generation_popup_view_cocoa.h, please. (The file's header should _always_ be first) https://codereview.chromium.org/487193003/diff/20001/chrome/browser/ui/cocoa/... chrome/browser/ui/cocoa/autofill/password_generation_popup_view_cocoa.mm:105: helpSection_.reset([[NSView alloc] init]); You can actually skip |helpSection_|, since |helpTextView_| is its only subview, and the section only provides padding and background AFAICT You can do those directly on |helpTextView_|: * Padding : -setTextContainerInset: * Background: -setDrawsBackground and backgroundColor https://codereview.chromium.org/487193003/diff/20001/chrome/browser/ui/cocoa/... chrome/browser/ui/cocoa/autofill/password_generation_popup_view_cocoa.mm:121: [helpTextView_ setVerticallyResizable:YES]; Are you sure you need this? This is only required if the text view is supposed to resize itself according to text changes? https://codereview.chromium.org/487193003/diff/20001/chrome/browser/ui/cocoa/... chrome/browser/ui/cocoa/autofill/password_generation_popup_view_cocoa.mm:125: value:[NSNumber numberWithInt:NSUnderlineStyleNone] Instead of numberWithInt, just use value:@(NSUnderlineStyleNone) https://codereview.chromium.org/487193003/diff/20001/chrome/browser/ui/cocoa/... chrome/browser/ui/cocoa/autofill/password_generation_popup_view_cocoa.mm:148: [NSBezierPath fillRect:highlightBounds]; Curious: Why NSBezierPath instead of NSRectFill?. (Not that it matters, I'm just wondering if I miss something:) https://codereview.chromium.org/487193003/diff/20001/chrome/browser/ui/cocoa/... chrome/browser/ui/cocoa/autofill/password_generation_popup_view_cocoa.mm:185: - (void)updatePassword { Since it's public API, should it be in the header? Or is this private and it should be in a different section? https://codereview.chromium.org/487193003/diff/20001/chrome/browser/ui/cocoa/... chrome/browser/ui/cocoa/autofill/password_generation_popup_view_cocoa.mm:186: NSAttributedString* currentPassword = [passwordField_ attributedStringValue]; Why not just create a new NSAttributedString and set that, instead of copying the old one, replacing the contents, and then setting that copy? https://codereview.chromium.org/487193003/diff/20001/chrome/browser/ui/cocoa/... chrome/browser/ui/cocoa/autofill/password_generation_popup_view_cocoa.mm:197: - (void)updateBoundsAndRedrawPopup { I'm wondering - it seems the width of all UI elements is determined by |popupWidth| Could you simplify the layout further by using autoresizingMask on the main view, and have it resize its children? (I'm not sure if that works - it might well be the layout is too complex for this - so don't feel compelled unless it immediately makes sense) https://codereview.chromium.org/487193003/diff/20001/chrome/browser/ui/cocoa/... chrome/browser/ui/cocoa/autofill/password_generation_popup_view_cocoa.mm:205: // The password can change while the bubble is shown: If the user has -updateBoundsAndRedrawPopup is only called for the initial -show, though - is that comment relevant? https://codereview.chromium.org/487193003/diff/20001/chrome/browser/ui/cocoa/... chrome/browser/ui/cocoa/autofill/password_generation_popup_view_cocoa.mm:223: std::ceil(controller_->kPopupPasswordSectionHeight / 2.0) - One std::ceil for the total term is probably enough :) https://codereview.chromium.org/487193003/diff/20001/chrome/browser/ui/cocoa/... chrome/browser/ui/cocoa/autofill/password_generation_popup_view_cocoa.mm:225: [keyIcon_ setFrame:{ NSMakePoint(keyX, keyY), [[keyIcon_ image] size] }]; -setFrameOrigin https://codereview.chromium.org/487193003/diff/20001/chrome/browser/ui/cocoa/... chrome/browser/ui/cocoa/autofill/password_generation_popup_view_cocoa.mm:237: std::ceil(controller_->kPopupPasswordSectionHeight / 2.0) - One ceil, as above
https://codereview.chromium.org/487193003/diff/20001/chrome/browser/ui/cocoa/... File chrome/browser/ui/cocoa/autofill/password_generation_popup_view_bridge.mm (right): https://codereview.chromium.org/487193003/diff/20001/chrome/browser/ui/cocoa/... chrome/browser/ui/cocoa/autofill/password_generation_popup_view_bridge.mm:12: #include "chrome/browser/ui/autofill/popup_constants.h" On 2014/08/21 04:40:05, groby wrote: > I believe both of these are unnecessary? Done. https://codereview.chromium.org/487193003/diff/20001/chrome/browser/ui/cocoa/... File chrome/browser/ui/cocoa/autofill/password_generation_popup_view_cocoa.h (right): https://codereview.chromium.org/487193003/diff/20001/chrome/browser/ui/cocoa/... chrome/browser/ui/cocoa/autofill/password_generation_popup_view_cocoa.h:41: // Determines whether |point| falls inside the password section of the popup. On 2014/08/21 04:40:06, groby wrote: > Do you need this function? You're not consuming it anywhere. If you do, please > annotate that |point| needs to be in the popup's coordinate system. It's being consumed by PasswordGenerationPopupViewBridge::IsPointInPasswordBounds. Annotation done. https://codereview.chromium.org/487193003/diff/20001/chrome/browser/ui/cocoa/... File chrome/browser/ui/cocoa/autofill/password_generation_popup_view_cocoa.mm (right): https://codereview.chromium.org/487193003/diff/20001/chrome/browser/ui/cocoa/... chrome/browser/ui/cocoa/autofill/password_generation_popup_view_cocoa.mm:5: #include <cmath> On 2014/08/21 04:40:06, groby wrote: > After password_generation_popup_view_cocoa.h, please. (The file's header should > _always_ be first) Done. https://codereview.chromium.org/487193003/diff/20001/chrome/browser/ui/cocoa/... chrome/browser/ui/cocoa/autofill/password_generation_popup_view_cocoa.mm:105: helpSection_.reset([[NSView alloc] init]); On 2014/08/21 04:40:06, groby wrote: > You can actually skip |helpSection_|, since |helpTextView_| is its only subview, > and the section only provides padding and background AFAICT > > You can do those directly on |helpTextView_|: > * Padding : -setTextContainerInset: > * Background: -setDrawsBackground and backgroundColor Done. https://codereview.chromium.org/487193003/diff/20001/chrome/browser/ui/cocoa/... chrome/browser/ui/cocoa/autofill/password_generation_popup_view_cocoa.mm:121: [helpTextView_ setVerticallyResizable:YES]; On 2014/08/21 04:40:06, groby wrote: > Are you sure you need this? This is only required if the text view is supposed > to resize itself according to text changes? Done. https://codereview.chromium.org/487193003/diff/20001/chrome/browser/ui/cocoa/... chrome/browser/ui/cocoa/autofill/password_generation_popup_view_cocoa.mm:125: value:[NSNumber numberWithInt:NSUnderlineStyleNone] On 2014/08/21 04:40:06, groby wrote: > Instead of numberWithInt, just use value:@(NSUnderlineStyleNone) Done. https://codereview.chromium.org/487193003/diff/20001/chrome/browser/ui/cocoa/... chrome/browser/ui/cocoa/autofill/password_generation_popup_view_cocoa.mm:148: [NSBezierPath fillRect:highlightBounds]; On 2014/08/21 04:40:06, groby wrote: > Curious: Why NSBezierPath instead of NSRectFill?. (Not that it matters, I'm just > wondering if I miss something:) No idea. This was inherited. https://codereview.chromium.org/487193003/diff/20001/chrome/browser/ui/cocoa/... chrome/browser/ui/cocoa/autofill/password_generation_popup_view_cocoa.mm:185: - (void)updatePassword { On 2014/08/21 04:40:06, groby wrote: > Since it's public API, should it be in the header? Or is this private and it > should be in a different section? It's private. Thanks, moved. https://codereview.chromium.org/487193003/diff/20001/chrome/browser/ui/cocoa/... chrome/browser/ui/cocoa/autofill/password_generation_popup_view_cocoa.mm:186: NSAttributedString* currentPassword = [passwordField_ attributedStringValue]; On 2014/08/21 04:40:06, groby wrote: > Why not just create a new NSAttributedString and set that, instead of copying > the old one, replacing the contents, and then setting that copy? Done. https://codereview.chromium.org/487193003/diff/20001/chrome/browser/ui/cocoa/... chrome/browser/ui/cocoa/autofill/password_generation_popup_view_cocoa.mm:197: - (void)updateBoundsAndRedrawPopup { On 2014/08/21 04:40:06, groby wrote: > I'm wondering - it seems the width of all UI elements is determined by > |popupWidth| Could you simplify the layout further by using autoresizingMask on > the main view, and have it resize its children? (I'm not sure if that works - it > might well be the layout is too complex for this - so don't feel compelled > unless it immediately makes sense) After thinking through this, it seems like that would involve one of the following two refactorings: 1. Move all of this logic into resizeSubviewsWithOldSize: 2. Create NSView subclasses for each of the subviews that will update their sizes properly, and only keep the origin calculations in resizeSubviewsWithOldSize: I'm not convinced that's any clearer than this is. Have I missed something? I suppose I could also use autolayout, but this is clearer to me and works already. https://codereview.chromium.org/487193003/diff/20001/chrome/browser/ui/cocoa/... chrome/browser/ui/cocoa/autofill/password_generation_popup_view_cocoa.mm:205: // The password can change while the bubble is shown: If the user has On 2014/08/21 04:40:06, groby wrote: > -updateBoundsAndRedrawPopup is only called for the initial -show, though - is > that comment relevant? It's actually called as the mouse moves over the bubble, via the C++ bridge class. That appears to be part of the superclass implementation. https://codereview.chromium.org/487193003/diff/20001/chrome/browser/ui/cocoa/... chrome/browser/ui/cocoa/autofill/password_generation_popup_view_cocoa.mm:223: std::ceil(controller_->kPopupPasswordSectionHeight / 2.0) - On 2014/08/21 04:40:06, groby wrote: > One std::ceil for the total term is probably enough :) It looked nicer this way :( Done. https://codereview.chromium.org/487193003/diff/20001/chrome/browser/ui/cocoa/... chrome/browser/ui/cocoa/autofill/password_generation_popup_view_cocoa.mm:225: [keyIcon_ setFrame:{ NSMakePoint(keyX, keyY), [[keyIcon_ image] size] }]; On 2014/08/21 04:40:06, groby wrote: > -setFrameOrigin It doesn't work, actually -- this size stays 0,0. And the other examples I've found in chrome/browser/ui/cocoa seem to manually set the view size to the image size as well. https://codereview.chromium.org/487193003/diff/20001/chrome/browser/ui/cocoa/... chrome/browser/ui/cocoa/autofill/password_generation_popup_view_cocoa.mm:237: std::ceil(controller_->kPopupPasswordSectionHeight / 2.0) - On 2014/08/21 04:40:06, groby wrote: > One ceil, as above Done.
c/b/ui/cocoa LGTM, thank you! https://codereview.chromium.org/487193003/diff/20001/chrome/browser/ui/cocoa/... File chrome/browser/ui/cocoa/autofill/password_generation_popup_view_cocoa.mm (right): https://codereview.chromium.org/487193003/diff/20001/chrome/browser/ui/cocoa/... chrome/browser/ui/cocoa/autofill/password_generation_popup_view_cocoa.mm:197: - (void)updateBoundsAndRedrawPopup { On 2014/08/21 10:44:26, dconnelly wrote: > On 2014/08/21 04:40:06, groby wrote: > > I'm wondering - it seems the width of all UI elements is determined by > > |popupWidth| Could you simplify the layout further by using autoresizingMask > on > > the main view, and have it resize its children? (I'm not sure if that works - > it > > might well be the layout is too complex for this - so don't feel compelled > > unless it immediately makes sense) > > After thinking through this, it seems like that would involve one of the > following two refactorings: > > 1. Move all of this logic into resizeSubviewsWithOldSize: > 2. Create NSView subclasses for each of the subviews that will update their > sizes properly, and only keep the origin calculations in > resizeSubviewsWithOldSize: > > I'm not convinced that's any clearer than this is. Have I missed something? I > suppose I could also use autolayout, but this is clearer to me and works > already. Both approaches sound like overkill, and we can't have constraint layout (10.7+ only), so ignore me :) https://codereview.chromium.org/487193003/diff/20001/chrome/browser/ui/cocoa/... chrome/browser/ui/cocoa/autofill/password_generation_popup_view_cocoa.mm:205: // The password can change while the bubble is shown: If the user has On 2014/08/21 10:44:26, dconnelly wrote: > On 2014/08/21 04:40:06, groby wrote: > > -updateBoundsAndRedrawPopup is only called for the initial -show, though - is > > that comment relevant? > > It's actually called as the mouse moves over the bubble, via the C++ bridge > class. That appears to be part of the superclass implementation. Ah. Foiled by cs.chromium.org having an incomplete corpus. https://codereview.chromium.org/487193003/diff/20001/chrome/browser/ui/cocoa/... chrome/browser/ui/cocoa/autofill/password_generation_popup_view_cocoa.mm:225: [keyIcon_ setFrame:{ NSMakePoint(keyX, keyY), [[keyIcon_ image] size] }]; On 2014/08/21 10:44:26, dconnelly wrote: > On 2014/08/21 04:40:06, groby wrote: > > -setFrameOrigin > > It doesn't work, actually -- this size stays 0,0. And the other examples I've > found in chrome/browser/ui/cocoa seem to manually set the view size to the image > size as well. a [keyIcon_ sizeToFit] at creation time would address that, IIRC.
LGTM2 % nits. Thanks for sharing your Cocoa expertise, Rachel! :) https://codereview.chromium.org/487193003/diff/40001/chrome/browser/ui/cocoa/... File chrome/browser/ui/cocoa/autofill/password_generation_popup_view_cocoa.mm (right): https://codereview.chromium.org/487193003/diff/40001/chrome/browser/ui/cocoa/... chrome/browser/ui/cocoa/autofill/password_generation_popup_view_cocoa.mm:36: // The width of the divider between the password and help sections, in pixels. nit: "width" -> "height" https://codereview.chromium.org/487193003/diff/40001/chrome/browser/ui/cocoa/... chrome/browser/ui/cocoa/autofill/password_generation_popup_view_cocoa.mm:195: // Layout the password section, which includes the key icon, the title, and nit: "Layout" -> "Lay out" (noun vs. verb) https://codereview.chromium.org/487193003/diff/40001/chrome/browser/ui/cocoa/... chrome/browser/ui/cocoa/autofill/password_generation_popup_view_cocoa.mm:239: // beneach the border). nit: "beneach" -> "beneath"
https://codereview.chromium.org/487193003/diff/40001/chrome/browser/ui/cocoa/... File chrome/browser/ui/cocoa/autofill/password_generation_popup_view_cocoa.mm (right): https://codereview.chromium.org/487193003/diff/40001/chrome/browser/ui/cocoa/... chrome/browser/ui/cocoa/autofill/password_generation_popup_view_cocoa.mm:36: // The width of the divider between the password and help sections, in pixels. On 2014/08/22 06:20:11, Ilya Sherman wrote: > nit: "width" -> "height" Done. https://codereview.chromium.org/487193003/diff/40001/chrome/browser/ui/cocoa/... chrome/browser/ui/cocoa/autofill/password_generation_popup_view_cocoa.mm:195: // Layout the password section, which includes the key icon, the title, and On 2014/08/22 06:20:11, Ilya Sherman wrote: > nit: "Layout" -> "Lay out" (noun vs. verb) Done. https://codereview.chromium.org/487193003/diff/40001/chrome/browser/ui/cocoa/... chrome/browser/ui/cocoa/autofill/password_generation_popup_view_cocoa.mm:239: // beneach the border). On 2014/08/22 06:20:11, Ilya Sherman wrote: > nit: "beneach" -> "beneath" Done.
The CQ bit was checked by dconnelly@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/dconnelly@chromium.org/487193003/60001
The CQ bit was unchecked by dconnelly@chromium.org
The CQ bit was checked by dconnelly@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/dconnelly@chromium.org/487193003/100001
FYI, CQ is re-trying this CL (attempt #1). The failing builders are: android_chromium_gn_compile_rel on tryserver.chromium.linux (http://build.chromium.org/p/tryserver.chromium.linux/builders/android_chromiu...)
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: android_chromium_gn_compile_rel on tryserver.chromium.linux (http://build.chromium.org/p/tryserver.chromium.linux/builders/android_chromiu...)
The CQ bit was checked by dconnelly@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/dconnelly@chromium.org/487193003/100001
FYI, CQ is re-trying this CL (attempt #1). The failing builders are: android_chromium_gn_compile_rel on tryserver.chromium.linux (http://build.chromium.org/p/tryserver.chromium.linux/builders/android_chromiu...)
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: android_chromium_gn_compile_rel on tryserver.chromium.linux (http://build.chromium.org/p/tryserver.chromium.linux/builders/android_chromiu...)
The CQ bit was checked by dconnelly@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/dconnelly@chromium.org/487193003/100001
FYI, CQ is re-trying this CL (attempt #1). The failing builders are: android_chromium_gn_compile_rel on tryserver.chromium.linux (http://build.chromium.org/p/tryserver.chromium.linux/builders/android_chromiu...)
Message was sent while issue was closed.
Committed patchset #5 (100001) as 291389 |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
