[webkit-reviews] review granted: [Bug 21923] Create an abstraction for script execution context : [Attachment 24716] 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 21923: Create an abstraction for script execution context
https://bugs.webkit.org/show_bug.cgi?id=21923

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

------- 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.

r=me