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

[bugzilla-daemon at webkit.org]

Adam Barth <abarth 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 133963: Patch
https://bugs.webkit.org/attachment.cgi?id=133963&action=review

------- Additional Comments from Adam Barth <abarth at webkit.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=133963&action=review


This patch has improved a lot since the last time I looked at it.  I'm somewhat
concerned that there appears to be a bunch of copy/paste code in here that
doesn't really make sense.  I've noted a number of examples below.  These sorts
of things are red flags and means that I would need to review the patch much
more carefully before R+ing it.

> Source/WebCore/Modules/networkinfo/NavigatorNetworkInfoConnection.cpp:64
> +	   navigatorConnection->m_connection =
NetworkInfoConnection::create(context, navigator);

This isn't correct.  You should get the ScriptExecutionContext via
navigator->frame()->document().

> Source/WebCore/Modules/networkinfo/NavigatorNetworkInfoConnection.idl:23
> +	   CallWith=ScriptExecutionContext,

This should be removed.

> Source/WebCore/Modules/networkinfo/NetworkInfoClient.h:50
> +    using RefCounted<NetworkInfoClient>::ref;
> +    using RefCounted<NetworkInfoClient>::deref;

You don't need these lines of code.

> Source/WebCore/Modules/networkinfo/NetworkInfoConnection.cpp:87
> +bool NetworkInfoConnection::addEventListener(const AtomicString& eventType,
PassRefPtr<EventListener> listener, bool useCapture)
> +{
> +    if (!EventTarget::addEventListener(eventType, listener, useCapture))
> +	   return false;
> +
> +    return true;
> +}

This function is not needed.

> Source/WebCore/Modules/networkinfo/NetworkInfoConnection.cpp:95
> +bool NetworkInfoConnection::removeEventListener(const AtomicString&
eventType, EventListener* listener, bool useCapture)
> +{
> +    if (!EventTarget::removeEventListener(eventType, listener, useCapture))
> +	   return false;
> +
> +    return true;
> +}

This function is not needed.

> Source/WebCore/Modules/networkinfo/NetworkInfoController.cpp:81
> +bool NetworkInfoController::isActive(Page* page)
> +{
> +	return static_cast<bool>(NetworkInfoController::from(page));
> +}

This function isn't needed.