The default bug view has changed. See this FAQ.

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

RESOLVED FIXED in Firefox 47

Status

()

Firefox
Tabbed Browser
RESOLVED FIXED
a year ago
a year ago

People

(Reporter: kmag, Assigned: kmag)

Tracking

({addon-compat, dev-doc-needed})

Trunk
Firefox 47
addon-compat, dev-doc-needed
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox47 fixed)

Details

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(1 attachment, 1 obsolete attachment)

(Assignee)

Description

a year ago
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.
(Assignee)

Comment 1

a year ago
Created attachment 8714559 [details] [diff] [review]
Add more detail to TabOpen/TabClose events for tabs moved between windows
Attachment #8714559 - Flags: review?(dao)
(Assignee)

Comment 2

a year ago
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.
Attachment #8714559 - Flags: review?(dao) → review?(gijskruitbosch+bugs)

Comment 3

a year ago
This will need to be documented and will likely affect add-ons.
Keywords: addon-compat, dev-doc-needed

Comment 4

a year ago
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?
Attachment #8714559 - Flags: review?(gijskruitbosch+bugs)

Comment 5

a year ago
(clearing review to sort out if more needs doing)
(Assignee)

Comment 6

a year ago
(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.
(Assignee)

Comment 7

a year ago
(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

a year ago
(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.
Flags: needinfo?(gijskruitbosch+bugs)

Comment 9

a year ago
(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... :-)
Flags: needinfo?(gijskruitbosch+bugs)
(Assignee)

Comment 10

a year ago
(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.
(Assignee)

Comment 11

a year ago
Created attachment 8718189 [details]
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/
Attachment #8718189 - Flags: review?(gijskruitbosch+bugs)
(Assignee)

Comment 12

a year ago
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.
(Assignee)

Updated

a year ago
Attachment #8714559 - Attachment is obsolete: true

Updated

a year ago
Attachment #8718189 - Flags: review?(gijskruitbosch+bugs) → review+

Comment 13

a year ago
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
(Assignee)

Comment 14

a year ago
Thanks
(Assignee)

Comment 15

a year ago
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

a year ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/307c1bd67eb6
Status: NEW → RESOLVED
Last Resolved: a year ago
status-firefox47: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 47
You need to log in before you can comment on or make changes to this bug.