Bug 21923 – Create an abstraction for script execution context

• Component: WebCore JavaScript
• Status: RESOLVED FIXED

[Alexey Proskuryakov]

HTML5: "The script execution context of a script is defined when that script is created. In this specification, it is either a Window object or an empty object; other specifications might make the script execution context be some other object."

We need to abstract it out to enable DOM objects to work in both documents and workers.

Patch forthcoming.

[Alexey Proskuryakov]

Created attachment 24716 [details]
proposed patch

[Darin Adler]

Comment on attachment 24716 [details]
proposed patch

> +        * 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

[Alexey Proskuryakov]

(In reply to comment #2)
> Maybe the argument could be called just "context"?

I did consider this, but thought that we have lots "context" and "global" objects around, so a littele differentiation could help. Even though it's a horribly long name.

Do you suggest to change the variable name globally, or just in these simpler methods?

> > +#include "JSDOMGlobalObject.h"
> 
> Does adding this include to JSDOMBinding.h create a cycle?

Hmm, JSDOMGlobalObject.h only includes from kjs, am I missing something?

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

I think that we now have enough classes in WebCore to eventually get rid of JSDOMBinding, and move all functions from there to their new homes. Do you think it makes sense?

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

I generally agree, but for a simple closure that is defined in a .cpp file and is not exported, I am not aware of any negative consequences, so I think that brevity is enough of a reason.

Another case where I used inline virtual functions was for isDocument() and isWorkerContext() methods - this matches prevailing WebCore style and improves readability, but maybe there are sufficient reasons not to? What I do know if that this needlessly increases the pressure on compiler and linker, but I don't know how much.

> r=me

Thank you!

[Darin Adler]

(In reply to comment #3)
> Do you suggest to change the variable name globally, or just in these simpler
> methods?

Just when appropriate, mostly in shorter, simpler functions.

> > > +#include "JSDOMGlobalObject.h"
> > 
> > Does adding this include to JSDOMBinding.h create a cycle?
> 
> Hmm, JSDOMGlobalObject.h only includes from kjs, am I missing something?

I think the answer is no, then.

> > 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.
> 
> I think that we now have enough classes in WebCore to eventually get rid of
> JSDOMBinding, and move all functions from there to their new homes. Do you
> think it makes sense?

Sure.

> > > +    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 enough.
> 
> I generally agree, but for a simple closure that is defined in a .cpp file and
> is not exported, I am not aware of any negative consequences, so I think that
> brevity is enough of a reason.
> 
> Another case where I used inline virtual functions was for isDocument() and
> isWorkerContext() methods - this matches prevailing WebCore style and improves
> readability, but maybe there are sufficient reasons not to? What I do know if
> that this needlessly increases the pressure on compiler and linker, but I don't
> know how much.

Everything you say makes sense. I have nothing to add.

[Alexey Proskuryakov]

Committed revision 37966.