Bug 21923 - Create an abstraction for script execution context
Summary: Create an abstraction for script execution context
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebCore JavaScript (show other bugs)
Version: 528+ (Nightly build)
Hardware: Macintosh OS X 10.5
: P2 Normal
Assignee: Alexey Proskuryakov
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2008-10-28 08:21 PDT by Alexey Proskuryakov
Modified: 2008-10-29 03:27 PDT (History)
1 user (show)

See Also:


Attachments
proposed patch (64.85 KB, patch)
2008-10-28 09:03 PDT, Alexey Proskuryakov
darin: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Alexey Proskuryakov 2008-10-28 08:21:48 PDT
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.
Comment 1 Alexey Proskuryakov 2008-10-28 09:03:43 PDT
Created attachment 24716 [details]
proposed patch
Comment 2 Darin Adler 2008-10-28 11:46:22 PDT
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
Comment 3 Alexey Proskuryakov 2008-10-28 12:20:25 PDT
(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!
Comment 4 Darin Adler 2008-10-28 12:25:23 PDT
(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.
Comment 5 Alexey Proskuryakov 2008-10-29 03:27:52 PDT
Committed revision 37966.