Bug (NetInfoAPI) – Support the Network Information API

• Component: Platform
• Status: RESOLVED FIXED

[Gyuyoung Kim]

Network Information APIs is to provide an interface for Web Applications to access the underlying network information of device. In Web Application case, they need to know what current network interface it uses. Because, it is important to know current network backbone(wifi, 3g, 4G and so on) in mobile domain. Thus, Web Application can let user know whether current network bearer is 3G or wifi via this new functionality. In addition, in streaming service case, Web Application can control content resolution according to kind of network.

Spec : http://www.w3.org/TR/netinfo-api/


I submit a prototype for this feature. But, I'm not sure if location of concrete implementation is correct. For now, I add concrete implementation to NetworkStateNotifier.cpp first. Please give any advices or opinions for this feature.

[Gyuyoung Kim]

Created attachment 117352 [details]
Patch

[Early Warning System Bot]

Comment on attachment 117352 [details]
Patch

Attachment 117352 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/10704519

[Mark Rowe (bdash)]

A few comments:
1) There are many style issues in the patch. Indentation is wonky in many places, and there are a few constructors where members should be initialized via the initialization list rather than assignment in the body.

2) I’m not sure that the provided implementation that is currently wrapped in OS(UNIX) should be OS(UNIX). Network interface names vary widely between different variants of UNIX. On Mac OS X, for instance, Ethernet interfaces are prefixed with “en” rather than “eth”.

3) The provided implementation may also misreport wifi interfaces as ethernet.

4) For platforms that have no implementation I suspect it would be preferable to not expose the interface at all rather than exposing an implementation that always returns “unknown”. This would make it possible to determine from script whether it will ever receive a useful result.

5) Your test as written is not usable. Different machines have different network configurations so we cannot hard-code in the result that an ethernet connection was detected.

6) The constants you’ve used do not match those in the specification:
> The value returned is one of the following strings, *case-sensitively*: unknown, ethernet, wifi, 2g, 3g, 4g, none. 

7) Your patch adds a networkInfo attribute to the navigator object that the specification does not mention at all.  The intent is for the navigator object to implement the NetworkInfo interface, not to expose an attribute that implements it.

8) I’m a little concerned about the specification itself. Many modern devices have more than one network connection available to them at any one time, and may even use different network interfaces for accessing different resources on the same page. The device may also bring a new network interface online in response to the browser requesting a resource (consider a mobile device that drops its wifi connection but will reestablish it on demand, or a device may establish and use a VPN connection for accessing particular hosts). The specification doesn’t appear to have considered these sorts of situations.

[Ojan Vafai]

Also, all new features need an email to webkit-dev first. http://www.webkit.org/coding/adding-features.html

[Gyuyoung Kim]

(In reply to comment #4)
> Also, all new features need an email to webkit-dev first. http://www.webkit.org/coding/adding-features.html

Yes, I'm going to notify this patch according to new feature guidance. Thank you.

[WebKit Review Bot]

Comment on attachment 117352 [details]
Patch

Attachment 117352 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/10708011

[Gyuyoung Kim]

(In reply to comment #3)

First of all, thank you for your detail review.

> A few comments:
> 1) There are many style issues in the patch. Indentation is wonky in many places, and there are a few constructors where members should be initialized via the initialization list rather than assignment in the body.

I'm sorry. I will fix style nits in next patch.

> 2) I’m not sure that the provided implementation that is currently wrapped in OS(UNIX) should be OS(UNIX). Network interface names vary widely between different variants of UNIX. On Mac OS X, for instance, Ethernet interfaces are prefixed with “en” rather than “eth”.

This patch is prototyping yet. The NetworkStateNotifier::networkInformation() will be improved for variants platforms. I will improve the implementation in next patch.


> 3) The provided implementation may also misreport wifi interfaces as ethernet.

I will support wifi as well.

> 4) For platforms that have no implementation I suspect it would be preferable to not expose the interface at all rather than exposing an implementation that always returns “unknown”. This would make it possible to determine from script whether it will ever receive a useful result.


> 5) Your test as written is not usable. Different machines have different network configurations so we cannot hard-code in the result that an ethernet connection was detected.

