Bug 30747 – [v8] Do not retrieve proxy when fetching context

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKit Misc.

[anton muhin]

On some paths caller already has a proxy, so there is no need to retrieve it another time when fetching context.

[anton muhin]

Created attachment 41787 [details]
First take

Adam, it looks safe, but I'd appreciate if you had another look at it.

[Adam Barth]

Comment on attachment 41787 [details]
First take

It doesn't make sense to have a static method that takes effectively a |this| pointer as an argument.  Can you re-phrase this code using instance methods?

[Dimitri Glazkov (Google)]

+1. Also, while working on event listeners recently, I was baffled and not amused at how loose and weird our ScriptExecutionContext <-> ScriptController <--> V8Proxy <--> v8::Context relationship graph looks. We should really try to simplify this. I know peeps want performance and efficiency, but I bet there's lots of skeletons buried along this path.

(I am stopping short, to avoid becoming unpositive over existence of WorkerProxyExecutionContext and how it's a "look at me, I am a V8Proxy", except its not, ... alrighty then.).

[Adam Barth]

The correct relationship is as follows:

ScriptExecutionContext == Document == DOMWindow
V8Proxy == ScriptController == Frame

V8Proxy and ScriptController are actually the same thing, just split apart for no reason.  v8::Context == GlobalObject, which has a many-to-one relationship with DOMWindows because of IsolatedWorlds.

Not sure if that was helpful.

[anton muhin]

(In reply to comment #2)
> (From update of attachment 41787 [details])
> It doesn't make sense to have a static method that takes effectively a |this|
> pointer as an argument.  Can you re-phrase this code using instance methods?

Very good point, thank you.   Even more---following you advice I found there is already the method I need.  Doing a new build (which takes a lot of time on my Mac) and sending a new patch.

[anton muhin]

(In reply to comment #4)
> The correct relationship is as follows:
> 
> ScriptExecutionContext == Document == DOMWindow
> V8Proxy == ScriptController == Frame
> 
> V8Proxy and ScriptController are actually the same thing, just split apart for
> no reason.  v8::Context == GlobalObject, which has a many-to-one relationship
> with DOMWindows because of IsolatedWorlds.
> 
> Not sure if that was helpful.

That was, at least for me.  Adam, Dmitry, maybe we should refactor it?  For example for me as a new comer, I see that there is a proxy and frame and they look exactly the way Adam describes, but are separate.  Now I am not sure if they are always the same or there are some edge cases/cases I amnot aware of when they are not.

[Adam Barth]

> That was, at least for me.  Adam, Dmitry, maybe we should refactor it?

I support combining ScriptController and V8Proxy.  If there's a clean way to do this, please go for it (in a separate patch).

[anton muhin]

Created attachment 41830 [details]
2nd try

[Adam Barth]

Comment on attachment 41830 [details]
2nd try

A patch of beauty!

[WebKit Commit Bot]

Comment on attachment 41830 [details]
2nd try

Clearing flags on attachment: 41830

Committed r50049: <http://trac.webkit.org/changeset/50049>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.