Extract ExtensionRequestSender from SchemaGeneratedBindings.

Extract ExtensionRequestSender from SchemaGeneratedBindings.

This change is intended to make the functions in SchemaGeneratedBindings less
coupled, and the lifecycle of pending requests clearer. After this change it
should be possible to replace SchemaGeneratedBindings with several smaller,
more targeted subclasses of ChromeV8Extension, eg: SetIconNatives and
SendRequestNatives.


BUG=none
TEST=existing browser tests


Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=130343

[benwells, 2012-03-29 23:06:36]

drive by review... oh, nice name btw

https://chromiumcodereview.appspot.com/9903010/diff/1/chrome/renderer/extensi...
File chrome/renderer/extensions/extension_request_sender.cc (right):

https://chromiumcodereview.appspot.com/9903010/diff/1/chrome/renderer/extensi...
chrome/renderer/extensions/extension_request_sender.cc:44: void
ExtensionRequestSender::InsertRequest(int request_id,
If it is invalid to reuse a request id, should we DCHECK that there is no
request with the id already in the map?

https://chromiumcodereview.appspot.com/9903010/diff/1/chrome/renderer/extensi...
File chrome/renderer/extensions/extension_request_sender.h (right):

https://chromiumcodereview.appspot.com/9903010/diff/1/chrome/renderer/extensi...
chrome/renderer/extensions/extension_request_sender.h:24: // Calls a named
extension API method in the browser.
... and routes the responses back to the caller.

https://chromiumcodereview.appspot.com/9903010/diff/1/chrome/renderer/extensi...
chrome/renderer/extensions/extension_request_sender.h:31: void
HandleResponse(int request_id,
Would be nice to document how these two methods are called.

subjective nit: I'd list the StartRequest method.

https://chromiumcodereview.appspot.com/9903010/diff/1/chrome/renderer/extensi...
chrome/renderer/extensions/extension_request_sender.h:37: int request_id,
Could you also move the GetNextRequestId stuff in here? Or even remove it from
the interface (i.e. autogenerate it and return it from StartRequest).

If not would be good to document this parameter a bit (i.e. the fact that it has
to be unique)

[koz (OOO until 15th September), 2012-03-29 23:55:46]

Thanks, Ben.

https://chromiumcodereview.appspot.com/9903010/diff/1/chrome/renderer/extensi...
File chrome/renderer/extensions/extension_request_sender.cc (right):

https://chromiumcodereview.appspot.com/9903010/diff/1/chrome/renderer/extensi...
chrome/renderer/extensions/extension_request_sender.cc:44: void
ExtensionRequestSender::InsertRequest(int request_id,
On 2012/03/29 23:06:36, benwells wrote:
> If it is invalid to reuse a request id, should we DCHECK that there is no
> request with the id already in the map?

Done.

https://chromiumcodereview.appspot.com/9903010/diff/1/chrome/renderer/extensi...
File chrome/renderer/extensions/extension_request_sender.h (right):

https://chromiumcodereview.appspot.com/9903010/diff/1/chrome/renderer/extensi...
chrome/renderer/extensions/extension_request_sender.h:24: // Calls a named
extension API method in the browser.
On 2012/03/29 23:06:36, benwells wrote:
> ... and routes the responses back to the caller.

Done.

https://chromiumcodereview.appspot.com/9903010/diff/1/chrome/renderer/extensi...
chrome/renderer/extensions/extension_request_sender.h:31: void
HandleResponse(int request_id,
On 2012/03/29 23:06:36, benwells wrote:
> Would be nice to document how these two methods are called.
> 
> subjective nit: I'd list the StartRequest method.

Done.

https://chromiumcodereview.appspot.com/9903010/diff/1/chrome/renderer/extensi...
chrome/renderer/extensions/extension_request_sender.h:37: int request_id,
On 2012/03/29 23:06:36, benwells wrote:
> Could you also move the GetNextRequestId stuff in here? Or even remove it from
> the interface (i.e. autogenerate it and return it from StartRequest).
> 
> If not would be good to document this parameter a bit (i.e. the fact that it
has
> to be unique)

As discussed, I'll do this in a follow-up CL. I've added a TODO to remove this
parameter.

[not at google - send to devlin, 2012-03-30 03:14:31]

basically lgtm but I do have a bunch of comments there (the meaty ones not
actually due to your code, but there you go...)

http://codereview.chromium.org/9903010/diff/1/chrome/renderer/extensions/chro...
File chrome/renderer/extensions/chrome_v8_extension.cc (right):

http://codereview.chromium.org/9903010/diff/1/chrome/renderer/extensions/chro...
chrome/renderer/extensions/chrome_v8_extension.cc:81: bool
ChromeV8Extension::CheckCurrentContextAccessToExtensionAPI(
You can probably just delete this method, from what I can tell the current call
sites have access to the extension dispatcher.

http://codereview.chromium.org/9903010/diff/1/chrome/renderer/extensions/exte...
File chrome/renderer/extensions/extension_dispatcher.h (right):

http://codereview.chromium.org/9903010/diff/1/chrome/renderer/extensions/exte...
chrome/renderer/extensions/extension_dispatcher.h:28: class
ExtensionRequestSender;
nit: alphabetical order

http://codereview.chromium.org/9903010/diff/2002/chrome/renderer/extensions/e...
File chrome/renderer/extensions/extension_request_sender.cc (right):

http://codereview.chromium.org/9903010/diff/2002/chrome/renderer/extensions/e...
chrome/renderer/extensions/extension_request_sender.cc:46:
DCHECK(pending_requests.count(request_id) == 0);
DCHECK_EQ

http://codereview.chromium.org/9903010/diff/2002/chrome/renderer/extensions/e...
chrome/renderer/extensions/extension_request_sender.cc:47:
pending_requests_[request_id].reset(pending_request);
so much overkill having this in a separate method though, only used in 1 place
and it's a 1-line function.

http://codereview.chromium.org/9903010/diff/2002/chrome/renderer/extensions/e...
chrome/renderer/extensions/extension_request_sender.cc:90: return;
this one is a bit funny, the JS side of things should now have enough
permissions checking so that this never happens. I think. I wouldn't trust it
enough to not have this check though, or to make it a CHECK. Maybe a TODO to
make it into a CHECK?

http://codereview.chromium.org/9903010/diff/2002/chrome/renderer/extensions/e...
chrome/renderer/extensions/extension_request_sender.cc:129: PendingRequest*
request = GetPendingRequest(request_id);
Could this be RemovePendingRequest and not have GetPendingRequest()? Seems like
all we ever want to do is remove it if it's there.

Related comment, the thing on line 139 worries me, if the frame "went away" do
we want to leak these request objects? Confused...

http://codereview.chromium.org/9903010/diff/2002/chrome/renderer/extensions/e...
File chrome/renderer/extensions/extension_request_sender.h (right):

http://codereview.chromium.org/9903010/diff/2002/chrome/renderer/extensions/e...
chrome/renderer/extensions/extension_request_sender.h:25: // response back to
the caller.
This reads more like the documentation that would be on a method. Could you
just... tweak it slightly?

http://codereview.chromium.org/9903010/diff/2002/chrome/renderer/extensions/e...
chrome/renderer/extensions/extension_request_sender.h:36: void
StartRequest(const std::string& name,
Looking at the implementation of this method it seems strange to me that we just
drop various failure conditions on the floor. This was the current behaviour I
suppose, but still. Perhaps this can return a success value or something to say
whether a request was actually made? Not that we necessarily need to do anything
about it in this class, let the caller deal with that (not in this patch), but
at least it's there.

http://codereview.chromium.org/9903010/diff/2002/chrome/renderer/extensions/e...
chrome/renderer/extensions/extension_request_sender.h:43: void
HandleResponse(int request_id,
"OnResponse" feels like a better name, it implies the relationship between
StartRequest and this method more accurately, makes the role of this class a bit
more clear... IMO...

Don't feel strongly though. I guess the JS side uses "handleResponse" and it's
too much effort to go and change that.

http://codereview.chromium.org/9903010/diff/2002/chrome/renderer/extensions/s...
File chrome/renderer/extensions/schema_generated_bindings.h (right):

http://codereview.chromium.org/9903010/diff/2002/chrome/renderer/extensions/s...
chrome/renderer/extensions/schema_generated_bindings.h:16: class
ChromeV8Extension;
nit that has nothing to do with your change: :sort

http://codereview.chromium.org/9903010/diff/2002/chrome/renderer/extensions/s...
chrome/renderer/extensions/schema_generated_bindings.h:35: explicit
SchemaGeneratedBindings(ExtensionDispatcher* extension_dispatcher,
doesn't need to be "explicit" anymore

[benwells, 2012-03-30 04:34:25]

lgtm

[koz (OOO until 15th September), 2012-04-03 00:15:17]

Cheers.

http://codereview.chromium.org/9903010/diff/1/chrome/renderer/extensions/chro...
File chrome/renderer/extensions/chrome_v8_extension.cc (right):

http://codereview.chromium.org/9903010/diff/1/chrome/renderer/extensions/chro...
chrome/renderer/extensions/chrome_v8_extension.cc:81: bool
ChromeV8Extension::CheckCurrentContextAccessToExtensionAPI(
On 2012/03/30 03:14:31, kalman wrote:
> You can probably just delete this method, from what I can tell the current
call
> sites have access to the extension dispatcher.

Done.

http://codereview.chromium.org/9903010/diff/1/chrome/renderer/extensions/exte...
File chrome/renderer/extensions/extension_dispatcher.h (right):

http://codereview.chromium.org/9903010/diff/1/chrome/renderer/extensions/exte...
chrome/renderer/extensions/extension_dispatcher.h:28: class
ExtensionRequestSender;
On 2012/03/30 03:14:31, kalman wrote:
> nit: alphabetical order

Done.

http://codereview.chromium.org/9903010/diff/2002/chrome/renderer/extensions/e...
File chrome/renderer/extensions/extension_request_sender.cc (right):

http://codereview.chromium.org/9903010/diff/2002/chrome/renderer/extensions/e...
chrome/renderer/extensions/extension_request_sender.cc:46:
DCHECK(pending_requests.count(request_id) == 0);
On 2012/03/30 03:14:31, kalman wrote:
> DCHECK_EQ

Done.

http://codereview.chromium.org/9903010/diff/2002/chrome/renderer/extensions/e...
chrome/renderer/extensions/extension_request_sender.cc:47:
pending_requests_[request_id].reset(pending_request);
On 2012/03/30 03:14:31, kalman wrote:
> so much overkill having this in a separate method though, only used in 1 place
> and it's a 1-line function.

Eh, it makes the code a bit more readable, I think. No messing around with
linked_ptrs or iterators.

http://codereview.chromium.org/9903010/diff/2002/chrome/renderer/extensions/e...
chrome/renderer/extensions/extension_request_sender.cc:90: return;
On 2012/03/30 03:14:31, kalman wrote:
> this one is a bit funny, the JS side of things should now have enough
> permissions checking so that this never happens. I think. I wouldn't trust it
> enough to not have this check though, or to make it a CHECK. Maybe a TODO to
> make it into a CHECK?

Done.

http://codereview.chromium.org/9903010/diff/2002/chrome/renderer/extensions/e...
chrome/renderer/extensions/extension_request_sender.cc:129: PendingRequest*
request = GetPendingRequest(request_id);
On 2012/03/30 03:14:31, kalman wrote:
> Could this be RemovePendingRequest and not have GetPendingRequest()? Seems
like
> all we ever want to do is remove it if it's there.
> 
> Related comment, the thing on line 139 worries me, if the frame "went away" do
> we want to leak these request objects? Confused...

Done.

http://codereview.chromium.org/9903010/diff/2002/chrome/renderer/extensions/e...
File chrome/renderer/extensions/extension_request_sender.h (right):

http://codereview.chromium.org/9903010/diff/2002/chrome/renderer/extensions/e...
chrome/renderer/extensions/extension_request_sender.h:25: // response back to
the caller.
On 2012/03/30 03:14:31, kalman wrote:
> This reads more like the documentation that would be on a method. Could you
> just... tweak it slightly?

Done.

http://codereview.chromium.org/9903010/diff/2002/chrome/renderer/extensions/e...
chrome/renderer/extensions/extension_request_sender.h:36: void
StartRequest(const std::string& name,
On 2012/03/30 03:14:31, kalman wrote:
> Looking at the implementation of this method it seems strange to me that we
just
> drop various failure conditions on the floor. This was the current behaviour I
> suppose, but still. Perhaps this can return a success value or something to
say
> whether a request was actually made? Not that we necessarily need to do
anything
> about it in this class, let the caller deal with that (not in this patch), but
> at least it's there.

Perhaps the caller doesn't care? Maybe the silent fail cases are only hit in
shutdown or something. Either way, I'm leery of adding a return for hypothetical
use cases.

http://codereview.chromium.org/9903010/diff/2002/chrome/renderer/extensions/e...
chrome/renderer/extensions/extension_request_sender.h:43: void
HandleResponse(int request_id,
On 2012/03/30 03:14:31, kalman wrote:
> "OnResponse" feels like a better name, it implies the relationship between
> StartRequest and this method more accurately, makes the role of this class a
bit
> more clear... IMO...
> 
> Don't feel strongly though. I guess the JS side uses "handleResponse" and it's
> too much effort to go and change that.

Yeah, it doesn't seem worth the change. HandleResponse() vs OnResponse() I think
are basically equivalent.

http://codereview.chromium.org/9903010/diff/2002/chrome/renderer/extensions/s...
File chrome/renderer/extensions/schema_generated_bindings.h (right):

http://codereview.chromium.org/9903010/diff/2002/chrome/renderer/extensions/s...
chrome/renderer/extensions/schema_generated_bindings.h:16: class
ChromeV8Extension;
On 2012/03/30 03:14:31, kalman wrote:
> nit that has nothing to do with your change: :sort

This class gets deleted in the follow-up, so I'll let that clean this up :-)

http://codereview.chromium.org/9903010/diff/2002/chrome/renderer/extensions/s...
chrome/renderer/extensions/schema_generated_bindings.h:35: explicit
SchemaGeneratedBindings(ExtensionDispatcher* extension_dispatcher,
On 2012/03/30 03:14:31, kalman wrote:
> doesn't need to be "explicit" anymore

ditto

[commit-bot: I haz the power, 2012-04-03 00:41:34]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/koz@chromium.org/9903010/7004

[commit-bot: I haz the power, 2012-04-03 01:02:25]

Try job failure for 9903010-7004 (retry) on linux_chromeos for step "compile"
(clobber build).
It's a second try, previously, step "compile" failed.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_chro...

[commit-bot: I haz the power, 2012-04-03 01:33:10]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/koz@chromium.org/9903010/12002

[commit-bot: I haz the power, 2012-04-03 02:48:31]

Try job failure for 9903010-12002 (retry) on linux_rel for step "browser_tests".
It's a second try, previously, step "browser_tests" failed.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_rel&...

[commit-bot: I haz the power, 2012-04-03 06:44:12]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/koz@chromium.org/9903010/7011

[commit-bot: I haz the power, 2012-04-03 08:30:07]

Change committed as 130343