Closed Bug 1128500 Opened 5 years ago Closed 5 years ago

Put |HiddenFrame| from CustomizationTabPreloader.jsm in HiddenFrame.jsm

Categories

(Firefox :: General, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
Firefox 38
Tracking Status
firefox37 --- fixed
firefox38 --- fixed

People

(Reporter: Dexter, Assigned: Dexter)

References

Details

Attachments

(2 files, 5 obsolete files)

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.
Blocks: 1111022
Assignee: nobody → alessio.placitelli
Attachment #8557908 - Flags: feedback?(ttaubert)
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 :)
Attachment #8557908 - Flags: feedback?(ttaubert) → feedback+
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.
Attachment #8557908 - Attachment is obsolete: true
Attachment #8559144 - Flags: review?(ttaubert)
Attachment #8559145 - Flags: review?(ttaubert) → review+
Status: NEW → ASSIGNED
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.
Attachment #8559144 - Flags: review?(ttaubert) → review+
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!
Updated the commit message.
Attachment #8559144 - Attachment is obsolete: true
Updated the commit message.
Attachment #8560319 - Attachment is obsolete: true
Updated the commit message.
Attachment #8559145 - Attachment is obsolete: true
Removes the browser before cleaning up the frame.
Attachment #8560323 - Attachment is obsolete: true
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/0f245324c0af
https://hg.mozilla.org/mozilla-central/rev/96bcf8f603f2
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Whiteboard: [fixed-in-fx-team]
Target Milestone: --- → Firefox 38
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.
Flags: needinfo?(ttaubert)
Risk should be rather low. We're just moving around code, not really changing how things work.
Flags: needinfo?(ttaubert)
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
Attachment #8560322 - Flags: approval-mozilla-beta?
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.
Attachment #8560877 - Flags: approval-mozilla-beta?
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+
Attachment #8560322 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Attachment #8560877 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
You need to log in before you can comment on or make changes to this bug.