Closed
Bug 1256746
Opened 8 years ago
Closed 8 years ago
Fix browser_UITour_detach_tab.js to work in e10s mode
Categories
(Firefox :: Tours, defect)
Firefox
Tours
Tracking
()
RESOLVED
FIXED
Firefox 48
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.
Assignee | ||
Updated•8 years ago
|
status-firefox47:
--- → affected
Summary: Fix browser_detachTab.js to work in e10s mode → Fix browser_UITour_detach_tab.js to work in e10s mode
Assignee | ||
Comment 1•8 years ago
|
||
Review commit: https://reviewboard.mozilla.org/r/40177/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/40177/
Attachment #8730837 -
Flags: review?(MattN+bmo)
Updated•8 years ago
|
Blocks: e10s-tests
tracking-e10s:
--- → +
Comment 2•8 years ago
|
||
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)
Assignee | ||
Comment 3•8 years ago
|
||
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?
Assignee | ||
Comment 4•8 years ago
|
||
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?
Assignee | ||
Comment 5•8 years ago
|
||
(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)
Comment 6•8 years ago
|
||
(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)
Assignee | ||
Comment 7•8 years ago
|
||
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 8•8 years ago
|
||
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+
Comment 10•8 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/e8bbee9743bd
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 48
You need to log in
before you can comment on or make changes to this bug.
Description
•