MacViews: Reposition permission bubble on browser resize.

chrome/browser/ui/cocoa/browser_window_controller.mm

 // Copyright 2012 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #import "chrome/browser/ui/cocoa/browser_window_controller.h"
 
 #include <cmath>
 #include <numeric>
 #include <utility>
 
@@ skipping 1695 matching lines @@
 // Delegate method called when window is resized.
 - (void)windowDidResize:(NSNotification*)notification {
   [self saveWindowPositionIfNeeded];
 
   // Resize (and possibly move) the status bubble. Note that we may get called
   // when the status bubble does not exist.
   if (statusBubble_) {
     statusBubble_->UpdateSizeAndPosition();
   }
 
+  // Reposition the permission bubble if it exists.
+  PermissionRequestManager* manager = [self permissionRequestManager];
+  if (manager)
+    manager->UpdateAnchorPosition();

[tapted, 2016/08/08 07:10:18]

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)

[karandeepb, 2016/08/08 10:37:29]

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?
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).

[tapted, 2016/08/09 01:58:39]

Don't know of a bug for it - it's pretty obscure. There may be other things
using BaseBubbleController that are similarly affected though.
well.. it probably does if a find bar is showing and calls layoutSubviews :)
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.

[karandeepb, 2016/08/09 07:51:32]

Will file a bug for it.
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.
See comment on top.
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?

[tapted, 2016/08/10 01:27:20]

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.
I think not yet -- just permission bubbles. But we will soon when the extension
bubbles get ported.

+
   // The FindBar needs to know its own position to properly detect overlaps
   // with find results. The position changes whenever the window is resized,
   // and |layoutSubviews| computes the FindBar's position.
   // TODO: calling |layoutSubviews| here is a waste, find a better way to
   // do this.
   if ([findBarCocoaController_ isFindBarVisible])
     [self layoutSubviews];
 }
 
 // Delegate method called when window did move. (See below for why we don't use
@@ skipping 329 matching lines @@
 
 - (NSRect)savedRegularWindowFrame {
   return savedRegularWindowFrame_;
 }
 
 - (BOOL)isFullscreenTransitionInProgress {
   return enteringAppKitFullscreen_ || exitingAppKitFullscreen_;
 }
 
 @end  // @implementation BrowserWindowController(WindowType)