Yes, you right. I will make each expected result files for each platform.

> 6) The constants you’ve used do not match those in the specification:
> > The value returned is one of the following strings, *case-sensitively*: unknown, ethernet, wifi, 2g, 3g, 4g, none. 

As mentioned in above, I will improve the implementation.


> 7) Your patch adds a networkInfo attribute to the navigator object that the specification does not mention at all.  The intent is for the navigator object to implement the NetworkInfo interface, not to expose an attribute that implements it.
>

Ok, I will fix it.


> 8) I’m a little concerned about the specification itself. Many modern devices have more than one network connection available to them at any one time, and may even use different network interfaces for accessing different resources on the same page. The device may also bring a new network interface online in response to the browser requesting a resource (consider a mobile device that drops its wifi connection but will reestablish it on demand, or a device may establish and use a VPN connection for accessing particular hosts). The specification doesn’t appear to have considered these sorts of situations.

This specification is not finished yet. This specification is also considering the issues you mentioned.(How to process online and offline situation) In my opinion, this issues will be resolved. But, I think basic functionality will be helpful for Web Application for now.

[Adam Barth]

Comment on attachment 117352 [details]
Patch

Also, all these new files should be in Modules/netinfo and should be behind an ENABLE(NETINFO) compile flag.

[Gustavo Noronha (kov)]

Comment on attachment 117352 [details]
Patch

Attachment 117352 [details] did not pass gtk-ews (gtk):
Output: http://queues.webkit.org/results/10690239

[Gyuyoung Kim]

(In reply to comment #8)
> (From update of attachment 117352 [details])
> Also, all these new files should be in Modules/netinfo and should be behind an ENABLE(NETINFO) compile flag.

Ok, I will use ENABLE(NETINFO) macro and change new file location from next patch. I will submit improved patch soon. Thank you.

[Gyuyoung Kim]

Created attachment 117847 [details]
Prototype - 2

Hello Mark,

>> 5) Your test as written is not usable. Different machines have different network configurations so we cannot hard-code in the result that an ethernet connection was detected.

Do I need to make different expected result file per each port ? 

BTW, I add concrete implementation to WebCore/platform/network/NetworkStateNotifire.cpp for now. But, I'm considering which place is best. For example, WebKit/WebCoreSupport, WebCore/platform/(port directory) or WebCore/platform/network/new file(NetworkInformation.cpp). How do you think which one is better ?

[Mark Rowe (bdash)]

(In reply to comment #11)
> Created an attachment (id=117847) [details]
> Prototype - 2
> 
> Hello Mark,
> 
> >> 5) Your test as written is not usable. Different machines have different network configurations so we cannot hard-code in the result that an ethernet connection was detected.
> 
> Do I need to make different expected result file per each port ? 

I’m not sure why you’re asking this. The test you have provided so far will give different results per machine, not per port.

> BTW, I add concrete implementation to WebCore/platform/network/NetworkStateNotifire.cpp for now. But, I'm considering which place is best. For example, WebKit/WebCoreSupport, WebCore/platform/(port directory) or WebCore/platform/network/new file(NetworkInformation.cpp). How do you think which one is better ?

I’m not sure that NetworkStateNotifier is a good place for this. WebKit I think is also a bad place for this. I’m not sure which of the other alternatives is best.

[Gyuyoung Kim]

