782892 - about:neterror no longer automatically disables Offline Mode, frontend should handle it

Status: RESOLVED FIXED

Description

[Blair McBride [:Unfocused] (UNAVAILABLE)]

Usage of nsIDOMWindowUtils.goOnline() was removed from about:neterror in bug 779680, as that method is going away (bug 775868). Previously this was used to automatically disable Offline Mode when clicking "retry", after a page couldn't be loaded due to being in Offline Mode.

Bug 779680 handled this in Firefox by having the browser chrome listen for a click event on the errorTryAgain button in about:neterror (via a capturing event listener), checking "e=netOffline" is in the URL, and doing:
  Services.io.offline = false;

Comment 1

[Philip Chee]

http://tinderbox.mozilla.org/showlog.cgi?log=SeaMonkey/1345241103.1345244340.1392.gz#err7

TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/docshell/test/browser/browser_bug435325.js | After clicking the Try Again button, we're back  online. This depends on Components.interfaces.nsIDOMWindowUtils being available from untrusted content (bug 435325).

Comment 2

[Blair McBride [:Unfocused] (UNAVAILABLE)]

Bug 783763 will correct that log message, and move that test into /browser/

Comment 3

[Philip Chee]

Created attachment 656070 [details] [diff] [review]
Patch v1.0 Use TabsProgressListener

This patch works when I test off-line manually but the test copied from Firefox fails for a reason I cannot discern.

Comment 4

[[e-mail]]

Created attachment 656243 [details] [diff] [review]
Possible patch

This version seems to pass your test, as far as I can tell...

Comment 5

[Philip Chee]

Comment on attachment 656243 [details] [diff] [review]
Possible patch

Passes all the tests.

*** Start BrowserChrome Test Results ***
TEST-INFO | checking window state
TEST-START | chrome://mochitests/content/browser/suite/browser/test/browser_bug435325.js
TEST-INFO | chrome://mochitests/content/browser/suite/browser/test/browser_bug435325.js | got about:blank, which is expected once, so return
Error loading URL http://example.com/ : 804b0046 (NS_ERROR_DOCUMENT_NOT_CACHED)
TEST-PASS | chrome://mochitests/content/browser/suite/browser/test/browser_bug435325.js | Setting Services.io.offline to true.
TEST-PASS | chrome://mochitests/content/browser/suite/browser/test/browser_bug435325.js | Loading the Offline mode neterror page.
TEST-PASS | chrome://mochitests/content/browser/suite/browser/test/browser_bug435325.js | The errorpage has got a #errorTryAgain element
TEST-PASS | chrome://mochitests/content/browser/suite/browser/test/browser_bug435325.js | After clicking the Try Again button, we're back online.
INFO TEST-END | chrome://mochitests/content/browser/suite/browser/test/browser_bug435325.js | finished in 187ms
TEST-INFO | checking window state

INFO TEST-START | Shutdown
Browser Chrome Test Summary
        Passed: 4
        Failed: 0
        Todo: 0

*** End BrowserChrome Test Results ***

> +        <![CDATA[
> +          if (/^about:neterror\?e=netOffline/.test(event.target.documentURI))
> +            event.target.addEventListener("click", function(event) {
Please name this anonymous function.

[As discussed over IRC, while it may be best practice to remove the eventListener afterwards, it may not be practicable to do so.]

> +              if (event.target.id == "errorTryAgain")
> +                Services.io.offline = false;
This adds a new dependency on Services.
Your bonus for 10: ioservice is used in several other places. Consider turning it into a field.

> +            }, true);
> +        ]]>

f=me

Comment 6

[[e-mail]]

(In reply to Philip Chee from comment #5)
> Your bonus for 10: ioservice is used in several other places. Consider
> turning it into a field.
It's used inside a callback, so I'd have to mess around with bind or some such...

Comment 7

[[e-mail]]

Created attachment 656612 [details] [diff] [review]
Proposed patch

Comment 8

[Philip Chee]

> It's used inside a callback, so I'd have to mess around with bind or some such...
Yeah, too much effort for not enough gain.

Comment 9

[Ian Neal]

Comment on attachment 656612 [details] [diff] [review]
Proposed patch

Don't we still need the test from the first patch?

Comment 10

[Philip Chee]

> Don't we still need the test from the first patch?
Yes. I can check both in if you r+ the test.

Comment 11

[Ian Neal]

Comment on attachment 656070 [details] [diff] [review]
Patch v1.0 Use TabsProgressListener

r=me for the test

Comment 12

[Philip Chee]

Pushed:
http://hg.mozilla.org/comm-central/rev/0e501d59baac
http://hg.mozilla.org/comm-central/rev/b7d36ee71002 (test)

Comment 13

[Philip Chee]

Comment on attachment 656612 [details] [diff] [review]
Proposed patch

[Approval Request Comment]
Regression caused by (bug #): 779680
User impact if declined: Unable to go online when the "Try Again" button is pressed.
Testing completed (on m-c, etc.): comm-central.
Risk to taking this patch (and alternatives if risky): minimal risk, just missed the train. Bug 779680 landed on mozilla17 so we need to land this on comm-aurora.
String changes made by this patch: none.

Comment 14

[Ian Neal]

Comment on attachment 656612 [details] [diff] [review]
Proposed patch

a=me for this (and test if required)

Comment 15

[Philip Chee]

Pushed to comm-aurora:
http://hg.mozilla.org/releases/comm-aurora/rev/ada7d03ccea4
http://hg.mozilla.org/releases/comm-aurora/rev/ee74d240d1ce (test)