Closed Bug 782892 Opened 8 years ago Closed 8 years ago

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

Categories

(SeaMonkey :: General, defect)

defect
Not set

Tracking

(seamonkey2.14+ fixed, seamonkey2.15 fixed)

RESOLVED FIXED
seamonkey2.15
Tracking Status
seamonkey2.14 + fixed
seamonkey2.15 --- fixed

People

(Reporter: Unfocused, Assigned: neil)

References

Details

Attachments

(2 files, 1 obsolete file)

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;
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).
Bug 783763 will correct that log message, and move that test into /browser/
Depends on: 783763
Assignee: nobody → philip.chee
Status: NEW → ASSIGNED
Version: unspecified → Trunk
This patch works when I test off-line manually but the test copied from Firefox fails for a reason I cannot discern.
Attachment #656070 - Flags: feedback?(neil)
Attached patch Possible patch (obsolete) — Splinter Review
This version seems to pass your test, as far as I can tell...
Attachment #656243 - Flags: feedback?(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
Attachment #656243 - Flags: feedback?(philip.chee) → feedback+
(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...
Attached patch Proposed patchSplinter Review
Assignee: philip.chee → neil
Attachment #656070 - Attachment is obsolete: true
Attachment #656243 - Attachment is obsolete: true
Attachment #656070 - Flags: feedback?(neil)
Attachment #656612 - Flags: review?(iann_bugzilla)
> 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 on attachment 656612 [details] [diff] [review]
Proposed patch

Don't we still need the test from the first patch?
Attachment #656612 - Flags: review?(iann_bugzilla) → review+
> Don't we still need the test from the first patch?
Yes. I can check both in if you r+ the test.
Comment on attachment 656070 [details] [diff] [review]
Patch v1.0 Use TabsProgressListener

r=me for the test
Attachment #656070 - Attachment is obsolete: false
Attachment #656070 - Flags: review+
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.
Attachment #656612 - Flags: approval-comm-aurora?
Comment on attachment 656612 [details] [diff] [review]
Proposed patch

a=me for this (and test if required)
Attachment #656612 - Flags: approval-comm-aurora? → approval-comm-aurora+
Pushed to comm-aurora:
http://hg.mozilla.org/releases/comm-aurora/rev/ada7d03ccea4
http://hg.mozilla.org/releases/comm-aurora/rev/ee74d240d1ce (test)
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → seamonkey2.15
You need to log in before you can comment on or make changes to this bug.