[webkit-reviews] review denied: [Bug 129358] [iOS] Support network state notification using CPNetworkObserver : [Attachment 225216] [iOS] Support network state notification using CPNetworkObserver

[bugzilla-daemon at webkit.org]

mitz at webkit.org <mitz at webkit.org> has denied Andy Estes <aestes at apple.com>'s
request for review:
Bug 129358: [iOS] Support network state notification using CPNetworkObserver
https://bugs.webkit.org/show_bug.cgi?id=129358

Attachment 225216: [iOS] Support network state notification using
CPNetworkObserver
https://bugs.webkit.org/attachment.cgi?id=225216&action=review

------- Additional Comments from mitz at webkit.org <mitz at webkit.org>
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).