Closed Bug 1302846 Opened 3 years ago Closed 2 years ago

Add Test Pilot/Future Discovery Pane add-ons to e10s whitelist

Categories

(Firefox :: Extension Compatibility, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
Tracking Status
e10s + ---
firefox49 --- affected
firefox50 --- fixed
firefox51 --- fixed
firefox52 --- wontfix

People

(Reporter: clouserw, Assigned: vladikoff)

References

()

Details

Attachments

(4 files, 1 obsolete file)

This bug is a request to add the Test Pilot add-ons to the e10s rollout whitelist at http://searchfox.org/mozilla-central/source/toolkit/mozapps/extensions/internal/E10SAddonsRollout.jsm .  The Test Pilot add-on IDs:

> Test Pilot:            @testpilot-addon
> Tab Center:            tabcentertest1@mozilla.com
> Activity Stream:       @activity-streams
> Min Vid:               @min-vid
> Page Shot:             jid1-NeEaf3sAHdKHPA@jetpack

I'm told webextensions don't need to be on this list, but if I'm wrong, those IDs are:

> Tracking Protection:   blok@mozilla.org
> No More 404s:          wayback_machine@mozilla.org

The Test Pilot add-ons are marked compatible with e10s and have gone through our regular QA testing on release and nightly versions.  If there are additional requirements I'm happy to run our add-ons through them.

Test Pilot is doing a substantial marketing push on the 28th of this month (with the launch of three additional experiments) and we see significant user drop-off for add-on install prompts.  We haven't had to deal with restarts as well, but we expect it will have a weighty effect on our engagement numbers.  After discussion with some folks in the know (most of the CC list!), we concluded being on the whitelist was the best option.

My apologies for the late bug, we weren't following the e10s rollout plan closely enough.
I would like to get a patch up for review for this issue ASAP. Wil, is that patch going to come from your team?
Flags: needinfo?(wclouser)
Patch will need to come from Felipe, who manages the add-on, and Shell also needs to review, as she's managing the e10s/addons testing and rollout process. 

The whitelist is mis-named; currently in Fx48 all add-ons that install to an e10s-enabled version of Firefox (e.g. a profile that is not a RTL locale, no add-ons installed, and a11y disabled - roughly 60% of ADIs) will trigger a restart. With 49, we'll be introducing a small set of known, good add-ons for the purposes of testing whether e10s-compatible add-ons affect our stability and perf numbers appreciably. It's less of a whitelist, and more of a small group of add-ons we're testing e10s compatibility against, which will lead to leveling up to all MPC-compatible addons, including WebExtensions, in Fx 50.

Adding more extensions to the test list may add risk to the test, and I'd like Felipe, Shell, and dzeber to weigh in on those risks, and whether it might adversely affect (analysis of) results (given the number of addons we'd be adding, and the potential marketing campaign behind them, it's a real risk). We're using these numbers as a gating release for 50; if our known good add-ons perform as expected, we'll be enabling any addon that is multi-process compatible (MPX true) and webextensions to the e10s cohort.

Because this is a gating mechanism to a broader set of e10s-compatible add-ons, it's really important for us to be able to interpret results in 49.
Flags: needinfo?(sescalante)
Flags: needinfo?(felipc)
Flags: needinfo?(dzeber)
I think vlad might have just volunteered to write this in #fxa.

It looks like we need to specify minimum version numbers for each addon. The currently-released addon versions should work:

> Test Pilot:            @testpilot-addon
version 0.8.5

> Tab Center:            tabcentertest1@mozilla.com
version 1.24

> Activity Stream:       @activity-streams
version 1.1.3

> Min Vid:               @min-vid
version: 0.0.1

> Page Shot:             jid1-NeEaf3sAHdKHPA@jetpack
version: 0.0.1
Eh, just assigning to :vladikoff, so we don't waste other folks' time writing the same patch.

:felipe, feel free to steal or just review whatever vlad comes up with :-)
Assignee: nobody → vlad
current webextension versions:

