Update Screenshots to version 19.0.0

RESOLVED FIXED

Status

()

Firefox
Screenshots
RESOLVED FIXED
10 months ago
10 months ago

People

(Reporter: ianbicking, Assigned: ianbicking)

Tracking

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)

(Assignee)

Description

10 months ago
Version 18.0.0 exported in Bug 1396060 didn't stick, so here's another try with version 19.0.0.

Changes in 19.0.0:

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

* Remove Photon-related conditionals in tests (https://github.com/mozilla-services/screenshots/commit/0b31d58)
* remove browserAction. This changes the add-on to require the Photon page action, with no fallback to a browserAction
  - Removes test for Photon (assumes it is present)
  - Removes bootstrap.js code that deletes the browserAction button
  - Removes webextension code references to browserAction
  - Removes Photon conditionals (i.e., assume it's always Photon)
  - Make 57a1 the minimum version for the webextension/install.rdf. Fixes https://github.com/mozilla-services/screenshots/issues/3468 (https://github.com/mozilla-services/screenshots/commit/d4c56ae)
* Add setting to control binary or base64 upload. This adds `$SCREENSHOTS_UPLOAD_BINARY=true` to turn the feature on. Fixes https://github.com/mozilla-services/screenshots/issues/3481 (https://github.com/mozilla-services/screenshots/commit/92b0d53)
* Convert to semantic locale strings for the slides. The numeric locale ids have made reordering complicated (https://github.com/mozilla-services/screenshots/commit/3d590fb)
* Avoid form uploads from being truncated. `FormData` was not creating correct request bodies for large images. This changes the code to manually construct the form upload. Fixes https://github.com/mozilla-services/screenshots/issues/3472 (https://github.com/mozilla-services/screenshots/commit/671b003)
* Put a guard around the exception stack rewriting (https://github.com/mozilla-services/screenshots/commit/dbc4750)
* Revert "Remove the full page and save visible buttons from onboarding". This reverts commit 1887c38903ce91199f389a345095d6a0546004ac. (https://github.com/mozilla-services/screenshots/commit/33bb5ff)
* Add a new slide to the tour. This slide includes the pageAction interface instead of the old toolbar button. Fixes https://github.com/mozilla-services/screenshots/issues/3442 (https://github.com/mozilla-services/screenshots/commit/9f072c0) (https://github.com/mozilla-services/screenshots/commit/2550449)
* Change slide image to match Photon-style in browser image. Fixes https://github.com/mozilla-services/screenshots/issues/3443 (https://github.com/mozilla-services/screenshots/commit/dbad266)


Changes from 18.0.0:


* Run all add-on svg files through svgo (https://github.com/mozilla-services/screenshots/commit/40b9fe0)
* Fix icon appearance for Photon page action.  See Bug 1395284]. Right now, the icon is too dark, so it doesn't match the appearance of the other Photon page actions. The problem is that the URI passed as the action's iconURL is a `file://` URI.  The Photon theme uses context-fill and context-fill-opacity in SVG in order to style SVG icons correctly, and SVG context painting is not supported for file `bootstrap.js` should pass a `moz-extension://` URI instead, which context painting does support, and which is what the WebExtension browser action toolbar button uses. Additionally, the icon SVG used by the Photon page action needs to be updated with fill-opacity="context-fill-opacity". (https://github.com/mozilla-services/screenshots/commit/b246cb9)
* Add logging of unexpected clipboard state (https://github.com/mozilla-services/screenshots/issues/3430). This logs cases when the passed-in text is empty, or the textarea select doesn't appear to work. Logs are sent to Sentry. Fixes https://github.com/mozilla-services/screenshots/issues/3406 (https://github.com/mozilla-services/screenshots/commit/10b7c0f)
* Fixed next and prev buttons for rtl (https://github.com/mozilla-services/screenshots/commit/5a08464)
* Moved Save/Cancel buttons from right to left for rtl languages (https://github.com/mozilla-services/screenshots/issues/3412) (https://github.com/mozilla-services/screenshots/commit/115d6ed)
* Send image to server in binary (https://github.com/mozilla-services/screenshots/commit/9c558ce)
Comment hidden (mozreview-request)
(Assignee)

Updated

10 months ago
Assignee: nobody → ianb

Updated

10 months ago
Depends on: 1396438
(Assignee)

Comment 3

10 months ago
QA reviewed this, they found #3491 (https://github.com/mozilla-services/screenshots/issues/3491) - Screenshots works in Private Browsing Mode, if it's used from Nightly "Page Actions" menu - normal

Not that bug exists in the current 16.0.0 as well.
(Assignee)

Updated

10 months ago
Attachment #8907779 - Flags: review?(kmaglione+bmo)

Comment 4

10 months ago
mozreview-review
Comment on attachment 8907779 [details]
Bug 1399615 - Export Screenshots 19.0.0 to Firefox

https://reviewboard.mozilla.org/r/179460/#review185216

r=me for everything but the form data encoding. I might be able to be convinced that it's acceptable, but it would take a lot.

::: browser/extensions/screenshots/bootstrap.js:2
(Diff revision 1)
> +// TODO: re-enable
> +/* eslint-disable */

Why do you need to disable ESLint?

::: browser/extensions/screenshots/test/browser/.eslintrc.yml:9
(Diff revision 1)
> +  no-unused-vars: off
> +  no-undef: off

Why do you need to disable these rules?

::: browser/extensions/screenshots/webextension/background/takeshot.js:159
(Diff revision 1)
> +  /** Creates a multipart TypedArray, given {name: value} fields
> +      and {name: blob} files
> +
> +      Returns {body, "content-type"}
> +      */
> +  function createMultipart(fields, fileField, fileFilename, blob) {
> +    let boundary = "---------------------------ScreenshotBoundary" + Date.now();
> +    return blobToArray(blob).then((blobAsBuffer) => {
> +      let body = [];
> +      for (let name in fields) {
> +        body.push("--" + boundary);
> +        body.push(`Content-Disposition: form-data; name="${name}"`);
> +        body.push("");
> +        body.push(fields[name]);
> +      }
> +      body.push("--" + boundary);
> +      body.push(`Content-Disposition: form-data; name="${fileField}"; filename="${fileFilename}"`);
> +      body.push(`Content-Type: ${blob.type}`);
> +      body.push("");
> +      body.push("");
> +      body = body.join("\r\n");
> +      let enc = new TextEncoder("utf-8");
> +      body = enc.encode(body);
> +      body = concatBuffers(body.buffer, blobAsBuffer);
> +      let tail = "\r\n" + "--" + boundary + "--";
> +      tail = enc.encode(tail);
> +      body = concatBuffers(body, tail.buffer);
> +      return {
> +        "content-type": `multipart/form-data; boundary=${boundary}`,
> +        body
> +      };
> +    });
> +  }

You should be using a FormData object for this. Aside from being considerably simpler, it's also considerably more efficient and reliable.
Attachment #8907779 - Flags: review?(kmaglione+bmo) → review+
(Assignee)

Comment 5

10 months ago
mozreview-review
Comment on attachment 8907779 [details]
Bug 1399615 - Export Screenshots 19.0.0 to Firefox

https://reviewboard.mozilla.org/r/179460/#review185454

::: browser/extensions/screenshots/bootstrap.js:2
(Diff revision 1)
> +// TODO: re-enable
> +/* eslint-disable */

We have [a bug](https://github.com/mozilla-services/screenshots/issues/3450) to put this back, but at the moment the Mozilla eslint plugin is broken (see [Bug 1395879](https://bugzilla.mozilla.org/show_bug.cgi?id=1395879)

Since we're not doing much work on this file it seemed easiest to disable it for now and wait for the upstream fix.

::: browser/extensions/screenshots/webextension/background/takeshot.js:159
(Diff revision 1)
> +  /** Creates a multipart TypedArray, given {name: value} fields
> +      and {name: blob} files
> +
> +      Returns {body, "content-type"}
> +      */
> +  function createMultipart(fields, fileField, fileFilename, blob) {
> +    let boundary = "---------------------------ScreenshotBoundary" + Date.now();
> +    return blobToArray(blob).then((blobAsBuffer) => {
> +      let body = [];
> +      for (let name in fields) {
> +        body.push("--" + boundary);
> +        body.push(`Content-Disposition: form-data; name="${name}"`);
> +        body.push("");
> +        body.push(fields[name]);
> +      }
> +      body.push("--" + boundary);
> +      body.push(`Content-Disposition: form-data; name="${fileField}"; filename="${fileFilename}"`);
> +      body.push(`Content-Type: ${blob.type}`);
> +      body.push("");
> +      body.push("");
> +      body = body.join("\r\n");
> +      let enc = new TextEncoder("utf-8");
> +      body = enc.encode(body);
> +      body = concatBuffers(body.buffer, blobAsBuffer);
> +      let tail = "\r\n" + "--" + boundary + "--";
> +      tail = enc.encode(tail);
> +      body = concatBuffers(body, tail.buffer);
> +      return {
> +        "content-type": `multipart/form-data; boundary=${boundary}`,
> +        body
> +      };
> +    });
> +  }

Using FormData specifically broke for large uploads, the server wasn't being sent the entire payload. I should probably open a bug for that on the Firefox side; our bug was [#3472](https://github.com/mozilla-services/screenshots/issues/3472). Request bodies were being truncated at around 1200 bytes.

While the manual body construction fixed it, I didn't feel great about that solution, so the entire form upload portion is pref'd off for now (the setting is in buildSettings.js), and so this version of the add-on will still send the images as data URLs.
(Assignee)

Updated

10 months ago
Keywords: checkin-needed

Comment 6

10 months ago
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/c56a205100ac
Export Screenshots 19.0.0 to Firefox r=kmag
Keywords: checkin-needed
Backed out for failing browser-chrome's browser/components/uitour/test/browser_UITour_availableTargets.js, at least on OS X:

https://hg.mozilla.org/integration/autoland/rev/3cb1ce131f7cab332f036cecd59e5d104c981fa8

Push with failure: https://treeherder.mozilla.org/#/jobs?repo=autoland&revision=c56a205100ac8747670ad5ad5ba1d6c66ce77310&filter-resultStatus=testfailed&filter-resultStatus=busted&filter-resultStatus=exception&filter-resultStatus=retry&filter-resultStatus=usercancel&filter-resultStatus=runnable
Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=131323634&repo=autoland

10:39:36     INFO -  936 INFO TEST-START | browser/components/uitour/test/browser_UITour_availableTargets.js
10:39:42     INFO -  TEST-INFO | started process screencapture
10:39:42     INFO -  TEST-INFO | screencapture: exit 0
10:39:42     INFO -  Buffered messages logged at 10:39:36
10:39:42     INFO -  937 INFO Entering test bound setup_UITourTest
10:39:42     INFO -  938 INFO Leaving test bound setup_UITourTest
10:39:42     INFO -  939 INFO Entering test bound test_availableTargets
10:39:42     INFO -  Buffered messages finished
10:39:42    ERROR -  940 INFO TEST-UNEXPECTED-FAIL | browser/components/uitour/test/browser_UITour_availableTargets.js | Should enable Screenshots - timed out after 50 tries. -
10:39:42     INFO -  Stack trace:
10:39:42     INFO -  chrome://mochitests/content/browser/browser/components/uitour/test/head.js:add_UITour_task/genFun/</</</funcPromise<:450
Flags: needinfo?(ianb)
Please verify that browser/extensions/screenshots/test/browser/browser_screenshots_ui_check.js is also fixed: https://treeherder.mozilla.org/logviewer.html#?job_id=131324151&repo=autoland

Comment 9

10 months ago
(In reply to Ian Bicking (:ianbicking) from comment #5)
> Using FormData specifically broke for large uploads, the server wasn't being
> sent the entire payload. I should probably open a bug for that on the
> Firefox side; our bug was
> [#3472](https://github.com/mozilla-services/screenshots/issues/3472).
> Request bodies were being truncated at around 1200 bytes.

Yes, please file a Firefox (/Necko) bug for this and CC me.
I'll take a look at the UITour test failure, clearing the needinfo on Ian.
Flags: needinfo?(ianb)
(Assignee)

Updated

10 months ago
Attachment #8907779 - Flags: review+
(Assignee)

Updated

10 months ago
Attachment #8907779 - Attachment is obsolete: true
(Assignee)

Updated

10 months ago
Attachment #8907779 - Attachment is obsolete: false
Attachment #8907779 - Flags: review?(kmaglione+bmo)
(Assignee)

Updated

10 months ago
Attachment #8907779 - Flags: review?(kmaglione+bmo)
Comment hidden (mozreview-request)
(Assignee)

Updated

10 months ago
Attachment #8907779 - Flags: review?(kmaglione+bmo)
(Assignee)

Comment 12

10 months ago
Sorry about those tests, I was a overfocused on our CI tests. Both tests are fixed in the revised patch.

Comment 13

10 months ago
mozreview-review
Comment on attachment 8907779 [details]
Bug 1399615 - Export Screenshots 19.0.0 to Firefox

https://reviewboard.mozilla.org/r/179460/#review185630
Attachment #8907779 - Flags: review?(kmaglione+bmo) → review+
(Assignee)

Updated

10 months ago
Keywords: checkin-needed

Comment 14

10 months ago
We're sorry, Autoland could not rebase your commits for you automatically. Please manually rebase your commits and try again.

hg error in cmd: hg rebase -s 0e744eeacd74 -d 008c2a926077: rebasing 420911:0e744eeacd74 "Bug 1399615 - Export Screenshots 19.0.0 to Firefox r=kmag" (tip)
merging browser/extensions/screenshots/bootstrap.js
merging browser/extensions/screenshots/install.rdf
merging browser/extensions/screenshots/moz.build
merging browser/extensions/screenshots/test/browser/browser_screenshots_ui_check.js
merging browser/extensions/screenshots/test/browser/head.js
merging browser/extensions/screenshots/webextension/_locales/ach/messages.json
merging browser/extensions/screenshots/webextension/_locales/ar/messages.json
merging browser/extensions/screenshots/webextension/_locales/ast/messages.json
merging browser/extensions/screenshots/webextension/_locales/az/messages.json
merging browser/extensions/screenshots/webextension/_locales/be/messages.json
merging browser/extensions/screenshots/webextension/_locales/bg/messages.json
merging browser/extensions/screenshots/webextension/_locales/bn_BD/messages.json
merging browser/extensions/screenshots/webextension/_locales/ca/messages.json
merging browser/extensions/screenshots/webextension/_locales/cak/messages.json
merging browser/extensions/screenshots/webextension/_locales/cs/messages.json
merging browser/extensions/screenshots/webextension/_locales/cy/messages.json
merging browser/extensions/screenshots/webextension/_locales/da/messages.json
merging browser/extensions/screenshots/webextension/_locales/de/messages.json
merging browser/extensions/screenshots/webextension/_locales/dsb/messages.json
merging browser/extensions/screenshots/webextension/_locales/el/messages.json
merging browser/extensions/screenshots/webextension/_locales/en_GB/messages.json
merging browser/extensions/screenshots/webextension/_locales/en_US/messages.json
merging browser/extensions/screenshots/webextension/_locales/eo/messages.json
merging browser/extensions/screenshots/webextension/_locales/es_AR/messages.json
merging browser/extensions/screenshots/webextension/_locales/es_CL/messages.json
merging browser/extensions/screenshots/webextension/_locales/es_ES/messages.json
merging browser/extensions/screenshots/webextension/_locales/es_MX/messages.json
merging browser/extensions/screenshots/webextension/_locales/et/messages.json
merging browser/extensions/screenshots/webextension/_locales/fa/messages.json
merging browser/extensions/screenshots/webextension/_locales/fi/messages.json
merging browser/extensions/screenshots/webextension/_locales/fr/messages.json
merging browser/extensions/screenshots/webextension/_locales/fy_NL/messages.json
merging browser/extensions/screenshots/webextension/_locales/ga_IE/messages.json
merging browser/extensions/screenshots/webextension/_locales/gu_IN/messages.json
merging browser/extensions/screenshots/webextension/_locales/he/messages.json
merging browser/extensions/screenshots/webextension/_locales/hi_IN/messages.json
merging browser/extensions/screenshots/webextension/_locales/hr/messages.json
merging browser/extensions/screenshots/webextension/_locales/hsb/messages.json
merging browser/extensions/screenshots/webextension/_locales/hu/messages.json
merging browser/extensions/screenshots/webextension/_locales/hy_AM/messages.json
merging browser/extensions/screenshots/webextension/_locales/id/messages.json
merging browser/extensions/screenshots/webextension/_locales/it/messages.json
merging browser/extensions/screenshots/webextension/_locales/ja/messages.json
merging browser/extensions/screenshots/webextension/_locales/ka/messages.json
merging browser/extensions/screenshots/webextension/_locales/kab/messages.json
merging browser/extensions/screenshots/webextension/_locales/kk/messages.json
merging browser/extensions/screenshots/webextension/_locales/ko/messages.json
merging browser/extensions/screenshots/webextension/_locales/lij/messages.json
merging browser/extensions/screenshots/webextension/_locales/lo/messages.json
merging browser/extensions/screenshots/webextension/_locales/lt/messages.json
merging browser/extensions/screenshots/webextension/_locales/mk/messages.json
merging browser/extensions/screenshots/webextension/_locales/mr/messages.json
merging browser/extensions/screenshots/webextension/_locales/ms/messages.json
merging browser/extensions/screenshots/webextension/_locales/my/messages.json
merging browser/extensions/screenshots/webextension/_locales/nb_NO/messages.json
merging browser/extensions/screenshots/webextension/_locales/nl/messages.json
merging browser/extensions/screenshots/webextension/_locales/nn_NO/messages.json
merging browser/extensions/screenshots/webextension/_locales/pl/messages.json
merging browser/extensions/screenshots/webextension/_locales/pt_BR/messages.json
merging browser/extensions/screenshots/webextension/_locales/pt_PT/messages.json
merging browser/extensions/screenshots/webextension/_locales/rm/messages.json
merging browser/extensions/screenshots/webextension/_locales/ro/messages.json
merging browser/extensions/screenshots/webextension/_locales/ru/messages.json
merging browser/extensions/screenshots/webextension/_locales/sk/messages.json
merging browser/extensions/screenshots/webextension/_locales/sl/messages.json
merging browser/extensions/screenshots/webextension/_locales/sq/messages.json
merging browser/extensions/screenshots/webextension/_locales/sr/messages.json
merging browser/extensions/screenshots/webextension/_locales/sv_SE/messages.json
merging browser/extensions/screenshots/webextension/_locales/ta/messages.json
merging browser/extensions/screenshots/webextension/_locales/te/messages.json
merging browser/extensions/screenshots/webextension/_locales/th/messages.json
merging browser/extensions/screenshots/webextension/_locales/tl/messages.json
merging browser/extensions/screenshots/webextension/_locales/tr/messages.json
merging browser/extensions/screenshots/webextension/_locales/uk/messages.json
merging browser/extensions/screenshots/webextension/_locales/ur/messages.json
merging browser/extensions/screenshots/webextension/_locales/vi/messages.json
merging browser/extensions/screenshots/webextension/_locales/zh_CN/messages.json
merging browser/extensions/screenshots/webextension/_locales/zh_TW/messages.json
merging browser/extensions/screenshots/webextension/background/main.js
merging browser/extensions/screenshots/webextension/background/senderror.js
merging browser/extensions/screenshots/webextension/background/startBackground.js
merging browser/extensions/screenshots/webextension/background/takeshot.js
merging browser/extensions/screenshots/webextension/build/buildSettings.js
merging browser/extensions/screenshots/webextension/build/inlineSelectionCss.js
merging browser/extensions/screenshots/webextension/build/onboardingCss.js
merging browser/extensions/screenshots/webextension/build/onboardingHtml.js
merging browser/extensions/screenshots/webextension/build/shot.js
merging browser/extensions/screenshots/webextension/clipboard.js
merging browser/extensions/screenshots/webextension/icons/back-highlight.svg
merging browser/extensions/screenshots/webextension/icons/back.svg
merging browser/extensions/screenshots/webextension/icons/cancel.svg
merging browser/extensions/screenshots/webextension/icons/cloud.svg
merging browser/extensions/screenshots/webextension/icons/done.svg
merging browser/extensions/screenshots/webextension/icons/download.svg
merging browser/extensions/screenshots/webextension/icons/icon-16-v2.svg
merging browser/extensions/screenshots/webextension/icons/icon-32-v2.svg
merging browser/extensions/screenshots/webextension/icons/icon-highlight-32-v2.svg
merging browser/extensions/screenshots/webextension/icons/icon-starred-32-v2.svg
merging browser/extensions/screenshots/webextension/icons/icon-welcome-face-without-eyes.svg
merging browser/extensions/screenshots/webextension/icons/menu-fullpage.svg
merging browser/extensions/screenshots/webextension/icons/menu-myshot-white.svg
merging browser/extensions/screenshots/webextension/icons/menu-myshot.svg
merging browser/extensions/screenshots/webextension/icons/menu-visible.svg
merging browser/extensions/screenshots/webextension/icons/onboarding-2.png
merging browser/extensions/screenshots/webextension/icons/onboarding-3.png
merging browser/extensions/screenshots/webextension/icons/onboarding-4.png
merging browser/extensions/screenshots/webextension/manifest.json
merging browser/extensions/screenshots/webextension/onboarding/slides.html
merging browser/extensions/screenshots/webextension/selector/shooter.js
warning: /repos/mozreview-gecko/browser/extensions/screenshots/webextension/icons/onboarding-2.png looks like a binary file.
warning: conflicts while merging browser/extensions/screenshots/webextension/icons/onboarding-2.png! (edit, then use 'hg resolve --mark')
warning: /repos/mozreview-gecko/browser/extensions/screenshots/webextension/icons/onboarding-3.png looks like a binary file.
warning: conflicts while merging browser/extensions/screenshots/webextension/icons/onboarding-3.png! (edit, then use 'hg resolve --mark')
warning: /repos/mozreview-gecko/browser/extensions/screenshots/webextension/icons/onboarding-4.png looks like a binary file.
warning: conflicts while merging browser/extensions/screenshots/webextension/icons/onboarding-4.png! (edit, then use 'hg resolve --mark')
unresolved conflicts (see hg resolve, then hg rebase --continue)
Flags: needinfo?(ianb)
Keywords: checkin-needed
Comment hidden (mozreview-request)

Comment 17

10 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/fa1052b9a952
Status: NEW → RESOLVED
Last Resolved: 10 months ago
status-firefox57: --- → fixed
Resolution: --- → FIXED
Clearing the needinfo on Ian since it doesn't look like it's necessary anymore.  Apologies if I'm wrong.
Flags: needinfo?(ianb)

Updated

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