Closed
Bug 1399615
Opened 7 years ago
Closed 7 years ago
Update Screenshots to version 19.0.0
Categories
(Firefox :: Screenshots, defect)
Firefox
Screenshots
Tracking
()
RESOLVED
FIXED
Tracking | Status | |
---|---|---|
firefox57 | --- | fixed |
People
(Reporter: ianbicking, Assigned: ianbicking)
References
Details
Attachments
(1 file)
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 | ||
Comment 1•7 years ago
|
||
Comment hidden (mozreview-request) |
Assignee | ||
Updated•7 years ago
|
Assignee: nobody → ianb
Assignee | ||
Comment 3•7 years 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•7 years ago
|
Attachment #8907779 -
Flags: review?(kmaglione+bmo)
Comment 4•7 years 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•7 years 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•7 years ago
|
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
Comment 7•7 years ago
|
||
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)
Comment 8•7 years ago
|
||
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•7 years 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.
Comment 10•7 years ago
|
||
I'll take a look at the UITour test failure, clearing the needinfo on Ian.
Flags: needinfo?(ianb)
Assignee | ||
Updated•7 years ago
|
Attachment #8907779 -
Flags: review+
Assignee | ||
Updated•7 years ago
|
Attachment #8907779 -
Attachment is obsolete: true
Assignee | ||
Updated•7 years ago
|
Attachment #8907779 -
Attachment is obsolete: false
Attachment #8907779 -
Flags: review?(kmaglione+bmo)
Assignee | ||
Updated•7 years ago
|
Attachment #8907779 -
Flags: review?(kmaglione+bmo)
Comment hidden (mozreview-request) |
Assignee | ||
Updated•7 years ago
|
Attachment #8907779 -
Flags: review?(kmaglione+bmo)
Assignee | ||
Comment 12•7 years ago
|
||
Sorry about those tests, I was a overfocused on our CI tests. Both tests are fixed in the revised patch.
Comment 13•7 years 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•7 years ago
|
Keywords: checkin-needed
Comment 14•7 years 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)
Updated•7 years ago
|
Flags: needinfo?(ianb)
Keywords: checkin-needed
Comment hidden (mozreview-request) |
Comment 16•7 years ago
|
||
bugherder landing |
Comment 17•7 years ago
|
||
bugherder |
Comment 18•7 years ago
|
||
Clearing the needinfo on Ian since it doesn't look like it's necessary anymore. Apologies if I'm wrong.
Flags: needinfo?(ianb)
Updated•7 years ago
|
Product: Cloud Services → Firefox
You need to log in
before you can comment on or make changes to this bug.
Description
•