Bug 22390 – Abstract away JSC:: usage in WebCore/xml

• Status: RESOLVED
• Resolution: FIXED
• Component: WebCore Misc.

[Darin Fisher (:fishd, Google)]

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.

[Darin Fisher (:fishd, Google)]

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.

[Sam Weinig]

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.

[Sam Weinig]

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.

[Darin Fisher (:fishd, Google)]

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}!

[Darin Fisher (:fishd, Google)]

Whoops, I left some spurious changes to JSDOMBinding.{h,cpp} in that patch.  Revision coming up.

[Darin Fisher (:fishd, Google)]

Created attachment 25340 [details]
v2.1 patch

[Geoffrey Garen]

Comment on attachment 25340 [details]
v2.1 patch

Looks good to me.

[Darin Fisher (:fishd, Google)]

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