Bug 129358 - [iOS] Support network state notification using CPNetworkObserver
Summary: [iOS] Support network state notification using CPNetworkObserver
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Andy Estes
URL:
Keywords: InRadar
: 129198 (view as bug list)
Depends on:
Blocks:
 
Reported: 2014-02-25 19:48 PST by Andy Estes
Modified: 2014-03-05 21:31 PST (History)
3 users (show)

See Also:


Attachments
[iOS] Support network state notification using CPNetworkObserver (22.24 KB, patch)
2014-02-25 20:35 PST, Andy Estes
no flags Details | Formatted Diff | Diff
[iOS] Support network state notification using CPNetworkObserver (23.21 KB, patch)
2014-02-26 15:32 PST, Andy Estes
mitz: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Andy Estes 2014-02-25 19:48:12 PST
[iOS] Support network state notification using CPNetworkObserver
Comment 1 Andy Estes 2014-02-25 20:16:14 PST
<rdar://problem/15961366>
Comment 2 Andy Estes 2014-02-25 20:35:49 PST
Created attachment 225216 [details]
[iOS] Support network state notification using CPNetworkObserver
Comment 3 WebKit Commit Bot 2014-02-25 20:37:03 PST
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 4 mitz 2014-02-25 20:43:52 PST
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 5 mitz 2014-02-25 21:16:53 PST
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).
Comment 6 Andy Estes 2014-02-25 22:29:46 PST
(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.
Comment 7 Andy Estes 2014-02-26 15:32:39 PST
Created attachment 225310 [details]
[iOS] Support network state notification using CPNetworkObserver
Comment 8 WebKit Commit Bot 2014-02-26 15:33:49 PST
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.
Comment 9 Andy Estes 2014-02-26 16:14:20 PST
Committed r164757: <http://trac.webkit.org/changeset/164757>
Comment 10 Andy Estes 2014-03-05 21:31:33 PST
*** Bug 129198 has been marked as a duplicate of this bug. ***