Update Screenshots to version 16.1.0

RESOLVED FIXED

Status

()

Firefox
Screenshots
RESOLVED FIXED
11 months ago
10 months ago

People

(Reporter: ianbicking, Assigned: ianbicking)

Tracking

(Blocks: 1 bug)

unspecified
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox57 fixed)

Details

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(1 attachment, 1 obsolete attachment)

(Assignee)

Description

11 months ago
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)
(Assignee)

Updated

11 months ago
No longer blocks: 1386457
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)

Updated

11 months ago
Blocks: 1366041
(Assignee)

Updated

11 months ago
Attachment #8897950 - Flags: review?(kmaglione+bmo)
(Assignee)

Updated

11 months ago
Attachment #8897951 - Flags: review?(kmaglione+bmo)
(Assignee)

Comment 3

11 months ago
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

11 months ago
mozreview-review
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+

Updated

11 months ago
Blocks: 1387512
Attachment #8897951 - Flags: review?(dtownsend)
(Assignee)

Updated

11 months ago
Attachment #8897951 - Flags: review?(dtownsend)

Updated

11 months ago
Blocks: 1366026

Comment 5

11 months ago
mozreview-review
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+
Comment hidden (mozreview-request)
(Assignee)

Updated

11 months ago
Attachment #8897951 - Attachment is obsolete: true
(Assignee)

Comment 7

11 months ago
mozreview-review-reply
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)
(Assignee)

Updated

11 months ago
Summary: Update Screenshots to version 16.0.0 → Update Screenshots to version 16.1.0

Updated

11 months ago
Blocks: 1393668
(Assignee)

Comment 8

11 months ago
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 9

11 months ago
mozreview-review
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+
(Assignee)

Updated

11 months ago
Keywords: checkin-needed

Comment 10

11 months ago
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
Last Resolved: 11 months ago
status-firefox57: --- → fixed
Resolution: --- → FIXED

Updated

10 months ago
Product: Cloud Services → Firefox
You need to log in before you can comment on or make changes to this bug.