Closed Bug 1099154 Opened 5 years ago Closed 5 years ago

Fix browser_backButtonFitts.js to work in e10s mode

Categories

(Firefox :: Toolbars and Customization, defect)

All
Windows 8.1
defect
Not set
Points:
3

Tracking

()

RESOLVED FIXED
Firefox 41
Iteration:
41.1 - May 25
Tracking Status
e10s + ---
firefox41 --- fixed

People

(Reporter: Gijs, Assigned: enndeakin)

References

(Blocks 1 open bug)

Details

Attachments

(1 file)

This test adds an event listener to the content window and then gets stuck after a new tab has been loaded.
Points: --- → 3
Flags: qe-verify-
Flags: in-testsuite+
Flags: firefox-backlog+
Success on try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=ad8793ebfea1
Assignee: nobody → enndeakin
Status: NEW → ASSIGNED
Attachment #8595680 - Flags: review?(gijskruitbosch+bugs)
Comment on attachment 8595680 [details] [diff] [review]
browser_backButtonFitts

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

::: browser/base/content/test/general/browser_backButtonFitts.js
@@ +11,5 @@
>  
> +    content.addEventListener("popstate", function onPopState() {
> +      content.removeEventListener("popstate", onPopState, false);
> +      sendAsyncMessage("Test:PopStateOccurred", { location: content.document.location.href });
> +    }, false);

Pretty sure you could return a promise here that you'd resolve from onPopState, and then the ContentTask.spawn call will yield you the thing you resolve the promise with. Then you don't need to do your own message passing here and below.
Attachment #8595680 - Flags: review?(gijskruitbosch+bugs) → feedback+
Iteration: --- → 40.2 - 27 Apr
> Pretty sure you could return a promise here that you'd resolve from
> onPopState, and then the ContentTask.spawn call will yield you the thing you
> resolve the promise with. Then you don't need to do your own message passing
> here and below.

I don't think I follow here. The popstate event doesn't happen until the mouse event occurs later on, long after the ContentTask.spawn has already finished.
(In reply to Neil Deakin from comment #3)
> > Pretty sure you could return a promise here that you'd resolve from
> > onPopState, and then the ContentTask.spawn call will yield you the thing you
> > resolve the promise with. Then you don't need to do your own message passing
> > here and below.
> 
> I don't think I follow here. The popstate event doesn't happen until the
> mouse event occurs later on, long after the ContentTask.spawn has already
> finished.

let promise = ContentTask.spawn(gBrowser.selectedBrowser, {}, function* () {
  content.history.pushState("page2", "page2", "page2");
  return new Promise((resolve, reject) => {
    content.addEventListener("popstate", function onPopState() {
      content.removeEventListener("popstate", onPopState, false);
      resolve(content.document.location.href);
    }, false);
  });
});

// trigger click etc.

let uri = yield promise;

is(uri, ...);


See e.g. http://mxr.mozilla.org/mozilla-central/source/dom/html/test/browser_DOMDocElementInserted.js#6 for another test that uses this structure.
> let promise = ContentTask.spawn(gBrowser.selectedBrowser, {}, function* () {
>   content.history.pushState("page2", "page2", "page2");
>   return new Promise((resolve, reject) => {
>     content.addEventListener("popstate", function onPopState() {
>       content.removeEventListener("popstate", onPopState, false);
>       resolve(content.document.location.href);
>     }, false);
>   });
> });
> 
>
> // trigger click etc.

Yes, but I want the call to pushState to happen before the mouse click.

I used a message here only to avoid having two separate ContentTasks.
(In reply to Neil Deakin from comment #5)
> > let promise = ContentTask.spawn(gBrowser.selectedBrowser, {}, function* () {
> >   content.history.pushState("page2", "page2", "page2");
> >   return new Promise((resolve, reject) => {
> >     content.addEventListener("popstate", function onPopState() {
> >       content.removeEventListener("popstate", onPopState, false);
> >       resolve(content.document.location.href);
> >     }, false);
> >   });
> > });
> > 
> >
> > // trigger click etc.
> 
> Yes, but I want the call to pushState to happen before the mouse click.
> 
> I used a message here only to avoid having two separate ContentTasks.

Huh, I hadn't thought of that. Fair point. I would actually prefer the two-contentTask solution, I guess? With a comment, I suppose, about needing it to have happened before the click.
Iteration: 40.2 - 27 Apr → 40.3 - 11 May
https://hg.mozilla.org/mozilla-central/rev/41ca098906eb
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 41
Iteration: 40.3 - 11 May → 41.1 - May 25
You need to log in before you can comment on or make changes to this bug.