Closed Bug 1381132 Opened 3 years ago Closed 3 years ago

Update Screenshots to version 10.7.0

Categories

(Firefox :: Screenshots, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
Firefox 56
Tracking Status
firefox55 + fixed
firefox56 - fixed

People

(Reporter: ianbicking, Assigned: _6a68)

References

Details

Attachments

(2 files)

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

Changelog: https://github.com/mozilla-services/screenshots/blob/master/CHANGELOG.md#version-800
Summary: Update Screenshots to version 8.1.0 → Update Screenshots to version 10.6.0
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
Blocks: 1380616
Attachment #8886702 - Flags: review?(dtownsend) → review?(kmaglione+bmo)
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+
Assignee: ianb → jhirsch
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
Blocks: 1381963
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
https://hg.mozilla.org/mozilla-central/rev/c0815bdf7b72
Status: NEW → RESOLVED
Closed: 3 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
Product: Cloud Services → Firefox
You need to log in before you can comment on or make changes to this bug.