Bug 22066 – Implement Worker global object

• Component: WebCore JavaScript
• Status: RESOLVED FIXED

[Alexey Proskuryakov]

Patch with an initial implementation of WorkerGlobalScope is forthcoming.

[Alexey Proskuryakov]

Created attachment 24889 [details]
proposed patch

Some of this code is far from being final, but I'd like to get it landed to simplify logistics of working on code that is executed in Workers.

[Alexey Proskuryakov]

Created attachment 24890 [details]
proposed patch

Removed a change that I do not intend to check in yet.

[Darin Adler]

Comment on attachment 24890 [details]
proposed patch

> +    JSWorkerContext.lut.h \

I was really hoping we could avoid adding more of these. I've been trying to eliminate them for a long time. Could we find some way to get the bindings-generation script to do this work instead? We can still do custom implementations but I'd really like to phase out use of the create_hash_table script ASAP. I think the IDL syntax is much better than the @begin syntax.

> +        return static_cast<WorkerContext*>(scriptExecutionContext)->script()->jsWorkerContext();

Given that this is on the ScriptController, could we omit the "js" prefix? The "script" in ScriptController already means JavaScript.

> +    : JSDOMGlobalObject(JSWorkerContext::createStructureID(new (Heap::heap(this)->globalData()) JSWorkerContextPrototype(JSWorkerContextPrototype::createStructureID(jsNull()))), new JSDOMGlobalObjectData, this)

Maybe you could use a helper function (inline with internal linkage) to make this long line a little shorter?

> +ScriptExecutionContext* JSWorkerContext::scriptExecutionContext() const
> +{
> +    return m_impl.get();
> +}

Should be inline and defined in the header?

> +WorkerScriptController::~WorkerScriptController()
> +{
> +    m_jsWorkerContext = 0;
> +
> +    ASSERT(!m_globalData->heap.protectedObjectCount());
> +    ASSERT(!m_globalData->heap.isBusy());
> +    m_globalData->heap.destroy();
> +}

Might want a comment explaining why setting m_jsWorkerContext to 0 is needed.

> +    if (comp.complType() == Throw)
> +        fprintf(stderr, "%s\n", comp.value()->toString(exec).ascii());

I think we want utf8() here, not ascii(). But also, do we really want to do an fprintf to stderr here? That seems wrong long-term and cross-platform, especially unconditionally.

> +    class WorkerScriptController {

This class looks noncopyable, but it has no noncopyable members. I think it should inherit from Noncopyable.

> +        JSWorkerContext* jsWorkerContext()

I don't think this needs a "js" prefix. Except that now I see that you're using this to distinguish two objects, m_workerContext and m_jsWorkerContext. Maybe the latter should be called m_workerContextWrapper? I think I like that better than the JS prefix.

> +    // The document may have been destroyed already.
> +    if (!document())
> +        return;

Are there other conditions where the document is "done" other than "destroyed"? Maybe this is not just about the document having been destroyed, but more about when it is done. Since the document is a garbage-collected object I don't think that destruction time is a well-defined event. Maybe you mean something subtly different?

> +    // FIXME: Should we change the KURL constructor to have this behavior?
> +    if (url.isNull())
> +        return KURL();
> +    // FIXME: does this need to provide a charset, like Document::completeURL does?
> +    return KURL(m_location->url(), url);

Good FIXMEs. I don't have the answer for either at the moment.

> +        WorkerLocation(const KURL& url) : m_url(url) {}

We should get consistent about whether we use a space between the parentheses in places like this; maybe put it in the style guidelines. I personally like it slightly better with the space.

> +        String m_scriptURL;

It seems strange to take a KURL argument and then store it as a String. That makes it more expensive to create a new URL. Is this the right tradeoff? Are we trying to save space?

r=me, this seems like it's in a good state to land

[Darin Adler]

Comment on attachment 24890 [details]
proposed patch

One more comment.

~WorkerContext should be marked virtual, because it's going to be virtual anyway due to the inheritance from ScriptExecutionContext.

[Alexey Proskuryakov]

(In reply to comment #3)
> (From update of attachment 24890 [details] [edit])
> > +    JSWorkerContext.lut.h \
> 
> I was really hoping we could avoid adding more of these. I've been trying to
> eliminate them for a long time. Could we find some way to get the
> bindings-generation script to do this work instead? 

I did try, but couldn't make it work - I think the code generator will need to be improved to handle JSDOMWindowBase and JSWorkerContext.

> > +ScriptExecutionContext* JSWorkerContext::scriptExecutionContext() const
> > +{
> > +    return m_impl.get();
> > +}
> 
> Should be inline and defined in the header?

No, because it's virtual. Maybe we'll need to add a non-virtual variant of it in the future.

> > +    if (comp.complType() == Throw)
> > +        fprintf(stderr, "%s\n", comp.value()->toString(exec).ascii());
> 
> I think we want utf8() here, not ascii(). But also, do we really want to do an
> fprintf to stderr here? That seems wrong long-term and cross-platform,
> especially unconditionally.

Oops, forgot to write up a FIXME here. Yes, this needs to be channeled via Frame to console.

> I don't think this needs a "js" prefix. Except that now I see that you're using
> this to distinguish two objects, m_workerContext and m_jsWorkerContext. Maybe
> the latter should be called m_workerContextWrapper? I think I like that better
> than the JS prefix.

I don't really think of the global object as a wrapper, but OK.

> > +    // The document may have been destroyed already.
> > +    if (!document())
> > +        return;
> 
> Are there other conditions where the document is "done" other than "destroyed"?
> Maybe this is not just about the document having been destroyed, but more about
> when it is done. Since the document is a garbage-collected object I don't think
> that destruction time is a well-defined event. Maybe you mean something subtly
> different?

This check is about the document having been destroyed, but you are right, it should bail out if the document was detached, too. I'm not quite sure how to best do it (tie to FrameLoader::stopLoading() somehow?), added a FIXME.

> > +        String m_scriptURL;
> 
> It seems strange to take a KURL argument and then store it as a String. That
> makes it more expensive to create a new URL. Is this the right tradeoff? Are we
> trying to save space?

We need to deep copy the string to pass it to another thread. It doesn't really matter when to re-parse it, but I prefer to only pass strings between threads for simplicity. I guess we could add a deep copy method to KURL to avoid re-parsing, but I don't think it would help all that much.

Thanks!

[Alexey Proskuryakov]

Committed revision 38150.