Bug 92343

Created attachment 154559 [details] Patch

Comment on attachment 154559 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=154559&action=review The metered() is going to be supported after investigating a little further. I think this is good approach to support both WK1 and WK2. However, Kenneth objected to implement this feature to WebCore because functions(bandwidth and metered) may need to interact with user. If Kenneth reviewed this, I don't object to land this as well. > Source/WebCore/platform/efl/NetworkInfoProviderEfl.cpp:2 > + * Copyright (C) 2012 Intel Corporation. All rights reserved. Because concrete implementation comes from Samsung, you should keep Samsung copyright. > Source/WebCore/platform/efl/NetworkInfoProviderEfl.h:2 > + * Copyright (C) 2012 Intel Corporation. All rights reserved. ditto. > Source/WebKit2/UIProcess/API/efl/NetworkInfoProvider.cpp:92 > + It looks this is unneeded line.

Comment on attachment 154559 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=154559&action=review >> Source/WebCore/platform/efl/NetworkInfoProviderEfl.cpp:2 >> + * Copyright (C) 2012 Intel Corporation. All rights reserved. > > Because concrete implementation comes from Samsung, you should keep Samsung copyright. Yes, indeed, my bad. >> Source/WebKit2/UIProcess/API/efl/NetworkInfoProvider.cpp:92 >> + > > It looks this is unneeded line. I like having blank line before a return.

Created attachment 154571 [details] Patch Fixed the copyright. Sorry about that.

Looks fine. Let's wait for Kenneth comment.

Created attachment 154666 [details] Patch WebKit EFL no longer needs to link against EEZE library since the code was moved to WebCore (which already links against EEZE due to Gamepad support).

Comment on attachment 154666 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=154666&action=review r=me as you are basically just moving this around > Source/WebCore/platform/efl/NetworkInfoProviderEfl.cpp:59 > + // FIXME : This function should consider cellular network as well. For example, 2G, 3G and 4G. > + // See https://bugs.webkit.org/show_bug.cgi?id=89851 for detail. > + Eeze_Net* ethNet = eeze_net_new(ethernetInterface); didn't the spec change so that is not necessary anymore? The bug report says almost nothing > Source/WebCore/platform/efl/NetworkInfoProviderEfl.cpp:61 > + if (!ethNet) > + return 0; so 0 is unknown? What does the spec say?

Comment on attachment 154666 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=154666&action=review >> Source/WebCore/platform/efl/NetworkInfoProviderEfl.cpp:59 >> + Eeze_Net* ethNet = eeze_net_new(ethernetInterface); > > didn't the spec change so that is not necessary anymore? The bug report says almost nothing I'm not sure what you're referring to. Gyuyoung probably knows more since he implemented this. Based on the latest spec, we should report the bandwidth of the connection currently being used. Currently we hardcode "eth0" which is of course not a good idea since eth0 may not be the interface that is used. I think this is what the comment is about. >> Source/WebCore/platform/efl/NetworkInfoProviderEfl.cpp:61 >> + return 0; > > so 0 is unknown? What does the spec say? Spec says 0 is offline and infinity is unknown. I will switch to infinity then.

Created attachment 154676 [details] Patch for landing Properly return infinity when the bandwidth is unknown, instead of 0, as per the spec.

I was referring to the comment. Maybe Gyuyoung can answer that.

Comment on attachment 154676 [details] Patch for landing Are there any tests for this (for WebKit1) that tests the infinity thing?

Comment on attachment 154676 [details] Patch for landing Clearing flags on attachment: 154676 Committed r123769: <http://trac.webkit.org/changeset/123769>

All reviewed patches have been landed. Closing bug.

Existing implementation is partial implementation because EFL library doesn't support full feature for this specification. When I submitted this patch, I said this is initial patch. I will update this implementation after discussing with EFL developers. I'm finding how to support cellular network based on EFL library, how to support metered option, how to support network is changed well. Please wait for my updated patch.

(In reply to comment #7) > (From update of attachment 154666 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=154666&action=review > > r=me as you are basically just moving this around > > > Source/WebCore/platform/efl/NetworkInfoProviderEfl.cpp:59 > > + // FIXME : This function should consider cellular network as well. For example, 2G, 3G and 4G. > > + // See https://bugs.webkit.org/show_bug.cgi?id=89851 for detail. > > + Eeze_Net* ethNet = eeze_net_new(ethernetInterface); > > didn't the spec change so that is not necessary anymore? The bug report says almost nothing I implemented this feature based on latest editor draft. http://dev.w3.org/2009/dap/netinfo/ The editor draft is not interested in network type. In EFL case, however, eeze library supports network information. In order to get network information via eeze, I needed to distinguish between ethernet and wireless network. For now, eeze doesn't support wireless network. So, I used ethNet variable name. I discussed this issue with EFL developers. But, I didn't find proper solution yet. I'm going to update patch as soon as I found it. By the way, if eeze_net_new returns NULL, I thought network is offline. Spec say *infinity* if the bandwidth is unknown and *0* if the user is currently offline; if (!ethNet) return std::numeric_limits<double>::infinity(); // Spec says we should return infinity if unknown bandwidth is being checked below code. double bandwidth; const char* attribute = eeze_net_attribute_get(ethNet, "speed"); if (attribute) { bool ok; bandwidth = String::fromUTF8(attribute).toUIntStrict(&ok); } else bandwidth = std::numeric_limits<double>::infinity(); // If bandwidth is unknown, return infinity value. So' I would like to revert this modification on Bug 92454 for now.