Move online state detection from the WebProcess to the NetworkProcess. This avoid executing the same (expensive) code in EACH web process whenever a network interface's state changes.
<rdar://problem/37093299>
Created attachment 336488 [details] Patch
Attachment 336488 [details] did not pass style-queue: ERROR: Source/WebCore/workers/service/context/ServiceWorkerThread.cpp:75: Missing spaces around : [whitespace/init] [4] Total errors found: 1 in 21 files If any of these errors are false positives, please file a bug against check-webkit-style.
Comment on attachment 336488 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=336488&action=review > Source/WebKit/WebProcess/Network/WebLoaderStrategy.cpp:561 > + for (auto& listener : m_onlineStateChangeListeners) > + listener(isOnLine); Can the listener modify m_onlineStateChangeListeners? Even if not, what guarantees that this will not become a problem in the future? > Source/WebKit/WebProcess/Network/WebLoaderStrategy.h:101 > + bool m_isOnLine { true }; That's optimistic :)
Comment on attachment 336488 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=336488&action=review >> Source/WebKit/WebProcess/Network/WebLoaderStrategy.cpp:561 >> + listener(isOnLine); > > Can the listener modify m_onlineStateChangeListeners? Even if not, what guarantees that this will not become a problem in the future? not An issue right now but I will make a copy for future proofing. >> Source/WebKit/WebProcess/Network/WebLoaderStrategy.h:101 >> + bool m_isOnLine { true }; > > That's optimistic :) I think this matches the spec which says to return false only if we know we do not have network access.
Created attachment 336499 [details] Patch
Attachment 336499 [details] did not pass style-queue: ERROR: Source/WebCore/workers/service/context/ServiceWorkerThread.cpp:75: Missing spaces around : [whitespace/init] [4] Total errors found: 1 in 21 files If any of these errors are false positives, please file a bug against check-webkit-style.
ping review.
Comment on attachment 336499 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=336499&action=review > Source/WebCore/loader/LoaderStrategy.h:79 > + virtual void addOnlineStateChangeListener(WTF::Function<void(bool)>&&) = 0; I think we no longer need the WTF prefix. > Source/WebKit/NetworkProcess/NetworkConnectionToWebProcess.cpp:257 > + return; Connection::sendMessage is already checking whether valid or not. We do not gain much except not encoding isOnline. In some other places we are not checking this and still call send. Maybe we should do the same? > Source/WebKit/NetworkProcess/NetworkProcess.cpp:130 > + }); Although ok, it seems somehow strange that we are never removing the listener. Ideally, we would add the listener/remove the listener whenever NetworkProcess has a WebProcess connection. > Source/WebKit/WebProcess/Network/WebLoaderStrategy.cpp:548 > +void WebLoaderStrategy::addOnlineStateChangeListener(WTF::Function<void(bool)>&& listener) WTF:: here and below? > Source/WebKit/WebProcess/Network/WebLoaderStrategy.cpp:551 > + m_onlineStateChangeListeners.append(WTFMove(listener)); It seems we will only call WebLoaderStrategy::addOnlineStateChangeListener once. Maybe m_onlineStateChangeListeners should not be a Vector.
Comment on attachment 336499 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=336499&action=review >> Source/WebCore/loader/LoaderStrategy.h:79 >> + virtual void addOnlineStateChangeListener(WTF::Function<void(bool)>&&) = 0; > > I think we no longer need the WTF prefix. ok >> Source/WebKit/NetworkProcess/NetworkConnectionToWebProcess.cpp:257 >> + return; > > Connection::sendMessage is already checking whether valid or not. > We do not gain much except not encoding isOnline. > In some other places we are not checking this and still call send. > Maybe we should do the same? ok >> Source/WebKit/NetworkProcess/NetworkProcess.cpp:130 >> + }); > > Although ok, it seems somehow strange that we are never removing the listener. > Ideally, we would add the listener/remove the listener whenever NetworkProcess has a WebProcess connection. NetworkStateNotifier does not even have an API to remove listeners. Listeners are expected to be permanent at the moment. >> Source/WebKit/WebProcess/Network/WebLoaderStrategy.cpp:548 >> +void WebLoaderStrategy::addOnlineStateChangeListener(WTF::Function<void(bool)>&& listener) > > WTF:: here and below? ok >> Source/WebKit/WebProcess/Network/WebLoaderStrategy.cpp:551 >> + m_onlineStateChangeListeners.append(WTFMove(listener)); > > It seems we will only call WebLoaderStrategy::addOnlineStateChangeListener once. > Maybe m_onlineStateChangeListeners should not be a Vector. This is inaccurate, it is called twice, once for pages and once for workers.
Created attachment 336606 [details] Patch
Comment on attachment 336499 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=336499&action=review >>> Source/WebKit/NetworkProcess/NetworkProcess.cpp:130 >>> + }); >> >> Although ok, it seems somehow strange that we are never removing the listener. >> Ideally, we would add the listener/remove the listener whenever NetworkProcess has a WebProcess connection. > > NetworkStateNotifier does not even have an API to remove listeners. Listeners are expected to be permanent at the moment. I guess we could add an API to NetworkStateNotifier to remove listeners but I am not sure it is worth the code / complexity since having a NetworkProcess and no WebProcess is rare.
Attachment 336606 [details] did not pass style-queue: ERROR: Source/WebCore/workers/service/context/ServiceWorkerThread.cpp:75: Missing spaces around : [whitespace/init] [4] Total errors found: 1 in 21 files If any of these errors are false positives, please file a bug against check-webkit-style.
> >> Source/WebKit/WebProcess/Network/WebLoaderStrategy.cpp:551 > >> + m_onlineStateChangeListeners.append(WTFMove(listener)); > > > > It seems we will only call WebLoaderStrategy::addOnlineStateChangeListener once. > > Maybe m_onlineStateChangeListeners should not be a Vector. > > This is inaccurate, it is called twice, once for pages and once for workers. OK but it could be easily reduced to only one listener. It feels like we are propagating NetworkStateNotifier multi listener-API although that does not seem necessary.
Comment on attachment 336606 [details] Patch r = me. I would try to look at simplifying the loader strategy API. Maybe we could just have something like loaderStrategy->startListeningToOnlineChange().
Comment on attachment 336606 [details] Patch Clearing flags on attachment: 336606 Committed r230007: <https://trac.webkit.org/changeset/230007>
All reviewed patches have been landed. Closing bug.