547485 - Listen for Gecko auto-off/online notifications and update Work Offline/Go Online menu title

Status: RESOLVED INCOMPLETE

(Camino Graveyard :: Toolbars & Menus, defect)

Description

[Smokey Ardisson (offline for a while; not following bugs - do not email)]

In 1.9.1 and greater, bug 426932 brought Gecko autodetection of network state, so it automatically goes online and offline depending on your network state.  We need to listen for state changes and update our menu so things don't go awry.

From bug 156552 comment 24:

> On 1.9.2, if Gecko auto-detects network state, we need a follow-up bug to
> listen for whatever notification it gives us and update accordingly; presumably
> with just this patch, if Gecko decides you are offline, the menu is wrong
> (although at least it would toggle it, doing the opposite of whatever it said).
> Or, if that's a pain, we could have the menu validation code query Gecko for
> the current state, assuming it's not too expensive a check in their code.

Comment 1

[Smokey Ardisson (offline for a while; not following bugs - do not email)]

I can't really track down where and what we should be listening for, exactly; it seems like nsINetworkLinkService.idl and nsIIOService2.idl define these events, and we're currently getting "offline" from the frozen nsIIOService.idl (MainController.mm 281 and 1515, before the patch for bug 156552).

http://mxr.mozilla.org/mozilla1.9.2/source/browser/base/content/browser.js#5313 (var BrowserOffline) seems to be where Fx does all this.

Comment 2

[Smokey Ardisson (offline for a while; not following bugs - do not email)]

I forgot about this one; it should be 2.1? because otherwise the UI will be out-of-sync with reality when Gecko auto-detects.

Comment 3

[Smokey Ardisson (offline for a while; not following bugs - do not email)]

