I think chrome port needs to support this spec as well as EFL and blackberry ports. Patch is coming.
Created attachment 178393 [details] Work-in-progress
Created attachment 178480 [details] Patch
Please wait for approval from abarth@webkit.org, dglazkov@chromium.org, fishd@chromium.org, jamesr@chromium.org or tkent@chromium.org before submitting, as this patch contains changes to the Chromium public API. See also https://trac.webkit.org/wiki/ChromiumWebKitAPI.
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
Created attachment 178495 [details] Patch
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..
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.
(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.
(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 ?
(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.
Created attachment 178951 [details] Patch - Support network info change
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.
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.
Given comment #6 I don't think it's worth using reviewer time on this patch at this time.