========================================================================
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