> Tracking Protection:   blok@mozilla.org
version 1.0.0

> No More 404s:          wayback_machine@mozilla.org
version 1.5.6
Ah, we also should add universal search, universal-search@mozilla.com, version 1.0.9
> Ah, we also should add universal search, universal-search@mozilla.com, version 1.0.9

Oops, nope. QA needs to take a pass before we whitelist the search addon.
Kev, Shell is on PTO until after 49 goes out. 

My only real concern here is matching the data between beta and release and that we don't have this information on beta right now. I would probably say that's not going to buy us much and we should add these. But please correct me if I'm wrong.
> "need to update browser/extensions/e10srollout/bootstrap.js to update the name"

Let me know what needs be updated there (if needed)
(In reply to Andy McKay [:andym] from comment #9)
> My only real concern here is matching the data between beta and release and
> that we don't have this information on beta right now. I would probably say
> that's not going to buy us much and we should add these. But please correct
> me if I'm wrong.

My concern is it adds to complexity of analysis, because we're essentially doubling the number of addons, and they haven't been part of the test process to date. There's no throttling of the test cohort, so it means that if we see differences in the control to the test, it'll make it that much more difficult todetermine whether it's a specific add-on vs. e10s on the whole with add-ons to back out, and if the results are negative it could also affect leveling up in 50 to include MPC-true and WebExtensions in the e10s by default group. 

There's also a marketing campaign planned for these, which may create a larger test cohort than planned, which has been a concern to date.

Timing on this isn't great, but I think everyone understands that. From what I understand the marketing campaign is planned for end of month, so we have a little tie if we want to push a system addon update separately (which I realize has it's own set of processes, and may not be possible for 49 launch date).
Comment on attachment 8791404 [details]
Bug 1302846 - Add Test Pilot add-ons to e10s whitelist

https://reviewboard.mozilla.org/r/78818/#review77478

You'll need to change the line

`"release" : "49a", // 10 tested add-ons + any WebExtension`

to the name of the new policy that will be used  ("49testpilota")

from browser/extensions/e10srollout/bootstrap.js

Please do that on a separate patch which should help with approvals

::: toolkit/mozapps/extensions/internal/E10SAddonsRollout.jsm:129
(Diff revision 1)
> +const setTestPilot49 = [
> +  ADDONS.TestPilot,
> +  ADDONS.TabCenter,
> +  ADDONS.ActivityStream,
> +  ADDONS.MinVid,
> +  ADDONS.PageShot
> +]

Can you add the webextension ones too, in case we need to remove the carte blanche to any webextension?

::: toolkit/mozapps/extensions/internal/E10SAddonsRollout.jsm:153
(Diff revision 1)
> +  "49testpilota": { addons: setTestPilot49, webextensions: true },
> +  "49testpilotb": { addons: setTestPilot49, webextensions: false },

you'll need to union this with the other set, "set49Release".

you can do [...set49Release, ...setTestPilot49]


And can you have another two policies ("a" and "b") where only the Test Pilot add-on itself is whitelisted?
Attachment #8791404 - Flags: review-
My suggestion to reduce the risk here is the following:

Only ship with the Test Pilot add-on itself whitelisted. This means that the new install flow to Test Pilot can be used. Then installing the actual test add-ons (hmm confusing naming scheme, how are the called?) would still use the previous flow (with the doorhanger), but that way the main concern is gone and you can handle the flow better from inside the Test Pilot add-on.

Then, with more time to test each individual add-on, we can ship a GoFaster update mid-cycle to add the other ones to the whitelist.
Flags: needinfo?(felipc)
Comment on attachment 8791404 [details]
Bug 1302846 - Add Test Pilot add-ons to e10s whitelist

https://reviewboard.mozilla.org/r/78818/#review77482

