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

[bugzilla-daemon at webkit.org]

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

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


> Source/WebCore/Modules/networkinfo/NavigatorNetworkInfoConnection.h:38
> +class NavigatorNetworkInfoConnection : public Supplement<Navigator> {

I might have just called this class NavigatorNetworkInfo (to match the module
name), but this is fine too.

> Source/WebCore/Modules/networkinfo/NetworkInfo.cpp:32
> +#if ENABLE(NETWORK_INFO)

I think we normally have a blank line after file-wide ifdefs.

> Source/WebCore/Modules/networkinfo/NetworkInfoController.cpp:75
> +	for (NetworkInfoListenerList::iterator it = m_listeners.begin(); it !=
m_listeners.end(); ++it)
> +	    (*it)->didChangeNetworkInformation(event, networkInformation);

Do we need to make a new event object for each listener?  My worry is that when
the Event gets a JavaScript wrapper, the wrapper will be re-used for each
listener, which could be in different security contexts.  Can you write a test
for this case to make sure they get different wrappers?