Bug 22345 – Abstract away JSC:: usage in WebCore/loader

• Component: WebCore Misc.
• Status: RESOLVED FIXED

[Darin Fisher (:fishd, Google)]

Abstract away JSC:: usage in WebCore/loader

There is just a small bit of code in FrameLoader that uses JSC::Value* directly.  Most of the rest of the code just uses the ScriptController to abstract away the details of JSC.  With a small change, we can go the rest of the way.

This is a proposal to define a ScriptValue class that acts like a holder for a JSC::JSValue*.  ScriptController::evaluate is modified to return a ScriptValue instead of a JSC::JSValue*.  Consumer can either convert the ScriptValue to a String or get the underlying JSValue if they need to further inspect the value.

This change avoids forking WebCore/loader to support V8, and it also seems like a reasonable cleanup change consistent with the work that has already been done for ScriptController.

[Darin Fisher (:fishd, Google)]

Created attachment 25250 [details]
webcore changes

[Darin Fisher (:fishd, Google)]

Created attachment 25251 [details]
webkit changes

[Eric Seidel (no email)]

Comment on attachment 25250 [details]
webcore changes

Seems sane enough to me.

[Eric Seidel (no email)]

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.

[Darin Fisher (:fishd, Google)]

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.

[Geoffrey Garen]

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.

[Darin Fisher (:fishd, Google)]

> 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.)

[Darin Fisher (:fishd, Google)]

Created attachment 25256 [details]
webcore changes - v2

[Darin Fisher (:fishd, Google)]

Created attachment 25257 [details]
webkit changes - v2

trivial:  toJSValue -> jsValue

[Geoffrey Garen]

Comment on attachment 25256 [details]
webcore changes - v2

r=me

[Geoffrey Garen]

Comment on attachment 25257 [details]
webkit changes - v2

r=me

[Darin Fisher (:fishd, Google)]

http://trac.webkit.org/changeset/38610

[Darin Fisher (:fishd, Google)]

FYI, this caused some bustage for the ports.  I filed bug 22373 for the fix.