Last Comment Bug 663253 - Remove the 'browser.offline' preference (don't remember offline mode from the previous session)
: Remove the 'browser.offline' preference (don't remember offline mode from the...
Status: VERIFIED FIXED
:
Product: Firefox
Classification: Client Software
Component: Preferences (show other bugs)
: Trunk
: All All
: -- normal (vote)
: Firefox 7
Assigned To: Steffen Wilberg
:
Mentors:
http://mxr.mozilla.org/mozilla-centra...
Depends on: 435325
Blocks: 667596 636148
  Show dependency treegraph
 
Reported: 2011-06-09 15:15 PDT by Steffen Wilberg
Modified: 2013-11-12 00:56 PST (History)
6 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
patch (5.86 KB, patch)
2011-06-11 10:26 PDT, Steffen Wilberg
gavin.sharp: review+
fryn: feedback+
Details | Diff | Splinter Review

Description Steffen Wilberg 2011-06-09 15:15:20 PDT
Gavin asked in bug 435325 comment 37 to remove the browser.offline pref:
> I don't think core code should be touching this pref. It's Firefox-specific
> AFAICT. Our use of browser.offline is bogus and I think we should get rid of
> it, but that should happen in a followup bug.

That pref currently sets the initial state to offline after startup, if the user had selected "Work Offline" in the previous session. Removing that pref means that Firefox will always start online, unless the NetworkManager sets it to offline (if the network.manage-offline-status pref is true; it had been disabled in bug 620472).

This fixes bug 636148 (Upgrading to 4.0 from 3.6 with offline state auto detected to true leaves the user offline since 4.0 no longer manages offline state).

http://mxr.mozilla.org/mozilla-central/search?string=%22browser.offline%22&tree=mozilla-central
Comment 1 Steffen Wilberg 2011-06-11 10:26:04 PDT
Created attachment 538703 [details] [diff] [review]
patch

Simple code removal, shouldn't take long to review..
The comment in nsDBusService.h is obviously referring to nsBrowserGlue.js; dbusservice doesn't touch the pref itself.

I'll file a follow-up on mozilla/extensions/irc/xul/content/static.js.
Comment 2 :Gavin Sharp [email: gavin@gavinsharp.com] 2011-06-13 19:18:02 PDT
I may have been hasty in suggesting we just rip it out - we could also just  fix _updateOfflineUI to also set the pref so that it reflects the actual offline state at all times (so it would effectively just be a way to ensure that the offline state is persisted across restarts).

On the flip side, we didn't really support that kind of persistence when automatic link detection was enabled prior to Firefox 4, AFAICT (unless you manually disabled link detection), and I hadn't heard of anyone being too upset about that, so maybe we can just kill it.
Comment 3 :Gavin Sharp [email: gavin@gavinsharp.com] 2011-06-20 13:47:05 PDT
Comment on attachment 538703 [details] [diff] [review]
patch

OK, let's just do this.

The changes to xpinstall/tests/browser_offline.js conflict with bug 652376 - best to just omit them and save Mossop the merging trouble, I think.
Comment 4 Frank Yan (:fryn) 2011-06-20 17:18:14 PDT
Comment on attachment 538703 [details] [diff] [review]
patch

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

There are still a few appearances of "browser.offline" that should be removed:
https://mxr.mozilla.org/mozilla-central/search?string=browser.offline

::: browser/components/nsBrowserGlue.js
@@ -358,5 @@
> -      try {
> -        Services.io.offline = Services.prefs.getBoolPref("browser.offline");
> -      }
> -      catch (e) {
> -        Services.io.offline = false;

Do you make sure that, after removing this, Services.io.offline is still correctly set to false?
Comment 5 Steffen Wilberg 2011-06-21 00:11:33 PDT
Comment on attachment 538703 [details] [diff] [review]
patch

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

> There are still a few appearances of "browser.offline" that should be
> removed:
> https://mxr.mozilla.org/mozilla-central/search?string=browser.offline
That search also returns "browser.offline-apps.notify" and "services.sync.prefs.sync.browser.offline-apps.notify", which are unrelated.
https://mxr.mozilla.org/mozilla-central/source/js/src/tests/e4x/Regress/regress-308111.js#591 is not worth changing, since it's a regression test "Do not crash when searching large e4x tree" which doesn't read or write prefs, but just uses the names; could replace that by "foo.bar" as well. See bug 636148 comment 17: "There's no reason to keep this file in sync with actual prefs."

I used http://mxr.mozilla.org/mozilla-central/search?string=%22browser.offline%22&tree=mozilla-central and covered all of those.

::: browser/components/nsBrowserGlue.js
@@ -358,5 @@
> -      try {
> -        Services.io.offline = Services.prefs.getBoolPref("browser.offline");
> -      }
> -      catch (e) {
> -        Services.io.offline = false;

I don't, since not setting it is the point of this bug: don't remember a user-selected offline state from the previous session. See comment 0:
"Removing that pref means that Firefox will always start online, unless the NetworkManager sets it to offline (if the network.manage-offline-status pref is true; it had been disabled in bug 620472)."
But maybe I'm misunderstanding you?
Comment 6 Frank Yan (:fryn) 2011-06-21 12:41:21 PDT
Comment on attachment 538703 [details] [diff] [review]
patch

(In reply to comment #5)

No, I meant, to put it more concisely:
Do we still need the line:
Services.io.offline = false;
or is it already set to false initially?
It turns out that it's true initially, but when manage-office-status is false, my patch for bug 627332 (on which bug 620472 depended) ensures that it gets set to false, so you're fine! :)
(the line I was looking for: https://mxr.mozilla.org/mozilla-central/source/netwerk/base/src/nsIOService.cpp#275 )
Comment 7 Steffen Wilberg 2011-06-21 13:36:27 PDT
Changes to xpinstall/tests/browser_offline.js removed.
http://hg.mozilla.org/integration/mozilla-inbound/rev/e3797dc14be1
Comment 8 Matt Brubeck (:mbrubeck) 2011-06-22 10:17:50 PDT
http://hg.mozilla.org/mozilla-central/rev/e3797dc14be1
Comment 9 George Carstoiu 2011-06-29 01:18:45 PDT
Mozilla/5.0 (X11; Linux i686; rv:7.0a1) Gecko/20110628 Firefox/7.0a1

Preference no longer present on builds from WinXP, Win7, Ubuntu 11.04 and MAC OS X 10.6.

Setting status to Verified Fixed.

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