Summary: | Abstract away JSC:: usage in WebCore/loader | ||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Darin Fisher (:fishd, Google) <fishd> | ||||||||||
Component: | WebCore Misc. | Assignee: | Darin Fisher (:fishd, Google) <fishd> | ||||||||||
Status: | RESOLVED FIXED | ||||||||||||
Severity: | Normal | CC: | ap, dglazkov | ||||||||||
Priority: | P2 | ||||||||||||
Version: | 528+ (Nightly build) | ||||||||||||
Hardware: | All | ||||||||||||
OS: | All | ||||||||||||
Attachments: |
|
Description
Darin Fisher (:fishd, Google)
2008-11-18 15:23:15 PST
Created attachment 25250 [details]
webcore changes
Created attachment 25251 [details]
webkit changes
Comment on attachment 25250 [details]
webcore changes
Seems sane enough to me.
Comment on attachment 25251 [details]
webkit changes
Makes sense given the other patch. It's really the first one which needs a comment from one of the JSC folks.
The question came up in #webkit about why we don't just create a fake JSC::JSValue struct to hold the underlying V8 object. The reason lies with the fact that V8 has a handle-based GC. It is necessary for us to dispose of the handle when it is no longer needed. This means that we need to track references to the V8 object. Because JSValue pointers are copied around, it is not possible to know how many references exist to a particular JSValue object. For this reason, the current patch proposes replacing raw JSValue pointers with a "smart" pointer class, named ScriptValue. To match the WebKit coding style, let's put ScriptValue in its own file. In JavaScriptCore, it's important to distinguish between temporary stack data and heap data. Heap data must be protected from GC explicitly, using a JSC::ProtectedPtr. Since ScriptValue is an abstraction that allows the programmer to forget about JSValue*, I think it needs to be robust in both situations. So, please use JSC::ProtectedPtr. Minor nit: I would name "toJSValue" just plain "jsValue", since it doesn't perform a conversion. It's really hard to see how this abstraction works, since the code that uses it isn't in the WebKit tree. Do you plan to commit the Chrome code that uses ScriptValue? r- because of ProtectedPtr. > To match the WebKit coding style, let's put ScriptValue in its own file. Sure thing! > In JavaScriptCore, it's important to distinguish between temporary stack data > and heap data. Heap data must be protected from GC explicitly, using a > JSC::ProtectedPtr. Since ScriptValue is an abstraction that allows the > programmer to forget about JSValue*, I think it needs to be robust in both > situations. So, please use JSC::ProtectedPtr. OK. > Minor nit: I would name "toJSValue" just plain "jsValue", since it doesn't > perform a conversion. OK. > It's really hard to see how this abstraction works, since the code that uses > it isn't in the WebKit tree. Do you plan to commit the Chrome code that uses > ScriptValue? Yes, definitely. We are in the process of cleaning up our port so that it can be upstreamed. Presently, it has too many ties back to our source tree, which makes it not yet ready to upstream. You can see what we did locally to FrameLoader.cpp in the V8 case here: http://build.chromium.org/merge/WebCore-loader-FrameLoader.cpp-before.html (that is a slightly old representation of our mods to that file.) Created attachment 25256 [details]
webcore changes - v2
Created attachment 25257 [details]
webkit changes - v2
trivial: toJSValue -> jsValue
Comment on attachment 25256 [details]
webcore changes - v2
r=me
Comment on attachment 25257 [details]
webkit changes - v2
r=me
FYI, this caused some bustage for the ports. I filed bug 22373 for the fix. |