Mac Changes for BubbleManager

Mac Changes for BubbleManager

BUG=496955

[hcarmona, 2015-08-14 17:48:08]

hcarmona@chromium.org changed reviewers:
+ groby@chromium.org, rouslan@chromium.org

[hcarmona, 2015-08-14 17:48:08]

Mac Changes (Not updated with yesterday's changes form msw@)

[hcarmona, 2015-08-14 17:49:12]

On 2015/08/14 17:48:08, Hector Carmona wrote:
> Mac Changes (Not updated with yesterday's changes form msw@)

Ignore all unit test files, I had just tagged them to fix. No changes there.

[groby-ooo-7-16, 2015-08-14 18:15:14]

Could you upload a CL that's based on the Views/PermissionBubble changes, just
so they don't show up?

[groby-ooo-7-16, 2015-08-14 18:26:05]

And a few notes

https://codereview.chromium.org/1292353006/diff/1/chrome/browser/ui/cocoa/bro...
File chrome/browser/ui/cocoa/browser_window_cocoa.mm (right):

https://codereview.chromium.org/1292353006/diff/1/chrome/browser/ui/cocoa/bro...
chrome/browser/ui/cocoa/browser_window_cocoa.mm:713: if (event.windowsKeyCode ==
ui::VKEY_ESCAPE)
Ugh. That's a big one. I'd rather we don't touch that for a moment. Can you keep
it dismissPermissionBubble and open a thread with egm/aisnlie where we discuss
if we want that for all bubbles? 

(I.e. dismissing the bubble if you press ESC, even if it doesn't have focus)

https://codereview.chromium.org/1292353006/diff/1/chrome/browser/ui/cocoa/bro...
File chrome/browser/ui/cocoa/browser_window_controller.mm (right):

https://codereview.chromium.org/1292353006/diff/1/chrome/browser/ui/cocoa/bro...
chrome/browser/ui/cocoa/browser_window_controller.mm:1301:
manager->TabFocus(newContents);
I believe we talked about having a manager per browser, instead of per
browsercontext?

If that's the case, we could move this onto Browser::ActiveTabChanged and handle
this in a platform-neutral way.

https://codereview.chromium.org/1292353006/diff/1/chrome/browser/ui/cocoa/bro...
File chrome/browser/ui/cocoa/browser_window_controller_private.mm (right):

https://codereview.chromium.org/1292353006/diff/1/chrome/browser/ui/cocoa/bro...
chrome/browser/ui/cocoa/browser_window_controller_private.mm:240:
->UpdateAnchorPosition(contents);
Ideally, I'd like a way to do this platform-neutral. Can we move that
functionality on the Bubble(controller)?

https://codereview.chromium.org/1292353006/diff/1/chrome/browser/ui/cocoa/bro...
chrome/browser/ui/cocoa/browser_window_controller_private.mm:452: *
TODO(hcarmona): bring this back
Well, that's kind of a huge part of the cocoa side :)

Also, ideally, that's handled in the bubble manager for all bubbles that survive
fullscreen mode.

https://codereview.chromium.org/1292353006/diff/1/chrome/browser/ui/cocoa/web...
File chrome/browser/ui/cocoa/website_settings/permission_bubble_cocoa.mm
(right):

https://codereview.chromium.org/1292353006/diff/1/chrome/browser/ui/cocoa/web...
chrome/browser/ui/cocoa/website_settings/permission_bubble_cocoa.mm:30: if
(!bubbleController_) {
Can we migrate this out so it's shared across bubbles?

[hcarmona, 2015-08-14 18:28:30]

Patchset #2 (id:20001) has been deleted

[hcarmona, 2015-08-18 02:22:21]

https://codereview.chromium.org/1292353006/diff/1/chrome/browser/ui/cocoa/bro...
File chrome/browser/ui/cocoa/browser_window_cocoa.mm (right):

https://codereview.chromium.org/1292353006/diff/1/chrome/browser/ui/cocoa/bro...
chrome/browser/ui/cocoa/browser_window_cocoa.mm:713: if (event.windowsKeyCode ==
ui::VKEY_ESCAPE)
On 2015/08/14 18:26:04, groby wrote:
> Ugh. That's a big one. I'd rather we don't touch that for a moment. Can you
keep
> it dismissPermissionBubble and open a thread with egm/aisnlie where we discuss
> if we want that for all bubbles? 
> 
> (I.e. dismissing the bubble if you press ESC, even if it doesn't have focus)

Done.

https://codereview.chromium.org/1292353006/diff/1/chrome/browser/ui/cocoa/bro...
File chrome/browser/ui/cocoa/browser_window_controller.mm (right):

https://codereview.chromium.org/1292353006/diff/1/chrome/browser/ui/cocoa/bro...
chrome/browser/ui/cocoa/browser_window_controller.mm:1301:
manager->TabFocus(newContents);
On 2015/08/14 18:26:04, groby wrote:
> I believe we talked about having a manager per browser, instead of per
> browsercontext?
> 
> If that's the case, we could move this onto Browser::ActiveTabChanged and
handle
> this in a platform-neutral way.

Done. Moved to the browser and no longer part of the browser window.

https://codereview.chromium.org/1292353006/diff/1/chrome/browser/ui/cocoa/bro...
File chrome/browser/ui/cocoa/browser_window_controller_private.mm (right):

https://codereview.chromium.org/1292353006/diff/1/chrome/browser/ui/cocoa/bro...
chrome/browser/ui/cocoa/browser_window_controller_private.mm:452: *
TODO(hcarmona): bring this back
On 2015/08/14 18:26:04, groby wrote:
> Well, that's kind of a huge part of the cocoa side :)
> 
> Also, ideally, that's handled in the bubble manager for all bubbles that
survive
> fullscreen mode.

Yes, it's a big part. How should we handle this? Currently, the bubble's
native view is exposed to this code. I'm not a big fan of that because
it's exposing implementation details about bubbles for this. I also
would like to avoid adding code to the bubble API that's just for cocoa,
but that may be unavoidable.