Blake Kaplan (:mrbkap) (inactive) Assignee
	Description • 9 years ago

A couple of them were easy, one required a bunch of code to be moved into a frame script and one changed behavior slightly.

	Blake Kaplan (:mrbkap) (inactive) Assignee
	Comment 1 • 9 years ago

This was straightforward -- just use promises and existing test utils.

	Blake Kaplan (:mrbkap) (inactive) Assignee
	Comment 2 • 9 years ago

This is almost the same as the previous patch. I don't know why we need two tests for this.

	Blake Kaplan (:mrbkap) (inactive) Assignee
	Comment 3 • 9 years ago

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.

	Blake Kaplan (:mrbkap) (inactive) Assignee
	Comment 4 • 9 years ago

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.

	Blake Kaplan (:mrbkap) (inactive) Assignee
	Comment 5 • 9 years ago

A few of these patches are easier to read with whitespace differences being ignored. Here's a roll-up of everything with -w.

	Blake Kaplan (:mrbkap) (inactive) Assignee
	Comment 6 • 9 years ago

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".

	Blake Kaplan (:mrbkap) (inactive) Assignee
	Comment 7 • 9 years ago

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.

	Bill McCloskey [inactive unless it's an emergency] (:billm)
	Comment 8 • 8 years ago

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)

	Bill McCloskey [inactive unless it's an emergency] (:billm)
	Comment 9 • 8 years ago

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.

	Bill McCloskey [inactive unless it's an emergency] (:billm)
	Comment 10 • 8 years ago

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.

	Blake Kaplan (:mrbkap) (inactive) Assignee
	Comment 11 • 8 years ago

https://treeherder.mozilla.org/#/jobs?repo=try&revision=61c443a875dd

	Blake Kaplan (:mrbkap) (inactive) Assignee
	Comment 12 • 8 years ago

https://treeherder.mozilla.org/#/jobs?repo=try&revision=603c757781c4

	Blake Kaplan (:mrbkap) (inactive) Assignee
	Comment 13 • 8 years ago

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).

	Blake Kaplan (:mrbkap) (inactive) Assignee
	Comment 15 • 8 years ago

https://treeherder.mozilla.org/#/jobs?repo=try&revision=1b27d4c1be04

	Blake Kaplan (:mrbkap) (inactive) Assignee
	Comment 16 • 8 years ago

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

	Blake Kaplan (:mrbkap) (inactive) Assignee
	Comment 17 • 8 years ago

docShell doesn't have ClassInfo so might not have been QI'd to nsIWebNavigation.

	Pulsebot
	Comment 18 • 8 years ago

https://hg.mozilla.org/integration/mozilla-inbound/rev/f405663b5570

	Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)
	Comment 19 • 8 years ago
bugherder

https://hg.mozilla.org/mozilla-central/rev/f405663b5570