Bug 22345 - Abstract away JSC:: usage in WebCore/loader
: Abstract away JSC:: usage in WebCore/loader
Status: RESOLVED FIXED
Product: WebKit
Classification: Unclassified
Component: WebCore Misc.
: 528+ (Nightly build)
: All All
: P2 Normal
Assigned To: Darin Fisher (:fishd, Google)
:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2008-11-18 15:23 PST by Darin Fisher (:fishd, Google)
Modified: 2008-11-19 18:26 PST (History)
2 users (show)

See Also:


Attachments
webcore changes (6.21 KB, patch)
2008-11-18 15:33 PST, Darin Fisher (:fishd, Google)
ggaren: review-
Details | Formatted Diff | Diff
webkit changes (4.23 KB, patch)
2008-11-18 15:36 PST, Darin Fisher (:fishd, Google)
ggaren: review+
Details | Formatted Diff | Diff
webcore changes - v2 (17.45 KB, patch)
2008-11-18 18:29 PST, Darin Fisher (:fishd, Google)
ggaren: review+
Details | Formatted Diff | Diff
webkit changes - v2 (5.20 KB, patch)
2008-11-18 18:31 PST, Darin Fisher (:fishd, Google)
ggaren: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Darin Fisher (:fishd, Google) 2008-11-18 15:23:15 PST
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.
Comment 1 Darin Fisher (:fishd, Google) 2008-11-18 15:33:30 PST
Created attachment 25250 [details]
webcore changes
Comment 2 Darin Fisher (:fishd, Google) 2008-11-18 15:36:36 PST
Created attachment 25251 [details]
webkit changes
Comment 3 Eric Seidel 2008-11-18 15:46:01 PST
Comment on attachment 25250 [details]
webcore changes

Seems sane enough to me.
Comment 4 Eric Seidel 2008-11-18 15:47:03 PST
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.
Comment 5 Darin Fisher (:fishd, Google) 2008-11-18 16:08:37 PST
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.
Comment 6 Geoffrey Garen 2008-11-18 16:28:09 PST
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.
Comment 7 Darin Fisher (:fishd, Google) 2008-11-18 16:45:55 PST
> 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.)
Comment 8 Darin Fisher (:fishd, Google) 2008-11-18 18:29:54 PST
Created attachment 25256 [details]
webcore changes - v2
Comment 9 Darin Fisher (:fishd, Google) 2008-11-18 18:31:26 PST
Created attachment 25257 [details]
webkit changes - v2

trivial:  toJSValue -> jsValue
Comment 10 Geoffrey Garen 2008-11-19 15:34:03 PST
Comment on attachment 25256 [details]
webcore changes - v2

r=me
Comment 11 Geoffrey Garen 2008-11-19 15:34:09 PST
Comment on attachment 25257 [details]
webkit changes - v2

r=me
Comment 12 Darin Fisher (:fishd, Google) 2008-11-19 18:06:29 PST
http://trac.webkit.org/changeset/38610
Comment 13 Darin Fisher (:fishd, Google) 2008-11-19 18:26:55 PST
FYI, this caused some bustage for the ports.  I filed bug 22373 for the fix.