Use JSValue::toWTFString instead of calling toString(exec) and value(exec)
Created attachment 296996 [details] Patch
Created attachment 296997 [details] Patch
Created attachment 296999 [details] Patch
Comment on attachment 296999 [details] Patch Looks like you have some build issues but r=me if it builds.
Committed r209792: <http://trac.webkit.org/changeset/209792>
Re-opened since this is blocked by bug 165841
Created attachment 297070 [details] Patch Patch for landing
Committed r209801: <http://trac.webkit.org/changeset/209801>
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?
(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!
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.
(In reply to comment #11) > I found quite a few call sites that were calling JSValue::isString first Patch in bug 165895 now.