Bug 95271

Summary: Replace JSC::UString by WTF::String
Product: WebKit Reporter: Benjamin Poulain <benjamin>
Component: JavaScriptCoreAssignee: Benjamin Poulain <benjamin>
Status: RESOLVED FIXED    
Severity: Normal CC: abarth, barraclough, ggaren, gns, gtk-ews, gyuyoung.kim, haraken, hausmann, japhet, mifenton, philn, rakuco, tmpsantos, webkit.review.bot, xan.lopez, zan
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Patch
none
Patch
none
Patch for GTK build fix
none
Patch
none
Patch ggaren: review+, gyuyoung.kim: commit-queue-

Description Benjamin Poulain 2012-08-28 17:54:06 PDT
JSC is gonna be sweeeeeet.
Comment 1 Benjamin Poulain 2012-08-28 18:20:17 PDT
Created attachment 161108 [details]
Patch
Comment 2 Benjamin Poulain 2012-08-28 18:21:43 PDT
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.
Comment 3 Geoffrey Garen 2012-08-28 19:18:03 PDT
Please test SunSpider before landing. It's heavy in the string library functions.
Comment 4 Benjamin Poulain 2012-08-28 19:22:48 PDT
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.
Comment 5 Gyuyoung Kim 2012-08-28 19:39:26 PDT
Comment on attachment 161108 [details]
Patch

Attachment 161108 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/13683016
Comment 6 Gyuyoung Kim 2012-08-28 23:27:01 PDT
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.
Comment 7 Benjamin Poulain 2012-08-28 23:56:52 PDT
> > 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.
Comment 8 Benjamin Poulain 2012-08-29 21:30:09 PDT
Created attachment 161396 [details]
Patch
Comment 9 Benjamin Poulain 2012-08-29 21:36:19 PDT
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.
Comment 10 Simon Hausmann 2012-08-29 22:31:49 PDT
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.
Comment 11 Gyuyoung Kim 2012-08-29 22:57:56 PDT
Comment on attachment 161396 [details]
Patch

Attachment 161396 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/13694551
Comment 12 kov's GTK+ EWS bot 2012-08-29 22:58:34 PDT
Comment on attachment 161396 [details]
Patch

Attachment 161396 [details] did not pass gtk-ews (gtk):
Output: http://queues.webkit.org/results/13681556
Comment 13 Gyuyoung Kim 2012-08-29 23:03:41 PDT
Comment on attachment 161396 [details]
Patch

Attachment 161396 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/13683609
Comment 14 Zan Dobersek 2012-08-30 00:16:27 PDT
I'm looking into the GTK build failures and will hopefully produce a fix soon.
Comment 15 Zan Dobersek 2012-08-30 00:32:37 PDT
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.
Comment 16 Benjamin Poulain 2012-08-30 01:01:18 PDT
> 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.
Comment 17 Benjamin Poulain 2012-08-30 01:07:42 PDT
Created attachment 161423 [details]
Patch
Comment 18 Benjamin Poulain 2012-08-30 01:15:57 PDT
> 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.
Comment 19 Gyuyoung Kim 2012-08-30 02:30:36 PDT
Comment on attachment 161423 [details]
Patch

Attachment 161423 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/13693649
Comment 20 Gyuyoung Kim 2012-08-30 02:40:37 PDT
Comment on attachment 161423 [details]
Patch

Attachment 161423 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/13694610
Comment 21 Benjamin Poulain 2012-08-30 09:29:43 PDT
Created attachment 161492 [details]
Patch
Comment 22 Gyuyoung Kim 2012-08-30 11:03:39 PDT
Comment on attachment 161492 [details]
Patch

Attachment 161492 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/13691771
Comment 23 Gyuyoung Kim 2012-08-30 11:10:29 PDT
Comment on attachment 161492 [details]
Patch

Attachment 161492 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/13695756
Comment 24 Geoffrey Garen 2012-08-30 12:47:51 PDT
Comment on attachment 161492 [details]
Patch

r=me
Comment 25 Thiago Marcos P. Santos 2012-08-30 12:52:27 PDT
Benjamin,

May I have 10 minutes just to try the patch locally here on EFL?
Comment 26 Thiago Marcos P. Santos 2012-08-30 13:16:03 PDT
(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.
Comment 27 Thiago Marcos P. Santos 2012-08-30 13:36:08 PDT
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);
     }
Comment 28 Benjamin Poulain 2012-08-30 14:23:37 PDT
Committed r127191: <http://trac.webkit.org/changeset/127191>