Alessio Placitelli [:Dexter] Assignee
	Description • 9 years ago

As suggested in bug 1111022, since we might be reusing this code in different places, put the |HiddenFrame| object from CustomizationTabPreloader.jsm in a separate module, HiddenFrame.jsm.

	Tim Taubert [:ttaubert] (inactive)
	Comment 2 • 9 years ago

Comment on attachment 8557908 [details] [diff] [review]
part 1 - Create HiddenFrame.jsm module.

Review of attachment 8557908 [details] [diff] [review]:
-----------------------------------------------------------------

Is that also the bug for converting CustomizationTabPreloader.jsm to use it?

::: browser/modules/HiddenFrame.jsm
@@ +64,5 @@
> +      this._frame.addEventListener("load", this, true);
> +      doc.documentElement.appendChild(this._frame);
> +    } else {
> +      let flags = Ci.nsIThread.DISPATCH_NORMAL;
> +      Services.tm.currentThread.dispatch(() => this._create(), flags);

Can you please use setTimeout() from Timer.jsm here? I think that didn't exist back then. Or I just didn't know of it yet :)

	Alessio Placitelli [:Dexter] Assignee
	Comment 3 • 9 years ago

As discussed over IRC, this patch changes the behaviour of HiddenFrame so that the IFRAME is no longer shared among different users.

This prevents problems when cleaning up and should make it easier to use HiddenFrame in other places in the codebase.

	Tim Taubert [:ttaubert] (inactive)
	Comment 5 • 9 years ago

Comment on attachment 8559144 [details] [diff] [review]
part 1 - Create HiddenFrame.jsm module.

Review of attachment 8559144 [details] [diff] [review]:
-----------------------------------------------------------------

LGTM! Please check that it passes the Customizable UI tests.

	Alessio Placitelli [:Dexter] Assignee
	Comment 6 • 9 years ago

Try push for mochitests (Linux, Macos, Win):

https://treeherder.mozilla.org/#/jobs?repo=try&revision=4e9a3171f944

There were 2 intermittent failures and 1 exceptions. I've retriggered them, all green now!

	Alessio Placitelli [:Dexter] Assignee
	Comment 7 • 9 years ago

More comprehensive testing: https://treeherder.mozilla.org/#/jobs?repo=try&revision=c1c2d7f1a014

	Alessio Placitelli [:Dexter] Assignee
	Comment 8 • 9 years ago

Updated the commit message.

	Alessio Placitelli [:Dexter] Assignee
	Comment 9 • 9 years ago

Updated the commit message.

	Alessio Placitelli [:Dexter] Assignee
	Comment 10 • 9 years ago

Updated the commit message.

	Alessio Placitelli [:Dexter] Assignee
	Comment 11 • 9 years ago

Removes the browser before cleaning up the frame.

	Carsten Book [:Tomcat]
	Comment 12 • 9 years ago

https://hg.mozilla.org/integration/fx-team/rev/0f245324c0af
https://hg.mozilla.org/integration/fx-team/rev/96bcf8f603f2

	Ryan VanderMeulen [:RyanVM]
	Comment 13 • 9 years ago

https://hg.mozilla.org/mozilla-central/rev/0f245324c0af
https://hg.mozilla.org/mozilla-central/rev/96bcf8f603f2

	Alessio Placitelli [:Dexter] Assignee
	Comment 14 • 9 years ago

Tim, do you think this could be uplifted to 37 beta 2? What's the risk involved in doing that? We need it for bug 1111022 uplift.

	Tim Taubert [:ttaubert] (inactive)
	Comment 15 • 9 years ago

Risk should be rather low. We're just moving around code, not really changing how things work.

	Alessio Placitelli [:Dexter] Assignee
	Comment 16 • 9 years ago

Comment on attachment 8560322 [details] [diff] [review]
part 1 - Create HiddenFrame.jsm module.

We would like to uplift this to Firefox 37 Beta 2. This should be uplifted along the following bugs (order matters):

* 1128500 (this bug)
* 1126756 (UITour changes)
* 1128564 (whitelist selfsupport URL)
* 1111022 (Self-Support backend)

[Feature/regressing bug #]:
Needed for heartbeat / selfsupport v0, bug 1111016

[User impact if declined]:
Heartbeat and self-support will not be available for the user.

[Describe test coverage new/current, TreeHerder]: Customizable UI Tests cover this change.
[Risks and why]:  Risk should be rather low. We're just moving around code, not really changing how things work.
[String/UUID change made/needed]: No

	Alessio Placitelli [:Dexter] Assignee
	Comment 17 • 9 years ago

Comment on attachment 8560877 [details] [diff] [review]
part 2 - Make CustomizationTabPreloader.jsm use HiddenFrame.jsm (v2)

We should uplift this after part 1 as well.

	Lawrence Mandel [:lmandel] (use needinfo)
	Comment 18 • 9 years ago

Comment on attachment 8560322 [details] [diff] [review]
part 1 - Create HiddenFrame.jsm module.

Code change is isolated for use with Heatbeat and has been on m-c for 2 weeks. Beta+

	Ryan VanderMeulen [:RyanVM]
	Comment 19 • 9 years ago

https://hg.mozilla.org/releases/mozilla-beta/rev/3d6eaf96ca69
https://hg.mozilla.org/releases/mozilla-beta/rev/cae785163094