[PermissionBubble] Remove bubble from previous manager.

[PermissionBubble] Remove bubble from previous manager.

Properly transfer ownership between PermissionBubbleManagers as
tabs are switched.

BUG=340720
R=rohitrao@chromium.org

Committed: https://crrev.com/a0a825ff6e71576850e25a2a7e8db4fb32d671a6
Cr-Commit-Position: refs/heads/master@{#330208}

[rohitrao (ping after 24h), 2015-05-15 18:38:02]

https://codereview.chromium.org/1130603005/diff/40001/chrome/browser/ui/cocoa...
File chrome/browser/ui/cocoa/browser_window_controller.mm (right):

https://codereview.chromium.org/1130603005/diff/40001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/browser_window_controller.mm:1269: -
(void)onActiveTabChanged:(content::WebContents*)oldContents
How is this different from onActivateTabWithContents?  Should the two be
combined somehow?

[groby-ooo-7-16, 2015-05-15 19:06:58]

https://codereview.chromium.org/1130603005/diff/40001/chrome/browser/ui/cocoa...
File chrome/browser/ui/cocoa/browser_window_controller.mm (right):

https://codereview.chromium.org/1130603005/diff/40001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/browser_window_controller.mm:1269: -
(void)onActiveTabChanged:(content::WebContents*)oldContents
On 2015/05/15 18:38:02, rohitrao wrote:
> How is this different from onActivateTabWithContents?  Should the two be
> combined somehow?

onActivateTabWithContents is called at a very specific point in sequence of
events - the old contents have gone, and the view has been switched out for a
view with the new web contents. Semantically, the old contents have no meaning
for that function, so I'd prefer not to pass them in anyways.

It's also part of the TabStripController, not the BrowserWindowController, so it
would have to get very roundabout access to the permission bubble.

For more comments on this mess, see crbug.com/340720

(Also: This is part of a series of CLs. I'm ultimately planning to hoist all
this out into platform-independent code, because there is nothing
platform-specific here. It'll just require quite a bit of shuffling, so I'm
doing it in small steps)

[rohitrao (ping after 24h), 2015-05-15 19:24:04]

Good to hear that there's a grand plan afoot =)

I think both onActiveTabChanged and onActivateTabWithContents are driven from
the same exact TabStripModelObserver call, just they take different paths to
BWC.  This is why I think it's weird to have both of them.

TSMO -> TSMOBridge::ActiveTabChanged() -> [TabStripController
activateTabWithContents] -> [TabStripController swapInTabAtIndex] -> [BWC
onActivateTabWithContents]

TSMO -> Browser::ActiveTabChanged() -> BWCocoa::OnActiveTabChanged() -> [BWC
onActiveTabChanged]

If you agree that I got the control flow correct above (please double-check my
work =) ), then would it make sense to put all of the new code either at the
beginning or the end of the existing onActivateTabWithContents method?  Or does
it make more sense to keep the two methods separate?

[rohitrao (ping after 24h), 2015-05-15 19:30:12]

Oh, I see, the existing method doesn't give access to |newContents|, which is
what you need.

LGTM, assuming there's no ordering dependency where we need to guarantee that
one method runs before the other

Does it make sense to add a TODO to combine those two methods?  In theory it
should just be a matter of plumbing through |newContents|, if we're ok with
letting TabStripController keep driving the show for now?

[groby-ooo-7-16, 2015-05-15 19:34:12]

As far as I can tell, that's the correct flow.

But: onActivateTabWithContents only gets (deliberately) the new contents. As far
as BWC is concerned, there are no old contents at this point. 

The code in onActiveTabChanged: gets both, and I need to use both.

I could pass them through, but that's semantically incorrect. We're essentially
violating the statement that onActivateTabWithContents: doesn't care about old
contents.

There's also the issue that onActiveTabChanged is guaranteed to happen with the
TabStripController in a defined state, either on the old or the new contents. If
I move code over there, we're mid-transition.

I don't _think_ that is an issue, but I don't know why we chose this approach in
the first place. So I'm reluctant to put things there. Especially since
onActiveTabChanged will soon (hopefully within 2 weeks) die a peaceful death,
surrounded by its loved ones.

[groby-ooo-7-16, 2015-05-15 21:46:30]

On 2015/05/15 19:30:12, rohitrao wrote:
> Oh, I see, the existing method doesn't give access to |newContents|, which is
> what you need.
> 
> LGTM, assuming there's no ordering dependency where we need to guarantee that
> one method runs before the other
> 
> Does it make sense to add a TODO to combine those two methods?  In theory it
> should just be a matter of plumbing through |newContents|, if we're ok with
> letting TabStripController keep driving the show for now?

Let's take this discussion out of the CL, and onto the bug. 

At the moment, I simply don't understand why we give the responsibility to the
TabStrip in Cocoa and to the BrowserView in Views. I'd like to grok that before
committing to where we put things :)

[groby-ooo-7-16, 2015-05-15 21:46:38]

The CQ bit was checked by groby@chromium.org

[commit-bot: I haz the power, 2015-05-15 21:50:11]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1130603005/40001

[commit-bot: I haz the power, 2015-05-15 21:57:59]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2015-05-18 11:28:03]

Patchset 3 (id:??) landed as
https://crrev.com/a0a825ff6e71576850e25a2a7e8db4fb32d671a6
Cr-Commit-Position: refs/heads/master@{#330208}