Bug 129358 – [iOS] Support network state notification using CPNetworkObserver

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Andy Estes]

[iOS] Support network state notification using CPNetworkObserver

[Andy Estes]

<rdar://problem/15961366>

[Andy Estes]

Created attachment 225216 [details]
[iOS] Support network state notification using CPNetworkObserver

[WebKit Commit Bot]

Attachment 225216 [details] did not pass style-queue:


ERROR: Source/WebCore/platform/network/NetworkStateNotifier.h:53:  "wtf/RetainPtr.h" already included at Source/WebCore/platform/network/NetworkStateNotifier.h:36  [build/include] [4]
ERROR: Source/WebCore/ChangeLog:32:  Need whitespace between colon and description  [changelog/filechangedescriptionwhitespace] [5]
Total errors found: 2 in 11 files


If any of these errors are false positives, please file a bug against check-webkit-style.

Comment on attachment 225216 [details]
[iOS] Support network state notification using CPNetworkObserver

View in context: https://bugs.webkit.org/attachment.cgi?id=225216&action=review

> Source/WebCore/platform/network/ios/NetworkStateNotifierIOS.mm:49
> +@interface NetworkStateObserver : NSObject

I thought we had a script build phase that verified this, but WebCore mustn’t contain Objective-C classes whose names don’t start with DOM or Web.

Comment on attachment 225216 [details]
[iOS] Support network state notification using CPNetworkObserver

View in context: https://bugs.webkit.org/attachment.cgi?id=225216&action=review

> Source/WebCore/platform/network/NetworkStateNotifier.h:105
> +    enum ShouldSetInitialOnLineValueTag { SetInitialOnLineValue, DoNotSetInitialOnLineValue };

I think we normally use Tag for one-value enums (where the type is really just a tag). Also odd that “do” maps to 0 and “don’t” maps to 1. If someone treated this as a bool they’d have a bad time.

> Source/WebCore/platform/network/ios/NetworkStateNotifierIOS.mm:62
> +    self = [super init];
> +    if (!self)
> +        return nil;

We normally write if (!(self = [super init])

> Source/WebCore/platform/network/ios/NetworkStateNotifierIOS.mm:63
> +    ASSERT(notifier);

This can be ASSERT_ARG and come first.

> Source/WebCore/platform/network/ios/NetworkStateNotifierIOS.mm:91
> +

Unless there is some guarantee that instances of this class are never destructed (and the EFL port appears to think that there isn’t), you need a destructor here that clears the observer’s pointer back to this object.

> Source/WebCore/platform/network/ios/NetworkStateNotifierIOS.mm:106
> +bool NetworkStateNotifier::onLine() const
> +{
> +    lazyInitialize(SetInitialOnLineValue);
> +    return m_isOnLine;
> +}

Is there no way for this to be called after an earlier call to lazyInitialize(DoNotSetInitialOnLineValue), but before a state change? Seems like in that case, the return value could be wrong.

> Source/WebKit/mac/WebView/WebViewPrivate.h:590
> ++ (void)disableReachability;

This is a weird name. Since it’s private it should have a leading underscore. But I also think the name could be more verbose. Perhaps something along the lines of +_disableNetworkReachabilityObserving, or +_doNotStartObservingNetworkReachability (which also communicates that if it’s already started, then it’s too late).

[Andy Estes]

(In reply to comment #5)
> (From update of attachment 225216 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=225216&action=review
> 
> > Source/WebCore/platform/network/ios/NetworkStateNotifierIOS.mm:91
> > +
> 
> Unless there is some guarantee that instances of this class are never destructed (and the EFL port appears to think that there isn’t), you need a destructor here that clears the observer’s pointer back to this object.

CPNetworkObserver doesn't retain network reachable observers, so I'm trusting the compiler-generated destructor to dealloc the observer. That's probably too fragile though, so I'll add code to explicitly clear it.

> 
> > Source/WebCore/platform/network/ios/NetworkStateNotifierIOS.mm:106
> > +bool NetworkStateNotifier::onLine() const
> > +{
> > +    lazyInitialize(SetInitialOnLineValue);
> > +    return m_isOnLine;
> > +}
> 
> Is there no way for this to be called after an earlier call to lazyInitialize(DoNotSetInitialOnLineValue), but before a state change? Seems like in that case, the return value could be wrong.

Nope, you're right. For what it's worth, an earlier version of the patch did the lazy initialization after receiving the first notification, which I should go back to doing.

> 
> > Source/WebKit/mac/WebView/WebViewPrivate.h:590
> > ++ (void)disableReachability;
> 
> This is a weird name. Since it’s private it should have a leading underscore. But I also think the name could be more verbose. Perhaps something along the lines of +_disableNetworkReachabilityObserving, or +_doNotStartObservingNetworkReachability (which also communicates that if it’s already started, then it’s too late).

I like +_doNotStartObservingNetworkReachability. Thanks for the suggestions.

[Andy Estes]

Created attachment 225310 [details]
[iOS] Support network state notification using CPNetworkObserver

[WebKit Commit Bot]

Attachment 225310 [details] did not pass style-queue:


ERROR: Source/WebCore/platform/network/NetworkStateNotifier.h:53:  "wtf/RetainPtr.h" already included at Source/WebCore/platform/network/NetworkStateNotifier.h:36  [build/include] [4]
ERROR: Source/WebCore/ChangeLog:32:  Need whitespace between colon and description  [changelog/filechangedescriptionwhitespace] [5]
Total errors found: 2 in 11 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Andy Estes]

Committed r164757: <http://trac.webkit.org/changeset/164757>

[Andy Estes]

*** Bug 129198 has been marked as a duplicate of this bug. ***