Bug 89870 – [WK2] Add support for Network Information API

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKit2

[Chris Dumez]

There is currently no support for the Network Information API in WK2 even though it is implemented in WK1 and used by EFL port.
We need to implement Network Information API in WebKit2.

[Chris Dumez]

Created attachment 149326 [details]
Patch

[Gyuyoung Kim]

Comment on attachment 149326 [details]
Patch

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

> Source/WebKit2/WebProcess/NetworkInfo/WebNetworkInfoManager.cpp:75
> +    unsigned int bandwidth = true;

Why do you initialize *unsigned int* with boolean type ? In addition, why true ?

According to specification, bandwidth returns below values.

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

> Source/WebKit2/WebProcess/NetworkInfo/WebNetworkInfoManager.cpp:82
> +    bool metered = true;

Basically, metered means operator limits network usage. So, I think it is good to set 'false' by default.

[Chris Dumez]

Comment on attachment 149326 [details]
Patch

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

>> Source/WebKit2/WebProcess/NetworkInfo/WebNetworkInfoManager.cpp:75
>> +    unsigned int bandwidth = true;
> 
> Why do you initialize *unsigned int* with boolean type ? In addition, why true ?
> 
> According to specification, bandwidth returns below values.
> 
> - 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.

Bad copy/paste, thanks for catching it.

>> Source/WebKit2/WebProcess/NetworkInfo/WebNetworkInfoManager.cpp:82
>> +    bool metered = true;
> 
> Basically, metered means operator limits network usage. So, I think it is good to set 'false' by default.

Yes, it should be false. It is already set to false by default on Proxy side but it should be consistent.

[Chris Dumez]

Created attachment 149458 [details]
Patch

Take Gyuyoung's feedback into consideration.

[Gyuyoung Kim]

Comment on attachment 149458 [details]
Patch

Almost looks good to me.

[Chris Dumez]

(In reply to comment #5)
> (From update of attachment 149458 [details])
> Almost looks good to me.

Almost? ;) Then please tell me what is wrong so that I can make it look good to you in its entirety.

[Gyuyoung Kim]

(In reply to comment #6)
> (In reply to comment #5)
> > (From update of attachment 149458 [details] [details])
> > Almost looks good to me.
> 
> Almost? ;) Then please tell me what is wrong so that I can make it look good to you in its entirety.

As we said on irc, return type of bandwidth needs to be consistent with all places. I file a bug.

[Chris Dumez]

Created attachment 149512 [details]
Patch

Call sendSync() on m_process->connection() instead of m_page, in WebNetworkInfoManager.cpp.

[Chris Dumez]

C APIs will be provided in a separate bug report / patch to decrease patch size (in case someone is wondering).

[Chris Dumez]

Comment on attachment 149512 [details]
Patch

Clearing flags as the patch will need updating once the dependency lands.

[Gyuyoung Kim]

Comment on attachment 149512 [details]
Patch

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

> Source/WebKit2/Shared/WebNetworkInfo.h:46
> +        unsigned int bandwidth;

bandwidth type is changed to double data type. This patch also needs to change type of bandwidth.

[Chris Dumez]

Created attachment 149680 [details]
Patch

Update patch to use "double" instead of "unsigned int" for the bandwidth, now that the dependency has landed.

[Gyuyoung Kim]

Comment on attachment 149680 [details]
Patch

LGTM.

[Chris Dumez]

Created attachment 150782 [details]
Patch

Rebase on master and add new files to GNUMakefile and Qt project.

[Kenneth Rohde Christiansen]

Comment on attachment 150782 [details]
Patch

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

> Source/WebKit2/UIProcess/WebNetworkInfoProvider.h:45
> +    bool metered(WebNetworkInfoManagerProxy*) const;

why not isMetered here?

> Source/WebKit2/WebProcess/NetworkInfo/WebNetworkInfoManager.cpp:77
> +    // The spec indicates that we should return "infinity" if the bandwidth is unknown.
> +    double bandwidth = std::numeric_limits<double>::infinity();

Isnt there a smarter way to emit "Infinity" in JS?

[Chris Dumez]

Comment on attachment 150782 [details]
Patch

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

>> Source/WebKit2/UIProcess/WebNetworkInfoProvider.h:45
>> +    bool metered(WebNetworkInfoManagerProxy*) const;
> 
> why not isMetered here?

Right, this is better, I'll fix it. For some reason it is called metered() in WebCore.

>> Source/WebKit2/WebProcess/NetworkInfo/WebNetworkInfoManager.cpp:77
>> +    double bandwidth = std::numeric_limits<double>::infinity();
> 
> Isnt there a smarter way to emit "Infinity" in JS?

I don't know of any other. I know this works though:
Source/JavaScriptCore/runtime/NumberConstructor.cpp:    return jsNumber(std::numeric_limits<double>::infinity());

[Chris Dumez]

Created attachment 150883 [details]
Patch

Take Kenneth's feedback into consideration.

[WebKit Review Bot]

Comment on attachment 150883 [details]
Patch

Clearing flags on attachment: 150883

Committed r121989: <http://trac.webkit.org/changeset/121989>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.