Pass pepper apps API calls through the existing js apps API bindings.

Pass pepper apps API calls through the existing js apps API bindings.

Previously, pepper apps API calls which are dispatched through the
renderer bypassed the js bindings and missed out on any custom behavior
implemented in those bindings. With this change, the API calls are
passed through the js bindings.

BUG=323067
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=240044

[yzshen1, 2013-11-26 19:33:10]

Mostly looks good. Thanks!

https://codereview.chromium.org/61383003/diff/350001/chrome/renderer/extensio...
File chrome/renderer/extensions/dispatcher.cc (right):

https://codereview.chromium.org/61383003/diff/350001/chrome/renderer/extensio...
chrome/renderer/extensions/dispatcher.cc:936:
scoped_ptr<extensions::NativeHandler>(
nit: no need to have extensions::

https://codereview.chromium.org/61383003/diff/350001/chrome/renderer/extensio...
chrome/renderer/extensions/dispatcher.cc:937: new
extensions::PepperRequestNatives(context)));
ditto

https://codereview.chromium.org/61383003/diff/350001/chrome/renderer/extensio...
File chrome/renderer/extensions/pepper_request_natives.cc (right):

https://codereview.chromium.org/61383003/diff/350001/chrome/renderer/extensio...
chrome/renderer/extensions/pepper_request_natives.cc:7: #include "base/values.h"
nit: please include what this file uses:
logging.h
string

https://codereview.chromium.org/61383003/diff/350001/chrome/renderer/extensio...
File chrome/renderer/extensions/pepper_request_proxy.cc (right):

https://codereview.chromium.org/61383003/diff/350001/chrome/renderer/extensio...
chrome/renderer/extensions/pepper_request_proxy.cc:33:
converter->ToV8Value(&args, context_->v8_context());
Why do we need this line?

https://codereview.chromium.org/61383003/diff/350001/chrome/renderer/extensio...
chrome/renderer/extensions/pepper_request_proxy.cc:44: return false;
In this case, do we need to remove the entry added on line 24?

https://codereview.chromium.org/61383003/diff/350001/chrome/renderer/extensio...
File chrome/renderer/extensions/pepper_request_proxy.h (right):

https://codereview.chromium.org/61383003/diff/350001/chrome/renderer/extensio...
chrome/renderer/extensions/pepper_request_proxy.h:43: // OnResponseReceived will
only be called if |has_callback| is true.
Will it be called if |has_callback| is true but the method failed? (I guess not
but better to be explicit.)

