Bug 22345 - Abstract away JSC:: usage in WebCore/loader
: Abstract away JSC:: usage in WebCore/loader
Status: RESOLVED FIXED
: WebKit
WebCore Misc.
: 528+ (Nightly build)
: All All
: P2 Normal
Assigned To:
:
:
:
:
  Show dependency treegraph
 
Reported: 2008-11-18 15:23 PST by
Modified: 2008-11-19 18:26 PST (History)


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


Note

You need to log in before you can comment on or make changes to this bug.


Description From 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 From 2008-11-18 15:33:30 PST -------
Created an attachment (id=25250) [details]
webcore changes
------- Comment #2 From 2008-11-18 15:36:36 PST -------
Created an attachment (id=25251) [details]
webkit changes
------- Comment #3 From 2008-11-18 15:46:01 PST -------
(From update of attachment 25250 [details])
Seems sane enough to me.
------- Comment #4 From 2008-11-18 15:47:03 PST -------
(From update of attachment 25251 [details])
Makes sense given the other patch.  It's really the first one which needs a comment from one of the JSC folks.
------- Comment #5 From 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 From 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 From 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 From 2008-11-18 18:29:54 PST -------
Created an attachment (id=25256) [details]
webcore changes - v2
------- Comment #9 From 2008-11-18 18:31:26 PST -------
Created an attachment (id=25257) [details]
webkit changes - v2

trivial:  toJSValue -> jsValue
------- Comment #10 From 2008-11-19 15:34:03 PST -------
(From update of attachment 25256 [details])
r=me
------- Comment #11 From 2008-11-19 15:34:09 PST -------
(From update of attachment 25257 [details])
r=me
------- Comment #12 From 2008-11-19 18:06:29 PST -------
http://trac.webkit.org/changeset/38610
------- Comment #13 From 2008-11-19 18:26:55 PST -------
FYI, this caused some bustage for the ports.  I filed bug 22373 for the fix.