Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1280673003:
[Mac] Enable MacViews site settings bubble behind --enable-mac-views-dialogs. (Closed)
|
Created:
5 years, 4 months ago by jackhou1 Modified:
5 years, 3 months ago CC:
chromium-reviews, tfarina, markusheintz_, raymes+watch_chromium.org, chrome-apps-syd-reviews_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@enabledialogs Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
Description[Mac] Enable MacViews site settings bubble behind --enable-mac-views-dialogs.
This allows BrowserWindowCocoa to show the Views
WebsiteSettingsPopup.
BubbleDelegateView expects a views::View to anchor the bubble, but on
Mac there is only an anchor point. Add
WebsiteSettingsPopupView::ShowPopupAtRect() and pass a zero-size rect
located at the anchor point. This works for bubbles that close as soon as
they lose focus. In the long term the anchor will need to be a Widget to
support bubbles that can stay open.
Don't mirror the bubble position if it is partially offscreen. This is
consistent with Cocoa behavior, and avoids the bubble overlapping the
icon since the Cocoa anchor point is always below the icon.
This will also use the Views CollectedCookies dialog if opened from
WebsiteSettingsPopupView.
Estimated framework size increase (based at r343614):
Before: 108,068,520
After: 108,171,776
Delta: 103,256 (0.1%)
While there's little additional code, the size increase is attributable to
additional parts of toolkit-views being used which can no longer be
culled at link time.
BUG=511051
Committed: https://crrev.com/700bafd0eba1167c3313739142e26dc10a46cd09
Cr-Commit-Position: refs/heads/master@{#343823}
Patch Set 1 #
Total comments: 16
Patch Set 2 : Address comments. #Patch Set 3 : Fix compile. #Patch Set 4 : Sync #
Total comments: 34
Patch Set 5 : Address comments. #
Total comments: 5
Patch Set 6 : Address comments. #Patch Set 7 : Sync #
Total comments: 6
Patch Set 8 : Address comments. #Patch Set 9 : Nits. #
Total comments: 5
Patch Set 10 : Address comments. #
Total comments: 6
Patch Set 11 : Address comments. #Patch Set 12 : Missed a comment. #
Total comments: 13
Patch Set 13 : Address comments. #Messages
Total messages: 40 (4 generated)
jackhou@chromium.org changed reviewers: + tapted@chromium.org
tapted, PTAL This seems to work except for the delay when the bubble appears. I think it's the same delay as the animation, but it doesn't animate on Mac. On Cocoa, the bubble only ever appears below and to the right of the icon. With Views, it can show below and to the left if the icon is close to the right edge of the screen. The collected cookies dialog also works fine except that it's draggable. Seems to be the same with app info. On Windows, app info is draggable, but collected cookies is not.
we should double-check the impact to sizes too. It's "probably" small, but we need to keep tabs on it while this is still experimental. https://codereview.chromium.org/1280673003/diff/1/chrome/browser/ui/browser_d... File chrome/browser/ui/browser_dialogs.h (right): https://codereview.chromium.org/1280673003/diff/1/chrome/browser/ui/browser_d... chrome/browser/ui/browser_dialogs.h:16: class GURL; nit: sort https://codereview.chromium.org/1280673003/diff/1/chrome/browser/ui/browser_d... chrome/browser/ui/browser_dialogs.h:43: class View; this feels out of place here.... Maybe there's a nice fix.. https://codereview.chromium.org/1280673003/diff/1/chrome/browser/ui/browser_d... File chrome/browser/ui/browser_dialogs_mac.cc (right): https://codereview.chromium.org/1280673003/diff/1/chrome/browser/ui/browser_d... chrome/browser/ui/browser_dialogs_mac.cc:10: #include "ui/views/view.h" this include shouldn't be allowed here, but I think DEPS misses it on a technicality (suggested fix follows) https://codereview.chromium.org/1280673003/diff/1/chrome/browser/ui/browser_d... chrome/browser/ui/browser_dialogs_mac.cc:34: views::View* anchor_view = new views::View(); memory leak? This is going to be the hardest thing to fix. Maybe we just inherit from WebsiteSettingsPopupView that deletes the view jammed into the parent Widget when the popup is torn down (should be as easy as `delete GetAnchorView();` -- I think the ViewStorage in BubbleDelegateView will return null if the parent hierarcy somehow gets destroyed first). Then roll a custom WebsiteSettingsPopupView::ShowPopup() for Mac -- there's not much logic there and might help further decouple things. https://codereview.chromium.org/1280673003/diff/1/chrome/browser/ui/cocoa/bro... File chrome/browser/ui/cocoa/browser_window_cocoa.mm (right): https://codereview.chromium.org/1280673003/diff/1/chrome/browser/ui/cocoa/bro... chrome/browser/ui/cocoa/browser_window_cocoa.mm:704: LocationBarViewMac* location_bar = [controller_ locationBarBridge]; I think this needs to move to website_settings_bubble_controller.mm. In WebsiteSettingsUIBridge::Show() Then, the logic is just gfx::Point anchor = gfx::ScreenRectFromNSRect( gfx::Rect(AnchorForWindow(parent), gfx::Size()).origin(); Or.. possibly gfx::ScreenPointFromNSPoint(..). Maybe we've dodged around needing that long enough and we should just add it. `AnchorForWindow()` is [WebsiteSettingsBubbleController anchorPointForWindowWithHeight:]. It has a `bubbleHeight` argument, but it's unused and should be removed. It also doesn't need to be an instance method. And, it's missing a prototype. So, lots of things to cleanup -- it should just be an anonymous function. https://codereview.chromium.org/1280673003/diff/1/chrome/browser/ui/cocoa/bro... chrome/browser/ui/cocoa/browser_window_cocoa.mm:709: NSMaxY([[window() screen] frame]) - bubble_point_screen.y); I don't think this is right -- flipping always needs to happen in screen[0]. https://codereview.chromium.org/1280673003/diff/1/chrome/browser/ui/cocoa/bro... chrome/browser/ui/cocoa/browser_window_cocoa.mm:712: bubble_point_views, profile, web_contents, url, ssl, browser_); I think the browser_ argument can be refactored out... it seems only used for browser_->OpenURL(..) and the webcontents could do this just as capably, like it does on Mac... https://codereview.chromium.org/1280673003/diff/1/chrome/browser/ui/views/web... File chrome/browser/ui/views/website_settings/website_settings_popup_view.cc (right): https://codereview.chromium.org/1280673003/diff/1/chrome/browser/ui/views/web... chrome/browser/ui/views/website_settings/website_settings_popup_view.cc:833: void ShowWebsiteSettingsBubbleViews(views::View* anchor_view, since it's called via a virtual method, this can be #ifdef mac to be clear it's not used elsewhere. In fact, even nicer would be to add chrome/browser/ui/views/browser_dialogs_views_mac.cc oooh - then I think you can even merge this and ShowWebsiteSettingsBubbleViewsAtPoint into one function
oh, and it's possible a refactoring CL will need to land before this.. Maybe we can remove the ShowWebsiteSettings() virtual method altogether so that it's more consistent with other browser dialogs. glob knows BrowserWindow could do with some simplification..
> oh, and it's possible a refactoring CL will need to land before this.. Maybe we > can remove the ShowWebsiteSettings() virtual method altogether so that it's more > consistent with other browser dialogs. glob knows BrowserWindow could do with > some simplification.. So there's https://codereview.chromium.org/1268243003/. Not sure what you mean by remove ShowWebsiteSettings(), it looks pretty consistent with the other Show*Bubble() in BrowserView. Looks like typically BrowserView supplies a pointer to ToolbarView or similar so the bubble knows where to anchor. https://codereview.chromium.org/1280673003/diff/1/chrome/browser/ui/browser_d... File chrome/browser/ui/browser_dialogs.h (right): https://codereview.chromium.org/1280673003/diff/1/chrome/browser/ui/browser_d... chrome/browser/ui/browser_dialogs.h:16: class GURL; On 2015/08/10 05:44:23, tapted wrote: > nit: sort Done. https://codereview.chromium.org/1280673003/diff/1/chrome/browser/ui/browser_d... chrome/browser/ui/browser_dialogs.h:43: class View; On 2015/08/10 05:44:23, tapted wrote: > this feels out of place here.... Maybe there's a nice fix.. Done. https://codereview.chromium.org/1280673003/diff/1/chrome/browser/ui/browser_d... File chrome/browser/ui/browser_dialogs_mac.cc (right): https://codereview.chromium.org/1280673003/diff/1/chrome/browser/ui/browser_d... chrome/browser/ui/browser_dialogs_mac.cc:10: #include "ui/views/view.h" On 2015/08/10 05:44:23, tapted wrote: > this include shouldn't be allowed here, but I think DEPS misses it on a > technicality (suggested fix follows) Done. https://codereview.chromium.org/1280673003/diff/1/chrome/browser/ui/browser_d... chrome/browser/ui/browser_dialogs_mac.cc:34: views::View* anchor_view = new views::View(); On 2015/08/10 05:44:23, tapted wrote: > memory leak? > > This is going to be the hardest thing to fix. > > Maybe we just inherit from WebsiteSettingsPopupView that deletes the view jammed > into the parent Widget when the popup is torn down (should be as easy as `delete > GetAnchorView();` -- I think the ViewStorage in BubbleDelegateView will return > null if the parent hierarcy somehow gets destroyed first). > > Then roll a custom WebsiteSettingsPopupView::ShowPopup() for Mac -- there's not > much logic there and might help further decouple things. We'd have to make InternalPageInfoPopupView public, which might not be that bad. But I think we can just delete the anchor view when the bubble's widget goes away. https://codereview.chromium.org/1280673003/diff/1/chrome/browser/ui/cocoa/bro... File chrome/browser/ui/cocoa/browser_window_cocoa.mm (right): https://codereview.chromium.org/1280673003/diff/1/chrome/browser/ui/cocoa/bro... chrome/browser/ui/cocoa/browser_window_cocoa.mm:704: LocationBarViewMac* location_bar = [controller_ locationBarBridge]; On 2015/08/10 05:44:23, tapted wrote: > I think this needs to move to website_settings_bubble_controller.mm. In > WebsiteSettingsUIBridge::Show() Then, the logic is just > > gfx::Point anchor = gfx::ScreenRectFromNSRect( > gfx::Rect(AnchorForWindow(parent), gfx::Size()).origin(); > > Or.. possibly gfx::ScreenPointFromNSPoint(..). Maybe we've dodged around needing > that long enough and we should just add it. > > `AnchorForWindow()` is [WebsiteSettingsBubbleController > anchorPointForWindowWithHeight:]. It has a `bubbleHeight` argument, but it's > unused and should be removed. It also doesn't need to be an instance method. > And, it's missing a prototype. So, lots of things to cleanup -- it should just > be an anonymous function. Done. https://codereview.chromium.org/1280673003/diff/1/chrome/browser/ui/cocoa/bro... chrome/browser/ui/cocoa/browser_window_cocoa.mm:709: NSMaxY([[window() screen] frame]) - bubble_point_screen.y); On 2015/08/10 05:44:24, tapted wrote: > I don't think this is right -- flipping always needs to happen in screen[0]. Done. https://codereview.chromium.org/1280673003/diff/1/chrome/browser/ui/cocoa/bro... chrome/browser/ui/cocoa/browser_window_cocoa.mm:712: bubble_point_views, profile, web_contents, url, ssl, browser_); On 2015/08/10 05:44:23, tapted wrote: > I think the browser_ argument can be refactored out... it seems only used for > browser_->OpenURL(..) and the webcontents could do this just as capably, like it > does on Mac... Done in https://codereview.chromium.org/1268243003/. https://codereview.chromium.org/1280673003/diff/1/chrome/browser/ui/views/web... File chrome/browser/ui/views/website_settings/website_settings_popup_view.cc (right): https://codereview.chromium.org/1280673003/diff/1/chrome/browser/ui/views/web... chrome/browser/ui/views/website_settings/website_settings_popup_view.cc:833: void ShowWebsiteSettingsBubbleViews(views::View* anchor_view, On 2015/08/10 05:44:24, tapted wrote: > since it's called via a virtual method, this can be #ifdef mac to be clear it's > not used elsewhere. > > In fact, even nicer would be to add > > chrome/browser/ui/views/browser_dialogs_views_mac.cc > > oooh - then I think you can even merge this and > ShowWebsiteSettingsBubbleViewsAtPoint into one function Done.
On 2015/08/10 09:11:09, jackhou1 wrote: > > oh, and it's possible a refactoring CL will need to land before this.. Maybe > we > > can remove the ShowWebsiteSettings() virtual method altogether so that it's > more > > consistent with other browser dialogs. glob knows BrowserWindow could do with > > some simplification.. > > So there's https://codereview.chromium.org/1268243003/. Not sure what you mean > by remove ShowWebsiteSettings(), it looks pretty consistent with the other > Show*Bubble() in BrowserView. Looks like typically BrowserView supplies a > pointer to ToolbarView or similar so the bubble knows where to anchor. Ah, by this, I meant completely kill the BrowserWindow::ShowWebsiteSettings virtual method. It looks like there's one caller in browser_commands.cc which just fowards arguments. BrowserWindowCocoa also just forwards arguments so is kinda pointless. But BrowserWindowView's implementation is a bit more involved, so this might actually be a bit tricky :/
The coordinate_conversion stuff probably warrants its own CL since it needs a few cleanups too.. Then we should get input from msw about the anchor view thing too, since he's the resident bubble expert. https://codereview.chromium.org/1280673003/diff/60001/chrome/browser/ui/cocoa... File chrome/browser/ui/cocoa/website_settings/website_settings_bubble_controller.mm (right): https://codereview.chromium.org/1280673003/diff/60001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/website_settings/website_settings_bubble_controller.mm:786: [self setAnchorPoint:anchorPoint]; nit: might as well just call AnchorPointForWindow here and omit the anchorPoint temporary https://codereview.chromium.org/1280673003/diff/60001/chrome/browser/ui/views... File chrome/browser/ui/views/browser_dialogs_views_mac.cc (right): https://codereview.chromium.org/1280673003/diff/60001/chrome/browser/ui/views... chrome/browser/ui/views/browser_dialogs_views_mac.cc:18: // anchor view does not have it's own Widget for the BubbleDelegateView to nit: it's -> its https://codereview.chromium.org/1280673003/diff/60001/chrome/browser/ui/views... chrome/browser/ui/views/browser_dialogs_views_mac.cc:19: // watch. watch -> observe? https://codereview.chromium.org/1280673003/diff/60001/chrome/browser/ui/views... chrome/browser/ui/views/browser_dialogs_views_mac.cc:36: widget_ = nullptr; I think this needs to call `delete this`, and the anchor_view_ be stored in a scoped_ptr. `delete this` will also stop observing the webcontents, so no need to set widget_ = null (or even have the data member). https://codereview.chromium.org/1280673003/diff/60001/chrome/browser/ui/views... chrome/browser/ui/views/browser_dialogs_views_mac.cc:41: void WebContentsDestroyed() override { Should any navigation do this too? Maybe that's not interesting for bubbles that always dismiss themselves. https://codereview.chromium.org/1280673003/diff/60001/chrome/browser/ui/views... chrome/browser/ui/views/browser_dialogs_views_mac.cc:47: views::View* anchor_view_; when this becomes scoped_ptr, needs a note to say that it's never inserted into a view hierarchy https://codereview.chromium.org/1280673003/diff/60001/chrome/browser/ui/views... chrome/browser/ui/views/browser_dialogs_views_mac.cc:64: // View::ConvertPointToScreen will assume it's origin is 0,0. Create a rect it's -> its https://codereview.chromium.org/1280673003/diff/60001/chrome/browser/ui/views... chrome/browser/ui/views/browser_dialogs_views_mac.cc:68: // WebsiteSettingsPopupView::kLocationIconVerticalMargin. This needs to be pulled out into a header file somewhere. (and it seems possible the LocationIconVerticalMargin in website_settings_popup_view.cc should really be referring to a constant used when creating the omnibox rahter than being redefined in the anonymous namespace therE) https://codereview.chromium.org/1280673003/diff/60001/chrome/browser/ui/views... chrome/browser/ui/views/browser_dialogs_views_mac.cc:71: views::View* anchor_view = new views::View(); I think it would be nicer to do this directly into a scoped_ptr owned by BubbleAnchorViewDeleter. Maybe a better name then is BubbleAnchorViewAdapter. (i.e. this function becomes merely // BubbleAnchorViewAdapter deletes itself. new BubbleAnchorViewAdapater(anchor_point, profile, web_contents, url, ssl); https://codereview.chromium.org/1280673003/diff/60001/chrome/browser/ui/views... chrome/browser/ui/views/browser_dialogs_views_mac.cc:72: anchor_view->SetBoundsRect(anchor_view_rect); Maybe just SetBounds(0, 0, anchor_point.x() * 2, anchor_point.y() + WebsiteSettingsPopupView::kLocationIconVerticalMargin); https://codereview.chromium.org/1280673003/diff/60001/chrome/browser/ui/views... chrome/browser/ui/views/browser_dialogs_views_mac.cc:75: new BubbleAnchorViewDeleter(anchor_view, bubble_view->GetWidget(), (I think this is leaked now, but `delete this` should fix it..) https://codereview.chromium.org/1280673003/diff/60001/ui/gfx/mac/coordinate_c... File ui/gfx/mac/coordinate_conversion.h (right): https://codereview.chromium.org/1280673003/diff/60001/ui/gfx/mac/coordinate_c... ui/gfx/mac/coordinate_conversion.h:27: GFX_EXPORT NSPoint ScreenPointToNSPoint(const gfx::Point& point); hm - these are all in namespace gfx:: already - can you remove the ones above too? https://codereview.chromium.org/1280673003/diff/60001/ui/gfx/mac/coordinate_c... ui/gfx/mac/coordinate_conversion.h:29: // Convert an AppKit Point with origin in the bottom left of the primary nit Point -> NSPoint https://codereview.chromium.org/1280673003/diff/60001/ui/gfx/mac/coordinate_c... ui/gfx/mac/coordinate_conversion.h:30: // display into a gfx::Rect with origin at the top left of the primary display. gfx::Rect -> gfx::Point https://codereview.chromium.org/1280673003/diff/60001/ui/gfx/mac/coordinate_c... File ui/gfx/mac/coordinate_conversion_unittest.mm (right): https://codereview.chromium.org/1280673003/diff/60001/ui/gfx/mac/coordinate_c... ui/gfx/mac/coordinate_conversion_unittest.mm:105: EXPECT_EQ(gfx_rect.ToString(), ScreenRectFromNSRect(ns_rect).ToString()); hm - these ToString() calls are probably unnecessary too https://codereview.chromium.org/1280673003/diff/60001/ui/gfx/mac/coordinate_c... ui/gfx/mac/coordinate_conversion_unittest.mm:108: TEST_F(MacCoordinateConversionTest, ScreenPointToFromNSPoint) { nit: add a boring comment https://codereview.chromium.org/1280673003/diff/60001/ui/gfx/mac/coordinate_c... ui/gfx/mac/coordinate_conversion_unittest.mm:114: // Window in a screen to the left of the primary screen. Window -> Point (or Coordinate). More below
https://codereview.chromium.org/1280673003/diff/60001/chrome/browser/ui/cocoa... File chrome/browser/ui/cocoa/website_settings/website_settings_bubble_controller.mm (right): https://codereview.chromium.org/1280673003/diff/60001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/website_settings/website_settings_bubble_controller.mm:786: [self setAnchorPoint:anchorPoint]; On 2015/08/11 01:23:19, tapted wrote: > nit: might as well just call AnchorPointForWindow here and omit the anchorPoint > temporary Done. https://codereview.chromium.org/1280673003/diff/60001/chrome/browser/ui/views... File chrome/browser/ui/views/browser_dialogs_views_mac.cc (right): https://codereview.chromium.org/1280673003/diff/60001/chrome/browser/ui/views... chrome/browser/ui/views/browser_dialogs_views_mac.cc:18: // anchor view does not have it's own Widget for the BubbleDelegateView to On 2015/08/11 01:23:19, tapted wrote: > nit: it's -> its Done. https://codereview.chromium.org/1280673003/diff/60001/chrome/browser/ui/views... chrome/browser/ui/views/browser_dialogs_views_mac.cc:19: // watch. On 2015/08/11 01:23:19, tapted wrote: > watch -> observe? Done. https://codereview.chromium.org/1280673003/diff/60001/chrome/browser/ui/views... chrome/browser/ui/views/browser_dialogs_views_mac.cc:36: widget_ = nullptr; On 2015/08/11 01:23:19, tapted wrote: > I think this needs to call `delete this`, and the anchor_view_ be stored in a > scoped_ptr. `delete this` will also stop observing the webcontents, so no need > to set widget_ = null (or even have the data member). Done. Still need |widget_| to call Widget::Close(). https://codereview.chromium.org/1280673003/diff/60001/chrome/browser/ui/views... chrome/browser/ui/views/browser_dialogs_views_mac.cc:41: void WebContentsDestroyed() override { On 2015/08/11 01:23:19, tapted wrote: > Should any navigation do this too? Maybe that's not interesting for bubbles that > always dismiss themselves. I think that would take focus away, which closes the bubble. Closing the parent window doesn't focus it first. https://codereview.chromium.org/1280673003/diff/60001/chrome/browser/ui/views... chrome/browser/ui/views/browser_dialogs_views_mac.cc:47: views::View* anchor_view_; On 2015/08/11 01:23:19, tapted wrote: > when this becomes scoped_ptr, needs a note to say that it's never inserted into > a view hierarchy Done. https://codereview.chromium.org/1280673003/diff/60001/chrome/browser/ui/views... chrome/browser/ui/views/browser_dialogs_views_mac.cc:64: // View::ConvertPointToScreen will assume it's origin is 0,0. Create a rect On 2015/08/11 01:23:19, tapted wrote: > it's -> its Done. https://codereview.chromium.org/1280673003/diff/60001/chrome/browser/ui/views... chrome/browser/ui/views/browser_dialogs_views_mac.cc:68: // WebsiteSettingsPopupView::kLocationIconVerticalMargin. On 2015/08/11 01:23:19, tapted wrote: > This needs to be pulled out into a header file somewhere. (and it seems possible > the LocationIconVerticalMargin in website_settings_popup_view.cc should really > be referring to a constant used when creating the omnibox rahter than being > redefined in the anonymous namespace therE) The margin is built into the assets. kLocationIconVerticalMargin is the only definition I could find. zoom_button_view.cc hard codes it. So I've just make kLocationIconVerticalMargin public. https://codereview.chromium.org/1280673003/diff/60001/chrome/browser/ui/views... chrome/browser/ui/views/browser_dialogs_views_mac.cc:71: views::View* anchor_view = new views::View(); On 2015/08/11 01:23:19, tapted wrote: > I think it would be nicer to do this directly into a scoped_ptr owned by > BubbleAnchorViewDeleter. > > Maybe a better name then is BubbleAnchorViewAdapter. (i.e. this function becomes > merely > > // BubbleAnchorViewAdapter deletes itself. > new BubbleAnchorViewAdapater(anchor_point, profile, web_contents, url, ssl); Done. https://codereview.chromium.org/1280673003/diff/60001/chrome/browser/ui/views... chrome/browser/ui/views/browser_dialogs_views_mac.cc:72: anchor_view->SetBoundsRect(anchor_view_rect); On 2015/08/11 01:23:19, tapted wrote: > Maybe just > > SetBounds(0, 0, anchor_point.x() * 2, anchor_point.y() + > WebsiteSettingsPopupView::kLocationIconVerticalMargin); Done. https://codereview.chromium.org/1280673003/diff/60001/chrome/browser/ui/views... chrome/browser/ui/views/browser_dialogs_views_mac.cc:75: new BubbleAnchorViewDeleter(anchor_view, bubble_view->GetWidget(), On 2015/08/11 01:23:19, tapted wrote: > (I think this is leaked now, but `delete this` should fix it..) Done. https://codereview.chromium.org/1280673003/diff/60001/ui/gfx/mac/coordinate_c... File ui/gfx/mac/coordinate_conversion.h (right): https://codereview.chromium.org/1280673003/diff/60001/ui/gfx/mac/coordinate_c... ui/gfx/mac/coordinate_conversion.h:27: GFX_EXPORT NSPoint ScreenPointToNSPoint(const gfx::Point& point); On 2015/08/11 01:23:19, tapted wrote: > hm - these are all in namespace gfx:: already - can you remove the ones above > too? Done. https://codereview.chromium.org/1280673003/diff/60001/ui/gfx/mac/coordinate_c... ui/gfx/mac/coordinate_conversion.h:29: // Convert an AppKit Point with origin in the bottom left of the primary On 2015/08/11 01:23:19, tapted wrote: > nit Point -> NSPoint Done. https://codereview.chromium.org/1280673003/diff/60001/ui/gfx/mac/coordinate_c... ui/gfx/mac/coordinate_conversion.h:30: // display into a gfx::Rect with origin at the top left of the primary display. On 2015/08/11 01:23:19, tapted wrote: > gfx::Rect -> gfx::Point Done. https://codereview.chromium.org/1280673003/diff/60001/ui/gfx/mac/coordinate_c... File ui/gfx/mac/coordinate_conversion_unittest.mm (right): https://codereview.chromium.org/1280673003/diff/60001/ui/gfx/mac/coordinate_c... ui/gfx/mac/coordinate_conversion_unittest.mm:105: EXPECT_EQ(gfx_rect.ToString(), ScreenRectFromNSRect(ns_rect).ToString()); On 2015/08/11 01:23:20, tapted wrote: > hm - these ToString() calls are probably unnecessary too Done. https://codereview.chromium.org/1280673003/diff/60001/ui/gfx/mac/coordinate_c... ui/gfx/mac/coordinate_conversion_unittest.mm:108: TEST_F(MacCoordinateConversionTest, ScreenPointToFromNSPoint) { On 2015/08/11 01:23:19, tapted wrote: > nit: add a boring comment Done. https://codereview.chromium.org/1280673003/diff/60001/ui/gfx/mac/coordinate_c... ui/gfx/mac/coordinate_conversion_unittest.mm:114: // Window in a screen to the left of the primary screen. On 2015/08/11 01:23:19, tapted wrote: > Window -> Point (or Coordinate). More below Done.
sorry.. found another spanner to throw :( https://codereview.chromium.org/1280673003/diff/80001/chrome/browser/ui/views... File chrome/browser/ui/views/browser_dialogs_views_mac.cc (right): https://codereview.chromium.org/1280673003/diff/80001/chrome/browser/ui/views... chrome/browser/ui/views/browser_dialogs_views_mac.cc:33: 0, 0, anchor_point.x() * 2, ooh - I just realised something... What happens in a dual-screen setup, and it shows on a screen to the left of the menubar. I think anchor_point.x will be negative in this case. https://codereview.chromium.org/1280673003/diff/80001/chrome/browser/ui/views... File chrome/browser/ui/views/website_settings/website_settings_popup_view.h (right): https://codereview.chromium.org/1280673003/diff/80001/chrome/browser/ui/views... chrome/browser/ui/views/website_settings/website_settings_popup_view.h:45: // at the Omnibox's edge, the bubble's anchor rect will be insetted by this nit: insetted -> inset is right I think. But I think your discovery is the more pertinent comment to make. e.g. // Margin built-in to the icon asset in the anchor view. Used to ensure the // bubble arrow points to the actual icon.
https://codereview.chromium.org/1280673003/diff/80001/chrome/browser/ui/views... File chrome/browser/ui/views/browser_dialogs_views_mac.cc (right): https://codereview.chromium.org/1280673003/diff/80001/chrome/browser/ui/views... chrome/browser/ui/views/browser_dialogs_views_mac.cc:33: 0, 0, anchor_point.x() * 2, On 2015/08/11 03:20:37, tapted wrote: > ooh - I just realised something... What happens in a dual-screen setup, and it > shows on a screen to the left of the menubar. I think anchor_point.x will be > negative in this case. lgtm with a comment about the dual-screen thing (here and in the CL description), and a TODO here. Maybe msw has some ideas for an alternative fix
https://codereview.chromium.org/1280673003/diff/80001/chrome/browser/ui/views... File chrome/browser/ui/views/browser_dialogs_views_mac.cc (right): https://codereview.chromium.org/1280673003/diff/80001/chrome/browser/ui/views... chrome/browser/ui/views/browser_dialogs_views_mac.cc:33: 0, 0, anchor_point.x() * 2, On 2015/08/11 03:45:42, tapted wrote: > On 2015/08/11 03:20:37, tapted wrote: > > ooh - I just realised something... What happens in a dual-screen setup, and it > > shows on a screen to the left of the menubar. I think anchor_point.x will be > > negative in this case. > > lgtm with a comment about the dual-screen thing (here and in the CL > description), and a TODO here. > > Maybe msw has some ideas for an alternative fix Done. https://codereview.chromium.org/1280673003/diff/80001/chrome/browser/ui/views... File chrome/browser/ui/views/website_settings/website_settings_popup_view.h (right): https://codereview.chromium.org/1280673003/diff/80001/chrome/browser/ui/views... chrome/browser/ui/views/website_settings/website_settings_popup_view.h:45: // at the Omnibox's edge, the bubble's anchor rect will be insetted by this On 2015/08/11 03:20:38, tapted wrote: > nit: insetted -> inset is right I think. > > But I think your discovery is the more pertinent comment to make. e.g. > > // Margin built-in to the icon asset in the anchor view. Used to ensure the > // bubble arrow points to the actual icon. Done.
jackhou@chromium.org changed reviewers: + msw@chromium.org
msw, please review for OWNERS Also, you may have a better suggestion for how to provide an anchor view to WebsiteSettingsPopupView.
https://codereview.chromium.org/1280673003/diff/120001/chrome/browser/ui/view... File chrome/browser/ui/views/browser_dialogs_views_mac.cc (right): https://codereview.chromium.org/1280673003/diff/120001/chrome/browser/ui/view... chrome/browser/ui/views/browser_dialogs_views_mac.cc:20: class BubbleAnchorViewAdapter : public views::WidgetObserver, Yeah, this seems far from optimal... (1) Using a point instead of a rect will cause the bubble to overlap the button/view it's anchored to, if the bubble needs to mirror itself to fit on screen. (2) This seems more heavy-handed than necessary. Can you instead just use BubbleDelegateView::SetAnchorRect()? https://codereview.chromium.org/1280673003/diff/120001/chrome/browser/ui/view... File chrome/browser/ui/views/website_settings/website_settings_popup_view.cc (right): https://codereview.chromium.org/1280673003/diff/120001/chrome/browser/ui/view... chrome/browser/ui/views/website_settings/website_settings_popup_view.cc:284: } else { nit: no else after return
Just a question. (All of Sydney eng is at an offsite so I won't be able to update the code until we get back). https://codereview.chromium.org/1280673003/diff/120001/chrome/browser/ui/view... File chrome/browser/ui/views/browser_dialogs_views_mac.cc (right): https://codereview.chromium.org/1280673003/diff/120001/chrome/browser/ui/view... chrome/browser/ui/views/browser_dialogs_views_mac.cc:20: class BubbleAnchorViewAdapter : public views::WidgetObserver, On 2015/08/11 22:09:11, msw wrote: > Yeah, this seems far from optimal... (1) Using a point instead of a rect will > cause the bubble to overlap the button/view it's anchored to, if the bubble > needs to mirror itself to fit on screen. (2) This seems more heavy-handed than > necessary. Can you instead just use BubbleDelegateView::SetAnchorRect()? SetAnchorRect would be good. Do you think it's okay to just make it public? Otherwise we could change BubbleDelegateView::GetAnchorRect to check whether there's a Widget and, if not, use View::bounds() instead of GetBoundsInScreen since the latter ignores the origin if there's no Widget. Another option is https://codereview.chromium.org/1279023002/ but that may have wider implications.
https://codereview.chromium.org/1280673003/diff/120001/chrome/browser/ui/view... File chrome/browser/ui/views/browser_dialogs_views_mac.cc (right): https://codereview.chromium.org/1280673003/diff/120001/chrome/browser/ui/view... chrome/browser/ui/views/browser_dialogs_views_mac.cc:20: class BubbleAnchorViewAdapter : public views::WidgetObserver, On 2015/08/12 06:02:11, jackhou1 wrote: > On 2015/08/11 22:09:11, msw wrote: > > Yeah, this seems far from optimal... (1) Using a point instead of a rect will > > cause the bubble to overlap the button/view it's anchored to, if the bubble > > needs to mirror itself to fit on screen. (2) This seems more heavy-handed than > > necessary. Can you instead just use BubbleDelegateView::SetAnchorRect()? > > SetAnchorRect would be good. Do you think it's okay to just make it public? > Otherwise we could change BubbleDelegateView::GetAnchorRect to check whether > there's a Widget and, if not, use View::bounds() instead of GetBoundsInScreen > since the latter ignores the origin if there's no Widget. Another option is > https://codereview.chromium.org/1279023002/ but that may have wider > implications. Make a public helper on WebsiteSettingsPopupView instead of exposing BubbleDelegateView::SetAnchorRect (at least for now, unless/until we decide that it's needed in more places). I don't think changing GetAnchorRect as you suggest is any better. I'm dubious about the other CL, but sky@ might approve that with a good justification and set of tests. I suggest setting a custom anchor_rect_ as the simplest approach.
https://codereview.chromium.org/1280673003/diff/120001/chrome/browser/ui/view... File chrome/browser/ui/views/browser_dialogs_views_mac.cc (right): https://codereview.chromium.org/1280673003/diff/120001/chrome/browser/ui/view... chrome/browser/ui/views/browser_dialogs_views_mac.cc:20: class BubbleAnchorViewAdapter : public views::WidgetObserver, On 2015/08/12 16:52:43, msw wrote: > On 2015/08/12 06:02:11, jackhou1 wrote: > > On 2015/08/11 22:09:11, msw wrote: > > > Yeah, this seems far from optimal... (1) Using a point instead of a rect > will > > > cause the bubble to overlap the button/view it's anchored to, if the bubble > > > needs to mirror itself to fit on screen. (2) This seems more heavy-handed > than > > > necessary. Can you instead just use BubbleDelegateView::SetAnchorRect()? > > > > SetAnchorRect would be good. Do you think it's okay to just make it public? > > Otherwise we could change BubbleDelegateView::GetAnchorRect to check whether > > there's a Widget and, if not, use View::bounds() instead of GetBoundsInScreen > > since the latter ignores the origin if there's no Widget. Another option is > > https://codereview.chromium.org/1279023002/ but that may have wider > > implications. > > Make a public helper on WebsiteSettingsPopupView instead of exposing > BubbleDelegateView::SetAnchorRect (at least for now, unless/until we decide that > it's needed in more places). I don't think changing GetAnchorRect as you suggest > is any better. I'm dubious about the other CL, but sky@ might approve that with > a good justification and set of tests. I suggest setting a custom anchor_rect_ > as the simplest approach. Done. Added a static helper WebsiteSettingsPopupView::ShowPopupAtRect() to avoid exposing InternalPageInfoPopupView. It turns out on Cocoa, the bubble is never mirrored, it is always below and to the right. Seems simplest to keep this the same for MacViews as we won't need to get the rect of the location icon. Plus it's more consistent with Cocoa. https://codereview.chromium.org/1280673003/diff/120001/chrome/browser/ui/view... File chrome/browser/ui/views/website_settings/website_settings_popup_view.cc (right): https://codereview.chromium.org/1280673003/diff/120001/chrome/browser/ui/view... chrome/browser/ui/views/website_settings/website_settings_popup_view.cc:284: } else { On 2015/08/11 22:09:11, msw wrote: > nit: no else after return Done.
https://codereview.chromium.org/1280673003/diff/160001/chrome/browser/ui/view... File chrome/browser/ui/views/browser_dialogs_views_mac.cc (right): https://codereview.chromium.org/1280673003/diff/160001/chrome/browser/ui/view... chrome/browser/ui/views/browser_dialogs_views_mac.cc:21: class BubbleAnchorAdapter : public views::WidgetObserver, (as discussed, I think we can get rid of the adapter with a call to BubbleDelegateView::set_parent_view before Widget::Init gets called -- NativeWidgetMac has machinery to observe its parent being closed. But... https://codereview.chromium.org/1280673003/diff/160001/chrome/browser/ui/view... File chrome/browser/ui/views/website_settings/website_settings_popup_view.cc (right): https://codereview.chromium.org/1280673003/diff/160001/chrome/browser/ui/view... chrome/browser/ui/views/website_settings/website_settings_popup_view.cc:813: GetAnchorView()->GetWidget()->GetNativeWindow() : nullptr; ... But stuff like this might need an update to substitute: nullptr -> parent_window(). That's probably good for a follow-up though once we know it's a problem. (particularly since parent_window() is a gfx::NativeView and this wants to assign into a gfx::NativeWindow)
https://codereview.chromium.org/1280673003/diff/160001/chrome/browser/ui/view... File chrome/browser/ui/views/browser_dialogs_views_mac.cc (right): https://codereview.chromium.org/1280673003/diff/160001/chrome/browser/ui/view... chrome/browser/ui/views/browser_dialogs_views_mac.cc:21: class BubbleAnchorAdapter : public views::WidgetObserver, On 2015/08/17 05:15:03, tapted wrote: > BubbleDelegateView::set_parent_view before Widget::Init gets called -- uhh set_parent_window(..) that is.
https://codereview.chromium.org/1280673003/diff/160001/chrome/browser/ui/view... File chrome/browser/ui/views/browser_dialogs_views_mac.cc (right): https://codereview.chromium.org/1280673003/diff/160001/chrome/browser/ui/view... chrome/browser/ui/views/browser_dialogs_views_mac.cc:21: class BubbleAnchorAdapter : public views::WidgetObserver, On 2015/08/17 05:15:45, tapted wrote: > On 2015/08/17 05:15:03, tapted wrote: > > BubbleDelegateView::set_parent_view before Widget::Init gets called -- > > uhh set_parent_window(..) that is. Done. https://codereview.chromium.org/1280673003/diff/160001/chrome/browser/ui/view... File chrome/browser/ui/views/website_settings/website_settings_popup_view.cc (right): https://codereview.chromium.org/1280673003/diff/160001/chrome/browser/ui/view... chrome/browser/ui/views/website_settings/website_settings_popup_view.cc:813: GetAnchorView()->GetWidget()->GetNativeWindow() : nullptr; On 2015/08/17 05:15:03, tapted wrote: > ... But stuff like this might need an update to substitute: nullptr -> > parent_window(). That's probably good for a follow-up though once we know it's a > problem. > > (particularly since parent_window() is a gfx::NativeView and this wants to > assign into a gfx::NativeWindow) Acknowledged.
https://codereview.chromium.org/1280673003/diff/180001/chrome/browser/ui/view... File chrome/browser/ui/views/website_settings/website_settings_popup_view.cc (right): https://codereview.chromium.org/1280673003/diff/180001/chrome/browser/ui/view... chrome/browser/ui/views/website_settings/website_settings_popup_view.cc:144: InternalPageInfoPopupView(views::View* anchor_view, nit: Maybe a comment here. E.g. it's not obvious that either anchor_view or parent_window is expected to be null. https://codereview.chromium.org/1280673003/diff/180001/chrome/browser/ui/view... chrome/browser/ui/views/website_settings/website_settings_popup_view.cc:295: void WebsiteSettingsPopupView::ShowPopupAtRect( I *think* a helper function in an anonymous namespace would be an improvement. The benefit is borderline, but duplicated logic usually has bigger downsides for maintainability. I'm thinking something like the following, but msw may have other ideas :) views::BubbleDelegateView* CreatePopup( views::View* anchor_view, Profile* profile, content::WebContents* web_contents, const GURL& url, const content::SSLStatus& ssl) { gfx::NativeWindow parent = anchor_view ? nullptr : web_contents->GetNativeView(); if (InternalChromePage(url)) return new InternalPageInfoPopupView(anchor_view, parent); return new WebsiteSettingsPopupView( anchor_view, parent, profile, web_contents, url, ssl); } ShowPopup(...) { is_popup_showing = true; CreatePopup(...)->GetWidget()->Show(); } ShowPopupAtRect(...) { is_popup_showing = true; InternalPageInfoPopupView* popup = CreatePopup(...); popup->SetAnchorRect(anchor_rect); popup->GetWidget()->Show(); }
https://codereview.chromium.org/1280673003/diff/180001/chrome/browser/ui/view... File chrome/browser/ui/views/website_settings/website_settings_popup_view.cc (right): https://codereview.chromium.org/1280673003/diff/180001/chrome/browser/ui/view... chrome/browser/ui/views/website_settings/website_settings_popup_view.cc:295: void WebsiteSettingsPopupView::ShowPopupAtRect( On 2015/08/17 06:00:49, tapted wrote: > I *think* a helper function in an anonymous namespace would be an improvement. > The benefit is borderline, but duplicated logic usually has bigger downsides for > maintainability. I'm thinking something like the following, but msw may have > other ideas :) > > views::BubbleDelegateView* CreatePopup( > views::View* anchor_view, > Profile* profile, > content::WebContents* web_contents, > const GURL& url, > const content::SSLStatus& ssl) { > gfx::NativeWindow parent = anchor_view ? > nullptr : web_contents->GetNativeView(); > if (InternalChromePage(url)) > return new InternalPageInfoPopupView(anchor_view, parent); > return new WebsiteSettingsPopupView( > anchor_view, parent, profile, web_contents, url, ssl); > } > > ShowPopup(...) { > is_popup_showing = true; > CreatePopup(...)->GetWidget()->Show(); > } > > ShowPopupAtRect(...) { > is_popup_showing = true; > InternalPageInfoPopupView* popup = CreatePopup(...); > popup->SetAnchorRect(anchor_rect); > popup->GetWidget()->Show(); > } I don't think ShowPopupAtRect would be able to call BubbleDelegateView::SetAnchorRect like this. At the moment, it's calling WebsiteSettingsPopupView::SetAnchorRect, and InternalPageInfoPopupView::SetAnchorRect as a friend.
https://codereview.chromium.org/1280673003/diff/180001/chrome/browser/ui/view... File chrome/browser/ui/views/website_settings/website_settings_popup_view.cc (right): https://codereview.chromium.org/1280673003/diff/180001/chrome/browser/ui/view... chrome/browser/ui/views/website_settings/website_settings_popup_view.cc:295: void WebsiteSettingsPopupView::ShowPopupAtRect( On 2015/08/17 06:19:13, jackhou1 wrote: > On 2015/08/17 06:00:49, tapted wrote: > > I *think* a helper function in an anonymous namespace would be an improvement. > > The benefit is borderline, but duplicated logic usually has bigger downsides > for > > maintainability. I'm thinking something like the following, but msw may have > > other ideas :) > > > > views::BubbleDelegateView* CreatePopup( > > views::View* anchor_view, > > Profile* profile, > > content::WebContents* web_contents, > > const GURL& url, > > const content::SSLStatus& ssl) { > > gfx::NativeWindow parent = anchor_view ? > > nullptr : web_contents->GetNativeView(); > > if (InternalChromePage(url)) > > return new InternalPageInfoPopupView(anchor_view, parent); > > return new WebsiteSettingsPopupView( > > anchor_view, parent, profile, web_contents, url, ssl); > > } > > > > ShowPopup(...) { > > is_popup_showing = true; > > CreatePopup(...)->GetWidget()->Show(); > > } > > > > ShowPopupAtRect(...) { > > is_popup_showing = true; > > InternalPageInfoPopupView* popup = CreatePopup(...); > > popup->SetAnchorRect(anchor_rect); > > popup->GetWidget()->Show(); > > } > > I don't think ShowPopupAtRect would be able to call > BubbleDelegateView::SetAnchorRect like this. At the moment, it's calling > WebsiteSettingsPopupView::SetAnchorRect, and > InternalPageInfoPopupView::SetAnchorRect as a friend. On 2015/08/17 06:19:13, jackhou1 wrote: > https://codereview.chromium.org/1280673003/diff/180001/chrome/browser/ui/view... > File chrome/browser/ui/views/website_settings/website_settings_popup_view.cc > (right): > > https://codereview.chromium.org/1280673003/diff/180001/chrome/browser/ui/view... > chrome/browser/ui/views/website_settings/website_settings_popup_view.cc:295: > void WebsiteSettingsPopupView::ShowPopupAtRect( > On 2015/08/17 06:00:49, tapted wrote: > > I *think* a helper function in an anonymous namespace would be an improvement. > > The benefit is borderline, but duplicated logic usually has bigger downsides > for > > maintainability. I'm thinking something like the following, but msw may have > > other ideas :) > > > > views::BubbleDelegateView* CreatePopup( > > views::View* anchor_view, > > Profile* profile, > > content::WebContents* web_contents, > > const GURL& url, > > const content::SSLStatus& ssl) { > > gfx::NativeWindow parent = anchor_view ? > > nullptr : web_contents->GetNativeView(); > > if (InternalChromePage(url)) > > return new InternalPageInfoPopupView(anchor_view, parent); > > return new WebsiteSettingsPopupView( > > anchor_view, parent, profile, web_contents, url, ssl); > > } > > > > ShowPopup(...) { > > is_popup_showing = true; > > CreatePopup(...)->GetWidget()->Show(); > > } > > > > ShowPopupAtRect(...) { > > is_popup_showing = true; > > InternalPageInfoPopupView* popup = CreatePopup(...); > > popup->SetAnchorRect(anchor_rect); > > popup->GetWidget()->Show(); > > } > > I don't think ShowPopupAtRect would be able to call > BubbleDelegateView::SetAnchorRect like this. At the moment, it's calling > WebsiteSettingsPopupView::SetAnchorRect, and > InternalPageInfoPopupView::SetAnchorRect as a friend. Ah. good point :/. That does make sense of the repetition - maybe just comment above `InternalPageInfoPopupView* popup =..` like // Use the concrete type so that SetAnchorRect can be called as a friend. I guess the alternative is to alter the constructor, but that feels messier.
https://codereview.chromium.org/1280673003/diff/180001/chrome/browser/ui/view... File chrome/browser/ui/views/website_settings/website_settings_popup_view.cc (right): https://codereview.chromium.org/1280673003/diff/180001/chrome/browser/ui/view... chrome/browser/ui/views/website_settings/website_settings_popup_view.cc:295: void WebsiteSettingsPopupView::ShowPopupAtRect( On 2015/08/17 06:30:57, tapted wrote: > On 2015/08/17 06:19:13, jackhou1 wrote: > > On 2015/08/17 06:00:49, tapted wrote: > > > I *think* a helper function in an anonymous namespace would be an > improvement. > > > The benefit is borderline, but duplicated logic usually has bigger downsides > > for > > > maintainability. I'm thinking something like the following, but msw may have > > > other ideas :) > > > > > > views::BubbleDelegateView* CreatePopup( > > > views::View* anchor_view, > > > Profile* profile, > > > content::WebContents* web_contents, > > > const GURL& url, > > > const content::SSLStatus& ssl) { > > > gfx::NativeWindow parent = anchor_view ? > > > nullptr : web_contents->GetNativeView(); > > > if (InternalChromePage(url)) > > > return new InternalPageInfoPopupView(anchor_view, parent); > > > return new WebsiteSettingsPopupView( > > > anchor_view, parent, profile, web_contents, url, ssl); > > > } > > > > > > ShowPopup(...) { > > > is_popup_showing = true; > > > CreatePopup(...)->GetWidget()->Show(); > > > } > > > > > > ShowPopupAtRect(...) { > > > is_popup_showing = true; > > > InternalPageInfoPopupView* popup = CreatePopup(...); > > > popup->SetAnchorRect(anchor_rect); > > > popup->GetWidget()->Show(); > > > } > > > > I don't think ShowPopupAtRect would be able to call > > BubbleDelegateView::SetAnchorRect like this. At the moment, it's calling > > WebsiteSettingsPopupView::SetAnchorRect, and > > InternalPageInfoPopupView::SetAnchorRect as a friend. > > On 2015/08/17 06:19:13, jackhou1 wrote: > > > https://codereview.chromium.org/1280673003/diff/180001/chrome/browser/ui/view... > > File chrome/browser/ui/views/website_settings/website_settings_popup_view.cc > > (right): > > > > > https://codereview.chromium.org/1280673003/diff/180001/chrome/browser/ui/view... > > chrome/browser/ui/views/website_settings/website_settings_popup_view.cc:295: > > void WebsiteSettingsPopupView::ShowPopupAtRect( > > On 2015/08/17 06:00:49, tapted wrote: > > > I *think* a helper function in an anonymous namespace would be an > improvement. > > > The benefit is borderline, but duplicated logic usually has bigger downsides > > for > > > maintainability. I'm thinking something like the following, but msw may have > > > other ideas :) > > > > > > views::BubbleDelegateView* CreatePopup( > > > views::View* anchor_view, > > > Profile* profile, > > > content::WebContents* web_contents, > > > const GURL& url, > > > const content::SSLStatus& ssl) { > > > gfx::NativeWindow parent = anchor_view ? > > > nullptr : web_contents->GetNativeView(); > > > if (InternalChromePage(url)) > > > return new InternalPageInfoPopupView(anchor_view, parent); > > > return new WebsiteSettingsPopupView( > > > anchor_view, parent, profile, web_contents, url, ssl); > > > } > > > > > > ShowPopup(...) { > > > is_popup_showing = true; > > > CreatePopup(...)->GetWidget()->Show(); > > > } > > > > > > ShowPopupAtRect(...) { > > > is_popup_showing = true; > > > InternalPageInfoPopupView* popup = CreatePopup(...); > > > popup->SetAnchorRect(anchor_rect); > > > popup->GetWidget()->Show(); > > > } > > > > I don't think ShowPopupAtRect would be able to call > > BubbleDelegateView::SetAnchorRect like this. At the moment, it's calling > > WebsiteSettingsPopupView::SetAnchorRect, and > > InternalPageInfoPopupView::SetAnchorRect as a friend. > > Ah. good point :/. That does make sense of the repetition - maybe just comment > above `InternalPageInfoPopupView* popup =..` like > // Use the concrete type so that SetAnchorRect can be called as a friend. > > I guess the alternative is to alter the constructor, but that feels messier. Done.
https://codereview.chromium.org/1280673003/diff/180001/chrome/browser/ui/view... File chrome/browser/ui/views/website_settings/website_settings_popup_view.cc (right): https://codereview.chromium.org/1280673003/diff/180001/chrome/browser/ui/view... chrome/browser/ui/views/website_settings/website_settings_popup_view.cc:144: InternalPageInfoPopupView(views::View* anchor_view, On 2015/08/17 06:00:49, tapted wrote: > nit: Maybe a comment here. E.g. it's not obvious that either anchor_view or > parent_window is expected to be null. Done.
lgtm
msw, PTAL
Can you post screenshots of the bubble and the collected cookies dialog (Cocoa vs. Views) to the bug? lgtm with minor comments and qs. https://codereview.chromium.org/1280673003/diff/220001/chrome/browser/ui/view... File chrome/browser/ui/views/browser_dialogs_views_mac.cc (right): https://codereview.chromium.org/1280673003/diff/220001/chrome/browser/ui/view... chrome/browser/ui/views/browser_dialogs_views_mac.cc:8: #include "content/public/browser/web_contents_observer.h" This doesn't seem to be needed, forward declare content::WebContents if that's even needed (already done in the header) https://codereview.chromium.org/1280673003/diff/220001/chrome/browser/ui/view... chrome/browser/ui/views/browser_dialogs_views_mac.cc:9: #include "ui/views/widget/widget_observer.h" This doesn't seem to be needed. https://codereview.chromium.org/1280673003/diff/220001/chrome/browser/ui/view... chrome/browser/ui/views/browser_dialogs_views_mac.cc:11: // This file provides definitions of desktop browser dialog-creation methods for q: Why doesn't this match Trent's pattern from https://codereview.chromium.org/1274083002 https://codereview.chromium.org/1280673003/diff/220001/chrome/browser/ui/view... File chrome/browser/ui/views/website_settings/website_settings_popup_view.cc (left): https://codereview.chromium.org/1280673003/diff/220001/chrome/browser/ui/view... chrome/browser/ui/views/website_settings/website_settings_popup_view.cc:254: SizeToContents(); Is removing these calls okay? (no regressions?) https://codereview.chromium.org/1280673003/diff/220001/chrome/browser/ui/view... File chrome/browser/ui/views/website_settings/website_settings_popup_view.cc (right): https://codereview.chromium.org/1280673003/diff/220001/chrome/browser/ui/view... chrome/browser/ui/views/website_settings/website_settings_popup_view.cc:280: void WebsiteSettingsPopupView::ShowPopup(views::View* anchor_view, nit: maybe merge this into the new function, and let callers pass a nullptr |anchor_view| or a default gfx::Rect |anchor_rect|?
> Can you post screenshots of the bubble and the collected cookies dialog (Cocoa > vs. Views) to the bug? Done. https://codereview.chromium.org/1280673003/diff/220001/chrome/browser/ui/view... File chrome/browser/ui/views/browser_dialogs_views_mac.cc (right): https://codereview.chromium.org/1280673003/diff/220001/chrome/browser/ui/view... chrome/browser/ui/views/browser_dialogs_views_mac.cc:8: #include "content/public/browser/web_contents_observer.h" On 2015/08/17 17:31:44, msw wrote: > This doesn't seem to be needed, forward declare content::WebContents if that's > even needed (already done in the header) Done. https://codereview.chromium.org/1280673003/diff/220001/chrome/browser/ui/view... chrome/browser/ui/views/browser_dialogs_views_mac.cc:9: #include "ui/views/widget/widget_observer.h" On 2015/08/17 17:31:45, msw wrote: > This doesn't seem to be needed. Done. https://codereview.chromium.org/1280673003/diff/220001/chrome/browser/ui/view... chrome/browser/ui/views/browser_dialogs_views_mac.cc:11: // This file provides definitions of desktop browser dialog-creation methods for On 2015/08/17 17:31:44, msw wrote: > q: Why doesn't this match Trent's pattern from > https://codereview.chromium.org/1274083002 Updated this comment to mention how it fits into builds. The difference is: c/b/ui/browser_dialogs_mac.cc: Only on Cocoa builds. c/b/ui/views/browser_dialogs_views.cc: Only on non-Mac and mac_views_browser=1. c/b/ui/views/browser_dialogs_views_mac.cc: On Cocoa builds, and needs to access Views implementations. https://codereview.chromium.org/1280673003/diff/220001/chrome/browser/ui/view... File chrome/browser/ui/views/website_settings/website_settings_popup_view.cc (left): https://codereview.chromium.org/1280673003/diff/220001/chrome/browser/ui/view... chrome/browser/ui/views/website_settings/website_settings_popup_view.cc:254: SizeToContents(); On 2015/08/17 17:31:45, msw wrote: > Is removing these calls okay? (no regressions?) Tests pass. Tried it on my Windows machine, no regressions AFAICT. It seems to be redundant since it's called by BubbleDelegateView::CreateBubble after creating the Widget. https://codereview.chromium.org/1280673003/diff/220001/chrome/browser/ui/view... File chrome/browser/ui/views/website_settings/website_settings_popup_view.cc (right): https://codereview.chromium.org/1280673003/diff/220001/chrome/browser/ui/view... chrome/browser/ui/views/website_settings/website_settings_popup_view.cc:280: void WebsiteSettingsPopupView::ShowPopup(views::View* anchor_view, On 2015/08/17 17:31:45, msw wrote: > nit: maybe merge this into the new function, and let callers pass a nullptr > |anchor_view| or a default gfx::Rect |anchor_rect|? Done. And updated c/b/ui/views/frame/browser_view.cc.
https://codereview.chromium.org/1280673003/diff/220001/chrome/browser/ui/view... File chrome/browser/ui/views/browser_dialogs_views_mac.cc (right): https://codereview.chromium.org/1280673003/diff/220001/chrome/browser/ui/view... chrome/browser/ui/views/browser_dialogs_views_mac.cc:11: // This file provides definitions of desktop browser dialog-creation methods for On 2015/08/18 01:20:53, jackhou1 wrote: > On 2015/08/17 17:31:44, msw wrote: > > q: Why doesn't this match Trent's pattern from > > https://codereview.chromium.org/1274083002 > > Updated this comment to mention how it fits into builds. > > The difference is: > > c/b/ui/browser_dialogs_mac.cc: Only on Cocoa builds. > c/b/ui/views/browser_dialogs_views.cc: Only on non-Mac and mac_views_browser=1. > c/b/ui/views/browser_dialogs_views_mac.cc: On Cocoa builds, and needs to access > Views implementations. Another aspect behind the difference is that ShowWebsiteSettings is called via a vtable on BrowserWindow. So, for WebsiteSettings, *all* platforms have the same definition of the global function to show the dialog, in browser_commands.cc ( https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/ui/... ) We looked into doing the same pattern (i.e. replacing the ShowWebsiteSettings global function with per-platform definitions). On Mac, both this global function, and the BrowserWindow member function just end up forwarding arguments, so it looked promising. However, for Views, some extra arguments from BrowserWindow private members are added by the member function, so it wasn't a neat fit. (more about this in https://codereview.chromium.org/1280673003/#msg6 )
lgtm, thanks https://codereview.chromium.org/1280673003/diff/220001/chrome/browser/ui/view... File chrome/browser/ui/views/browser_dialogs_views_mac.cc (right): https://codereview.chromium.org/1280673003/diff/220001/chrome/browser/ui/view... chrome/browser/ui/views/browser_dialogs_views_mac.cc:11: // This file provides definitions of desktop browser dialog-creation methods for On 2015/08/18 01:29:51, tapted wrote: > On 2015/08/18 01:20:53, jackhou1 wrote: > > On 2015/08/17 17:31:44, msw wrote: > > > q: Why doesn't this match Trent's pattern from > > > https://codereview.chromium.org/1274083002 > > > > Updated this comment to mention how it fits into builds. > > > > The difference is: > > > > c/b/ui/browser_dialogs_mac.cc: Only on Cocoa builds. > > c/b/ui/views/browser_dialogs_views.cc: Only on non-Mac and > mac_views_browser=1. > > c/b/ui/views/browser_dialogs_views_mac.cc: On Cocoa builds, and needs to > access > > Views implementations. > > Another aspect behind the difference is that ShowWebsiteSettings is called via a > vtable on BrowserWindow. So, for WebsiteSettings, *all* platforms have the same > definition of the global function to show the dialog, in browser_commands.cc ( > https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/ui/... > ) > > We looked into doing the same pattern (i.e. replacing the ShowWebsiteSettings > global function with per-platform definitions). On Mac, both this global > function, and the BrowserWindow member function just end up forwarding > arguments, so it looked promising. However, for Views, some extra arguments from > BrowserWindow private members are added by the member function, so it wasn't a > neat fit. > > (more about this in https://codereview.chromium.org/1280673003/#msg6 ) Acknowledged. https://codereview.chromium.org/1280673003/diff/220001/chrome/browser/ui/view... File chrome/browser/ui/views/website_settings/website_settings_popup_view.cc (left): https://codereview.chromium.org/1280673003/diff/220001/chrome/browser/ui/view... chrome/browser/ui/views/website_settings/website_settings_popup_view.cc:254: SizeToContents(); On 2015/08/18 01:20:53, jackhou1 wrote: > On 2015/08/17 17:31:45, msw wrote: > > Is removing these calls okay? (no regressions?) > > Tests pass. Tried it on my Windows machine, no regressions AFAICT. It seems to > be redundant since it's called by BubbleDelegateView::CreateBubble after > creating the Widget. Acknowledged.
The CQ bit was checked by jackhou@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from tapted@chromium.org Link to the patchset: https://codereview.chromium.org/1280673003/#ps240001 (title: "Address comments.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1280673003/240001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1280673003/240001
Message was sent while issue was closed.
Committed patchset #13 (id:240001)
Message was sent while issue was closed.
Patchset 13 (id:??) landed as https://crrev.com/700bafd0eba1167c3313739142e26dc10a46cd09 Cr-Commit-Position: refs/heads/master@{#343823}
Message was sent while issue was closed.
On 2015/08/18 05:24:31, commit-bot: I haz the power wrote: > Patchset 13 (id:??) landed as > https://crrev.com/700bafd0eba1167c3313739142e26dc10a46cd09 > Cr-Commit-Position: refs/heads/master@{#343823} this change causes both: permissions_bubble_view.cc and permission_bubble_cocoa.mmincluded in toolit_views=1, mac_views_browser=0 configuration. Both of them having static scoped_ptr<PermissionBubbleView> PermissionBubbleView::Create implementation. Unfortunately, this doesn't cause linking error on mac. It looks like it should be fixed so that client code: https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/ui/... will use views function with MacViewsDialogs and cocoa function withot them. Is that right?
Message was sent while issue was closed.
On 2015/09/09 16:01:03, eugenebng wrote: > On 2015/08/18 05:24:31, commit-bot: I haz the power wrote: > > Patchset 13 (id:??) landed as > > https://crrev.com/700bafd0eba1167c3313739142e26dc10a46cd09 > > Cr-Commit-Position: refs/heads/master@{#343823} > > this change causes both: > permissions_bubble_view.cc and permission_bubble_cocoa.mmincluded in > toolit_views=1, mac_views_browser=0 configuration. > Both of them having static scoped_ptr<PermissionBubbleView> > PermissionBubbleView::Create implementation. > Unfortunately, this doesn't cause linking error on mac. > It looks like it should be fixed so that client code: > https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/ui/... > will use views function with MacViewsDialogs and cocoa function withot them. Is > that right? Yeah - I think you're right. It looks like Chrome Mac is currently using Cocoa for permission bubbles on Mac regardless of the flag. That was the intention (i.e. we didn't want to port those bubbles with this change - just the site settings). But maybe that's luck :/. I'll investigate further when I'm back next week.
Message was sent while issue was closed.
On 2015/09/10 04:50:28, tapted wrote: > On 2015/09/09 16:01:03, eugenebng wrote: > > On 2015/08/18 05:24:31, commit-bot: I haz the power wrote: > > > Patchset 13 (id:??) landed as > > > https://crrev.com/700bafd0eba1167c3313739142e26dc10a46cd09 > > > Cr-Commit-Position: refs/heads/master@{#343823} > > > > this change causes both: > > permissions_bubble_view.cc and permission_bubble_cocoa.mmincluded in > > toolit_views=1, mac_views_browser=0 configuration. > > Both of them having static scoped_ptr<PermissionBubbleView> > > PermissionBubbleView::Create implementation. > > Unfortunately, this doesn't cause linking error on mac. > > It looks like it should be fixed so that client code: > > > https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/ui/... > > will use views function with MacViewsDialogs and cocoa function withot them. > Is > > that right? > > Yeah - I think you're right. It looks like Chrome Mac is currently using Cocoa > for permission bubbles on Mac regardless of the flag. That was the intention > (i.e. we didn't want to port those bubbles with this change - just the site > settings). But maybe that's luck :/. I'll investigate further when I'm back next > week. Fix landed in http://crrev.com/349297 While toying with it, it looks like the linker accepted the ODR violation (and always used the Cocoa bubble), because if it *tried* to link the views permissions bubble, it would result in undefined references in PermissionBubbleViewViews::GetAnchorView(), so just ended up pruning the views duplicate.
Message was sent while issue was closed.
On 2015/09/17 01:03:27, tapted wrote: > On 2015/09/10 04:50:28, tapted wrote: > > On 2015/09/09 16:01:03, eugenebng wrote: > > > On 2015/08/18 05:24:31, commit-bot: I haz the power wrote: > > > > Patchset 13 (id:??) landed as > > > > https://crrev.com/700bafd0eba1167c3313739142e26dc10a46cd09 > > > > Cr-Commit-Position: refs/heads/master@{#343823} > > > > > > this change causes both: > > > permissions_bubble_view.cc and permission_bubble_cocoa.mmincluded in > > > toolit_views=1, mac_views_browser=0 configuration. > > > Both of them having static scoped_ptr<PermissionBubbleView> > > > PermissionBubbleView::Create implementation. > > > Unfortunately, this doesn't cause linking error on mac. > > > It looks like it should be fixed so that client code: > > > > > > https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/ui/... > > > will use views function with MacViewsDialogs and cocoa function withot them. > > Is > > > that right? > > > > Yeah - I think you're right. It looks like Chrome Mac is currently using Cocoa > > for permission bubbles on Mac regardless of the flag. That was the intention > > (i.e. we didn't want to port those bubbles with this change - just the site > > settings). But maybe that's luck :/. I'll investigate further when I'm back > next > > week. > > Fix landed in http://crrev.com/349297 > > While toying with it, it looks like the linker accepted the ODR violation (and > always used the Cocoa bubble), because if it *tried* to link the views > permissions bubble, it > would result in undefined references in > PermissionBubbleViewViews::GetAnchorView(), so just ended up pruning the views > duplicate. Thank you very much. Haven't expected that fix already landed so quickly. |
