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
anton muhin
2009-10-24 11:57:33 PDT
Created attachment 41787 [details]
First take
Adam, it looks safe, but I'd appreciate if you had another look at it.
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?
+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.). 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. (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. (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. > 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).
Created attachment 41830 [details]
2nd try
Comment on attachment 41830 [details]
2nd try
A patch of beauty!
Comment on attachment 41830 [details] 2nd try Clearing flags on attachment: 41830 Committed r50049: <http://trac.webkit.org/changeset/50049> All reviewed patches have been landed. Closing bug. |