1306125 - [telemetry-experiment] layout.show_previous_page on beta channel for non-10s users

Status: NEW

(Core :: Layout, defect, P3)

Description

[:Harald Kirschner :digitarald]

Follow up trial for layout.show_previous_page on beta.

Problem: layout.show_previous_page changes the perceived page load. While the browser currently keeps the old inactive page around, the pref will cause drawing the page background when available. This might generally be perceived as faster loading, as it shortens the wait from interaction to visual feedback but might cause visual glitches, layout jitter and additional reflows/styling.

Questions:
- Will layout.show_previous_page regress page load performance significantly?
- Will the change annoy users and cause lower engagement?
- What is the combined impact of layout.show_previous_page and nglayout.paint.delay

Independent Variables:
- layout.show_previous_page: on, off (default)
- nglayout.paint.delay: 0, 250 (default)

Depended Variables:
- Page load metrics: SIMPLE_MEASURES_FIRSTPAINT, FX_PAGE_LOAD, tbd
- Phase 1 User engagement metrics (bug 1252625)

Population: Users on beta channel without e10s

Sampling:
- 50% random sample from population
- Randomly assigned to
  - Control: Defaults
  - Test 1: layout.show_previous_page on
  - Test 2: layout.show_previous_page on && nglayout.paint.delay 0

Risks:
- Sample size too small as a lot of users have e10s
- 3rd test group might be needed to separate impact of the 2 prefs

Comment 1

[Benjamin Smedberg]

Harald what info do you need from me?

Comment 2

[:Harald Kirschner :digitarald]

I hoped for some review of the experimental design. Some people have looked at it so far and I iterated on the User Story; so I think we got the details down.

I'll work on the addon next so there is actual code to review.

Comment 3

[(Currently slow to respond) Philipp Sackl [:phlsa] (Firefox UX) please use needinfo]

Hey Harald! I was wondering what mechanism we use to get results back. Is it going to be a survey or something different? That should probably be included here :)

Comment 4

[:Dominik Strohmeier [:dstrohmeier]]

Hi Philipp,

the study will run on Telemetry Experiments, aka Telex, as included in the title ;)

This is a specific feature in our Telemetry system that allows Firefox to automatically download and run specially-designed restartless addons based on certain conditions. More info here: https://wiki.mozilla.org/Telemetry/Experiments

Right now, there is an additional proposal under discussion to build a similar tool based on SHIELD that will allow for follow-up surveys.

Helps?

Comment 5

[(Currently slow to respond) Philipp Sackl [:phlsa] (Firefox UX) please use needinfo]

Yes, thanks!
My question was mostly about how we will know that the performance is perceived as higher by users after the experiment.
Between your answer here and our conversations in Berlin that has been cleared up :)
If I understood you correctly, we will watch engagement metrics and verify their validity with a parallel SHIELD study.

Comment 6

[:Harald Kirschner :digitarald]

Attachment: bug-1306125-experiment.patch

Comment 7

[:Felipe Gomes (needinfo for replies!)]

Comment on attachment 8800758 [details] [diff] [review]
bug-1306125-experiment.patch

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

