Bug 118832 – WorkerGlobalScope should support onoffline/ononline event handlers

• Component: DOM
• Status: RESOLVED FIXED

[Kwang Yul Seo]

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.

[Kwang Yul Seo]

Created attachment 206953 [details]
Patch

[Chris Dumez]

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.

[Kwang Yul Seo]

Created attachment 206960 [details]
Patch

[Kwang Yul Seo]

(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.

[Build Bot]

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

[Build Bot]

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

[Kwang Yul Seo]

Created attachment 206962 [details]
Patch

[Alexey Proskuryakov]

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.

[Kwang Yul Seo]

(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.

[Kwang Yul Seo]

Created attachment 207034 [details]
Patch

[Alexey Proskuryakov]

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.

[Kwang Yul Seo]

(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.

[Kwang Yul Seo]

Committed r152885: <http://trac.webkit.org/changeset/152885>

[Zoltan Arvai]

(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."

[Kwang Yul Seo]

(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.

[Kwang Yul Seo]

(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