Unbreak tabs.onRemove extension API in face of fast tab closure

Unbreak tabs.onRemove extension API in face of fast tab closure

Fast tab closure changes the way tabs are closed:
* Run onbeforeunload event handler
* Web contents are first detached from containing view.
* Unload handler is run independently of GUI and tab is closed asynchronously.

This means that a state exists where a tab is detached as an initial step to closing it.

The problem is that the extension API was sending a tabs.onDetached event for this scenario rather than tabs.onRemoved.

Fix this by:
* Making CoreTabHealper aware of the fact it's being detached as part of an unload via OnUnloadAboutToDetach() and GetWebContentsDetachedToClose().
* Teach the extension system's BrowserEventRouter about this state and have it dispatch the right event.

BUG=248998
TEST=ExtensionApiTest.TabOnRemoved

[jeremy, 2013-06-18 14:52:11]

slamm/avi: main reviewers.
joi: content/public/browser/
asargent: browser/extensions

Sending this out for early review to make sure we're all on the same page:

* If you think there are others who should review this then I would really
appreciate it if you could add them now.
* Does the general approach look ok?
* If you don't like the method names, please provide specific suggestions.  I'm
absolutely not attached to any of the method names here :o)

Remaining work:
* I want to add the TabStripModel as the first parameter of
TabStripModelObserver::TabDetachedAt() so that observers can call
TabStripModel::closing_all() - does this sound ok?
* Unit tests if possible.

Thanks!,
Jeremy

[Avi (use Gerrit), 2013-06-18 15:04:13]

I'm just now catching the whole "detach" thing; sorry to be springing it on you
like that.

I'm bummed that WebContents has an inkling of "detachment". The idea of a tab
existing but being "detached" from a tab strip is a chrome/ concept. Can we get
away with not having that in content/?

Adding jam@.

https://codereview.chromium.org/17382005/diff/1/content/public/browser/web_co...
File content/public/browser/web_contents.h (right):

