Bug 54516 – [chromium] Add WebKit Chromium hooks allowing access to NetworkStateNotifier

• Component: WebCore Misc.
• Status: RESOLVED FIXED

[Adam Klein]

[chromium] Add code to WebView to allow Chromium access to NetworkStateNotifier

[Adam Klein]

Created attachment 82562 [details]
Patch

[Adam Klein]

This is a prerequisite for fixing http://crbug.com/7469

[Darin Fisher (:fishd, Google)]

Comment on attachment 82562 [details]
Patch

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

> Source/WebKit/chromium/public/WebView.h:370
> +    WEBKIT_API static void networkStateChange(bool onLine);

nit: I'm not sure that this makes best sense on WebView.  Other things in WebKit could
care about the network state.  A new, separate header file would be OK, even though it
would only have a single method in it.

Also, it is increasingly popular in WebKit code to avoid functions that just take a bool
parameter (unless the function is a setter for a boolean property).  Instead, people
prefer using enums or separate functions so that code at call-sites is more readable.

[Adam Klein]

Created attachment 82671 [details]
Patch

[Adam Klein]

Comment on attachment 82562 [details]
Patch

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

>> Source/WebKit/chromium/public/WebView.h:370
>> +    WEBKIT_API static void networkStateChange(bool onLine);
> 
> nit: I'm not sure that this makes best sense on WebView.  Other things in WebKit could
> care about the network state.  A new, separate header file would be OK, even though it
> would only have a single method in it.
> 
> Also, it is increasingly popular in WebKit code to avoid functions that just take a bool
> parameter (unless the function is a setter for a boolean property).  Instead, people
> prefer using enums or separate functions so that code at call-sites is more readable.

Thanks for the feedback, this is my first time extending the WebKit API so I knew I'd make some blunders.  I've created WebNetworkStateNotifier.{h,cpp}, and changed both it and WebCore::NetworkStateNotifier to use an enum to pass this information.

[Darin Fisher (:fishd, Google)]

Comment on attachment 82671 [details]
Patch

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

> Source/WebCore/platform/network/NetworkStateNotifier.h:73
> +    void networkStateChange(bool online);

It might be nice to keep the ANDROID and CHROMIUM ports similar.

It might also make more sense to name this function as a setter
for the onLine property.  setOnLine(bool onLine)

This way you do not need to define an enum at this level, and it
nicely mirrors the NetworkStateNotifier::onLine getter.

If you go the setOnLine route, you could define that in
NetworkStateNotifier.cpp behind an #ifdef as it would be the
trivial implementation that other ports could also use.  This
would allow you to kill NetworkStateNotifierAndroid.cpp and
NetworkStateNotifierChromium.cpp.

> Source/WebKit/chromium/public/WebNetworkStateNotifier.h:38
> +class WebNetworkStateNotifier {

Given my comments about WebCore::NetworkStateNotifier.  I now wonder
if this shouldn't just become a simple setter too: setOnLine(bool onLine)

I think the name of the method "networkStateChange" led me to want to
define enums, but when a simple boolean property setter will do, it seems
like it is better to just go with that.

[Adam Klein]

Comment on attachment 82671 [details]
Patch

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

>> Source/WebCore/platform/network/NetworkStateNotifier.h:73
>> +    void networkStateChange(bool online);
> 
> It might be nice to keep the ANDROID and CHROMIUM ports similar.
> 
> It might also make more sense to name this function as a setter
> for the onLine property.  setOnLine(bool onLine)
> 
> This way you do not need to define an enum at this level, and it
> nicely mirrors the NetworkStateNotifier::onLine getter.
> 
> If you go the setOnLine route, you could define that in
> NetworkStateNotifier.cpp behind an #ifdef as it would be the
> trivial implementation that other ports could also use.  This
> would allow you to kill NetworkStateNotifierAndroid.cpp and
> NetworkStateNotifierChromium.cpp.

I actually used setOnLine originally, but switched to networkStateChange to match Android. And then to the enum once the name didn't make the bool parameter clear.

The trouble with setOnLine is that I can't fully unify Android and Chromium, as Android calls into this code directly: http://codesearch.google.com/codesearch/p?hl=en#N6Qhr5kJSgQ/WebKit/android/jni/JavaBridge.cpp&q=%5C.networkStateChange%5C(&sa=N&cd=1&ct=rc&l=352

So I can switch to setOnLine, and get rid of NetworkStateNotifierAndroid.cpp, but will still need some #if PLATFORM(ANDROID) code in the header file to forward from networkStateChange() to setOnLine.  Which is probably acceptable, so I'll go that route.

>> Source/WebKit/chromium/public/WebNetworkStateNotifier.h:38
>> +class WebNetworkStateNotifier {
> 
> Given my comments about WebCore::NetworkStateNotifier.  I now wonder
> if this shouldn't just become a simple setter too: setOnLine(bool onLine)
> 
> I think the name of the method "networkStateChange" led me to want to
> define enums, but when a simple boolean property setter will do, it seems
> like it is better to just go with that.

Indeed, it seems that this should be setOnLine too.  The only advantage of the "networkStateChange" name is that it implies when it will be called. But given the known flakiness of that signal, and that setOnLine will already check internally that it actually _is_ a change, I think it's fine to lose that information.

[Darin Fisher (:fishd, Google)]

> Indeed, it seems that this should be setOnLine too.  The only advantage of the "networkStateChange" name is that it implies when it will be called. But given the known flakiness of that signal, and that setOnLine will already check internally that it actually _is_ a change, I think it's fine to lose that information.

Cool.

[Adam Klein]

Created attachment 83521 [details]
Switched to setOnLine, consolidated Chromium and Android

[Adam Klein]

Note that I didn't see any references to NetworkStateNotifierAndroid.cpp in the Android makefiles, so I didn't make any build changes relating to that removal.  Not sure how to test that this won't break the Android build.

[WebKit Commit Bot]

Comment on attachment 83521 [details]
Switched to setOnLine, consolidated Chromium and Android

Clearing flags on attachment: 83521

Committed r79563: <http://trac.webkit.org/changeset/79563>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.