::: experiments/show_previous_page-beta/bootstrap.js
@@ +41,5 @@
> +
> +    let defaultPrefs = new Preferences({defaultBranch: true});
> +    switch (branch) {
> +      case "control":
> +      case "disqualified-e10s":

"disqualified-e10s" case can be removed

::: experiments/show_previous_page-beta/install.rdf
@@ +10,5 @@
> +    <!-- Firefox -->
> +    <em:targetApplication>
> +      <Description>
> +        <em:id>{ec8030f7-c20a-464f-9b0e-13a3a9e97384}</em:id>
> +        <em:minVersion>50.0b6</em:minVersion>

You should use 50.0 for minVersion and 50.* for maxVersion

::: experiments/show_previous_page-beta/manifest.json
@@ +6,5 @@
> +  "info"        : "<p><a href=\"https://bugzilla.mozilla.org/show_bug.cgi?id=1306125\">Related bug</a></p>",
> +  "manifest"    : {
> +    "id"               : "[e-mail]",
> +    "startTime"        : 1476378149,
> +    "endTime"          : 1476379421,

endTime needs to be updated with a correct value

@@ +7,5 @@
> +  "manifest"    : {
> +    "id"               : "[e-mail]",
> +    "startTime"        : 1476378149,
> +    "endTime"          : 1476379421,
> +    "maxActiveSeconds" : 1209600,

is 2 weeks the expected length of this experiment?

Comment 8

[:Harald Kirschner :digitarald]

> endTime needs to be updated with a correct value

I assume this is just startTime + maxActiveSeconds in this case?

> is 2 weeks the expected length of this experiment?

Yes, as planned in the initial description.

Comment 9

[:Harald Kirschner :digitarald]

Attachment: bug-1306125-experiment-r2.patch

Comment 10

[:Felipe Gomes (needinfo for replies!)]

I believe you posted the same patch as before

(In reply to Harald Kirschner :digitarald from comment #8)
> > endTime needs to be updated with a correct value
> 
> I assume this is just startTime + maxActiveSeconds in this case?

Yeah, but you can keep startTime in the past and have endTime be a bit more forward than (startTime + maxActiveSeconds) to account for the time until we actually release it.

endTime is a hard deadline, but for each user the experiment will run until either endTime or (startTime + maxActiveSeconds), whichever comes first

Comment 11

[:Felipe Gomes (needinfo for replies!)]

I mean the actual start time on their machine, not the specified one. In simpler words: it will run for 14 days after installed, or stop earlier if it reaches endTime.

Comment 12

[:Harald Kirschner :digitarald]

> I mean the actual start time on their machine, not the specified one. In simpler words: it will run for 14 days after installed, or stop earlier if it reaches endTime.

Ok, that's how I understood it, which is why I added "in this case" as some experiments could be time limited.

Comment 13

[:Felipe Gomes (needinfo for replies!)]

Comment on attachment 8800857 [details] [diff] [review]
bug-1306125-experiment-r2.patch

> I believe you posted the same patch as before

Not sure if you noticed this in the last comment, so clearing the r? to make it clear. The patch r2 is identical to the first version :)

Comment 14

[:Harald Kirschner :digitarald]

Attachment: bug-1306125-experimen-r3.patch

Attached correct r2

Comment 15

[:Harald Kirschner :digitarald]

Dominik, if you have no concerns with the current experiment setup, please send an intent to ship email to release-drivers, so we can kick this off Tuesday next week (I am off tomorrow and we need Monday to create the XPIs).

Comment 16

[:Dominik Strohmeier [:dstrohmeier]]

Intend to ship sent to release-drivers.
Let's make sure next week that also our experiment tracking is in place by Monday EOD.

Comment 17

[:Harald Kirschner :digitarald]

Mike, there was a concern on release-drivers on possible impact on crashes. Just want to make sure before I reply if there is any risk for crashes due to the pref change?

Comment 18

[Mike Conley (:mconley) (:⚙️)]

(In reply to Harald Kirschner :digitarald from comment #17)
> Mike, there was a concern on release-drivers on possible impact on crashes.
> Just want to make sure before I reply if there is any risk for crashes due
> to the pref change?

I can't think of one - though let's double check with tn, who reviewed the patch that added the pref (bug 1297996).

tn - I assume there's no added crash risk here, right? I mean, software is complicated, but we're about equally safe with this pref in either state, right?

Comment 19

[Timothy Nikkel (:tnikkel)]

Hmm, if I were making this change on nightly for all users I would want it to ride to trains to get an answer to such a question. But we aren't doing that.

Comment 20

[Mike Conley (:mconley) (:⚙️)]

(In reply to Harald Kirschner :digitarald from comment #17)
> Mike, there was a concern on release-drivers on possible impact on crashes.
> Just want to make sure before I reply if there is any risk for crashes due
> to the pref change?

I have to agree with tn. We uplifted this patch to beta, but really have no idea what impact it will have to stability out in the wild, since we've never turned it on out there. From inspection, I can't see where things would go wrong, but software is complicated.

Would it be possible to measure the stability with a smaller cohort on beta (or perhaps a pre-release channel) to measure any impact to stability?

Comment 21

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Harald and I discussed it this morning as well. We agree that the best next steps is to re-target this to Beta51. You could also turn this on in Aurora51 now so it gets a bit more testing on that channel during the next 3 weeks before Merge day.

Comment 22

[:Harald Kirschner :digitarald]

Filed bug 1311448 for the Aurora tests to keep this bug for beta.