Bug 71757 – WebWorkerRunLoop wrapper around WorkerRunLoop

• Component: New Bugs
• Status: RESOLVED FIXED

[David Grogan]

beginning of WebWorkerRunLoop wrapper around WorkerRunLoop

[David Grogan]

Created attachment 113971 [details]
Patch

[WebKit Review Bot]

Please wait for approval from [e-mail] before submitting because this patch contains changes to the Chromium public API.

[Early Warning System Bot]

Comment on attachment 113971 [details]
Patch

Attachment 113971 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/10338834

[David Grogan]

Created attachment 113973 [details]
Patch

[Early Warning System Bot]

Comment on attachment 113973 [details]
Patch

Attachment 113973 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/10337976

[David Grogan]

Created attachment 113974 [details]
Patch

[David Grogan]

Comment on attachment 113974 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=113974&action=review

Michael, this isn't ready to be submitted, it doesn't do any task posting, just some minimal runloop tracking.  I'm adding the meat of the tracking (maps from ids to MessageLoops) to the chromium code.

Could you take a look to ensure that this is approximately what you were expecting?

> Source/WebKit/chromium/public/WebWorkerRunLoop.h:33
> +    static WebWorkerRunLoop* current();

I still might change this to return the "id", which is just the WebCore::WorkerRunLoop* casted to a uintptr_t.  Depending on how the chrome changes go, the id might be more convenient.

[Early Warning System Bot]

Comment on attachment 113974 [details]
Patch

Attachment 113974 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/10338841

[David Grogan]

Also welcome are testing suggestions.

I'd like to supply a Mock WebKitPlatformSupport, start a few workers and check that workerRunLoopStarted and workerRunLoopStopped are called the right number of times in the right order.  None of the classes of tests that I'm aware of would easily allow for that.  Though I'm only familiar with ui, py_auto, browser, layout and unit tests.

[Darin Fisher (:fishd, Google)]

Comment on attachment 113974 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=113974&action=review

