Closed Bug 1390985 Opened 2 years ago Closed 2 years ago

Update Screenshots to version 16.1.0

Categories

(Firefox :: Screenshots, enhancement)

enhancement
Not set

Tracking

()

RESOLVED FIXED
Tracking Status
firefox57 --- fixed

People

(Reporter: ianbicking, Assigned: ianbicking)

References

Details

Attachments

(1 file, 1 obsolete file)

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)
No longer blocks: 1386457
Blocks: 1366041
Attachment #8897950 - Flags: review?(kmaglione+bmo)
Attachment #8897951 - Flags: review?(kmaglione+bmo)
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 on attachment 8897950 [details]
Bug 1390985 - Export Screenshots 16.1.0 to Firefox

https://reviewboard.mozilla.org/r/169250/#review175520
Attachment #8897950 - Flags: review?(kmaglione+bmo) → review+
Blocks: 1387512
Attachment #8897951 - Flags: review?(dtownsend)
Blocks: 1366026
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.
Attachment #8897951 - Flags: review?(kmaglione+bmo) → review+
Attachment #8897951 - Attachment is obsolete: true
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)
Summary: Update Screenshots to version 16.0.0 → Update Screenshots to version 16.1.0
Blocks: 1393668
Comment on attachment 8897950 [details]
Bug 1390985 - Export Screenshots 16.1.0 to Firefox

Comments have followup issues or fixes per discussion.
Attachment #8897950 - Flags: review?(kmaglione+bmo)
Comment on attachment 8897950 [details]
Bug 1390985 - Export Screenshots 16.1.0 to Firefox

https://reviewboard.mozilla.org/r/169250/#review178858
Attachment #8897950 - Flags: review?(kmaglione+bmo) → review+
Keywords: checkin-needed
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/7acf5f94b0ad
Export Screenshots 16.1.0 to Firefox r=kmag
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/7acf5f94b0ad
Status: NEW → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Product: Cloud Services → Firefox
You need to log in before you can comment on or make changes to this bug.