[webkit-reviews] review denied: [Bug 73528] Support the Network Information API : [Attachment 120309] 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 120309: Patch
https://bugs.webkit.org/attachment.cgi?id=120309&action=review

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


> Source/WebCore/GNUmakefile.list.am:5111
> +	Source/WebCore/platform/gtk/NetworkInfoClientGtk.cpp \

No, clients should be implemented in WebKit/.../WebCoreSupport/

> Source/WebCore/Target.pri:3868
> +	   page/WebKitConnection.h \

Wouldn't NetworkInfoConnection make a whole lot more sense?

> Source/WebCore/page/WebKitConnection.cpp:39
> +WebKitConnection::WebKitConnection(Frame* frame, NetworkInfoClient* client)

WebKitConnection is even worse than just Connection. It is the connection
object for getting network info, why not call it NetworkInfoConnection

> Source/WebCore/platform/NetworkInfoClient.h:44
> +#if PLATFORM(EFL)
> +    String getWirelessNetworkType() const;
> +    String getEthernetNetworkType() const;
> +#endif

These should only be defined in your own client implementation, not here. The
clients should, if at all possible, not contain platform dependent things

> Source/WebCore/platform/posix/NetworkInfoClientPOSIX.cpp:68
> +{
> +    struct ifreq* ifStart, *ifEnd;
> +    struct ifreq ifRequest, ifRequests[10];
> +    struct ifconf ifConfiguration;

i dont think we allow to define multiple variable on one line

Why not define ifStart and ifEnd where you actually use them? It is also more
common to use it/end.

const struct ifreq* end = ...
for (const struct ifreq* it, it < end; ++it) {

}