Bug 73528 (NetInfoAPI) - Support the Network Information API
Summary: Support the Network Information API
Status: RESOLVED FIXED
Alias: NetInfoAPI
Product: WebKit
Classification: Unclassified
Component: Platform (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Enhancement
Assignee: Gyuyoung Kim
URL: http://www.w3.org/TR/2011/WD-netinfo-...
Keywords:
Depends on: 131841
Blocks: DAP
  Show dependency treegraph
 
Reported: 2011-11-30 23:21 PST by Gyuyoung Kim
Modified: 2014-04-18 03:49 PDT (History)
31 users (show)

See Also:


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

Note You need to log in before you can comment on or make changes to this bug.
Description Gyuyoung Kim 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 Gyuyoung Kim 2011-11-30 23:28:01 PST
Created attachment 117352 [details]
Patch
Comment 2 Early Warning System Bot 2011-11-30 23:42:07 PST
Comment on attachment 117352 [details]
Patch

Attachment 117352 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/10704519
Comment 3 Mark Rowe (bdash) 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 Ojan Vafai 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 Gyuyoung Kim 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 WebKit Review Bot 2011-12-01 01:30:36 PST
Comment on attachment 117352 [details]
Patch

Attachment 117352 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/10708011
Comment 7 Gyuyoung Kim 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 Adam Barth 2011-12-01 11:38:46 PST
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.
Comment 9 Gustavo Noronha (kov) 2011-12-01 13:03:29 PST
Comment on attachment 117352 [details]
Patch

Attachment 117352 [details] did not pass gtk-ews (gtk):
Output: http://queues.webkit.org/results/10690239
Comment 10 Gyuyoung Kim 2011-12-01 20:36:14 PST
(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.
Comment 11 Gyuyoung Kim 2011-12-04 23:03:18 PST
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 ?
Comment 12 Mark Rowe (bdash) 2011-12-04 23:08:51 PST
(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.
Comment 13 Gyuyoung Kim 2011-12-05 00:02:54 PST
(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.
Comment 14 Mark Rowe (bdash) 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]
> > > 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 John Knottenbelt 2011-12-05 05:50:44 PST
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
Comment 16 Gyuyoung Kim 2011-12-06 06:48:52 PST
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.
Comment 17 Gyuyoung Kim 2011-12-06 20:06:39 PST
Created attachment 118164 [details]
Prototype - 3

Some files were missed in previous patch. And, I add new file - NetworkInformation.cpp | h for this feature.
Comment 18 Gyuyoung Kim 2011-12-08 03:01:20 PST
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.
Comment 19 Gyuyoung Kim 2011-12-13 01:44:48 PST
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.
Comment 20 Raphael Kubo da Costa (:rakuco) 2011-12-13 04:22:20 PST
(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).
Comment 21 Gyuyoung Kim 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 Paul Irish 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 Gyuyoung Kim 2011-12-19 16:50:59 PST
Created attachment 119951 [details]
Patch
Comment 24 Gyuyoung Kim 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 Kenneth Rohde Christiansen 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 Kenneth Rohde Christiansen 2011-12-20 01:36:00 PST
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
Comment 27 Gyuyoung Kim 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 Gyuyoung Kim 2011-12-22 04:45:46 PST
Created attachment 120305 [details]
Patch
Comment 29 Gyuyoung Kim 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 Gyuyoung Kim 2011-12-22 05:03:24 PST
Created attachment 120309 [details]
Patch
Comment 31 Kenneth Rohde Christiansen 2011-12-22 05:49:23 PST
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) {

}
Comment 32 Gyuyoung Kim 2011-12-23 02:49:22 PST
Created attachment 120447 [details]
Patch
Comment 33 Gyuyoung Kim 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 Kenneth Rohde Christiansen 2011-12-23 02:59:25 PST
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?
Comment 35 Kenneth Rohde Christiansen 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 Kenneth Rohde Christiansen 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 Gyuyoung Kim 2011-12-23 03:23:42 PST
(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.
Comment 38 WebKit Review Bot 2011-12-23 06:02:39 PST
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
Comment 39 Gyuyoung Kim 2011-12-26 04:15:25 PST
Created attachment 120547 [details]
Patch
Comment 40 Gyuyoung Kim 2011-12-26 04:16:09 PST
I modify this patch again. And also, I add dummy files to WebKit2 as well.
Comment 41 WebKit Review Bot 2011-12-26 05:29:19 PST
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
Comment 42 WebKit Review Bot 2011-12-26 06:20:46 PST
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
Comment 43 Gyuyoung Kim 2011-12-27 01:35:28 PST
Created attachment 120573 [details]
Patch
Comment 44 Gyuyoung Kim 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 Kenneth Rohde Christiansen 2011-12-27 03:06:46 PST
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?
Comment 46 Gyuyoung Kim 2011-12-27 07:38:34 PST
Created attachment 120590 [details]
Patch
Comment 47 Gyuyoung Kim 2011-12-27 07:44:58 PST
(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
Comment 48 Gyuyoung Kim 2011-12-29 03:13:49 PST
Created attachment 120728 [details]
Patch
Comment 49 Gyuyoung Kim 2011-12-29 03:15:44 PST
I add NetworkInfoClilentImpl files for chromium port.
Comment 50 Jonathan Dixon 2012-01-06 05:19:46 PST
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
Comment 51 Gyuyoung Kim 2012-01-06 08:43:13 PST
(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.
Comment 52 Gyuyoung Kim 2012-01-08 21:59:00 PST
Created attachment 121619 [details]
Patch
Comment 53 Adam Barth 2012-01-08 22:30:33 PST
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.
Comment 54 Gyuyoung Kim 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 Adam Barth 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 Gyuyoung Kim 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 Kenneth Rohde Christiansen 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 Adam Barth 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 Gyuyoung Kim 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 Kenneth Rohde Christiansen 2012-01-09 02:24:25 PST
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.
Comment 61 Adam Barth 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 Kenneth Rohde Christiansen 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 Gyuyoung Kim 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 Gyuyoung Kim 2012-03-26 09:28:50 PDT
Created attachment 133830 [details]
Patch
Comment 65 Gyuyoung Kim 2012-03-26 09:42:49 PDT
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 WebKit Review Bot 2012-03-26 10:28:24 PDT
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
Comment 67 WebKit Review Bot 2012-03-26 10:28:35 PDT
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
Comment 68 Gyuyoung Kim 2012-03-26 11:16:28 PDT
Created attachment 133851 [details]
Patch
Comment 69 Build Bot 2012-03-26 12:56:29 PDT
Comment on attachment 133851 [details]
Patch

Attachment 133851 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/12145002
Comment 70 Gyuyoung Kim 2012-03-26 19:49:14 PDT
Created attachment 133963 [details]
Patch
Comment 71 Adam Barth 2012-03-27 15:47:25 PDT
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.
Comment 72 Gyuyoung Kim 2012-03-28 08:10:52 PDT
Created attachment 134298 [details]
Patch
Comment 73 Gyuyoung Kim 2012-03-28 08:20:23 PDT
(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
Comment 74 Adam Barth 2012-03-28 11:00:16 PDT
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 Gyuyoung Kim 2012-03-28 18:10:01 PDT
(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 Gyuyoung Kim 2012-03-28 22:12:31 PDT
Created attachment 134497 [details]
Patch
Comment 77 Gyuyoung Kim 2012-03-28 22:14:09 PDT
explicit keyword is added to construct as well.
Comment 78 Adam Barth 2012-03-31 17:45:45 PDT
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?
Comment 79 Adam Barth 2012-03-31 17:46:48 PDT
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 Adam Barth 2012-03-31 17:47:07 PDT
Note: The test can be in a follow-up patch, if you like.
Comment 81 Gyuyoung Kim 2012-04-01 07:15:28 PDT
(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 WebKit Review Bot 2012-04-01 08:09:45 PDT
Comment on attachment 134497 [details]
Patch

Clearing flags on attachment: 134497

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