Ian Bicking (:ianbicking) Reporter
	Description • 7 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

	Ian Bicking (:ianbicking) Reporter
	Comment 1 • 7 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

	Kris Maglione [:kmag]
	Comment 3 • 7 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.

	Kris Maglione [:kmag]
	Comment 5 • 7 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

	Jared Hirsch [:jhirsch] (he/him) (Needinfo please) Assignee
	Comment 6 • 7 years ago

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.

	Jared Hirsch [:jhirsch] (he/him) (Needinfo please) Assignee
	Comment 7 • 7 years ago

OK, I've learned (from jimm via clouserw) that this particular test is not reliable enough that we should wait before landing.

Our results are in the same range as other recent data from m-c and autoland:

https://treeherder.mozilla.org/perf.html#/graphs?timerange=7776000&series=%5Bmozilla-central,601576adb24ca0990e4c2c6f8520cf7f6058dd6a,1,1%5D&series=%5Bmozilla-inbound,601576adb24ca0990e4c2c6f8520cf7f6058dd6a,1,1%5D&series=%5Btry,601576adb24ca0990e4c2c6f8520cf7f6058dd6a,1,1%5D&series=%5Bautoland,601576adb24ca0990e4c2c6f8520cf7f6058dd6a,1,1%5D&highlightedRevisions=5410cebd0e82&highlightedRevisions=9c33620f97ab

Canceling needinfo and requesting checkin.

	Pulsebot
	Comment 8 • 7 years ago

Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/c0815bdf7b72
Export Screenshots 10.7.0 to Firefox; r=kmag

	Ryan VanderMeulen [:RyanVM]
	Comment 9 • 7 years ago
bugherder

https://hg.mozilla.org/mozilla-central/rev/c0815bdf7b72

	Julien Cristau [:jcristau] (back April 22)
	Comment 10 • 7 years ago

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)

	Ryan VanderMeulen [:RyanVM]
	Comment 11 • 7 years ago
bugherder uplift

https://hg.mozilla.org/releases/mozilla-beta/rev/c84963b020db