1521168 - Export Screenshots 37.1.0

Status: RESOLVED FIXED

(Firefox :: Screenshots, defect)

Description

[Ian Bicking (:ianbicking)]

This release focuses on disabling upload, and providing migration instructions for existing users.

Changelog:

• Cache startup server check. This applies to the migration code that runs on startup. Fixes #5332 9f9b04a
• Show context menu when there's a text selection (#5334). Fixes #5330 ce5db49
• Enable pageAction on reader mode (#5282). Fixes #5235 242f4a2
• Add additional requestIdleCallback wait around migration server check (#5335). Fixes #5333 6506b2b
• • Screenshots keyboard shortcut.
    Just go with ctrl + shift + s, which will be command + shift + s on mac. Fixes #5089 dc35fa0
• Metrics improvements (#5315)
  • stop sending metrics if server isn't available
  • Also adds logic to not send timing information if server is unavailable
  • only send analytics for 10% of user. This random generates a number for users when Screenshots is loaded, then saves that number, to determine who will actually send metrics. Fixes #5293 Fixes #5264 52bda28
• make Enter download a shot. Fixes #5259 d919707
• Update first slide text 43c8db5
• Fix the tests. Remove tests only related to uploading. Convert some uploading tests to use copy or download. Tests seemed to interact in new ways with these changes, so this also changes around the initialization some. Autoselect had intermittent problems (but frequent) and doing a retry seems to help, as does moving the mouse to different locations. 521c355
• lots of ui changes for a serverless future 23ae30b
• Remove My Shots test, since My Shots button is usually gone 534eba4
• query user status from server. Also remove My Shots links for non-server-users
  pop open server page for people with indefinite shots. Fixes #5260 Fixes #5263 Fixes #5261 829560f
• use incognito: spanning in manifest (#5294)Soon Firefox will disable add-ons without this, or require people to opt-in. Fixes #5244 3314638
• turn down logging if sendEvent fails. This also turns off sending events for the session if it ever gets a 410 Gone response. Fixes #5277 ffd7294
• Import change from Bug 1512419. See also https://phabricator.services.mozilla.com/D13908 8e97ea6
• Import change from Bug 1501738. See also https://phabricator.services.mozilla.com/D9676 85adf6a
• Import change from Bug 1501791. See also https://phabricator.services.mozilla.com/D9696 8289709

Comment 1

[Ian Bicking (:ianbicking)]

Attachment: Bug 1521168 - Export Screenshots 37.1.0 to Firefox (translations only); r?_6a68

MozReview-Commit-ID: JNIGRJMNsyz

Comment 2

[Ian Bicking (:ianbicking)]

Attachment: Bug 1521168 - Export Screenshots 37.1.0 to Firefox (code excluding translations); r?_6a68

MozReview-Commit-ID: 3lIF2XGAwlj

Depends on D18102

Comment 3

[Ian Bicking (:ianbicking)]

Try run: https://treeherder.mozilla.org/#/jobs?repo=try&revision=7ab89299ba915fc0a4201bbf3af93f003c50d84f
Talos run: https://treeherder.mozilla.org/perf.html#/compare?originalProject=mozilla-central&newProject=try&newRevision=7ab89299ba915fc0a4201bbf3af93f003c50d84f&framework=1&showOnlyImportant=1&selectedTimeRange=172800

Comment 4

[Ian Bicking (:ianbicking)]

Comment on attachment 9040156 [details]
Bug 1521168 - Export Screenshots 37.1.0 to Firefox (code excluding translations); r?_6a68

Mossop: can you specifically look at the changes to startBackground.js? This adds a check (slightly delayed after add-on startup) both to add-on storage, and if the user has used the server, then it also asks the server if the user has active or indefinitely saved shots for the purpose of warning the user. I'm not sure if this is the best balance of doing the work to notify users, and the downside of piling more onto Firefox startup.

(Note these checks are only for messaging the server shutdown, and will be removed in Firefox 68)

Comment 5

[Jared Hirsch [:jhirsch] (he/him) (Needinfo please)]

Comment on attachment 9040155 [details]
Bug 1521168 - Export Screenshots 37.1.0 to Firefox (translations only); r?_6a68

Adding flod as a reviewer for the string changes.

Comment 6

[Francesco Lodolo [:flod]]

Comment on attachment 9040155 [details]
Bug 1521168 - Export Screenshots 37.1.0 to Firefox (translations only); r?_6a68

Asking on phabricator should be enough (also, use blocking reviewer when there's more than one)

Comment 7

[Jared Hirsch [:jhirsch] (he/him) (Needinfo please)]

Comment on attachment 9040156 [details]
Bug 1521168 - Export Screenshots 37.1.0 to Firefox (code excluding translations); r?_6a68

still confused about phabricator vs BMO. I guess I need to r+ here as well...

Comment 8

[Jared Hirsch [:jhirsch] (he/him) (Needinfo please)]

Attachment: graph of autoland results for tresize opt e10s stylo windows10-64 test

Looking at the Talos results, one major regression on windows is:

4.42% windows10-64 / tresize opt e10s stylo

Looking at the graph of recent results on autoland, I see a bimodal distribution over the past few days; this regression fits into the higher-valued end of the graph. (When comparing our result to the attached graph, note that the absolute value for this push is 7.73.)

Comment 9

[Jared Hirsch [:jhirsch] (he/him) (Needinfo please)]

Attachment: screenshot of talos results table

(To keep everything in one spot, I'll attach the table of regression data)

Comment 10

[Jared Hirsch [:jhirsch] (he/him) (Needinfo please)]

Attachment: graph of autoland results for tp5n main_normal_fileio opt e10s stylo win10-64

Another of the windows regressions is:

9.01% windows10-64 / tp5n main_normal_fileio opt e10s stylo

This result also falls in the current range seen on autoland. (Our absolute value was about 2.3 million.)

Comment 11

[Jared Hirsch [:jhirsch] (he/him) (Needinfo please)]

Attachment: graph of autoland results for sessionrestore_no_auto_restore opt e10s stylo on win7-32

The final observed windows regression is:

2.64% windows7-32 / sessionrestore_no_auto_restore opt e10s stylo

Looking at the autoland graph, our average value of 351.2 falls a bit outside the meat of the curve, though there have been a few outliers this week.

There is a corresponding (non-significant) regression of 1.46% for windows10-64 for the same test, which does fall within the range of autoland variation.

Probably would be a good idea to re-run this test and see if the regressions are still significant. I can't imagine why 32-bit windows 7 would be more impacted than 64-bit win 10, though.

Comment 12

[Jared Hirsch [:jhirsch] (he/him) (Needinfo please)]

Finally, without attaching curves for each of the linux64 regressions, I'll just note that these fall within the observed points on the autoland curve:

7.60% linux64-qr / about_preferences_basic opt e10s stylo
2.94% linux64 / sessionrestore opt e10s stylo
2.56% linux64-qr / sessionrestore_no_auto_restore opt e10s stylo
2.23% linux64 / tp5o responsiveness opt e10s stylo

I think we're good to land, provided the re-run of sessionrestore_no_auto_restore opt e10s stylo on win7-32 looks normal.

Comment 13

[Jared Hirsch [:jhirsch] (he/him) (Needinfo please)]

I don't see any regressions in the Talos results from the latest Try run, so I think we are good to land.

Comment 14

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/61b408c4e663
Export Screenshots 37.1.0 to Firefox (translations only); r=_6a68,flod
https://hg.mozilla.org/integration/autoland/rev/cc26e0ca64e8
Export Screenshots 37.1.0 to Firefox (code excluding translations); r=_6a68

Comment 15

[Razvan Maries]

https://hg.mozilla.org/mozilla-central/rev/61b408c4e663
https://hg.mozilla.org/mozilla-central/rev/cc26e0ca64e8