(In reply to comment #12)
> (In reply to comment #11)
> > Created an attachment (id=117847) [details] [details]
> > Prototype - 2
> > 
> > Hello Mark,
> > 
> > >> 5) Your test as written is not usable. Different machines have different network configurations so we cannot hard-code in the result that an ethernet connection was detected.
> > 
> > Do I need to make different expected result file per each port ? 
> 
> I’m not sure why you’re asking this. The test you have provided so far will give different results per machine, not per port.

My wrong expression. It looks LayoutTests/platform has different results set per machine. I think this patch needs to add different results to expected result set of each machine.

[Mark Rowe (bdash)]

(In reply to comment #13)
> (In reply to comment #12)
> > (In reply to comment #11)
> > > Created an attachment (id=117847) [details] [details] [details]
> > > Prototype - 2
> > > 
> > > Hello Mark,
> > > 
> > > >> 5) Your test as written is not usable. Different machines have different network configurations so we cannot hard-code in the result that an ethernet connection was detected.
> > > 
> > > Do I need to make different expected result file per each port ? 
> > 
> > I’m not sure why you’re asking this. The test you have provided so far will give different results per machine, not per port.
> 
> My wrong expression. It looks LayoutTests/platform has different results set per machine. I think this patch needs to add different results to expected result set of each machine.

The results in LayoutTests/platform are different results per platform. The results for your test will differ between machines as different machines will have different network configurations (e.g., a given computer may not have ethernet or may only have 3G). It isn’t a matter of checking in different results for different platforms. You’ll need to make the test given stable results across these variations between machines.

[John Knottenbelt]

Comment on attachment 117847 [details]
Prototype - 2

View in context: https://bugs.webkit.org/attachment.cgi?id=117847&action=review

> LayoutTests/fast/dom/navigator-networkInfo.html:7
> +    document.write("Current underlying network information : " + navigator.connection.type + "<br>");

The development spec says that the possible values are unknown, ethernet, wifi, 2g, 3g, 4g, none or some vendor-prefixed value. In terms of a layout test, I would normally expect to see some method setting up a mock state and then confirming that the API returns it. For example:

if (window.layoutTestController) {
   layoutTestController.setMockNetworkType("ethernet");
}

document.write("navigator.connection.type = " + navigator.connection.type + "<br>");

However, I think that in this case testing that the navigator.connection.type comes back with the value it has been mocked to be, is not that useful. 

I think a good alternative, or additional, test would be to display the type of the of the navigator.connection.type property. Something similar to what is happening in fast/workers/worker-navigator.html 

document.write("navigator.connection.type is a " + typeof navigator.connection.type);

And the expected result would be, of course,

navigator.connection.type is a string

[Gyuyoung Kim]

Created attachment 118042 [details]
Prototype - 3

>> I think a good alternative, or additional, test would be to display the type of the of the navigator.connection.type property. Something similar to what is happening in fast/workers/worker-navigator.html 

>> document.write("navigator.connection.type is a " + typeof navigator.connection.type);

I don't find best test case for this patch yet. So, I think your suggestion is good for now. And also, there are many test cases that check type of return value. I modify test case to check *return type* of connection.type using js-test-pre.js file. 

I'm implementing functions to check kind of network connection for each machine as well as considering which implementing place is best.

[Gyuyoung Kim]

Created attachment 118164 [details]
Prototype - 3

Some files were missed in previous patch. And, I add new file - NetworkInformation.cpp | h for this feature.

[Gyuyoung Kim]

Created attachment 118355 [details]
Prototype - 4

I improve this patch a little. Currently, this patch is able to be run on EFL, GTK and QT port. If there are no functions to know device type, I'm considering to find the type of network device according to network speed. If you know any opinions about this, please let me know.

[Gyuyoung Kim]

Created attachment 118979 [details]
Prototype - 5

I use value of "/proc/net/wireless" in order to check if network interface is wifi or ethernet (http://www.hpl.hp.com/personal/Jean_Tourrilhes/Linux/Linux.Wireless.Extensions.html). If wifi is enabled, the "/proc/net/wireless" shows enabled interface information. So, if there is interface information, I regard wifi is connected. However, I don't find a solution how to check type of cellular network yet. Anyway, current patch supports *ethernet*, *wifi* and *cellular*(2g, 3g and 4g). If you have any comments, please let me know. Thank you.

[Raphael Kubo da Costa (:rakuco)]

(In reply to comment #19)
> Created an attachment (id=118979) [details]
> Prototype - 5
> 
> I use value of "/proc/net/wireless" in order to check if network interface is wifi or ethernet (http://www.hpl.hp.com/personal/Jean_Tourrilhes/Linux/Linux.Wireless.Extensions.html). If wifi is enabled, the "/proc/net/wireless" shows enabled interface information. So, if there is interface information, I regard wifi is connected. However, I don't find a solution how to check type of cellular network yet. Anyway, current patch supports *ethernet*, *wifi* and *cellular*(2g, 3g and 4g). If you have any comments, please let me know. Thank you.

Small comment without reviewing the rest of the patch: those #if OS(UNIX) statements should really be Linux-only. "eth", /proc and friends are Linuxisms which will break the build for other Unixen (the BSDs, for example).

[Gyuyoung Kim]

I find mozilla is implementing this API as well.

Bug 677166 - Implement Network Status API
(https://bugzilla.mozilla.org/show_bug.cgi?id=677166)

And, it looks they modify specification of network information API.

WebAPI/NetworkAPI
https://wiki.mozilla.org/WebAPI/NetworkAPI#Proposed_API

Mozilla suggests new functions to connection object instead of type function. (bandwidth and restricted).

And also, they add moz as prefix. I think mozilla supports this feature first because this specification is not finished.

For example,

navigator.mozConnection.bandwidth, navigator.mozConnection.restricted.

So, I think webkit can support this feature similar to mozilla.
For example, navigator.webkitConnection.bandwidth, navigator.webkitConnection.restricted.


How do you think about my suggestion ?

[Paul Irish]

As a developer, exposing a potential bandwidth throughput instead of connection type is a huge plus. That is massively more useful. :)

[Gyuyoung Kim]

Created attachment 119951 [details]
Patch

[Gyuyoung Kim]

As mentioned in comment #21, I add "webkit" prefix to connection object like *navigator.webkitConnection.type*. Because, this network information API spec is not finished.

Though I don't implement full feature this spec yet, I would like to know if partially implementation can be landed to trunk. Because, although this patch supports ethernet, wifi and cellular on Linux, current support can help web application. For now, EFL port only enables this feature.

[Kenneth Rohde Christiansen]

I am not sure that we want the OS implementations in WebKit itself. Qt, for instance, has a quite nice API for querying all these things and it works on devices where the OS implementation might not work. I guess you might need to make a simple Client for this just like DeviceOrientationClient etc. Then this can also be nicely sandboxed.

[Kenneth Rohde Christiansen]

Comment on attachment 119951 [details]
Patch

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

[Gyuyoung Kim]

(In reply to comment #25)
> I am not sure that we want the OS implementations in WebKit itself. Qt, for instance, has a quite nice API for querying all these things and it works on devices where the OS implementation might not work. I guess you might need to make a simple Client for this just like DeviceOrientationClient etc. Then this can also be nicely sandboxed.

I considered where place is better for concrete implementation. Ok, I'm going to submit new patch according to your comments. Thank you for your review. :-)

[Gyuyoung Kim]

Created attachment 120305 [details]
Patch

[Gyuyoung Kim]

I update patch according to kenneth's comments. And, I rename *Connection* class to *WebKitConnection* class in order to clear class role more. And also, I add dummy files for GTK and QT port, NetworkInfoClientGtk.cpp / NetworkInfoClientQt.cpp

[Gyuyoung Kim]

Created attachment 120309 [details]
Patch

[Kenneth Rohde Christiansen]

Comment on attachment 120309 [details]
Patch

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) {

}

[Gyuyoung Kim]

Created attachment 120447 [details]
Patch

[Gyuyoung Kim]

I modify this patch according to Kenneth's review comments. Thank you.

BTW, I add NetworkInfoClient to PageClient. But, I'm not sure if the NetworkInfoClient can be added to PageClient list. If you think there is better place, please let me know.

[Kenneth Rohde Christiansen]

Comment on attachment 120447 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=120447&action=review

Looks a lot nicer now

> Source/WebCore/page/Navigator.h:99
> +#if ENABLE(NET_INFO)

Maybe NETWORK_INFO would be a better name

> Source/WebCore/page/NetworkInfoConnection.cpp:52
> +    if (!m_frame)
> +        return String();

Can this happen? Why not ASSERT(frame) in the ctor?

> Source/WebCore/page/NetworkInfoConnection.h:45
> +    static PassRefPtr<NetworkInfoConnection> create(Frame* frame, NetworkInfoClient* client) { return adoptRef(new NetworkInfoConnection(frame, client)); }

Please write this as multiple lines

> Source/WebKit/efl/WebCoreSupport/NetworkInfoClientEfl.cpp:93
> +    // FIXME : Needs to check if network type is 2g, 3g or 4g.

no space before :

Isnt it called 2G and not 2g?

> Source/api.pri:170
> +        $$PWD/WebKit/qt/WebCoreSupport/NetworkInfoClientQt.h

What about the other ports? WebKit2 etc?

[Kenneth Rohde Christiansen]

(In reply to comment #33)
> I modify this patch according to Kenneth's review comments. Thank you.
> 
> BTW, I add NetworkInfoClient to PageClient. But, I'm not sure if the NetworkInfoClient can be added to PageClient list. If you think there is better place, please let me know.

It should be added to the Page constructor as all other clients

[Kenneth Rohde Christiansen]

(In reply to comment #35)
> (In reply to comment #33)
> > I modify this patch according to Kenneth's review comments. Thank you.
> > 
> > BTW, I add NetworkInfoClient to PageClient. But, I'm not sure if the NetworkInfoClient can be added to PageClient list. If you think there is better place, please let me know.
> 
> It should be added to the Page constructor as all other clients

Ah I misunderstood you. You added it to pageClients... that seems to be the way to do it today instead of using the ctor. PageClient is a special Client different from pageClients, hense the confusion

[Gyuyoung Kim]

(In reply to comment #34)
> (From update of attachment 120447 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=120447&action=review
> 
> Looks a lot nicer now
> 
> > Source/WebCore/page/Navigator.h:99
> > +#if ENABLE(NET_INFO)
> 
> Maybe NETWORK_INFO would be a better name
> 
> > Source/WebCore/page/NetworkInfoConnection.cpp:52
> > +    if (!m_frame)
> > +        return String();
> 
> Can this happen? Why not ASSERT(frame) in the ctor?
> 
> > Source/WebCore/page/NetworkInfoConnection.h:45
> > +    static PassRefPtr<NetworkInfoConnection> create(Frame* frame, NetworkInfoClient* client) { return adoptRef(new NetworkInfoConnection(frame, client)); }
> 
> Please write this as multiple lines
> 
> > Source/WebKit/efl/WebCoreSupport/NetworkInfoClientEfl.cpp:93
> > +    // FIXME : Needs to check if network type is 2g, 3g or 4g.
> 
> no space before :
> 
> Isnt it called 2G and not 2g?
> 
> > Source/api.pri:170
> > +        $$PWD/WebKit/qt/WebCoreSupport/NetworkInfoClientQt.h
> 
> What about the other ports? WebKit2 etc?

 I also would like to support other ports. Next patch will support more ports.

[WebKit Review Bot]

Comment on attachment 120447 [details]
Patch

Attachment 120447 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/11016301

New failing tests:
fast/dom/navigator-networkInfo.html

[Gyuyoung Kim]

Created attachment 120547 [details]
Patch

[Gyuyoung Kim]

I modify this patch again. And also, I add dummy files to WebKit2 as well.

[WebKit Review Bot]

Comment on attachment 120547 [details]
Patch

Attachment 120547 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/11037079

New failing tests:
fast/dom/navigator-networkInfo.html

[WebKit Review Bot]

Comment on attachment 120547 [details]
Patch

Attachment 120547 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/11038087

New failing tests:
fast/dom/navigator-networkInfo.html

[Gyuyoung Kim]

Created attachment 120573 [details]
Patch

[Gyuyoung Kim]

To fix layout test fail in chromium port, I add this test case to chromium's skip list. And, I add dummy files to mac port.

[Kenneth Rohde Christiansen]

Comment on attachment 120573 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=120573&action=review

> Source/WebKit/mac/WebCoreSupport/WebNetworkInfoClient.h:38
> +    WebNetworkInfoClient(WebView *);

Why does this ever need the WebView?

> Source/WebKit/mac/WebCoreSupport/WebNetworkInfoClient.h:39
> +    WebView *webView() { return m_webView; }

I really wonder why you added this. It doesn't seem to belong here and it doesnt even seem to be used and is thus dead code

> Source/WebKit2/WebProcess/WebCoreSupport/WebNetworkInfoClient.h:42
> +    ~WebNetworkInfoClient();

why is this not virtual? comment?

[Gyuyoung Kim]

Created attachment 120590 [details]
Patch

[Gyuyoung Kim]

(In reply to comment #45)
> (From update of attachment 120573 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=120573&action=review
> 
> > Source/WebKit/mac/WebCoreSupport/WebNetworkInfoClient.h:38
> > +    WebNetworkInfoClient(WebView *);
> 
> Why does this ever need the WebView?
> 
> > Source/WebKit/mac/WebCoreSupport/WebNetworkInfoClient.h:39
> > +    WebView *webView() { return m_webView; }
> 
> I really wonder why you added this. It doesn't seem to belong here and it doesnt even seem to be used and is thus dead code

I have misunderstanding. Though this patch doesn't need to have webview, I understood WebCoreSupport file of mac port needs to have webview. I remove it. 
 
> > Source/WebKit2/WebProcess/WebCoreSupport/WebNetworkInfoClient.h:42
> > +    ~WebNetworkInfoClient();
> 
> why is this not virtual? comment?

When I make NetworkInfoClient interface, I expect that all ports need to do same activities in destructor. But, now, it looks better that each port do something for itself. So, I remove virtual destructor from NetworkInforClient.h

[Gyuyoung Kim]

Created attachment 120728 [details]
Patch

[Gyuyoung Kim]

I add NetworkInfoClilentImpl files for chromium port.

[Jonathan Dixon]

Comment on attachment 120728 [details]
Patch

couple drive-by comments:

View in context: https://bugs.webkit.org/attachment.cgi?id=120728&action=review

> Source/WebCore/page/NetworkInfoClient.h:36
> +    virtual String networkDeviceType() const = 0;

I think it's normal to add a virtual destructor.

> Source/WebKit/gtk/webkit/webkitwebview.cpp:3300
> +    pageClients.networkInfoClient = static_cast<WebCore::NetworkInfoClient*>(new NetworkInfoClientGtk);

shouldn't be any need for the explicit cast, AFAICT

[Gyuyoung Kim]

(In reply to comment #50)
> (From update of attachment 120728 [details])
> couple drive-by comments:
> 
> View in context: https://bugs.webkit.org/attachment.cgi?id=120728&action=review
> 
> > Source/WebCore/page/NetworkInfoClient.h:36
> > +    virtual String networkDeviceType() const = 0;
> 
> I think it's normal to add a virtual destructor.

Yes, it looks it is better to add virtual destructor.

> > Source/WebKit/gtk/webkit/webkitwebview.cpp:3300
> > +    pageClients.networkInfoClient = static_cast<WebCore::NetworkInfoClient*>(new NetworkInfoClientGtk);
> 
> shouldn't be any need for the explicit cast, AFAICT

I think currently gtk port don't need explict casting. But, if NetworkInfoClientGtk adds additional functions in future, explicit casting can be needed.

[Gyuyoung Kim]

Created attachment 121619 [details]
Patch

[Adam Barth]

Comment on attachment 121619 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=121619&action=review

> Source/WebCore/page/NetworkInfoConnection.cpp:42
> +    ASSERT(!m_frame);

Isn't this ASSERT reading uninitialized memory?

> Source/WebCore/page/NetworkInfoConnection.cpp:43
> +    m_frame = frame;

What stops this m_frame point from becoming a dangling reference?

> Source/WebKit/efl/WebCoreSupport/NetworkInfoClientEfl.cpp:63
> +String NetworkInfoClientEfl::networkDeviceType() const

This all looks like code that should be in Platform layer rather than the WebKit layer.

[Gyuyoung Kim]

(In reply to comment #53)

> > Source/WebKit/efl/WebCoreSupport/NetworkInfoClientEfl.cpp:63
> > +String NetworkInfoClientEfl::networkDeviceType() const
> 
> This all looks like code that should be in Platform layer rather than the WebKit layer.

I implemented this patch on Platform layer first. But, as mentioned in Comment #25, some ports(e.g. QT) may have APIs regarding network device functions. So, I move this feature to WebKit layer again. Do you think this feature should be implemented on Platform layer ?

[Adam Barth]

> I implemented this patch on Platform layer first. But, as mentioned in Comment #25, some ports(e.g. QT) may have APIs regarding network device functions. So, I move this feature to WebKit layer again. Do you think this feature should be implemented on Platform layer ?

I'm sorry to make you move the code all around again, but conceptually it belongs in Platform (it's querying information about the underlying platform).  We have the PlatformSupport and Strategies mechanisms to bounce this out to higher level APIs if that's necessary for some ports (e.g., because of sandboxing).

[Gyuyoung Kim]

(In reply to comment #55)
> > I implemented this patch on Platform layer first. But, as mentioned in Comment #25, some ports(e.g. QT) may have APIs regarding network device functions. So, I move this feature to WebKit layer again. Do you think this feature should be implemented on Platform layer ?
> 
> I'm sorry to make you move the code all around again, but conceptually it belongs in Platform (it's querying information about the underlying platform).  We have the PlatformSupport and Strategies mechanisms to bounce this out to higher level APIs if that's necessary for some ports (e.g., because of sandboxing).

Ok, I move this to Platform layer again. :-)

[Kenneth Rohde Christiansen]

(In reply to comment #55)
> > I implemented this patch on Platform layer first. But, as mentioned in Comment #25, some ports(e.g. QT) may have APIs regarding network device functions. So, I move this feature to WebKit layer again. Do you think this feature should be implemented on Platform layer ?
> 
> I'm sorry to make you move the code all around again, but conceptually it belongs in Platform (it's querying information about the underlying platform).  We have the PlatformSupport and Strategies mechanisms to bounce this out to higher level APIs if that's necessary for some ports (e.g., because of sandboxing).

I actually wonder why you cannot listen to changes, because on mobile phones the network usually changes a lot, like from 2g to 3g etc...happens all the time when I am taking the train. 

If that will be possible in some spec update, this is not that different from say geolocation, which also makes me wonder why I as a user does not have to grand access to this infomation.

[Adam Barth]

I'm not sure I fully understand your comment, but it does seem analogous to Geolocation.  I guess I don't have a strong preference as to whether this goes though Platform or Client.

Really, mostly I care about getting the memory safety bugs fixed!

[Gyuyoung Kim]

(In reply to comment #57) 
> I actually wonder why you cannot listen to changes, because on mobile phones the network usually changes a lot, like from 2g to 3g etc...happens all the time when I am taking the train. 

I also have considered how to control network change. But, current patch is huge, so I wanted to support it after landing this patch. If concrete implementation is moved to Platform layer, it looks I can support it to this patch.

Mozilla supports this as well.
https://wiki.mozilla.org/WebAPI/NetworkAPI#Proposed_API

> If that will be possible in some spec update, this is not that different from say geolocation, which also makes me wonder why I as a user does not have to grand access to this infomation.

I'm sorry I don't understand what your point. Do you mean this patch can similar to Geolocation implementation ?

[Kenneth Rohde Christiansen]

Comment on attachment 121619 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=121619&action=review

> Source/WebCore/page/Navigator.cpp:176
> +    if (!m_connection)
> +        m_connection = NetworkInfoConnection::create(m_frame, page->networkInfoClient());

Why is it a RefPtr when you use it as it was an OwnPtr? and why is m_connection mutable?

Who else can ref it? I actually wonder the same for geolocation. Maybe I am missing something here.

>> Source/WebCore/page/NetworkInfoConnection.cpp:43
>> +    m_frame = frame;
> 
> What stops this m_frame point from becoming a dangling reference?

Actually what are you using the frame pointer for? It only makes sense adding it when/if you implement permission support.

Btw, geolocation has a disconnectFrame(); for this which is called from Navigator.

[Adam Barth]

> Btw, geolocation has a disconnectFrame(); for this which is called from Navigator.

Not anymore.  Now it inherits from DOMWindowProperty, which handles the disconnection automatically.

[Kenneth Rohde Christiansen]

Btw, this spec doesnt seem very stable judging from the mailing lists. 

Apparently Firefox is not implementing the type() method which you are implementing here! https://wiki.mozilla.org/WebAPI/NetworkAPI#Proposed_API

You should really subscribe to the mailing list and follow this thread:

http://lists.w3.org/Archives/Public/public-device-status/2011Dec/0021.html

[Gyuyoung Kim]

(In reply to comment #62)
> Btw, this spec doesnt seem very stable judging from the mailing lists. 
> 
> Apparently Firefox is not implementing the type() method which you are implementing here! https://wiki.mozilla.org/WebAPI/NetworkAPI#Proposed_API
> 
> You should really subscribe to the mailing list and follow this thread:
> 
> http://lists.w3.org/Archives/Public/public-device-status/2011Dec/0021.html

As discussed on IRC, I participate in this thread. Then, let's discuss again.

[Gyuyoung Kim]

Created attachment 133830 [details]
Patch

[Gyuyoung Kim]

I submit updated patch based on modularization and Internals testing framework.

In addition, this patch is to support new editor draft version. New editor draft  was suggested by Mozilla. They already implemented this feature based on below editor draft.

http://dvcs.w3.org/hg/dap/raw-file/tip/network-api/index.html

I also notified to device API working group that I would like to implement this feature based on new draft because I couldn't find any cons in new draft. However, there is an argument for new draft still. It looks some operators want to support ".type" attribute. But, it is not decided yet. I'm almost sure there is no big change in the near future.

[WebKit Review Bot]

Comment on attachment 133830 [details]
Patch

Attachment 133830 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/12130976

New failing tests:
networkinformation/updates.html
networkinformation/window-property.html
networkinformation/basic-all-types-of-events.html
networkinformation/event-after-navigation.html
networkinformation/basic-operation.html
networkinformation/multiple-frames.html

[WebKit Review Bot]

Created attachment 133842 [details]
Archive of layout-test-results from ec2-cr-linux-04

The attached test failures were seen while running run-webkit-tests on the chromium-ews.
Bot: ec2-cr-linux-04  Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'>  Platform: Linux-2.6.35-28-virtual-x86_64-with-Ubuntu-10.10-maverick

[Gyuyoung Kim]

Created attachment 133851 [details]
Patch

[Build Bot]

Comment on attachment 133851 [details]
Patch

Attachment 133851 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/12145002

[Gyuyoung Kim]

Created attachment 133963 [details]
Patch

[Adam Barth]

Comment on attachment 133963 [details]
Patch

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.

[Gyuyoung Kim]

Created attachment 134298 [details]
Patch

[Gyuyoung Kim]

(In reply to comment #71)
> (From update of attachment 133963 [details])
> 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.

Some unneeded codes were added to this patch when modularization and internals testing were adjusted to previous patch. I'm sorry. I revise this patch again.

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

BatteryStatus added above line to Navigator idl file as well. So, it seems it gave a confusion to me. Anyway, it is not needed for this patch now.
http://trac.webkit.org/browser/trunk/Source/WebCore/Modules/battery/NavigatorBattery.idl#L23

[Adam Barth]

Sorry, I must have missed that in the battery-status patch.  Would you be willing to fix it there so that it doesn't confuse other folks in the future?

[Gyuyoung Kim]

(In reply to comment #74)
> Sorry, I must have missed that in the battery-status patch.  Would you be willing to fix it there so that it doesn't confuse other folks in the future?

Sure, I file a bug for this. Could you review it ?

Bug 82556 - Remove ScriptExecutionContext from NavigatorBattery.idl
https://bugs.webkit.org/show_bug.cgi?id=73528

[Gyuyoung Kim]

Created attachment 134497 [details]
Patch

[Gyuyoung Kim]

explicit keyword is added to construct as well.

[Adam Barth]

Comment on attachment 134497 [details]
Patch

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?

[Adam Barth]

Thanks for iterating on this patch.  My only (slight) worry is about re-using the Event object, but you might well be right about how that works.  In any case, we should add a test to confirm.

[Adam Barth]

Note: The test can be in a follow-up patch, if you like.

[Gyuyoung Kim]

(In reply to comment #80)
> Note: The test can be in a follow-up patch, if you like.

Thank you for your review again. As you know, this bug thread is too long as you can see. So, I would file a bug for your worry. I will file a bug soon. Thank you again.

[WebKit Review Bot]

Comment on attachment 134497 [details]
Patch

Clearing flags on attachment: 134497

Committed r112815: <http://trac.webkit.org/changeset/112815>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.