Plumb through NetworkChangeNotifier::IsOffline() to WebKit, enabling

Plumb through NetworkChangeNotifier::IsOffline() to WebKit, enabling
navigator.onLine and online/offline events.  Only works on Windows at the moment, as IsCurrentlyOffline() is supported only by NetworkChangeNotifierWin.

Most of the changes are due to the need to support two different kinds of NetworkChangeNotifier observers. Both observers currently happen to trigger on the same event, but that could change, e.g., if we store the previous online state and only notify on a change. Thus the need for two different observer interfaces, and associated Add/Remove methods.

BUG=7469
TEST=Load https://bug336359.bugzilla.mozilla.org/attachment.cgi?id=220609, unplug network cable, reload, see that page changes to note offline status

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=76985

[adamk, 2011-02-25 02:27:45]

No tests yet; recommendations on what should be unit tested here would be
appreciated.

[eroman, 2011-03-01 05:23:10]

LGTM, sorry for not getting to this sooner!

I like this breakdown between the IP address notification and the online/offline
state, however I am adding willchan for additional comment on the interface.

The future as I see it is for NetworkChangeNotifier to handle more than just IP
address changes, and provide a richer set of "the network has changed"
notifications. One signal of course is the IP address changing, but other
signals would include the DNS resolver becoming available, the HOSTS file
changing, etc.. (see for instance 67734 and 57102).

If we go this route, we will have to add separate observer types for each
granularity (i am ok with that). The alternative would be to lump them all
together into one interface.

http://codereview.chromium.org/6526059/diff/3001/net/base/network_change_noti...
File net/base/network_change_notifier_win.cc (right):

http://codereview.chromium.org/6526059/diff/3001/net/base/network_change_noti...
net/base/network_change_notifier_win.cc:156:
MessageLoop::current()->PostDelayedTask(
I suggest adding a bit to keep track of the outstanding task, and only allow 1
such task to be outstanding at a time.

You should be able to use the oneshottimer class and reset it to 1 second each
time.

This way we can avoid calling IsCurrentlyOffline() multiple times when lots of
activity is happening.

[eroman, 2011-03-01 05:23:27]

Actually add Will this time :)

[adamk, 2011-03-01 19:20:06]

http://codereview.chromium.org/6526059/diff/3001/net/base/network_change_noti...
File net/base/network_change_notifier_win.cc (right):

http://codereview.chromium.org/6526059/diff/3001/net/base/network_change_noti...
net/base/network_change_notifier_win.cc:156:
MessageLoop::current()->PostDelayedTask(
On 2011/03/01 05:23:10, eroman wrote:
> I suggest adding a bit to keep track of the outstanding task, and only allow 1
> such task to be outstanding at a time.
> 
> You should be able to use the oneshottimer class and reset it to 1 second each
> time.
> 
> This way we can avoid calling IsCurrentlyOffline() multiple times when lots of
> activity is happening.

Thanks for the tip, replaced with a OneShotTimer.  The one slightly odd thing is
that we're now calling a static function as though it were an instance method,
but from my understanding that's guaranteed to work fine.

A side note: it looks like the only reason the Notify method is static is to
make it easy to call from unit tests.  In a followup change I might make these
instance methods just for clarity's sake.

[adamk, 2011-03-01 20:09:01]

On 2011/03/01 19:20:06, Adam Klein wrote:
>
http://codereview.chromium.org/6526059/diff/3001/net/base/network_change_noti...
> File net/base/network_change_notifier_win.cc (right):
> 
>
http://codereview.chromium.org/6526059/diff/3001/net/base/network_change_noti...
> net/base/network_change_notifier_win.cc:156:
> MessageLoop::current()->PostDelayedTask(
> On 2011/03/01 05:23:10, eroman wrote:
> > I suggest adding a bit to keep track of the outstanding task, and only allow
1
> > such task to be outstanding at a time.
> > 
> > You should be able to use the oneshottimer class and reset it to 1 second
each
> > time.
> > 
> > This way we can avoid calling IsCurrentlyOffline() multiple times when lots
of
> > activity is happening.
> 
> Thanks for the tip, replaced with a OneShotTimer.  The one slightly odd thing
is
> that we're now calling a static function as though it were an instance method,
> but from my understanding that's guaranteed to work fine.

But of course this won't work because I'm not calling the method directly but
trying to pass it around, and static methods have different types than instance
methods.  Will upload a fixed patch soon.  No idea what's wrong with the
trybots.
 
> A side note: it looks like the only reason the Notify method is static is to
> make it easy to call from unit tests.  In a followup change I might make these
> instance methods just for clarity's sake.

Seems like I might as well do this now, at least for my notify method.

[eroman, 2011-03-04 01:29:38]

LGTM