Closed Bug 1239122 Opened 6 years ago Closed 6 years ago

Make docshell browser-chrome tests pass in e10s

Categories

(Core :: DOM: Navigation, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla46
Tracking Status
e10s + ---
firefox46 --- fixed

People

(Reporter: mrbkap, Assigned: mrbkap)

References

(Blocks 1 open bug)

Details

Attachments

(1 file, 8 obsolete files)

A couple of them were easy, one required a bunch of code to be moved into a frame script and one changed behavior slightly.
Attached patch Fix browser_bug349769.js (obsolete) — Splinter Review
This was straightforward -- just use promises and existing test utils.
Attachment #8707156 - Flags: review?(wmccloskey)
Attached patch Fix browser_bug388121-1.js (obsolete) — Splinter Review
This is almost the same as the previous patch. I don't know why we need two tests for this.
Attachment #8707161 - Flags: review?(wmccloskey)
Attached patch Fix browser_bug422543.js (obsolete) — Splinter Review
This required a bunch of typing. There isn't anything too difficult here. I couldn't use ContentTask.spawn because I need persistent state across invocations.
Attachment #8707164 - Flags: review?(wmccloskey)
Attached patch Fix browser_bug670318.js (obsolete) — Splinter Review
This was supposed to be more straightforward, but I spent a bunch of time before realizing that using BrowserTestUtils.withNewTab was causing one of the OnHistoryNewEntry calls to become a OnHistoryReplaceEntry. Once I figured that out, this was easy enough.
Attachment #8707166 - Flags: review?(wmccloskey)
Attached patch diff -w (obsolete) — Splinter Review
A few of these patches are easier to read with whitespace differences being ignored. Here's a roll-up of everything with -w.
Attachment #8707169 - Flags: review?(wmccloskey)
Blocks: 1220927
Attached patch Enable browser_bug343515.js (obsolete) — Splinter Review
I decided that testing the child frames is enough to assert that the remote browser property properly reflects the child docshell's active state.

This test could really have done with some promises and refactoring (and I've only made it worse, the fact that ContentTasks can't share code is rather annoying) but "oh well".
Attachment #8707245 - Flags: review?(wmccloskey)
Attached patch diff -w for the last test (obsolete) — Splinter Review
The second half of the test got refactored harder than the first but this makes the first half of the diff much easier to read.
Attachment #8707246 - Flags: review?(wmccloskey)
Attachment #8707156 - Flags: review?(wmccloskey) → review+
Attachment #8707161 - Flags: review?(wmccloskey) → review+
Comment on attachment 8707164 [details] [diff] [review]
Fix browser_bug422543.js

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

::: docshell/test/browser/browser_bug422543.js
@@ +8,4 @@
>  
>    // Check if all history listeners are always notified.
>    info("# part 1");
> +  yield whenPageShown(browser, () => { browser.loadURI("http://www.example.com/"); });

In many of these cases, you can use an expression arrow function: () => browser.loadURI(...) (and no semicolon at end).

@@ +93,5 @@
>  
> +function setListenerRetval(num, val) {
> +  return new Promise(resolve => {
> +    let mm = gBrowser.selectedBrowser.messageManager;
> +    mm.addMessageListener("bug422543:valSet", function vs() {

Perhaps it would be worth it to add a listenOnce function that takes a message manager, a message name, and a listener, and listens for the message once. This could go in BTU or maybe just this file.

@@ +129,5 @@
>  }
>  
> +function whenPageShown(aBrowser, aNavigation) {
> +  return new Promise(resolve => {
> +    aBrowser.addEventListener("pageshow", function onLoad() {

This will use a CPOW, so it would be nice to get rid of it given all the nice work you did in the rest of this test.

::: docshell/test/browser/file_bug422543_script.js
@@ +62,5 @@
> +  },
> +
> +  getListenerStatus() {
> +    sendAsyncMessage("bug422543:listenerStatus",
> +                     this.listeners.map((l) => { return l.last; }));

.map(l => l.last)
Attachment #8707164 - Flags: review?(wmccloskey) → review+
Comment on attachment 8707166 [details] [diff] [review]
Fix browser_bug670318.js

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

::: docshell/test/browser/browser_bug670318.js
@@ +43,5 @@
> +        OnHistoryGotoIndex: () => true,
> +        OnHistoryPurge: () => true,
> +        OnHistoryReplaceEntry: () => {
> +          // For some reason, the load of the initial page comes in as a
> +          // ReplaceEntry now.

Maybe because of the extra about:blank load? If I visit about:blank followed by some URL, going back from the URL doesn't go to about:blank. I don't see a good way to fix this though. But maybe mention it in the comment.
Attachment #8707166 - Flags: review?(wmccloskey) → review+
Comment on attachment 8707246 [details] [diff] [review]
diff -w for the last test

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

::: docshell/test/navigation/browser_bug343515.js
@@ +149,2 @@
>      // Go back
>      oneShotListener(ctx.tab2Browser, "pageshow", step5);

This will use a CPOW.
Attachment #8707246 - Flags: review?(wmccloskey) → review+
Attachment #8707169 - Flags: review?(wmccloskey) → review+
Attachment #8707245 - Flags: review?(wmccloskey) → review+
The try run from comment 11 shows "shistory is undefined" where shistory comes from |docShell.sessionHistory|. I don't understand how that can be undefined (as opposed to null).
Attached patch Rolled-up patch (obsolete) — Splinter Review
Attachment #8709559 - Flags: review+
Attachment #8707156 - Attachment is obsolete: true
Attachment #8707161 - Attachment is obsolete: true
Attachment #8707164 - Attachment is obsolete: true
Attachment #8707166 - Attachment is obsolete: true
Attachment #8707169 - Attachment is obsolete: true
Attachment #8707245 - Attachment is obsolete: true
Attachment #8707246 - Attachment is obsolete: true
docShell doesn't have ClassInfo so might not have been QI'd to nsIWebNavigation.
Attachment #8710143 - Flags: review+
Attachment #8709559 - Attachment is obsolete: true
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/f405663b5570
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla46
Depends on: 1241704
You need to log in before you can comment on or make changes to this bug.