Closed Bug 1399615 Opened 7 years ago Closed 7 years ago

Update Screenshots to version 19.0.0

Categories

(Firefox :: Screenshots, defect)

Product:
Firefox
Component:
Screenshots
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Tracking Flags:
Tracking Status
firefox57 --- fixed

People

(Reporter: ianbicking, Assigned: ianbicking)

References

Details

Attachments

(1 file)

Bug 1399615 - Export Screenshots 19.0.0 to Firefox
59 bytes, text/x-review-board-request
Details 
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)
Assignee: nobody → ianb
Depends on: 1396438
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.
Attachment #8907779 - Flags: review?(kmaglione+bmo)
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+
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.
Keywords: checkin-needed
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
(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)
Attachment #8907779 - Flags: review+
Attachment #8907779 - Attachment is obsolete: true
Attachment #8907779 - Attachment is obsolete: false
Attachment #8907779 - Flags: review?(kmaglione+bmo)
Attachment #8907779 - Flags: review?(kmaglione+bmo)
Attachment #8907779 - Flags: review?(kmaglione+bmo)
Sorry about those tests, I was a overfocused on our CI tests. Both tests are fixed in the revised patch.
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+
Keywords: checkin-needed
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
https://hg.mozilla.org/mozilla-central/rev/fa1052b9a952
Status: NEW → RESOLVED
Closed: 7 years 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)
Product: Cloud Services → Firefox
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: