Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1935993004:
MacViews: support Views permission bubble (Closed)
|
Created:
4 years, 7 months ago by Elly Fong-Jones Modified:
4 years, 7 months ago CC:
chromium-reviews, tfarina, markusheintz_, msramek+watch_chromium.org, raymes+watch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionMacViews: support Views permission bubble
The existing PermissionBubbleViewViews class needs an anchor view or point, so
this CL adds some machinery to pass an anchor point down from the Cocoa layer,
and adds a delegate so platforms can produce their own anchors.
BUG=605667
Committed: https://crrev.com/5763588fd12942f8829b5c21300ad094bb0e5203
Cr-Commit-Position: refs/heads/master@{#395850}
Patch Set 1 #
Total comments: 32
Patch Set 2 : factor out Views delegate #
Total comments: 38
Patch Set 3 : next version! #Patch Set 4 : add forgotten docs #
Total comments: 6
Patch Set 5 : Fold in https://codereview.chromium.org/1970113002 and rebase #
Total comments: 3
Patch Set 6 : delegate removed #Patch Set 7 : fix typo in chrome_browser_ui.gypi #Messages
Total messages: 37 (14 generated)
Description was changed from ========== MacViews: support Views permission bubble The existing PermissionBubbleViewViews class needs an anchor view or point, so this CL adds some machinery to pass an anchor point down from the Cocoa layer, and adds a delegate so platforms can produce their own anchors. BUG=605667 ========== to ========== MacViews: support Views permission bubble The existing PermissionBubbleViewViews class needs an anchor view or point, so this CL adds some machinery to pass an anchor point down from the Cocoa layer, and adds a delegate so platforms can produce their own anchors. BUG=605667 ==========
ellyjones@chromium.org changed reviewers: + tapted@chromium.org
tapted: ptal? :)
https://codereview.chromium.org/1935993004/diff/1/chrome/browser/ui/cocoa/web... File chrome/browser/ui/cocoa/website_settings/permission_bubble_anchor_delegate_views_cocoa.h (right): https://codereview.chromium.org/1935993004/diff/1/chrome/browser/ui/cocoa/web... chrome/browser/ui/cocoa/website_settings/permission_bubble_anchor_delegate_views_cocoa.h:1: // Copyright 2016 The Chromium Authors. All rights reserved. I think we can avoid having to provide this .h file -- it can all be in the .mm by adding a PermissionBubbleViewViews::AnchorDelegate* PermissionBubbleViewViews::CreateAnchorDelegate(Browser* browser); declared in permissions_bubble_view.h and defined in permission_bubble_anchor_delegate_views_cocoa.mm (for MacCocoa) and in permission_bubble_anchor_delegate_views.cc (for Aura and mac_views_browser) https://codereview.chromium.org/1935993004/diff/1/chrome/browser/ui/cocoa/web... File chrome/browser/ui/cocoa/website_settings/permission_bubble_cocoa.mm (right): https://codereview.chromium.org/1935993004/diff/1/chrome/browser/ui/cocoa/web... chrome/browser/ui/cocoa/website_settings/permission_bubble_cocoa.mm:37: new PermissionBubbleViewViews(browser, std::move(delegate))); so... for IWYU, permission_bubble_cocoa.mm really should be #including "chrome/browser/ui/views/website_settings/permissions_bubble_view.h" in order to call this constructor, but of course you can't because of DEPS :p (the trick now is that you get it transitively via permission_bubble_anchor_delegate_views_cocoa.h, but I just suggested we delete that :). The right fix is to move the entire PermissionBubbleView::Create implementation into permission_bubble_anchor_delegate_views_cocoa.mm That also gives us an excuse to call the file just permission_bubble_views_cocoa.mm I'd still go with the name PermissionBubbleAnchorDelegateViewsCocoa, but make sure it has a comment saying it's designed for anchoring off a toolkit-views permission bubble off a Cocoa Browser https://codereview.chromium.org/1935993004/diff/1/chrome/browser/ui/views/web... File chrome/browser/ui/views/website_settings/permissions_bubble_view.cc (right): https://codereview.chromium.org/1935993004/diff/1/chrome/browser/ui/views/web... chrome/browser/ui/views/website_settings/permissions_bubble_view.cc:15: #include "chrome/browser/ui/views/frame/browser_view.h" none of browser_view.h top_container_view.h location_bar_view.h location_icon_view.h are compiled into the Mac build. We really need to move all the things that need them into their own file -- probably along with the ViewsAnchorDelegate (which, hopefully, is also the _only_ thing that needs them...) https://codereview.chromium.org/1935993004/diff/1/chrome/browser/ui/views/web... chrome/browser/ui/views/website_settings/permissions_bubble_view.cc:152: gfx::Point anchor_point, nit: const reference https://codereview.chromium.org/1935993004/diff/1/chrome/browser/ui/views/web... chrome/browser/ui/views/website_settings/permissions_bubble_view.cc:206: SetAnchorRect(gfx::Rect(anchor_point, gfx::Size())); (see later comment about UpdateAnchor() -- it might be possible to just call UpdateAnchor() rather than passing in anchor_point to the constructor as well) https://codereview.chromium.org/1935993004/diff/1/chrome/browser/ui/views/web... chrome/browser/ui/views/website_settings/permissions_bubble_view.cc:398: #if !defined(OS_MACOSX) This approach won't work with the mac_views_browser build -- there we need to anchor off a toolkit-views browser still, but on Mac. But once this is in its own file, this should be easily doable with gyp variables https://codereview.chromium.org/1935993004/diff/1/chrome/browser/ui/views/web... chrome/browser/ui/views/website_settings/permissions_bubble_view.cc:399: class ViewAnchorDelegate : public PermissionBubbleViewViews::AnchorDelegate { Perhaps ViewsBrowserAnchorDelegate? It should have a comment describing it too. Like // Permits a toolkit-views permissions bubble to be anchored off a toolkit-views BrowserView. https://codereview.chromium.org/1935993004/diff/1/chrome/browser/ui/views/web... chrome/browser/ui/views/website_settings/permissions_bubble_view.cc:403: views::View* GetAnchorView() override; nit: // AnchorDelegate: https://codereview.chromium.org/1935993004/diff/1/chrome/browser/ui/views/web... chrome/browser/ui/views/website_settings/permissions_bubble_view.cc:409: }; nit: DISALLOW_COPY_AND_ASSIGN(..) https://codereview.chromium.org/1935993004/diff/1/chrome/browser/ui/views/web... chrome/browser/ui/views/website_settings/permissions_bubble_view.cc:476: bubble_delegate_->set_parent_window(anchor_delegate_->GetParentView()); I don't think we need AnchorDelegate::GetParentView(), instead do: platform_util::GetViewForWindow(browser_->window()->GetNativeWindow()); https://codereview.chromium.org/1935993004/diff/1/chrome/browser/ui/views/web... chrome/browser/ui/views/website_settings/permissions_bubble_view.cc:499: bubble_delegate_->UpdateAnchor(GetAnchorView(), GetAnchorArrow()); This needs to support a null AnchorView - probably by allowing an AnchorPoint to be passed in to UpdateAnchor as well https://codereview.chromium.org/1935993004/diff/1/chrome/browser/ui/views/web... File chrome/browser/ui/views/website_settings/permissions_bubble_view.h (right): https://codereview.chromium.org/1935993004/diff/1/chrome/browser/ui/views/web... chrome/browser/ui/views/website_settings/permissions_bubble_view.h:33: AnchorDelegate(){} nit: space before {} https://codereview.chromium.org/1935993004/diff/1/chrome/browser/ui/views/web... chrome/browser/ui/views/website_settings/permissions_bubble_view.h:34: virtual ~AnchorDelegate(){} I think we can omit the destructor altogether for pure interfaces https://codereview.chromium.org/1935993004/diff/1/chrome/browser/ui/views/web... chrome/browser/ui/views/website_settings/permissions_bubble_view.h:41: std::unique_ptr<AnchorDelegate> anchor_delegate); Since both AnchorDelegates are just created from the Browser* I think we can avoid having to pass this in, and just initialize anchor_delegate_ with the CreateAnchorDelegate(browser) function I mentioned earlier https://codereview.chromium.org/1935993004/diff/1/chrome/browser/ui/views/web... chrome/browser/ui/views/website_settings/permissions_bubble_view.h:61: gfx::Point GetAnchorPoint(); this might not be needed. In fact we can probably remove GetAnchorView as well, since it's just anchor_delegate->GetAnchorView() now and there are just 1-2 call sites (depending how much we can lean on UpdateAnchor())
tapted: ptal? :) https://codereview.chromium.org/1935993004/diff/1/chrome/browser/ui/cocoa/web... File chrome/browser/ui/cocoa/website_settings/permission_bubble_anchor_delegate_views_cocoa.h (right): https://codereview.chromium.org/1935993004/diff/1/chrome/browser/ui/cocoa/web... chrome/browser/ui/cocoa/website_settings/permission_bubble_anchor_delegate_views_cocoa.h:1: // Copyright 2016 The Chromium Authors. All rights reserved. On 2016/05/03 12:02:38, tapted wrote: > I think we can avoid having to provide this .h file -- it can all be in the .mm > by adding a > > PermissionBubbleViewViews::AnchorDelegate* > PermissionBubbleViewViews::CreateAnchorDelegate(Browser* browser); > > declared in permissions_bubble_view.h and defined in > permission_bubble_anchor_delegate_views_cocoa.mm (for MacCocoa) and in > permission_bubble_anchor_delegate_views.cc (for Aura and mac_views_browser) Done. https://codereview.chromium.org/1935993004/diff/1/chrome/browser/ui/cocoa/web... File chrome/browser/ui/cocoa/website_settings/permission_bubble_cocoa.mm (right): https://codereview.chromium.org/1935993004/diff/1/chrome/browser/ui/cocoa/web... chrome/browser/ui/cocoa/website_settings/permission_bubble_cocoa.mm:37: new PermissionBubbleViewViews(browser, std::move(delegate))); On 2016/05/03 12:02:38, tapted wrote: > so... for IWYU, permission_bubble_cocoa.mm really should be #including > "chrome/browser/ui/views/website_settings/permissions_bubble_view.h" in order to > call this constructor, but of course you can't because of DEPS :p > > (the trick now is that you get it transitively via > permission_bubble_anchor_delegate_views_cocoa.h, but I just suggested we delete > that :). > > The right fix is to move the entire PermissionBubbleView::Create implementation > into permission_bubble_anchor_delegate_views_cocoa.mm > > That also gives us an excuse to call the file just > > permission_bubble_views_cocoa.mm > > I'd still go with the name PermissionBubbleAnchorDelegateViewsCocoa, but make > sure it has a comment saying it's designed for anchoring off a toolkit-views > permission bubble off a Cocoa Browser Done. https://codereview.chromium.org/1935993004/diff/1/chrome/browser/ui/views/web... File chrome/browser/ui/views/website_settings/permissions_bubble_view.cc (right): https://codereview.chromium.org/1935993004/diff/1/chrome/browser/ui/views/web... chrome/browser/ui/views/website_settings/permissions_bubble_view.cc:15: #include "chrome/browser/ui/views/frame/browser_view.h" On 2016/05/03 12:02:38, tapted wrote: > none of browser_view.h top_container_view.h location_bar_view.h > location_icon_view.h are compiled into the Mac build. We really need to move all > the things that need them into their own file -- probably along with the > ViewsAnchorDelegate (which, hopefully, is also the _only_ thing that needs > them...) Done. https://codereview.chromium.org/1935993004/diff/1/chrome/browser/ui/views/web... chrome/browser/ui/views/website_settings/permissions_bubble_view.cc:152: gfx::Point anchor_point, On 2016/05/03 12:02:38, tapted wrote: > nit: const reference Done. https://codereview.chromium.org/1935993004/diff/1/chrome/browser/ui/views/web... chrome/browser/ui/views/website_settings/permissions_bubble_view.cc:206: SetAnchorRect(gfx::Rect(anchor_point, gfx::Size())); On 2016/05/03 12:02:38, tapted wrote: > (see later comment about UpdateAnchor() -- it might be possible to just call > UpdateAnchor() rather than passing in anchor_point to the constructor as well) It's not safe to call UpdateAnchor() yet at this point, and it seems if I don't do this here, the view ends up anchored at (0,0). I am not 100% sure why this is, since UpdateAnchor should get called soon after this constructor runs. https://codereview.chromium.org/1935993004/diff/1/chrome/browser/ui/views/web... chrome/browser/ui/views/website_settings/permissions_bubble_view.cc:398: #if !defined(OS_MACOSX) On 2016/05/03 12:02:39, tapted wrote: > This approach won't work with the mac_views_browser build -- there we need to > anchor off a toolkit-views browser still, but on Mac. But once this is in its > own file, this should be easily doable with gyp variables Done. I moved this class, and the CreateAnchorDelegate function, *and* the PermissionBubbleView::Create views variant into a separate file, which I made "non mac views". https://codereview.chromium.org/1935993004/diff/1/chrome/browser/ui/views/web... chrome/browser/ui/views/website_settings/permissions_bubble_view.cc:399: class ViewAnchorDelegate : public PermissionBubbleViewViews::AnchorDelegate { On 2016/05/03 12:02:38, tapted wrote: > Perhaps ViewsBrowserAnchorDelegate? > > It should have a comment describing it too. Like > > // Permits a toolkit-views permissions bubble to be anchored off a toolkit-views > BrowserView. Done. https://codereview.chromium.org/1935993004/diff/1/chrome/browser/ui/views/web... chrome/browser/ui/views/website_settings/permissions_bubble_view.cc:403: views::View* GetAnchorView() override; On 2016/05/03 12:02:38, tapted wrote: > nit: > // AnchorDelegate: Done. https://codereview.chromium.org/1935993004/diff/1/chrome/browser/ui/views/web... chrome/browser/ui/views/website_settings/permissions_bubble_view.cc:409: }; On 2016/05/03 12:02:38, tapted wrote: > nit: DISALLOW_COPY_AND_ASSIGN(..) Done. https://codereview.chromium.org/1935993004/diff/1/chrome/browser/ui/views/web... chrome/browser/ui/views/website_settings/permissions_bubble_view.cc:476: bubble_delegate_->set_parent_window(anchor_delegate_->GetParentView()); On 2016/05/03 12:02:38, tapted wrote: > I don't think we need AnchorDelegate::GetParentView(), instead do: > > platform_util::GetViewForWindow(browser_->window()->GetNativeWindow()); Done. https://codereview.chromium.org/1935993004/diff/1/chrome/browser/ui/views/web... chrome/browser/ui/views/website_settings/permissions_bubble_view.cc:499: bubble_delegate_->UpdateAnchor(GetAnchorView(), GetAnchorArrow()); On 2016/05/03 12:02:39, tapted wrote: > This needs to support a null AnchorView - probably by allowing an AnchorPoint to > be passed in to UpdateAnchor as well Done. https://codereview.chromium.org/1935993004/diff/1/chrome/browser/ui/views/web... File chrome/browser/ui/views/website_settings/permissions_bubble_view.h (right): https://codereview.chromium.org/1935993004/diff/1/chrome/browser/ui/views/web... chrome/browser/ui/views/website_settings/permissions_bubble_view.h:33: AnchorDelegate(){} On 2016/05/03 12:02:39, tapted wrote: > nit: space before {} Done. https://codereview.chromium.org/1935993004/diff/1/chrome/browser/ui/views/web... chrome/browser/ui/views/website_settings/permissions_bubble_view.h:34: virtual ~AnchorDelegate(){} On 2016/05/03 12:02:39, tapted wrote: > I think we can omit the destructor altogether for pure interfaces Then the destructor isn't virtual and can't be overridden in the children, it seems. https://codereview.chromium.org/1935993004/diff/1/chrome/browser/ui/views/web... chrome/browser/ui/views/website_settings/permissions_bubble_view.h:41: std::unique_ptr<AnchorDelegate> anchor_delegate); On 2016/05/03 12:02:39, tapted wrote: > Since both AnchorDelegates are just created from the Browser* I think we can > avoid having to pass this in, and just initialize anchor_delegate_ with the > CreateAnchorDelegate(browser) function I mentioned earlier Done. https://codereview.chromium.org/1935993004/diff/1/chrome/browser/ui/views/web... chrome/browser/ui/views/website_settings/permissions_bubble_view.h:61: gfx::Point GetAnchorPoint(); On 2016/05/03 12:02:39, tapted wrote: > this might not be needed. In fact we can probably remove GetAnchorView as well, > since it's just anchor_delegate->GetAnchorView() now and there are just 1-2 call > sites (depending how much we can lean on UpdateAnchor()) Done.
looks good in general - should be fine to loop in msw if the comments resolve ok https://codereview.chromium.org/1935993004/diff/1/chrome/browser/ui/views/web... File chrome/browser/ui/views/website_settings/permissions_bubble_view.cc (right): https://codereview.chromium.org/1935993004/diff/1/chrome/browser/ui/views/web... chrome/browser/ui/views/website_settings/permissions_bubble_view.cc:206: SetAnchorRect(gfx::Rect(anchor_point, gfx::Size())); On 2016/05/10 21:21:02, Elly Jones wrote: > On 2016/05/03 12:02:38, tapted wrote: > > (see later comment about UpdateAnchor() -- it might be possible to just call > > UpdateAnchor() rather than passing in anchor_point to the constructor as well) > > It's not safe to call UpdateAnchor() yet at this point, and it seems if I don't > do this here, the view ends up anchored at (0,0). I am not 100% sure why this > is, since UpdateAnchor should get called soon after this constructor runs. Sorry, I meant, instead of bubble_delegate_ = new PermissionsBubbleDialogDelegateView( anchor_delegate_->GetAnchorView(), anchor_delegate_->GetAnchorPoint(), GetAnchorArrow(), this, requests, values); we should be able to do bubble_delegate_ = new PermissionsBubbleDialogDelegateView(this, requests, values); bubble_delegate_->UpdateAnchor(anchor_delegate_->GetAnchorView(), anchor_delegate_->GetAnchorPoint(), GetAnchorArrow()); https://codereview.chromium.org/1935993004/diff/1/chrome/browser/ui/views/web... File chrome/browser/ui/views/website_settings/permissions_bubble_view.h (right): https://codereview.chromium.org/1935993004/diff/1/chrome/browser/ui/views/web... chrome/browser/ui/views/website_settings/permissions_bubble_view.h:34: virtual ~AnchorDelegate(){} On 2016/05/10 21:21:02, Elly Jones wrote: > On 2016/05/03 12:02:39, tapted wrote: > > I think we can omit the destructor altogether for pure interfaces > > Then the destructor isn't virtual and can't be overridden in the children, it > seems. I don't think we need to override it in the children either :). (but if we ever did, declaring it `virtual` rather than `override` in the children should fix that) https://codereview.chromium.org/1935993004/diff/20001/chrome/browser/ui/cocoa... File chrome/browser/ui/cocoa/website_settings/permission_bubble_anchor_delegate_views_cocoa.mm (right): https://codereview.chromium.org/1935993004/diff/20001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/website_settings/permission_bubble_anchor_delegate_views_cocoa.mm:14: #include "ui/gfx/mac/coordinate_conversion.h" nit: import https://codereview.chromium.org/1935993004/diff/20001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/website_settings/permission_bubble_anchor_delegate_views_cocoa.mm:16: // Implementation of PermissionBubbleViewViews' AnchorDelegate for Cocoa you can start an anonymous namespace { here https://codereview.chromium.org/1935993004/diff/20001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/website_settings/permission_bubble_anchor_delegate_views_cocoa.mm:22: PermissionBubbleAnchorDelegateViewsCocoa(Browser* browser); nit: explicit https://codereview.chromium.org/1935993004/diff/20001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/website_settings/permission_bubble_anchor_delegate_views_cocoa.mm:23: ~PermissionBubbleAnchorDelegateViewsCocoa() override; does this need to be declared? the default one should work https://codereview.chromium.org/1935993004/diff/20001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/website_settings/permission_bubble_anchor_delegate_views_cocoa.mm:31: DISALLOW_COPY_AND_ASSIGN(PermissionBubbleAnchorDelegateViewsCocoa); nit: blank line before https://codereview.chromium.org/1935993004/diff/20001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/website_settings/permission_bubble_anchor_delegate_views_cocoa.mm:59: } // namespace https://codereview.chromium.org/1935993004/diff/20001/chrome/browser/ui/cocoa... File chrome/browser/ui/cocoa/website_settings/permission_bubble_cocoa.mm (right): https://codereview.chromium.org/1935993004/diff/20001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/website_settings/permission_bubble_cocoa.mm:8: #include "chrome/browser/ui/browser.h" I think all the 7 new headers showing up in the diff can be removed https://codereview.chromium.org/1935993004/diff/20001/chrome/browser/ui/views... File chrome/browser/ui/views/website_settings/permission_bubble_anchor_delegate_views.cc (right): https://codereview.chromium.org/1935993004/diff/20001/chrome/browser/ui/views... chrome/browser/ui/views/website_settings/permission_bubble_anchor_delegate_views.cc:7: #include "chrome/browser/ui/browser_window.h" there should be a lot more #includes needed than these. I regularly `git cl try` right after uploading to ensure I've got clean runs on all the bots (cross platform changes are hard :) At least the views/frame and views/location_bar stuff deleted from permissions_bubble_view.cc should be needed https://codereview.chromium.org/1935993004/diff/20001/chrome/browser/ui/views... chrome/browser/ui/views/website_settings/permission_bubble_anchor_delegate_views.cc:8: namespace { https://codereview.chromium.org/1935993004/diff/20001/chrome/browser/ui/views... chrome/browser/ui/views/website_settings/permission_bubble_anchor_delegate_views.cc:16: ~PermissionBubbleAnchorDelegateViews() override; this probably doesn't need to be declared https://codereview.chromium.org/1935993004/diff/20001/chrome/browser/ui/views... chrome/browser/ui/views/website_settings/permission_bubble_anchor_delegate_views.cc:23: } DISALLOW_COPY_AND_ASSIGN(..) (also there's a missing semicolon on this line) https://codereview.chromium.org/1935993004/diff/20001/chrome/browser/ui/views... chrome/browser/ui/views/website_settings/permission_bubble_anchor_delegate_views.cc:29: PermissionBubbleAnchorDelegateViews::~PermissionBubbleAnchorDelegateViews() {} nit (if it's kept): blank line before It's also fine to put simple stuff like this inline. https://codereview.chromium.org/1935993004/diff/20001/chrome/browser/ui/views... chrome/browser/ui/views/website_settings/permission_bubble_anchor_delegate_views.cc:46: } // namespace https://codereview.chromium.org/1935993004/diff/20001/chrome/browser/ui/views... File chrome/browser/ui/views/website_settings/permissions_bubble_view.cc (right): https://codereview.chromium.org/1935993004/diff/20001/chrome/browser/ui/views... chrome/browser/ui/views/website_settings/permissions_bubble_view.cc:411: bubble_delegate_ = new PermissionsBubbleDialogDelegateView( (so bubble_delegate_ = new PermissionsBubbleDialogDelegateView(this, requests, values); bubble_delegate_->UpdateAnchor(anchor_delegate_->GetAnchorView(), anchor_delegate_->GetAnchorPoint(), GetAnchorArrow()); might work here, and would reduce the number of codepaths we need to worry about) https://codereview.chromium.org/1935993004/diff/20001/chrome/browser/ui/views... chrome/browser/ui/views/website_settings/permissions_bubble_view.cc:417: platform_util::GetViewForWindow(browser_->window()->GetNativeWindow())); I think set_parent_window needs to be called in UpdateAnchorPosition() as well to handle entering fullscreen. E.g. the following should work: 1. Go to https://permission.site/ 2. Click "Location", get a prompt (leave it open) 3. Click "Fullscreen" should go into fullscreen -- Browser* stays the same, but (on Mac) this makes a new parent_window. (Location permission request should still be visible, but it should lose its arrow) 4. Press Esc to go back to not-fullscreen. Location permission request should still be visible. Those steps work fine on Windows. (On the Mac Cocoa browser it's actually horribly broken currently - eep. Step 3. moves the bubble but (a) it has an arrow (b) it's in the wrong place (c) the fullscreen window has a menubar/toolbar/locationbar when it shouldn't. Then on Step 4. the bubble disappears, and clicking "location" again doesn't show it [interestingly, switching to a different tab and back shows the bubble again]). But fixing all that might turn out to be more complicated -- the bubble may need to be hidden and reshown, or we may need to fix some of the bugs in the cocoa browser first. Maybe try it out - if a set_parent_window call in UpdateAnchorPosition fixes it then yay, but I suspect there's more to it and it's better left for a follow-up CL. https://codereview.chromium.org/1935993004/diff/20001/chrome/browser/ui/views... File chrome/browser/ui/views/website_settings/permissions_bubble_view.h (right): https://codereview.chromium.org/1935993004/diff/20001/chrome/browser/ui/views... chrome/browser/ui/views/website_settings/permissions_bubble_view.h:13: #include "chrome/browser/platform_util.h" nit: is this needed here? https://codereview.chromium.org/1935993004/diff/20001/chrome/browser/ui/views... chrome/browser/ui/views/website_settings/permissions_bubble_view.h:27: // A PermissionBubbleViewViews owns an AnchorDelegate, which is passed in when nit: update comment (e.g. the delegate is no longer passed in) https://codereview.chromium.org/1935993004/diff/20001/chrome/browser/ui/views... chrome/browser/ui/views/website_settings/permissions_bubble_view.h:35: virtual ~AnchorDelegate() {} (removing all 3 AnchorDelegate destructors should do the trick to make the compiler happy, and we can remove this) https://codereview.chromium.org/1935993004/diff/20001/chrome/browser/ui/views... chrome/browser/ui/views/website_settings/permissions_bubble_view.h:61: static std::unique_ptr<AnchorDelegate> CreateAnchorDelegate(Browser* browser); nit: needs a comment - (be sure to mention that it's implemented in platform-specific files)
tapted: ptal? :) https://codereview.chromium.org/1935993004/diff/20001/chrome/browser/ui/cocoa... File chrome/browser/ui/cocoa/website_settings/permission_bubble_anchor_delegate_views_cocoa.mm (right): https://codereview.chromium.org/1935993004/diff/20001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/website_settings/permission_bubble_anchor_delegate_views_cocoa.mm:14: #include "ui/gfx/mac/coordinate_conversion.h" On 2016/05/11 07:06:18, tapted wrote: > nit: import Done. https://codereview.chromium.org/1935993004/diff/20001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/website_settings/permission_bubble_anchor_delegate_views_cocoa.mm:16: // Implementation of PermissionBubbleViewViews' AnchorDelegate for Cocoa On 2016/05/11 07:06:18, tapted wrote: > you can start an anonymous > > namespace { > > here Done. https://codereview.chromium.org/1935993004/diff/20001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/website_settings/permission_bubble_anchor_delegate_views_cocoa.mm:22: PermissionBubbleAnchorDelegateViewsCocoa(Browser* browser); On 2016/05/11 07:06:18, tapted wrote: > nit: explicit Done. https://codereview.chromium.org/1935993004/diff/20001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/website_settings/permission_bubble_anchor_delegate_views_cocoa.mm:23: ~PermissionBubbleAnchorDelegateViewsCocoa() override; On 2016/05/11 07:06:18, tapted wrote: > does this need to be declared? the default one should work Done. https://codereview.chromium.org/1935993004/diff/20001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/website_settings/permission_bubble_anchor_delegate_views_cocoa.mm:31: DISALLOW_COPY_AND_ASSIGN(PermissionBubbleAnchorDelegateViewsCocoa); On 2016/05/11 07:06:18, tapted wrote: > nit: blank line before Done. https://codereview.chromium.org/1935993004/diff/20001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/website_settings/permission_bubble_anchor_delegate_views_cocoa.mm:59: On 2016/05/11 07:06:18, tapted wrote: > } // namespace Done. https://codereview.chromium.org/1935993004/diff/20001/chrome/browser/ui/cocoa... File chrome/browser/ui/cocoa/website_settings/permission_bubble_cocoa.mm (right): https://codereview.chromium.org/1935993004/diff/20001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/website_settings/permission_bubble_cocoa.mm:8: #include "chrome/browser/ui/browser.h" On 2016/05/11 07:06:18, tapted wrote: > I think all the 7 new headers showing up in the diff can be removed Done. https://codereview.chromium.org/1935993004/diff/20001/chrome/browser/ui/views... File chrome/browser/ui/views/website_settings/permission_bubble_anchor_delegate_views.cc (right): https://codereview.chromium.org/1935993004/diff/20001/chrome/browser/ui/views... chrome/browser/ui/views/website_settings/permission_bubble_anchor_delegate_views.cc:7: #include "chrome/browser/ui/browser_window.h" On 2016/05/11 07:06:19, tapted wrote: > there should be a lot more #includes needed than these. I regularly `git cl try` > right after uploading to ensure I've got clean runs on all the bots (cross > platform changes are hard :) > > At least the views/frame and views/location_bar stuff deleted from > permissions_bubble_view.cc should be needed Done. https://codereview.chromium.org/1935993004/diff/20001/chrome/browser/ui/views... chrome/browser/ui/views/website_settings/permission_bubble_anchor_delegate_views.cc:8: On 2016/05/11 07:06:18, tapted wrote: > namespace { Done. https://codereview.chromium.org/1935993004/diff/20001/chrome/browser/ui/views... chrome/browser/ui/views/website_settings/permission_bubble_anchor_delegate_views.cc:16: ~PermissionBubbleAnchorDelegateViews() override; On 2016/05/11 07:06:18, tapted wrote: > this probably doesn't need to be declared Done. https://codereview.chromium.org/1935993004/diff/20001/chrome/browser/ui/views... chrome/browser/ui/views/website_settings/permission_bubble_anchor_delegate_views.cc:23: } On 2016/05/11 07:06:19, tapted wrote: > > DISALLOW_COPY_AND_ASSIGN(..) > > (also there's a missing semicolon on this line) Done. https://codereview.chromium.org/1935993004/diff/20001/chrome/browser/ui/views... chrome/browser/ui/views/website_settings/permission_bubble_anchor_delegate_views.cc:29: PermissionBubbleAnchorDelegateViews::~PermissionBubbleAnchorDelegateViews() {} On 2016/05/11 07:06:19, tapted wrote: > nit (if it's kept): blank line before > > It's also fine to put simple stuff like this inline. Done. https://codereview.chromium.org/1935993004/diff/20001/chrome/browser/ui/views... chrome/browser/ui/views/website_settings/permission_bubble_anchor_delegate_views.cc:46: On 2016/05/11 07:06:18, tapted wrote: > } // namespace Done. https://codereview.chromium.org/1935993004/diff/20001/chrome/browser/ui/views... File chrome/browser/ui/views/website_settings/permissions_bubble_view.cc (right): https://codereview.chromium.org/1935993004/diff/20001/chrome/browser/ui/views... chrome/browser/ui/views/website_settings/permissions_bubble_view.cc:411: bubble_delegate_ = new PermissionsBubbleDialogDelegateView( On 2016/05/11 07:06:19, tapted wrote: > (so > > bubble_delegate_ = new PermissionsBubbleDialogDelegateView(this, requests, > values); > bubble_delegate_->UpdateAnchor(anchor_delegate_->GetAnchorView(), > anchor_delegate_->GetAnchorPoint(), GetAnchorArrow()); > > might work here, and would reduce the number of codepaths we need to worry > about) Aha, I think I finally understand what is meant here. I made this change and it does simplify the code quite a bit. https://codereview.chromium.org/1935993004/diff/20001/chrome/browser/ui/views... chrome/browser/ui/views/website_settings/permissions_bubble_view.cc:417: platform_util::GetViewForWindow(browser_->window()->GetNativeWindow())); On 2016/05/11 07:06:19, tapted wrote: > I think set_parent_window needs to be called in UpdateAnchorPosition() as well > to handle entering fullscreen. E.g. the following should work: > > 1. Go to https://permission.site/ > 2. Click "Location", get a prompt (leave it open) > 3. Click "Fullscreen" should go into fullscreen -- Browser* stays the same, but > (on Mac) this makes a new parent_window. (Location permission request should > still be visible, but it should lose its arrow) > 4. Press Esc to go back to not-fullscreen. Location permission request should > still be visible. > > Those steps work fine on Windows. > > > (On the Mac Cocoa browser it's actually horribly broken currently - eep. Step 3. > moves the bubble but (a) it has an arrow (b) it's in the wrong place (c) the > fullscreen window has a menubar/toolbar/locationbar when it shouldn't. Then on > Step 4. the bubble disappears, and clicking "location" again doesn't show it > [interestingly, switching to a different tab and back shows the bubble again]). > > > But fixing all that might turn out to be more complicated -- the bubble may need > to be hidden and reshown, or we may need to fix some of the bugs in the cocoa > browser first. Maybe try it out - if a set_parent_window call in > UpdateAnchorPosition fixes it then yay, but I suspect there's more to it and > it's better left for a follow-up CL. Just calling set_parent_window() does not seem to help. The bubble moves to just under where the toolbar "would be" if it was above the top of the screen, although the arrow does disappear. I think this behavior will need concerted fixing in a separate CL. https://codereview.chromium.org/1935993004/diff/20001/chrome/browser/ui/views... File chrome/browser/ui/views/website_settings/permissions_bubble_view.h (right): https://codereview.chromium.org/1935993004/diff/20001/chrome/browser/ui/views... chrome/browser/ui/views/website_settings/permissions_bubble_view.h:13: #include "chrome/browser/platform_util.h" On 2016/05/11 07:06:19, tapted wrote: > nit: is this needed here? Done. https://codereview.chromium.org/1935993004/diff/20001/chrome/browser/ui/views... chrome/browser/ui/views/website_settings/permissions_bubble_view.h:27: // A PermissionBubbleViewViews owns an AnchorDelegate, which is passed in when On 2016/05/11 07:06:19, tapted wrote: > nit: update comment (e.g. the delegate is no longer passed in) Done. https://codereview.chromium.org/1935993004/diff/20001/chrome/browser/ui/views... chrome/browser/ui/views/website_settings/permissions_bubble_view.h:35: virtual ~AnchorDelegate() {} On 2016/05/11 07:06:19, tapted wrote: > (removing all 3 AnchorDelegate destructors should do the trick to make the > compiler happy, and we can remove this) Done. https://codereview.chromium.org/1935993004/diff/20001/chrome/browser/ui/views... chrome/browser/ui/views/website_settings/permissions_bubble_view.h:61: static std::unique_ptr<AnchorDelegate> CreateAnchorDelegate(Browser* browser); On 2016/05/11 07:06:19, tapted wrote: > nit: needs a comment - (be sure to mention that it's implemented in > platform-specific files) Done.
If you're happy to apply the the changes in https://codereview.chromium.org/1970113002 then this lg -- see below https://codereview.chromium.org/1935993004/diff/60001/chrome/browser/ui/cocoa... File chrome/browser/ui/cocoa/website_settings/permission_bubble_anchor_delegate_views_cocoa.mm (right): https://codereview.chromium.org/1935993004/diff/60001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/website_settings/permission_bubble_anchor_delegate_views_cocoa.mm:53: point = NSMakePoint(NSMidX(frame), NSMaxY(frame)); I found some time to try this out today in between sheriff duties. As it is, this will regress http://crbug.com/604085 Also, the code as is doesn't remove the arrow from bubbles in fullscreen. To fix, we need to share more code with permisison_bubble_controller.mm, but that's pretty easy to do -- have a look at https://codereview.chromium.org/1970113002 . That CL is based off this one (after fixing the patch failure). The `GetAnchorPoint()` function becomes simply return gfx::ScreenPointFromNSPoint( [PermissionBubbleController getAnchorPointForBrowser:browser_]); But we do also need to farm out PermisisonBubbleView::GetAnchorArrow() to the delegate. that CL adds PermissionBubbleAnchorDelegate::GetAnchorArrow and implements it for views-views and views-cocoa After applying https://codereview.chromium.org/1970113002, then permission bubbles should work properly while already in fullscreen. There are still problems switching into or out of fullscreen while the permission bubble is showing, but that's not a regression (whereas the broken positioning of the permission bubble in fullscreen is a regression). https://codereview.chromium.org/1935993004/diff/60001/chrome/browser/ui/views... File chrome/browser/ui/views/website_settings/permission_bubble_anchor_delegate_views.cc (right): https://codereview.chromium.org/1935993004/diff/60001/chrome/browser/ui/views... chrome/browser/ui/views/website_settings/permission_bubble_anchor_delegate_views.cc:11: #include "ui/gfx/geometry/point.h" a few more headers were needed -- see https://codereview.chromium.org/1970113002 https://codereview.chromium.org/1935993004/diff/60001/chrome/browser/ui/views... chrome/browser/ui/views/website_settings/permission_bubble_anchor_delegate_views.cc:54: nit: // static
tapted: ptal? :) merged your changes https://codereview.chromium.org/1935993004/diff/60001/chrome/browser/ui/cocoa... File chrome/browser/ui/cocoa/website_settings/permission_bubble_anchor_delegate_views_cocoa.mm (right): https://codereview.chromium.org/1935993004/diff/60001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/website_settings/permission_bubble_anchor_delegate_views_cocoa.mm:53: point = NSMakePoint(NSMidX(frame), NSMaxY(frame)); On 2016/05/12 09:01:17, tapted wrote: > I found some time to try this out today in between sheriff duties. As it is, > this will regress http://crbug.com/604085 > > Also, the code as is doesn't remove the arrow from bubbles in fullscreen. > > To fix, we need to share more code with permisison_bubble_controller.mm, but > that's pretty easy to do -- have a look at > https://codereview.chromium.org/1970113002 . That CL is based off this one > (after fixing the patch failure). > > The `GetAnchorPoint()` function becomes simply > > return gfx::ScreenPointFromNSPoint( > [PermissionBubbleController getAnchorPointForBrowser:browser_]); > > > But we do also need to farm out PermisisonBubbleView::GetAnchorArrow() to the > delegate. that CL adds PermissionBubbleAnchorDelegate::GetAnchorArrow and > implements it for views-views and views-cocoa > > > > After applying https://codereview.chromium.org/1970113002, then permission > bubbles should work properly while already in fullscreen. > > There are still problems switching into or out of fullscreen while the > permission bubble is showing, but that's not a regression (whereas the broken > positioning of the permission bubble in fullscreen is a regression). Okay, I incorporated https://codereview.chromium.org/1970113002 into this one. Things look good. The bubble disappears during the fullscreen<->windowed transition, but reappears at the right place afterwards, which is good enough for me. https://codereview.chromium.org/1935993004/diff/60001/chrome/browser/ui/views... File chrome/browser/ui/views/website_settings/permission_bubble_anchor_delegate_views.cc (right): https://codereview.chromium.org/1935993004/diff/60001/chrome/browser/ui/views... chrome/browser/ui/views/website_settings/permission_bubble_anchor_delegate_views.cc:11: #include "ui/gfx/geometry/point.h" On 2016/05/12 09:01:18, tapted wrote: > a few more headers were needed -- see https://codereview.chromium.org/1970113002 Okay. I patched in your CL and I believe this is now fixed. https://codereview.chromium.org/1935993004/diff/60001/chrome/browser/ui/views... chrome/browser/ui/views/website_settings/permission_bubble_anchor_delegate_views.cc:54: On 2016/05/12 09:01:18, tapted wrote: > nit: // static Done.
lgtm . probably msw is the right owner for this - he is the bubble expert
ellyjones@chromium.org changed reviewers: + msw@chromium.org
msw: ptal? :)
https://codereview.chromium.org/1935993004/diff/80001/chrome/browser/ui/views... File chrome/browser/ui/views/website_settings/permissions_bubble_view.h (right): https://codereview.chromium.org/1935993004/diff/80001/chrome/browser/ui/views... chrome/browser/ui/views/website_settings/permissions_bubble_view.h:31: class AnchorDelegate { I don't really like the added complexity of this AnchorDelegate, can we just have some file-local functions in permissions_bubble_view.cc that return the view/point/arrow? namespace { views::View* GetAnchorView(Browser* browser) { #if defined(OS_MACOSX) return nullptr; #else // Inline PermissionBubbleViewViews::GetAnchorView code. } gfx::Point GetAnchorPoint(Browser* browser) { #if defined(OS_MACOSX) return gfx::ScreenPointFromNSPoint( [PermissionBubbleController getAnchorPointForBrowser:browser]); #else // An anchor view is used instead of an anchor point on non-Mac platforms. return gfx::Point(); } views::BubbleBorder::Arrow GetAnchorArrow(Browser* browser) { #if defined(OS_MACOSX) return [PermissionBubbleController hasVisibleLocationBarForBrowser:browser] ? views::BubbleBorder::TOP_LEFT : views::BubbleBorder::NONE; #else // Inline PermissionBubbleViewViews::GetAnchorArrow code. } } // namespace
msw: ptal? https://codereview.chromium.org/1935993004/diff/80001/chrome/browser/ui/views... File chrome/browser/ui/views/website_settings/permissions_bubble_view.h (right): https://codereview.chromium.org/1935993004/diff/80001/chrome/browser/ui/views... chrome/browser/ui/views/website_settings/permissions_bubble_view.h:31: class AnchorDelegate { On 2016/05/18 18:57:00, msw wrote: > I don't really like the added complexity of this AnchorDelegate, can we just > have some file-local functions in permissions_bubble_view.cc that return the > view/point/arrow? > > namespace { > > views::View* GetAnchorView(Browser* browser) { > #if defined(OS_MACOSX) > return nullptr; > #else > // Inline PermissionBubbleViewViews::GetAnchorView code. > } > > gfx::Point GetAnchorPoint(Browser* browser) { > #if defined(OS_MACOSX) > return gfx::ScreenPointFromNSPoint( > [PermissionBubbleController getAnchorPointForBrowser:browser]); > #else > // An anchor view is used instead of an anchor point on non-Mac platforms. > return gfx::Point(); > } > > views::BubbleBorder::Arrow GetAnchorArrow(Browser* browser) { > #if defined(OS_MACOSX) > return [PermissionBubbleController hasVisibleLocationBarForBrowser:browser] > ? views::BubbleBorder::TOP_LEFT > : views::BubbleBorder::NONE; > #else > // Inline PermissionBubbleViewViews::GetAnchorArrow code. > } > > } // namespace I think this technique will run into trouble for mac_views_browser=1 builds, which are OS_MACOSX, but want to use Views for everything. There's no preprocessor macro or build flag (atm) for detecting mac_views_browser=1 either :( How would you feel about this, as methods on PermissionBubbleViewViews: private views::View* GetAnchorView(Browser* browser); private gfx::Point GetAnchorPoint(Browser* browser); private views::BubbleBorder::Arrow GetAnchorArrow(Browser* browser); and then having two separate implementations of those functions, one in chrome/browser/ui/cocoa and one in chrome/browser/ui/views?
https://codereview.chromium.org/1935993004/diff/80001/chrome/browser/ui/views... File chrome/browser/ui/views/website_settings/permissions_bubble_view.h (right): https://codereview.chromium.org/1935993004/diff/80001/chrome/browser/ui/views... chrome/browser/ui/views/website_settings/permissions_bubble_view.h:31: class AnchorDelegate { On 2016/05/18 19:13:11, Elly Jones wrote: > On 2016/05/18 18:57:00, msw wrote: > > I don't really like the added complexity of this AnchorDelegate, can we just > > have some file-local functions in permissions_bubble_view.cc that return the > > view/point/arrow? > > > > namespace { > > > > views::View* GetAnchorView(Browser* browser) { > > #if defined(OS_MACOSX) > > return nullptr; > > #else > > // Inline PermissionBubbleViewViews::GetAnchorView code. > > } > > > > gfx::Point GetAnchorPoint(Browser* browser) { > > #if defined(OS_MACOSX) > > return gfx::ScreenPointFromNSPoint( > > [PermissionBubbleController getAnchorPointForBrowser:browser]); > > #else > > // An anchor view is used instead of an anchor point on non-Mac platforms. > > return gfx::Point(); > > } > > > > views::BubbleBorder::Arrow GetAnchorArrow(Browser* browser) { > > #if defined(OS_MACOSX) > > return [PermissionBubbleController hasVisibleLocationBarForBrowser:browser] > > ? views::BubbleBorder::TOP_LEFT > > : views::BubbleBorder::NONE; > > #else > > // Inline PermissionBubbleViewViews::GetAnchorArrow code. > > } > > > > } // namespace > > I think this technique will run into trouble for mac_views_browser=1 builds, > which are OS_MACOSX, but want to use Views for everything. There's no > preprocessor macro or build flag (atm) for detecting mac_views_browser=1 either > :( > > How would you feel about this, as methods on PermissionBubbleViewViews: > > private views::View* GetAnchorView(Browser* browser); > private gfx::Point GetAnchorPoint(Browser* browser); > private views::BubbleBorder::Arrow GetAnchorArrow(Browser* browser); > > and then having two separate implementations of those functions, one in > chrome/browser/ui/cocoa and one in chrome/browser/ui/views? Ah, and this views bubble is used for the cocoa browser window too? Could we rely on some runtime check like GetBrowserViewForBrowser returning null in Cocoa or getting a valid BrowserWindowCocoa? If not, your suggestion seems a bit cleaner than the current patch set, but if you disagree, I can review it as-is.
Checking at runtime doesn't work, unfortunately - some of the symbols needed for the Views implementation aren't defined at all in Cocoa builds. There have to be two separate implementations, only one of which is compiled in at a time. Anyway, I deleted the delegate, and replaced it with three per-platform methods. What do you think?
lgtm; thanks!
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/1935993004/#ps100001 (title: "delegate removed")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1935993004/100001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1935993004/100001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: chromeos_x86-generic_chromium_compile_only_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_x86-ge...)
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/1935993004/100001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1935993004/100001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_chromeos_ozone_rel_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_chromeos_rel_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
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, msw@chromium.org Link to the patchset: https://codereview.chromium.org/1935993004/#ps120001 (title: "fix typo in chrome_browser_ui.gypi")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1935993004/120001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1935993004/120001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: win_chromium_rel_ng on tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
The CQ bit was checked by ellyjones@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1935993004/120001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1935993004/120001
Message was sent while issue was closed.
Description was changed from ========== MacViews: support Views permission bubble The existing PermissionBubbleViewViews class needs an anchor view or point, so this CL adds some machinery to pass an anchor point down from the Cocoa layer, and adds a delegate so platforms can produce their own anchors. BUG=605667 ========== to ========== MacViews: support Views permission bubble The existing PermissionBubbleViewViews class needs an anchor view or point, so this CL adds some machinery to pass an anchor point down from the Cocoa layer, and adds a delegate so platforms can produce their own anchors. BUG=605667 ==========
Message was sent while issue was closed.
Committed patchset #7 (id:120001)
Message was sent while issue was closed.
Description was changed from ========== MacViews: support Views permission bubble The existing PermissionBubbleViewViews class needs an anchor view or point, so this CL adds some machinery to pass an anchor point down from the Cocoa layer, and adds a delegate so platforms can produce their own anchors. BUG=605667 ========== to ========== MacViews: support Views permission bubble The existing PermissionBubbleViewViews class needs an anchor view or point, so this CL adds some machinery to pass an anchor point down from the Cocoa layer, and adds a delegate so platforms can produce their own anchors. BUG=605667 Committed: https://crrev.com/5763588fd12942f8829b5c21300ad094bb0e5203 Cr-Commit-Position: refs/heads/master@{#395850} ==========
Message was sent while issue was closed.
Patchset 7 (id:??) landed as https://crrev.com/5763588fd12942f8829b5c21300ad094bb0e5203 Cr-Commit-Position: refs/heads/master@{#395850} |
