[webkit-reviews] review denied: [Bug 73528] Support the Network Information API : [Attachment 119951] Patch

[bugzilla-daemon at webkit.org]

Kenneth Rohde Christiansen <kenneth at webkit.org> has denied Gyuyoung Kim
<gyuyoung.kim at samsung.com>'s request for review:
Bug 73528: Support the Network Information API
https://bugs.webkit.org/show_bug.cgi?id=73528

Attachment 119951: Patch
https://bugs.webkit.org/attachment.cgi?id=119951&action=review

------- Additional Comments from Kenneth Rohde Christiansen
<kenneth at webkit.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=119951&action=review


> Source/WebCore/page/Connection.h:42
> +class Connection : public RefCounted<Connection> {

That is a very very generic name, too generic to me. What kind of connection?

> Source/WebCore/page/Navigator.cpp:139
>	   return String();
> -	   
> +
>      // If the frame is already detached, FrameLoader::userAgent may
malfunction, because it calls a client method
>      // that uses frame's WebView (at least, in Mac WebKit).
>      if (!m_frame->page())
>	   return String();
> -	   
> +

Don't mix this patch with cleanup

> Source/WebCore/platform/network/NetworkInformation.cpp:46
> +NetworkInformation& networkInformation()

I would really prefer a NetworkInfoClient instead

> Source/WebCore/platform/network/NetworkInformation.cpp:62
> +#if OS(UNIX)
> +    return "eth";
> +#elif OS(MAX_OS_X)
> +    return "en";
> +#else
> +    return "";
> +#endif
> +}

This is not going to scale nicely

> Source/WebCore/platform/network/NetworkInformation.cpp:114
> +    return String("unknown");
> +#elif OS(ANDROID)
> +    notImplemented();
> +    return String("unknown");
> +#elif OS(MAC_OS_X)
> +    notImplemented();
> +    return String("unknown");
> +#elif OS(IOS)
> +    notImplemented();
> +    return String("unknown");
> +#elif OS(QNX)
> +    notImplemented();
> +    return String("unknown");
> +#else
> +    return String("unknown");

Why didn't you join these? They are all the same

#elif OS(ANDROID) || ...

> Source/WebCore/platform/network/NetworkInformation.cpp:131
> +String NetworkInformation::getEthernetNetworkType()
> +{
> +    FILE* fileHandle;
> +    int startReadLine = -2; 
> +    char readLine[1024];
> +

I would really make a client instead. You could make a default implementation
such as NetworkInfoClientPOSIX or so if you want to share the client code for
EFL and GTK+

> Source/WebCore/platform/network/NetworkInformation.h:36
> +    WTF_MAKE_NONCOPYABLE(NetworkInformation); WTF_MAKE_FAST_ALLOCATED;

Why one line?

> Source/WebCore/platform/network/NetworkInformation.h:45
> +#if OS(UNIX)
> +    String getWirelessNetworkType();
> +    String getEthernetNetworkType();
> +#endif

That really seems like an implementation detail