[Webkit-unassigned] [Bug 54516] [chromium] Add WebKit Chromium hooks allowing access to NetworkStateNotifier

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=54516





--- Comment #7 from Adam Klein <adamk at chromium.org>  2011-02-23 11:16:04 PST ---
(From update of attachment 82671)
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.

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.