Closed Bug 1283812 Opened 4 years ago Closed 3 years ago

Stop dispatching the TabSwitched event

Categories

(Firefox :: Tabbed Browser, defect, P3)

defect

Tracking

()

RESOLVED FIXED
Firefox 50
Tracking Status
firefox50 --- fixed

People

(Reporter: dao, Assigned: dao)

References

Details

Attachments

(1 file, 4 obsolete files)

Bug 1188543 added a TabSwitched event, but there's already a TabSwitchDone event. It's impossible to tell the difference between these events from the name.

I think we should rename the TabSwitched event such that it's clear that it's meant for perf diagnosis purposes, not for other random tabbrowser API consumers. It might also make sense to dispatch this event only under certain conditions. (Is there a pref or something that tells the application that it's running talos?)
Flags: needinfo?(wmccloskey)
Flags: needinfo?(bgirard)
I'm not sure if we have a clean way to detect if we have talos running. I agree the name could be improved to be more unique. I'd hope that anyone relying on these events would be checking how they work but it is a bit of a footgun. I'm pretty flexible on what it could be renamed to.
Flags: needinfo?(bgirard)
Attached patch browser part (obsolete) — Splinter Review
Assignee: nobody → dao+bmo
Status: NEW → ASSIGNED
Attachment #8770095 - Flags: review?(bgirard)
Attached patch talos part (obsolete) — Splinter Review
Attachment #8770096 - Flags: review?(blassey.bugs)
Attachment #8770096 - Flags: review?(blassey.bugs) → review+
Attachment #8770095 - Flags: review?(bgirard) → review+
Can somebody advise the best way to land these patches? Should I just push to both repositories at the same time?
Flags: needinfo?(wmccloskey) → needinfo?(blassey.bugs)
jmaher would know. He should be back on Friday.

I *think* it should be landed on Talos first and when it's deployed we should land this in Gecko.
Flags: needinfo?(jmaher)
Flags: needinfo?(blassey.bugs)
Attached patch talos part (obsolete) — Splinter Review
(In reply to Benoit Girard (:BenWa) from comment #5)
> I *think* it should be landed on Talos first and when it's deployed we
> should land this in Gecko.

But talos' waitForTabSwitchDone would fail until the Firefox part has landed. So I suppose as an interim arrangement, waitForTabSwitchDone should just listen to both the new PerfDiagnosis:TabSwitched and the old TabSwitched. This patch does that.
Attachment #8770096 - Attachment is obsolete: true
talos is in-tree, so if you land gecko changes, land the talos changes at the same time.  The only out of tree bits of talos are the tp5 pagesets.
Flags: needinfo?(jmaher)
Attachment #8770895 - Attachment is obsolete: true
Attachment #8770096 - Attachment is obsolete: false
Comment on attachment 8770096 [details] [diff] [review]
talos part

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

keep in mind we have signed addons for talos in inbound/fx-team/central/aurora/beta.  For try or locally, we don't require it.  Please update the version and include the signed .xpi:
https://wiki.mozilla.org/EngineeringProductivity/HowTo/SignExtensions
Good lord, I'm supposed to create an XPI to land a talos change? What XPI, where? Is this documented somewhere or, alternatively, can somebody in charge of talos take care of this?
the joys of signing addons.  We have many addons in Firefox, in this case the tabswitch addon is being modified.  I don't mind signing this- let me update the patch here
hmm, this doesn't seem to be out of the m-c repo, this seems to be from the old talos repo from 9+ months ago.
ok, the code is different enough in-tree, that I would prefer the patch be updated, here is a pointer to bootstrap.js:
https://dxr.mozilla.org/mozilla-central/source/testing/talos/talos/tests/tabswitch/bootstrap.js#266
Attached patch patch (obsolete) — Splinter Review
(In reply to Joel Maher ( :jmaher ) from comment #7)
> talos is in-tree, so if you land gecko changes, land the talos changes at
> the same time.  The only out of tree bits of talos are the tp5 pagesets.

Btw, it looks like DXR doesn't index the in-tree talos folder, so I didn't see this in my search: https://dxr.mozilla.org/mozilla-central/search?q=TabSwitched&=mozilla-central&redirect=true
Attachment #8770095 - Attachment is obsolete: true
Attachment #8770096 - Attachment is obsolete: true
Comment on attachment 8771376 [details] [diff] [review]
patch

Even though this is just supposed to be a renaming, the first hunk in this patch refers to TabSwitched and the second one refers to TabSwitchDone!

It looks like the "TabSwitched" event is not used after bug 1188543. You should just be able to remove it entirely. No Talos changes needed.
Attachment #8771376 - Flags: review-
(In reply to Bill McCloskey (:billm) from comment #14)
> Comment on attachment 8771376 [details] [diff] [review]
> patch
> 
> Even though this is just supposed to be a renaming, the first hunk in this
> patch refers to TabSwitched and the second one refers to TabSwitchDone!

Oops, I guess that explains why DXR didn't find TabSwitched there! And then the obsolete talos repository added to the confusion; are there any plans to decommission that?

Also perfect example for how these event names are confusing.

> It looks like the "TabSwitched" event is not used after bug 1188543. You
> should just be able to remove it entirely. No Talos changes needed.

Talos did use it after bug 1188543 according to the obsolete talos repository. I did some more research, looks like bug 1261152 made talos use TabSwitchDone instead. Why, don't know.
Summary: Find a better name for the TabSwitched event → Stop dispatching the TabSwitched event
Blocks: 1261152, 1188543
Attached patch patch v2Splinter Review
Attachment #8771376 - Attachment is obsolete: true
Attachment #8772358 - Flags: review?(mconley)
Comment on attachment 8772358 [details] [diff] [review]
patch v2

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

Did a cursory poke around for add-on usage on DXR, and couldn't find any. I think this is safe to remove. Thanks Dao!
Attachment #8772358 - Flags: review?(mconley) → review+
Pushed by dgottwald@mozilla.com:
https://hg.mozilla.org/integration/fx-team/rev/4720fe92629a
Stop dispatching the TabSwitched event. r=mconley
https://hg.mozilla.org/mozilla-central/rev/4720fe92629a
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 50
You need to log in before you can comment on or make changes to this bug.