Closed Bug 1099156 Opened 5 years ago Closed 4 years ago

Fix browser_bug435325.js to work on e10s

Categories

(Firefox :: General, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
Firefox 47
Tracking Status
e10s + ---
firefox46 --- fixed
firefox47 --- fixed

People

(Reporter: Gijs, Assigned: dao)

References

(Blocks 1 open bug)

Details

Attachments

(1 file, 1 obsolete file)

This test tries to access the net error page in content, to click the 'go online' button, and then to try to check if that actually works. It times out when run under e10s.
Flags: qe-verify-
Flags: in-testsuite+
Flags: firefox-backlog+
Assignee: nobody → dao
Attached patch patch (obsolete) — Splinter Review
I removed hasBrowserHandlers from tab-content.js since it's unnecessary and misleading. Unlike the browser.js code setting that attribute, this code deals with about:home.
Attachment #8725636 - Flags: review?(mconley)
Comment on attachment 8725636 [details] [diff] [review]
patch

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

Hey dao, thanks for the patch!

I gave a suggestion on how to refactor the test using BrowserTestUtils to make it less callback-y using add_task, but that's optional. What I'm most interested in fixing is just flat-out removing the hasBrowserHandlers stuff entirely (not just for e10s), since it appears to me to be without use.

Let me know if you have questions.

::: browser/base/content/tab-content.js
@@ -149,5 @@
>    },
>  
>    onPageLoad: function() {
> -    let doc = content.document;
> -    if (doc.documentElement.hasAttribute("hasBrowserHandlers")) {

Is hasBrowserHandlers even useful for the non-e10s case? It looks like it's mainly used to make sure we only add a click event handler on a browser at an about: page once...

But that click event handler (BrowserOnClick) doesn't seem to do much. In fact, it seems like it's only used for about:newtab in the e10s case here, and that was added to make sure that we properly flip to the remote case when clicking a tile.

That was added back in bug 907342, and since then, much work has been done to make the remoteness flipping logic more "robust". I believe we can remove the event handler from BrowserOnClick entirely, to no ill effect (local testing seems to confirm this).

So I think this can be simplified by just flat-out removing hasBrowserHandlers wholesale, from here and from browser.js, along with the event handler on BrowserOnClick.

::: browser/base/content/test/general/browser_bug435325.js
@@ +27,5 @@
> +
> +      /* If this is a remote browser, the hasBrowserHandlers attribute
> +         won't be set and the content script should respond to click events
> +         immediately. */
> +      if (${gBrowser.selectedBrowser.isRemoteBrowser}) {

Pretty neat meta programming you've got going on here. ;)

With the removal of hasBrowserHandlers, I don't believe this will be necessary any longer.

In fact, this can all probably be simplified down to:

add_task(function*() {
  // ... put us in offline, disable caches, etc, and register a cleanup function
  // to revert all that stuff.

  gBrowser.selectedTab = gBrowser.addTab("http://example.com/");

  let browser = gBrowser.selectedBrowser;

  yield ContentTask.spawn(browser, null, function*() {
    yield ContentTaskUtils.waitForEvent(content, "DOMContentLoaded");

    // Do the documentURI check here
  });

  // Re-enable the proxy...


  let onlinePromise = TestUtils.topicObserved("network:offline-status-changed");

  yield BrowserTestUtils.synthesizeMouseAtCenter("#errorTryAgain", {}, browser);
  
  yield onlinePromise;

  yield BrowserTestUtils.removeTab(gBrowser.selectedTab);
});
Attachment #8725636 - Flags: review?(mconley) → review-
Attached patch patch v2Splinter Review
> So I think this can be simplified by just flat-out removing
> hasBrowserHandlers wholesale, from here and from browser.js, along with the
> event handler on BrowserOnClick.

Please file a new bug on this. It feels both detached and risky enough that I don't want it to be part of this patch.
Attachment #8725636 - Attachment is obsolete: true
Attachment #8726127 - Flags: review?(mconley)
Comment on attachment 8726127 [details] [diff] [review]
patch v2

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

Alright, r=me since this gets this working. I've filed bug 1253272 to remove hasBrowserHandlers and bug 1253274 to update the test to use add_task.
Attachment #8726127 - Flags: review?(mconley) → review+
https://hg.mozilla.org/mozilla-central/rev/4e811a988416
https://hg.mozilla.org/mozilla-central/rev/50f6df5b5581
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 47
You need to log in before you can comment on or make changes to this bug.