Bug 104465 – [Chromium] Support for Network Information API

• Status: RESOLVED
• Resolution: WONTFIX
• Component: WebKit Misc.

[Gyuyoung Kim]

I think chrome port needs to support this spec as well as EFL and blackberry ports. Patch is coming.

[Gyuyoung Kim]

Created attachment 178393 [details]
Work-in-progress

[Gyuyoung Kim]

Created attachment 178480 [details]
Patch

[WebKit Review Bot]

Please wait for approval from [e-mail], [e-mail], [e-mail], [e-mail] or [e-mail] before submitting, as this patch contains changes to the Chromium public API. See also https://trac.webkit.org/wiki/ChromiumWebKitAPI.

[WebKit Review Bot]

Comment on attachment 178480 [details]
Patch

Attachment 178480 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/15252001

New failing tests:
fast/dom/navigator-detached-no-crash.html

[Gyuyoung Kim]

Created attachment 178495 [details]
Patch

[Peter Beverloo]

Hi Gyuyoung Kim, thank you for the patch.

We've just started reviewing the Network Information API, and (at least on the Android side) have doubts about both the quality of the API and the feasibility of implementing it correctly.

To give an example, it's going to be very hard to support the "bandwidth" attribute in a correct way on mobile devices. Conditions can change very significantly within minutes. While the "change" event can be triggered when the connection type changes (i.e. from 3G to WiFi), that doesn't give us any new estimates. 
Firefox has a mapping between the connection type and the theoretically possible connection speed, but being connected to LTE doesn't guarantee that the website is actually going to get 50 Mbps.

We'll be sending an e-mail regarding the concerns as soon as possible. For now, however, I'd prefer not to land this..

[Darin Fisher (:fishd, Google)]

Comment on attachment 178495 [details]
Patch

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

R- based on Peter Beverloo's comments.

> Source/WebKit/chromium/public/WebViewClient.h:372
> +    virtual WebNetworkInfoClient* networkInfoClient() { return 0; }

WebKit API review notes: Network information is not a per-WebView property, so it
does not belong on WebViewClient.  There should instead be a Platform-level API
to query network information and to receive an event when network information
changes.

[Gyuyoung Kim]

(In reply to comment #6)
> Hi Gyuyoung Kim, thank you for the patch.
> 
> We've just started reviewing the Network Information API, and (at least on the Android side) have doubts about both the quality of the API and the feasibility of implementing it correctly.
> 
> To give an example, it's going to be very hard to support the "bandwidth" attribute in a correct way on mobile devices. Conditions can change very significantly within minutes. While the "change" event can be triggered when the connection type changes (i.e. from 3G to WiFi), that doesn't give us any new estimates.

> Firefox has a mapping between the connection type and the theoretically possible connection speed, but being connected to LTE doesn't guarantee that the website is actually going to get 50 Mbps.
> 
> We'll be sending an e-mail regarding the concerns as soon as possible. For now, however, I'd prefer not to land this..

Hi Peter, yes, Firefox and WebKit blackberry supported bandwidth property based on theoretical bandwidth speed. So, I also implemented this feature for Tizen platform as same method.

In blackberry port case, they notify network type change to JS callback by this implementation. I'm considering to let WebCore know network type change via virtual interface.

void NetworkInfoClientBlackBerry::onCurrentNetworkTypeChange(BlackBerry::Platform::InternalNetworkConnectionType newInternalNetworkType)
{
    if (m_isActive) {
        RefPtr<NetworkInfo> newNetworkInfo = NetworkInfo::create(bandwidth(), metered());
        NetworkInfoController::from(m_webPagePrivate->m_page)->didChangeNetworkInformation(eventNames().webkitnetworkinfochangeEvent , newNetworkInfo.get());
    }
}

Anyway, if you have doubts in this implementations, I'm willing to wait for your consideration.

[Gyuyoung Kim]

(In reply to comment #7)
> (From update of attachment 178495 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=178495&action=review
> 
> R- based on Peter Beverloo's comments.
> 
> > Source/WebKit/chromium/public/WebViewClient.h:372
> > +    virtual WebNetworkInfoClient* networkInfoClient() { return 0; }
> 
> WebKit API review notes: Network information is not a per-WebView property, so it
> does not belong on WebViewClient.  There should instead be a Platform-level API
> to query network information and to receive an event when network information
> changes.

In EFL port, we placed concrete implementation to WebCore/platform/efl in order to share it with both WK1 and WK2. But, we keep an interface for metered() functionality because we wasn't convinced if all platforms can support metered(). In Mozilla and blackberry cases, they support metered() by checking if current network is cellular. I also supported metered() functionality for Tizen as mozilla and blackberry. Do you think all chromium ports(win, linux, android and so on) can support metered() in platform-level API ?

[Darin Fisher (:fishd, Google)]

(In reply to comment #9)
...
> In EFL port, we placed concrete implementation to WebCore/platform/efl in order to share it with both WK1 and WK2. But, we keep an interface for metered() functionality because we wasn't convinced if all platforms can support metered(). In Mozilla and blackberry cases, they support metered() by checking if current network is cellular. I also supported metered() functionality for Tizen as mozilla and blackberry. Do you think all chromium ports(win, linux, android and so on) can support metered() in platform-level API ?

I'm sorry, I don't know what metered() means.  My comment is that network information does not seem to be WebView-dependent.  It seems to be a property that is global to the system, and it does not depend on any DOM constructs.  In the Chromium world, that implies that the method should live on the WebKit::Platform interface.  NOTE: That interface is implemented by Chromium via dependency injection.

[Gyuyoung Kim]

Created attachment 178951 [details]
Patch - Support network info change

[Gyuyoung Kim]

I add function to update network type change as battery status implementation. But, I still need to consider how to move this implementation to Platform layer. 

By the way, I doubt how was battery status implementation landed before. Because, IMHO, the battery status doesn't need to have webview dependency as well. I will check it.

[Gyuyoung Kim]

Adam, do you think network information API should be moved to Platform layer ? I made this patch based on BatteryStatus API for chromium port (Bug 83284). Because, I'm not sure that *metered* functionality can be supported by WebKit::Platform interface. If there is difference between battery status and network info, please let me know.

[James Robinson]

Given comment #6 I don't think it's worth using reviewer time on this patch at this time.