[chromium] Add code to WebView to allow Chromium access to NetworkStateNotifier
Created attachment 82562 [details] Patch
This is a prerequisite for fixing http://crbug.com/7469
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.
Created attachment 82671 [details] Patch
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.
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.
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.
> 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.
Created attachment 83521 [details] Switched to setOnLine, consolidated Chromium and Android
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.
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>
All reviewed patches have been landed. Closing bug.