Bug 21970

Summary: Make MessagePort event dispatch work in workers
Product: WebKit Reporter: Alexey Proskuryakov <ap>
Component: WebCore JavaScriptAssignee: Alexey Proskuryakov <ap>
Status: RESOLVED FIXED    
Severity: Normal    
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Mac   
OS: OS X 10.5   
Attachments:
Description Flags
proposed patch
none
updated patch darin: review+

Description Alexey Proskuryakov 2008-10-30 09:09:19 PDT
JSEventListener code assumes that the context is a Document, which will no longer be true.

Patch forthcoming.
Comment 1 Alexey Proskuryakov 2008-10-30 09:16:37 PDT
Created attachment 24771 [details]
proposed patch
Comment 2 Alexey Proskuryakov 2008-10-30 09:33:32 PDT
Created attachment 24772 [details]
updated patch

Removed a little too many unrelated changes...
Comment 3 Darin Adler 2008-10-30 11:44:52 PDT
Comment on attachment 24772 [details]
updated patch

> +JSDOMGlobalObject::JSDOMGlobalObjectData::JSDOMGlobalObjectData()
> +    : evt(0)

In practice, this "current event" could, and maybe should, be per-thread data, not per-global object data. It's set on entry and then cleared on exit by the code that dispatches events.

> +JSDOMGlobalObject::~JSDOMGlobalObject()
> +{
> +    // Clear any backpointers to the window
> +    ListenersMap::iterator i2 = d()->jsEventListeners.begin();
> +    ListenersMap::iterator e2 = d()->jsEventListeners.end();

A little strange that 2 comes before 1 in this function; but I guess that was copied and pasted from elsewhere.

>  void JSDOMWindowBase::clearHelperObjectProperties()
>  {
> -    d()->evt = 0;
> +    setCurrentEvent(0);
>  }

I believe this function is unnecessary. I'm not insisting that you remove it, but I believe that it's doing no good at all.

> Index: WebCore/dom/Document.h
> ===================================================================
> --- WebCore/dom/Document.h	(revision 37996)
> +++ WebCore/dom/Document.h	(working copy)
> @@ -410,7 +410,7 @@ public:
>  
>      bool wellFormed() const { return m_wellFormed; }
>  
> -    const KURL& url() const { return m_url; }
> +    virtual const KURL& url() const ;

Extra space before the semicolon.

It could be bad for performance to make Document::url() virtual just because ScriptExecutionContext::url() needs to be. Maybe you should consider the style I used for Node::localName() and Element::localName() that allows the latter to be a non-virtual function?

I guess it's premature optimization to assume that it's going to be a performance problem, but it does worry me a bit.

r=me
Comment 4 Alexey Proskuryakov 2008-10-31 02:45:00 PDT
Committed revision 38033.

(In reply to comment #3)
> In practice, this "current event" could, and maybe should, be per-thread data,
> not per-global object data.

I'm not sure about this - per-thread keys are a rather limited process-wide resource, so frameworks shouldn't use them unless there is a significant benefit.

> A little strange that 2 comes before 1 in this function; but I guess that was
> copied and pasted from elsewhere.

It was, fixed now.

> It could be bad for performance to make Document::url() virtual just because
> ScriptExecutionContext::url() needs to be. Maybe you should consider the style
> I used for Node::localName() and Element::localName() that allows the latter to
> be a non-virtual function?

OK.

> r=me

Thank you!