Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
11740033:
[Autofill] Add Mac implementation for the in-browser process popup view. (Closed)
|
Created:
7 years, 11 months ago by Ilya Sherman Modified:
7 years, 11 months ago CC:
chromium-reviews, Raman Kakilate, benquan, dhollowa+watch_chromium.org, ahutter, dbeam+watch-autofill_chromium.org, sail+watch_chromium.org, Dane Wallinga, dyu1, Albert Bodenhamer, estade+watch_chromium.org, Ilya Sherman Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
Description[Autofill] Add Mac implementation for the in-browser process popup view.
There are still some TODOs, but the basic mouse-based functionality is implemented.
As additional context: The stable version of the Autofill popup UI is currently implemented in the renderer process -- actually, as part of WebKit. It's implemented (cross-platform) as a heavily styled version of the <select> popup that we use on non-Mac platforms. This CL implements this same popup as a native UI, to reach parity with GTK and Views.
BUG=51644
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=176558
Patch Set 1 #
Total comments: 40
Patch Set 2 : Calibrated colors, fewer defaults #
Total comments: 19
Patch Set 3 : Flipped view, broken out mouse events, style fixes. #
Total comments: 3
Patch Set 4 : Screen handling #
Total comments: 21
Patch Set 5 : Rename _mac to _bridge, added TODOs for cleanup per Nico's comments. #
Messages
Total messages: 22 (0 generated)
Scott, would you mind reviewing (the Mac parts of) this CL? It's been an embarrassingly long time since I've written Cocoa code, so I'd really appreciate a thorough review from someone who knows Cocoa inside and out :)
https://chromiumcodereview.appspot.com/11740033/diff/1/chrome/browser/ui/coco... File chrome/browser/ui/cocoa/autofill/autofill_popup_view_cocoa.mm (right): https://chromiumcodereview.appspot.com/11740033/diff/1/chrome/browser/ui/coco... chrome/browser/ui/cocoa/autofill/autofill_popup_view_cocoa.mm:198: // TODO(isherman): Set font, colors, and any other appropriate attributes. This is the only TODO(isherman) line in this file that I'm intending to check in as part of this CL. The rest are essentially questions for friendly Cocoa experts that I'd like to resolve during the code review :) https://chromiumcodereview.appspot.com/11740033/diff/1/chrome/browser/ui/coco... File chrome/browser/ui/cocoa/autofill/autofill_popup_view_mac.mm (right): https://chromiumcodereview.appspot.com/11740033/diff/1/chrome/browser/ui/coco... chrome/browser/ui/cocoa/autofill/autofill_popup_view_mac.mm:24: // NSFloatingWindowLevel or NSModalPanelWindowLevel instead? Friendly Cocoa reviewers: Thoughts? https://chromiumcodereview.appspot.com/11740033/diff/1/chrome/browser/ui/coco... chrome/browser/ui/cocoa/autofill/autofill_popup_view_mac.mm:26: // TODO(isherman): Are these explicit calls needed / recommended? Friendly Cocoa reviewers: Thoughts? https://chromiumcodereview.appspot.com/11740033/diff/1/chrome/browser/ui/coco... chrome/browser/ui/cocoa/autofill/autofill_popup_view_mac.mm:73: // be in view coordinates. Friendly Cocoa reviewers: Thoughts?
Also, I am somewhat inclined to put the NSView implementation right in the same file as the C++ subclass. Having a _cocoa and a _mac version of the file seems odd, and I can't really describe how one might know which thing to look at for which piece of whatever. In the end, it's just helper code anyhow, not something that stands on its own. [I'm not suggesting do this right now, we can get further into the review, first.] I might have a suggestion along the lines of "how to break down the window/containerview/view" type of thing once I patch it in somewhere. I might not have any suggestion. Time will tell. https://chromiumcodereview.appspot.com/11740033/diff/1/chrome/browser/about_f... File chrome/browser/about_flags.cc (right): https://chromiumcodereview.appspot.com/11740033/diff/1/chrome/browser/about_f... chrome/browser/about_flags.cc:1130: kOsMac | kOsWin | kOsLinux | kOsCrOS, I see that we don't have consistent sorting standards in this file? https://chromiumcodereview.appspot.com/11740033/diff/1/chrome/browser/ui/coco... File chrome/browser/ui/cocoa/autofill/autofill_popup_view_cocoa.mm (right): https://chromiumcodereview.appspot.com/11740033/diff/1/chrome/browser/ui/coco... chrome/browser/ui/cocoa/autofill/autofill_popup_view_cocoa.mm:41: - (NSImage*)iconAtIndex:(int)index; size_t to match the caller. Or NSUInteger, but I think size_t makes more sense for Chromium. https://chromiumcodereview.appspot.com/11740033/diff/1/chrome/browser/ui/coco... chrome/browser/ui/cocoa/autofill/autofill_popup_view_cocoa.mm:45: - (gfx::Point)flipNSPointToPoint:(NSPoint)point; Rather than making explicit conversions, it might make sense to override -isFlipped to return YES. All your math is going to change, of course. https://chromiumcodereview.appspot.com/11740033/diff/1/chrome/browser/ui/coco... chrome/browser/ui/cocoa/autofill/autofill_popup_view_cocoa.mm:94: // sets clearColor for the window's background color... -clearColor will be transparent, I think. https://chromiumcodereview.appspot.com/11740033/diff/1/chrome/browser/ui/coco... chrome/browser/ui/cocoa/autofill/autofill_popup_view_cocoa.mm:96: // colorWithCalibratedWhite:alpha:] instead? I would prefer -whiteColor or comparable if it works. https://chromiumcodereview.appspot.com/11740033/diff/1/chrome/browser/ui/coco... chrome/browser/ui/cocoa/autofill/autofill_popup_view_cocoa.mm:106: // to each other. What existing UI do you mean in this? Can you steal the color from there? Since this is local UI which won't be transferred across devices, I'd just use whichever calibrated or device variant other people use. https://chromiumcodereview.appspot.com/11740033/diff/1/chrome/browser/ui/coco... chrome/browser/ui/cocoa/autofill/autofill_popup_view_cocoa.mm:174: blue:220/255.0 If they're all the same, -colorWithDeviceWhite: should work. https://chromiumcodereview.appspot.com/11740033/diff/1/chrome/browser/ui/coco... chrome/browser/ui/cocoa/autofill/autofill_popup_view_cocoa.mm:188: // color. What about one of +controlHighlighColor or +controlLightHighlightColor or +highlightColor? https://chromiumcodereview.appspot.com/11740033/diff/1/chrome/browser/ui/coco... chrome/browser/ui/cocoa/autofill/autofill_popup_view_cocoa.mm:243: y = bounds.origin.y + (bounds.size.height - iconSize.height) / 2; Depending on what the icon is, I'm not sure if drawing on a half boundary is what you want. https://chromiumcodereview.appspot.com/11740033/diff/1/chrome/browser/ui/coco... File chrome/browser/ui/cocoa/autofill/autofill_popup_view_mac.mm (right): https://chromiumcodereview.appspot.com/11740033/diff/1/chrome/browser/ui/coco... chrome/browser/ui/cocoa/autofill/autofill_popup_view_mac.mm:24: // NSFloatingWindowLevel or NSModalPanelWindowLevel instead? On 2013/01/04 01:24:29, Ilya Sherman wrote: > Friendly Cocoa reviewers: Thoughts? Omnibox is at NSNormalWindowLevel. Levels are a bit weird - so long as you order above your window, and are operating WRT that window (not as something independent), you maybe don't need to worry about this at all. Actually, I see that below you're making it a child window - in that case I don't think you need to worry about levels at all. https://chromiumcodereview.appspot.com/11740033/diff/1/chrome/browser/ui/coco... chrome/browser/ui/cocoa/autofill/autofill_popup_view_mac.mm:26: // TODO(isherman): Are these explicit calls needed / recommended? On 2013/01/04 01:24:29, Ilya Sherman wrote: > Friendly Cocoa reviewers: Thoughts? For borderless, you might need to disable moving by background. I'm not sure how the background color and alpha interact. The opaque setting is a signal others use to optimize things (setting to YES signals that you're arranging it to be opaque). https://chromiumcodereview.appspot.com/11740033/diff/1/chrome/browser/ui/coco... chrome/browser/ui/cocoa/autofill/autofill_popup_view_mac.mm:42: [view_ controllerDestroyed]; Since this is called from the destructor, you might want to document on the view side that the controller_ should not be called at this point. https://chromiumcodereview.appspot.com/11740033/diff/1/chrome/browser/ui/coco... chrome/browser/ui/cocoa/autofill/autofill_popup_view_mac.mm:73: // be in view coordinates. On 2013/01/04 01:24:29, Ilya Sherman wrote: > Friendly Cocoa reviewers: Thoughts? I think I'm going to need to patch this in and look at it before I can speculate. What kind of thing do I need to be doing in a webpage to see this at work?
On 2013/01/05 01:06:49, shess wrote: > Also, I am somewhat inclined to put the NSView implementation right in the same > file as the C++ subclass. Having a _cocoa and a _mac version of the file seems > odd, and I can't really describe how one might know which thing to look at for > which piece of whatever. In the end, it's just helper code anyhow, not > something that stands on its own. > > [I'm not suggesting do this right now, we can get further into the review, > first.] Fair enough. I'll hold off on moving it for now, so that diffs in the review continue to make sense. FWIW, I initially had it in the same file, and decided to move it out so that the file would be smaller... but I wasn't sure which spot for it was better. https://chromiumcodereview.appspot.com/11740033/diff/1/chrome/browser/about_f... File chrome/browser/about_flags.cc (right): https://chromiumcodereview.appspot.com/11740033/diff/1/chrome/browser/about_f... chrome/browser/about_flags.cc:1130: kOsMac | kOsWin | kOsLinux | kOsCrOS, On 2013/01/05 01:06:49, shess wrote: > I see that we don't have consistent sorting standards in this file? Yeah, I copied the order from kOsAll at the top of the file. https://chromiumcodereview.appspot.com/11740033/diff/1/chrome/browser/ui/coco... File chrome/browser/ui/cocoa/autofill/autofill_popup_view_cocoa.mm (right): https://chromiumcodereview.appspot.com/11740033/diff/1/chrome/browser/ui/coco... chrome/browser/ui/cocoa/autofill/autofill_popup_view_cocoa.mm:41: - (NSImage*)iconAtIndex:(int)index; On 2013/01/05 01:06:49, shess wrote: > size_t to match the caller. Or NSUInteger, but I think size_t makes more sense > for Chromium. Done. https://chromiumcodereview.appspot.com/11740033/diff/1/chrome/browser/ui/coco... chrome/browser/ui/cocoa/autofill/autofill_popup_view_cocoa.mm:45: - (gfx::Point)flipNSPointToPoint:(NSPoint)point; On 2013/01/05 01:06:49, shess wrote: > Rather than making explicit conversions, it might make sense to override > -isFlipped to return YES. All your math is going to change, of course. I thought about that, but I think isFlipped just flips the coordinate within the view's coordinate system, whereas we want to flip the coordinate and then translate it so that it's in screen coordinates. https://chromiumcodereview.appspot.com/11740033/diff/1/chrome/browser/ui/coco... chrome/browser/ui/cocoa/autofill/autofill_popup_view_cocoa.mm:94: // sets clearColor for the window's background color... On 2013/01/05 01:06:49, shess wrote: > -clearColor will be transparent, I think. I figured out why the view needs to set a background color: otherwise it needs to not be opaque. https://chromiumcodereview.appspot.com/11740033/diff/1/chrome/browser/ui/coco... chrome/browser/ui/cocoa/autofill/autofill_popup_view_cocoa.mm:96: // colorWithCalibratedWhite:alpha:] instead? On 2013/01/05 01:06:49, shess wrote: > I would prefer -whiteColor or comparable if it works. Done. https://chromiumcodereview.appspot.com/11740033/diff/1/chrome/browser/ui/coco... chrome/browser/ui/cocoa/autofill/autofill_popup_view_cocoa.mm:106: // to each other. On 2013/01/05 01:06:49, shess wrote: > What existing UI do you mean in this? Can you steal the color from there? Sorry, I should have provided more context for this CL: The stable version of the Autofill popup UI is currently implemented in the renderer process -- actually, as part of WebKit. It's implemented (cross-platform) as a heavily styled version of the <select> popup that we use on non-Mac platforms. The color I'm using below matches the color of this implementation. This CL is moving the UI implementation into the browser process. That move has already been done for GTK and Views. So, the "other platforms" referred to in the TODO are the native Chrome UI implementations on GTK and Views. Specifically, the strange color is kBorderColor here: https://code.google.com/searchframe#OAMlx_jo-ck/src/chrome/browser/ui/gtk/aut... > Since this is local UI which won't be transferred across devices, I'd just use > whichever calibrated or device variant other people use. Ok, I'll just go with "calibrated" everywhere, I guess -- looks like that's what the omnibox popup view uses :) https://chromiumcodereview.appspot.com/11740033/diff/1/chrome/browser/ui/coco... chrome/browser/ui/cocoa/autofill/autofill_popup_view_cocoa.mm:174: blue:220/255.0 On 2013/01/05 01:06:49, shess wrote: > If they're all the same, -colorWithDeviceWhite: should work. Done. https://chromiumcodereview.appspot.com/11740033/diff/1/chrome/browser/ui/coco... chrome/browser/ui/cocoa/autofill/autofill_popup_view_cocoa.mm:188: // color. On 2013/01/05 01:06:49, shess wrote: > What about one of +controlHighlighColor or +controlLightHighlightColor or > +highlightColor? The docs at [1] say not to use the "control" ones: """Note: A number of system colors, while still valid, are no longer meaningful under Aqua. These include any of the ones with "control" in their name. They return the old Platinum appearance colors which are usually a shade of gray.""" Judging by those docs, I probably want selectedTextBackgroundColor. OTOH, the omnibox popup uses controlHighlightColor, which seems to work. I'd prefer to leave this as a TODO for now, especially since the color I'm using below matches what GTK and Views do, so it's not 100% clear what the "right" color is. [1] https://developer.apple.com/library/mac/#documentation/Cocoa/Conceptual/DrawC... https://chromiumcodereview.appspot.com/11740033/diff/1/chrome/browser/ui/coco... chrome/browser/ui/cocoa/autofill/autofill_popup_view_cocoa.mm:243: y = bounds.origin.y + (bounds.size.height - iconSize.height) / 2; On 2013/01/05 01:06:49, shess wrote: > Depending on what the icon is, I'm not sure if drawing on a half boundary is > what you want. Hmm, what do you mean by drawing on a half boundary? This code is meant to center the icon vertically. https://chromiumcodereview.appspot.com/11740033/diff/1/chrome/browser/ui/coco... File chrome/browser/ui/cocoa/autofill/autofill_popup_view_mac.mm (right): https://chromiumcodereview.appspot.com/11740033/diff/1/chrome/browser/ui/coco... chrome/browser/ui/cocoa/autofill/autofill_popup_view_mac.mm:24: // NSFloatingWindowLevel or NSModalPanelWindowLevel instead? On 2013/01/05 01:06:49, shess wrote: > On 2013/01/04 01:24:29, Ilya Sherman wrote: > > Friendly Cocoa reviewers: Thoughts? > > Omnibox is at NSNormalWindowLevel. Levels are a bit weird - so long as you > order above your window, and are operating WRT that window (not as something > independent), you maybe don't need to worry about this at all. Actually, I see > that below you're making it a child window - in that case I don't think you need > to worry about levels at all. Done. https://chromiumcodereview.appspot.com/11740033/diff/1/chrome/browser/ui/coco... chrome/browser/ui/cocoa/autofill/autofill_popup_view_mac.mm:26: // TODO(isherman): Are these explicit calls needed / recommended? On 2013/01/05 01:06:49, shess wrote: > On 2013/01/04 01:24:29, Ilya Sherman wrote: > > Friendly Cocoa reviewers: Thoughts? > > For borderless, you might need to disable moving by background. I'm not sure > how the background color and alpha interact. The opaque setting is a signal > others use to optimize things (setting to YES signals that you're arranging it > to be opaque). It looks like everything works if I remove all of these, including the window level, so I've removed them (other than opaque), on the assumption that less code is better. Let me know if there are some you think I should definitely keep. https://chromiumcodereview.appspot.com/11740033/diff/1/chrome/browser/ui/coco... chrome/browser/ui/cocoa/autofill/autofill_popup_view_mac.mm:42: [view_ controllerDestroyed]; On 2013/01/05 01:06:49, shess wrote: > Since this is called from the destructor, you might want to document on the view > side that the controller_ should not be called at this point. Sorry, I'm confused -- document where? https://chromiumcodereview.appspot.com/11740033/diff/1/chrome/browser/ui/coco... chrome/browser/ui/cocoa/autofill/autofill_popup_view_mac.mm:73: // be in view coordinates. On 2013/01/05 01:06:49, shess wrote: > On 2013/01/04 01:24:29, Ilya Sherman wrote: > > Friendly Cocoa reviewers: Thoughts? > > I think I'm going to need to patch this in and look at it before I can > speculate. What kind of thing do I need to be doing in a webpage to see this at > work? As preconditions, you'll need to enable the flag for this UI, and you'll need to have some Autofill profiles or credit cards saved at chrome://settings/autofill. (Credit cards work best, since they have extra icons.) Then, navigate to a page like [1] with a fillable form, and double click one of the form fields to bring up the UI. For best results, compare to the same action in an official build, since this CL is about migrating that UI to be a Chrome native one. [1] https://www.corp.google.com/~isherman/no_crawl/autofill/cc.html
https://chromiumcodereview.appspot.com/11740033/diff/1/chrome/browser/ui/coco... File chrome/browser/ui/cocoa/autofill/autofill_popup_view_cocoa.mm (right): https://chromiumcodereview.appspot.com/11740033/diff/1/chrome/browser/ui/coco... chrome/browser/ui/cocoa/autofill/autofill_popup_view_cocoa.mm:45: - (gfx::Point)flipNSPointToPoint:(NSPoint)point; On 2013/01/05 03:14:10, Ilya Sherman wrote: > On 2013/01/05 01:06:49, shess wrote: > > Rather than making explicit conversions, it might make sense to override > > -isFlipped to return YES. All your math is going to change, of course. > > I thought about that, but I think isFlipped just flips the coordinate within the > view's coordinate system, whereas we want to flip the coordinate and then > translate it so that it's in screen coordinates. If it returned YES to -isFlipped, then -convertPoint:fromView: would have already handled things, and you'd just convert the NSPoint to gfx::Point. Also the explicit flip in drawing would go away, I think (at which point all of your explicit flips would be gone). https://chromiumcodereview.appspot.com/11740033/diff/1/chrome/browser/ui/coco... chrome/browser/ui/cocoa/autofill/autofill_popup_view_cocoa.mm:188: // color. On 2013/01/05 03:14:10, Ilya Sherman wrote: > On 2013/01/05 01:06:49, shess wrote: > > What about one of +controlHighlighColor or +controlLightHighlightColor or > > +highlightColor? > > The docs at [1] say not to use the "control" ones: > > """Note: A number of system colors, while still valid, are no longer meaningful > under Aqua. These include any of the ones with "control" in their name. They > return the old Platinum appearance colors which are usually a shade of gray.""" > > Judging by those docs, I probably want selectedTextBackgroundColor. OTOH, the > omnibox popup uses controlHighlightColor, which seems to work. I'd prefer to > leave this as a TODO for now, especially since the color I'm using below matches > what GTK and Views do, so it's not 100% clear what the "right" color is. > > [1] > https://developer.apple.com/library/mac/#documentation/Cocoa/Conceptual/DrawC... > In that case, I'll just quibble with the oddity of using decimal in some places and hex in others. We don't really have a cross-platform color library, I think, so there's not much to be done about having constants all over, and having static's for each of RGB is annoying, but I have found something that having a function in the anonymous namespace to return BackgroundColor() or something of the sort does make things slightly easier to read. https://chromiumcodereview.appspot.com/11740033/diff/1/chrome/browser/ui/coco... chrome/browser/ui/cocoa/autofill/autofill_popup_view_cocoa.mm:243: y = bounds.origin.y + (bounds.size.height - iconSize.height) / 2; On 2013/01/05 03:14:10, Ilya Sherman wrote: > On 2013/01/05 01:06:49, shess wrote: > > Depending on what the icon is, I'm not sure if drawing on a half boundary is > > what you want. > > Hmm, what do you mean by drawing on a half boundary? This code is meant to > center the icon vertically. Basically, the image will be drawn at either floor(x),floor(y) or ceil(x),ceil(y), except on hidpi displays which can align to the midpoint. So generally I think it's best to choose the alignment yourself, or to simply arrange for alignment not to be a problem in the first place by making things evenly divisible by 2. This is probably a non-issue, though, so ignore the comment. https://chromiumcodereview.appspot.com/11740033/diff/1/chrome/browser/ui/coco... File chrome/browser/ui/cocoa/autofill/autofill_popup_view_mac.mm (right): https://chromiumcodereview.appspot.com/11740033/diff/1/chrome/browser/ui/coco... chrome/browser/ui/cocoa/autofill/autofill_popup_view_mac.mm:42: [view_ controllerDestroyed]; On 2013/01/05 03:14:10, Ilya Sherman wrote: > On 2013/01/05 01:06:49, shess wrote: > > Since this is called from the destructor, you might want to document on the > view > > side that the controller_ should not be called at this point. > > Sorry, I'm confused -- document where? I mean that -controllerDestroyed cannot send any messages to |this|, so it might be worth documenting that at -controllerDestroyed's implementation. https://chromiumcodereview.appspot.com/11740033/diff/1/chrome/browser/ui/coco... chrome/browser/ui/cocoa/autofill/autofill_popup_view_mac.mm:73: // be in view coordinates. On 2013/01/05 03:14:10, Ilya Sherman wrote: > As preconditions, you'll need to enable the flag for this UI, and you'll need to > have some Autofill profiles or credit cards saved at chrome://settings/autofill. > (Credit cards work best, since they have extra icons.) Then, navigate to a > page like [1] with a fillable form, and double click one of the form fields to > bring up the UI. For best results, compare to the same action in an official > build, since this CL is about migrating that UI to be a Chrome native one. > > [1] https://www.corp.google.com/%7Eisherman/no_crawl/autofill/cc.html Weird. When I double-click a field, then click (either in the box or elsewhere on the page), the Chromium window dissappears. I can get it back by switching to another application and then back, but the popup stays up and I can't get it to go away, even when switching tabs. I'm also seeing lots of string truncation ("Chromium Autofi"). This is on a retina mac running 10.8. Anyhow, looking at things, if I were to make suggestions about the "best" way to break things down, I'd use a window with a custom content view to draw the place you're putting things, then a subview which was an NSMatrix for the elements, which would be custom cells. But the use of a separator may mess that up. The rationale for using this kind of setup is that various framework things will kick in automatically, like handling of accessibility, and possibly the RTL stuff. Also - what is your scrolling story if the popup gets too big? https://chromiumcodereview.appspot.com/11740033/diff/7001/chrome/browser/ui/c... File chrome/browser/ui/cocoa/autofill/autofill_popup_view_cocoa.mm (right): https://chromiumcodereview.appspot.com/11740033/diff/7001/chrome/browser/ui/c... chrome/browser/ui/cocoa/autofill/autofill_popup_view_cocoa.mm:136: NSEventType event_type = [theEvent type]; It seems kind of roundabout to override -mouseUp:, -mouseExited: -mouseMoved: and -mouseDragged: to call -mouseEvent: and then dispatch manually to specific controller_ functions. Does it look that bad to have them coded up individually, perhaps with -mouseDragged: just calling -mouseMoved:? If that is too much, move event_type down to where it's used, and convert to eventType because of Objective-C. https://chromiumcodereview.appspot.com/11740033/diff/7001/chrome/browser/ui/c... chrome/browser/ui/cocoa/autofill/autofill_popup_view_cocoa.mm:140: // Convert to Chrome's screen coordinates. I don't understand what "screen coordinates" means in this case? AFAICT what you're calculating is the mouse location relative to the upper-left of the popup. Also, screenLocation not screen_location. https://chromiumcodereview.appspot.com/11740033/diff/7001/chrome/browser/ui/c... chrome/browser/ui/cocoa/autofill/autofill_popup_view_cocoa.mm:154: - (void)controllerDestroyed { This is where I meant to drop a comment. When this is called, it indicated that controller_ cannot be safely accessed. Just so someone doesn't do something like one-last-update or whatever. https://chromiumcodereview.appspot.com/11740033/diff/7001/chrome/browser/ui/c... chrome/browser/ui/cocoa/autofill/autofill_popup_view_cocoa.mm:260: rect.size = NSZeroSize; NSRect rect = {point, NSZeroSize}; should work. Or NSMakeRect(point.x, point.y, 0, 0) is pretty readible, too. https://chromiumcodereview.appspot.com/11740033/diff/7001/chrome/browser/ui/c... File chrome/browser/ui/cocoa/autofill/autofill_popup_view_mac.mm (right): https://chromiumcodereview.appspot.com/11740033/diff/7001/chrome/browser/ui/c... chrome/browser/ui/cocoa/autofill/autofill_popup_view_mac.mm:28: frame:[window_ frame]] autorelease]; [window_ frame] seems wrong. I expect that -setContentView: is resizing things to fit the window. [[window_ contentView] frame] is probably better. https://chromiumcodereview.appspot.com/11740033/diff/7001/chrome/browser/ui/c... chrome/browser/ui/cocoa/autofill/autofill_popup_view_mac.mm:64: // be in view coordinates. I'm not sure the various coordinate systems this TODO mentions even relate to hidpi. It seems to position right on my hidpi box, so I'd be inclined to remove the TODO. https://chromiumcodereview.appspot.com/11740033/diff/7001/chrome/browser/ui/c... chrome/browser/ui/cocoa/autofill/autofill_popup_view_mac.mm:66: controller_->element_bounds().bottom(), I found this confusing until I read it a couple times, as I thought it was flipping things. I believe that this is placing the popup below the element which initiated it? https://chromiumcodereview.appspot.com/11740033/diff/7001/chrome/browser/ui/c... chrome/browser/ui/cocoa/autofill/autofill_popup_view_mac.mm:73: bounds.set_y([screen frame].size.height - bounds.y() - bounds.height()); *splode* I don't fully understand this. Shouldn't the bounds help by the controller_ always be rooted at the upper-left, with the flipping happening in UpdateBoundsAndRedrawPopup() when converting from the gfx::Rect to the NSRect?
https://codereview.chromium.org/11740033/diff/7001/chrome/browser/ui/cocoa/au... File chrome/browser/ui/cocoa/autofill/autofill_popup_view_cocoa.mm (right): https://codereview.chromium.org/11740033/diff/7001/chrome/browser/ui/cocoa/au... chrome/browser/ui/cocoa/autofill/autofill_popup_view_cocoa.mm:207: deleteIcon = rb.GetImageNamed(IDR_CLOSE_BAR_H).ToNSImage(); drive-by: use GetNativeImageNamed to avoid an Skia conversion
https://chromiumcodereview.appspot.com/11740033/diff/1/chrome/browser/ui/coco... File chrome/browser/ui/cocoa/autofill/autofill_popup_view_cocoa.mm (right): https://chromiumcodereview.appspot.com/11740033/diff/1/chrome/browser/ui/coco... chrome/browser/ui/cocoa/autofill/autofill_popup_view_cocoa.mm:45: - (gfx::Point)flipNSPointToPoint:(NSPoint)point; On 2013/01/09 21:36:17, shess wrote: > On 2013/01/05 03:14:10, Ilya Sherman wrote: > > On 2013/01/05 01:06:49, shess wrote: > > > Rather than making explicit conversions, it might make sense to override > > > -isFlipped to return YES. All your math is going to change, of course. > > > > I thought about that, but I think isFlipped just flips the coordinate within > the > > view's coordinate system, whereas we want to flip the coordinate and then > > translate it so that it's in screen coordinates. > > If it returned YES to -isFlipped, then -convertPoint:fromView: would have > already handled things, and you'd just convert the NSPoint to gfx::Point. Also > the explicit flip in drawing would go away, I think (at which point all of your > explicit flips would be gone). You're right, I was misunderstanding what space exactly I was flipping within. Done. https://chromiumcodereview.appspot.com/11740033/diff/1/chrome/browser/ui/coco... chrome/browser/ui/cocoa/autofill/autofill_popup_view_cocoa.mm:188: // color. On 2013/01/09 21:36:17, shess wrote: > On 2013/01/05 03:14:10, Ilya Sherman wrote: > > On 2013/01/05 01:06:49, shess wrote: > > > What about one of +controlHighlighColor or +controlLightHighlightColor or > > > +highlightColor? > > > > The docs at [1] say not to use the "control" ones: > > > > """Note: A number of system colors, while still valid, are no longer > meaningful > > under Aqua. These include any of the ones with "control" in their name. They > > return the old Platinum appearance colors which are usually a shade of > gray.""" > > > > Judging by those docs, I probably want selectedTextBackgroundColor. OTOH, the > > omnibox popup uses controlHighlightColor, which seems to work. I'd prefer to > > leave this as a TODO for now, especially since the color I'm using below > matches > > what GTK and Views do, so it's not 100% clear what the "right" color is. > > > > [1] > > > https://developer.apple.com/library/mac/#documentation/Cocoa/Conceptual/DrawC... > > > > In that case, I'll just quibble with the oddity of using decimal in some places > and hex in others. We don't really have a cross-platform color library, I > think, so there's not much to be done about having constants all over, and > having static's for each of RGB is annoying, but I have found something that > having a function in the anonymous namespace to return BackgroundColor() or > something of the sort does make things slightly easier to read. Done. https://chromiumcodereview.appspot.com/11740033/diff/1/chrome/browser/ui/coco... File chrome/browser/ui/cocoa/autofill/autofill_popup_view_mac.mm (right): https://chromiumcodereview.appspot.com/11740033/diff/1/chrome/browser/ui/coco... chrome/browser/ui/cocoa/autofill/autofill_popup_view_mac.mm:42: [view_ controllerDestroyed]; On 2013/01/09 21:36:17, shess wrote: > On 2013/01/05 03:14:10, Ilya Sherman wrote: > > On 2013/01/05 01:06:49, shess wrote: > > > Since this is called from the destructor, you might want to document on the > > view > > > side that the controller_ should not be called at this point. > > > > Sorry, I'm confused -- document where? > > I mean that -controllerDestroyed cannot send any messages to |this|, so it might > be worth documenting that at -controllerDestroyed's implementation. Done. https://chromiumcodereview.appspot.com/11740033/diff/1/chrome/browser/ui/coco... chrome/browser/ui/cocoa/autofill/autofill_popup_view_mac.mm:73: // be in view coordinates. On 2013/01/09 21:36:17, shess wrote: > On 2013/01/05 03:14:10, Ilya Sherman wrote: > > As preconditions, you'll need to enable the flag for this UI, and you'll need > to > > have some Autofill profiles or credit cards saved at > chrome://settings/autofill. > > (Credit cards work best, since they have extra icons.) Then, navigate to a > > page like [1] with a fillable form, and double click one of the form fields to > > bring up the UI. For best results, compare to the same action in an official > > build, since this CL is about migrating that UI to be a Chrome native one. > > > > [1] https://www.corp.google.com/%257Eisherman/no_crawl/autofill/cc.html > > Weird. When I double-click a field, then click (either in the box or elsewhere > on the page), the Chromium window dissappears. I can get it back by switching > to another application and then back, but the popup stays up and I can't get it > to go away, even when switching tabs. Huh, I don't see that behavior. I guess this means I'm doing something dumb that causes unspecified behavior, but I'm not sure what that something dumb is -- any ideas? > I'm also seeing lots of string truncation ("Chromium Autofi"). This is on a retina mac running 10.8. Yeah, the strings are being intentionally truncated in the cross-platform controller because we haven't figured out the story for eliding the strings when they get to long. This is our slap-you-in-the-face reminder not to ship the UI prior to figuring that out. > Anyhow, looking at things, if I were to make suggestions about the "best" way to > break things down, I'd use a window with a custom content view to draw the place > you're putting things, then a subview which was an NSMatrix for the elements, > which would be custom cells. But the use of a separator may mess that up. The > rationale for using this kind of setup is that various framework things will > kick in automatically, like handling of accessibility, and possibly the RTL > stuff. Hmm, I'll investigate that. > Also - what is your scrolling story if the popup gets too big? Not sure... I should probably figure that out. Added a TODO for now. https://chromiumcodereview.appspot.com/11740033/diff/7001/chrome/browser/ui/c... File chrome/browser/ui/cocoa/autofill/autofill_popup_view_cocoa.mm (right): https://chromiumcodereview.appspot.com/11740033/diff/7001/chrome/browser/ui/c... chrome/browser/ui/cocoa/autofill/autofill_popup_view_cocoa.mm:136: NSEventType event_type = [theEvent type]; On 2013/01/09 21:36:17, shess wrote: > It seems kind of roundabout to override -mouseUp:, -mouseExited: -mouseMoved: > and -mouseDragged: to call -mouseEvent: and then dispatch manually to specific > controller_ functions. Does it look that bad to have them coded up > individually, perhaps with -mouseDragged: just calling -mouseMoved:? > > If that is too much, move event_type down to where it's used, and convert to > eventType because of Objective-C. Done. I was mostly just following the BaseView class design, on the assumption that there was some reason for funneling mouse events that way. https://chromiumcodereview.appspot.com/11740033/diff/7001/chrome/browser/ui/c... chrome/browser/ui/cocoa/autofill/autofill_popup_view_cocoa.mm:140: // Convert to Chrome's screen coordinates. On 2013/01/09 21:36:17, shess wrote: > I don't understand what "screen coordinates" means in this case? AFAICT what > you're calculating is the mouse location relative to the upper-left of the > popup. > > Also, screenLocation not screen_location. Moot. https://chromiumcodereview.appspot.com/11740033/diff/7001/chrome/browser/ui/c... chrome/browser/ui/cocoa/autofill/autofill_popup_view_cocoa.mm:154: - (void)controllerDestroyed { On 2013/01/09 21:36:17, shess wrote: > This is where I meant to drop a comment. When this is called, it indicated that > controller_ cannot be safely accessed. Just so someone doesn't do something > like one-last-update or whatever. Done. https://chromiumcodereview.appspot.com/11740033/diff/7001/chrome/browser/ui/c... chrome/browser/ui/cocoa/autofill/autofill_popup_view_cocoa.mm:207: deleteIcon = rb.GetImageNamed(IDR_CLOSE_BAR_H).ToNSImage(); On 2013/01/09 22:17:30, rsesek wrote: > drive-by: use GetNativeImageNamed to avoid an Skia conversion Thanks, done. https://chromiumcodereview.appspot.com/11740033/diff/7001/chrome/browser/ui/c... chrome/browser/ui/cocoa/autofill/autofill_popup_view_cocoa.mm:260: rect.size = NSZeroSize; On 2013/01/09 21:36:17, shess wrote: > NSRect rect = {point, NSZeroSize}; should work. > Or NSMakeRect(point.x, point.y, 0, 0) is pretty readible, too. Moot. https://chromiumcodereview.appspot.com/11740033/diff/7001/chrome/browser/ui/c... File chrome/browser/ui/cocoa/autofill/autofill_popup_view_mac.mm (right): https://chromiumcodereview.appspot.com/11740033/diff/7001/chrome/browser/ui/c... chrome/browser/ui/cocoa/autofill/autofill_popup_view_mac.mm:28: frame:[window_ frame]] autorelease]; On 2013/01/09 21:36:17, shess wrote: > [window_ frame] seems wrong. I expect that -setContentView: is resizing things > to fit the window. [[window_ contentView] frame] is probably better. Done. https://chromiumcodereview.appspot.com/11740033/diff/7001/chrome/browser/ui/c... chrome/browser/ui/cocoa/autofill/autofill_popup_view_mac.mm:64: // be in view coordinates. On 2013/01/09 21:36:17, shess wrote: > I'm not sure the various coordinate systems this TODO mentions even relate to > hidpi. It seems to position right on my hidpi box, so I'd be inclined to remove > the TODO. Done. https://chromiumcodereview.appspot.com/11740033/diff/7001/chrome/browser/ui/c... chrome/browser/ui/cocoa/autofill/autofill_popup_view_mac.mm:66: controller_->element_bounds().bottom(), On 2013/01/09 21:36:17, shess wrote: > I found this confusing until I read it a couple times, as I thought it was > flipping things. I believe that this is placing the popup below the element > which initiated it? Done. https://chromiumcodereview.appspot.com/11740033/diff/7001/chrome/browser/ui/c... chrome/browser/ui/cocoa/autofill/autofill_popup_view_mac.mm:73: bounds.set_y([screen frame].size.height - bounds.y() - bounds.height()); On 2013/01/09 21:36:17, shess wrote: > *splode* > > I don't fully understand this. Shouldn't the bounds help by the controller_ > always be rooted at the upper-left, with the flipping happening in > UpdateBoundsAndRedrawPopup() when converting from the gfx::Rect to the NSRect? That's a really good point. Done.
https://chromiumcodereview.appspot.com/11740033/diff/1/chrome/browser/ui/coco... File chrome/browser/ui/cocoa/autofill/autofill_popup_view_mac.mm (right): https://chromiumcodereview.appspot.com/11740033/diff/1/chrome/browser/ui/coco... chrome/browser/ui/cocoa/autofill/autofill_popup_view_mac.mm:73: // be in view coordinates. On 2013/01/10 02:21:21, Ilya Sherman wrote: > On 2013/01/09 21:36:17, shess wrote: > > Anyhow, looking at things, if I were to make suggestions about the "best" way > to > > break things down, I'd use a window with a custom content view to draw the > place > > you're putting things, then a subview which was an NSMatrix for the elements, > > which would be custom cells. But the use of a separator may mess that up. > The > > rationale for using this kind of setup is that various framework things will > > kick in automatically, like handling of accessibility, and possibly the RTL > > stuff. > > Hmm, I'll investigate that. According to the docs, the separators do indeed mess that up: "The cell objects that an NSMatrix contains are usually of a single subclass of NSCell, but they can be of multiple subclasses of NSCell. The only restriction is that all cell objects must be the same size." I suppose I could theoretically draw the separator as a border on the cell above it, though it would mean that the controller and the view would disagree on the view's size by a pixel per separator. This approach sounds generally kind of icky, and I'd prefer to find some moar better solution. WDYT?
lgtm. I'm now finding myself mixed on whether these should be all-one-file or two. I still think _cocoa versus _mac is confusing, but 400 lines is starting to seem largish. OTOH, 400 lines really isn't that big for Chrome ... https://chromiumcodereview.appspot.com/11740033/diff/7001/chrome/browser/ui/c... File chrome/browser/ui/cocoa/autofill/autofill_popup_view_cocoa.mm (right): https://chromiumcodereview.appspot.com/11740033/diff/7001/chrome/browser/ui/c... chrome/browser/ui/cocoa/autofill/autofill_popup_view_cocoa.mm:136: NSEventType event_type = [theEvent type]; On 2013/01/10 02:21:21, Ilya Sherman wrote: > On 2013/01/09 21:36:17, shess wrote: > > It seems kind of roundabout to override -mouseUp:, -mouseExited: -mouseMoved: > > and -mouseDragged: to call -mouseEvent: and then dispatch manually to specific > > controller_ functions. Does it look that bad to have them coded up > > individually, perhaps with -mouseDragged: just calling -mouseMoved:? > > > > If that is too much, move event_type down to where it's used, and convert to > > eventType because of Objective-C. > > Done. I was mostly just following the BaseView class design, on the assumption > that there was some reason for funneling mouse events that way. I think BaseView might do that for purposes of subclasses which need to funnel all events somewhere else for dispatch. Though I might be wrong. Regardless, I think leaving them broken out makes it somewhat clearer which bits of per-event magic belong together, which might prevent future developers from making ill-advised changes. [It's also possible you shouldn't be a BaseView subclass in the first place. Maybe Avi would know.] https://chromiumcodereview.appspot.com/11740033/diff/15001/chrome/browser/ui/... File chrome/browser/ui/cocoa/autofill/autofill_popup_view_mac.mm (right): https://chromiumcodereview.appspot.com/11740033/diff/15001/chrome/browser/ui/... chrome/browser/ui/cocoa/autofill/autofill_popup_view_mac.mm:63: [screen frame].size.height - frame.origin.y - frame.size.height; This could be more compactly phrased as: frame.origin.y = [screen frame].size.height - NSMaxY(frame); Instead of using screen 0, I think [[controller_->container_view() window] screen] would be the screen that your window's coordinates are relative to. Of course _that_ might not match the coordinates from controller_->popup_bounds() :-). https://chromiumcodereview.appspot.com/11740033/diff/15001/chrome/browser/ui/... chrome/browser/ui/cocoa/autofill/autofill_popup_view_mac.mm:76: controller_->GetPopupRequiredHeight()); I like all this better than before.
On 2013/01/10 04:17:15, Ilya Sherman wrote: > https://chromiumcodereview.appspot.com/11740033/diff/1/chrome/browser/ui/coco... > File chrome/browser/ui/cocoa/autofill/autofill_popup_view_mac.mm (right): > > https://chromiumcodereview.appspot.com/11740033/diff/1/chrome/browser/ui/coco... > chrome/browser/ui/cocoa/autofill/autofill_popup_view_mac.mm:73: // be in view > coordinates. > On 2013/01/10 02:21:21, Ilya Sherman wrote: > > On 2013/01/09 21:36:17, shess wrote: > > > Anyhow, looking at things, if I were to make suggestions about the "best" > way > > to > > > break things down, I'd use a window with a custom content view to draw the > > place > > > you're putting things, then a subview which was an NSMatrix for the > elements, > > > which would be custom cells. But the use of a separator may mess that up. > > The > > > rationale for using this kind of setup is that various framework things will > > > kick in automatically, like handling of accessibility, and possibly the RTL > > > stuff. > > > > Hmm, I'll investigate that. > > According to the docs, the separators do indeed mess that up: "The cell objects > that an NSMatrix contains are usually of a single subclass of NSCell, but they > can be of multiple subclasses of NSCell. The only restriction is that all cell > objects must be the same size." > > I suppose I could theoretically draw the separator as a border on the cell above > it, though it would mean that the controller and the view would disagree on the > view's size by a pixel per separator. This approach sounds generally kind of > icky, and I'd prefer to find some moar better solution. WDYT? I guess it depends on what the goal is. One option would be to have a separator space between all the cells, and only draw it for the case where you need it. Another option would be to hope someone writes a custom menu-looking thing for general use, though don't hold your breath. For now, just keep it in mind, if you find yourself manually implementing behaviors which should have otherwise been system-implemented, it can be re-visited.
+Nico for chrome/ OWNERS approval On 2013/01/10 22:02:57, shess wrote: > I'm now finding myself mixed on whether these should be all-one-file or two. I > still think _cocoa versus _mac is confusing, but 400 lines is starting to seem > largish. OTOH, 400 lines really isn't that big for Chrome ... If you're not too particular, I'd prefer to keep the files separated for now, despite the confusingly similar names. Maybe Nico will have a strong opinion, though... https://chromiumcodereview.appspot.com/11740033/diff/15001/chrome/browser/ui/... File chrome/browser/ui/cocoa/autofill/autofill_popup_view_mac.mm (right): https://chromiumcodereview.appspot.com/11740033/diff/15001/chrome/browser/ui/... chrome/browser/ui/cocoa/autofill/autofill_popup_view_mac.mm:63: [screen frame].size.height - frame.origin.y - frame.size.height; On 2013/01/10 22:02:57, shess wrote: > This could be more compactly phrased as: > frame.origin.y = [screen frame].size.height - NSMaxY(frame); > > Instead of using screen 0, I think [[controller_->container_view() window] > screen] would be the screen that your window's coordinates are relative to. Of > course _that_ might not match the coordinates from controller_->popup_bounds() > :-). Done.
https://chromiumcodereview.appspot.com/11740033/diff/21001/chrome/browser/ui/... File chrome/browser/ui/cocoa/autofill/autofill_popup_view_cocoa.h (right): https://chromiumcodereview.appspot.com/11740033/diff/21001/chrome/browser/ui/... chrome/browser/ui/cocoa/autofill/autofill_popup_view_cocoa.h:19: __weak AutofillPopupController* controller_; We don't use arc, so __weak does nothing for us. Don't we just use "not scoped_nsobject" for weak? (feel free to ignore) https://chromiumcodereview.appspot.com/11740033/diff/21001/chrome/browser/ui/... File chrome/browser/ui/cocoa/autofill/autofill_popup_view_cocoa.mm (right): https://chromiumcodereview.appspot.com/11740033/diff/21001/chrome/browser/ui/... chrome/browser/ui/cocoa/autofill/autofill_popup_view_cocoa.mm:34: return [NSColor colorWithCalibratedWhite:220/255.0 alpha:1]; spaces around / https://chromiumcodereview.appspot.com/11740033/diff/21001/chrome/browser/ui/... chrome/browser/ui/cocoa/autofill/autofill_popup_view_cocoa.mm:38: return [NSColor colorWithCalibratedWhite:(205 / 255.0) alpha:1]; no parens https://chromiumcodereview.appspot.com/11740033/diff/21001/chrome/browser/ui/... chrome/browser/ui/cocoa/autofill/autofill_popup_view_cocoa.mm:125: [border stroke]; Is there any chance to move all drawing to assets? That's what we're trying to do with the rest of the UI (on all platforms). https://chromiumcodereview.appspot.com/11740033/diff/21001/chrome/browser/ui/... chrome/browser/ui/cocoa/autofill/autofill_popup_view_cocoa.mm:221: BOOL isRTL = base::i18n::IsRTL(); We don't really support RTL anywhere else (just saying) https://chromiumcodereview.appspot.com/11740033/diff/21001/chrome/browser/ui/... chrome/browser/ui/cocoa/autofill/autofill_popup_view_cocoa.mm:241: // Draw the delete icon, if one is needed. Shouldn't this be a subview? https://chromiumcodereview.appspot.com/11740033/diff/21001/chrome/browser/ui/... File chrome/browser/ui/cocoa/autofill/autofill_popup_view_mac.mm (right): https://chromiumcodereview.appspot.com/11740033/diff/21001/chrome/browser/ui/... chrome/browser/ui/cocoa/autofill/autofill_popup_view_mac.mm:7: #include "chrome/browser/ui/cocoa/autofill/autofill_popup_view_mac.h" I'm not a huge fan of _mac / _cocoa either, especially since we're fairly inconsistent about it. Having @intefaces in _mac files is a bit more common: https://code.google.com/p/chromium/source/search?q=file%3A_mac%5C.h+%40interf... https://code.google.com/p/chromium/source/search?q=file%3A_cocoa%5C.h+%40inte... But your naming feels more logical to me. I suppose you can file a cleanup bug to make the naming consistent for all files (it's not all that many), and discuss which way to there and then we can slowly move to that state. I don't think it should block this CL. https://chromiumcodereview.appspot.com/11740033/diff/21001/chrome/browser/ui/... chrome/browser/ui/cocoa/autofill/autofill_popup_view_mac.mm:63: frame.origin.y = [screen frame].size.height - NSMaxY(frame); NSHeight()
https://chromiumcodereview.appspot.com/11740033/diff/21001/chrome/browser/ui/... File chrome/browser/ui/cocoa/autofill/autofill_popup_view_cocoa.h (right): https://chromiumcodereview.appspot.com/11740033/diff/21001/chrome/browser/ui/... chrome/browser/ui/cocoa/autofill/autofill_popup_view_cocoa.h:19: __weak AutofillPopupController* controller_; On 2013/01/11 16:27:38, Nico wrote: > We don't use arc, so __weak does nothing for us. Don't we just use "not > scoped_nsobject" for weak? > > (feel free to ignore) I've used __weak in the past instead of "// weak" which is what we do when it's not in scoped_nsobject
On 2013/01/11 16:28:30, rsesek wrote: > https://chromiumcodereview.appspot.com/11740033/diff/21001/chrome/browser/ui/... > File chrome/browser/ui/cocoa/autofill/autofill_popup_view_cocoa.h (right): > > https://chromiumcodereview.appspot.com/11740033/diff/21001/chrome/browser/ui/... > chrome/browser/ui/cocoa/autofill/autofill_popup_view_cocoa.h:19: __weak > AutofillPopupController* controller_; > On 2013/01/11 16:27:38, Nico wrote: > > We don't use arc, so __weak does nothing for us. Don't we just use "not > > scoped_nsobject" for weak? > > > > (feel free to ignore) > > I've used __weak in the past instead of "// weak" which is what we do when it's > not in scoped_nsobject I think it's slightly confusing to use syntax that looks like it does something. Most people will think it's needed somehow. With "// weak" it's obvious that it's just an annotation. (And we're likely not moving to arc soon.)
On 2013/01/11 16:31:35, Nico wrote: > On 2013/01/11 16:28:30, rsesek wrote: > > > https://chromiumcodereview.appspot.com/11740033/diff/21001/chrome/browser/ui/... > > File chrome/browser/ui/cocoa/autofill/autofill_popup_view_cocoa.h (right): > > > > > https://chromiumcodereview.appspot.com/11740033/diff/21001/chrome/browser/ui/... > > chrome/browser/ui/cocoa/autofill/autofill_popup_view_cocoa.h:19: __weak > > AutofillPopupController* controller_; > > On 2013/01/11 16:27:38, Nico wrote: > > > We don't use arc, so __weak does nothing for us. Don't we just use "not > > > scoped_nsobject" for weak? > > > > > > (feel free to ignore) > > > > I've used __weak in the past instead of "// weak" which is what we do when > it's > > not in scoped_nsobject > > I think it's slightly confusing to use syntax that looks like it does something. > Most people will think it's needed somehow. With "// weak" it's obvious that > it's just an annotation. (And we're likely not moving to arc soon.) We had this discussion last time with Mark, and the conclusion was that both were acceptable.
https://chromiumcodereview.appspot.com/11740033/diff/21001/chrome/browser/ui/... File chrome/browser/ui/cocoa/autofill/autofill_popup_view_cocoa.mm (right): https://chromiumcodereview.appspot.com/11740033/diff/21001/chrome/browser/ui/... chrome/browser/ui/cocoa/autofill/autofill_popup_view_cocoa.mm:34: return [NSColor colorWithCalibratedWhite:220/255.0 alpha:1]; On 2013/01/11 16:27:38, Nico wrote: > spaces around / Done. https://chromiumcodereview.appspot.com/11740033/diff/21001/chrome/browser/ui/... chrome/browser/ui/cocoa/autofill/autofill_popup_view_cocoa.mm:38: return [NSColor colorWithCalibratedWhite:(205 / 255.0) alpha:1]; On 2013/01/11 16:27:38, Nico wrote: > no parens Done. https://chromiumcodereview.appspot.com/11740033/diff/21001/chrome/browser/ui/... chrome/browser/ui/cocoa/autofill/autofill_popup_view_cocoa.mm:125: [border stroke]; On 2013/01/11 16:27:38, Nico wrote: > Is there any chance to move all drawing to assets? That's what we're trying to > do with the rest of the UI (on all platforms). Sorry, I don't understand the question :/ Could you elaborate on what it means to move drawing to assets? https://chromiumcodereview.appspot.com/11740033/diff/21001/chrome/browser/ui/... chrome/browser/ui/cocoa/autofill/autofill_popup_view_cocoa.mm:221: BOOL isRTL = base::i18n::IsRTL(); On 2013/01/11 16:27:38, Nico wrote: > We don't really support RTL anywhere else (just saying) The (WebKit) UI this is replacing supports RTL, so this is maintaining parity with that UI. Why don't we support RTL elsewhere, out of curiosity? https://chromiumcodereview.appspot.com/11740033/diff/21001/chrome/browser/ui/... chrome/browser/ui/cocoa/autofill/autofill_popup_view_cocoa.mm:241: // Draw the delete icon, if one is needed. On 2013/01/11 16:27:38, Nico wrote: > Shouldn't this be a subview? What's the advantage? In general, I'm not sure when it's appropriate to use subviews vs. drawing in a single view. Is there a set of best practices, perhaps described in a doc somewhere? https://chromiumcodereview.appspot.com/11740033/diff/21001/chrome/browser/ui/... File chrome/browser/ui/cocoa/autofill/autofill_popup_view_mac.mm (right): https://chromiumcodereview.appspot.com/11740033/diff/21001/chrome/browser/ui/... chrome/browser/ui/cocoa/autofill/autofill_popup_view_mac.mm:7: #include "chrome/browser/ui/cocoa/autofill/autofill_popup_view_mac.h" On 2013/01/11 16:27:38, Nico wrote: > I'm not a huge fan of _mac / _cocoa either, especially since we're fairly > inconsistent about it. Having @intefaces in _mac files is a bit more common: > > > https://code.google.com/p/chromium/source/search?q=file%253A_mac%255C.h+%2540... > > https://code.google.com/p/chromium/source/search?q=file%253A_cocoa%255C.h+%25... > > > But your naming feels more logical to me. I suppose you can file a cleanup bug > to make the naming consistent for all files (it's not all that many), and > discuss which way to there and then we can slowly move to that state. I don't > think it should block this CL. Filed as [ http://crbug.com/169604 ]. Even if we were to combine the files, we'd still need different names for the C++ and Objective-C classes, which I would still be tempted to name *Mac and *Cocoa, which is still roughly equally confusing :/ https://chromiumcodereview.appspot.com/11740033/diff/21001/chrome/browser/ui/... chrome/browser/ui/cocoa/autofill/autofill_popup_view_mac.mm:63: frame.origin.y = [screen frame].size.height - NSMaxY(frame); On 2013/01/11 16:27:38, Nico wrote: > NSHeight() Done.
https://chromiumcodereview.appspot.com/11740033/diff/21001/chrome/browser/ui/... File chrome/browser/ui/cocoa/autofill/autofill_popup_view_cocoa.mm (right): https://chromiumcodereview.appspot.com/11740033/diff/21001/chrome/browser/ui/... chrome/browser/ui/cocoa/autofill/autofill_popup_view_cocoa.mm:125: [border stroke]; On 2013/01/11 23:32:26, Ilya Sherman wrote: > On 2013/01/11 16:27:38, Nico wrote: > > Is there any chance to move all drawing to assets? That's what we're trying to > > do with the rest of the UI (on all platforms). > > Sorry, I don't understand the question :/ Could you elaborate on what it means > to move drawing to assets? Loading bitmap resources and drawing them, so that updating the UI in many cases just requires swapping out a few bitmaps and no code changes. (And also, sharing the same bitmap assets between ports.) https://chromiumcodereview.appspot.com/11740033/diff/21001/chrome/browser/ui/... chrome/browser/ui/cocoa/autofill/autofill_popup_view_cocoa.mm:221: BOOL isRTL = base::i18n::IsRTL(); On 2013/01/11 23:32:26, Ilya Sherman wrote: > On 2013/01/11 16:27:38, Nico wrote: > > We don't really support RTL anywhere else (just saying) > > The (WebKit) UI this is replacing supports RTL, so this is maintaining parity > with that UI. Why don't we support RTL elsewhere, out of curiosity? OS X didn't support it until 10.8 (or 7?) as far as I know. https://chromiumcodereview.appspot.com/11740033/diff/21001/chrome/browser/ui/... chrome/browser/ui/cocoa/autofill/autofill_popup_view_cocoa.mm:241: // Draw the delete icon, if one is needed. On 2013/01/11 23:32:26, Ilya Sherman wrote: > On 2013/01/11 16:27:38, Nico wrote: > > Shouldn't this be a subview? > > What's the advantage? In general, I'm not sure when it's appropriate to use > subviews vs. drawing in a single view. Is there a set of best practices, > perhaps described in a doc somewhere? Abstraction, less code, composability and so on.
https://chromiumcodereview.appspot.com/11740033/diff/21001/chrome/browser/ui/... File chrome/browser/ui/cocoa/autofill/autofill_popup_view_cocoa.mm (right): https://chromiumcodereview.appspot.com/11740033/diff/21001/chrome/browser/ui/... chrome/browser/ui/cocoa/autofill/autofill_popup_view_cocoa.mm:221: BOOL isRTL = base::i18n::IsRTL(); On 2013/01/11 23:40:12, Nico wrote: > On 2013/01/11 23:32:26, Ilya Sherman wrote: > > On 2013/01/11 16:27:38, Nico wrote: > > > We don't really support RTL anywhere else (just saying) > > > > The (WebKit) UI this is replacing supports RTL, so this is maintaining parity > > with that UI. Why don't we support RTL elsewhere, out of curiosity? > > OS X didn't support it until 10.8 (or 7?) as far as I know. Do we have any idea how often the RTL is in use? As things stand, this code looks like a bug magnet, especially if you start wanting to test mouse hits against various parts of things. It may make sense to pull the RTL-handling out to a separate helper which fills an array of NSPoint (or NSRect) based on parameters. That would concentrate the RTL-handling in a way which might make it easier to inspect, leaving the drawing code here. Also, it might allow you to just write two straight versions of the layout, one RTL and one non-RTL, rather than having all of the trinary operators. https://chromiumcodereview.appspot.com/11740033/diff/21001/chrome/browser/ui/... chrome/browser/ui/cocoa/autofill/autofill_popup_view_cocoa.mm:241: // Draw the delete icon, if one is needed. On 2013/01/11 23:40:12, Nico wrote: > On 2013/01/11 23:32:26, Ilya Sherman wrote: > > On 2013/01/11 16:27:38, Nico wrote: > > > Shouldn't this be a subview? > > > > What's the advantage? In general, I'm not sure when it's appropriate to use > > subviews vs. drawing in a single view. Is there a set of best practices, > > perhaps described in a doc somewhere? > > Abstraction, less code, composability and so on. If you're already manually drawing and manually positioning, I would prefer to have it all be done in one place rather than artificially distributing things. Now, if you start needing the close button to hover distinctly from the overall view, or otherwise need to start testing whether you're in the close button or not all over the place, then it makes sense to pull that all out to a separate object.
Ilya tells me that they expect to rewrite the UI anyway, so lgtm.
Renamed the Mac class to Bridge instead, and added TODOs for Nico's comments that we should be using assets and a custom subview for the delete button, as these both require cross-platform changes. I'm going to assume that the name change is OK and check the commit-queue box, but please feel free to cancel the commit or ask me to rename the class in a follow-up CL if the new name is no good. Thanks!
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/isherman@chromium.org/11740033/31001
Message was sent while issue was closed.
Change committed as 176558 |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
