1244496 - TabOpen and TabClose events should provide more detail about tabs moved between windows

Status: RESOLVED FIXED

(Firefox :: Tabbed Browser, defect)

Description

[Kris Maglione [:kmag]]

The WebExtension tabs API requires us to treat tabs that are moved between windows as the same tabs, preserving the ID, and dispatching onAttached/onDetached events, rather than onCreated/onDestroyed events in the process.

Bug 491431 added an event detail to TabClose events do allow listeners to determine if a tab is being moved rather than simply closed. This isn't quite enough for our purposes, so I'm proposing to switch to CustomEvents, and use a detail object for both open and close event, specifying the adopting/adopter tabs in both.

Comment 1

[Kris Maglione [:kmag]]

Attachment: Add more detail to TabOpen/TabClose events for tabs moved between windows

Comment 2

[Kris Maglione [:kmag]]

Comment on attachment 8714559 [details] [diff] [review]
Add more detail to TabOpen/TabClose events for tabs moved between windows

I just heard that Dao is out.

Gijs, I don't know if you're a good person to review this, but if not, can you forward it to someone who is?

Thanks.

Comment 3

[:Gijs (he/him)]

This will need to be documented and will likely affect add-ons.

Comment 4

[:Gijs (he/him)]

Comment on attachment 8714559 [details] [diff] [review]
Add more detail to TabOpen/TabClose events for tabs moved between windows

Review of attachment 8714559 [details] [diff] [review]:
-----------------------------------------------------------------

I don't really understand why this is necessary. Which then means I don't know if this:

https://dxr.mozilla.org/mozilla-central/source/browser/base/content/tabbrowser.xml#2833-2849

