Last Comment Bug 782892 - about:neterror no longer automatically disables Offline Mode, frontend should handle it
: about:neterror no longer automatically disables Offline Mode, frontend should...
Status: RESOLVED FIXED
:
Product: SeaMonkey
Classification: Client Software
Component: General (show other bugs)
: Trunk
: All All
: -- normal (vote)
: seamonkey2.15
Assigned To: neil@parkwaycc.co.uk
:
Mentors:
Depends on: 779680 783763
Blocks:
  Show dependency treegraph
 
Reported: 2012-08-15 01:36 PDT by Blair McBride [:Unfocused] (mostly unavailable, needinfo open, reviews not)
Modified: 2012-09-06 08:50 PDT (History)
3 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
+
fixed
fixed


Attachments
Patch v1.0 Use TabsProgressListener (6.11 KB, patch)
2012-08-28 10:14 PDT, Philip Chee
iann_bugzilla: review+
Details | Diff | Review
Possible patch (1.11 KB, patch)
2012-08-28 16:06 PDT, neil@parkwaycc.co.uk
philip.chee: feedback+
Details | Diff | Review
Proposed patch (1.26 KB, patch)
2012-08-29 14:25 PDT, neil@parkwaycc.co.uk
iann_bugzilla: review+
iann_bugzilla: approval‑comm‑aurora+
Details | Diff | Review

Description Blair McBride [:Unfocused] (mostly unavailable, needinfo open, reviews not) 2012-08-15 01:36:37 PDT
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 2012-08-17 21:28:56 PDT
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] (mostly unavailable, needinfo open, reviews not) 2012-08-18 05:09:30 PDT
Bug 783763 will correct that log message, and move that test into /browser/
Comment 3 Philip Chee 2012-08-28 10:14:25 PDT
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 neil@parkwaycc.co.uk 2012-08-28 16:06:23 PDT
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 2012-08-29 08:38:29 PDT
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 neil@parkwaycc.co.uk 2012-08-29 14:23:19 PDT
(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 neil@parkwaycc.co.uk 2012-08-29 14:25:24 PDT
Created attachment 656612 [details] [diff] [review]
Proposed patch
Comment 8 Philip Chee 2012-08-29 21:29:45 PDT
> 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 2012-09-03 16:07:37 PDT
Comment on attachment 656612 [details] [diff] [review]
Proposed patch

Don't we still need the test from the first patch?
Comment 10 Philip Chee 2012-09-03 21:09:37 PDT
> 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 2012-09-04 15:16:21 PDT
Comment on attachment 656070 [details] [diff] [review]
Patch v1.0 Use TabsProgressListener

r=me for the test
Comment 13 Philip Chee 2012-09-05 02:35:13 PDT
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 2012-09-05 03:21:24 PDT
Comment on attachment 656612 [details] [diff] [review]
Proposed patch

a=me for this (and test if required)

Note You need to log in before you can comment on or make changes to this bug.