Bug 26340 – [Qt] Build break introduced by r44550

• Status: RESOLVED
• Resolution: DUPLICATE
• Component: JavaScriptCore

[Laszlo Gombos]

I found two problems:
 1. JavaScriptCore/runtime/JSONObject.cpp needs to be listed as a LUT_FILE as well
 2. name collision between WebCore/inspector/JSONObject.cpp and the newly created JavaScriptCore/runtime/JSONObject.cpp (all .o's are generated in the same dir for QtWebKit)

I was not sure how to resolve 2. I few ideas:
 - rename one of the JSONObject.cpp (e.g. to InspectorJSONObject.cpp)
 - create JavaScriptCore library first and and link it to WebCore to avoid collision

Will submit patch for 1. (LUT_FILE); this should at least fix the standalone jsc build.

[Laszlo Gombos]

Created attachment 31190 [details]
Add JSONObject.cpp to LUT files for JavaScripCore

[Jan Alonzo]

Comment on attachment 31190 [details]
Add JSONObject.cpp to LUT files for JavaScripCore

> --- JavaScriptCore/JavaScriptCore.pri	(revision 44621)
> +++ JavaScriptCore/JavaScriptCore.pri	(working copy)
> @@ -69,6 +69,7 @@ include(pcre/pcre.pri)
>  
>  LUT_FILES += \
>      runtime/DatePrototype.cpp \
> +    runtime/JSONObject.cpp \
>      runtime/NumberConstructor.cpp \
>      runtime/StringPrototype.cpp \
>      runtime/ArrayPrototype.cpp \

Looks fine. r=me.

[Laszlo Gombos]

Comment on attachment 31190 [details]
Add JSONObject.cpp to LUT files for JavaScripCore

Landed as http://trac.webkit.org/changeset/44623.

[Laszlo Gombos]

Based on some discussions on #webkit the proposal to resolve the remaining build break is to rename WebCore/inspector/JSONObject.cpp to InspectorJSONObject.cpp (and change the class name as well).

[Joseph Ligman]

Created attachment 31199 [details]
Fix Qt build break by rename JSONObject in inspector

I don't have all the ports so was unable to verify all builds.

[Joseph Ligman]

Created attachment 31204 [details]
Fix Qt build break by rename JSONObject in inspector


There was a problem with the ChangeLog in the previous patch. 
I don't have all the ports so I am unable to verify all builds.

[Dimitri Glazkov (Google)]

Did you use svn mv for the file move?

[Joseph Ligman]

Sorry I did not. I can do that, but maybe pfeldman will have some other comments. 

[Jan Alonzo]

Comment on attachment 31190 [details]
Add JSONObject.cpp to LUT files for JavaScripCore

Clearing review flag as the patch has landed.

[Joseph Ligman]

Created attachment 31235 [details]
Fix Qt build break by rename JSONObject in inspector

This one uses svn move instead of just add and delete

[Timothy Hatcher]

Now that I look at JSONObject, it is just to similar to ScriptObject. I think it can go, and just make ScriptObject support JSON stringify and creation from a JSON string. Dimitri, Pavel, do you think that would work?

[Timothy Hatcher]

*** This bug has been marked as a duplicate of 26293 ***

[Pavel Feldman]

(In reply to comment #11)
> Now that I look at JSONObject, it is just to similar to ScriptObject. I think
> it can go, and just make ScriptObject support JSON stringify and creation from
> a JSON string. Dimitri, Pavel, do you think that would work?
> 

I agree that there is a redundancy here. However, I am not sure that Script* is now exactly what we need, so we might need to tweak them a bit. To reduce the number of the moving parts, I'd suggest that this one is fixed as suggested (via renaming Inspector's one to a more specific name) now. In the meanwhile, I would like to learn more on the new stringify API in WebKit and find a good way of Script*, new JSON.stringify and Chromium's JSON capabilities playing together nicely.

[Dimitri Glazkov (Google)]

I am fine with Pavel's opinion. I kinda like Timothy's suggestion though. Color me ambivalent.

[Simon Hausmann]

Comment on attachment 31235 [details]
Fix Qt build break by rename JSONObject in inspector

Thanks Joe!

[Simon Hausmann]

Comment on attachment 31235 [details]
Fix Qt build break by rename JSONObject in inspector

Landed in 44682. Clearning review but leaving the bug open for further discussion around the json classes. Pavel, Timothy, do you prefer a separate bugzilla entry for that?

[Tor Arne Vestbø]

(In reply to comment #16)
> (From update of attachment 31235 [details] [review])
> Landed in 44682. Clearning review but leaving the bug open for further
> discussion around the json classes. Pavel, Timothy, do you prefer a separate
> bugzilla entry for that?

I think discussions are in https://bugs.webkit.org/show_bug.cgi?id=26293