::: toolkit/mozapps/extensions/internal/E10SAddonsRollout.jsm:139
(Diff revision 2)
> +]
> +
> +// Main Test Pilot add-on + test add-ons for Firefox 49
> +const setTestPilotAddons49 = [
> +  ADDONS.TestPilot,
> +  ADDONS.TabCenter,
> +  ADDONS.ActivityStream,
> +  ADDONS.MinVid,
> +  ADDONS.PageShot,
> +  ADDONS.TrackingProtection,
> +  ADDONS.NoMore404s
> +]

nit: I know the existing one is missing the ending semicolon, but since you're here can you add them?

(after the `]`)
Attachment #8791404 - Flags: review?(felipc) → review+
Comment on attachment 8791459 [details] [diff] [review]
Bug 1302846 part 2 - Enable Test Pilot add-ons in E10S rollout

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

the "beta" line just changed on inbound (from bug 1297755) and it should remain unchanged (i.e., keep it configured for 50 as set by bug 1297755).

A pre-emptive r+ for the changes in the "release" line to get the conversation going. Although let's see what's ended up decided about it, specially with comment 13 in mind.
Attachment #8791459 - Flags: review?(felipc) → review+
(In reply to :Felipe Gomes (needinfo me!) from comment #13)
> My suggestion to reduce the risk here is the following:
> 
> Only ship with the Test Pilot add-on itself whitelisted. This means that the
> new install flow to Test Pilot can be used. Then installing the actual test
> add-ons (hmm confusing naming scheme, how are the called?) would still use
> the previous flow (with the doorhanger), but that way the main concern is
> gone and you can handle the flow better from inside the Test Pilot add-on.
> 
> Then, with more time to test each individual add-on, we can ship a GoFaster

Requiring restart for Test Pilot experiments will break the on-boarding flow and require refactoring of Test Pilot. I'd rather get reviews in prior to shipping. Our team has dedicated QA if needed to help this process.
> update mid-cycle to add the other ones to the whitelist.
(In reply to Kev Needham [:kev] from comment #11)
> (In reply to Andy McKay [:andym] from comment #9)
> > My only real concern here is matching the data between beta and release and
> > that we don't have this information on beta right now. I would probably say
> > that's not going to buy us much and we should add these. But please correct
> > me if I'm wrong.
> 
> My concern is it adds to complexity of analysis, because we're essentially
> doubling the number of addons, and they haven't been part of the test
> process to date. There's no throttling of the test cohort, so it means that
> if we see differences in the control to the test, it'll make it that much
> more difficult todetermine whether it's a specific add-on vs. e10s on the
> whole with add-ons to back out, and if the results are negative it could
> also affect leveling up in 50 to include MPC-true and WebExtensions in the
> e10s by default group. 
> 
> There's also a marketing campaign planned for these, which may create a
> larger test cohort than planned, which has been a concern to date.
> 
> Timing on this isn't great, but I think everyone understands that. From what
> I understand the marketing campaign is planned for end of month, so we have
> a little tie if we want to push a system addon update separately (which I
> realize has it's own set of processes, and may not be possible for 49 launch
> date).

I think what we need to focus on is identifying what work needs to be done in order to be more comfortable with the risk, then work towards that. I like the idea of whitelisting test pilot itself and possibly Activity Stream initially and then working through a prioritized list of additional add-ons.

For the record Kev I don't see the risk as being actual ( I run test pilot and other experiments with e10s all the time ) but I do see the value in engaging in real validation testing.
Can we apply Comment 13 to all the Test Pilot add-ons (including TP itself)?

If we add these to the whitelist halfway through the cycle, then we get some basis for comparison. For the first half of the cycle, the e10s-with-addons group would include the same add-ons as in Beta. Then, once the whitelist update gets pushed out, we can see whether there are any substantial changes to the stability metrics for that group. If necessary, we could also potentially do a before/after comparison analysis within the profile histories of profiles with TP.

Even if we don't do this, we should give a separate e10scohort tag to those profiles who have at least one of the Test Pilot add-ons installed. Ideally we could have separate tags for: no add-ons, TP addons only, original whitelist only, original whitelist + TP, all of whom would get e10s switched on. (I'm not sure if this tagging was already included in the patch). This will let us easily split out the groups for stability comparisons.

As it is, we don't really have a control group with the same characteristics (ie. add-ons) but with e10s staying off, so we'll be comparing between groups (ie. between e10s-with-addon and e10s-no-addons). If we can break out profiles that have TP add-ons, and if the groups are all big enough, I don't adding the TP add-ons will add significant risk to the analysis.
Flags: needinfo?(dzeber)
(In reply to Jeff Griffiths (:canuckistani) (:⚡︎) from comment #20)
> (In reply to Kev Needham [:kev] from comment #11)
> > (In reply to Andy McKay [:andym] from comment #9)
> > > My only real concern here is matching the data between beta and release and
> > > that we don't have this information on beta right now. I would probably say
> > > that's not going to buy us much and we should add these. But please correct
> > > me if I'm wrong.
> > 
> > My concern is it adds to complexity of analysis, because we're essentially
> > doubling the number of addons, and they haven't been part of the test
> > process to date. There's no throttling of the test cohort, so it means that
> > if we see differences in the control to the test, it'll make it that much
> > more difficult todetermine whether it's a specific add-on vs. e10s on the
> > whole with add-ons to back out, and if the results are negative it could
> > also affect leveling up in 50 to include MPC-true and WebExtensions in the
> > e10s by default group. 
> > 
> > There's also a marketing campaign planned for these, which may create a
> > larger test cohort than planned, which has been a concern to date.
> > 
> > Timing on this isn't great, but I think everyone understands that. From what
> > I understand the marketing campaign is planned for end of month, so we have
> > a little tie if we want to push a system addon update separately (which I
> > realize has it's own set of processes, and may not be possible for 49 launch
> > date).
> 
> I think what we need to focus on is identifying what work needs to be done
> in order to be more comfortable with the risk, then work towards that. I
> like the idea of whitelisting test pilot itself and possibly Activity Stream
> initially and then working through a prioritized list of additional add-ons.

Test Pilot uses mozAddonManager to handle the installation and uninstallation of individual experiments. This means that we will need to whitelist all Test Pilot + all experiments at the same time, and not just Test Pilot + some, or just Test Pilot. 

Here's the disaster path I want to avoid:

* Test Pilot is whitelisted.
* e10s user installs test pilot add on
* b/c Test Pilot is whitelisted e10s remains on
* user goes to install an experiment in test pilot, install fails over silently (no restart doorhanger)

here's a video of what that looks like: https://www.dropbox.com/s/8y5ointszsefue4/e10s-restart-required-silent-fail.mov?dl=0
 
> For the record Kev I don't see the risk as being actual ( I run test pilot
> and other experiments with e10s all the time ) but I do see the value in
> engaging in real validation testing.
Flags: needinfo?(jgriffiths)
(In reply to [:jgruen] from comment #22)
...
> Test Pilot uses mozAddonManager to handle the installation and
> uninstallation of individual experiments. This means that we will need to
> whitelist all Test Pilot + all experiments at the same time, and not just
> Test Pilot + some, or just Test Pilot. 
> 
> Here's the disaster path I want to avoid:
> 
> * Test Pilot is whitelisted.
> * e10s user installs test pilot add on
> * b/c Test Pilot is whitelisted e10s remains on
> * user goes to install an experiment in test pilot, install fails over
> silently (no restart doorhanger)

Thanks for clarifying, this is a potential disaster.

> here's a video of what that looks like:
> https://www.dropbox.com/s/8y5ointszsefue4/e10s-restart-required-silent-fail.
> mov?dl=0
>  
> > For the record Kev I don't see the risk as being actual ( I run test pilot
> > and other experiments with e10s all the time ) but I do see the value in
> > engaging in real validation testing.

Update: looks like we're engaging with Softvision to prioritize validation of the test pilot add-ons ASAP.
Flags: needinfo?(jgriffiths)
:canuckistani Not a material change for the issue, but for the sake of precision...

Test Pilot uses the SDK AddonManager API to install experiments, not the mozAddonManager API
Felipe, is this possible?

(In reply to Dave Zeber [:dzeber] from comment #21)
> 
> Even if we don't do this, we should give a separate e10scohort tag to those
> profiles who have at least one of the Test Pilot add-ons installed. Ideally
> we could have separate tags for: no add-ons, TP addons only, original
> whitelist only, original whitelist + TP, all of whom would get e10s switched
> on. (I'm not sure if this tagging was already included in the patch). This
> will let us easily split out the groups for stability comparisons.
Flags: needinfo?(felipc)
Not at this point for 49 as that would require significant code changes.

But both telemetry and crash reports include the list of add-ons that the user was using, so that information can still be derived on these sources. Just not by looking at the cohort name.
Flags: needinfo?(felipc)
(In reply to :Felipe Gomes (needinfo me!) from comment #26)
> Not at this point for 49 as that would require significant code changes.

Fair enough. We can do this check on the server side, but we'll need to either modify the derived datasets we look at or run against the raw pings.
(In reply to Dave Zeber [:dzeber] from comment #27)
> (In reply to :Felipe Gomes (needinfo me!) from comment #26)
> > Not at this point for 49 as that would require significant code changes.
> 
> Fair enough. We can do this check on the server side, but we'll need to
> either modify the derived datasets we look at or run against the raw pings.

Splitting out cohorts server side seems to obviate the risk Kev raised about confounding the e10s data by adding Test Pilot + Experiments. 

One note, before landing this patch, we will need to add Universal Search to the list of test pilot experiments being added.

The requisite info for universal search is:

> Universal Search:   universal-search@mozilla.com
> Min Version: 1.0.10
Flags: needinfo?(vlad)
Flags: needinfo?(kev)
wil, elan, kev, jgriffiths, jgruen and I met this morning. Updating this bug with a summary.

This bug remains a priority and wants to be resolved as soon as possible. In the meantime, the Test Pilot team is moving forward with a patch to mitigate the unfavorable UI in the current flow.

With respect to this bug: it looks like we're cleared for a checkin once comment 28 is applied (:vladikoff), and :shell has a chance to review and approve these additions to the list. It also looks like the cohort/analysis concern is cleared in comment 27 (:kev?).
Flags: needinfo?(wclouser)
(In reply to Dave Zeber [:dzeber] from comment #27)
> (In reply to :Felipe Gomes (needinfo me!) from comment #26)
> > Not at this point for 49 as that would require significant code changes.
> 
> Fair enough. We can do this check on the server side, but we'll need to
> either modify the derived datasets we look at or run against the raw pings.

Do we expect this cohort to be large enough to warrant special attention? I'm thinking that it might be of too small a size to move the needle as part of a larger cohort, or to give us statistically-significant results on its own.
(In reply to Chris H-C :chutten from comment #30)
> Do we expect this cohort to be large enough to warrant special attention?

Not necessarily - if we don't see any significant issues, we may not need to delve into this. The question is, if we do see a major increase in, say, crashes, is this due to: some change in 49? e10s getting turned on? e10s interacting with TP? Without a control group(s), we don't really have a basis to make these comparisons. However, if we do need to investigate, we can try separating TP users by checking add-ons in the raw pings
(In reply to [:jgruen] from comment #28)
> (In reply to Dave Zeber [:dzeber] from comment #27)
> > (In reply to :Felipe Gomes (needinfo me!) from comment #26)
> > > Not at this point for 49 as that would require significant code changes.
> > 
> > Fair enough. We can do this check on the server side, but we'll need to
> > either modify the derived datasets we look at or run against the raw pings.
> 
> Splitting out cohorts server side seems to obviate the risk Kev raised about
> confounding the e10s data by adding Test Pilot + Experiments. 
> 
> One note, before landing this patch, we will need to add Universal Search to
> the list of test pilot experiments being added.
> 
> The requisite info for universal search is:
> 
> > Universal Search:   universal-search@mozilla.com
> > Min Version: 1.0.10

Added 
Flags: needinfo?(vlad)
Clearing kev and shell's NI as this was approved over email.

NI :felipe for review on comment 32. Once done, we'll be requesting uplift straight to mozilla-release with the hopes of catching a dot release.
Flags: needinfo?(sescalante)
Flags: needinfo?(kev)
Flags: needinfo?(felipc)
No need for a re-review there. It was just one addition to the list, and the r+ was carried forward.
Flags: needinfo?(felipc)
Comment on attachment 8791404 [details]
Bug 1302846 - Add Test Pilot add-ons to e10s whitelist

Approval Request Comment
[Feature/regressing bug #]:
See comment 22 for information. We made some mitigations in the UI to warn users of the restart, but a significant number of our users are exposed to this.

[User impact if declined]:
The requirement to restart the browser after install will affect many users who are hoping to get in and start experimenting with Test Pilot

[Describe test coverage new/current, TreeHerder]:
[Risks and why]: 
[String/UUID change made/needed]:
Attachment #8791404 - Flags: approval-mozilla-release?
Comment on attachment 8791459 [details] [diff] [review]
Bug 1302846 part 2 - Enable Test Pilot add-ons in E10S rollout

See comment 36 for reasons.
Attachment #8791459 - Flags: approval-mozilla-release?
NOTE:  we need to add a few more ID's to this list.  Sending to Felipe in next hour or so - to make change and request mconley reviews.  Jorge's providing info to Shell =]
Flags: needinfo?(sescalante)
Flags: needinfo?(jorge)
Here are the additions:

YouTube High Definition
https://addons.mozilla.org/firefox/addon/youtube-high-definition/
7b1bf0b6-a1b9-42b0-b75d-252036438bdc 48.3


Weather Underground
https://addons.mozilla.org/firefox/addon/weather-forecast-plus/
jid1-w3xH9kJhd3KJUp@jetpack 0.2.1


Dictionary Anywhere
https://addons.mozilla.org/firefox/addon/dictionary-anywhere/
jid0-fbHwsGfb6kJyq2hj65KnbGte3yT@jetpack 0.1.9


Gmail Notifier
https://addons.mozilla.org/firefox/addon/gmail-notifier-restartless/
jid0-GjwrPchS3Ugt7xydvqVK4DQk8Ls@jetpack 0.6.5
Flags: needinfo?(sescalante)
Flags: needinfo?(jorge)
Flags: needinfo?(felipc)
Changing bug title to also include the 4 add-ons planned to be presented in the Discovery Pane during 49.
Flags: needinfo?(felipc)
Summary: Add Test Pilot add-ons to e10s whitelist → Add Test Pilot/Future Discovery Pane add-ons to e10s whitelist
This removes the "test pilot only" set because the Test Pilot team said that one wouldn't be useful, and adds the 4 new requested add-ons (from comment 39) into a new set
Attachment #8793910 - Flags: review?(mconley)
And this enables this configuration in the system add-on, bumping its version.

(This patch makes the patch "part 2" obselete.)
Attachment #8791459 - Attachment is obsolete: true
Attachment #8791459 - Flags: approval-mozilla-release?
Attachment #8793911 - Flags: review?(mconley)
Comment on attachment 8793910 [details] [diff] [review]
Add new requested add-ons to the list

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

Tentative r+, assuming the testpilot sets never got shipped to users (I don't recall that they did, but just making sure).

::: toolkit/mozapps/extensions/internal/E10SAddonsRollout.jsm
@@ -138,5 @@
> -]
> -
> -// Main Test Pilot add-on for Firefox 49
> -const setTestPilot49 = [
> -  ADDONS.TestPilot

Are we certain that this removal, plus the changes on line 196-197, don't violate http://searchfox.org/mozilla-central/rev/8910ca900f826a9b714607fd23bfa1b37a191eca/toolkit/mozapps/extensions/internal/E10SAddonsRollout.jsm#67 ? Like, none of this got sent out to users, right?

@@ +165,5 @@
>    ADDONS.UniversalSearch,
> +];
> +
> +const set49Expanded = [
> +  ...set49Release,

Might as well be consistent and either put the space before set49Release, or get rid of it on the next line. I'm not sure which one is better. But consistency trumps. :)
Attachment #8793910 - Flags: review?(mconley) → review+
Comment on attachment 8793911 [details] [diff] [review]
Enable new set of add-ons in E10srollout

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

::: browser/extensions/e10srollout/bootstrap.js
@@ +16,5 @@
>    "release" : 1.0,  // 100%
>  };
>  
>  const ADDON_ROLLOUT_POLICY = {
> +  "beta"    : "49expandeda", // 10 tested add-ons + Test Pilot add-ons + any WebExtension

Not 10 tested add-ons anymore - we just added a bunch more. Is it worth just dropping the number at this point?

::: browser/extensions/e10srollout/install.rdf.in
@@ +9,5 @@
>       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.4</em:version>

:D
Attachment #8793911 - Flags: review?(mconley) → review+
(In reply to Mike Conley (:mconley) - (needinfo me!) from comment #43)
> > -// Main Test Pilot add-on for Firefox 49
> > -const setTestPilot49 = [
> > -  ADDONS.TestPilot
> 
> Are we certain that this removal, plus the changes on line 196-197, don't
> violate
> http://searchfox.org/mozilla-central/rev/
> 8910ca900f826a9b714607fd23bfa1b37a191eca/toolkit/mozapps/extensions/internal/
> E10SAddonsRollout.jsm#67 ? Like, none of this got sent out to users, right?

Correct. Those sets were from the previous patch on this bug, which hasn't landed yet. If we land them all together this should be ok.


> 
> @@ +165,5 @@
> >    ADDONS.UniversalSearch,
> > +];
> > +
> > +const set49Expanded = [
> > +  ...set49Release,
> 
> Might as well be consistent and either put the space before set49Release, or
> get rid of it on the next line. I'm not sure which one is better. But
> consistency trumps. :)

Yeah I just noticed that after posting.. my eye twitched too. I'll fix it on landing

(In reply to Mike Conley (:mconley) - (needinfo me!) from comment #44)
> >  const ADDON_ROLLOUT_POLICY = {
> > +  "beta"    : "49expandeda", // 10 tested add-ons + Test Pilot add-ons + any WebExtension
> 
> Not 10 tested add-ons anymore - we just added a bunch more. Is it worth just
> dropping the number at this point?

Good call
Comment on attachment 8793910 [details] [diff] [review]
Add new requested add-ons to the list

Approval Request Comment
[Feature/regressing bug #]: This adds 4 other add-ons that have been tested and are planned to be featured in the "Discovery Pane" of the Add-ons Manager.

Since we are updating the whitelist with the Test Pilot add-ons, might as well take the opportunity and do it here at the same time.
Attachment #8793910 - Flags: approval-mozilla-release?
Comment on attachment 8793911 [details] [diff] [review]
Enable new set of add-ons in E10srollout

Approval Request Comment
[Feature/regressing bug #]: This configures the system add-on to use the newly updated list
Attachment #8793911 - Flags: approval-mozilla-release?
Hi Lizz - wondering if you've had a chance to review this for uplift to release? We'd like to be in the position to catch a ride on 49.0.2 if/when it happens.
Flags: needinfo?(lhenry)
So far, no driver for a 49.0.2 has emerged. We have several potential ridealongs. We can take another look next week and re-evaluate. Here, it sounds like people trying out Test Pilot would have to restart to see the changes.  For a dot release, one thing we know is that we "orphan" a significant amount of users with each dot release. So, right now, I'm trying to avoid having to do one, and I'm not sure the restart users outweigh that consideration of users stuck on old versions of Firefox.
Flags: needinfo?(lhenry)
NI :ritu - assuming there is no dot release, can we please uplift this to 50?
Flags: needinfo?(rkothari)
(In reply to Cory Price [:ckprice] from comment #50)
> NI :ritu - assuming there is no dot release, can we please uplift this to 50?

Sure if that's the decision of the product team. Felipe, do you want to request an uplift to Aurora51 and Beta50 for this one?
Flags: needinfo?(rkothari) → needinfo?(felipc)
I made some simplifications to the patch due to it not being used in release, and squashed everything together to simplify the approval request

Approval Request Comment
[Feature/regressing bug #]: Add the current set of Test Pilot and Discovery Pane addons to the e10s rollout whitelist. This _might_ not be necessary if bug 1297755 rolls out to release, but that's still unclear, so it's better to have these lists ready to go. 
[User impact if declined]: Installing these add-ons will require a restart and will disable e10s
[Describe test coverage new/current, TreeHerder]: just adding things to the whitelist already in use
[Risks and why]: minimal, limited to the e10s whitelist
[String/UUID change made/needed]: none
Flags: needinfo?(felipc)
Attachment #8798579 - Flags: review+
Attachment #8798579 - Flags: approval-mozilla-beta?
Attachment #8798579 - Flags: approval-mozilla-aurora?
Comment on attachment 8798579 [details] [diff] [review]
Patch for aurora and beta, author=vladikoff r=felipe

I am told these add-ons have been tested in e10s mode, Aurora51+, Beta50+
Attachment #8798579 - Flags: approval-mozilla-beta?
Attachment #8798579 - Flags: approval-mozilla-beta+
Attachment #8798579 - Flags: approval-mozilla-aurora?
Attachment #8798579 - Flags: approval-mozilla-aurora+
We are planning a dot release now and could consider taking this as a ridealong. That way, users who try out the Test Pilot add-ons can also enable e10s and they won't have to restart. 

Do we have any info about how well this is working on beta?
Hi Liz, 

I'm one of the embedded QAs for Test Pilot. We are testing 90% of the time Test Pilot add-on together with all the experiments on Nightly channel with e10s enabled. From July and until now, no major issues were observed between e10s and anything from Test Pilot.

However, if you guys want, we can double check this by using the latest Fx beta next week and provide some results. Just let us know so we can schedule the tests.
Hi John,
Hi Cory,

There is a dot release that is being looked at shipping, is this still needed before Fx50?  I think test pilot was trying to hit a marketing date - and i'm not sure if that is still relevant.
Flags: needinfo?(lhenry)
Flags: needinfo?(jgruen)
Flags: needinfo?(cprice)
From talking with shell and Cory, sounds like we would prefer to hold off until 50 at this point.
Flags: needinfo?(lhenry)
(In reply to Liz Henry (:lizzard) (needinfo? me) from comment #59)
> From talking with shell and Cory, sounds like we would prefer to hold off
> until 50 at this point.
Confirmed -- it's a nice to have on the Test Pilot side at this point, but we can wait a few more weeks.
Flags: needinfo?(jgruen)
Flags: needinfo?(cprice)
Attachment #8791404 - Flags: approval-mozilla-release? → approval-mozilla-release-
Attachment #8793910 - Flags: approval-mozilla-release? → approval-mozilla-release-
Attachment #8793911 - Flags: approval-mozilla-release? → approval-mozilla-release-
Too late for firefox 52, mass-wontfix.
Wil, I think we can close this now?
Flags: needinfo?(wclouser)
I think so.  Thanks everyone!
Status: NEW → RESOLVED
Closed: 2 years ago
Flags: needinfo?(wclouser)
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.