Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1570153003:
Use MacViews for ContentSettingBubbles. (Closed)
|
Created:
4 years, 11 months ago by Elly Fong-Jones Modified:
4 years, 11 months ago CC:
chromium-reviews, tfarina Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionUse MacViews for ContentSettingBubbles.
Use Views to create bubbles for content settings, behind the "mac-views-dialogs"
flag.
BUG=564906
Committed: https://crrev.com/87882ce3314272e4cedcef82f78bc4061903e863
Cr-Commit-Position: refs/heads/master@{#369417}
Patch Set 1 #
Total comments: 17
Patch Set 2 : Fixes for tapted #
Total comments: 10
Patch Set 3 : More fixes :) #
Total comments: 4
Patch Set 4 : nits for tapted #
Total comments: 1
Messages
Total messages: 20 (7 generated)
ellyjones@chromium.org changed reviewers: + tapted@chromium.org
ptal? :)
some of my comments may become redundant with the bigger suggestions. I've left them in in case things don't pan out, but make sure you read them all before resolving things :) https://codereview.chromium.org/1570153003/diff/1/chrome/browser/ui/cocoa/loc... File chrome/browser/ui/cocoa/location_bar/content_setting_bubble_views_mac.h (right): https://codereview.chromium.org/1570153003/diff/1/chrome/browser/ui/cocoa/loc... chrome/browser/ui/cocoa/location_bar/content_setting_bubble_views_mac.h:1: // Copyright (c) 2016 The Chromium Authors. All rights reserved. nit: no `(c)`, more in other files. https://codereview.chromium.org/1570153003/diff/1/chrome/browser/ui/cocoa/loc... chrome/browser/ui/cocoa/location_bar/content_setting_bubble_views_mac.h:4: #include guards? also stuff under c/b/ui/cocoa doesn't usually need a _mac suffix https://codereview.chromium.org/1570153003/diff/1/chrome/browser/ui/cocoa/loc... chrome/browser/ui/cocoa/location_bar/content_setting_bubble_views_mac.h:17: ContentSettingBubbleBridge(ContentSettingBubbleModel* model, I think this can just be a standalone function that also does the call to Show()? (it would be different if the destructor had a task that needed doing, but I think it's effectively a no-op here). perhaps even in browser_dialogs.h / browser_dialogs_views_mac.cc Edit: that might disrupt a later comment wrt the SetAnchorRect call. So, new idea: make this static function [include the Show() call] on a class like `ContentSettingBubbleBridgeMac`. Forward-declare ContentSettingBubbleBridgeMac in content_setting_bubble_contents.h and make it a friend of ContentSettingBubbleContents. Note: I still think this "class-with-single-static-method" should still be declared in browser_dialogs.h / browser_dialogs_views_mac.cc . It _could_ stay here, but it would be a little bit naughty: forward-declaring a class implemented under c/b/ui/cocoa to be a friend of a class in c/b/ui/views technically breaks DEPS. I a friend class in browse_dialogs.h is a nice alternative to run by the c/b/ui/views OWNERS [versus modifying/overloading the ContentSettingBubbleContents constructor to accept a gfx::Point instead of a views::View]. https://codereview.chromium.org/1570153003/diff/1/chrome/browser/ui/cocoa/loc... chrome/browser/ui/cocoa/location_bar/content_setting_bubble_views_mac.h:23: private: nit: blank line before https://codereview.chromium.org/1570153003/diff/1/chrome/browser/ui/cocoa/loc... chrome/browser/ui/cocoa/location_bar/content_setting_bubble_views_mac.h:25: }; nit: DISALLOW_COPY_AND_ASSIGN(..) https://codereview.chromium.org/1570153003/diff/1/chrome/browser/ui/cocoa/loc... File chrome/browser/ui/cocoa/location_bar/content_setting_bubble_views_mac.mm (right): https://codereview.chromium.org/1570153003/diff/1/chrome/browser/ui/cocoa/loc... chrome/browser/ui/cocoa/location_bar/content_setting_bubble_views_mac.mm:1: // Copyright (c) 2016 The Chromium Authors. All rights reserved. can this be .cc instead of .mm? https://codereview.chromium.org/1570153003/diff/1/chrome/browser/ui/cocoa/loc... chrome/browser/ui/cocoa/location_bar/content_setting_bubble_views_mac.mm:5: #include "chrome/browser/ui/cocoa/location_bar/content_setting_bubble_views_mac.h" nit: blank line after the #include for a .cc's corresponding .h file https://codereview.chromium.org/1570153003/diff/1/chrome/browser/ui/cocoa/loc... chrome/browser/ui/cocoa/location_bar/content_setting_bubble_views_mac.mm:14: web_contents, NULL, nit: NULL -> nullptr https://codereview.chromium.org/1570153003/diff/1/chrome/browser/ui/cocoa/loc... chrome/browser/ui/cocoa/location_bar/content_setting_bubble_views_mac.mm:15: views::BubbleBorder::Arrow::TOP_RIGHT); I *think* CreateBubbleWidget(..) in ui/views/bubble/bubble_delegate.cc should be setting a value from the parent_window() when initializing the Widget. This sets up a kind of ownership (e.g. when the parent window closes), ensures the bubble is always layered above the parent, and (ignoring resizes) ensures it is "stuck" to the anchor when the window moves around. Without it, I think there will be weird behaviour, e.g., when the window is fading out or when clicking the location bar decoration multiple times. It looks like we might be able to just call contents->set_parent_window(web_contents->GetTopLevelNativeWindow()) after this https://codereview.chromium.org/1570153003/diff/1/chrome/browser/ui/cocoa/loc... chrome/browser/ui/cocoa/location_bar/content_setting_bubble_views_mac.mm:16: contents->SetExplicitAnchorRect(gfx::Rect(anchor, gfx::Size(0, 0))); nit: Size(0, 0) -> Size() https://codereview.chromium.org/1570153003/diff/1/chrome/browser/ui/cocoa/loc... File chrome/browser/ui/cocoa/location_bar/content_setting_decoration.h (right): https://codereview.chromium.org/1570153003/diff/1/chrome/browser/ui/cocoa/loc... chrome/browser/ui/cocoa/location_bar/content_setting_decoration.h:11: #import "chrome/browser/ui/cocoa/location_bar/content_setting_bubble_views_mac.h" nit: I think this can be forward declared. Also in the .cc (ubernit): import -> include https://codereview.chromium.org/1570153003/diff/1/chrome/browser/ui/cocoa/loc... chrome/browser/ui/cocoa/location_bar/content_setting_decoration.h:26: namespace views { nit: forward dec not needed (I think?). But also forward-declarations should be subject to the same DEPS rules as #includes. (i.e. only forward-declare stuff if it would also be valid to #include the corresponding header) https://codereview.chromium.org/1570153003/diff/1/chrome/browser/ui/cocoa/loc... chrome/browser/ui/cocoa/location_bar/content_setting_decoration.h:76: std::unique_ptr<ContentSettingBubbleBridge> bubble_bridge_; std::unique_ptr is still on the "to be discussed" section of https://chromium-cpp.appspot.com/ so I think we need to stick with scoped_ptr for now https://codereview.chromium.org/1570153003/diff/1/chrome/browser/ui/cocoa/loc... File chrome/browser/ui/cocoa/location_bar/content_setting_decoration.mm (right): https://codereview.chromium.org/1570153003/diff/1/chrome/browser/ui/cocoa/loc... chrome/browser/ui/cocoa/location_bar/content_setting_decoration.mm:283: // |SetExplicitAnchorRect| below to give it an anchor rect directly. nit: this comment feels better above the line that calls `new ContentSettingBubbleContents` https://codereview.chromium.org/1570153003/diff/1/chrome/browser/ui/cocoa/loc... File chrome/browser/ui/cocoa/location_bar/location_bar_view_mac.h (right): https://codereview.chromium.org/1570153003/diff/1/chrome/browser/ui/cocoa/loc... chrome/browser/ui/cocoa/location_bar/location_bar_view_mac.h:243: scoped_ptr<LocationBarViewDecoration> test_decoration_; Is this bit used? https://codereview.chromium.org/1570153003/diff/1/chrome/browser/ui/cocoa/loc... File chrome/browser/ui/cocoa/location_bar/location_bar_view_mac.mm (right): https://codereview.chromium.org/1570153003/diff/1/chrome/browser/ui/cocoa/loc... chrome/browser/ui/cocoa/location_bar/location_bar_view_mac.mm:42: #import "chrome/browser/ui/cocoa/location_bar/location_bar_view_decoration.h" do you need to git-add this? https://codereview.chromium.org/1570153003/diff/1/chrome/browser/ui/views/con... File chrome/browser/ui/views/content_setting_bubble_contents.h (right): https://codereview.chromium.org/1570153003/diff/1/chrome/browser/ui/views/con... chrome/browser/ui/views/content_setting_bubble_contents.h:63: // Temporary hack for MacViews. Do not use this. I think if you declare ContentSettingBubbleBridge as a friend, you'll be able to call SetAnchorRect directly which is a bit neater (and it's a common pattern for a Bridge class to be a friend/inner-class of one of the things it's bridging). But! of course... you can't do this if we go for a standalone function in browser_dialogs.h, so I'll update the comment there. I think a friend is the least intrusive approach, but c/b/ui/views owners may have a different opinion (e.g. for some of the other dialogs, we overloaded the constructor) I guess we could also declare a friend *function*, but that's not a common practice in Chrome -- friend classes are easier to understand and nicer to maintain.
On 2016/01/11 02:50:58, tapted wrote: > some of my comments may become redundant with the bigger suggestions. I've left > them in in case things don't pan out, but make sure you read them all before > resolving things :) > > https://codereview.chromium.org/1570153003/diff/1/chrome/browser/ui/cocoa/loc... > File chrome/browser/ui/cocoa/location_bar/content_setting_bubble_views_mac.h > (right): > > https://codereview.chromium.org/1570153003/diff/1/chrome/browser/ui/cocoa/loc... > chrome/browser/ui/cocoa/location_bar/content_setting_bubble_views_mac.h:1: // > Copyright (c) 2016 The Chromium Authors. All rights reserved. > nit: no `(c)`, more in other files. > > https://codereview.chromium.org/1570153003/diff/1/chrome/browser/ui/cocoa/loc... > chrome/browser/ui/cocoa/location_bar/content_setting_bubble_views_mac.h:4: > #include guards? > > also stuff under c/b/ui/cocoa doesn't usually need a _mac suffix > > https://codereview.chromium.org/1570153003/diff/1/chrome/browser/ui/cocoa/loc... > chrome/browser/ui/cocoa/location_bar/content_setting_bubble_views_mac.h:17: > ContentSettingBubbleBridge(ContentSettingBubbleModel* model, > I think this can just be a standalone function that also does the call to > Show()? (it would be different if the destructor had a task that needed doing, > but I think it's effectively a no-op here). > > perhaps even in browser_dialogs.h / browser_dialogs_views_mac.cc > > Edit: that might disrupt a later comment wrt the SetAnchorRect call. So, new > idea: make this static function [include the Show() call] on a class like > `ContentSettingBubbleBridgeMac`. Forward-declare ContentSettingBubbleBridgeMac > in content_setting_bubble_contents.h and make it a friend of > ContentSettingBubbleContents. > > Note: I still think this "class-with-single-static-method" should still be > declared in browser_dialogs.h / browser_dialogs_views_mac.cc . It _could_ stay > here, but it would be a little bit naughty: forward-declaring a class > implemented under c/b/ui/cocoa to be a friend of a class in c/b/ui/views > technically breaks DEPS. I a friend class in browse_dialogs.h is a nice > alternative to run by the c/b/ui/views OWNERS [versus modifying/overloading the > ContentSettingBubbleContents constructor to accept a gfx::Point instead of a > views::View]. > > https://codereview.chromium.org/1570153003/diff/1/chrome/browser/ui/cocoa/loc... > chrome/browser/ui/cocoa/location_bar/content_setting_bubble_views_mac.h:23: > private: > nit: blank line before > > https://codereview.chromium.org/1570153003/diff/1/chrome/browser/ui/cocoa/loc... > chrome/browser/ui/cocoa/location_bar/content_setting_bubble_views_mac.h:25: }; > nit: DISALLOW_COPY_AND_ASSIGN(..) > > https://codereview.chromium.org/1570153003/diff/1/chrome/browser/ui/cocoa/loc... > File chrome/browser/ui/cocoa/location_bar/content_setting_bubble_views_mac.mm > (right): > > https://codereview.chromium.org/1570153003/diff/1/chrome/browser/ui/cocoa/loc... > chrome/browser/ui/cocoa/location_bar/content_setting_bubble_views_mac.mm:1: // > Copyright (c) 2016 The Chromium Authors. All rights reserved. > can this be .cc instead of .mm? > > https://codereview.chromium.org/1570153003/diff/1/chrome/browser/ui/cocoa/loc... > chrome/browser/ui/cocoa/location_bar/content_setting_bubble_views_mac.mm:5: > #include > "chrome/browser/ui/cocoa/location_bar/content_setting_bubble_views_mac.h" > nit: blank line after the #include for a .cc's corresponding .h file > > https://codereview.chromium.org/1570153003/diff/1/chrome/browser/ui/cocoa/loc... > chrome/browser/ui/cocoa/location_bar/content_setting_bubble_views_mac.mm:14: > web_contents, NULL, > nit: NULL -> nullptr > > https://codereview.chromium.org/1570153003/diff/1/chrome/browser/ui/cocoa/loc... > chrome/browser/ui/cocoa/location_bar/content_setting_bubble_views_mac.mm:15: > views::BubbleBorder::Arrow::TOP_RIGHT); > I *think* CreateBubbleWidget(..) in ui/views/bubble/bubble_delegate.cc should be > setting a value from the parent_window() when initializing the Widget. This sets > up a kind of ownership (e.g. when the parent window closes), ensures the bubble > is always layered above the parent, and (ignoring resizes) ensures it is "stuck" > to the anchor when the window moves around. Without it, I think there will be > weird behaviour, e.g., when the window is fading out or when clicking the > location bar decoration multiple times. > > It looks like we might be able to just call > contents->set_parent_window(web_contents->GetTopLevelNativeWindow()) after this > > https://codereview.chromium.org/1570153003/diff/1/chrome/browser/ui/cocoa/loc... > chrome/browser/ui/cocoa/location_bar/content_setting_bubble_views_mac.mm:16: > contents->SetExplicitAnchorRect(gfx::Rect(anchor, gfx::Size(0, 0))); > nit: Size(0, 0) -> Size() > > https://codereview.chromium.org/1570153003/diff/1/chrome/browser/ui/cocoa/loc... > File chrome/browser/ui/cocoa/location_bar/content_setting_decoration.h (right): > > https://codereview.chromium.org/1570153003/diff/1/chrome/browser/ui/cocoa/loc... > chrome/browser/ui/cocoa/location_bar/content_setting_decoration.h:11: #import > "chrome/browser/ui/cocoa/location_bar/content_setting_bubble_views_mac.h" > nit: I think this can be forward declared. Also in the .cc (ubernit): import -> > include > > https://codereview.chromium.org/1570153003/diff/1/chrome/browser/ui/cocoa/loc... > chrome/browser/ui/cocoa/location_bar/content_setting_decoration.h:26: namespace > views { > nit: forward dec not needed (I think?). But also forward-declarations should be > subject to the same DEPS rules as #includes. (i.e. only forward-declare stuff if > it would also be valid to #include the corresponding header) > > https://codereview.chromium.org/1570153003/diff/1/chrome/browser/ui/cocoa/loc... > chrome/browser/ui/cocoa/location_bar/content_setting_decoration.h:76: > std::unique_ptr<ContentSettingBubbleBridge> bubble_bridge_; > std::unique_ptr is still on the "to be discussed" section of > https://chromium-cpp.appspot.com/ so I think we need to stick with scoped_ptr > for now > > https://codereview.chromium.org/1570153003/diff/1/chrome/browser/ui/cocoa/loc... > File chrome/browser/ui/cocoa/location_bar/content_setting_decoration.mm (right): > > https://codereview.chromium.org/1570153003/diff/1/chrome/browser/ui/cocoa/loc... > chrome/browser/ui/cocoa/location_bar/content_setting_decoration.mm:283: // > |SetExplicitAnchorRect| below to give it an anchor rect directly. > nit: this comment feels better above the line that calls `new > ContentSettingBubbleContents` > > https://codereview.chromium.org/1570153003/diff/1/chrome/browser/ui/cocoa/loc... > File chrome/browser/ui/cocoa/location_bar/location_bar_view_mac.h (right): > > https://codereview.chromium.org/1570153003/diff/1/chrome/browser/ui/cocoa/loc... > chrome/browser/ui/cocoa/location_bar/location_bar_view_mac.h:243: > scoped_ptr<LocationBarViewDecoration> test_decoration_; > Is this bit used? > > https://codereview.chromium.org/1570153003/diff/1/chrome/browser/ui/cocoa/loc... > File chrome/browser/ui/cocoa/location_bar/location_bar_view_mac.mm (right): > > https://codereview.chromium.org/1570153003/diff/1/chrome/browser/ui/cocoa/loc... > chrome/browser/ui/cocoa/location_bar/location_bar_view_mac.mm:42: #import > "chrome/browser/ui/cocoa/location_bar/location_bar_view_decoration.h" > do you need to git-add this? > > https://codereview.chromium.org/1570153003/diff/1/chrome/browser/ui/views/con... > File chrome/browser/ui/views/content_setting_bubble_contents.h (right): > > https://codereview.chromium.org/1570153003/diff/1/chrome/browser/ui/views/con... > chrome/browser/ui/views/content_setting_bubble_contents.h:63: // Temporary hack > for MacViews. Do not use this. > I think if you declare ContentSettingBubbleBridge as a friend, you'll be able to > call SetAnchorRect directly which is a bit neater (and it's a common pattern for > a Bridge class to be a friend/inner-class of one of the things it's bridging). > > But! of course... you can't do this if we go for a standalone function in > browser_dialogs.h, so I'll update the comment there. I think a friend is the > least intrusive approach, but c/b/ui/views owners may have a different opinion > (e.g. for some of the other dialogs, we overloaded the constructor) > > I guess we could also declare a friend *function*, but that's not a common > practice in Chrome -- friend classes are easier to understand and nicer to > maintain. I think I addressed everything in this redesign. PTAL?
https://codereview.chromium.org/1570153003/diff/20001/chrome/browser/ui/brows... File chrome/browser/ui/browser_dialogs.h (right): https://codereview.chromium.org/1570153003/diff/20001/chrome/browser/ui/brows... chrome/browser/ui/browser_dialogs.h:122: class ContentSettingBubbleViewsBridge { I think this should be up in the #if defined(OS_MACOSX) section, just to make it clear that only Mac will call it https://codereview.chromium.org/1570153003/diff/20001/chrome/browser/ui/brows... chrome/browser/ui/browser_dialogs.h:129: ContentSettingBubbleViewsBridge(); DISALLOW_IMPLICIT_CONSTRUCTORS(..) https://codereview.chromium.org/1570153003/diff/20001/chrome/browser/ui/cocoa... File chrome/browser/ui/cocoa/location_bar/content_setting_decoration.mm (right): https://codereview.chromium.org/1570153003/diff/20001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/location_bar/content_setting_decoration.mm:283: // |SetExplicitAnchorRect| below to give it an anchor rect directly. update comment? https://codereview.chromium.org/1570153003/diff/20001/chrome/browser/ui/views... File chrome/browser/ui/views/content_setting_bubble_contents.cc (right): https://codereview.chromium.org/1570153003/diff/20001/chrome/browser/ui/views... chrome/browser/ui/views/content_setting_bubble_contents.cc:505: void ContentSettingBubbleViewsBridge::Show(gfx::NativeView parent_view, Can this go in c/b/ui/views/browser_dialog_views_mac.cc - this would make it clear that it's something we can remove once the browser window is no longer Cocoa https://codereview.chromium.org/1570153003/diff/20001/chrome/browser/ui/views... chrome/browser/ui/views/content_setting_bubble_contents.cc:514: // TODO(ellyjones): why doesn't this leak the bubble? Yeah it's spooky isn't it.. Probably CreateBubble should take a scoped_ptr (or at least say that it assumes ownership). Instead it takes a `BubbleDelegateView`, which inherits from views::View. Views tend to quickly get inserted into a View hierarchy which transfers ownership to the root view of the Widget, but scoped_ptrs haven't caught on -- it would make a lot of the UI code really clumsy. So there's a convention among toolkit-views UI code that things inheriting from a views::View* just use raw pointers and you "should" always plonk them into a view hierarchy before the method returns. Where the ownership is actually transferred is by (also) setting the BubbleDelegateView as the Widget delegate, and providing an override for `GetContentsView`. Then if you look in Widget::Init(..), you can see it will either make a `new NonClientView` and set that as the ContentsView, or set the ContentsView from delegate->GetContentsView(). BubbleDelegateView::GetContentsView() doesn't allocate with `new` -- it just returns `this` and so it effectively provides that `new ContentSettingBubbleContents` that happens just above this line in place of the `new NonClientView` that Widget::Init would do otherwise.
https://codereview.chromium.org/1570153003/diff/20001/chrome/browser/ui/brows... File chrome/browser/ui/browser_dialogs.h (right): https://codereview.chromium.org/1570153003/diff/20001/chrome/browser/ui/brows... chrome/browser/ui/browser_dialogs.h:122: class ContentSettingBubbleViewsBridge { On 2016/01/11 23:21:38, tapted wrote: > I think this should be up in the #if defined(OS_MACOSX) section, just to make it > clear that only Mac will call it I think it also needs to be inside the TOOLKIT_VIEWS ifdef, so I added a new OS_MACOSX section inside that. https://codereview.chromium.org/1570153003/diff/20001/chrome/browser/ui/brows... chrome/browser/ui/browser_dialogs.h:129: ContentSettingBubbleViewsBridge(); On 2016/01/11 23:21:38, tapted wrote: > DISALLOW_IMPLICIT_CONSTRUCTORS(..) Done. https://codereview.chromium.org/1570153003/diff/20001/chrome/browser/ui/cocoa... File chrome/browser/ui/cocoa/location_bar/content_setting_decoration.mm (right): https://codereview.chromium.org/1570153003/diff/20001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/location_bar/content_setting_decoration.mm:283: // |SetExplicitAnchorRect| below to give it an anchor rect directly. On 2016/01/11 23:21:38, tapted wrote: > update comment? Done (deleted it) https://codereview.chromium.org/1570153003/diff/20001/chrome/browser/ui/views... File chrome/browser/ui/views/content_setting_bubble_contents.cc (right): https://codereview.chromium.org/1570153003/diff/20001/chrome/browser/ui/views... chrome/browser/ui/views/content_setting_bubble_contents.cc:505: void ContentSettingBubbleViewsBridge::Show(gfx::NativeView parent_view, On 2016/01/11 23:21:38, tapted wrote: > Can this go in c/b/ui/views/browser_dialog_views_mac.cc - this would make it > clear that it's something we can remove once the browser window is no longer > Cocoa Done. https://codereview.chromium.org/1570153003/diff/20001/chrome/browser/ui/views... chrome/browser/ui/views/content_setting_bubble_contents.cc:514: // TODO(ellyjones): why doesn't this leak the bubble? On 2016/01/11 23:21:39, tapted wrote: > Yeah it's spooky isn't it.. Probably CreateBubble should take a scoped_ptr (or > at least say that it assumes ownership). Instead it takes a > `BubbleDelegateView`, which inherits from views::View. Views tend to quickly get > inserted into a View hierarchy which transfers ownership to the root view of the > Widget, but scoped_ptrs haven't caught on -- it would make a lot of the UI code > really clumsy. So there's a convention among toolkit-views UI code that things > inheriting from a views::View* just use raw pointers and you "should" always > plonk them into a view hierarchy before the method returns. > > Where the ownership is actually transferred is by (also) setting the > BubbleDelegateView as the Widget delegate, and providing an override for > `GetContentsView`. > > Then if you look in Widget::Init(..), you can see it will either make a `new > NonClientView` and set that as the ContentsView, or set the ContentsView from > delegate->GetContentsView(). BubbleDelegateView::GetContentsView() doesn't > allocate with `new` -- it just returns `this` and so it effectively provides > that `new ContentSettingBubbleContents` that happens just above this line in > place of the `new NonClientView` that Widget::Init would do otherwise. Ick. Ack.
lgtm % nits, you'll need a c/b/ui/views reviewer - maybe msw since he's familiar with the other bubbles we set up https://codereview.chromium.org/1570153003/diff/40001/chrome/browser/ui/views... File chrome/browser/ui/views/content_setting_bubble_contents.cc (right): https://codereview.chromium.org/1570153003/diff/40001/chrome/browser/ui/views... chrome/browser/ui/views/content_setting_bubble_contents.cc:18: #include "chrome/browser/ui/browser_dialogs.h" nit: I think this can be removed now https://codereview.chromium.org/1570153003/diff/40001/chrome/browser/ui/views... File chrome/browser/ui/views/content_setting_bubble_contents.h (right): https://codereview.chromium.org/1570153003/diff/40001/chrome/browser/ui/views... chrome/browser/ui/views/content_setting_bubble_contents.h:75: friend class chrome::ContentSettingBubbleViewsBridge; nit: it's subtle why this is needed. Although it repeats what's on the class, perhaps a brief comment like // Permit ContentSettingBubbleViewsBridge to call SetAnchorRect().
ellyjones@chromium.org changed reviewers: + msw@chromium.org
thanks trent :) msw: PTAL for owner review? https://codereview.chromium.org/1570153003/diff/40001/chrome/browser/ui/views... File chrome/browser/ui/views/content_setting_bubble_contents.cc (right): https://codereview.chromium.org/1570153003/diff/40001/chrome/browser/ui/views... chrome/browser/ui/views/content_setting_bubble_contents.cc:18: #include "chrome/browser/ui/browser_dialogs.h" On 2016/01/12 22:02:42, tapted wrote: > nit: I think this can be removed now Done. https://codereview.chromium.org/1570153003/diff/40001/chrome/browser/ui/views... File chrome/browser/ui/views/content_setting_bubble_contents.h (right): https://codereview.chromium.org/1570153003/diff/40001/chrome/browser/ui/views... chrome/browser/ui/views/content_setting_bubble_contents.h:75: friend class chrome::ContentSettingBubbleViewsBridge; On 2016/01/12 22:02:42, tapted wrote: > nit: it's subtle why this is needed. Although it repeats what's on the class, > perhaps a brief comment like > > // Permit ContentSettingBubbleViewsBridge to call SetAnchorRect(). Done.
lgtm with a heads up fyi. https://codereview.chromium.org/1570153003/diff/60001/chrome/browser/ui/views... File chrome/browser/ui/views/browser_dialogs_views_mac.cc (right): https://codereview.chromium.org/1570153003/diff/60001/chrome/browser/ui/views... chrome/browser/ui/views/browser_dialogs_views_mac.cc:56: contents->SetAnchorRect(gfx::Rect(anchor, gfx::Size())); fyi: using an empty size here means that if the bubble needs to flip vertically, then the arrow will overlap the icon that it's anchored beneath.
The CQ bit was checked by ellyjones@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1570153003/60001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1570153003/60001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by ellyjones@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/1570153003/#ps60001 (title: "nits for tapted")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1570153003/60001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1570153003/60001
Message was sent while issue was closed.
Committed patchset #4 (id:60001)
Message was sent while issue was closed.
Description was changed from ========== Use MacViews for ContentSettingBubbles. Use Views to create bubbles for content settings, behind the "mac-views-dialogs" flag. BUG=564906 ========== to ========== Use MacViews for ContentSettingBubbles. Use Views to create bubbles for content settings, behind the "mac-views-dialogs" flag. BUG=564906 Committed: https://crrev.com/87882ce3314272e4cedcef82f78bc4061903e863 Cr-Commit-Position: refs/heads/master@{#369417} ==========
Message was sent while issue was closed.
Patchset 4 (id:??) landed as https://crrev.com/87882ce3314272e4cedcef82f78bc4061903e863 Cr-Commit-Position: refs/heads/master@{#369417} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
