LGTM -- Brad
On Wed, Sep 10, 2008 at 8:15 PM, Nigel Tao <[EMAIL PROTECTED]> wrote: > ======================================================================== > > http://mondrian.corp.google.com/file/8226669///depot/googleclient/gears/opensource/gears/blob/blob_builder_module.cc?a=1 > File > //depot/googleclient/gears/opensource/gears/blob/blob_builder_module.cc > (snapshot 1) > ------------------------------------ > Line 55: } else { > On Wed Sep 10 04:42:36 2008 PDT, bgarcia wrote: > > Should we confirm that appendee_type is JSPARAM_MODULE? > > This shouldn't be necessary - if appendee_type is not JSPARAM_MODULE, then > the > JsCallContext::GetArguments call a few lines down will fail. > ------------------------------------ > Line 86: if (!blob_interface.get()) { > On Wed Sep 10 04:46:59 2008 PDT, bgarcia wrote: > > You should be able to assert this. > > Done. > ======================================================================== > > http://mondrian.corp.google.com/file/8226669///depot/googleclient/gears/opensource/gears/blob/blob_builder_module.h?a=1 > File //depot/googleclient/gears/opensource/gears/blob/blob_builder_module.h > (snapshot 1) > ------------------------------------ > Line 31: #include "gears/blob/blob_interface.h" > On Wed Sep 10 04:39:07 2008 PDT, bgarcia wrote: > > This include doesn't appear to be necessary. > > Done. > ------------------------------------ > Line 32: #include "gears/blob/blob_builder.h" > On Wed Sep 10 04:39:35 2008 PDT, bgarcia wrote: > > This include can be replaced with a forward reference to BlobBuilder if > the > > constructor implementation is moved to the cc file. > > Done, although this does mean adding an explicit GearsBlobBuilder > destructor, > otherwise the implicit one in the .h file can't destroy the > scoped_ptr<BlobInterface> since BlobInterface is an incomplete class. > ------------------------------------ > Line 38: GearsBlobBuilder() > On Wed Sep 10 04:38:29 2008 PDT, bgarcia wrote: > > I suggest moving the constructor implementation to the .cc file. > > Done. > ------------------------------------ > Line 48: void AsBlob(JsCallContext *context); > On Wed Sep 10 04:48:22 2008 PDT, bgarcia wrote: > > Consider adding Get to name: GetAsBlob, or GetBlob? > > It's currently not clear if you're getting a blob or adding a blob from > the > > name. > > OK... I've gone with GetAsBlob. > ======================================================================== > > -- > To respond, reply to this email or visit > http://mondrian.corp.google.com/8226669 >
