Closed Bug 1188543 Opened 4 years ago Closed 4 years ago

TPS doesn't score against the proper e10s tab switch event

Categories

(Testing :: Talos, defect)

defect
Not set

Tracking

(e10s+, firefox43 fixed)

RESOLVED FIXED
mozilla43
Tracking Status
e10s + ---
firefox43 --- fixed

People

(Reporter: BenWa, Assigned: BenWa)

References

Details

Attachments

(3 files, 3 obsolete files)

Similar to bug 1187451 but we need to make sure the content process is ready for the tab switch as well.
Attached patch part 2: Modify Talos (obsolete) — Splinter Review
Assignee: nobody → bgirard
Status: NEW → ASSIGNED
Attachment #8640020 - Flags: review?(blassey.bugs)
Attached patch part 1: add TabSwitched event (obsolete) — Splinter Review
I started looking at the event 'TabSwitchDone' first but that happens too late. It's waiting for unload for unused tabs which has a ~UNLOAD_DELAY (300ms) delay after the tab has been shown to the user.

This new event matches the times that we record to telemetry.
Attachment #8640022 - Flags: review?(wmccloskey)
Attachment #8640020 - Flags: review?(blassey.bugs) → review+
Comment on attachment 8640022 [details] [diff] [review]
part 1: add TabSwitched event

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

::: browser/base/content/tabbrowser.xml
@@ +3431,5 @@
>              },
>  
>              finishTabSwitch: function () {
>                if (this.requestedTab && this.getTabState(this.requestedTab) == this.STATE_LOADED) {
> +                // After this point the tab has switch from the content thread' point of view.

switch -> switched
thread' -> thread's

@@ +3433,5 @@
>              finishTabSwitch: function () {
>                if (this.requestedTab && this.getTabState(this.requestedTab) == this.STATE_LOADED) {
> +                // After this point the tab has switch from the content thread' point of view.
> +                // The changes will be visible after the next refresh driver tick + composite.
> +                let event = new CustomEvent("TabSwitched", {

I kind of like the name TabSwitchComplete better, but it doesn't really matter.
Attachment #8640022 - Flags: review?(wmccloskey) → review+
(In reply to Bill McCloskey (:billm) from comment #3)
> I kind of like the name TabSwitchComplete better, but it doesn't really
> matter.

Oh, I forgot we also have TabSwitchDone. I guess TabSwitched is better.
Attachment #8640022 - Attachment is obsolete: true
Attachment #8640591 - Flags: review+
url:        https://hg.mozilla.org/integration/mozilla-inbound/rev/c9822fda68ff0212b8ec811c4f4f524268274751
changeset:  c9822fda68ff0212b8ec811c4f4f524268274751
user:       Benoit Girard <b56girard@gmail.com>
date:       Wed Jul 29 13:28:05 2015 -0400
description:
Bug 1188543 - Part 1: Add TabSwitchComplete event. r=billm
Attached patch part 2: Modify Talos (obsolete) — Splinter Review
rebased
Attachment #8640020 - Attachment is obsolete: true
url:        https://hg.mozilla.org/build/talos/rev/2beaa8352bb48879b5784d076ab980b40b3f7bf9
changeset:  2beaa8352bb48879b5784d076ab980b40b3f7bf9
user:       Benoit Girard <b56girard@gmail.com>
date:       Wed Jul 29 13:56:23 2015 -0400
description:
Bug 1188543 - TPS e10s should listen for TabSwitched event. r=blassey
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
backed out talos change:
http://hg.mozilla.org/build/talos/rev/24ecc5140689

this is because tps didn't run on try and timed out- it appears to be this change:
http://hg.mozilla.org/build/talos/rev/da09edf6d984

but backing it out has a lot of merge conflicts so we backed all of them out.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Bug 1188543 - Make sure finishTabSwitch is always on layers ready. r=blassey
Attachment #8646547 - Flags: review?(blassey.bugs)
Comment on attachment 8646547 [details]
MozReview Request: Bug 1188543 - Make sure finishTabSwitch is always on layers ready. r=blassey

https://reviewboard.mozilla.org/r/15799/#review14085

Ship It!
Attachment #8646547 - Flags: review?(blassey.bugs) → review+
url:        https://hg.mozilla.org/integration/mozilla-inbound/rev/e9906ce9db6fe49213a23fdb4242ac915f059b9c
changeset:  e9906ce9db6fe49213a23fdb4242ac915f059b9c
user:       Benoit Girard <b56girard@gmail.com>
date:       Tue Aug 11 16:03:30 2015 -0400
description:
Bug 1188543 - Make sure finishTabSwitch is always called on layers ready. r=blassey
https://hg.mozilla.org/mozilla-central/rev/e9906ce9db6f
Status: REOPENED → RESOLVED
Closed: 4 years ago4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla43
Opps, still waiting on part 2
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Part 2 is based on top of bug 1188638, we can either wait for it to land or rebase the patch. For now I'm leaning towards wait.
Depends on: 1188638
Attachment #8640627 - Attachment is obsolete: true
Attachment #8648838 - Flags: review+
Now making sure we run the test under the e10s config:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=f69af64c6ba3
url:        https://hg.mozilla.org/build/talos/rev/e970f49a2dd28c051318c577afdf84f7f0a2709f
changeset:  e970f49a2dd28c051318c577afdf84f7f0a2709f
user:       Benoit Girard <b56girard@gmail.com>
date:       Wed Jul 29 13:56:23 2015 -0400
description:
Bug 1188543 - TPS e10s should listen for TabSwitched event. r=blassey
this is live in production, can we close this?
Status: REOPENED → RESOLVED
Closed: 4 years ago4 years ago
Resolution: --- → FIXED
(In reply to Joel Maher (:jmaher) from comment #22)
> this is live in production, can we close this?

I think this didn't actually go into production until I updated the talos config in m-c today, and received a flood of 250% talos tabswitch regressions as a result.

http://hg.mozilla.org/integration/mozilla-inbound/diff/f1b54a9c9f9b/testing/talos/talos.json
That's unfortunate. But AFAIK before we were measuring the wrong thing and now we're measuring something closer to what we need. There might be further improvements we can make. But it's not like Gecko has regressed here.
Depends on: 1283812
You need to log in before you can comment on or make changes to this bug.