Bug 22390 - Abstract away JSC:: usage in WebCore/xml
Summary: Abstract away JSC:: usage in WebCore/xml
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebCore Misc. (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Darin Fisher (:fishd, Google)
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2008-11-20 18:27 PST by Darin Fisher (:fishd, Google)
Modified: 2008-11-21 16:12 PST (History)
3 users (show)

See Also:


Attachments
v1 patch (11.89 KB, patch)
2008-11-20 18:42 PST, Darin Fisher (:fishd, Google)
no flags Details | Formatted Diff | Diff
v2 patch (11.03 KB, patch)
2008-11-20 22:24 PST, Darin Fisher (:fishd, Google)
no flags Details | Formatted Diff | Diff
v2.1 patch (10.17 KB, patch)
2008-11-20 22:34 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-20 18:27:52 PST
Abstract away JSC:: usage in WebCore/xml

There is just a small bit of XMLHttpRequest code that uses JSC::Value*
directly.  It uses it to provide a fast responseText getter to JS.

This is a proposal to define a ScriptString class that acts like a holder for a
JSC::UString.  XMLHttpRequest::m_responseText is modified to be a ScriptString instead of a JSC::UString.  ScriptString can be implicitly converted to UString for convenience.

Along the way, I found that there is no UString specific jsStringOrNull, which means that the optimization of storing responseText as a UString was basically being defeated due to an implicit conversion to String.  It turns out that providing a jsStringOrNull that takes a UString causes 'ambiguous implicit conversion' errors elsewhere (see JSElement.cpp), but using ScriptString works fine, so I have done that to help performance of accessing responseText.

If this proposal looks good, then I'll do a follow-up patch that applies ScriptString elsewhere.  For now, I am just focused on the WebCore/xml directory.
Comment 1 Darin Fisher (:fishd, Google) 2008-11-20 18:42:16 PST
Created attachment 25328 [details]
v1 patch

This patch also includes some const fixes to InspectorController that enable implicit conversion from ScriptString to UString work (and it's just a good idea to pass these as const references anyways).

I was not able to completely abstract away JSC from XMLHttpRequest, so I moved a few bits into #if USE(JSC).  Those bits do not need a V8 equivalent.

I decided to change XMLHttpRequest::m_lastSendURL into a WebCore::String since it is only used as a String.
Comment 2 Sam Weinig 2008-11-20 21:05:39 PST
If the UString stored in XMLHttpRequest is being converted to a WebCore::String, this is a regression.  This probably happened when converting JSXMLHttpRequest to be autogenerated a few months back.  We should perf test it again, as I know in the past this optimization has been a good win.
Comment 3 Sam Weinig 2008-11-20 21:18:58 PST
Indeed, looking at the 3.2 branch, the old code for responseText was:

            JSValue* result = jsOwnedStringOrNull(m_impl->getResponseText(ec));
            setDOMException(exec, ec);

I have filed bug 22392 to track fixing it.
Comment 4 Darin Fisher (:fishd, Google) 2008-11-20 22:24:32 PST
Created attachment 25339 [details]
v2 patch

Adjusted the patch to take into account Sam's change.  Now, it is not necessary to augment JSDOMBinding.{h,cpp}!
Comment 5 Darin Fisher (:fishd, Google) 2008-11-20 22:29:08 PST
Whoops, I left some spurious changes to JSDOMBinding.{h,cpp} in that patch.  Revision coming up.
Comment 6 Darin Fisher (:fishd, Google) 2008-11-20 22:34:39 PST
Created attachment 25340 [details]
v2.1 patch
Comment 7 Geoffrey Garen 2008-11-21 15:55:08 PST
Comment on attachment 25340 [details]
v2.1 patch

Looks good to me.
Comment 8 Darin Fisher (:fishd, Google) 2008-11-21 16:12:35 PST
http://trac.webkit.org/changeset/38680