[webkit-reviews] review granted: [Bug 21923] Create an abstraction for script execution context : [Attachment 24716] proposed patch

bugzilla-daemon at webkit.org bugzilla-daemon at webkit.org
Tue Oct 28 11:46:22 PDT 2008

Darin Adler <darin at apple.com> has granted Alexey Proskuryakov <ap at webkit.org>'s
request for review:
Bug 21923: Create an abstraction for script execution context

Attachment 24716: proposed patch

------- Additional Comments from Darin Adler <darin at apple.com>
> +	   * bindings/js/JSDocumentCustom.cpp: (WebCore::JSDocument::mark):
> +	   Call markActiveObjectsForContext() by its ne name.

Typo: "new name".

There's also the word "gtwo0argument" in here.

> -JSAudioConstructor::JSAudioConstructor(ExecState* exec, Document* document)
> +JSAudioConstructor::JSAudioConstructor(ExecState* exec,
ScriptExecutionContext* scriptExecutionContext)

Maybe the argument could be called just "context"?

> +#include "JSDOMGlobalObject.h"

Does adding this include to JSDOMBinding.h create a cycle?

I'm not sure that we should move getDOMConstructor to JSDOMGlobalObject just
because the storage is inside the global object. I can live with it, but I
think we could consider that a hidden implementation detail.

> +    virtual void fired()
> +    {
> +	   m_scriptExecutionContext->dispatchMessagePortEvents();
> +	   delete this;
> +    }

I think it's typically a mistake to put virtual functions into class
definitions. We can do it if there's a clear benefit, but I don't think that
brevity alone is a enough.


More information about the webkit-reviews mailing list