(In reply to comment #2)
> I forgot about this one; it should be 2.1? because otherwise the UI will be
> out-of-sync with reality when Gecko auto-detects.

Or we could set the not-extant-by-default hidden pref "network.manage-offline-status" to false to make Gecko not auto-detect.

Comment 4

[philippe (part-time)]

(In reply to comment #3)
 
> Or we could set the not-extant-by-default hidden pref
> "network.manage-offline-status" to false to make Gecko not auto-detect.

in bug 620472, there are quite a few comments about that auto-detection code being unreliable. Maybe a good idea to set it to false ?

(I never experienced any problems, personally, though)

Comment 5

[Smokey Ardisson (offline for a while; not following bugs - do not email)]

I started to look at this last night/today.  My big-picture plan to fix this was to have the nsIObserverService listen for NS_IOSERVICE_OFFLINE_STATUS_TOPIC (and I guess figure out how to get the data, NS_IOSERVICE_OFFLINE  or NS_IOSERVICE_ONLINE), then send a notification that the menu-updating code (and later the window-title updating code) could listen for and then do their updating.

Rereading the code 11 months later, it seems obvious what and how to do. The only problem is

[4:39pm] ardissone: i think i know exactly how to fix bug 547485, but i have no idea how to do the the geckxpcom gunk :(
[4:43pm] ardissone: apparently i've learned quite a bit in the last 11 months, but still not enough :P

Comment 6

[Stuart Morgan]

We can try to consult on this on irc sometime.

Comment 7

[Oscar van Vliet]

I have had no end of silliness with this mode, please set the "network.manage-offline-status" to false by default.

I know that this also disables showing cached pages as well, but those might be outdated anyway and Camino doesn't obviously show you that.

Comment 8

[philippe (part-time)]

(In reply to comment #7)
> I have had no end of silliness with this mode, please set the
> "network.manage-offline-status" to false by default.

Can you clarify what kind of issues you've seen ?

Comment 9

[Smokey Ardisson (offline for a while; not following bugs - do not email)]

Should we just set the pref here?  Comment 5 seemed so close :(

Comment 10

[Smokey Ardisson (offline for a while; not following bugs - do not email)]

(In reply to Smokey Ardisson (back-ish; no bugmail - do not email) from comment #9)
> Should we just set the pref here?  Comment 5 seemed so close :(

No one but Oscar has complained so far (sorry Oscar), and I'd like to take another stab at comment 5 for 2.1.1.

Of course, if lots of people complain in 2.1, we can reconsider flipping the pref, too.

Comment 11

[Smokey Ardisson (offline for a while; not following bugs - do not email)]

This is on my list of things to look at for 2.1.2, particularly now that offline state is accessible to scripts.

Comment 12

[Smokey Ardisson (offline for a while; not following bugs - do not email)]

Attachment: First stab

(In reply to comment #5)
> I started to look at this last night/today.  My big-picture plan to fix this
> was to have the nsIObserverService listen for
> NS_IOSERVICE_OFFLINE_STATUS_TOPIC (and I guess figure out how to get the
> data, NS_IOSERVICE_OFFLINE or NS_IOSERVICE_ONLINE), then send a
> notification that the menu-updating code (and later the window-title
> updating code) could listen for and then do their updating.
> 
> Rereading the code 11 months later, it seems obvious what and how to do. The
> only problem is
> 
> [4:39pm] ardissone: i think i know exactly how to fix bug 547485, but i have
> no idea how to do the the geckxpcom gunk :(
> [4:43pm] ardissone: apparently i've learned quite a bit in the last 11
> months, but still not enough :P

Through the miracles of "copying Stuart's Keychain code" and "judicious use of MXR", I have an implementation of this that not only compiles, but also appears to work and not crash!

I have no idea if things are in the right place, conceptually, or whether the Gecko memory management is right, and lots of other unknown unknowns, and the existing menu-updating code probably ought to call the new method, too, but this is a start.

Stuart, sorry to toss this on your plate, too, and in this rough state, but I don't think there's anyone else around who is qualified to review or can give me the feedback needed :-(

Comment 13

[Smokey Ardisson (offline for a while; not following bugs - do not email)]

Attachment: Updated first stab

There was a recent thread on the forum about this, and one of the things that I realized (or remembered) as a result of it is that the Gecko auto-offline pref ("network.manage-offline-status") doesn't exist by default (comment 3), so users wanting to disable Gecko management have to manually insert the pref syntax into prefs.js, which is sub-optimal.  So I've added a pref declaration in all-camino.js.in to this patch to ameliorate that.  I think regardless of whether we take the fix to make the menu do the right thing, we should land the pref declaration (and maybe I should have split it into a separate patch).

The other thing I've changed in this patch is that I've addressed my own comment and made the existing menu-item-updating code call the new menu-item-updating method I added for handling auto-offline menu-item-updating.

Comment 14

[Stuart Morgan]

Comment on attachment 608247 [details] [diff] [review]
First stab

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

::: src/application/MainController.h
@@ +38,5 @@
>   * ***** END LICENSE BLOCK ***** */
>  
>  #import <Cocoa/Cocoa.h>
>  
> +#include "nsIObserver.h"

Just forward-declare this with
class nsIObserver, and then move the subclass declaration to the start of the .mm file.

@@ +134,5 @@
>  
>      GrowlController*               mGrowlController;
>  
>      NSMutableDictionary*           mCharsets;
> +    

Whitespace

@@ +250,5 @@
> +class OfflineStatusObserver : public nsIObserver
> +{
> +public:
> +  OfflineStatusObserver();
> +  virtual ~OfflineStatusObserver();

Inline the do-nothing constructor and destructor:
  OfflineStatusObserver() {}
  virtual ~OfflineStatusObserver() {}

::: src/application/MainController.mm
@@ +167,5 @@
>  
>  - (void)dealloc
>  {
> +  NS_IF_RELEASE(mOfflineStatusObserver);
> +  

Whitespace, here and elsewhere in this file.

@@ +288,5 @@
>    ioService->GetOffline(&offline);
>    mOffline = offline;
> +  
> +  // Register with the Gecko observer service for offline status notifications.
> +  //XXX Doesn't the AbbObserver need to be cleaned up on dealloc?

This class outlives XPCOM shutdown, so it would have to be during that, not dealloc. But since XPCOM is being shut down, it doesn't really matter, so we can just ignore the removal.

@@ +291,5 @@
> +  // Register with the Gecko observer service for offline status notifications.
> +  //XXX Doesn't the AbbObserver need to be cleaned up on dealloc?
> +  nsCOMPtr<nsIObserverService> observerService = do_GetService("@mozilla.org/observer-service;1");
> +  mOfflineStatusObserver = new OfflineStatusObserver();
> +  if (observerService && mOfflineStatusObserver) {

No need to check mOfflineStatusObserver; it can only fail if we're out of memory, and then we're screwed anyway. But move the creation of mOfflineStatusObserver into the |if (observerService)|

@@ +1183,5 @@
>    if (textZoomOnlyPref != textZoomOnlyUISet)
>      [self toggleTextZoom:nil];
>  }
>  
> +- (void)updateOfflineMenuState:(NSNotification *)notification

Call this offlineStateChanged: so it's not confused with the other method.

@@ +2522,5 @@
> +
> +OfflineStatusObserver::~OfflineStatusObserver()
> +{
> +  //NSLog(@"Offline status observer died.");
> +}

These two method implementationswill go away, since you'll have inlined the empty implementations.

@@ +2530,5 @@
> +{
> +  BOOL newOnlineState = NO;
> +  if (nsDependentString(someData).EqualsLiteral(NS_IOSERVICE_ONLINE))
> +    newOnlineState = YES;
> +//  else if (nsDependentString(aData).EqualsLiteral(NS_IOSERVICE_OFFLINE))

Remove this line.

Comment 15

[Stuart Morgan]

Comment on attachment 643718 [details] [diff] [review]
Updated first stab

I somehow managed to review the obsolete patch; I think everything still applies though.

Comment 16

[Smokey Ardisson (offline for a while; not following bugs - do not email)]

Attachment: Updated for review comments

I had no internet at home last night, so I worked on this patch, and addressing the review comments turned out to be a lot simpler than I had initially thought.