Add more of the server-side plumbing for extension APIs. Also

Add more of the server-side plumbing for extension APIs. Also
added getTabsForWindow() to drive it.

[Matt Perry, 2009-03-27 01:18:32]

Good stuff.  One nitpicky meta comment on naming: it bugs me that we have all
the classes/messages with API in the name (like SyncExtensionAPI), when each is
really only a handler for 1 function call.  API implies a set of functions.  It
would make more sense to have SyncExtensionFunctionHandler and
ViewMsg_Extension_SetFunctionNames, or something like that.  What do you think?

http://codereview.chromium.org/42680/diff/1034/1035
File base/json_reader.h (right):

http://codereview.chromium.org/42680/diff/1034/1035#newcode109
Line 109: // disable the root check.
this comment should be redone since the reasoning changed.

http://codereview.chromium.org/42680/diff/1034/1037
File chrome/browser/extensions/extension_api_handler.cc (right):

http://codereview.chromium.org/42680/diff/1034/1037#newcode10
Line 10: #include "base/singleton.h"
order

http://codereview.chromium.org/42680/diff/1034/1037#newcode18
Line 18: typedef SyncExtensionAPI*(*SyncExtensionAPIFactory)();
space before first (

http://codereview.chromium.org/42680/diff/1034/1037#newcode26
Line 26: SyncExtensionAPI* CreateSyncAPI(const std::string& name);
This name confuses me.  It sounds like it's creating a new API type, but really
you're just allocating an instance of a function handler.  Maybe something like
NewSyncFunctionHandler?  Ditto for the other Create*API functions.

http://codereview.chromium.org/42680/diff/1034/1037#newcode62
Line 62: Singleton<ExtensionAPIRegistry>::get()->GetAllAPINames(names);
nit: since you use the Singleton getter in multiple places, I'd add a static
get() function on ExtensionAPIRegistry itself and use that instead.

http://codereview.chromium.org/42680/diff/1034/1037#newcode77
Line 77: // valid JSON.
This may be true, but we don't want to crash the browser if a renderer bug
causes this to be invalid.  Rule of thumb: never trust a renderer to give you
valid data.

http://codereview.chromium.org/42680/diff/1034/1037#newcode81
Line 81: SyncExtensionAPI* api =
What's the lifetime of this guy?  Should it be scoped?

http://codereview.chromium.org/42680/diff/1034/1037#newcode91
Line 91: // TODO(aa): In case of failure, send the error message to an error
callback.
line length

http://codereview.chromium.org/42680/diff/1034/1038
File chrome/browser/extensions/extension_api_handler.h (right):

http://codereview.chromium.org/42680/diff/1034/1038#newcode28
Line 28: // Send a respnose to an API.
spelling

http://codereview.chromium.org/42680/diff/1034/1039
File chrome/browser/extensions/extension_tabs_module.cc (right):

http://codereview.chromium.org/42680/diff/1034/1039#newcode46
Line 46: result_.reset(Value::CreateIntegerValue(
I'd expect this to return a TabValue, just like GetTabsForWindow returns a list
of.

http://codereview.chromium.org/42680/diff/1034/1041
File chrome/browser/extensions/sync_extension_api.h (right):

http://codereview.chromium.org/42680/diff/1034/1041#newcode15
Line 15: class SyncExtensionAPI {
sync... that's an unfortunate name.  I expected it to be sync from the
perspective of the extension, not from the browser process.  I wonder if we
really need to make this distinction at all?  Maybe a "sync" extension API is
just an async one that calls its "i'm done" function right away, and from the
perspective of the ExtensionAPIHandler, there is no difference.

http://codereview.chromium.org/42680/diff/1034/1041#newcode24
Line 24: std::string* error() { return &error_; }
nit: since error can never return NULL, I'd return a const-ref rather than a
pointer.

http://codereview.chromium.org/42680/diff/1034/1046
File chrome/renderer/extensions/extension_process_bindings.cc (right):

http://codereview.chromium.org/42680/diff/1034/1046#newcode33
Line 33: else if (api_names_->find(*v8::String::AsciiValue(name)) !=
sweet!

http://codereview.chromium.org/42680/diff/1034/1050
File chrome/renderer/renderer.vcproj (right):

http://codereview.chromium.org/42680/diff/1034/1050#newcode188
Line 188: RelativePath=".\extensions\extension_process_bindings.cc"
looks like you can revert this (non)change.

[Aaron Boodman, 2009-03-27 08:55:16]

Ready for another look. I renamed the classes to ExtensionFunction,
ExtensionFunctionDispatcher, etc.

http://codereview.chromium.org/42680/diff/1034/1035
File base/json_reader.h (right):

http://codereview.chromium.org/42680/diff/1034/1035#newcode109
Line 109: // disable the root check.
On 2009/03/27 01:18:32, Matt Perry wrote:
> this comment should be redone since the reasoning changed.

Done.

http://codereview.chromium.org/42680/diff/1034/1037
File chrome/browser/extensions/extension_api_handler.cc (right):

http://codereview.chromium.org/42680/diff/1034/1037#newcode10
Line 10: #include "base/singleton.h"
On 2009/03/27 01:18:32, Matt Perry wrote:
> order

Done.

http://codereview.chromium.org/42680/diff/1034/1037#newcode18
Line 18: typedef SyncExtensionAPI*(*SyncExtensionAPIFactory)();
On 2009/03/27 01:18:32, Matt Perry wrote:
> space before first (

Done.

http://codereview.chromium.org/42680/diff/1034/1037#newcode26
Line 26: SyncExtensionAPI* CreateSyncAPI(const std::string& name);
On 2009/03/27 01:18:32, Matt Perry wrote:
> This name confuses me.  It sounds like it's creating a new API type, but
really
> you're just allocating an instance of a function handler.  Maybe something
like
> NewSyncFunctionHandler?  Ditto for the other Create*API functions.

Done.

http://codereview.chromium.org/42680/diff/1034/1037#newcode62
Line 62: Singleton<ExtensionAPIRegistry>::get()->GetAllAPINames(names);
On 2009/03/27 01:18:32, Matt Perry wrote:
> nit: since you use the Singleton getter in multiple places, I'd add a static
> get() function on ExtensionAPIRegistry itself and use that instead.

Done.

http://codereview.chromium.org/42680/diff/1034/1037#newcode77
Line 77: // valid JSON.
On 2009/03/27 01:18:32, Matt Perry wrote:
> This may be true, but we don't want to crash the browser if a renderer bug
> causes this to be invalid.  Rule of thumb: never trust a renderer to give you
> valid data.

Done.

http://codereview.chromium.org/42680/diff/1034/1037#newcode81
Line 81: SyncExtensionAPI* api =
On 2009/03/27 01:18:32, Matt Perry wrote:
> What's the lifetime of this guy?  Should it be scoped?

Whoops, thanks. Fixed.

http://codereview.chromium.org/42680/diff/1034/1037#newcode91
Line 91: // TODO(aa): In case of failure, send the error message to an error
callback.
On 2009/03/27 01:18:32, Matt Perry wrote:
> line length

Done.

http://codereview.chromium.org/42680/diff/1034/1038
File chrome/browser/extensions/extension_api_handler.h (right):

http://codereview.chromium.org/42680/diff/1034/1038#newcode28
Line 28: // Send a respnose to an API.
On 2009/03/27 01:18:32, Matt Perry wrote:
> spelling

Done.

http://codereview.chromium.org/42680/diff/1034/1039
File chrome/browser/extensions/extension_tabs_module.cc (right):

http://codereview.chromium.org/42680/diff/1034/1039#newcode46
Line 46: result_.reset(Value::CreateIntegerValue(
On 2009/03/27 01:18:32, Matt Perry wrote:
> I'd expect this to return a TabValue, just like GetTabsForWindow returns a
list
> of.

I had intended that, but just not gotten around to it. Fixed.

http://codereview.chromium.org/42680/diff/1034/1041
File chrome/browser/extensions/sync_extension_api.h (right):

http://codereview.chromium.org/42680/diff/1034/1041#newcode15
Line 15: class SyncExtensionAPI {
On 2009/03/27 01:18:32, Matt Perry wrote:
> sync... that's an unfortunate name.  I expected it to be sync from the
> perspective of the extension, not from the browser process.  I wonder if we
> really need to make this distinction at all?  Maybe a "sync" extension API is
> just an async one that calls its "i'm done" function right away, and from the
> perspective of the ExtensionAPIHandler, there is no difference.

I removed the distinction between sync and async in the new snapshot. It's a
little less convenient for writing sync functions, but we can try it out.

http://codereview.chromium.org/42680/diff/1034/1041#newcode24
Line 24: std::string* error() { return &error_; }
On 2009/03/27 01:18:32, Matt Perry wrote:
> nit: since error can never return NULL, I'd return a const-ref rather than a
> pointer.

Done.

http://codereview.chromium.org/42680/diff/1034/1050
File chrome/renderer/renderer.vcproj (right):

http://codereview.chromium.org/42680/diff/1034/1050#newcode188
Line 188: RelativePath=".\extensions\extension_process_bindings.cc"
On 2009/03/27 01:18:32, Matt Perry wrote:
> looks like you can revert this (non)change.

Done.

[Matt Perry, 2009-03-27 20:11:56]

Great, I like the new naming!  One last suggestion.  (the Peer thing is just an
idea for future work, not for this CL)

http://codereview.chromium.org/42680/diff/3007/3014
File chrome/browser/extensions/extension_function_dispatcher.cc (right):

http://codereview.chromium.org/42680/diff/3007/3014#newcode97
Line 97: // TODO(aa): This will get a bit more complicated when we support
functions
I had an idea about that last night.  The problem is that the
ExtensionFunctionDispatcher could be destroyed while the async function is
running.  One solution is to add a ExtensionFunctionDispatcherPeer, which is
refcounted, and pointed to by the EFD.  Give the Peer to functions instead of a
direct reference to the EFD.  When the EFD dies, it tells the Peer, and the Peer
can ignore any functions that try to send responses.

http://codereview.chromium.org/42680/diff/3007/3016
File chrome/browser/extensions/extension_tabs_module.cc (right):

http://codereview.chromium.org/42680/diff/3007/3016#newcode16
Line 16: SendResponse(false);
You're right, this is cumbersome.  Idea: on ExtensionFunction:
  virtual bool RunImpl() = 0;
  void Run() { SendResponse(RunImpl()); }

we'd have to change it for the async case, but at least:
- the sync case remains simple
- ExtensionFunctionDispatcher doesn't need to know the difference

[Aaron Boodman, 2009-03-27 21:02:49]

Ok, I introduced SyncExtensionFunction which is a bit more convenient for simple
function implementations.

[Matt Perry, 2009-03-27 21:10:41]

lgtm with scons projects fixed