Update Screenshots to version 10.7.0

RESOLVED FIXED in Firefox 55

Status

()

defect
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: ianbicking, Assigned: _6a68)

Tracking

unspecified
Firefox 56
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox55+ fixed, firefox56- fixed)

Details

Attachments

(2 attachments)

Reporter

Description

2 years ago
+++ This bug was initially created as a clone of Bug #1368146 +++

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

Updated

2 years ago
Summary: Update Screenshots to version 8.1.0 → Update Screenshots to version 10.6.0
Reporter

Comment 1

2 years ago
Actual changelog: https://github.com/mozilla-services/screenshots/blob/95258707d23b3147fb45159bd2165ef2c8279657/CHANGELOG.md#version-1060

- Iframe tests: validate iframe URLs, remove unneeded iframe onload handlers
- Put temporary clipboard TEXTAREA in an iframe, with iframe URL validation
Assignee: nobody → ianb
Comment hidden (mozreview-request)
Reporter

Updated

2 years ago
Blocks: 1380616
Attachment #8886702 - Flags: review?(dtownsend) → review?(kmaglione+bmo)

Comment 3

2 years ago
mozreview-review
Comment on attachment 8886702 [details]
Bug 1381132 - Export Screenshots 10.6.0 to Firefox;

https://reviewboard.mozilla.org/r/157486/#review162642

::: browser/extensions/screenshots/webextension/assertIsBlankDocumentUrl.js:5
(Diff revision 1)
> +/** For use inside an iframe onload function, throws an Error if iframe src is not blank.html
> +
> +    Should be applied *inside* catcher.watchFunction
> +*/
> +this.assertIsBlankDocumentUrl = function assertIsBlankDocumentUrl(documentUrl) {

I'd rather we pass the document object here rather than it's URL. And we'd probably be better off checking `document.documentURI` rather than `document.URL`.

::: browser/extensions/screenshots/webextension/clipboard.js:30
(Diff revision 1)
> -    const copied = document.execCommand("copy");
> +          const copied = doc.execCommand("copy");
> -    document.body.removeChild(el);
> -    if (!copied) {
> +          if (!copied) {
> -      catcher.unhandled(new Error("Clipboard copy failed"));
> +            catcher.unhandled(new Error("Clipboard copy failed"));
> -    }
> +          }
> -    return copied;
> +          doc.body.removeChild(el);

Nit: `el.remove()`

::: browser/extensions/screenshots/webextension/clipboard.js:33
(Diff revision 1)
> -      catcher.unhandled(new Error("Clipboard copy failed"));
> +            catcher.unhandled(new Error("Clipboard copy failed"));
> -    }
> +          }
> -    return copied;
> +          doc.body.removeChild(el);
> +          resolve(copied);
> +        } finally {
> +          document.body.removeChild(element);

Nit: `element.remove()`

::: browser/extensions/screenshots/webextension/onboarding/slides.js:38
(Diff revision 1)
>        iframe.onload = catcher.watchFunction(() => {
> +        iframe.onload = null;

At this point, we should probably also probably switch from setting the `onload` property to `addEventListener("load", func, {once: true})`. The `on*` getters and setters are actually shared between the content page and the content script sandbox (and any other content script sandbox).

The same goes for the other iframes we inject this way.
Attachment #8886702 - Flags: review?(kmaglione+bmo) → review+
Reporter

Updated

2 years ago
Assignee: ianb → jhirsch
Comment hidden (mozreview-request)

Comment 5

2 years ago
mozreview-review
Comment on attachment 8887267 [details]
Bug 1381132 - Export Screenshots 10.7.0 to Firefox;

https://reviewboard.mozilla.org/r/158060/#review163246
Attachment #8887267 - Flags: review?(kmaglione+bmo) → review+
Seeing some unusual Talos results: the tresize test regressed by 11.86% on win64.

https://treeherder.mozilla.org/perf.html#/compare?originalProject=try&originalRevision=5410cebd0e82&newProject=try&newRevision=9c33620f97ab077ef202ff1af5ffb464dd1fc267&framework=1

Given how small the diff is, it's hard for me to understand how that one test could have regressed significantly.

Going to investigate a bit before requesting checkin/uplift for this patch.
Flags: needinfo?(jmaher)
Summary: Update Screenshots to version 10.6.0 → Update Screenshots to version 10.7.0

Updated

2 years ago
Blocks: 1381963

Comment 8

2 years ago
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/c0815bdf7b72
Export Screenshots 10.7.0 to Firefox; r=kmag
Keywords: checkin-needed

Comment 9

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/c0815bdf7b72
Status: NEW → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Comment on attachment 8887267 [details]
Bug 1381132 - Export Screenshots 10.7.0 to Firefox;

[Triage Comment]
screenshots update, should go in 55.0b12 along with bug 1382754 (10.8.0)
Attachment #8887267 - Flags: approval-mozilla-beta+
Target Milestone: --- → mozilla56
Target Milestone: mozilla56 → Firefox 56

Updated

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