Put beta users of DevTools in a special e10s cohort that always qualify for e10s

RESOLVED FIXED in Firefox 53

Status

P1
normal
RESOLVED FIXED
2 years ago
8 months ago

People

(Reporter: Felipe, Assigned: jryans)

Tracking

Trunk
Firefox 54
Bug Flags:
qe-verify -

Firefox Tracking Flags

(firefox53 fixed, firefox54 fixed)

Details

Attachments

(3 attachments)

(Reporter)

Description

2 years ago
Ir order to make sure users on beta are testing the e10s versions of devtools features, we want to take these users out of the A/B roll-of-dice split and put them in a cohort that will always qualify for e10s.

Note that they might still be disqualified by other criteria, which are: a11y, WinXP, mpc=false addons.
(Reporter)

Comment 1

2 years ago
I'd be happy to review but I'd like someone from devtools to take it. From what I read in an email thread the main motivation for this is testing RDM in e10s mode.  IIRC there's a pref flag that tells if someone is using it (or using devtools in general, if that's the intention).

The code to be modified is here: http://searchfox.org/mozilla-central/rev/60ae6514e4c559c0c234f0e7aefccb101b8beb2e/browser/extensions/e10srollout/bootstrap.js#54

I suggest taking a look at the getTemporaryDisqualification() function and making a similar one but which will qualify.

That function is now empty but you can see how it was used to temporarily disable e10s for the ru locale, bug 1304164
(Assignee)

Comment 2

2 years ago
I can make an attempt at this.  I'll aim for including anyone who has used DevTools before.

When various DevTools features are used for the first time, the pref "devtools.telemetry.tools.opened.version" is updated.  So, I think we can check whether this pref has a user set value as a proxy for "has DevTools ever been used?".
Assignee: nobody → jryans
Status: NEW → ASSIGNED
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Updated

2 years ago
Priority: -- → P1
(Reporter)

Comment 6

2 years ago
mozreview-review
Comment on attachment 8841055 [details]
Bug 1342490 - Allow e10srollout to control temporary qualifications.

https://reviewboard.mozilla.org/r/115396/#review118418

::: browser/extensions/e10srollout/bootstrap.js:113
(Diff revision 1)
>      // For these volatile disqualification reasons, however, we must not try
>      // to activate e10s because the backend doesn't know about it. E10S_STATUS
>      // here will be accumulated as "2 - Disabled", which is fine too.
>      setCohort(`temp-disqualified-${temporaryDisqualification}`);
>      Preferences.reset(PREF_TOGGLE_E10S);
> +  } else if (temporaryQualification != "") {

please check that `disqualified` is not false too, because that comes from the backend and can't be overriden.

(But don't worry about it because the only reason a disqualification will occur now is by using an mpc=false addon or accessibility).
Attachment #8841055 - Flags: review?(felipc) → review+
(Reporter)

Comment 7

2 years ago
mozreview-review
Comment on attachment 8841056 [details]
Bug 1342490 - DevTools users are temporarily qualified for e10s.

https://reviewboard.mozilla.org/r/115398/#review118420
Attachment #8841056 - Flags: review?(felipc) → review+
(Reporter)

Comment 8

2 years ago
mozreview-review
Comment on attachment 8841057 [details]
Bug 1342490 - Bump e10srollout to version 1.11.

https://reviewboard.mozilla.org/r/115400/#review118422

::: browser/extensions/e10srollout/install.rdf.in:13
(Diff revision 1)
>  <RDF xmlns="http://www.w3.org/1999/02/22-rdf-syntax-ns#"
>       xmlns:em="http://www.mozilla.org/2004/em-rdf#">
>  
>    <Description about="urn:mozilla:install-manifest">
>      <em:id>e10srollout@mozilla.org</em:id>
> -    <em:version>1.9</em:version>
> +    <em:version>1.10</em:version>

I just landed on inbound something else that bumped this to 1.10, so please bump it again to 1.11
Attachment #8841057 - Flags: review?(felipc) → review+
(Assignee)

Comment 9

2 years ago
mozreview-review-reply
Comment on attachment 8841055 [details]
Bug 1342490 - Allow e10srollout to control temporary qualifications.

https://reviewboard.mozilla.org/r/115396/#review118418

> please check that `disqualified` is not false too, because that comes from the backend and can't be overriden.
> 
> (But don't worry about it because the only reason a disqualification will occur now is by using an mpc=false addon or accessibility).

We agreed on IRC that we want to check that `disqulified` _is_ false, so `!disqualified && temporaryQualification != ""`.
(Assignee)

Comment 10

2 years ago
mozreview-review-reply
Comment on attachment 8841057 [details]
Bug 1342490 - Bump e10srollout to version 1.11.

https://reviewboard.mozilla.org/r/115400/#review118422

> I just landed on inbound something else that bumped this to 1.10, so please bump it again to 1.11

Okay, bumped to 1.11.
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 17

2 years ago
Pushed by jryans@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/014e442400b4
Allow e10srollout to control temporary qualifications. r=Felipe
https://hg.mozilla.org/integration/autoland/rev/bc03995ce704
DevTools users are temporarily qualified for e10s. r=Felipe
https://hg.mozilla.org/integration/autoland/rev/8e515ff557a2
Bump e10srollout to version 1.11. r=Felipe
(Assignee)

Comment 18

2 years ago
Comment on attachment 8841055 [details]
Bug 1342490 - Allow e10srollout to control temporary qualifications.

Approval Request Comment
[Feature/Bug causing the regression]: Enable e10s for all DevTools users in next beta
[User impact if declined]: If declined, DevTools users won't get this special e10s rollout behavior
[Is this code covered by automated tests?]: No
[Has the fix been verified in Nightly?]: Has been tested locally on a beta simulation
[Needs manual test from QE? If yes, steps to reproduce]: No
[List of other uplifts needed for the feature/fix]: All patches in this bug must landed together
[Is the change risky?]: No
[Why is the change risky/not risky?]: Only checks a pref for DevTools users
[String changes made/needed]: None
Attachment #8841055 - Flags: approval-mozilla-aurora?
(Assignee)

Comment 19

2 years ago
Comment on attachment 8841056 [details]
Bug 1342490 - DevTools users are temporarily qualified for e10s.

See comment 18.
Attachment #8841056 - Flags: approval-mozilla-aurora?
(Assignee)

Comment 20

2 years ago
Comment on attachment 8841057 [details]
Bug 1342490 - Bump e10srollout to version 1.11.

See comment 18.
Attachment #8841057 - Flags: approval-mozilla-aurora?

Comment 21

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/014e442400b4
https://hg.mozilla.org/mozilla-central/rev/bc03995ce704
https://hg.mozilla.org/mozilla-central/rev/8e515ff557a2
Status: ASSIGNED → RESOLVED
Last Resolved: 2 years ago
status-firefox54: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 54
Comment on attachment 8841055 [details]
Bug 1342490 - Allow e10srollout to control temporary qualifications.

Bringing this to aurora as discussed in e10s meeting.
Attachment #8841055 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Attachment #8841056 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
status-firefox53: --- → affected
Attachment #8841057 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
We need to uplift the work from bug 1340195 first for this to work without rebasing.
(Reporter)

Comment 24

2 years ago
So the plan of record is to land this and see the effect: depending on the number of users that this takes out of the test/control cohorts we might leave it as is or re-evaluate.
Setting qe-verify- based on J. Ryan Stinnett's assessment on manual testing needs (see Comment 18).
Flags: qe-verify-
(Assignee)

Updated

2 years ago
Blocks: 1351425

Updated

8 months ago
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.