Bug 87067

Summary: [EFL] Implement Network Information API
Product: WebKit Reporter: Gyuyoung Kim <gyuyoung.kim>
Component: WebKit EFLAssignee: Gyuyoung Kim <gyuyoung.kim>
Status: RESOLVED FIXED    
Severity: Normal CC: gyuyoung.kim, lucas.de.marchi, mjs, peter, rakuco, tonikitoo, webkit.review.bot
Priority: P2 Keywords: WebExposed
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Prototype patch
gyuyoung.kim: commit-queue-
Prototype patch - 2
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch none

Description Gyuyoung Kim 2012-05-21 18:32:11 PDT
Patch will be submitted within 1~2 weeks.
Comment 1 Gyuyoung Kim 2012-05-30 23:08:59 PDT
Created attachment 144993 [details]
Prototype patch

This patch is just prototype. There is still crash issue and needs to implement network info spec. further.
Comment 2 Gyuyoung Kim 2012-06-05 05:15:49 PDT
Created attachment 145763 [details]
Prototype patch - 2
Comment 3 Gyuyoung Kim 2012-06-06 18:46:49 PDT
Created attachment 146175 [details]
Patch
Comment 4 Gyuyoung Kim 2012-06-06 18:49:27 PDT
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.
Comment 5 Gyuyoung Kim 2012-06-06 18:59:17 PDT
Created attachment 146176 [details]
Patch
Comment 6 Gyuyoung Kim 2012-06-06 22:25:06 PDT
Comment on attachment 146176 [details]
Patch

Attachment 146176 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/12910181
Comment 7 Gyuyoung Kim 2012-06-06 22:41:56 PDT
(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.
Comment 8 Gyuyoung Kim 2012-06-07 19:55:40 PDT
Created attachment 146457 [details]
Patch
Comment 9 Gyuyoung Kim 2012-06-07 23:08:18 PDT
Comment on attachment 146457 [details]
Patch

Attachment 146457 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/12924198
Comment 10 Gyuyoung Kim 2012-06-07 23:56:59 PDT
Created attachment 146493 [details]
Patch
Comment 11 Gyuyoung Kim 2012-06-08 05:38:36 PDT
Comment on attachment 146493 [details]
Patch

Attachment 146493 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/12924287
Comment 12 Gyuyoung Kim 2012-06-09 03:57:57 PDT
Kubo, could you review this patch informally ?
Comment 13 Chris Dumez 2012-06-09 07:19:33 PDT
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.
Comment 14 Gyuyoung Kim 2012-06-10 20:06:23 PDT
Created attachment 146767 [details]
Patch
Comment 15 Gyuyoung Kim 2012-06-10 20:19:32 PDT
(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
Comment 16 Gyuyoung Kim 2012-06-10 21:29:58 PDT
Comment on attachment 146767 [details]
Patch

Attachment 146767 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/12939258
Comment 17 Gyuyoung Kim 2012-06-19 22:16:10 PDT
CC'ing Antonio.
Comment 18 Kenneth Rohde Christiansen 2012-06-24 19:23:24 PDT
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?
Comment 19 Gyuyoung Kim 2012-06-25 03:26:46 PDT
Created attachment 149267 [details]
Patch
Comment 20 Gyuyoung Kim 2012-06-25 03:30:35 PDT
(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.
Comment 21 Gyuyoung Kim 2012-06-25 04:21:55 PDT
Comment on attachment 149267 [details]
Patch

Attachment 149267 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/13090112
Comment 22 Gyuyoung Kim 2012-06-25 08:26:40 PDT
Created attachment 149295 [details]
Patch
Comment 23 WebKit Review Bot 2012-06-25 11:02:00 PDT
Comment on attachment 149295 [details]
Patch

Clearing flags on attachment: 149295

Committed r121170: <http://trac.webkit.org/changeset/121170>
Comment 24 WebKit Review Bot 2012-06-25 11:02:08 PDT
All reviewed patches have been landed.  Closing bug.