Bug 87067 – [EFL] Implement Network Information API

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKit EFL

[Gyuyoung Kim]

Patch will be submitted within 1~2 weeks.

[Gyuyoung Kim]

Created attachment 144993 [details]
Prototype patch

This patch is just prototype. There is still crash issue and needs to implement network info spec. further.

[Gyuyoung Kim]

Created attachment 145763 [details]
Prototype patch - 2

[Gyuyoung Kim]

Created attachment 146175 [details]
Patch

[Gyuyoung Kim]

I implement bandwidth() function first. And also, bandwidth() supports ethernet network for now. I need to investigate how to support cellular network and meter() function. It seems bandwidth() needs to be landed first.

[Gyuyoung Kim]

Created attachment 146176 [details]
Patch

[Gyuyoung Kim]

Comment on attachment 146176 [details]
Patch

Attachment 146176 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/12910181

[Gyuyoung Kim]

(In reply to comment #6)
> (From update of attachment 146176 [details])
> Attachment 146176 [details] did not pass efl-ews (efl):
> Output: http://queues.webkit.org/results/12910181

This patch includes eeze dependency related to jhbuild. When this patch is landed, this patch can be passed. I don't wanna install eeze library to eflews unnecessarily.

[Gyuyoung Kim]

Created attachment 146457 [details]
Patch

[Gyuyoung Kim]

Comment on attachment 146457 [details]
Patch

Attachment 146457 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/12924198

[Gyuyoung Kim]

Created attachment 146493 [details]
Patch

[Gyuyoung Kim]

Comment on attachment 146493 [details]
Patch

Attachment 146493 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/12924287

[Gyuyoung Kim]

Kubo, could you review this patch informally ?

[Chris Dumez]

Comment on attachment 146493 [details]
Patch

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

> Source/WebKit/efl/WebCoreSupport/NetworkInfoClientEfl.cpp:56
> +    if (eeze_init())

Are we really updating anything after eeze_init() is called? I would move this to the constructor and leave this function as notImplemented().

> Source/WebKit/efl/WebCoreSupport/NetworkInfoClientEfl.cpp:62
> +    eeze_shutdown();

I would move this to the destructor and leave this as notImplemented() for now.

> Source/WebKit/efl/WebCoreSupport/NetworkInfoClientEfl.cpp:67
> +    unsigned int bandwidth = -1;

It is unsigned so initializing it to -1 does not make sense here.

> Source/WebKit/efl/WebCoreSupport/NetworkInfoClientEfl.cpp:70
> +    Eeze_Net* ethNet = eeze_net_new(ethernetInterface);

This will return NULL if eth0 does not exist. Maybe if could add a NULL check right after?

> Source/WebKit/efl/WebCoreSupport/NetworkInfoClientEfl.cpp:73
> +    // FIXME : The eeze library doesn't support EEZE_NET_ADDR_TYPE_IP type yet. So, EEZE_NET_ADDR_TYPE_BROADCAST

Based on the code, it looks as if it is implemented in v1.2.0. Have you tried? If so, please disregard my comment.

> Source/WebKit/efl/WebCoreSupport/NetworkInfoClientEfl.cpp:76
> +    if (String::fromUTF8(address).isEmpty())

if (!address || !*address) might be lighter here?

> Source/WebKit/efl/WebCoreSupport/NetworkInfoClientEfl.cpp:81
> +        bandwidth = atoi(attribute);

How about using String::fromUTF8(attribute).toUIntStrict(&ok) to check for success and fallback to default value otherwise?

> Source/WebKit/efl/WebCoreSupport/NetworkInfoClientEfl.cpp:82
> +    else

As per WebKit coding style, you should use curly brackets for this else case (because of the comment line).

> Source/WebKit/efl/WebCoreSupport/NetworkInfoClientEfl.cpp:84
> +        bandwidth = std::numeric_limits<double>::infinity();

bandwidth is an unsigned integer so I'm unsure what the result of this line is. Maybe we could use UINT_MAX from limits.h instead? or maybe a reasonable default value like 100Mb/s.

> Source/WebKit/efl/WebCoreSupport/NetworkInfoClientEfl.cpp:88
> +    return bandwidth / 10; // MB/s

Why are you dividing by 10 here? What's the unit of the value returned by eeze_net_attribute_get(ethNet, "speed")?
Maybe you meant to divide by 8 to convert from Mbits to Mbytes?
Also, you probably don't want to divide in the case where bandwidth was unknown.

[Gyuyoung Kim]

Created attachment 146767 [details]
Patch

[Gyuyoung Kim]

(In reply to comment #13)
> (From update of attachment 146493 [details])

> > Source/WebKit/efl/WebCoreSupport/NetworkInfoClientEfl.cpp:62
> > +    eeze_shutdown();
 
> I would move this to the destructor and leave this as notImplemented() for now.

I think stopUpdating() is proper place. Because NetworkInfoController in WebCore manages life cycle of network info via startUpdating() and stopUpdating(). See also, BatteryClientEfl.cpp. 

This patch is based on latest editor draft.

 - 0 if the user is currently offline;
 - Infinity if the bandwidth is unknown;
 - an estimation of the current bandwidth in MB/s (Megabytes per seconds) available for communication with the browsing context active document's domain.

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

[Gyuyoung Kim]

Comment on attachment 146767 [details]
Patch

Attachment 146767 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/12939258

[Gyuyoung Kim]

CC'ing Antonio.

[Kenneth Rohde Christiansen]

Comment on attachment 146767 [details]
Patch

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

> Source/WebKit/efl/WebCoreSupport/NetworkInfoClientEfl.cpp:57
> +    if (eeze_init())
> +        return;

Maybe it is worth printing an error to the console?

> Source/WebKit/efl/WebCoreSupport/NetworkInfoClientEfl.cpp:67
> +    // FIXME : This function should consider cellular network as well. For example, 2G, 3G and 4G.

Add bugzilla link?

> Source/WebKit/efl/WebCoreSupport/NetworkInfoClientEfl.cpp:75
> +    // FIXME : The eeze library doesn't support EEZE_NET_ADDR_TYPE_IP type yet. So, EEZE_NET_ADDR_TYPE_BROADCAST
> +    // is used for now.

bug link pls

> Source/WebKit/efl/WebCoreSupport/NetworkInfoClientEfl.cpp:84
> +        bool ok = false;
> +        bandwidth = String::fromUTF8(attribute).toUIntStrict(&ok);

Seem to be no reason to initialize that? why not just call it bool ignoredOK; But are you sure it is needed?

[Gyuyoung Kim]

Created attachment 149267 [details]
Patch

[Gyuyoung Kim]

(In reply to comment #18)
> (From update of attachment 146767 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=146767&action=review
> 
> > Source/WebKit/efl/WebCoreSupport/NetworkInfoClientEfl.cpp:57
> > +    if (eeze_init())
> > +        return;
> 
> Maybe it is worth printing an error to the console?
> 
> > Source/WebKit/efl/WebCoreSupport/NetworkInfoClientEfl.cpp:67
> > +    // FIXME : This function should consider cellular network as well. For example, 2G, 3G and 4G.
> 
> Add bugzilla link?
> 
> > Source/WebKit/efl/WebCoreSupport/NetworkInfoClientEfl.cpp:75
> > +    // FIXME : The eeze library doesn't support EEZE_NET_ADDR_TYPE_IP type yet. So, EEZE_NET_ADDR_TYPE_BROADCAST
> > +    // is used for now.
> 
> bug link pls

I fix all you mentioned. 

> > Source/WebKit/efl/WebCoreSupport/NetworkInfoClientEfl.cpp:84
> > +        bool ok = false;
> > +        bandwidth = String::fromUTF8(attribute).toUIntStrict(&ok);
> 
> Seem to be no reason to initialize that? why not just call it bool ignoredOK; But are you sure it is needed?

I think we don't need to initialize ok variable. So, I modify it. Thanks.

[Gyuyoung Kim]

Comment on attachment 149267 [details]
Patch

Attachment 149267 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/13090112

[Gyuyoung Kim]

Created attachment 149295 [details]
Patch

[WebKit Review Bot]

Comment on attachment 149295 [details]
Patch

Clearing flags on attachment: 149295

Committed r121170: <http://trac.webkit.org/changeset/121170>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.