Bug 92343 – [EFL][WK2] Implement Network Information provider

• Component: WebKit EFL
• Status: RESOLVED FIXED

[Chris Dumez]

We need to provide a Network Information provider for WebKit2 EFL and try to share as much code as possible with WebKit1 for this.

[Chris Dumez]

Created attachment 154559 [details]
Patch

[Gyuyoung Kim]

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.

[Chris Dumez]

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.

[Chris Dumez]

Created attachment 154571 [details]
Patch

Fixed the copyright. Sorry about that.

[Gyuyoung Kim]

Looks fine. Let's wait for Kenneth comment.

[Chris Dumez]

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

[Kenneth Rohde Christiansen]

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?

[Chris Dumez]

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.

[Chris Dumez]

Created attachment 154676 [details]
Patch for landing

Properly return infinity when the bandwidth is unknown, instead of 0, as per the spec.

[Kenneth Rohde Christiansen]

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

[Kenneth Rohde Christiansen]

Comment on attachment 154676 [details]
Patch for landing

Are there any tests for this (for WebKit1) that tests the infinity thing?

[WebKit Review Bot]

Comment on attachment 154676 [details]
Patch for landing

Clearing flags on attachment: 154676

Committed r123769: <http://trac.webkit.org/changeset/123769>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Gyuyoung Kim]

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.

[Gyuyoung Kim]

(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.