Bug 30747

Summary: [v8] Do not retrieve proxy when fetching context
Product: WebKit Reporter: anton muhin <antonm>
Component: WebKit Misc.Assignee: Nobody <webkit-unassigned>
Status: RESOLVED FIXED    
Severity: Normal CC: abarth, commit-queue, dglazkov
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: All   
OS: All   
Attachments:
Description Flags
First take
abarth: review-
2nd try none

Description anton muhin 2009-10-24 11:57:33 PDT
On some paths caller already has a proxy, so there is no need to retrieve it another time when fetching context.
Comment 1 anton muhin 2009-10-24 12:00:34 PDT
Created attachment 41787 [details]
First take

Adam, it looks safe, but I'd appreciate if you had another look at it.
Comment 2 Adam Barth 2009-10-24 13:19:07 PDT
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?
Comment 3 Dimitri Glazkov (Google) 2009-10-24 13:39:43 PDT
+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.).
Comment 4 Adam Barth 2009-10-24 13:49:48 PDT
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.
Comment 5 anton muhin 2009-10-24 15:08:23 PDT
(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.
Comment 6 anton muhin 2009-10-24 15:14:51 PDT
(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.
Comment 7 Adam Barth 2009-10-24 15:31:50 PDT
> 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).
Comment 8 anton muhin 2009-10-25 15:10:51 PDT
Created attachment 41830 [details]
2nd try
Comment 9 Adam Barth 2009-10-25 21:23:28 PDT
Comment on attachment 41830 [details]
2nd try

A patch of beauty!
Comment 10 WebKit Commit Bot 2009-10-25 21:33:09 PDT
Comment on attachment 41830 [details]
2nd try

Clearing flags on attachment: 41830

Committed r50049: <http://trac.webkit.org/changeset/50049>
Comment 11 WebKit Commit Bot 2009-10-25 21:33:13 PDT
All reviewed patches have been landed.  Closing bug.