1390985 - Update Screenshots to version 16.1.0

Status: RESOLVED FIXED

(Firefox :: Screenshots, enhancement)

Description

[Ian Bicking (:ianbicking)]

This (finally!) merges GitHub master. Changelog:

https://github.com/mozilla-services/screenshots/blob/master/CHANGELOG.md#version-1600

Note that we've been using one version number for the server and add-on, so versions 11-15 have been server-only releases. (We'll separate the two eventually, but haven't yet.)

The changes aren't that easy to copy over, so it would be best to view them on the link, but for the record I'll put them down here:

Version 16.0.0

* Fix tests failing when run against photon-y Firefox without MOZ_PHOTON_THEME defined [beec56b](https://github.com/mozilla-services/screenshots/commit/beec56b)

* Deal with lack of MOZ_PHOTON_THEME on 57+ [3e9eba3](https://github.com/mozilla-services/screenshots/commit/3e9eba3)
* Make run-addon work with new legacy pref. Fixes [#3333](https://github.com/mozilla-services/screenshots/issues/3333) [b9a776b](https://github.com/mozilla-services/screenshots/commit/b9a776b)
* Immediately exit when Firefox is exiting. Fixes [#3323](https://github.com/mozilla-services/screenshots/issues/3323) [916c353](https://github.com/mozilla-services/screenshots/commit/916c353)
* Fix tests, enable legacy extensions via pref during tests ([#3324](https://github.com/mozilla-services/screenshots/issues/3324)) [c60fd37](https://github.com/mozilla-services/screenshots/commit/c60fd37)
* Make tests resilient to a browserAction or pageAction ([#3317](https://github.com/mozilla-services/screenshots/issues/3317)) Fixes [#3306](https://github.com/mozilla-services/screenshots/issues/3306) [3c15014](https://github.com/mozilla-services/screenshots/commit/3c15014)
* Replace the WebExtension browser action with a Photon page action. ([#3239](https://github.com/mozilla-services/screenshots/issues/3239))
  * Replace the WebExtension browser action with a Photon page action.
  This removes the browser action and adds a Photon page action,
  pursuant to https://bugzilla.mozilla.org/show_bug.cgi?id=1366041.
  Right now Photon page actions are unrelated to WebExtension page
  actions unfortunately, which means that this patch has to do most
  of its work in bootstrap.js.  The WebExtension part passes
  messages to bootstrap.js to handle clicks and update the action's
  title and icon as necessary.
  * Test fix: Replace the WebExtension browser action with a Photon page action.
  Fix the test for the previous commit.
  * Update SHOOTER_BUTTON_ID in test.js.
  * Use the Photon page action when supported, the WebExtension browser action when not.
  Extend the Photon-related port used between bootstrap.js and the
  WebExtension so that bootstrap.js can tell the WebExtension
  whether it's OK to use the Photon page action.  The WebExtension
  should not attempt to use the browser action when Photon is
  enabled because bootstrap.js will have removed the browser
  action's navbar button.
  Modify the test so that it checks the Photon page action when
  Photon is enabled and the browser action when it's not.
  * Fix the expected button label in test.js for the Photon page action.
  * Maybe fix PageActions eslint errors. [4396250](https://github.com/mozilla-services/screenshots/commit/4396250)
* remove high DPI shot capture for full page [fde181d](https://github.com/mozilla-services/screenshots/commit/fde181d)
* Sets background highlight width to 100% [aee88eb](https://github.com/mozilla-services/screenshots/commit/aee88eb)
* Remove duplicate drawWindow call for shot preview [2a5dd27](https://github.com/mozilla-services/screenshots/commit/2a5dd27)
* Update shot preview save icon [735b3f9](https://github.com/mozilla-services/screenshots/commit/735b3f9)
* Add reason to webExtension.startup and shutdown Will be useful when https://bugzilla.mozilla.org/show_bug.cgi?id=1372750 and
  https://bugzilla.mozilla.org/show_bug.cgi?id=1373749 are fixed [d97d4a7](https://github.com/mozilla-services/screenshots/commit/d97d4a7)
* Protect against an empty IPC response ([#3037](https://github.com/mozilla-services/screenshots/issues/3037))This happened in some weird corner case while debugging [14df39d](https://github.com/mozilla-services/screenshots/commit/14df39d)
* Guard access of this.save when un-disabling the save button ([#3030](https://github.com/mozilla-services/screenshots/issues/3030)). This can happen after the worker has been torn down, and this.save isn't defined [cf6e72a](https://github.com/mozilla-services/screenshots/commit/cf6e72a)
* Set dimensions for icon and add to startup ([#3136](https://github.com/mozilla-services/screenshots/issues/3136)) [9959ede](https://github.com/mozilla-services/screenshots/commit/9959ede)
* Disable Screenshots in private windows. Fixes [#3120](https://github.com/mozilla-services/screenshots/issues/3120) [aefc639](https://github.com/mozilla-services/screenshots/commit/aefc639)
* about:home is not treated like about:newtab ([#3088](https://github.com/mozilla-services/screenshots/issues/3088)). Fixes [#3029](https://github.com/mozilla-services/screenshots/issues/3029) [4633694](https://github.com/mozilla-services/screenshots/commit/4633694)
* preview shot before saving full page/visibleremove addToMyShots [c87db61](https://github.com/mozilla-services/screenshots/commit/c87db61)

Comment 1

[Ian Bicking (:ianbicking)]

Attachment: Bug 1390985 - Export Screenshots 16.1.0 to Firefox

Review commit: https://reviewboard.mozilla.org/r/169250/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/169250/

Comment 2

[Ian Bicking (:ianbicking)]

Attachment: Bug 1390985 - Export Screenshots 16.0.0 to Firefox (all code changes)

Review commit: https://reviewboard.mozilla.org/r/169252/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/169252/

Comment 3

[Ian Bicking (:ianbicking)]

Note SoftVision/Cosmin has QA'd a Nightly build with this version of the add-on.

At some point we should talk about the review process for ongoing updates – it's not easy to review a big patch like this, but then it wouldn't be easy to review a dozen smaller patches either.

Comment 4

[Kris Maglione [:kmag]]

Comment on attachment 8897950 [details]
Bug 1390985 - Export Screenshots 16.1.0 to Firefox

https://reviewboard.mozilla.org/r/169250/#review175520

Comment 5

[Kris Maglione [:kmag]]

Comment on attachment 8897951 [details]
Bug 1390985 - Export Screenshots 16.0.0 to Firefox (all code changes)

https://reviewboard.mozilla.org/r/169252/#review176970

r=me with follow-ups filed for CSS-based handling of icon selection and browserAction hiding, and the rest of the issues fixed.

::: browser/extensions/screenshots/bootstrap.js:74
(Diff revision 1)
> +  ITEM_ID: "appMenu-library-screenshots",
> +
> +  init(webExtension) {
> +    this._initialized = true;
> +    let permissionPages = [...webExtension.extension.permissions].filter(p => (/^https?:\/\//i).test(p));
> +    this.PAGE_TO_OPEN = permissionPages.length ? permissionPages[0].replace(/\*$/, "") : "https://screenshots.firefox.com/";

You shouldn't expect the ordering of host permissions to be deterministic, so if you're expecting only one host, please check `length == 1`

::: browser/extensions/screenshots/bootstrap.js:95
(Diff revision 1)
> +      let item = win.document.getElementById(this.ITEM_ID);
> +      if (item) {
> +        item.remove();
> +      }
> +    }
> +    CustomizableUI.removeListener(this);

Please set `this._initialized` to false here.

::: browser/extensions/screenshots/bootstrap.js:111
(Diff revision 1)
> +    let {nextSibling} = libraryViewInsertionPoint;
> +    let item = win.document.createElement("toolbarbutton");
> +    item.className = "subviewbutton subviewbutton-iconic";
> +    item.addEventListener("command", () => win.openUILinkIn(this.PAGE_TO_OPEN, "tab"));
> +    item.id = this.ITEM_ID;
> +    let iconURL = win.devicePixelRatio >= 1.1 ? this.ICON_URL_2X : this.ICON_URL;

`devicePixelRatio` can change as screens are attached or removed, or windows are moved between them. This should use CSS to determine the correct image.

::: browser/extensions/screenshots/bootstrap.js:244
(Diff revision 1)
> +
> +  let id = "screenshots";
> +  let port = null;
> +  let baseIconPath = addonResourceURI.spec + "webextension/";
> +
> +  let {Management: {global: {tabTracker}}} = Cu.import("resource://gre/modules/Extension.jsm", {});

Please don't reach into extension modules like this. Use the `tabManager` property of your extension object instead.

::: browser/extensions/screenshots/bootstrap.js:271
(Diff revision 1)
> +        CustomizableUI.destroyWidget(cuiWidgetID);
> +        CustomizableUI.removeListener(this);

This seems super dodgy. If you're going to leave the browserAction registered and hide it, I'd rather you just hide it with CSS.

Or, even better, just switch to a pageAction and have the extension never show it when it's not needed.

Comment 6

[Ian Bicking (:ianbicking)]

Comment on attachment 8897950 [details]
Bug 1390985 - Export Screenshots 16.1.0 to Firefox

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/169250/diff/1-2/

Comment 7

[Ian Bicking (:ianbicking)]

Comment on attachment 8897951 [details]
Bug 1390985 - Export Screenshots 16.0.0 to Firefox (all code changes)

https://reviewboard.mozilla.org/r/169252/#review176970

> You shouldn't expect the ordering of host permissions to be deterministic, so if you're expecting only one host, please check `length == 1`

Warning added [here](https://github.com/mozilla-services/screenshots/pull/3408/files#diff-4b80223300be1233ce41b6b9d0c08761L74)

> Please set `this._initialized` to false here.

Fixed [here](https://github.com/mozilla-services/screenshots/pull/3408/files#diff-4b80223300be1233ce41b6b9d0c08761R99)

> Please don't reach into extension modules like this. Use the `tabManager` property of your extension object instead.

Fixed [here](https://github.com/mozilla-services/screenshots/pull/3408/files#diff-4b80223300be1233ce41b6b9d0c08761L244)

Comment 8

[Ian Bicking (:ianbicking)]

Comment on attachment 8897950 [details]
Bug 1390985 - Export Screenshots 16.1.0 to Firefox

Comments have followup issues or fixes per discussion.

Comment 9

[Kris Maglione [:kmag]]

Comment on attachment 8897950 [details]
Bug 1390985 - Export Screenshots 16.1.0 to Firefox

https://reviewboard.mozilla.org/r/169250/#review178858

Comment 10

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/7acf5f94b0ad
Export Screenshots 16.1.0 to Firefox r=kmag

Comment 11

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/mozilla-central/rev/7acf5f94b0ad