https://codereview.chromium.org/61383003/diff/350001/chrome/renderer/extensio...
chrome/renderer/extensions/pepper_request_proxy.h:44: bool StartRequest(Source*
source,
Is it better to use a base::Callback instead of |source|?

https://codereview.chromium.org/61383003/diff/350001/chrome/renderer/extensio...
chrome/renderer/extensions/pepper_request_proxy.h:51: void OnResponse(int
request_id,
nit: OnResponseReceived might sound better.

https://codereview.chromium.org/61383003/diff/350001/chrome/renderer/extensio...
chrome/renderer/extensions/pepper_request_proxy.h:62: ChromeV8Context* context_;
Please comment about the ownership/lifespan of context_.

https://codereview.chromium.org/61383003/diff/350001/chrome/renderer/pepper/p...
File chrome/renderer/pepper/pepper_extensions_common_host.cc (right):

https://codereview.chromium.org/61383003/diff/350001/chrome/renderer/pepper/p...
chrome/renderer/pepper/pepper_extensions_common_host.cc:50:
blink::WebPluginContainer* container =
It seems better to move line 54-56 to be right below line 49.

https://codereview.chromium.org/61383003/diff/350001/chrome/renderer/pepper/p...
chrome/renderer/pepper/pepper_extensions_common_host.cc:105: bool success =
pepper_request_proxy_->StartRequest(
Please add a TODO (maybe in pepper_request_proxy files) and associate it with a
bug saying that the current approach is not optimal: in most cases we don't do
much in the JS code, and we convert back-and-forth between base::Value args and
v8 objects.

[Sam McNally, 2013-11-27 23:40:26]

https://codereview.chromium.org/61383003/diff/350001/chrome/renderer/extensio...
File chrome/renderer/extensions/dispatcher.cc (right):

https://codereview.chromium.org/61383003/diff/350001/chrome/renderer/extensio...
chrome/renderer/extensions/dispatcher.cc:936:
scoped_ptr<extensions::NativeHandler>(
On 2013/11/26 19:33:11, yzshen1 wrote:
> nit: no need to have extensions::

Done.

https://codereview.chromium.org/61383003/diff/350001/chrome/renderer/extensio...
chrome/renderer/extensions/dispatcher.cc:937: new
extensions::PepperRequestNatives(context)));
On 2013/11/26 19:33:11, yzshen1 wrote:
> ditto

Done.

https://codereview.chromium.org/61383003/diff/350001/chrome/renderer/extensio...
File chrome/renderer/extensions/pepper_request_natives.cc (right):

https://codereview.chromium.org/61383003/diff/350001/chrome/renderer/extensio...
chrome/renderer/extensions/pepper_request_natives.cc:7: #include "base/values.h"
On 2013/11/26 19:33:11, yzshen1 wrote:
> nit: please include what this file uses:
> logging.h
> string

Done.

https://codereview.chromium.org/61383003/diff/350001/chrome/renderer/extensio...
File chrome/renderer/extensions/pepper_request_proxy.cc (right):

https://codereview.chromium.org/61383003/diff/350001/chrome/renderer/extensio...
chrome/renderer/extensions/pepper_request_proxy.cc:33:
converter->ToV8Value(&args, context_->v8_context());
On 2013/11/26 19:33:11, yzshen1 wrote:
> Why do we need this line?

Done.

https://codereview.chromium.org/61383003/diff/350001/chrome/renderer/extensio...
chrome/renderer/extensions/pepper_request_proxy.cc:44: return false;
On 2013/11/26 19:33:11, yzshen1 wrote:
> In this case, do we need to remove the entry added on line 24?

Done.

https://codereview.chromium.org/61383003/diff/350001/chrome/renderer/extensio...
File chrome/renderer/extensions/pepper_request_proxy.h (right):

https://codereview.chromium.org/61383003/diff/350001/chrome/renderer/extensio...
chrome/renderer/extensions/pepper_request_proxy.h:43: // OnResponseReceived will
only be called if |has_callback| is true.
On 2013/11/26 19:33:11, yzshen1 wrote:
> Will it be called if |has_callback| is true but the method failed? (I guess
not
> but better to be explicit.)

Done.

https://codereview.chromium.org/61383003/diff/350001/chrome/renderer/extensio...
chrome/renderer/extensions/pepper_request_proxy.h:44: bool StartRequest(Source*
source,
On 2013/11/26 19:33:11, yzshen1 wrote:
> Is it better to use a base::Callback instead of |source|?

Done.

https://codereview.chromium.org/61383003/diff/350001/chrome/renderer/extensio...
chrome/renderer/extensions/pepper_request_proxy.h:51: void OnResponse(int
request_id,
On 2013/11/26 19:33:11, yzshen1 wrote:
> nit: OnResponseReceived might sound better.

Done.

https://codereview.chromium.org/61383003/diff/350001/chrome/renderer/extensio...
chrome/renderer/extensions/pepper_request_proxy.h:62: ChromeV8Context* context_;
On 2013/11/26 19:33:11, yzshen1 wrote:
> Please comment about the ownership/lifespan of context_.

Done.

https://codereview.chromium.org/61383003/diff/350001/chrome/renderer/pepper/p...
File chrome/renderer/pepper/pepper_extensions_common_host.cc (right):

https://codereview.chromium.org/61383003/diff/350001/chrome/renderer/pepper/p...
chrome/renderer/pepper/pepper_extensions_common_host.cc:50:
blink::WebPluginContainer* container =
On 2013/11/26 19:33:11, yzshen1 wrote:
> It seems better to move line 54-56 to be right below line 49.

Done.

https://codereview.chromium.org/61383003/diff/350001/chrome/renderer/pepper/p...
chrome/renderer/pepper/pepper_extensions_common_host.cc:105: bool success =
pepper_request_proxy_->StartRequest(
On 2013/11/26 19:33:11, yzshen1 wrote:
> Please add a TODO (maybe in pepper_request_proxy files) and associate it with
a
> bug saying that the current approach is not optimal: in most cases we don't do
> much in the JS code, and we convert back-and-forth between base::Value args
and
> v8 objects.

Done.

[yzshen1, 2013-11-27 23:56:52]

https://codereview.chromium.org/61383003/diff/460001/chrome/renderer/extensio...
File chrome/renderer/extensions/pepper_request_proxy.cc (right):

https://codereview.chromium.org/61383003/diff/460001/chrome/renderer/extensio...
chrome/renderer/extensions/pepper_request_proxy.cc:25:
scoped_ptr<base::ListValue> mutable_args(args.DeepCopy());
It is unfortunate that we have to do a deep copy here. :/
Is there any way we can avoid this? For example, maybe we could take ownership
of |args|. Or we probably don't have to modify |args| -- we convert it to
|v8_args|, we could add request_id / request_name there.

[Sam McNally, 2013-11-28 00:04:50]

https://codereview.chromium.org/61383003/diff/460001/chrome/renderer/extensio...
File chrome/renderer/extensions/pepper_request_proxy.cc (right):

https://codereview.chromium.org/61383003/diff/460001/chrome/renderer/extensio...
chrome/renderer/extensions/pepper_request_proxy.cc:25:
scoped_ptr<base::ListValue> mutable_args(args.DeepCopy());
On 2013/11/27 23:56:52, yzshen1 wrote:
> It is unfortunate that we have to do a deep copy here. :/
> Is there any way we can avoid this? For example, maybe we could take ownership
> of |args|. Or we probably don't have to modify |args| -- we convert it to
> |v8_args|, we could add request_id / request_name there.

Done.

[yzshen1, 2013-11-28 00:09:22]

LGTM with one question:

https://codereview.chromium.org/61383003/diff/480001/chrome/renderer/extensio...
File chrome/renderer/extensions/pepper_request_proxy.cc (right):

https://codereview.chromium.org/61383003/diff/480001/chrome/renderer/extensio...
chrome/renderer/extensions/pepper_request_proxy.cc:24:
pending_request_map_[*request_id] = callback;
Out of curiosity: is it possible that we never get a response for a request and
thus leaving an entry in |pending_request_map_| forever?

[Sam McNally, 2013-11-28 06:02:43]

https://codereview.chromium.org/61383003/diff/480001/chrome/renderer/extensio...
File chrome/renderer/extensions/pepper_request_proxy.cc (right):

https://codereview.chromium.org/61383003/diff/480001/chrome/renderer/extensio...
chrome/renderer/extensions/pepper_request_proxy.cc:24:
pending_request_map_[*request_id] = callback;
On 2013/11/28 00:09:22, yzshen1 wrote:
> Out of curiosity: is it possible that we never get a response for a request
and
> thus leaving an entry in |pending_request_map_| forever? 

I suppose it could if the browser never replies, but this applies for API
requests from JS too.

[Sam McNally, 2013-12-02 06:40:10]

+koz

[koz (OOO until 15th September), 2013-12-02 23:27:19]

lgtm with nits

https://codereview.chromium.org/61383003/diff/480001/chrome/renderer/extensio...
File chrome/renderer/extensions/pepper_request_natives.cc (right):

https://codereview.chromium.org/61383003/diff/480001/chrome/renderer/extensio...
chrome/renderer/extensions/pepper_request_natives.cc:25: DCHECK(args.Length() ==
3);
nit: DCHECK_EQ(3, args.Length());

https://codereview.chromium.org/61383003/diff/480001/chrome/renderer/extensio...
File chrome/renderer/extensions/pepper_request_natives.h (right):

https://codereview.chromium.org/61383003/diff/480001/chrome/renderer/extensio...
chrome/renderer/extensions/pepper_request_natives.h:17: // Custom bindings for
the PepperRequestNatives API.
Could you change this comment to describe what the PepperRequestNatives API
does?

https://codereview.chromium.org/61383003/diff/480001/chrome/renderer/extensio...
chrome/renderer/extensions/pepper_request_natives.h:23: void SendResponse(const
v8::FunctionCallbackInfo<v8::Value>& args);
Add a comment for this function and include what the expected parameters are.

[Sam McNally, 2013-12-02 23:57:10]

+sky for chrome/renderer/resources/renderer_resources.grd

https://codereview.chromium.org/61383003/diff/480001/chrome/renderer/extensio...
File chrome/renderer/extensions/pepper_request_natives.cc (right):

https://codereview.chromium.org/61383003/diff/480001/chrome/renderer/extensio...
chrome/renderer/extensions/pepper_request_natives.cc:25: DCHECK(args.Length() ==
3);
On 2013/12/02 23:27:19, koz wrote:
> nit: DCHECK_EQ(3, args.Length());

Done.

https://codereview.chromium.org/61383003/diff/480001/chrome/renderer/extensio...
File chrome/renderer/extensions/pepper_request_natives.h (right):

https://codereview.chromium.org/61383003/diff/480001/chrome/renderer/extensio...
chrome/renderer/extensions/pepper_request_natives.h:17: // Custom bindings for
the PepperRequestNatives API.
On 2013/12/02 23:27:19, koz wrote:
> Could you change this comment to describe what the PepperRequestNatives API
> does?

Done.

https://codereview.chromium.org/61383003/diff/480001/chrome/renderer/extensio...
chrome/renderer/extensions/pepper_request_natives.h:23: void SendResponse(const
v8::FunctionCallbackInfo<v8::Value>& args);
On 2013/12/02 23:27:19, koz wrote:
> Add a comment for this function and include what the expected parameters are.

Done.

[not at google - send to devlin, 2013-12-03 00:49:13]

FWIW, back from thanksgiving now and still trying to internalise this change. I
had 1 comment from back then and it's pretty trivial.

https://codereview.chromium.org/61383003/diff/350001/chrome/renderer/extensio...
File chrome/renderer/extensions/chrome_v8_context.h (right):

https://codereview.chromium.org/61383003/diff/350001/chrome/renderer/extensio...
chrome/renderer/extensions/chrome_v8_context.h:150:
scoped_ptr<PepperRequestProxy> pepper_request_proxy_;
No need to wrap this in a scoped_ptr?

[Sam McNally, 2013-12-03 08:52:58]

https://codereview.chromium.org/61383003/diff/350001/chrome/renderer/extensio...
File chrome/renderer/extensions/chrome_v8_context.h (right):

https://codereview.chromium.org/61383003/diff/350001/chrome/renderer/extensio...
chrome/renderer/extensions/chrome_v8_context.h:150:
scoped_ptr<PepperRequestProxy> pepper_request_proxy_;
On 2013/12/03 00:49:14, kalman wrote:
> No need to wrap this in a scoped_ptr?

Done.

[sky, 2013-12-03 17:25:18]

chrome/renderer/resources/renderer_resources.grd LGTM

[not at google - send to devlin, 2013-12-03 17:32:49]

https://codereview.chromium.org/61383003/diff/550001/chrome/renderer/extensio...
File chrome/renderer/extensions/pepper_request_natives.cc (right):

https://codereview.chromium.org/61383003/diff/550001/chrome/renderer/extensio...
chrome/renderer/extensions/pepper_request_natives.cc:41: success = false;
if success is false, could you set some kind of error?

https://codereview.chromium.org/61383003/diff/550001/chrome/renderer/extensio...
chrome/renderer/extensions/pepper_request_natives.cc:44: if
(!result->GetAsList(&result_list))
avoid mysterious failures elsehere by making this a CHECK(result->GetAsList).

https://codereview.chromium.org/61383003/diff/550001/chrome/renderer/extensio...
File chrome/renderer/extensions/pepper_request_proxy.cc (right):

https://codereview.chromium.org/61383003/diff/550001/chrome/renderer/extensio...
chrome/renderer/extensions/pepper_request_proxy.cc:44: args->Insert(0, new
base::StringValue(request_name));
it seems like you can avoid this weirdness of modify |args| (and having to pass
it as *args rather than const args&) by adding |requset_name| to v8_args?

the other place you modify is in StartCall but if there's not StartPost then you
could do a similar thing and push the request ID onto v8_args not args.

https://codereview.chromium.org/61383003/diff/550001/chrome/renderer/extensio...
File chrome/renderer/extensions/pepper_request_proxy.h (right):

https://codereview.chromium.org/61383003/diff/550001/chrome/renderer/extensio...
chrome/renderer/extensions/pepper_request_proxy.h:50: bool StartPost(const
std::string& request_name,
StartPost seems unnecessary, particularly if you remove the |request_id|
argument from StartCall. Callers can pass base::DoNothing() as the callback to
StartCall.

https://codereview.chromium.org/61383003/diff/550001/chrome/renderer/pepper/p...
File chrome/renderer/pepper/pepper_extensions_common_host.cc (right):

https://codereview.chromium.org/61383003/diff/550001/chrome/renderer/pepper/p...
chrome/renderer/pepper/pepper_extensions_common_host.cc:34: callback_ =
base::Bind(&PepperExtensionsCommonHost::OnResponseReceived,
holding onto this callback is weird. just create it each request.

https://codereview.chromium.org/61383003/diff/550001/chrome/renderer/pepper/p...
chrome/renderer/pepper/pepper_extensions_common_host.cc:104: base::ListValue&
args) {
this (and below) is a non-const reference? ok well I guess the call site isn't
in extensions, so whatever.

https://codereview.chromium.org/61383003/diff/550001/chrome/renderer/pepper/p...
chrome/renderer/pepper/pepper_extensions_common_host.cc:124:
pending_request_map_[request_id] =
I think that pending_request_map_ is unnecessary if you curry through this
ReplyMessageContext into the base::Bound OnResponseReceieved. You could do like

scoped_ptr<ReplyMessageContext> message_context(
    new ReplyMessageContext(context->MakeReplyMessageContext());
bool success = pepper_request_proxy_->StartCall(
    base::Bind(weak_ptr_factory_.GetWeakPtr(),
               &OnResponseReceived,
               base::Passed(&message_context)),
    request_name,
    &args,
    // no need for request_id anymore, woo
    &error);
return success ? PP_OK_COMPLETIONPENDING : PP_ERROR_FAILED;

then OnResponseReceiver would receive that scoped_ptr<ReplyMessageContext> as
its first argument.

obviously that would also require that you don't hold onto the callback as a
constant member variable. however, as I said above, it's rather unidiomatic to
do that regardless.

https://codereview.chromium.org/61383003/diff/550001/chrome/renderer/resource...
File chrome/renderer/resources/extensions/pepper_request.js (right):

https://codereview.chromium.org/61383003/diff/550001/chrome/renderer/resource...
chrome/renderer/resources/extensions/pepper_request.js:15:
checkAvailability(name);
having checkAvailability be a separate method doesn't add much.

https://codereview.chromium.org/61383003/diff/550001/chrome/renderer/resource...
chrome/renderer/resources/extensions/pepper_request.js:36: return e.message;
It would probably be nicer to catch this error from C++ but I can see that the
ModuleSystem doesn't like that.

We can fix this later - probably by having some interface like

ModuleSystem::ScopedExceptionCatcher exception_catcher(module_system);
module_system->CallModuleMethod(...);
if (exception_catcher.HasCaught...)
  ...

but for now could you add a TODO?

https://codereview.chromium.org/61383003/diff/550001/chrome/renderer/resource...
chrome/renderer/resources/extensions/pepper_request.js:40: function
postMethod(targetName) {
if you kill StartPost then you'd kill this method too, but I think that's fine.
the extra work in doing that SendResponse etc is negligible.

[Sam McNally, 2013-12-04 00:01:35]

https://codereview.chromium.org/61383003/diff/550001/chrome/renderer/extensio...
File chrome/renderer/extensions/pepper_request_natives.cc (right):

https://codereview.chromium.org/61383003/diff/550001/chrome/renderer/extensio...
chrome/renderer/extensions/pepper_request_natives.cc:41: success = false;
On 2013/12/03 17:32:50, kalman wrote:
> if success is false, could you set some kind of error?

Done.

https://codereview.chromium.org/61383003/diff/550001/chrome/renderer/extensio...
chrome/renderer/extensions/pepper_request_natives.cc:44: if
(!result->GetAsList(&result_list))
On 2013/12/03 17:32:50, kalman wrote:
> avoid mysterious failures elsehere by making this a CHECK(result->GetAsList).

Done.

https://codereview.chromium.org/61383003/diff/550001/chrome/renderer/extensio...
File chrome/renderer/extensions/pepper_request_proxy.cc (right):

https://codereview.chromium.org/61383003/diff/550001/chrome/renderer/extensio...
chrome/renderer/extensions/pepper_request_proxy.cc:44: args->Insert(0, new
base::StringValue(request_name));
On 2013/12/03 17:32:50, kalman wrote:
> it seems like you can avoid this weirdness of modify |args| (and having to
pass
> it as *args rather than const args&) by adding |requset_name| to v8_args?
> 
> the other place you modify is in StartCall but if there's not StartPost then
you
> could do a similar thing and push the request ID onto v8_args not args.

Done.

https://codereview.chromium.org/61383003/diff/550001/chrome/renderer/extensio...
File chrome/renderer/extensions/pepper_request_proxy.h (right):

https://codereview.chromium.org/61383003/diff/550001/chrome/renderer/extensio...
chrome/renderer/extensions/pepper_request_proxy.h:50: bool StartPost(const
std::string& request_name,
On 2013/12/03 17:32:50, kalman wrote:
> StartPost seems unnecessary, particularly if you remove the |request_id|
> argument from StartCall. Callers can pass base::DoNothing() as the callback to
> StartCall.

Done.

https://codereview.chromium.org/61383003/diff/550001/chrome/renderer/pepper/p...
File chrome/renderer/pepper/pepper_extensions_common_host.cc (right):

https://codereview.chromium.org/61383003/diff/550001/chrome/renderer/pepper/p...
chrome/renderer/pepper/pepper_extensions_common_host.cc:34: callback_ =
base::Bind(&PepperExtensionsCommonHost::OnResponseReceived,
On 2013/12/03 17:32:50, kalman wrote:
> holding onto this callback is weird. just create it each request.

Done.

https://codereview.chromium.org/61383003/diff/550001/chrome/renderer/pepper/p...
chrome/renderer/pepper/pepper_extensions_common_host.cc:104: base::ListValue&
args) {
On 2013/12/03 17:32:50, kalman wrote:
> this (and below) is a non-const reference? ok well I guess the call site isn't
> in extensions, so whatever.

Done.

https://codereview.chromium.org/61383003/diff/550001/chrome/renderer/pepper/p...
chrome/renderer/pepper/pepper_extensions_common_host.cc:124:
pending_request_map_[request_id] =
On 2013/12/03 17:32:50, kalman wrote:
> I think that pending_request_map_ is unnecessary if you curry through this
> ReplyMessageContext into the base::Bound OnResponseReceieved. You could do
like
> 
> scoped_ptr<ReplyMessageContext> message_context(
>     new ReplyMessageContext(context->MakeReplyMessageContext());
> bool success = pepper_request_proxy_->StartCall(
>     base::Bind(weak_ptr_factory_.GetWeakPtr(),
>                &OnResponseReceived,
>                base::Passed(&message_context)),
>     request_name,
>     &args,
>     // no need for request_id anymore, woo
>     &error);
> return success ? PP_OK_COMPLETIONPENDING : PP_ERROR_FAILED;
> 
> then OnResponseReceiver would receive that scoped_ptr<ReplyMessageContext> as
> its first argument.
> 
> obviously that would also require that you don't hold onto the callback as a
> constant member variable. however, as I said above, it's rather unidiomatic to
> do that regardless.

Done.

https://codereview.chromium.org/61383003/diff/550001/chrome/renderer/resource...
File chrome/renderer/resources/extensions/pepper_request.js (right):

https://codereview.chromium.org/61383003/diff/550001/chrome/renderer/resource...
chrome/renderer/resources/extensions/pepper_request.js:15:
checkAvailability(name);
On 2013/12/03 17:32:50, kalman wrote:
> having checkAvailability be a separate method doesn't add much.

Done.

https://codereview.chromium.org/61383003/diff/550001/chrome/renderer/resource...
chrome/renderer/resources/extensions/pepper_request.js:36: return e.message;
On 2013/12/03 17:32:50, kalman wrote:
> It would probably be nicer to catch this error from C++ but I can see that the
> ModuleSystem doesn't like that.
> 
> We can fix this later - probably by having some interface like
> 
> ModuleSystem::ScopedExceptionCatcher exception_catcher(module_system);
> module_system->CallModuleMethod(...);
> if (exception_catcher.HasCaught...)
>   ...
> 
> but for now could you add a TODO?

Done.

https://codereview.chromium.org/61383003/diff/550001/chrome/renderer/resource...
chrome/renderer/resources/extensions/pepper_request.js:40: function
postMethod(targetName) {
On 2013/12/03 17:32:50, kalman wrote:
> if you kill StartPost then you'd kill this method too, but I think that's
fine.
> the extra work in doing that SendResponse etc is negligible.

Done.

[not at google - send to devlin, 2013-12-10 22:22:16]

lgtm

https://codereview.chromium.org/61383003/diff/630001/chrome/renderer/extensio...
File chrome/renderer/extensions/pepper_request_natives.cc (right):

https://codereview.chromium.org/61383003/diff/630001/chrome/renderer/extensio...
chrome/renderer/extensions/pepper_request_natives.cc:39: DCHECK(result);
nothing to change here, but fwiw:

My distaste for the Value API means I would have just DCHECK(result &&
result->IsType(LIST)) then just static_cast<>'d within the OnResponseReceived
call.

https://codereview.chromium.org/61383003/diff/630001/chrome/renderer/extensio...
File chrome/renderer/extensions/pepper_request_proxy.cc (right):

https://codereview.chromium.org/61383003/diff/630001/chrome/renderer/extensio...
chrome/renderer/extensions/pepper_request_proxy.cc:28: v8::HandleScope
scope(v8::Isolate::GetCurrent());
hold onto the context's Isolate here somewhere, then use it here instead of
v8::Isolate::GetCurrent and below when you refer to the isolate.

https://codereview.chromium.org/61383003/diff/630001/chrome/renderer/pepper/p...
File chrome/renderer/pepper/pepper_extensions_common_host.cc (right):

https://codereview.chromium.org/61383003/diff/630001/chrome/renderer/pepper/p...
chrome/renderer/pepper/pepper_extensions_common_host.cc:103:
base::Bind(DoNothing), request_name, args, &error);
interesting, I thought this always needed to be &DoNothing for base::Bind. cool.

https://codereview.chromium.org/61383003/diff/630001/chrome/renderer/resource...
File chrome/renderer/resources/extensions/pepper_request.js (right):

https://codereview.chromium.org/61383003/diff/630001/chrome/renderer/resource...
chrome/renderer/resources/extensions/pepper_request.js:15: var parameters =
utils.lookup(functions, "name", functionName).parameters;
here and below use ' not "

[Sam McNally, 2013-12-10 23:19:09]

https://codereview.chromium.org/61383003/diff/630001/chrome/renderer/extensio...
File chrome/renderer/extensions/pepper_request_proxy.cc (right):

https://codereview.chromium.org/61383003/diff/630001/chrome/renderer/extensio...
chrome/renderer/extensions/pepper_request_proxy.cc:28: v8::HandleScope
scope(v8::Isolate::GetCurrent());
On 2013/12/10 22:22:16, kalman wrote:
> hold onto the context's Isolate here somewhere, then use it here instead of
> v8::Isolate::GetCurrent and below when you refer to the isolate.

Done.

https://codereview.chromium.org/61383003/diff/630001/chrome/renderer/pepper/p...
File chrome/renderer/pepper/pepper_extensions_common_host.cc (right):

https://codereview.chromium.org/61383003/diff/630001/chrome/renderer/pepper/p...
chrome/renderer/pepper/pepper_extensions_common_host.cc:103:
base::Bind(DoNothing), request_name, args, &error);
On 2013/12/10 22:22:16, kalman wrote:
> interesting, I thought this always needed to be &DoNothing for base::Bind.
cool.

The & is required for member functions. Added one here for consistency with the
other Bind.

https://codereview.chromium.org/61383003/diff/630001/chrome/renderer/resource...
File chrome/renderer/resources/extensions/pepper_request.js (right):

https://codereview.chromium.org/61383003/diff/630001/chrome/renderer/resource...
chrome/renderer/resources/extensions/pepper_request.js:15: var parameters =
utils.lookup(functions, "name", functionName).parameters;
On 2013/12/10 22:22:16, kalman wrote:
> here and below use ' not "

Done.

[commit-bot: I haz the power, 2013-12-10 23:20:51]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/sammc@chromium.org/61383003/650001

[commit-bot: I haz the power, 2013-12-11 00:19:25]

Retried try job too often on linux_rel for step(s) check_deps
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_rel&...

[commit-bot: I haz the power, 2013-12-11 02:02:31]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/sammc@chromium.org/61383003/650001

[commit-bot: I haz the power, 2013-12-11 06:56:33]

Change committed as 240044