https://codereview.chromium.org/17382005/diff/1/content/public/browser/web_co...
content/public/browser/web_contents.h:390: virtual void
OnUnloadDetachedStarted() = 0;
:( :(

[asargent_no_longer_on_chrome, 2013-06-18 16:16:53]

I'm at a 2-day hackathon and will be slow to respond to codereviews. If you
need more timely response please find another reviewer.


On Tue, Jun 18, 2013 at 8:04 AM, <avi@chromium.org> wrote:

> I'm just now catching the whole "detach" thing; sorry to be springing it
> on you
> like that.
>
> I'm bummed that WebContents has an inkling of "detachment". The idea of a
> tab
> existing but being "detached" from a tab strip is a chrome/ concept. Can
> we get
> away with not having that in content/?
>
> Adding jam@.
>
>
> https://codereview.chromium.**org/17382005/diff/1/content/**
>
public/browser/web_contents.h<https://codereview.chromium.org/17382005/diff/1/content/public/browser/web_contents.h>
> File content/public/browser/web_**contents.h (right):
>
> https://codereview.chromium.**org/17382005/diff/1/content/**
>
public/browser/web_contents.h#**newcode390<https://codereview.chromium.org/17382005/diff/1/content/public/browser/web_contents.h#newcode390>
> content/public/browser/web_**contents.h:390: virtual void
> OnUnloadDetachedStarted() = 0;
> :( :(
>
>
https://codereview.chromium.**org/17382005/<https://codereview.chromium.org/1...
>

[jam, 2013-06-18 16:20:59]

On 2013/06/18 15:04:13, Avi wrote:
> I'm just now catching the whole "detach" thing; sorry to be springing it on
you
> like that.
> 
> I'm bummed that WebContents has an inkling of "detachment". The idea of a tab
> existing but being "detached" from a tab strip is a chrome/ concept. Can we
get
> away with not having that in content/?
> 
> Adding jam@.

couldn't agree more. looks like the existing method was done for UMA histogrms,
but that code can be shifted into chrome. i'll look into it

[jam, 2013-06-18 16:22:12]

On 2013/06/18 16:20:59, jam wrote:
> On 2013/06/18 15:04:13, Avi wrote:
> > I'm just now catching the whole "detach" thing; sorry to be springing it on
> you
> > like that.
> > 
> > I'm bummed that WebContents has an inkling of "detachment". The idea of a
tab
> > existing but being "detached" from a tab strip is a chrome/ concept. Can we
> get
> > away with not having that in content/?
> > 
> > Adding jam@.
> 
> couldn't agree more. looks like the existing method was done for UMA
histogrms,
> but that code can be shifted into chrome. i'll look into it

oh, this code just got added in https://codereview.chromium.org/11016023 last
week. hmm, i would strongly prefer we roll back all these changes to content.
not lgtm

[jam, 2013-06-19 00:07:46]

I've landed https://codereview.chromium.org/17151010 which moves this unload
timing stuff to chrome. feel free to follow it in this cl so that no changes to
content are needed

[jeremy, 2013-06-19 10:29:34]

Most of this change is adding a close_all boolean to the TaDetach() function.

Avi/sky: does this look ok to you?

[Avi (use Gerrit), 2013-06-19 14:18:33]

Whoa. This isn't what I remember us talking about.

What I remember agreeing to is that a client of the TabStripModel could, when
calling DetachWebContentsAt, specify that it was a "detach with intent to
close", and that TabStripModel would send out "closing" notifications rather
than "detach" ones.

That doesn't seem to be the case here, so I reviewed TabStripModel files in
confusion, left a comment, and stopped.

BTW, is the CL description accurate? It reminds me of the first version of the
code.

https://codereview.chromium.org/17382005/diff/8001/chrome/browser/ui/tabs/tab...
File chrome/browser/ui/tabs/tab_strip_model_observer.h (right):

https://codereview.chromium.org/17382005/diff/8001/chrome/browser/ui/tabs/tab...
chrome/browser/ui/tabs/tab_strip_model_observer.h:76: bool closing_all);
What does "closing_all" mean in this context? This callback is about detachment
of a single tab; "all"?

[jeremy, 2013-06-19 15:34:18]

Yes, sorry for not mentioning that - What we discussed was for
tab_strip_model to send an TabRemoved() rather than a TabDetached() when a
tab was "detached with intent to kill" but as you said yesterday this is
too simple to work and when I tried it, it broke a bunch of other things in
the code e.g. closing a tab no longer updated the GUI.


On Wed, Jun 19, 2013 at 5:18 PM, <avi@chromium.org> wrote:

> Whoa. This isn't what I remember us talking about.
>
> What I remember agreeing to is that a client of the TabStripModel could,
> when
> calling DetachWebContentsAt, specify that it was a "detach with intent to
> close", and that TabStripModel would send out "closing" notifications
> rather
> than "detach" ones.
>
> That doesn't seem to be the case here, so I reviewed TabStripModel files in
> confusion, left a comment, and stopped.
>
> BTW, is the CL description accurate? It reminds me of the first version of
> the
> code.
>
>
> https://codereview.chromium.**org/17382005/diff/8001/chrome/**
>
browser/ui/tabs/tab_strip_**model_observer.h<https://codereview.chromium.org/17382005/diff/8001/chrome/browser/ui/tabs/tab_strip_model_observer.h>
> File chrome/browser/ui/tabs/tab_**strip_model_observer.h (right):
>
> https://codereview.chromium.**org/17382005/diff/8001/chrome/**
>
browser/ui/tabs/tab_strip_**model_observer.h#newcode76<https://codereview.chromium.org/17382005/diff/8001/chrome/browser/ui/tabs/tab_strip_model_observer.h#newcode76>
> chrome/browser/ui/tabs/tab_**strip_model_observer.h:76: bool closing_all);
> What does "closing_all" mean in this context? This callback is about
> detachment of a single tab; "all"?
>
>
https://codereview.chromium.**org/17382005/<https://codereview.chromium.org/1...
>

[Avi (use Gerrit), 2013-06-19 16:13:37]

The questions that I ask then about this CL still remain. And if it's this
tricky, do we want to revert it until we're sure it's right?

[sky, 2013-06-19 17:12:12]

We're close to a branch point. Your patch has broken extensions. It
has also broken session restore in some as yet undiagnosed way. I
agree with Avi. You should revert until the problems have been fixed.

  -Scott

On Wed, Jun 19, 2013 at 9:13 AM,  <avi@chromium.org> wrote:
> The questions that I ask then about this CL still remain. And if it's this
> tricky, do we want to revert it until we're sure it's right?
>
> https://codereview.chromium.org/17382005/

[jeremy, 2013-06-19 17:48:36]

Totally agree, revert seems like the right thing to do here - I'll do it
when I'm back in the office tomorrow morning unless someone beats me to it.

Regardless, I'd appreciate hearing your thoughts about what you think the
proper fix is.


On Wed, Jun 19, 2013 at 8:12 PM, Scott Violet <sky@chromium.org> wrote:

> We're close to a branch point. Your patch has broken extensions. It
> has also broken session restore in some as yet undiagnosed way. I
> agree with Avi. You should revert until the problems have been fixed.
>
>   -Scott
>
> On Wed, Jun 19, 2013 at 9:13 AM,  <avi@chromium.org> wrote:
> > The questions that I ask then about this CL still remain. And if it's
> this
> > tricky, do we want to revert it until we're sure it's right?
> >
> > https://codereview.chromium.org/17382005/
>

[Avi (use Gerrit), 2013-06-19 18:23:56]

On 2013/06/19 17:48:36, jeremy wrote:
> Totally agree, revert seems like the right thing to do here - I'll do it
> when I'm back in the office tomorrow morning unless someone beats me to it.

Looks like the tree will be broken all of today, so no one _can_ revert your
change even if they wanted to.

You'll have to do it once things green up.

[slamm, 2014-10-16 23:56:11]

slamm@chromium.org changed reviewers:
- slamm@chromium.org