Bug 73528 - (NetInfoAPI) Support the Network Information API
(NetInfoAPI)
: Support the Network Information API
Status: RESOLVED FIXED
: WebKit
Platform
: 528+ (Nightly build)
: All All
: P2 Enhancement
Assigned To:
: http://www.w3.org/TR/2011/WD-netinfo-...
:
: 131841
: 78947
  Show dependency treegraph
 
Reported: 2011-11-30 23:21 PST by
Modified: 2014-04-18 03:49 PST (History)


Attachments
Patch (22.78 KB, patch)
2011-11-30 23:28 PST, Gyuyoung Kim
webkit-ews: commit‑queue-
Review Patch | Details | Formatted Diff | Diff
Prototype - 2 (22.32 KB, patch)
2011-12-04 23:03 PST, Gyuyoung Kim
gyuyoung.kim: commit‑queue-
Review Patch | Details | Formatted Diff | Diff
Prototype - 3 (14.78 KB, patch)
2011-12-06 06:48 PST, Gyuyoung Kim
gyuyoung.kim: commit‑queue-
Review Patch | Details | Formatted Diff | Diff
Prototype - 3 (25.82 KB, patch)
2011-12-06 20:06 PST, Gyuyoung Kim
gyuyoung.kim: commit‑queue-
Review Patch | Details | Formatted Diff | Diff
Prototype - 4 (28.88 KB, patch)
2011-12-08 03:01 PST, Gyuyoung Kim
gyuyoung.kim: commit‑queue-
Review Patch | Details | Formatted Diff | Diff
Prototype - 5 (29.62 KB, patch)
2011-12-13 01:44 PST, Gyuyoung Kim
no flags Review Patch | Details | Formatted Diff | Diff
Patch (30.30 KB, patch)
2011-12-19 16:50 PST, Gyuyoung Kim
no flags Review Patch | Details | Formatted Diff | Diff
Patch (33.50 KB, patch)
2011-12-22 04:45 PST, Gyuyoung Kim
no flags Review Patch | Details | Formatted Diff | Diff
Patch (33.10 KB, patch)
2011-12-22 05:03 PST, Gyuyoung Kim
no flags Review Patch | Details | Formatted Diff | Diff
Patch (46.58 KB, patch)
2011-12-23 02:49 PST, Gyuyoung Kim
no flags Review Patch | Details | Formatted Diff | Diff
Patch (53.75 KB, patch)
2011-12-26 04:15 PST, Gyuyoung Kim
no flags Review Patch | Details | Formatted Diff | Diff
Patch (70.42 KB, patch)
2011-12-27 01:35 PST, Gyuyoung Kim
no flags Review Patch | Details | Formatted Diff | Diff
Patch (78.85 KB, patch)
2011-12-27 07:38 PST, Gyuyoung Kim
no flags Review Patch | Details | Formatted Diff | Diff
Patch (87.20 KB, patch)
2011-12-29 03:13 PST, Gyuyoung Kim
no flags Review Patch | Details | Formatted Diff | Diff
Patch (86.49 KB, patch)
2012-01-08 21:59 PST, Gyuyoung Kim
no flags Review Patch | Details | Formatted Diff | Diff
Patch (75.69 KB, patch)
2012-03-26 09:28 PST, Gyuyoung Kim
no flags Review Patch | Details | Formatted Diff | Diff
Archive of layout-test-results from ec2-cr-linux-04 (9.88 MB, application/zip)
2012-03-26 10:28 PST, WebKit Review Bot
no flags Details
Patch (78.10 KB, patch)
2012-03-26 11:16 PST, Gyuyoung Kim
no flags Review Patch | Details | Formatted Diff | Diff
Patch (78.14 KB, patch)
2012-03-26 19:49 PST, Gyuyoung Kim
no flags Review Patch | Details | Formatted Diff | Diff
Patch (75.02 KB, patch)
2012-03-28 08:10 PST, Gyuyoung Kim
no flags Review Patch | Details | Formatted Diff | Diff
Patch (75.09 KB, patch)
2012-03-28 22:12 PST, Gyuyoung Kim
no flags Review Patch | Details | Formatted Diff | Diff


Note

You need to log in before you can comment on or make changes to this bug.