> Source/WebKit/chromium/public/WebWorkerRunLoop.h:30
> +class WebWorkerRunLoop {

We already have WebThread and WebThread::Task.  We also have WebKitPlatformSupport::currentThread.

Could that be useful here?  I'm not clear on what a WebWorkerRunLoop is.

[Collabora GTK+ EWS bot]

Comment on attachment 113974 [details]
Patch

Attachment 113974 [details] did not pass gtk-ews (gtk):
Output: http://queues.webkit.org/results/10376063

[David Grogan]

(In reply to comment #10)
> We already have WebThread and WebThread::Task.  We also have WebKitPlatformSupport::currentThread.
> 
> Could that be useful here?  I'm not clear on what a WebWorkerRunLoop is.

WebThread and WebKitPlatformSupport::currentThread give webkit access to threads created by chromium.

WebWorkerRunLoop gives chromium access to the message loops belonging to worker threads created in WebCore, specifically WebCore::WorkerRunLoop - http://codesearch.google.com/codesearch#OAMlx_jo-ck/src/third_party/WebKit/Source/WebCore/workers/WorkerRunLoop.h

We could theoretically reuse the WebThread interface, but would need a different way to access the current thread that makes it clear that chromium is looking for a webcore-created thread.  And the meaning of WebThread ("Provides an interface to an embedder-defined thread implementation.") would have to change.


(As always, let me know if I've misunderstood something.)

[Darin Fisher (:fishd, Google)]

(In reply to comment #12)
> We could theoretically reuse the WebThread interface, but would need a different way to access the current thread that makes it clear that chromium is looking for a webcore-created thread.  And the meaning of WebThread ("Provides an interface to an embedder-defined thread implementation.") would have to change.

Perhaps it is reasonable to have either the embedder or WebKit implement WebThread.  Perhaps it is reasonable to assign a WebThread created by WebKit to the current thread, so that WebKitPlatformSupport::currentThread() can work.

I think I need to better understand what you are trying to accomplish with this change though.  I don't understand the purpose of the workerRunLoop{Started,Stopping} events.  I don't see how those are intended to be used, etc.

[Michael Nordman]

(In reply to comment #13)
> (In reply to comment #12)
> > We could theoretically reuse the WebThread interface, but would need a different way to access the current thread that makes it clear that chromium is looking for a webcore-created thread.  And the meaning of WebThread ("Provides an interface to an embedder-defined thread implementation.") would have to change.
> 
> Perhaps it is reasonable to have either the embedder or WebKit implement WebThread.  Perhaps it is reasonable to assign a WebThread created by WebKit to the current thread, so that WebKitPlatformSupport::currentThread() can work.

It might reasonable to reuse this same interface webcore created and embedder created threads, but there are differences with delete semantics and with the ability to post delayed tasks. Not clear that we should reuse it? The approach taken was to not do so.

> I think I need to better understand what you are trying to accomplish with this change though.  I don't understand the purpose of the workerRunLoop{Started,Stopping} events.  I don't see how those are intended to be used, etc.

Those are there so the embedder knows when the webcore created worker thread start/stop and to provide an interface for scheduling 'tasks' directly on that thread. If we resuse WebThread as an interface for chrome to poke at webcore created worker threads, i think we'd still need api in this vien to know of there existence.

[Michael Nordman]

Comment on attachment 113974 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=113974&action=review

> Source/WebCore/workers/WorkerRunLoop.cpp:119
> +        PlatformSupport::workerRunLoopStarted(&m_runLoop);

To ensure this only gets called once per instance, this would need to be in the if (!nested) condition.

> Source/WebCore/workers/WorkerRunLoop.cpp:127
> +        PlatformSupport::workerRunLoopStopping(&m_runLoop);

Similarly, this would need to be in the if (!nested) condition.

> Source/WebKit/chromium/public/WebKitPlatformSupport.h:332
> +    virtual void workerRunLoopStopped(uintptr_t id) { }

Do the 'id' parameters need to be here? Assuming the embedded maintains a map of workers, it can assign ids on its own, and lookup one from the other. I think we just need notification of starting/stopping and the 'loop' instance ptr.

> Source/WebKit/chromium/src/PlatformSupport.cpp:1061
> +    RefPtr<WebWorkerRunLoopImpl> webWorkerRunLoop = WebWorkerRunLoopImpl::create(loop);

Why refcounted? In the webkit API, this class should have a protected dtor so the embedded cannot delete it. An instance gets created here, and deleted in the following method (after calling out to workerRunLoopStopped()).

> Source/WebKit/chromium/src/PlatformSupport.cpp:1062
> +    webKitPlatformSupport()->workerRunLoopStarted(reinterpret_cast<uintptr_t>(loop), webWorkerRunLoop.get());

WebCore::WorkerRunLoop instance ptrs values may not be unique 'id' values over the life of the process.

> Source/WebKit/chromium/src/WebWorkerRunLoopImpl.cpp:59
> +        m_currentLoop = new WTF::ThreadSpecific<WebWorkerRunLoopImpl*>;

Is this a thread safe usage of a WTF::ThreadSpecific<> ctor, or do we need to construct the TLS slot in another way?

> Source/WebKit/chromium/src/WebWorkerRunLoopImpl.cpp:64
> +

Should the dtor clear the 'current' value?

[David Grogan]

Comment on attachment 113974 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=113974&action=review

Thanks a lot for the feedback, responses inline.

>> Source/WebCore/workers/WorkerRunLoop.cpp:119
>> +        PlatformSupport::workerRunLoopStarted(&m_runLoop);
> 
> To ensure this only gets called once per instance, this would need to be in the if (!nested) condition.

You're right, will do.

>> Source/WebKit/chromium/public/WebKitPlatformSupport.h:332
>> +    virtual void workerRunLoopStopped(uintptr_t id) { }
> 
> Do the 'id' parameters need to be here? Assuming the embedded maintains a map of workers, it can assign ids on its own, and lookup one from the other. I think we just need notification of starting/stopping and the 'loop' instance ptr.

Without the ids, PlatformSupport would have to maintain a static Map of WorkerRunLoop -> WebWorkerRunLoop so that it can pass the same WebWorkerRunLoop* to WebKitPlatforumSupport on start and stop.  I didn't see any other data structures in Source/WebKit/chromium/src/PlatformSupport.cpp so guessed that they should be avoided.

If a static Map in PlatformSupport is not a big deal then the ids can go.

>> Source/WebKit/chromium/src/PlatformSupport.cpp:1061
>> +    RefPtr<WebWorkerRunLoopImpl> webWorkerRunLoop = WebWorkerRunLoopImpl::create(loop);
> 
> Why refcounted? In the webkit API, this class should have a protected dtor so the embedded cannot delete it. An instance gets created here, and deleted in the following method (after calling out to workerRunLoopStopped()).

Ah yes, this is broken as it stands now. webWorkerRunLoop will be deleted when the method exits, and the embedder will have an invalid memory address.

>> Source/WebKit/chromium/src/PlatformSupport.cpp:1062
>> +    webKitPlatformSupport()->workerRunLoopStarted(reinterpret_cast<uintptr_t>(loop), webWorkerRunLoop.get());
> 
> WebCore::WorkerRunLoop instance ptrs values may not be unique 'id' values over the life of the process.

But two values won't be in use simultaneously, right?  Though if ids get tossed in favor of a HashMap the point is moot.

>> Source/WebKit/chromium/src/WebWorkerRunLoopImpl.cpp:59
>> +        m_currentLoop = new WTF::ThreadSpecific<WebWorkerRunLoopImpl*>;
> 
> Is this a thread safe usage of a WTF::ThreadSpecific<> ctor, or do we need to construct the TLS slot in another way?

I'll look further into this.

[Michael Nordman]

> > Do the 'id' parameters need to be here? Assuming the embedded maintains a map of workers, it can assign ids on its own, and lookup one from the other. I think we just need notification of starting/stopping and the 'loop' instance ptr.
> 
> Without the ids, PlatformSupport would have to maintain a static Map of WorkerRunLoop -> WebWorkerRunLoop so that it can pass the same WebWorkerRunLoop* to WebKitPlatforumSupport on start and stop.  I didn't see any other data structures in Source/WebKit/chromium/src/PlatformSupport.cpp so guessed that they should be avoided.
> 
> If a static Map in PlatformSupport is not a big deal then the ids can go.

We could just use WebWorkerRunLoop::current(), it will return the instance pointer.

[David Grogan]

Created attachment 115660 [details]
Patch

[David Grogan]

Comment on attachment 115660 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=115660&action=review

> Source/WebCore/workers/WorkerRunLoop.cpp:148
> +        PlatformSupport::workerRunLoopStopping(this);

I had to move workerRunLoopStopping from ~RunLoopSetup.

runInMode is called to load resources but the runloop is stopped before the worker script is evaluated.  If workerRunLoopStopping remained in ~RunLoopSetup, there would be no WebWorkerRunLoop for chrome to associate with outgoing indexeddb messages sent during the initial script evaluation.

Putting workerRunLoopStopping here smells like I'm missing a better scheme.  If it survives, I'll have to add a WorkerRunLoop::DeathObserver so that workerRunLoopStopping will be called even if the thread is killed before this main run() method executes.

[David Grogan]

Comment on attachment 115660 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=115660&action=review

> Source/WebKit/chromium/src/WebWorkerRunLoopImpl.cpp:81
> +    ASSERT(**m_currentLoop);

This ASSERT is just wrong.  I've deleted it locally.

[Michael Nordman]

Comment on attachment 115660 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=115660&action=review

>> Source/WebCore/workers/WorkerRunLoop.cpp:148
>> +        PlatformSupport::workerRunLoopStopping(this);
> 
> I had to move workerRunLoopStopping from ~RunLoopSetup.
> 
> runInMode is called to load resources but the runloop is stopped before the worker script is evaluated.  If workerRunLoopStopping remained in ~RunLoopSetup, there would be no WebWorkerRunLoop for chrome to associate with outgoing indexeddb messages sent during the initial script evaluation.
> 
> Putting workerRunLoopStopping here smells like I'm missing a better scheme.  If it survives, I'll have to add a WorkerRunLoop::DeathObserver so that workerRunLoopStopping will be called even if the thread is killed before this main run() method executes.

It might make sense to make these calls to starting/stopping within the WorkerThread::workerThread() method. Having them balanced in an obvious way would be good, that and it would make it more clear that 'starting' needs to be called prior to eval'ing the workerscript.

> Source/WebKit/chromium/src/PlatformSupport.cpp:1071
> +      ASSERT(loop == WebWorkerRunLoopImpl::current()->m_workerRunLoop);

Could we ASSERT(!WebWorkerRunLoopImpl::current()) here instead?

> Source/WebKit/chromium/src/WebWorkerRunLoopImpl.cpp:32
> +WTF::ThreadSpecific<WebWorkerRunLoopImpl*>* WebWorkerRunLoopImpl::m_currentLoop;

Seems like the <T> template parameter should be just WebWorkerRunLoopImpl... w/o the *? Is there a reason to put a ptr to a ptr as the TLS value?

> Source/WebKit/chromium/src/WebWorkerRunLoopImpl.cpp:43
> +        DEFINE_STATIC_LOCAL(WTF::Mutex, m_threadSpecificMutex, ());

I don't see where this will buy you any thread safety since DEFINE_STATIC_LOCAL is not thread safe? Also the m_ prefixing is confusing since this isn't defining a data member.

static ThreadSpecific<WebWorkerRunLoopImpl>& currentLoopTLS()
{
    AtomicallyInitializedStatic(ThreadSpecific<WebWorkerRunLoopImpl>*, currentLoopTLS = new ThreadSpecific<WebWorkerRunLoopImpl>);
    return *currentLoopTLS;
}

WebWorkerRunLoopImpl::WebWorkerRunLoopImpl(...)
{
   ASSERT(!currentLoopTLS().isSet());
   currentLoopTLS().set(this);
}

WebWorkerRunLoopImpl::~WebWorkerRunLoopImpl(...)
{
   ASSERT(currentLoopTLS().get() == this);
   currentLoopTLS().set(0);
   // Oh... i see, WTF::ThreadSpecific does let you change the value once set.
   // Not sure why that would be but i guess it does explain the ptr to a ptr thing.
}

[David Grogan]

Created attachment 116266 [details]
Patch

[Early Warning System Bot]

Comment on attachment 116266 [details]
Patch

Attachment 116266 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/10587004

[David Grogan]

Comment on attachment 115660 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=115660&action=review

>>> Source/WebCore/workers/WorkerRunLoop.cpp:148
>>> +        PlatformSupport::workerRunLoopStopping(this);
>> 
>> I had to move workerRunLoopStopping from ~RunLoopSetup.
>> 
>> runInMode is called to load resources but the runloop is stopped before the worker script is evaluated.  If workerRunLoopStopping remained in ~RunLoopSetup, there would be no WebWorkerRunLoop for chrome to associate with outgoing indexeddb messages sent during the initial script evaluation.
>> 
>> Putting workerRunLoopStopping here smells like I'm missing a better scheme.  If it survives, I'll have to add a WorkerRunLoop::DeathObserver so that workerRunLoopStopping will be called even if the thread is killed before this main run() method executes.
> 
> It might make sense to make these calls to starting/stopping within the WorkerThread::workerThread() method. Having them balanced in an obvious way would be good, that and it would make it more clear that 'starting' needs to be called prior to eval'ing the workerscript.

Good suggestion, moved them.

>> Source/WebKit/chromium/src/PlatformSupport.cpp:1071
>> +      ASSERT(loop == WebWorkerRunLoopImpl::current()->m_workerRunLoop);
> 
> Could we ASSERT(!WebWorkerRunLoopImpl::current()) here instead?

After the the stop/start move to WorkerThread::workerThread, yes.  Changed.

>> Source/WebKit/chromium/src/WebWorkerRunLoopImpl.cpp:43

> 
> I don't see where this will buy you any thread safety since DEFINE_STATIC_LOCAL is not thread safe? Also the m_ prefixing is confusing since this isn't defining a data member.
> 
> static ThreadSpecific<WebWorkerRunLoopImpl>& currentLoopTLS()
> {
>     AtomicallyInitializedStatic(ThreadSpecific<WebWorkerRunLoopImpl>*, currentLoopTLS = new ThreadSpecific<WebWorkerRunLoopImpl>);
>     return *currentLoopTLS;
> }
> 
> WebWorkerRunLoopImpl::WebWorkerRunLoopImpl(...)
> {
>    ASSERT(!currentLoopTLS().isSet());
>    currentLoopTLS().set(this);
> }
> 
> WebWorkerRunLoopImpl::~WebWorkerRunLoopImpl(...)
> {
>    ASSERT(currentLoopTLS().get() == this);
>    currentLoopTLS().set(0);
>    // Oh... i see, WTF::ThreadSpecific does let you change the value once set.
>    // Not sure why that would be but i guess it does explain the ptr to a ptr thing.
> }

Moved to the single static function that initializes with AtomicallyInitializedStatic.  Let me know if you still have reservations about the ptr to ptr.

[Gustavo Noronha (kov)]

Comment on attachment 116266 [details]
Patch

Attachment 116266 [details] did not pass gtk-ews (gtk):
Output: http://queues.webkit.org/results/10595001

[David Grogan]

Created attachment 116276 [details]
Patch

[Michael Nordman]

Comment on attachment 116276 [details]
Patch

The latest snapshot looks good to me.

View in context: https://bugs.webkit.org/attachment.cgi?id=116276&action=review

> Source/WebKit/chromium/src/WebWorkerRunLoopImpl.cpp:57
> +class TaskForwarder : public ScriptExecutionContext::Task {

maybe put this class and the currentLoopTLS() function in an anon namespace?

> Source/WebKit/chromium/src/WebWorkerRunLoopImpl.cpp:67
> +    virtual ~TaskForwarder() { }

is the empty dtor needed?

[David Grogan]

Comment on attachment 116276 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=116276&action=review

>> Source/WebKit/chromium/src/WebWorkerRunLoopImpl.cpp:57
>> +class TaskForwarder : public ScriptExecutionContext::Task {
> 
> maybe put this class and the currentLoopTLS() function in an anon namespace?

Done.

>> Source/WebKit/chromium/src/WebWorkerRunLoopImpl.cpp:67
>> +    virtual ~TaskForwarder() { }
> 
> is the empty dtor needed?

Nope. Removed.

[David Grogan]

Created attachment 116282 [details]
Patch

[David Levin]

Comment on attachment 116276 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=116276&action=review

> Source/WebKit/chromium/ChangeLog:9
> +

You changelog should ideally indicate what you are doing in each function. A short explanation suffices.

> Source/WebCore/workers/WorkerRunLoop.cpp:36
> +

All changes in this file are unrelated to "WebWorkerRunLoop wrapper around WorkerRunLoop". Ideally they would be done in a separate change.

> Source/WebKit/chromium/public/WebKitPlatformSupport.h:338
> +    virtual void workerRunLoopStarted(WebWorkerRunLoop* runLoop) { }

No need for runLoop here and in fact, it will likely give you an error on some platforms due to it not being used.

> Source/WebKit/chromium/public/WebWorkerRunLoop.h:2
> + * Copyright (C) 2011, Google Inc. All rights reserved.

No comma after the year.

> Source/WebKit/chromium/public/WebWorkerRunLoop.h:7
> + * 1.  Redistributions of source code must retain the above copyright

This looks odd with this extra space on the first line.

> Source/WebKit/chromium/public/WebWorkerRunLoop.h:33
> +     public:

Your indentation is off.

> Source/WebKit/chromium/src/WebWorkerRunLoopImpl.cpp:40
> +    AtomicallyInitializedStatic(ThreadSpecific<WebWorkerRunLoopImpl*>*, currentLoopTLS = new ThreadSpecific<WebWorkerRunLoopImpl*>);

Ideally this would be initialized on the main thread before any worker threads start and then you could avoid the AromicallyInitializedStatic which takes a lock every time this line is hit (and makes the function much slower than needed).

> Source/WebKit/chromium/src/WebWorkerRunLoopImpl.cpp:58
> + public:

Wrong indentation.

> Source/WebKit/chromium/src/WebWorkerRunLoopImpl.cpp:59
> +    TaskForwarder(WebWorkerRunLoop::Task* task)

This should take a PassOwnPtr<WebWorkerRunLoop::Task>

> Source/WebKit/chromium/src/WebWorkerRunLoopImpl.cpp:62
> +    }

Add blank line.

> Source/WebKit/chromium/src/WebWorkerRunLoopImpl.cpp:66
> +    }

Ditto.

> Source/WebKit/chromium/src/WebWorkerRunLoopImpl.cpp:74
> +    m_workerRunLoop->postTask(adoptPtr(new TaskForwarder(task)));

TaskForwarder should expose a create method which looks like this:

PassOwnPtr<TaskForwarder> create(PassOwnPtr<WebWorkerRunLoop::Task> task)
{
    return adoptPtr(new TaskForwarder(task));
}

the constructor should be private.

> Source/WebKit/chromium/src/WebWorkerRunLoopImpl.h:47
> +    virtual void postTask(Task*);

It should be a PassOwnPtr and then no comment is needed about "task ownership is transferred".

[David Levin]

Comment on attachment 116282 [details]
Patch

See comments above (which were for the previous patch but many still apply).

[David Levin]

Comment on attachment 116282 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=116282&action=review

> Source/WebKit/chromium/src/WebWorkerRunLoopImpl.h:32
> +#include <wtf/ThreadSpecific.h>

A lot of these doesn't appear to be needed. Please do a pass and remove the unnecessary ones. Please so fwd decl when possible.

> Source/WebKit/chromium/src/WebWorkerRunLoopImpl.h:40
> +class WebWorkerRunLoopImpl : public ThreadSafeRefCounted<WebWorkerRunLoopImpl>,

It doesn't make sense that this is ThreadSafeRefCounted when it is new/deleted.

[David Grogan]

Comment on attachment 116276 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=116276&action=review

>> Source/WebCore/workers/WorkerRunLoop.cpp:36

> 
> All changes in this file are unrelated to "WebWorkerRunLoop wrapper around WorkerRunLoop". Ideally they would be done in a separate change.

Removed these changes.

>> Source/WebKit/chromium/public/WebKitPlatformSupport.h:338
>> +    virtual void workerRunLoopStarted(WebWorkerRunLoop* runLoop) { }
> 
> No need for runLoop here and in fact, it will likely give you an error on some platforms due to it not being used.

Done.

>> Source/WebKit/chromium/public/WebWorkerRunLoop.h:2
>> + * Copyright (C) 2011, Google Inc. All rights reserved.
> 
> No comma after the year.

Done

>> Source/WebKit/chromium/public/WebWorkerRunLoop.h:7
>> + * 1.  Redistributions of source code must retain the above copyright
> 
> This looks odd with this extra space on the first line.

Done.

>> Source/WebKit/chromium/public/WebWorkerRunLoop.h:33
>> +     public:
> 
> Your indentation is off.

Fixed.

>> Source/WebKit/chromium/src/WebWorkerRunLoopImpl.cpp:40
>> +    AtomicallyInitializedStatic(ThreadSpecific<WebWorkerRunLoopImpl*>*, currentLoopTLS = new ThreadSpecific<WebWorkerRunLoopImpl*>);
> 
> Ideally this would be initialized on the main thread before any worker threads start and then you could avoid the AromicallyInitializedStatic which takes a lock every time this line is hit (and makes the function much slower than needed).

I can change this to a DEFINE_STATIC_LOCAL and then call WebKit::WebWorkerRunLoop:current() from the main thread in chromium but I feel like that's a bit brittle.

Alternatively, I could make ThreadGlobalData call a new PlatformSupport::perThreadInit(), which would call WebWorkerRunLoopImpl::current, but that feels intrusive.

Is there an easier way?  If not, which of these would you recommend?

>> Source/WebKit/chromium/src/WebWorkerRunLoopImpl.cpp:58
>> + public:
> 
> Wrong indentation.

Fixed.

>> Source/WebKit/chromium/src/WebWorkerRunLoopImpl.cpp:59
>> +    TaskForwarder(WebWorkerRunLoop::Task* task)
> 
> This should take a PassOwnPtr<WebWorkerRunLoop::Task>

Done.

>> Source/WebKit/chromium/src/WebWorkerRunLoopImpl.cpp:62
>> +    }
> 
> Add blank line.

Done.

>> Source/WebKit/chromium/src/WebWorkerRunLoopImpl.cpp:66
>> +    }
> 
> Ditto.

Done.

>> Source/WebKit/chromium/src/WebWorkerRunLoopImpl.cpp:74

> 
> TaskForwarder should expose a create method which looks like this:
> 
> PassOwnPtr<TaskForwarder> create(PassOwnPtr<WebWorkerRunLoop::Task> task)
> {
>     return adoptPtr(new TaskForwarder(task));
> }
> 
> the constructor should be private.

Done.

>> Source/WebKit/chromium/src/WebWorkerRunLoopImpl.h:47
>> +    virtual void postTask(Task*);
> 
> It should be a PassOwnPtr and then no comment is needed about "task ownership is transferred".

I don't think I can make this a PassOwnPtr because that would mean changing WebWorkerRunLoop's corresponding method signature, exposing WTF::PassOwnPtr to embedders.

[David Grogan]

Created attachment 116305 [details]
Patch

[David Levin]

(In reply to comment #33)
> (From update of attachment 116276 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=116276&action=review
> 


> >> Source/WebKit/chromium/src/WebWorkerRunLoopImpl.cpp:40
> >> +    AtomicallyInitializedStatic(ThreadSpecific<WebWorkerRunLoopImpl*>*, currentLoopTLS = new ThreadSpecific<WebWorkerRunLoopImpl*>);
> > 
> > Ideally this would be initialized on the main thread before any worker threads start and then you could avoid the AromicallyInitializedStatic which takes a lock every time this line is hit (and makes the function much slower than needed).
> 
> I can change this to a DEFINE_STATIC_LOCAL and then call WebKit::WebWorkerRunLoop:current() from the main thread in chromium but I feel like that's a bit brittle.
> 
> Alternatively, I could make ThreadGlobalData call a new PlatformSupport::perThreadInit(), which would call WebWorkerRunLoopImpl::current, but that feels intrusive.
> 
> Is there an easier way?  If not, which of these would you recommend?

I would make an init function creates it and call the init function only from the main thread.

Do assert(isMainThread()) in the init function.

Avoid using DEFINE_STATIC_LOCAL which will work even if someone doesn't call it on the main thread first.

Does that make sense?


> >> Source/WebKit/chromium/src/WebWorkerRunLoopImpl.h:47
> >> +    virtual void postTask(Task*);
> > 
> > It should be a PassOwnPtr and then no comment is needed about "task ownership is transferred".
> 
> I don't think I can make this a PassOwnPtr because that would mean changing WebWorkerRunLoop's corresponding method signature, exposing WTF::PassOwnPtr to embedders.

Sounds good.

[David Levin]

Comment on attachment 116305 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=116305&action=review

Just address that one thing about initialization an we're good. Or you could tell me why you think it is ok to take a lock there everytime. (I'm hesitant to have that because I had to clear up something similar in wtf and it is much harder to clean up later than it is to address it right now.)

> Source/WebKit/chromium/ChangeLog:35
> +        WebWorkerRunLoop TLS to |this|.

Inits WebWorkerRunLoop TLS.

I don't care if you change this. Just showing how it could even be more concise to minimize your typing :).

> Source/WebKit/chromium/ChangeLog:37
> +        WebWorkerRunLoop TLS to 0.

Clears WebWorkerRunLoop TLS.

[David Grogan]

Created attachment 116319 [details]
Patch

[David Levin]

Comment on attachment 116319 [details]
Patch

I'm r+'ing but "Please wait for approval from [e-mail] before submitting because this patch contains changes to the Chromium public API."

[David Levin]

Comment on attachment 116319 [details]
Patch

cq- pending ok from Darin Fisher.

[Darin Fisher (:fishd, Google)]

Comment on attachment 116319 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=116319&action=review

> Source/WebKit/chromium/public/WebKitPlatformSupport.h:338
> +    virtual void workerRunLoopStarted(WebWorkerRunLoop*) { }

Sorry, I should have mentioned this earlier, but for notifications like this, we normally
prefix them with either "will" or "did"...

workerRunLoopStarted -> didStartWorkerRunLoop

I have a question about workerRunLoopStopped.  On PlatformSupport.h, you call the
method workerRunLoopStopping, which suggests that the run loop hasn't stopped yet,
but soon will.  However, workerRunLoopStopped says that the run loop has already
stopped.  Clearly, these are not consistent.  Assuming, "will stop" is what you mean,
then I'd call this function "willStopWorkerRunLoop".

> Source/WebKit/chromium/public/WebWorkerRunLoop.h:38
> +    static WebWorkerRunLoop* current();

Static functions like this need to be marked up with WEBKIT_EXPORT, or else you
will break the component=shared_library build.

Can you say more about why you need 'current' to be implemented in the WebKit
layer, where it is apparently not needed by WebCore?  If it is only needed by
the embedder, then it seems like the embedder can maintain the TLS entry itself
based on the notifications didStartWorkerRunLoop and willStopWorkerRunLoop.

[Michael Nordman]

Comment on attachment 116319 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=116319&action=review

>> Source/WebKit/chromium/public/WebWorkerRunLoop.h:38
>> +    static WebWorkerRunLoop* current();
> 
> Static functions like this need to be marked up with WEBKIT_EXPORT, or else you
> will break the component=shared_library build.
> 
> Can you say more about why you need 'current' to be implemented in the WebKit
> layer, where it is apparently not needed by WebCore?  If it is only needed by
> the embedder, then it seems like the embedder can maintain the TLS entry itself
> based on the notifications didStartWorkerRunLoop and willStopWorkerRunLoop.

having the embedder maintain the TLS value would work too, in which case the embedder would have to be be responsible for deletion too so the dtor would need to be public

this would minimize the API on the class, but not sure it's an improvement since the caller will have to delete them and could not choose to ignore the didStart(loop) willStop(/*no param since the caller tracks current*/) methods

6 one way, half-dozen the other

[Darin Fisher (:fishd, Google)]

(In reply to comment #41)
> having the embedder maintain the TLS value would work too, in which case the embedder would have to be be responsible for deletion too so the dtor would need to be public
> 
> this would minimize the API on the class, but not sure it's an improvement since the caller will have to delete them and could not choose to ignore the didStart(loop) willStop(/*no param since the caller tracks current*/) methods
> 
> 6 one way, half-dozen the other

[I met up with Michael, and we chatted about this a bit.  Here's an alternative proposal.]

We don't need TLS in WebKit, and we don't need to heap allocate WebWorkerRunLoop.  Instead, WebWorkerRunLoop can just be a very simple concrete class that wraps a WebCore::WorkerRunLoop pointer.

  class WebWorkerRunLoop {
  public:
      class Task {
          ...
      };
      WEBKIT_EXPORT void postTask(Task*);
  private:
      WebCore::WorkerRunLoop* m_private;
  };

  class WebKitPlatformSupport {
  public:
      ...
      virtual void didStartWorkerRunLoop(const WebWorkerRunLoop&) { }
      virtual void willStopWorkerRunLoop(const WebWorkerRunLoop&) { }
  };

What the above does is give us a way to convey a "handle" to a WebCore::WorkerRunLoop without having the embedder actually have to see the WebCore type.  The handle is only valid until willStopWorkerRunLoop is called.

This accomplishes the goal (that I have) of keeping the WebKit layer very thin.  It just isolates Chromium from WebCore.  It also pushes all of the logic about retaining references to WebWorkerRunLoop instances to the Chromium side.  The Chromium side needs to store WebWorkerRunLoop instances in TLS so that function calls made on a worker thread can find its run loop, and Chromium also needs to store WebWorkerRunLoop instances in "hash maps" that can be used by other threads in Chromium that wish to communicate (postTask) back to the worker thread.  It seems ideal if all of this lifetime tracking code only had to live on the Chromium side.

-Darin

[Michael Nordman]

That looks pretty good to me too, nice lack of virtual methods and webcore TLS busywork :) A couple of details to work thru...

- does the willStop() method need the loop argument, what would the caller do with it?
- should the class have comparison/equality operators associated with it (that compare the m_private ptr value), to facilitate using instances in std collections.

[Darin Fisher (:fishd, Google)]

(In reply to comment #43)
> That looks pretty good to me too, nice lack of virtual methods and webcore TLS busywork :) A couple of details to work thru...
> 
> - does the willStop() method need the loop argument, what would the caller do with it?

Not strictly necessary, but suppose the embedder didn't use TLS but instead used only a hashmap.  They'd need a way to find the same object in such a container so they can remove it from the container.


> - should the class have comparison/equality operators associated with it (that compare the m_private ptr value), to facilitate using instances in std collections.

Yes, probably a good idea.  Those operators are only required when using WebWorkerRunLoop as a key to a STL container though, right?  Is that needed?

[David Grogan]

Created attachment 116819 [details]
Patch

[David Grogan]

(In reply to comment #44)
> (In reply to comment #43)
> > - should the class have comparison/equality operators associated with it (that compare the m_private ptr value), to facilitate using instances in std collections.
> 
> Yes, probably a good idea.  Those operators are only required when using WebWorkerRunLoop as a key to a STL container though, right?  Is that needed?

The WebWorkerRunLoops are going to be stored in a TLS and a non-TLS map.  I'm not yet sure if they are going to be keys or values, but given how trivial those operator implementations are, I added them to keep the flexibility.

I don't know when WEBKIT_EXPORT is needed.  Does any symbol used by the embedder need it?

[Darin Fisher (:fishd, Google)]

Comment on attachment 116819 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=116819&action=review

> Source/WebKit/chromium/public/WebWorkerRunLoop.h:38
> +    WebWorkerRunLoop(WebCore::WorkerRunLoop*);

Any public, non-inline function that is not guarded by WEBKIT_IMPLEMENTATION
needs to be annotated with WEBKIT_EXPORT.

However, this constructor should probably be wrapped with #if WEBKIT_IMPLEMENTATION
since it is only callable by WebKit.

What is the purpose of exposing operator< and operator==?  Normally, we do not
export operators.  We instead create an "equals" or "compare" function, and then
provide those operators as inline helper functions (at the WebKit namespace
level).

[David Levin]

Comment on attachment 116819 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=116819&action=review

> Source/WebKit/chromium/ChangeLog:33
> +        stored WorkerRunLoops.

That is obvious but the ideal thing to put here would be why this is even here. It looks rather odd.

[David Grogan]

Created attachment 116832 [details]
Patch

[David Grogan]

(In reply to comment #47)
> What is the purpose of exposing operator< and operator==?  Normally, we do not
> export operators.  We instead create an "equals" or "compare" function, and then
> provide those operators as inline helper functions (at the WebKit namespace
> level).

Got it, changed.  Why do we not export operators?

[David Grogan]

Created attachment 116869 [details]
updated changelogs

[David Grogan]

Created attachment 116874 [details]
including missing function defs

[David Levin]

Comment on attachment 116874 [details]
including missing function defs

Looks good to me.

[Darin Fisher (:fishd, Google)]

(In reply to comment #50)
> (In reply to comment #47)
> > What is the purpose of exposing operator< and operator==?  Normally, we do not
> > export operators.  We instead create an "equals" or "compare" function, and then
> > provide those operators as inline helper functions (at the WebKit namespace
> > level).
> 
> Got it, changed.  Why do we not export operators?

There's no technical problem with exporting operators.  It is more of a convention actually.

However, there's a concrete benefit to the way we structure things, which leads us to the convention:

 -- It is always preferable to define operator== and operator< at global scope.  That way, you can support type coercion on either the left-hand or right-hand arguments.  (This really only matters if a class has a subclass, which is not the case for WebWorkerRunLoop.  However, consistency is still a good thing.)

 -- Making operator== and operator< global would require that they be friends of the class, but instead we just give said class an equals or compare method.  This leads to implementing operator== and operator< inline.

[WebKit Review Bot]

Comment on attachment 116874 [details]
including missing function defs

Clearing flags on attachment: 116874

Committed r101335: <http://trac.webkit.org/changeset/101335>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.