Bug 89870 - [WK2] Add support for Network Information API
Summary: [WK2] Add support for Network Information API
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit2 (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Chris Dumez
URL:
Keywords:
Depends on: 89959
Blocks:
  Show dependency treegraph
 
Reported: 2012-06-25 05:31 PDT by Chris Dumez
Modified: 2012-07-06 12:12 PDT (History)
11 users (show)

See Also:


Attachments
Patch (50.01 KB, patch)
2012-06-25 11:12 PDT, Chris Dumez
no flags Details | Formatted Diff | Diff
Patch (50.24 KB, patch)
2012-06-25 22:44 PDT, Chris Dumez
no flags Details | Formatted Diff | Diff
Patch (50.42 KB, patch)
2012-06-26 05:10 PDT, Chris Dumez
no flags Details | Formatted Diff | Diff
Patch (50.39 KB, patch)
2012-06-26 22:35 PDT, Chris Dumez
no flags Details | Formatted Diff | Diff
Patch (60.34 KB, patch)
2012-07-04 06:05 PDT, Chris Dumez
no flags Details | Formatted Diff | Diff
Patch (60.30 KB, patch)
2012-07-04 23:36 PDT, Chris Dumez
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Chris Dumez 2012-06-25 05:31:15 PDT
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.
Comment 1 Chris Dumez 2012-06-25 11:12:25 PDT
Created attachment 149326 [details]
Patch
Comment 2 Gyuyoung Kim 2012-06-25 18:12:40 PDT
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.
Comment 3 Chris Dumez 2012-06-25 22:40:01 PDT
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.
Comment 4 Chris Dumez 2012-06-25 22:44:47 PDT
Created attachment 149458 [details]
Patch

Take Gyuyoung's feedback into consideration.
Comment 5 Gyuyoung Kim 2012-06-25 23:14:57 PDT
Comment on attachment 149458 [details]
Patch

Almost looks good to me.
Comment 6 Chris Dumez 2012-06-25 23:42:22 PDT
(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.
Comment 7 Gyuyoung Kim 2012-06-26 02:03:07 PDT
(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.
Comment 8 Chris Dumez 2012-06-26 05:10:41 PDT
Created attachment 149512 [details]
Patch

Call sendSync() on m_process->connection() instead of m_page, in WebNetworkInfoManager.cpp.
Comment 9 Chris Dumez 2012-06-26 07:43:56 PDT
C APIs will be provided in a separate bug report / patch to decrease patch size (in case someone is wondering).
Comment 10 Chris Dumez 2012-06-26 11:15:07 PDT
Comment on attachment 149512 [details]
Patch

Clearing flags as the patch will need updating once the dependency lands.
Comment 11 Gyuyoung Kim 2012-06-26 17:43:45 PDT
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.
Comment 12 Chris Dumez 2012-06-26 22:35:01 PDT
Created attachment 149680 [details]
Patch

Update patch to use "double" instead of "unsigned int" for the bandwidth, now that the dependency has landed.
Comment 13 Gyuyoung Kim 2012-06-26 22:43:11 PDT
Comment on attachment 149680 [details]
Patch

LGTM.
Comment 14 Chris Dumez 2012-07-04 06:05:02 PDT
Created attachment 150782 [details]
Patch

Rebase on master and add new files to GNUMakefile and Qt project.
Comment 15 Kenneth Rohde Christiansen 2012-07-04 20:19:07 PDT
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?
Comment 16 Chris Dumez 2012-07-04 23:33:58 PDT
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());
Comment 17 Chris Dumez 2012-07-04 23:36:26 PDT
Created attachment 150883 [details]
Patch

Take Kenneth's feedback into consideration.
Comment 18 WebKit Review Bot 2012-07-06 12:12:30 PDT
Comment on attachment 150883 [details]
Patch

Clearing flags on attachment: 150883

Committed r121989: <http://trac.webkit.org/changeset/121989>
Comment 19 WebKit Review Bot 2012-07-06 12:12:37 PDT
All reviewed patches have been landed.  Closing bug.