[webkit-reviews] review granted: [Bug 22066] Implement Worker global object : [Attachment 24890] proposed patch

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has granted Alexey Proskuryakov <ap at webkit.org>'s
request for review:
Bug 22066: Implement Worker global object
https://bugs.webkit.org/show_bug.cgi?id=22066

Attachment 24890: proposed patch
https://bugs.webkit.org/attachment.cgi?id=24890&action=edit

------- Additional Comments from Darin Adler <darin at apple.com>
> +    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