+gears-eng
========================================================================
http://mondrian.corp.google.com/file/10579925///depot/googleclient/gears/opensource/gears/blob/blob_builder_module.cc?a=2
File //depot/googleclient/gears/opensource/gears/blob/blob_builder_module.cc
(snapshot 2)
------------------------------------
Line 75: } else if (type == JSPARAM_OBJECT) {
There's some FUD around the distinction between JSPARAM_OBJECT and
JSPARAM_MODULE. Maybe accept either value in this test?
------------------------------------
Line 97: if (array_stack == NULL) {
Oh... and nested arrays too... i was going to say it might be nice to stick
with
the higher level JsCallContext interface to retrieve arguments, but this
explains why you dropped down to the inscrutible AbstractJsToken voodoo.
You still may be able to stick more exlusively to higher level APIs by using
JsArray.GetElementType() and JsArray.GetElementAsXXX()?
A comment from js_types.h that i like...
// TODO(nigeltao): Eliminate this function. Only code inside js_types_xx.cc
// should manipulate or refer to JsToken instances. Currently, the only
other
// use of JsToken is in js_marshal.cc, and we should be able to fix up
// JsScopedToken so that the js_marshal code need not need to know what a
// (script-engine specific) JsToken is.
AbstractJsToken JsTokenPtrToAbstractJsToken(JsToken *token);
------------------------------------
Line 117: !Append(array_element, js_context, array_stack)) {
There's one significant issue with the new Append behavior. If the caller
passes
in an array with a bunch of valid elements but has some undefined or illegal
element types interspersed in there... Append will return false, but will
have
appended some stuff.
I think if the method fails, nothing should have been appended... all or
none.
========================================================================
--
To respond, reply to this email or visit
http://mondrian.corp.google.com/10579925
