Closed Bug 1256746 Opened 4 years ago Closed 4 years ago

Fix browser_UITour_detach_tab.js to work in e10s mode

Categories

(Firefox :: Tours, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
Firefox 48
Tracking Status
e10s + ---
firefox47 --- affected
firefox48 --- fixed

People

(Reporter: Gijs, Assigned: Gijs)

References

(Blocks 1 open bug)

Details

Attachments

(1 file)

(NB: not 100% sure to what degree this depends on bug 1256625, but that feels like it's not the most important thing to sort out right now, either)

This test tries to use the content document and window directly and that makes e10s unhappy. Patch incoming.
Summary: Fix browser_detachTab.js to work in e10s mode → Fix browser_UITour_detach_tab.js to work in e10s mode
Blocks: e10s-tests
tracking-e10s: --- → +
Comment on attachment 8730837 [details]
MozReview Request: Bug 1256746 - fix browser_UITour_detach_tab.js to work in e10s, r?MattN

https://reviewboard.mozilla.org/r/40177/#review36687

::: browser/components/uitour/test/browser_UITour_detach_tab.js:64
(Diff revision 1)
> +    gContentWindow.messageManager.addMessageListener("UITourTest:IsDifferentWindow", msg => {
> +      msg.target.messageManager.sendAsyncMessage("UITourTest:DifferentWindow");
> +    });

Why isn't the visibilitychange listener calling showHighlight directly anymore? That needs to work (I think the web currently uses a setTimeout but ideally shouldn't have to) and I believe is part of what this test is testing.

::: browser/components/uitour/test/browser_UITour_detach_tab.js:70
(Diff revision 1)
>      // This highlight should be shown thanks to the visibilitychange listener.
>      let newWindowHighlight = gContentWindow.document.getElementById("UITourHighlight");

This comment is now slightly less accurate as the visibilitychange listener doesn't show the highlight directly (though see above as I think it should go back to that)

::: browser/components/uitour/test/browser_UITour_detach_tab.js:76
(Diff revision 1)
>      let newWindowHighlight = gContentWindow.document.getElementById("UITourHighlight");
>      yield elementVisiblePromise(newWindowHighlight);
>  
>      let selectedTab = gContentWindow.gBrowser.selectedTab;
> -    is(selectedTab.linkedBrowser && selectedTab.linkedBrowser.contentDocument, gContentDoc, "Document should be selected in new window");
> +    yield ContentTask.spawn(selectedTab.linkedBrowser, myDocIdentifier, myDocIdentifier => {
> +      is(content.document.myExpando, myDocIdentifier, "DOcument should be selected in new window");

Nit: "DOcument"
Attachment #8730837 - Flags: review?(MattN+bmo)
https://reviewboard.mozilla.org/r/40177/#review38221

::: browser/components/uitour/test/browser_UITour_detach_tab.js:64
(Diff revision 1)
> +    gContentWindow.messageManager.addMessageListener("UITourTest:IsDifferentWindow", msg => {
> +      msg.target.messageManager.sendAsyncMessage("UITourTest:DifferentWindow");
> +    });

There is no way to synchronously determine whether we've changed parent yet from the content process, AIUI. So how would you propose to test this?
https://reviewboard.mozilla.org/r/40177/#review36687

> Why isn't the visibilitychange listener calling showHighlight directly anymore? That needs to work (I think the web currently uses a setTimeout but ideally shouldn't have to) and I believe is part of what this test is testing.

There is no way to synchronously determine whether we've changed parent yet from the content process, AIUI. So how would you propose to test this?
(In reply to :Gijs Kruitbosch from comment #4)
> https://reviewboard.mozilla.org/r/40177/#review36687
> 
> > Why isn't the visibilitychange listener calling showHighlight directly anymore? That needs to work (I think the web currently uses a setTimeout but ideally shouldn't have to) and I believe is part of what this test is testing.
> 
> There is no way to synchronously determine whether we've changed parent yet
> from the content process, AIUI. So how would you propose to test this?
Flags: needinfo?(MattN+bmo)
(In reply to :Gijs Kruitbosch from comment #4)
> https://reviewboard.mozilla.org/r/40177/#review36687
> 
> > Why isn't the visibilitychange listener calling showHighlight directly anymore? That needs to work (I think the web currently uses a setTimeout but ideally shouldn't have to) and I believe is part of what this test is testing.
> 
> There is no way to synchronously determine whether we've changed parent yet
> from the content process, AIUI. So how would you propose to test this?

The visibilitychange is from the window change so isn't that sufficient? Or is there other visibility change with hidden = false that happens before the window change?
Flags: needinfo?(MattN+bmo)
Comment on attachment 8730837 [details]
MozReview Request: Bug 1256746 - fix browser_UITour_detach_tab.js to work in e10s, r?MattN

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/40177/diff/1-2/
Attachment #8730837 - Flags: review?(MattN+bmo)
Comment on attachment 8730837 [details]
MozReview Request: Bug 1256746 - fix browser_UITour_detach_tab.js to work in e10s, r?MattN

https://reviewboard.mozilla.org/r/40177/#review38253

Thanks. This is less to comprehend too.
Attachment #8730837 - Flags: review?(MattN+bmo) → review+
https://hg.mozilla.org/mozilla-central/rev/e8bbee9743bd
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 48
You need to log in before you can comment on or make changes to this bug.