Bug 165795 – Use JSValue::toWTFString instead of calling toString(exec) and value(exec)

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Yusuke Suzuki]

Use JSValue::toWTFString instead of calling toString(exec) and value(exec)

[Yusuke Suzuki]

Created attachment 296996 [details]
Patch

[Yusuke Suzuki]

Created attachment 296997 [details]
Patch

[Yusuke Suzuki]

Created attachment 296999 [details]
Patch

[Saam Barati]

Comment on attachment 296999 [details]
Patch

Looks like you have some build issues but r=me if it builds.

[Yusuke Suzuki]

Committed r209792: <http://trac.webkit.org/changeset/209792>

[WebKit Commit Bot]

Re-opened since this is blocked by bug 165841

[Yusuke Suzuki]

Created attachment 297070 [details]
Patch

Patch for landing

[Yusuke Suzuki]

Committed r209801: <http://trac.webkit.org/changeset/209801>

[Darin Adler]

So glad you did this Yusuke! Now I can delete my local git branch named "ToWTFString", or maybe I will merge it to see if I had any changes there on something you missed?

[Yusuke Suzuki]

(In reply to comment #9)
> So glad you did this Yusuke! Now I can delete my local git branch named
> "ToWTFString", or maybe I will merge it to see if I had any changes there on
> something you missed?

Nice! Can you check whether I missed something in your branch?
If your branch find a missed one, that's super awesome!

[Darin Adler]

Turns out my patch still has valuable changes, but it's hard to merge so I am not sure what I am going to do. Maybe just do the work below again.

I found quite a few call sites that were calling JSValue::isString first, then calling JSValue::toString or JSValue::toWTFString. Those could call JSValue::getString instead, which should be a bit more efficient. That has an additional benefit because we know those call sites cannot generate exceptions, so we can remove unneeded RETURN_IF_EXCEPTION or any other branching based on the exception.

In a couple other places, I did something similar except that I changed code to use the asString function rather than JSValue::getString.

At call sites that were calling Identifier::toString on the result, I changed them to call JSString::toIdentifier instead of JSString::value or JSValue::toWTFString, hoping to save on allocating a string that already exists in the atomic string table.

Another thing the patch had was that I had changed toWTFString to return a null string any time there is an exception, so the caller could check the string for null instead of checking the ExecState for an exception. That was possibly a valuable optimization back when I originally wrote this before we started using ThrowScope, but presumably is not needed now.

One other thing the patch had is that I also noticed that WebCore::Dictionary::getWithUndefinedOrNullCheck will return true when it encounters an exception in toWTFString, and I am not sure that is what we want, so I made it return false in that case in the patch.

[Darin Adler]

(In reply to comment #11)
> I found quite a few call sites that were calling JSValue::isString first

Patch in bug 165895 now.