Hello nigeltao,
There may be semantics with marshall/unmarshall that i'm not familiar with? It
looks like what comes out can be different than what went in, with some object
properties and array elements missing from what comes out. Is that intended or
a bug?
I'd like you to do a code review. Please execute
g4 diff -c 9455628
or point your web browser to
http://mondrian/9455628
to review the following code:
Change 9455628 by micha...@michaeln-scour2 on 2008/12/17 14:48:45 *pending*
Make failure to unmarshall an array element cause the unmarshalling of
the array itself to fail. Previously these were silent failures such that the
elements that did fail would appear as undefined in the resulting array (and
perhaps shorter than expected array).
R=nigeltao
CC=gears-internal,[email protected]
DELTA=6 (5 added, 0 deleted, 1 changed)
OCL=9455628
Affected files ...
... //depot/googleclient/gears/opensource/gears/base/common/js_marshal.cc#17
edit
6 delta lines: 5 added, 0 deleted, 1 changed
Also consider running:
g4 lint -c 9455628
which verifies that the changelist doesn't introduce new style violations.
If you can't do the review, please let me know as soon as possible. During
your review, please ensure that all new code has corresponding unit tests and
that existing unit tests are updated appropriately. Visit
http://www/eng/code_review.html for more information.
This is a semiautomated message from "g4 mail". Complaints or suggestions?
Mail [email protected].
Change 9455628 by micha...@michaeln-scour2 on 2008/12/17 14:48:45 *pending*
Make failure to unmarshall an array element cause the unmarshalling of
the array itself to fail. Previously these were silent failures such that the
elements that did fail would appear as undefined in the resulting array (and
perhaps shorter than expected array).
Affected files ...
... //depot/googleclient/gears/opensource/gears/base/common/js_marshal.cc#17
edit
==== //depot/googleclient/gears/opensource/gears/base/common/js_marshal.cc#17 -
C:\p4\michaeln-scour2/googleclient/gears/opensource/gears/base/common/js_marshal.cc
====
# action=edit type=text
--- googleclient/gears/opensource/gears/base/common/js_marshal.cc
2008-12-17 15:26:36.000000000 -0800
+++ googleclient/gears/opensource/gears/base/common/js_marshal.cc
2008-12-17 14:48:12.000000000 -0800
@@ -263,6 +263,7 @@
return false;
}
object->SetProperty(i->first, property_value);
+ // TODO(michaeln): Should we be looking at SetProperty's return value?
}
success = true;
break;
@@ -279,8 +280,12 @@
for (int i = 0; i < n; i++) {
JsScopedToken token;
MarshaledJsToken *mjt = (*a)[i];
- if (mjt && mjt->Unmarshal(module_environment, &token)) {
+ if (mjt) {
+ if (!mjt->Unmarshal(module_environment, &token)) {
+ return false;
+ }
array->SetElement(i, token);
+ // TODO(michaeln): Should we be looking at SetElement's return value?
}
}
success = true;
