http://www.whatwg.org/specs/web-apps/current-work/multipage/workers.html#handler-workerglobalscope-onoffline http://www.whatwg.org/specs/web-apps/current-work/multipage/offline.html#event-offline When the value that would be returned by the navigator.onLine attribute of a Window or WorkerGlobalScope changes from true to false, the user agent must queue a task to fire a simple event named offline at the Window or WorkerGlobalScope object. On the other hand, when the value that would be returned by the navigator.onLine attribute of a Window or WorkerGlobalScope changes from false to true, the user agent must queue a task to fire a simple event named online at the Window or WorkerGlobalScope object.
Created attachment 206953 [details] Patch
Comment on attachment 206953 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=206953&action=review > Source/WebCore/platform/network/NetworkStateNotifier.cpp:-44 > - ASSERT(!m_networkStateChangedFunction); We should probably assert that function is not null. > Source/WebCore/platform/network/NetworkStateNotifier.h:69 > + typedef void (*NetworkStateChangedFunction)(bool m_isOnLine); I think we usually call them "Listener" not "Function". > Source/WebCore/workers/WorkerGlobalScope.cpp:120 > +static HashSet<WorkerGlobalScope*>* workerGlobalScopes; Why a pointer to a HashSet and not simply a HashSet? > Source/WebCore/workers/WorkerGlobalScope.cpp:160 > + ASSERT(!workerGlobalScopes->contains(this)); This can also be written as: AddResult addResult = workerGlobalScopes->add(this); ASSERT_UNUSED(addResult, addResult.isNewEntry); To look up only once.
Created attachment 206960 [details] Patch
(In reply to comment #2) > > Source/WebCore/platform/network/NetworkStateNotifier.cpp:-44 > > - ASSERT(!m_networkStateChangedFunction); > > We should probably assert that function is not null. Done. > > Source/WebCore/platform/network/NetworkStateNotifier.h:69 > > + typedef void (*NetworkStateChangedFunction)(bool m_isOnLine); > > I think we usually call them "Listener" not "Function". Okay. :s/Function/Listener/g and adjusted method and member variable names accordingly. > > Source/WebCore/workers/WorkerGlobalScope.cpp:120 > > +static HashSet<WorkerGlobalScope*>* workerGlobalScopes; > > Why a pointer to a HashSet and not simply a HashSet? I followed the convention used in Page.cpp: static HashSet<Page*>* allPages; > > Source/WebCore/workers/WorkerGlobalScope.cpp:160 > > + ASSERT(!workerGlobalScopes->contains(this)); > > This can also be written as: > AddResult addResult = workerGlobalScopes->add(this); > ASSERT_UNUSED(addResult, addResult.isNewEntry); > > To look up only once. Done.
Comment on attachment 206960 [details] Patch Attachment 206960 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://webkit-queues.appspot.com/results/1104423 New failing tests: http/tests/security/cross-origin-plugin-private-browsing-toggled.html
Created attachment 206961 [details] Archive of layout-test-results from webkit-ews-15 for mac-mountainlion-wk2 The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews. Bot: webkit-ews-15 Port: mac-mountainlion-wk2 Platform: Mac OS X 10.8.3
Created attachment 206962 [details] Patch
Comment on attachment 206962 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=206962&action=review > Source/WebCore/workers/WorkerGlobalScope.cpp:118 > +static Mutex& workerGlobalScopeSetMutex() > +{ > + AtomicallyInitializedStatic(Mutex&, mutex = *new Mutex); > + return mutex; > +} This is not cool, adding mutexes complicates the design and makes deadlocks more likely. Talking to workers should be done through WorkerGlobalScopeProxy, there is no need to create new cross-thread messaging mechanisms for that.
(In reply to comment #8) > This is not cool, adding mutexes complicates the design and makes deadlocks more likely. > > Talking to workers should be done through WorkerGlobalScopeProxy, there is no need to create new cross-thread messaging mechanisms for that. I see your point. I will rework the patch to use WorkerGlobalScopeProxy. Thanks for the comment.
Created attachment 207034 [details] Patch
Comment on attachment 207034 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=207034&action=review Looks good to me. Please consider the suggestion below. > Source/WebCore/platform/network/NetworkStateNotifier.h:84 > + HashSet<NetworkStateChangeListener> m_listeners; Given that there is no way to remove from the set, and that the only operation we need is iteration, it seems much more efficient to use a vector than a HashSet.
(In reply to comment #11) > Looks good to me. Please consider the suggestion below. > > > Source/WebCore/platform/network/NetworkStateNotifier.h:84 > > + HashSet<NetworkStateChangeListener> m_listeners; > > Given that there is no way to remove from the set, and that the only operation we need is iteration, it seems much more efficient to use a vector than a HashSet. Thanks for the review. I will change HashSet to Vector as you suggested before landing the patch.
Committed r152885: <http://trac.webkit.org/changeset/152885>
(In reply to comment #13) > Committed r152885: <http://trac.webkit.org/changeset/152885> It seems the debug bots are in big trouble after the commit. On Qt debug we got 29501 failures. All debug bots on Apple master have "Failed exiting early after 50 crashes and 0 timeouts."
(In reply to comment #14) > (In reply to comment #13) > > Committed r152885: <http://trac.webkit.org/changeset/152885> > > It seems the debug bots are in big trouble after the commit. > On Qt debug we got 29501 failures. All debug bots on Apple master have "Failed exiting early after 50 crashes and 0 timeouts." Oops. I will fix it soon.
(In reply to comment #14) > (In reply to comment #13) > > Committed r152885: <http://trac.webkit.org/changeset/152885> > > It seems the debug bots are in big trouble after the commit. > On Qt debug we got 29501 failures. All debug bots on Apple master have "Failed exiting early after 50 crashes and 0 timeouts." Fixed. http://trac.webkit.org/changeset/152894