Bug 95271 – Replace JSC::UString by WTF::String

• Status: RESOLVED
• Resolution: FIXED
• Component: JavaScriptCore

[Benjamin Poulain]

JSC is gonna be sweeeeeet.

[Benjamin Poulain]

Created attachment 161108 [details]
Patch

[Benjamin Poulain]

The EFL EWS is gonna fail, there is nothing I can do about it with the patch. The bot does not regenerate the bindings and it fails to build because the old generated files reference UString.

[Geoffrey Garen]

Please test SunSpider before landing. It's heavy in the string library functions.

[Benjamin Poulain]

Thanks for the review, I was not expecting one tonight! :)

(In reply to comment #3)
> Please test SunSpider before landing. It's heavy in the string library functions.

Will do.

[Gyuyoung Kim]

Comment on attachment 161108 [details]
Patch

Attachment 161108 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/13683016

[Gyuyoung Kim]

Comment on attachment 161108 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=161108&action=review

> Source/WebCore/bindings/js/JSIntentConstructor.cpp:68
> +    const String type(MAYBE_MISSING_PARAMETER(exec, 1, DefaultIsUndefined).isEmpty() ? String() : MAYBE_MISSING_PARAMETER(exec, 1, DefaultIsUndefined).toWTFString(exec)));

It looks unneeded ")" is added at the end of line. When I remove ")", EFL build is fine.

[Benjamin Poulain]

> > Source/WebCore/bindings/js/JSIntentConstructor.cpp:68
> > +    const String type(MAYBE_MISSING_PARAMETER(exec, 1, DefaultIsUndefined).isEmpty() ? String() : MAYBE_MISSING_PARAMETER(exec, 1, DefaultIsUndefined).toWTFString(exec)));
> 
> It looks unneeded ")" is added at the end of line. When I remove ")", EFL build is fine.

Thanks for looking! I'll make an update tomorrow.

[Benjamin Poulain]

Created attachment 161396 [details]
Patch

[Benjamin Poulain]

Changes since last patch:
-Added getCharactersWithUpconvert<>() because the behavior of UString::getCharacters<>() was slightly different and we need the other behavior for StringPrototype.
-Added back WebCore::jsString, renamed it to jsStringWithCache so it is easier to differentiate when to use which function.

I haven't measured any difference on Sunspider with this version. Gavin gave me some tricks to perform more perf testing, I'll try that before landing.

The WebCore binary got even smaller with the changes and I have no clue why.

The EFL bot is covered, the GTK bot will be.

[Simon Hausmann]

Totally awesome stuff :)

(In reply to comment #9)
> The WebCore binary got even smaller with the changes and I have no clue why.

Perhaps it is the reduced number of entries in the PLT that contributes to the reduction in size.

[Gyuyoung Kim]

Comment on attachment 161396 [details]
Patch

Attachment 161396 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/13694551

[kov's GTK+ EWS bot]

Comment on attachment 161396 [details]
Patch

Attachment 161396 [details] did not pass gtk-ews (gtk):
Output: http://queues.webkit.org/results/13681556

[Gyuyoung Kim]

Comment on attachment 161396 [details]
Patch

Attachment 161396 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/13683609

[Zan Dobersek]

I'm looking into the GTK build failures and will hopefully produce a fix soon.

[Zan Dobersek]

Created attachment 161411 [details]
Patch for GTK build fix

The symbols.filter file just needs an update for the jsStringWithCacheSlowCase symbol. With that the build should be fixed.

[Benjamin Poulain]

> The symbols.filter file just needs an update for the jsStringWithCacheSlowCase symbol. With that the build should be fixed.

Aw snap. Thanks for the patch. I'll make an update.

[Benjamin Poulain]

Created attachment 161423 [details]
Patch

[Benjamin Poulain]

> Totally awesome stuff :)

Glad you like it :)

> Perhaps it is the reduced number of entries in the PLT that contributes to the reduction in size.

Maybe. I don't think I removed symbols between the two patches but I could have forgot.

[Gyuyoung Kim]

Comment on attachment 161423 [details]
Patch

Attachment 161423 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/13693649

[Gyuyoung Kim]

Comment on attachment 161423 [details]
Patch

Attachment 161423 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/13694610

[Benjamin Poulain]

Created attachment 161492 [details]
Patch

[Gyuyoung Kim]

Comment on attachment 161492 [details]
Patch

Attachment 161492 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/13691771

[Gyuyoung Kim]

Comment on attachment 161492 [details]
Patch

Attachment 161492 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/13695756

[Geoffrey Garen]

Comment on attachment 161492 [details]
Patch

r=me

[Thiago Marcos P. Santos]

Benjamin,

May I have 10 minutes just to try the patch locally here on EFL?

[Thiago Marcos P. Santos]

(In reply to comment #25)
> Benjamin,
> 
> May I have 10 minutes just to try the patch locally here on EFL?

It is indeed failing, I tried a clean build. Need a couple more minutes to fix.

[Thiago Marcos P. Santos]

This patch is good to go on EFL after the change bellow (but looks unrelated to the error reported by the bot). Please apply the change and ignore any further EFL EWS errors on this patch:

diff --git a/Source/WebCore/bindings/js/Dictionary.cpp b/Source/WebCore/bindings/js/Dictionary.cpp
index a16b5b9..e126b95 100644
--- a/Source/WebCore/bindings/js/Dictionary.cpp
+++ b/Source/WebCore/bindings/js/Dictionary.cpp
@@ -90,7 +90,7 @@ bool Dictionary::getOwnPropertyNames(WTF::Vector<String>& names) const
     PropertyNameArray propertyNames(exec);
     JSObject::getOwnPropertyNames(object, exec, propertyNames, ExcludeDontEnumProperties);
     for (PropertyNameArray::const_iterator it = propertyNames.begin(); it != propertyNames.end(); it++) {
-        String stringKey = ustringToString(it->ustring());
+        const String& stringKey = it->ustring();
         if (!stringKey.isEmpty())
             names.append(stringKey);
     }

[Benjamin Poulain]

Committed r127191: <http://trac.webkit.org/changeset/127191>