Description From 2011-11-30 23:21:02 PST
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.
------- Comment #1 From 2011-11-30 23:28:01 PST -------
Created an attachment (id=117352) [details]
Patch
------- Comment #2 From 2011-11-30 23:42:07 PST -------
(From update of attachment 117352 [details])
Attachment 117352 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/10704519
------- Comment #3 From 2011-12-01 00:20:24 PST -------
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.
------- Comment #4 From 2011-12-01 00:49:27 PST -------
Also, all new features need an email to webkit-dev first. http://www.webkit.org/coding/adding-features.html
------- Comment #5 From 2011-12-01 01:12:48 PST -------
(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.
------- Comment #6 From 2011-12-01 01:30:36 PST -------
(From update of attachment 117352 [details])
Attachment 117352 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/10708011
------- Comment #7 From 2011-12-01 01:48:15 PST -------
(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.
------- Comment #8 From 2011-12-01 11:38:46 PST -------
(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.
------- Comment #9 From 2011-12-01 13:03:29 PST -------
(From update of attachment 117352 [details])
Attachment 117352 [details] did not pass gtk-ews (gtk):
Output: http://queues.webkit.org/results/10690239
------- Comment #10 From 2011-12-01 20:36:14 PST -------
(In reply to comment #8)
> (From update of attachment 117352 [details] [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.
------- Comment #11 From 2011-12-04 23:03:18 PST -------
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 ? 

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 ?
------- Comment #12 From 2011-12-04 23:08:51 PST -------
(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.

> 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.
------- Comment #13 From 2011-12-05 00:02:54 PST -------
(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.
------- Comment #14 From 2011-12-05 00:06:43 PST -------
(In reply to comment #13)
> (In reply to comment #12)
> > (In reply to comment #11)
> > > Created an attachment (id=117847) [details] [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.
------- Comment #15 From 2011-12-05 05:50:44 PST -------
(From update of attachment 117847 [details])
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
------- Comment #16 From 2011-12-06 06:48:52 PST -------
Created an attachment (id=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.
------- Comment #17 From 2011-12-06 20:06:39 PST -------
Created an attachment (id=118164) [details]
Prototype - 3

Some files were missed in previous patch. And, I add new file - NetworkInformation.cpp | h for this feature.
------- Comment #18 From 2011-12-08 03:01:20 PST -------
Created an attachment (id=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.
------- Comment #19 From 2011-12-13 01:44:48 PST -------
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.
------- Comment #20 From 2011-12-13 04:22:20 PST -------
(In reply to comment #19)
> Created an attachment (id=118979) [details] [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).
------- Comment #21 From 2011-12-14 22:55:34 PST -------
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 ?
------- Comment #22 From 2011-12-15 10:37:42 PST -------
As a developer, exposing a potential bandwidth throughput instead of connection type is a huge plus. That is massively more useful. :)
------- Comment #23 From 2011-12-19 16:50:59 PST -------
Created an attachment (id=119951) [details]
Patch
------- Comment #24 From 2011-12-19 16:59:36 PST -------
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.
------- Comment #25 From 2011-12-20 01:29:05 PST -------
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.
------- Comment #26 From 2011-12-20 01:36:00 PST -------
(From update of attachment 119951 [details])
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
------- Comment #27 From 2011-12-20 06:21:51 PST -------
(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. :-)
------- Comment #28 From 2011-12-22 04:45:46 PST -------
Created an attachment (id=120305) [details]
Patch
------- Comment #29 From 2011-12-22 04:49:08 PST -------
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
------- Comment #30 From 2011-12-22 05:03:24 PST -------
Created an attachment (id=120309) [details]
Patch
------- Comment #31 From 2011-12-22 05:49:23 PST -------
(From update of attachment 120309 [details])
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) {

}
------- Comment #32 From 2011-12-23 02:49:22 PST -------
Created an attachment (id=120447) [details]
Patch
------- Comment #33 From 2011-12-23 02:51:47 PST -------
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.
------- Comment #34 From 2011-12-23 02:59:25 PST -------
(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?
------- Comment #35 From 2011-12-23 03:00:43 PST -------
(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
------- Comment #36 From 2011-12-23 03:17:26 PST -------
(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
------- Comment #37 From 2011-12-23 03:23:42 PST -------
(In reply to comment #34)
> (From update of attachment 120447 [details] [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.
------- Comment #38 From 2011-12-23 06:02:39 PST -------
(From update of attachment 120447 [details])
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
------- Comment #39 From 2011-12-26 04:15:25 PST -------
Created an attachment (id=120547) [details]
Patch
------- Comment #40 From 2011-12-26 04:16:09 PST -------
I modify this patch again. And also, I add dummy files to WebKit2 as well.
------- Comment #41 From 2011-12-26 05:29:19 PST -------
(From update of attachment 120547 [details])
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
------- Comment #42 From 2011-12-26 06:20:46 PST -------
(From update of attachment 120547 [details])
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
------- Comment #43 From 2011-12-27 01:35:28 PST -------
Created an attachment (id=120573) [details]
Patch
------- Comment #44 From 2011-12-27 01:37:01 PST -------
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.
------- Comment #45 From 2011-12-27 03:06:46 PST -------
(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

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

why is this not virtual? comment?
------- Comment #46 From 2011-12-27 07:38:34 PST -------
Created an attachment (id=120590) [details]
Patch
------- Comment #47 From 2011-12-27 07:44:58 PST -------
(In reply to comment #45)
> (From update of attachment 120573 [details] [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
------- Comment #48 From 2011-12-29 03:13:49 PST -------
Created an attachment (id=120728) [details]
Patch
------- Comment #49 From 2011-12-29 03:15:44 PST -------
I add NetworkInfoClilentImpl files for chromium port.
------- Comment #50 From 2012-01-06 05:19:46 PST -------
(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.

> 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
------- Comment #51 From 2012-01-06 08:43:13 PST -------
(In reply to comment #50)
> (From update of attachment 120728 [details] [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.
------- Comment #52 From 2012-01-08 21:59:00 PST -------
Created an attachment (id=121619) [details]
Patch
------- Comment #53 From 2012-01-08 22:30:33 PST -------
(From update of attachment 121619 [details])
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.
------- Comment #54 From 2012-01-08 23:00:16 PST -------
(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 ?
------- Comment #55 From 2012-01-08 23:04:47 PST -------
> 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).
------- Comment #56 From 2012-01-08 23:20:55 PST -------
(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. :-)
------- Comment #57 From 2012-01-09 01:38:04 PST -------
(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.
------- Comment #58 From 2012-01-09 02:01:51 PST -------
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!
------- Comment #59 From 2012-01-09 02:14:54 PST -------
(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 ?
------- Comment #60 From 2012-01-09 02:24:25 PST -------
(From update of attachment 121619 [details])
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.
------- Comment #61 From 2012-01-09 02:31:29 PST -------
> Btw, geolocation has a disconnectFrame(); for this which is called from Navigator.

Not anymore.  Now it inherits from DOMWindowProperty, which handles the disconnection automatically.
------- Comment #62 From 2012-01-09 02:46:30 PST -------
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
------- Comment #63 From 2012-01-09 03:00:48 PST -------
(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.
------- Comment #64 From 2012-03-26 09:28:50 PST -------
Created an attachment (id=133830) [details]
Patch
------- Comment #65 From 2012-03-26 09:42:49 PST -------
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.
------- Comment #66 From 2012-03-26 10:28:24 PST -------
(From update of attachment 133830 [details])
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
------- Comment #67 From 2012-03-26 10:28:35 PST -------
Created an attachment (id=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
------- Comment #68 From 2012-03-26 11:16:28 PST -------
Created an attachment (id=133851) [details]
Patch
------- Comment #69 From 2012-03-26 12:56:29 PST -------
(From update of attachment 133851 [details])
Attachment 133851 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/12145002
------- Comment #70 From 2012-03-26 19:49:14 PST -------
Created an attachment (id=133963) [details]
Patch
------- Comment #71 From 2012-03-27 15:47:25 PST -------
(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.

> 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.
------- Comment #72 From 2012-03-28 08:10:52 PST -------
Created an attachment (id=134298) [details]
Patch
------- Comment #73 From 2012-03-28 08:20:23 PST -------
(In reply to comment #71)
> (From update of attachment 133963 [details] [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
------- Comment #74 From 2012-03-28 11:00:16 PST -------
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?
------- Comment #75 From 2012-03-28 18:10:01 PST -------
(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
------- Comment #76 From 2012-03-28 22:12:31 PST -------
Created an attachment (id=134497) [details]
Patch
------- Comment #77 From 2012-03-28 22:14:09 PST -------
explicit keyword is added to construct as well.
------- Comment #78 From 2012-03-31 17:45:45 PST -------
(From update of attachment 134497 [details])
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?
------- Comment #79 From 2012-03-31 17:46:48 PST -------
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.
------- Comment #80 From 2012-03-31 17:47:07 PST -------
Note: The test can be in a follow-up patch, if you like.
------- Comment #81 From 2012-04-01 07:15:28 PST -------
(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.
------- Comment #82 From 2012-04-01 08:09:45 PST -------
(From update of attachment 134497 [details])
Clearing flags on attachment: 134497

Committed r112815: <http://trac.webkit.org/changeset/112815>
------- Comment #83 From 2012-04-01 08:10:10 PST -------
All reviewed patches have been landed.  Closing bug.