Closed Bug 1521168 Opened 2 years ago Closed 2 years ago

Export Screenshots 37.1.0


(Firefox :: Screenshots, defect)

Not set



Firefox 67
Tracking Status
firefox67 --- fixed


(Reporter: ianbicking, Assigned: ianbicking)




(6 files)

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


  • 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 8e97ea6
  • Import change from Bug 1501738. See also 85adf6a
  • Import change from Bug 1501791. See also 8289709
Summary: Export Screenshots 37.0.0 → Export Screenshots 37.1.0

MozReview-Commit-ID: 3lIF2XGAwlj

Depends on D18102

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)

Attachment #9040156 - Flags: review?(dtownsend)
Attachment #9040155 - Flags: review?(jhirsch)
Attachment #9040156 - Flags: review?(jhirsch)

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.

Attachment #9040155 - Flags: review?(francesco.lodolo)
Attachment #9040155 - Flags: review?(jhirsch) → review+

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)

Attachment #9040155 - Flags: review?(francesco.lodolo)
Attachment #9040156 - Flags: review?(dtownsend) → review+

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

Attachment #9040156 - Flags: review?(jhirsch) → review+

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.)

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

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.)

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.

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.

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

Pushed by
Export Screenshots 37.1.0 to Firefox (translations only); r=_6a68,flod
Export Screenshots 37.1.0 to Firefox (code excluding translations); r=_6a68
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 67
Depends on: 1531650
Blocks: 1365757
Regressions: 1562685
Regressions: 1666860
You need to log in before you can comment on or make changes to this bug.