(and the corresponding handler that then closes aTab once it's running)

needs to be updated, too. Otherwise this mostly looked OK, though note nits below.

::: browser/base/content/tabbrowser.xml
@@ +1894,5 @@
>  
>              // Dispatch a new tab notification.  We do this once we're
>              // entirely done, so that things are in a consistent state
>              // even if the event listener opens or closes tabs.
> +            var evt = new CustomEvent("TabOpen", { bubbles: true, detail: aEventDetail || {} });

Nit: please split this line using a temp or wrapping.

::: devtools/client/debugger/test/mochitest/browser_dbg_listtabs-02.js
@@ +171,5 @@
>  function removeNewWindow() {
>    let deferred = promise.defer();
>  
>    once(gNewWindow, "unload").then(aEvent => {
> +    ok(!aEvent.detail.adoptedBy, "This was a normal window close");

This is an unload event. I don't think this change is correct?

Comment 5

[:Gijs (he/him)]

(clearing review to sort out if more needs doing)

Comment 6

[Kris Maglione [:kmag]]

(In reply to :Gijs Kruitbosch from comment #4)
> I don't really understand why this is necessary. Which then means I don't
> know if this:
> 
> https://dxr.mozilla.org/mozilla-central/source/browser/base/content/
> tabbrowser.xml#2833-2849
> 
> (and the corresponding handler that then closes aTab once it's running)
> 
> needs to be updated, too. Otherwise this mostly looked OK, though note nits
> below.

Yeah, that's a tricky edge case that I'm still trying to figure out how to
handle. Right now, it correctly fires the TabClose event with the correct
`adoptedBy` property, but it doesn't do anything special on the TabOpen event.

I'm on the fence between trying to fire the initial TabOpen event with the
correct adoptedTab detail, or opening a new tab and then closing the initial
tab (which would avoid some code duplication).

Maybe you have an opinion on that.

> ::: devtools/client/debugger/test/mochitest/browser_dbg_listtabs-02.js
> @@ +171,5 @@
> >  function removeNewWindow() {
> >    let deferred = promise.defer();
> >  
> >    once(gNewWindow, "unload").then(aEvent => {
> > +    ok(!aEvent.detail.adoptedBy, "This was a normal window close");
> 
> This is an unload event. I don't think this change is correct?

Hm. That's a good question. I'm pretty sure I changed that because it was failing, but I may have just accidentally made an extra change.

I'm not sure what that's really supposed to be testing for, though. The detail for unload events should always be 0.

I'll double check. If the test passes without this change, I'll remove it.

Comment 7

[Kris Maglione [:kmag]]

(In reply to Kris Maglione [:kmag] from comment #6)
> I'm on the fence between trying to fire the initial TabOpen event with the
> correct adoptedTab detail, or opening a new tab and then closing the initial
> tab (which would avoid some code duplication).

(I was originally planning to sort this out from the TabClose listener, but
that's trickier than I expected, and I've since come to the conclusion that it
should be fixed from the tabbrowser side)

Comment 8

[:Gijs (he/him)]

(In reply to Kris Maglione [:kmag] from comment #7)
> (In reply to Kris Maglione [:kmag] from comment #6)
> > I'm on the fence between trying to fire the initial TabOpen event with the
> > correct adoptedTab detail, or opening a new tab and then closing the initial
> > tab (which would avoid some code duplication).
> 
> (I was originally planning to sort this out from the TabClose listener, but
> that's trickier than I expected, and I've since come to the conclusion that
> it
> should be fixed from the tabbrowser side)

I don't know off-hand what works here, I'll try to have a look tomorrow. ni to self for that.

Comment 9

[:Gijs (he/him)]

(In reply to Kris Maglione [:kmag] from comment #6)
> (In reply to :Gijs Kruitbosch from comment #4)
> > I don't really understand why this is necessary. Which then means I don't
> > know if this:
> > 
> > https://dxr.mozilla.org/mozilla-central/source/browser/base/content/
> > tabbrowser.xml#2833-2849
> > 
> > (and the corresponding handler that then closes aTab once it's running)
> > 
> > needs to be updated, too. Otherwise this mostly looked OK, though note nits
> > below.
> 
> Yeah, that's a tricky edge case that I'm still trying to figure out how to
> handle. Right now, it correctly fires the TabClose event with the correct
> `adoptedBy` property, but it doesn't do anything special on the TabOpen
> event.
> 
> I'm on the fence between trying to fire the initial TabOpen event with the
> correct adoptedTab detail, or opening a new tab and then closing the initial
> tab (which would avoid some code duplication).
> 
> Maybe you have an opinion on that.

I would prefer to fix the initial tab open because it would be better for perf, adding another tab might cause visual issues (flickering or whatever) and it just seems generally a bit hacky.

tabbrowser.xml is a huge thing that could probably, at this point, do with being split up - but then again, doing that is painful right now. So we struggle on... :-)

Comment 10

[Kris Maglione [:kmag]]

(In reply to :Gijs Kruitbosch from comment #9)
> I would prefer to fix the initial tab open because it would be better for
> perf, adding another tab might cause visual issues (flickering or whatever)
> and it just seems generally a bit hacky.

Cool, I'll try to make that work, then. Thanks.

Comment 11

[Kris Maglione [:kmag]]

Attachment: MozReview Request: Bug 1244496: Add more detail to TabOpen/TabClose events for tabs moved between windows. r?Gijs

Review commit: https://reviewboard.mozilla.org/r/34457/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/34457/

Comment 12

[Kris Maglione [:kmag]]

I gave up trying to find an elegant way to do this, and settled for checking the value of window.arguments[0] in my window open listener, along with nulling the value out in browser.js when the tab is actually adopted.

Ideally we'd add a detail to the TabOpen event for the first tab, but we don't actually fire a TabOpen event for the first tab. We could start... but I don't even want to think about the kinds of code that might be relying on us not sending it.

Comment 13

[:Gijs (he/him)]

Comment on attachment 8718189 [details]
MozReview Request: Bug 1244496: Add more detail to TabOpen/TabClose events for tabs moved between windows. r?Gijs

https://reviewboard.mozilla.org/r/34457/#review31197

Comment 14

[Kris Maglione [:kmag]]

Thanks

Comment 15

[Kris Maglione [:kmag]]

https://hg.mozilla.org/integration/fx-team/rev/307c1bd67eb60eb2e4213c5aed1b9460c1a5ed06
Bug 1244496: Add more detail to TabOpen/TabClose events for tabs moved between windows. r=Gijs

Comment 16

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/307c1bd67eb6