LGTM Minor suggestions below.
>> ======================================================================== >> http://mondrian.corp.google.com/file/8165297///depot/googleclient/gears/opensource/gears/base/common/js_types.h?a=2 >> File //depot/googleclient/gears/opensource/gears/base/common/js_types.h >> (snapshot 2) >> ------------------------------------ >> Line 531: // of a native object. Arguments may be required or optional and >> these types >> Required and optional args can't really be interspersed freely, right? I >> think >> it's more like zero or more required arguments, followed by zero or more >> optional arguments. > The new version of GetArguments does allow them to be interspersed. > I've added to testPassArgumentsOptional() to test this behaviour. Note > that none of the existing call sites make use of this. Ah, I see why I was confused. Previously, 'optional' meant whether the arguments were _present_. Now, an optional argument can be present but 'null'. If there's some good way to clarify the new behavior in the comment, it might be worthwhile. For example, the 3rd and 4th sentences could read: "Required arguments must always be specified, and they cannot be null or undefined. Optional arguments may be null or undefined, or they can be omitted entirely if not followed by any required arguments." >> ======================================================================== >> http://mondrian.corp.google.com/file/8165297///depot/googleclient/gears/opensource/gears/ui/common/permissions_dialog.cc?a=1 >> File >> //depot/googleclient/gears/opensource/gears/ui/common/permissions_dialog.cc >> (snapshot 1) >> ------------------------------------ >> Line 163: extra_message = STRING16(L""); >> Is there any plan to set unspecified optional arguments to default values? > I don't think so, so long as the value of was_specified can be relied > upon. GetArguments won't change the values of arguments that aren't > specified, so the caller can set a default before making the call. Ah, so the code here is intentionally _clearing_ any value that may have been set, if the other arguments were not given? That makes sense now -- consider clarifying the comments (e.g. "Ignore" -> "Clear").
