Only looked at elemental.json.* so far. Looks like the Js implementations haven't been used much in DevMode (which is not really surprising given the super-source implementation of element.json.Json, which is @GwtScriptOnly, so only an explicit 'new JsJsonFactory()', or casting a JSO to a JsonValue, would use it), despite trying to support it.
BTW, elemental.js.json.* isn't on the SVN. http://gwt-code-reviews.appspot.com/1728806/diff/1/elemental/src/elemental/js/json/JsJsonArray.java File elemental/src/elemental/js/json/JsJsonArray.java (right): http://gwt-code-reviews.appspot.com/1728806/diff/1/elemental/src/elemental/js/json/JsJsonArray.java#newcode44 elemental/src/elemental/js/json/JsJsonArray.java:44: return this[index]; Should box() the value. (JsJsonObject box()es its value in get(String)) http://gwt-code-reviews.appspot.com/1728806/diff/1/elemental/src/elemental/js/json/JsJsonFactory.java File elemental/src/elemental/js/json/JsJsonFactory.java (right): http://gwt-code-reviews.appspot.com/1728806/diff/1/elemental/src/elemental/js/json/JsJsonFactory.java#newcode67 elemental/src/elemental/js/json/JsJsonFactory.java:67: if (typeof value === 'object') { Is this really necessary? Object() of an object should return the object unmodified, right? http://gwt-code-reviews.appspot.com/1728806/diff/1/elemental/src/elemental/js/json/JsJsonFactory.java#newcode70 elemental/src/elemental/js/json/JsJsonFactory.java:70: return Object(value); Is that needed in prod mode? shouldn't JsJsonValue.box() be used here? Actually, is that needed at all, given both JsJsonArray and JsJsonObject 'box' their values before returning them? All that's needed is to box() the returned value, in case you parse a 'primitive' (e.g. parse("true")) http://gwt-code-reviews.appspot.com/1728806/diff/1/elemental/src/elemental/js/json/JsJsonValue.java File elemental/src/elemental/js/json/JsJsonValue.java (right): http://gwt-code-reviews.appspot.com/1728806/diff/1/elemental/src/elemental/js/json/JsJsonValue.java#newcode44 elemental/src/elemental/js/json/JsJsonValue.java:44: return Object.prototype.toString.apply(@elemental.js.json.JsJsonValue::debox(Lelemental/json/JsonValue;)(obj)) === '[object Array]'; How about Array.isArray(@...debox(...)(obj)) ? It's supported in Safari 5+ and iOS 4+ (Android 1.5+ and Chrome 5+) (not sure there's a need to "debox" for an array though). http://gwt-code-reviews.appspot.com/1728806/diff/1/elemental/src/elemental/js/json/JsJsonValue.java#newcode48 elemental/src/elemental/js/json/JsJsonValue.java:48: // TODO(cromwellian): if this moves to GWT, we may have to support more leniency Looks like it just moved to GWT ;-) http://gwt-code-reviews.appspot.com/1728806/diff/1/elemental/src/elemental/js/json/JsJsonValue.java#newcode59 elemental/src/elemental/js/json/JsJsonValue.java:59: ([email protected]::debox(Lelemental/json/JsonValue;)(this)).valueOf(); valueOf() should be called *before* the "!!": (!!new Boolean(true)).valueOf() -> false (!!new Boolean(false)).valueOf() -> false !!(new Boolean(true).valueOf()) -> true !!(new Boolean(false).valueOf()) -> false Because every JS object has a valueOf() method, that should Just Workâ˘. http://gwt-code-reviews.appspot.com/1728806/diff/1/elemental/src/elemental/js/json/JsJsonValue.java#newcode66 elemental/src/elemental/js/json/JsJsonValue.java:66: ([email protected]::debox(Lelemental/json/JsonValue;)(this)).valueOf(); Same here: valueOf() should be called *before* the coercion with '+'. http://gwt-code-reviews.appspot.com/1728806/diff/1/elemental/src/elemental/js/json/JsJsonValue.java#newcode114 elemental/src/elemental/js/json/JsJsonValue.java:114: return @elemental.js.json.JsJsonValue::debox(Lelemental/json/JsonValue;)(this); Why use JSNI here? http://gwt-code-reviews.appspot.com/1728806/diff/1/elemental/src/elemental/json/impl/JreJsonFactory.java File elemental/src/elemental/json/impl/JreJsonFactory.java (right): http://gwt-code-reviews.appspot.com/1728806/diff/1/elemental/src/elemental/json/impl/JreJsonFactory.java#newcode30 elemental/src/elemental/json/impl/JreJsonFactory.java:30: * Implementation of JsonFactory interface using org.json library. "using org.json library" ?! ;-) http://gwt-code-reviews.appspot.com/1728806/diff/1/elemental/src/elemental/json/impl/JreJsonFactory.java#newcode61 elemental/src/elemental/json/impl/JreJsonFactory.java:61: // some clients send in (json) expecting an eval is required I don't see this in JsJsonFactory, and JSON.parse() chokes on the '(' (tested in Chrome 21) http://gwt-code-reviews.appspot.com/1728806/diff/1/elemental/src/elemental/json/impl/JreJsonString.java File elemental/src/elemental/json/impl/JreJsonString.java (right): http://gwt-code-reviews.appspot.com/1728806/diff/1/elemental/src/elemental/json/impl/JreJsonString.java#newcode70 elemental/src/elemental/json/impl/JreJsonString.java:70: return getObject().equals(((JreJsonValue)value).getObject()); Is it me or all JreJsonValue subclasses have this exact same method? (except JreJsonNull) Couldn't it be moved to JreJsonValue? http://gwt-code-reviews.appspot.com/1728806/diff/1/elemental/src/elemental/json/impl/JsonUtil.java File elemental/src/elemental/json/impl/JsonUtil.java (right): http://gwt-code-reviews.appspot.com/1728806/diff/1/elemental/src/elemental/json/impl/JsonUtil.java#newcode53 elemental/src/elemental/json/impl/JsonUtil.java:53: toSkip.add("__gwt_ObjectId"); __gwt_ObjectId is not excluded in the JS implementation, is that an oversight? (also, having $H and __gwt_ObjectId on the server-side / plain JVM looks weird, but I can understand the need to have the same behavior as on the client-side, if anyone explicitly uses one of these keys) http://gwt-code-reviews.appspot.com/1728806/diff/1/elemental/src/elemental/json/impl/JsonUtil.java#newcode188 elemental/src/elemental/json/impl/JsonUtil.java:188: if (n.endsWith(".0")) { Too bad this is not shared with JreJsonValue, which does the exact same thing in its toJson() method. http://gwt-code-reviews.appspot.com/1728806/diff/1/elemental/src/elemental/json/impl/JsonUtil.java#newcode265 elemental/src/elemental/json/impl/JsonUtil.java:265: return stringify(jsonValue, 0); How about 'stringify(jsonValue, "")' or 'stringify(jsonValue, null)' here to avoid creating a StringBuffer just to construct an empty string? http://gwt-code-reviews.appspot.com/1728806/diff/1/elemental/src/elemental/json/impl/JsonUtil.java#newcode276 elemental/src/elemental/json/impl/JsonUtil.java:276: StringBuffer sb = new StringBuffer(); Why a StringBuffer rather than a StringBuilder? (the javadoc says StringBuilder is faster if you don't need synchronized access) The StringBuffer/StringBuilder should also probably be initialized with a capacity equal to 'spaces', to avoid allocating too much space. http://gwt-code-reviews.appspot.com/1728806/diff/1/elemental/src/elemental/json/impl/JsonUtil.java#newcode302 elemental/src/elemental/json/impl/JsonUtil.java:302: String hexValue = Integer.toString(match.charAt(0), 16); What's the purpose of passing a String here? All call places have a 'char' and use String.valueOf(c), and here we start by doing a charAt(0). Wouldn't it be faster/easier to simply pass a 'char'? http://gwt-code-reviews.appspot.com/1728806/diff/1/elemental/src/elemental/json/impl/JsonUtil.java#newcode303 elemental/src/elemental/json/impl/JsonUtil.java:303: hexValue = hexValue.length() > 4 ? hexValue.substring(hexValue.length() - 4) Can it ever happen that a 'char' is encoded into more than 4 hex digits? http://gwt-code-reviews.appspot.com/1728806/diff/1/elemental/src/elemental/json/impl/JsonUtil.java#newcode305 elemental/src/elemental/json/impl/JsonUtil.java:305: return "\\u0000" + hexValue; I must say I don't understand the presence of these 4 zeros. I bet the author had a .substring() in mind but forgot it: String template = "\\u0000"; return template.substring(0, template.length() - hexValue.length()) + hexValue; http://gwt-code-reviews.appspot.com/1728806/diff/1/elemental/src/elemental/json/impl/JsonUtil.java#newcode328 elemental/src/elemental/json/impl/JsonUtil.java:328: private static String replace(RegExp expression, String text, This method does not seem to be used. http://gwt-code-reviews.appspot.com/1728806/ -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
