The default bug view has changed. See this FAQ.

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

RESOLVED FIXED in Firefox 53

Status

()

Firefox
Developer Tools
P1
normal
RESOLVED FIXED
a month ago
a day ago

People

(Reporter: Felipe, Assigned: jryans)

Tracking

Trunk
Firefox 54
Points:
---
Bug Flags:
qe-verify -

Firefox Tracking Flags

(firefox53 fixed, firefox54 fixed)

Details

MozReview Requests

()

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

Attachments

(3 attachments)

(Reporter)

Description

a month 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

a month 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

a month 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

a month ago
Priority: -- → P1
(Reporter)

Comment 6

27 days 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

27 days 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

27 days 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

27 days 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

27 days 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

27 days 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
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 21

27 days 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: 27 days 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

26 days 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.

Comment 25

26 days ago
bugherderuplift
https://hg.mozilla.org/releases/mozilla-aurora/rev/0ba4b0ec4c6e
https://hg.mozilla.org/releases/mozilla-aurora/rev/f8757303ef9a
https://hg.mozilla.org/releases/mozilla-aurora/rev/44337c730a8a
status-firefox53: affected → fixed
Setting qe-verify- based on J. Ryan Stinnett's assessment on manual testing needs (see Comment 18).
Flags: qe-verify-
Blocks: 1351425
You need to log in before you can comment on or make changes to this bug.