Closed Bug 1342490 Opened 6 years ago Closed 6 years ago

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

Categories

(DevTools :: General, defect, P1)

Product:
DevTools
Component:
General
Type:
defect

Tracking

(firefox53 fixed, firefox54 fixed)

Status:
RESOLVED FIXED
Milestone:
Firefox 54
Tracking Flags:
Tracking Status
firefox53 --- fixed
firefox54 --- fixed

People

(Reporter: Felipe, Assigned: jryans)

References

Details

Attachments

(3 files)

Bug 1342490 - Allow e10srollout to control temporary qualifications.
59 bytes, text/x-review-board-request
Felipe
: review+
lizzard
: approval-mozilla-aurora+
Details
Bug 1342490 - DevTools users are temporarily qualified for e10s.
59 bytes, text/x-review-board-request
Felipe
: review+
lizzard
: approval-mozilla-aurora+
Details
Bug 1342490 - Bump e10srollout to version 1.11.
59 bytes, text/x-review-board-request
Felipe
: review+
lizzard
: approval-mozilla-aurora+
Details 
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.
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
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 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+
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+
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+
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 != ""`.
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.
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
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?
Comment on attachment 8841056 [details]
Bug 1342490 - DevTools users are temporarily qualified for e10s.

See comment 18.
Attachment #8841056 - Flags: approval-mozilla-aurora?
Comment on attachment 8841057 [details]
Bug 1342490 - Bump e10srollout to version 1.11.

See comment 18.
Attachment #8841057 - Flags: approval-mozilla-aurora?
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+
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.
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-
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.