Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2227513002:
MacViews: Reposition permission bubble on browser resize. (Closed)
|
Created:
4 years, 4 months ago by karandeepb Modified:
4 years, 4 months ago Reviewers:
tapted CC:
chromium-reviews, chrome-apps-syd-reviews_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionMacViews: Reposition permission bubble on browser resize.
On MacViews currently, permission bubble does not reposition itself on resizing
the browser. The Cocoa implementation for PermissionPrompt
(PermissionBubbleCocoa) has a PermissionBubbleController instance which listens
for window resize notifications. On a Views browser, the repositioning is
facilitated by the call to BubbleDialogDelegateView::OnWidgetBoundsChanged.
But these approaches don't work for MacViews since PermissionPromptImpl does not
have a PermissionBubbleController instance. Also, on the Cocoa browser
BubbleDialogDelegateView::OnWidgetBoundsChanged won't be fired on a browser
window resize.
This CL updates windowDidResize: method on BrowserWindowController to inform the
PermissionRequestManager to update the permission bubble position. It does this
by introducing a new method updatePermissionBubbleAnchor on
BrowserWindowController (Private). This also removes the need to override
parentWindowDidResize: on PermissionBubbleController.
BUG=616006
TEST=Go to https://permission.site/. Ensure permission setting for Notifications
is "Ask By Default". Click on Notifications. Permission bubble should appear.
Resize the browser window by dragging the mouse. Ensure the permission bubble
repositions itself correctly. Try with and without
chrome://flags/#mac-views-webui-dialogs enabled.
Committed: https://crrev.com/0b3631682321c7dee86957078c9c070a01bb3821
Cr-Commit-Position: refs/heads/master@{#410956}
Patch Set 1 #
Total comments: 9
Patch Set 2 : Address review. #Patch Set 3 : Add comment to parentWindowDidResize. #
Total comments: 2
Patch Set 4 : Remove redundant comment. #
Messages
Total messages: 26 (16 generated)
The CQ bit was checked by karandeepb@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Description was changed from ========== PermissionBubbleFix 1 ========== to ========== MacViews: Reposition permission bubble on browser resize. On MacViews currently, permission bubble does not reposition itself on resizing the browser. The Cocoa implementation for PermissionPrompt (PermissionBubbleCocoa) has a PermissionBubbleController instance which listens for window resize notifications. On a Views browser, the repositioning is facilitated by the call to BubbleDialogDelegateView::OnWidgetBoundsChanged. But these approaches don't work for MacViews since PermissionPromptImpl does not have a PermissionBubbleController instance. Also, on the Cocoa browser BubbleDialogDelegateView::OnWidgetBoundsChanged won't be fired on a browser window resize. This CL updates windowDidResize: method on BrowserWindowController to inform the PermissionRequestManager to update the permission bubble position. This also removes the need to override parentWindowDidResize: on PermissionBubbleController. BUG=616006 TEST=Go to https://permission.site/. Ensure permission setting for Notifications is "Ask By Default". Click on Notifications. Permission bubble should appear. Resize the browser window by dragging the mouse. Ensure the permission bubble repositions itself correctly. Try with and without chrome://flags/#mac-views-webui-dialogs enabled. ==========
Description was changed from ========== MacViews: Reposition permission bubble on browser resize. On MacViews currently, permission bubble does not reposition itself on resizing the browser. The Cocoa implementation for PermissionPrompt (PermissionBubbleCocoa) has a PermissionBubbleController instance which listens for window resize notifications. On a Views browser, the repositioning is facilitated by the call to BubbleDialogDelegateView::OnWidgetBoundsChanged. But these approaches don't work for MacViews since PermissionPromptImpl does not have a PermissionBubbleController instance. Also, on the Cocoa browser BubbleDialogDelegateView::OnWidgetBoundsChanged won't be fired on a browser window resize. This CL updates windowDidResize: method on BrowserWindowController to inform the PermissionRequestManager to update the permission bubble position. This also removes the need to override parentWindowDidResize: on PermissionBubbleController. BUG=616006 TEST=Go to https://permission.site/. Ensure permission setting for Notifications is "Ask By Default". Click on Notifications. Permission bubble should appear. Resize the browser window by dragging the mouse. Ensure the permission bubble repositions itself correctly. Try with and without chrome://flags/#mac-views-webui-dialogs enabled. ========== to ========== MacViews: Reposition permission bubble on browser resize. On MacViews currently, permission bubble does not reposition itself on resizing the browser. The Cocoa implementation for PermissionPrompt (PermissionBubbleCocoa) has a PermissionBubbleController instance which listens for window resize notifications. On a Views browser, the repositioning is facilitated by the call to BubbleDialogDelegateView::OnWidgetBoundsChanged. But these approaches don't work for MacViews since PermissionPromptImpl does not have a PermissionBubbleController instance. Also, on the Cocoa browser BubbleDialogDelegateView::OnWidgetBoundsChanged won't be fired on a browser window resize. This CL updates windowDidResize: method on BrowserWindowController to inform the PermissionRequestManager to update the permission bubble position. This also removes the need to override parentWindowDidResize: on PermissionBubbleController. BUG=616006 TEST=Go to https://permission.site/. Ensure permission setting for Notifications is "Ask By Default". Click on Notifications. Permission bubble should appear. Resize the browser window by dragging the mouse. Ensure the permission bubble repositions itself correctly. Try with and without chrome://flags/#mac-views-webui-dialogs enabled. ==========
karandeepb@chromium.org changed reviewers: + tapted@chromium.org
PTAL Trent.
https://codereview.chromium.org/2227513002/diff/1/chrome/browser/ui/cocoa/bro... File chrome/browser/ui/cocoa/browser_window_controller.mm (right): https://codereview.chromium.org/2227513002/diff/1/chrome/browser/ui/cocoa/bro... chrome/browser/ui/cocoa/browser_window_controller.mm:1719: manager->UpdateAnchorPosition(); This code fragment is in 3 places now... it's possible some other bubbles will miss out. E.g. if I go to chrome://extensions/, tick developer mode, and chrome://restart, I get that "Disable Developer Mode Extensions" popup - it goes a bit bonkers when resizing from the top left (that's for a different reason though -- basically BaseBubbleController::parentWindowDidResize is buggy) WDYT of a new method on BrowserWindowController (Private) - something like - (void)updateBubbleAnchors { PermissionRequestManager* manager = [self permissionRequestManager]; if (manager) manager->UpdateAnchorPosition(); browser_->GetBubbleManager()->UpdateAllBubbleAnchors(); } (it's also sad that PermissionRequestManager duplicates a whole bunch of stuff in BubbleManager, but it looks like merging them is still a work in progress .. and I guess BaseBubbleController should also disappear, but MacViews will make that obsolete anyways) https://codereview.chromium.org/2227513002/diff/1/chrome/browser/ui/cocoa/web... File chrome/browser/ui/cocoa/website_settings/permission_bubble_controller.mm (right): https://codereview.chromium.org/2227513002/diff/1/chrome/browser/ui/cocoa/web... chrome/browser/ui/cocoa/website_settings/permission_bubble_controller.mm:302: - (void)parentWindowDidResize:(NSNotification*)notification { we can just delete this - it's overriding a "private" method of BaseBubbleController anyway - ick
The CQ bit was checked by karandeepb@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Description was changed from ========== MacViews: Reposition permission bubble on browser resize. On MacViews currently, permission bubble does not reposition itself on resizing the browser. The Cocoa implementation for PermissionPrompt (PermissionBubbleCocoa) has a PermissionBubbleController instance which listens for window resize notifications. On a Views browser, the repositioning is facilitated by the call to BubbleDialogDelegateView::OnWidgetBoundsChanged. But these approaches don't work for MacViews since PermissionPromptImpl does not have a PermissionBubbleController instance. Also, on the Cocoa browser BubbleDialogDelegateView::OnWidgetBoundsChanged won't be fired on a browser window resize. This CL updates windowDidResize: method on BrowserWindowController to inform the PermissionRequestManager to update the permission bubble position. This also removes the need to override parentWindowDidResize: on PermissionBubbleController. BUG=616006 TEST=Go to https://permission.site/. Ensure permission setting for Notifications is "Ask By Default". Click on Notifications. Permission bubble should appear. Resize the browser window by dragging the mouse. Ensure the permission bubble repositions itself correctly. Try with and without chrome://flags/#mac-views-webui-dialogs enabled. ========== to ========== MacViews: Reposition permission bubble on browser resize. On MacViews currently, permission bubble does not reposition itself on resizing the browser. The Cocoa implementation for PermissionPrompt (PermissionBubbleCocoa) has a PermissionBubbleController instance which listens for window resize notifications. On a Views browser, the repositioning is facilitated by the call to BubbleDialogDelegateView::OnWidgetBoundsChanged. But these approaches don't work for MacViews since PermissionPromptImpl does not have a PermissionBubbleController instance. Also, on the Cocoa browser BubbleDialogDelegateView::OnWidgetBoundsChanged won't be fired on a browser window resize. This CL updates windowDidResize: method on BrowserWindowController to inform the PermissionRequestManager to update the permission bubble position. It does this by introducing a new method updatePermissionBubbleAnchor on BrowserWindowController (Private). This also removes the need to override parentWindowDidResize: on PermissionBubbleController. BUG=616006 TEST=Go to https://permission.site/. Ensure permission setting for Notifications is "Ask By Default". Click on Notifications. Permission bubble should appear. Resize the browser window by dragging the mouse. Ensure the permission bubble repositions itself correctly. Try with and without chrome://flags/#mac-views-webui-dialogs enabled. ==========
PTAL Trent. https://codereview.chromium.org/2227513002/diff/1/chrome/browser/ui/cocoa/bro... File chrome/browser/ui/cocoa/browser_window_controller.mm (right): https://codereview.chromium.org/2227513002/diff/1/chrome/browser/ui/cocoa/bro... chrome/browser/ui/cocoa/browser_window_controller.mm:1719: manager->UpdateAnchorPosition(); >E.g. if I go to chrome://extensions/, tick developer mode, and >chrome://restart, I get that "Disable Developer Mode Extensions" popup - it goes >a bit bonkers when resizing from the top left This is also a bug on stable, though can't seem to find a bug report. Do you know of one? Else I'll file a new bug report. Also, for the MacViews version of this bubble, is it behind a flag? >WDYT of a new method on BrowserWindowController (Private) - something like >- (void)updateBubbleAnchors { > PermissionRequestManager* manager = [self permissionRequestManager]; > if (manager) > manager->UpdateAnchorPosition(); > browser_->GetBubbleManager()->UpdateAllBubbleAnchors(); >} Currently BrowserWindowController's windowDidResize doesn't make a call to browser_->GetBubbleManager()->UpdateAllBubbleAnchors(), which means these bubbles, managed by the BubbleManager probably are depending on the windowDidResize notification. Calling browser_->GetBubbleManager()->UpdateAllBubbleAnchors() would then lead to multiple calls to these bubbles to update their anchor positions for the same resize. Have introduced a new function updatePermissionBubbleAnchor on BrowserWindowController (Private). https://codereview.chromium.org/2227513002/diff/1/chrome/browser/ui/cocoa/web... File chrome/browser/ui/cocoa/website_settings/permission_bubble_controller.mm (right): https://codereview.chromium.org/2227513002/diff/1/chrome/browser/ui/cocoa/web... chrome/browser/ui/cocoa/website_settings/permission_bubble_controller.mm:302: - (void)parentWindowDidResize:(NSNotification*)notification { On 2016/08/08 07:10:18, tapted wrote: > we can just delete this - it's overriding a "private" method of > BaseBubbleController anyway - ick Wouldn't that make a call to the base class version and we probably don't want that. Although there are no "observable" differences in behavior on removing this.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2227513002/diff/1/chrome/browser/ui/cocoa/bro... File chrome/browser/ui/cocoa/browser_window_controller.mm (right): https://codereview.chromium.org/2227513002/diff/1/chrome/browser/ui/cocoa/bro... chrome/browser/ui/cocoa/browser_window_controller.mm:1719: manager->UpdateAnchorPosition(); On 2016/08/08 10:37:29, karandeepb wrote: > >E.g. if I go to chrome://extensions/, tick developer mode, and > >chrome://restart, I get that "Disable Developer Mode Extensions" popup - it > goes > >a bit bonkers when resizing from the top left > > This is also a bug on stable, though can't seem to find a bug report. Do you > know of one? Else I'll file a new bug report. Also, for the MacViews version of > this bubble, is it behind a flag? Don't know of a bug for it - it's pretty obscure. There may be other things using BaseBubbleController that are similarly affected though. > > >WDYT of a new method on BrowserWindowController (Private) - something like > >- (void)updateBubbleAnchors { > > PermissionRequestManager* manager = [self permissionRequestManager]; > > if (manager) > > manager->UpdateAnchorPosition(); > > browser_->GetBubbleManager()->UpdateAllBubbleAnchors(); > >} > > Currently BrowserWindowController's windowDidResize doesn't make a call to > browser_->GetBubbleManager()->UpdateAllBubbleAnchors(), ' well.. it probably does if a find bar is showing and calls layoutSubviews :) > which means these > bubbles, managed by the BubbleManager > probably are depending on the windowDidResize notification. Calling > browser_->GetBubbleManager()->UpdateAllBubbleAnchors() would then lead to > multiple calls to these bubbles to update their anchor positions for the same > resize. Have introduced a new function updatePermissionBubbleAnchor on > BrowserWindowController (Private). > I can't find any callers of UpdateAllBubbleAnchors except for the one in layoutSubviews, one in TabDetachedAt and one in DidToggleFullscreenModeForTab. Possibilities might be - There are no bubbles on Mac using BubbleManager, or they are only non-persistent bubbles that disappear when a resize begins - There's a bug for these bubbles and nobody's noticed - There is some way the bubbles update other than UpdateAllBubbleAnchors The UpdateAllBubbleAnchors was added in https://codereview.chromium.org/1251633002 but BubbleManager wasn't very far along then - it's possible some codepaths were not tested. Looking at the BubbleManager bug, (http://crbug.com/496955) there is the extension installed bubble opted in. It's not persistent, but if you start installing an extension (e.g. https://chrome.google.com/webstore/detail/google-keep-chrome-extens/lpcaedmch...) then start resizing before the install completes (and keep resizing) the bubble will appear and it is buggy. But! Showing a Find bar doesn't fix this, so there might be something else going on. basically, there are a lot of codepaths for updating bubble anchors. To fix the permission bubble bug with MacViews we want to add yet another, so we should work towards consolidating it with some other codepaths. https://codereview.chromium.org/2227513002/diff/1/chrome/browser/ui/cocoa/web... File chrome/browser/ui/cocoa/website_settings/permission_bubble_controller.mm (right): https://codereview.chromium.org/2227513002/diff/1/chrome/browser/ui/cocoa/web... chrome/browser/ui/cocoa/website_settings/permission_bubble_controller.mm:302: - (void)parentWindowDidResize:(NSNotification*)notification { On 2016/08/08 10:37:29, karandeepb wrote: > On 2016/08/08 07:10:18, tapted wrote: > > we can just delete this - it's overriding a "private" method of > > BaseBubbleController anyway - ick > > Wouldn't that make a call to the base class version and we probably don't want > that. Although there are no "observable" differences in behavior on removing > this. Ah, yeh there might be a race depending on observer order. Let's also make it clear we're overriding the base class implementation (like parentWindowWillToggleFullScreen:)
PTAL Trent. https://codereview.chromium.org/2227513002/diff/1/chrome/browser/ui/cocoa/bro... File chrome/browser/ui/cocoa/browser_window_controller.mm (right): https://codereview.chromium.org/2227513002/diff/1/chrome/browser/ui/cocoa/bro... chrome/browser/ui/cocoa/browser_window_controller.mm:1719: manager->UpdateAnchorPosition(); >Don't know of a bug for it - it's pretty obscure. There may be other things >using BaseBubbleController that are similarly affected though. Will file a bug for it. >I can't find any callers of UpdateAllBubbleAnchors except for the one in >layoutSubviews, one in TabDetachedAt and one in DidToggleFullscreenModeForTab. >Possibilities might be > - There are no bubbles on Mac using BubbleManager, or they are only >non-persistent bubbles that disappear when a resize begins > - There's a bug for these bubbles and nobody's noticed > - There is some way the bubbles update other than UpdateAllBubbleAnchors So on Cocoa searching for BubbleUi (which the BubbleManager uses) on .mm files only brings up ChooserBubbleUiCocoa and the ExtensionInstallBubble. The first seems to be a work in progress (isn't included from any other file) and the second is the bubble you mentioned below. But it seems its updateAnchorPosition is also broken. Calling it on a resize does not fix the bubble position. Also these bubbles have controllers which also inherit from BaseBubbleController. So they also do get the resize notification and their base class' parentWindowDidResize is executed for them. >Looking at the BubbleManager bug, (http://crbug.com/496955) there is the >extension installed bubble opted in. It's not persistent, but if you start >installing an extension (e.g. >https://chrome.google.com/webstore/detail/google-keep-chrome-extens/lpcaedmch...) >then start resizing before the install completes (and keep resizing) the bubble >will appear and it is buggy. > >But! Showing a Find bar doesn't fix this, so there might be something else going >on. See comment on top. >basically, there are a lot of codepaths for updating bubble anchors. To fix the >permission bubble bug with MacViews we want to add yet another, so we should >work towards consolidating it with some other codepaths. I think most/all the bubbles on Cocoa have controllers which inherit from BaseBubbleController, so their base class atleast knows of a resize. The browser_->GetBubbleManager()->UpdateAllBubbleAnchors(); in layoutSubviews was added in https://codereview.chromium.org/1251633002, which did not involve any other Cocoa changes and is probably redundant. Maybe we can also listen to the resize notification, but don't think that'll fit nicely with how the code is structured currently. For MacViews on Cocoa browser, do we have any other persistent bubbles or those which may need to be notified of a resize? https://codereview.chromium.org/2227513002/diff/1/chrome/browser/ui/cocoa/web... File chrome/browser/ui/cocoa/website_settings/permission_bubble_controller.mm (right): https://codereview.chromium.org/2227513002/diff/1/chrome/browser/ui/cocoa/web... chrome/browser/ui/cocoa/website_settings/permission_bubble_controller.mm:302: - (void)parentWindowDidResize:(NSNotification*)notification { On 2016/08/09 01:58:39, tapted wrote: > On 2016/08/08 10:37:29, karandeepb wrote: > > On 2016/08/08 07:10:18, tapted wrote: > > > we can just delete this - it's overriding a "private" method of > > > BaseBubbleController anyway - ick > > > > Wouldn't that make a call to the base class version and we probably don't want > > that. Although there are no "observable" differences in behavior on removing > > this. > > Ah, yeh there might be a race depending on observer order. Let's also make it > clear we're overriding the base class implementation (like > parentWindowWillToggleFullScreen:) Done.
lgtm % nit. Thanks for investigating! https://codereview.chromium.org/2227513002/diff/1/chrome/browser/ui/cocoa/bro... File chrome/browser/ui/cocoa/browser_window_controller.mm (right): https://codereview.chromium.org/2227513002/diff/1/chrome/browser/ui/cocoa/bro... chrome/browser/ui/cocoa/browser_window_controller.mm:1719: manager->UpdateAnchorPosition(); On 2016/08/09 07:51:32, karandeepb wrote: > >Don't know of a bug for it - it's pretty obscure. There may be other things > >using BaseBubbleController that are similarly affected though. > > Will file a bug for it. > > >I can't find any callers of UpdateAllBubbleAnchors except for the one in > >layoutSubviews, one in TabDetachedAt and one in DidToggleFullscreenModeForTab. > >Possibilities might be > > - There are no bubbles on Mac using BubbleManager, or they are only > >non-persistent bubbles that disappear when a resize begins > > - There's a bug for these bubbles and nobody's noticed > > - There is some way the bubbles update other than UpdateAllBubbleAnchors > > So on Cocoa searching for BubbleUi (which the BubbleManager uses) on .mm files > only brings up ChooserBubbleUiCocoa and the ExtensionInstallBubble. The first > seems to be a work in progress (isn't included from any other file) and the > second is the bubble you mentioned below. But it seems its updateAnchorPosition > is also broken. Calling it on a resize does not fix the bubble position. Also > these bubbles have controllers which also inherit from BaseBubbleController. So > they also do get the resize notification and their base class' > parentWindowDidResize is executed for them. Yah - I think BaseBubbleController's updateAnchor just has a bug - it's getting called, but it always assumes the origin of the parent window is fixed. I guess that was true in Snow Leopard when you could only resize windows from the bottom right corner, but that hasn't been the case for a while now. > >Looking at the BubbleManager bug, (http://crbug.com/496955) there is the > >extension installed bubble opted in. It's not persistent, but if you start > >installing an extension (e.g. > >https://chrome.google.com/webstore/detail/google-keep-chrome-extens/lpcaedmch...) > >then start resizing before the install completes (and keep resizing) the bubble > >will appear and it is buggy. > > > >But! Showing a Find bar doesn't fix this, so there might be something else > going > >on. > > See comment on top. > > > >basically, there are a lot of codepaths for updating bubble anchors. To fix the > >permission bubble bug with MacViews we want to add yet another, so we should > >work towards consolidating it with some other codepaths. > > I think most/all the bubbles on Cocoa have controllers which inherit from > BaseBubbleController, so their base class atleast knows of a resize. The > browser_->GetBubbleManager()->UpdateAllBubbleAnchors(); in layoutSubviews was > added in https://codereview.chromium.org/1251633002, which did not involve any > other Cocoa changes and is probably redundant. Maybe we can also listen to the > resize notification, but don't think that'll fit nicely with how the code is > structured currently. > > For MacViews on Cocoa browser, do we have any other persistent bubbles or those > which may need to be notified of a resize? I think not yet -- just permission bubbles. But we will soon when the extension bubbles get ported. https://codereview.chromium.org/2227513002/diff/40001/chrome/browser/ui/cocoa... File chrome/browser/ui/cocoa/browser_window_controller_private.mm (right): https://codereview.chromium.org/2227513002/diff/40001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/browser_window_controller_private.mm:422: // Updates the permission bubble position. nit: this comment is obsolete with the method comment for updatePermissionBubbleAnchor];
https://codereview.chromium.org/2227513002/diff/40001/chrome/browser/ui/cocoa... File chrome/browser/ui/cocoa/browser_window_controller_private.mm (right): https://codereview.chromium.org/2227513002/diff/40001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/browser_window_controller_private.mm:422: // Updates the permission bubble position. On 2016/08/10 01:27:20, tapted wrote: > nit: this comment is obsolete with the method comment for > updatePermissionBubbleAnchor]; Done.
The CQ bit was checked by karandeepb@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/2227513002/#ps60001 (title: "Remove redundant comment.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Description was changed from ========== MacViews: Reposition permission bubble on browser resize. On MacViews currently, permission bubble does not reposition itself on resizing the browser. The Cocoa implementation for PermissionPrompt (PermissionBubbleCocoa) has a PermissionBubbleController instance which listens for window resize notifications. On a Views browser, the repositioning is facilitated by the call to BubbleDialogDelegateView::OnWidgetBoundsChanged. But these approaches don't work for MacViews since PermissionPromptImpl does not have a PermissionBubbleController instance. Also, on the Cocoa browser BubbleDialogDelegateView::OnWidgetBoundsChanged won't be fired on a browser window resize. This CL updates windowDidResize: method on BrowserWindowController to inform the PermissionRequestManager to update the permission bubble position. It does this by introducing a new method updatePermissionBubbleAnchor on BrowserWindowController (Private). This also removes the need to override parentWindowDidResize: on PermissionBubbleController. BUG=616006 TEST=Go to https://permission.site/. Ensure permission setting for Notifications is "Ask By Default". Click on Notifications. Permission bubble should appear. Resize the browser window by dragging the mouse. Ensure the permission bubble repositions itself correctly. Try with and without chrome://flags/#mac-views-webui-dialogs enabled. ========== to ========== MacViews: Reposition permission bubble on browser resize. On MacViews currently, permission bubble does not reposition itself on resizing the browser. The Cocoa implementation for PermissionPrompt (PermissionBubbleCocoa) has a PermissionBubbleController instance which listens for window resize notifications. On a Views browser, the repositioning is facilitated by the call to BubbleDialogDelegateView::OnWidgetBoundsChanged. But these approaches don't work for MacViews since PermissionPromptImpl does not have a PermissionBubbleController instance. Also, on the Cocoa browser BubbleDialogDelegateView::OnWidgetBoundsChanged won't be fired on a browser window resize. This CL updates windowDidResize: method on BrowserWindowController to inform the PermissionRequestManager to update the permission bubble position. It does this by introducing a new method updatePermissionBubbleAnchor on BrowserWindowController (Private). This also removes the need to override parentWindowDidResize: on PermissionBubbleController. BUG=616006 TEST=Go to https://permission.site/. Ensure permission setting for Notifications is "Ask By Default". Click on Notifications. Permission bubble should appear. Resize the browser window by dragging the mouse. Ensure the permission bubble repositions itself correctly. Try with and without chrome://flags/#mac-views-webui-dialogs enabled. ==========
Message was sent while issue was closed.
Committed patchset #4 (id:60001)
Message was sent while issue was closed.
Description was changed from ========== MacViews: Reposition permission bubble on browser resize. On MacViews currently, permission bubble does not reposition itself on resizing the browser. The Cocoa implementation for PermissionPrompt (PermissionBubbleCocoa) has a PermissionBubbleController instance which listens for window resize notifications. On a Views browser, the repositioning is facilitated by the call to BubbleDialogDelegateView::OnWidgetBoundsChanged. But these approaches don't work for MacViews since PermissionPromptImpl does not have a PermissionBubbleController instance. Also, on the Cocoa browser BubbleDialogDelegateView::OnWidgetBoundsChanged won't be fired on a browser window resize. This CL updates windowDidResize: method on BrowserWindowController to inform the PermissionRequestManager to update the permission bubble position. It does this by introducing a new method updatePermissionBubbleAnchor on BrowserWindowController (Private). This also removes the need to override parentWindowDidResize: on PermissionBubbleController. BUG=616006 TEST=Go to https://permission.site/. Ensure permission setting for Notifications is "Ask By Default". Click on Notifications. Permission bubble should appear. Resize the browser window by dragging the mouse. Ensure the permission bubble repositions itself correctly. Try with and without chrome://flags/#mac-views-webui-dialogs enabled. ========== to ========== MacViews: Reposition permission bubble on browser resize. On MacViews currently, permission bubble does not reposition itself on resizing the browser. The Cocoa implementation for PermissionPrompt (PermissionBubbleCocoa) has a PermissionBubbleController instance which listens for window resize notifications. On a Views browser, the repositioning is facilitated by the call to BubbleDialogDelegateView::OnWidgetBoundsChanged. But these approaches don't work for MacViews since PermissionPromptImpl does not have a PermissionBubbleController instance. Also, on the Cocoa browser BubbleDialogDelegateView::OnWidgetBoundsChanged won't be fired on a browser window resize. This CL updates windowDidResize: method on BrowserWindowController to inform the PermissionRequestManager to update the permission bubble position. It does this by introducing a new method updatePermissionBubbleAnchor on BrowserWindowController (Private). This also removes the need to override parentWindowDidResize: on PermissionBubbleController. BUG=616006 TEST=Go to https://permission.site/. Ensure permission setting for Notifications is "Ask By Default". Click on Notifications. Permission bubble should appear. Resize the browser window by dragging the mouse. Ensure the permission bubble repositions itself correctly. Try with and without chrome://flags/#mac-views-webui-dialogs enabled. Committed: https://crrev.com/0b3631682321c7dee86957078c9c070a01bb3821 Cr-Commit-Position: refs/heads/master@{#410956} ==========
Message was sent while issue was closed.
Patchset 4 (id:??) landed as https://crrev.com/0b3631682321c7dee86957078c9c070a01bb3821 Cr-Commit-Position: refs/heads/master@{#410956} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
