Thomas Broyer has posted comments on this change.
Change subject: Use JSON.parse() instead of eval() to deserialize rpc
callback payload
......................................................................
Patch Set 4:
(6 comments)
Looking at the history, I stumbled on another case besides the .concat hack
where the protocol does not emit JSON but rather JSON-like JavaScript: the
ClientSerializationStreamReader in DevMode uses Rhino to eval() the
response, and Rhino has a hard limit on the length of a string literal, so
the server emits a concatenation of string literals in this case. See
https://code.google.com/p/google-web-toolkit/source/detail?r=10888
That means we'll want to:
* only emit those concatenated strings in getVersion() < 8
* parse the response without Rhino (and as pure JSON) when it's in version
8.
About detecting version 8, I wonder if we shouldn't change the response
format to include the version outside the JSON. I have no idea what impact
it would have on the code though, so maybe it's a bad idea and extracting
the version from the JSON like you did is the way to go.
But much more importantly: I'd wait for our decision whether we continue to
support IE6/7 before going any farther. If we drop IE6/7 support, we can
simplify the client *and* server code.
Also, currently the server-side code supports back to version 5 of the
GWT-RPC protocol; but that support is actually limited to accepting
requests from old clients, because responses are always sent with the last
known version (according to the changes made in this patch). Is this really
needed? should backwards-compatibility be kept at all? only for the last 2
versions? (7 and 8 here) Note that clients will choke just after parsing
the response because getVersion() doesn't exactly match what they know how
to deal with, so supporting older requests on the server-side means that
the RPC is executed but the client can't decode the response. I don't know
what's best here (vs. failing early), but we're not really concerned here,
as we're only changing the response format, so we can continue to support
old requests; I don't think it's worth supporting clients older than v7
though (cached/running clients while the server is being updated/redeployed)
FYI, ServerSerializationStreamWriter has some code that depends on
getVersion() in writeLong, which is why I though the changes would be
limited to that class. Digging further, that code was added in
https://code.google.com/p/google-web-toolkit/source/detail?r=8274 and
simply copied from AbstractSerializationStreamWriter (shared between client
and server), so I think it was an oversight.
FYI, backwards-compat on the server-side for decoding requests was added at
https://code.google.com/p/google-web-toolkit/source/detail?r=8269
....................................................
File
user/src/com/google/gwt/user/client/rpc/impl/AbstractSerializationStreamWriter.java
Line 92: if (Double.isNaN(fieldValue) || Double.isInfinite(fieldValue))
{
if (getVersion() > 7 && (Double.isNaN(fieldValue) ||
Double.isInfinite(fieldValue))
(except with a the '7' replaced by a constant in
AbstractSerializationStream)
....................................................
File
user/src/com/google/gwt/user/client/rpc/impl/ClientSerializationStreamWriter.java
Line 191: int version = isJsonParseSupported() ? getVersion() : 7;
Use a constant instead of '7' (see comment on
AbstractSerializationStreamWriter too)
....................................................
File
user/src/com/google/gwt/user/server/rpc/impl/ServerSerializationStreamWriter.java
Depending on the version, this class should decide whether to use
LengthConstrainedArray (the .concat hack) or not (or tweak its behavior to
enable/disable the .concat hack)
That way, we can decide on the client-side whether to use eval() or
JSON.parse and be sure that in the JSON.parse case (version 8) we should
have valid JSON as input.
Line 636: if (getVersion() < 8) {
Should be done in AbstractSerializationStreamWriter (see comment there)
....................................................
File
user/super/com/google/gwt/user/translatable/com/google/gwt/user/client/rpc/impl/ClientSerializationStreamReader.java
Line 61: if(version < 8){
Formatting (spaces):
if•(version < 8)•{
and use a constant instead of a "magic number".
Line 62: // Versions prior to 8 uses invalid JSON; e.g., using
].concat([ or non-stringified NaN/Infinity.
Versions prior to 8 use JavaScript, not JSON.
--
To view, visit https://gwt-review.googlesource.com/2900
To unsubscribe, visit https://gwt-review.googlesource.com/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I6062180397f5fabed1dd5f08140c2bd43a19fa9f
Gerrit-PatchSet: 4
Gerrit-Project: gwt
Gerrit-Branch: master
Gerrit-Owner: John Ahlroos <j...@vaadin.com>
Gerrit-Reviewer: Brian Slesinsky <skybr...@google.com>
Gerrit-Reviewer: Colin Alworth <niloc...@gmail.com>
Gerrit-Reviewer: John Ahlroos <j...@vaadin.com>
Gerrit-Reviewer: Leeroy Jenkins <jenk...@gwtproject.org>
Gerrit-Reviewer: Matthew Dempsky <mdemp...@google.com>
Gerrit-Reviewer: Thomas Broyer <t.bro...@gmail.com>
Gerrit-HasComments: Yes
--
http://groups.google.com/group/Google-Web-Toolkit-Contributors
---
You received this message because you are subscribed to the Google Groups "GWT Contributors" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to google-web-toolkit-contributors+unsubscr...@googlegroups.com.
For more options, visit https://groups.google.com/groups/opt_out.