Apps V2 in Pepper: Host side implementation of ExntensionsCommon - Part 1.

Apps V2 in Pepper: Host side implementation of ExntensionsCommon - Part 1.

This change handles the most common case: There is no need to call into JS custom bindings; requests are directly relayed to the browser process.

BUG=None
TEST=None


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

[yzshen1, 2013-03-26 22:14:30]

Hi, Antony and Raymes.

Would you please take a look?

Antony: please review the changes to the extensions framework and how
ExtensionsCommon hooks up with it: For example, I used the main world script
context of the frame where the plugin element locates to execute those
extensions APIs. Are there any potential issues?

Raymes: please review the pepper-side change.

This change depends on https://codereview.chromium.org/13080002/
Besides, I will clean up the example testing code, and send it in a different
CL.

Thanks!

[asargent_no_longer_on_chrome, 2013-03-28 20:33:25]

LGTM

+cc kalman and koz as FYI

https://codereview.chromium.org/12567028/diff/1/chrome/renderer/extensions/re...
File chrome/renderer/extensions/request_sender.h (right):

https://codereview.chromium.org/12567028/diff/1/chrome/renderer/extensions/re...
chrome/renderer/extensions/request_sender.h:37: virtual ChromeV8Context*
GetContext() = 0;
Eventually it might be interesting to abstract this specific dependency on the
ChromeV8Context, but this CL overall is good incremental progress so I think
that could wait for future changes.

[raymes, 2013-03-28 20:51:16]

lgtm for pepper side stuff.

[yzshen1, 2013-03-28 22:25:22]

Thanks Antony and Raymes!

https://codereview.chromium.org/12567028/diff/1/chrome/renderer/extensions/re...
File chrome/renderer/extensions/request_sender.h (right):

https://codereview.chromium.org/12567028/diff/1/chrome/renderer/extensions/re...
chrome/renderer/extensions/request_sender.h:37: virtual ChromeV8Context*
GetContext() = 0;
I will bear that in mind. I think I will touch this part of codebase some more
in near future.

On 2013/03/28 20:33:26, Antony Sargent wrote:
> Eventually it might be interesting to abstract this specific dependency on the
> ChromeV8Context, but this CL overall is good incremental progress so I think
> that could wait for future changes.
>  
>

[commit-bot: I haz the power, 2013-03-29 06:26:14]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/yzshen@chromium.org/12567028/1

[commit-bot: I haz the power, 2013-03-29 11:42:41]

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

[commit-bot: I haz the power, 2013-03-29 14:45:07]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/yzshen@chromium.org/12567028/1

[commit-bot: I haz the power, 2013-03-29 14:46:22]

Change committed as 191355

[not at google - send to devlin, 2013-04-02 00:38:50]

What is the motivation for this change?

Where is the bug?

I don't really understand why we need to introduce this indirection and
complexity. ChromeV8Context is just a bundle of state with a couple of
unfortunate methods that deal with onLoad and chromeHidden, both of which are
deprecated. It shouldn't need to have any knowledge of request sending.

If I understand it correctly, the same problem could be solved by having just a
base::Bound callback to request sender. That would be much simpler. Changing
InvalidateContext to InvalidateSource is not the correct idiom,
InvalidateContext is very v8 specific.

[yzshen1, 2013-04-02 06:06:16]

Thanks, Benjamin!

On 2013/04/02 00:38:50, kalman wrote:
> What is the motivation for this change?
I am working on exposing apps V2 APIs as C/C++ interfaces for pepper
plugins.
For the most common case (i.e., no JS custom bindings are involved), the
requests from the plugin process are relayed by PepperExtensionsCommonHost
to RequestSender. From that point on, the requests follow the same code
path to the browser process.

> Where is the bug?
I didn't file a bug for this CL but I do have some documents. I will send
the URLs in a separate mail since they are within our corp domain.

> I don't really understand why we need to introduce this indirection and
> complexity. ChromeV8Context is just a bundle of state with a couple of
> unfortunate methods that deal with onLoad and chromeHidden, both of which
are
> deprecated. It shouldn't need to have any knowledge of request sending.

(The following explains why I came to this design, but I would be happy to
listen to your suggestions and make it nicer.)
I am adding one more client to use RequestSender. How each client handles
responses should be separated from RequestSender itself. That is why I
added an interface RequestSender::Source. Comparing with ChromeV8Context, I
think it may be better to let RequestSenderNatives inherit
RequestSender::Source. However, that requires a little more changes:
where/when to notify RequestSender to abandon pending requests. we should
also change SetIconNatives because it uses RequestSender too.

> If I understand it correctly, the same problem could be solved by having
just a
> base::Bound callback to request sender. That would be much simpler.

I have a few questions (in order to make sure I understand your suggestion
correctly.)
- When calling from RequestSenderNatives, do you mean binding a callback
using a weak ptr of RequestSenderNatives?
- SetIconNatives also needs to handle responses, how does it share code
(the response callback) with RequestSenderNatives? I think I should put the
share code in their base class ChromeV8Extension, because that is where
ChromeV8Context can be accessed, but that class has comment saying that it
should be deprecated too.

> Changing
> InvalidateContext to InvalidateSource is not the correct idiom,
> InvalidateContext is very v8 specific.

I used InvalidateSource() to notify RequestSender that a source has gone
away, I didn't intend to use it as a v8 concept --
PepperExtensionsCommonHost is a source but doesn't have anything to do with
v8. (Maybe that is a poor usage.)
If more changes are made as we talked above, this problem will go away.
Otherwise, maybe I could change it to RemoveSource().

Again, thanks for looking at the change. I will be happy to listen to your
suggestions and make the code better.


On Mon, Apr 1, 2013 at 5:38 PM, <kalman@chromium.org> wrote:

> What is the motivation for this change?
>
> Where is the bug?
>
> I don't really understand why we need to introduce this indirection and
> complexity. ChromeV8Context is just a bundle of state with a couple of
> unfortunate methods that deal with onLoad and chromeHidden, both of which
> are
> deprecated. It shouldn't need to have any knowledge of request sending.
>
> If I understand it correctly, the same problem could be solved by having
> just a
> base::Bound callback to request sender. That would be much simpler.
> Changing
> InvalidateContext to InvalidateSource is not the correct idiom,
> InvalidateContext is very v8 specific.
>
>
https://codereview.chromium.**org/12567028/<https://codereview.chromium.org/1...
>

[not at google - send to devlin, 2013-04-02 08:29:46]

Cool, quick response to some of your questions, and then I'll try to read the
documents tomorrow (I was on vacation for a few days and struggling to keep up
with things)

> (The following explains why I came to this design, but I would be happy to
> listen to your suggestions and make it nicer.)
> I am adding one more client to use RequestSender. How each client handles
> responses should be separated from RequestSender itself. That is why I
> added an interface RequestSender::Source. Comparing with ChromeV8Context, I
> think it may be better to let RequestSenderNatives inherit
> RequestSender::Source. However, that requires a little more changes:
> where/when to notify RequestSender to abandon pending requests. we should
> also change SetIconNatives because it uses RequestSender too.

Right, use case makes sense. I am not exactly sure what we do about cancelling
pending requests, i.e. where that comes into play, I'd need to refresh my memory
there.

> - When calling from RequestSenderNatives, do you mean binding a callback
> using a weak ptr of RequestSenderNatives?

Yep.

> - SetIconNatives also needs to handle responses, how does it share code
> (the response callback) with RequestSenderNatives?

If RequestSender's interface accepted a base::Callback then I think this
wouldn't really be a problem?

> I think I should put the
> share code in their base class ChromeV8Extension, because that is where
> ChromeV8Context can be accessed, but that class has comment saying that it
> should be deprecated too.

Yes, ChromeV8Extension shouldn't be used.

---

As for RequestSender - it should actually do all its own IPC handling and not be
funnelled through the ExtensionHelper->Dispatcher at all.

1. It should generate its own IDs
2. It should accept a callback rather than funnelling things into the JS
3. It should be per render view rather than global
4. The stuff that deals with contexts and v8 etc should be handled by the
SendRequestNatives/SetIconNatives things

The interface should really have just a single StartRequest method (or better,
SendRequest) with (name, has_callback, for_io_thread, ListValue*, callback). If
the sender is per render view then passing through the context is unnecessary
(everything can be determined that way), and if it generates its request IDs
internally then obviously the request_id parameter isn't necessary.

Basically, it should follow the model that ScriptExecutor uses, see:
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/ext...

(though it wouldn't need to do the funny unpacking thing since it'd be per
render view)

> 
> I used InvalidateSource() to notify RequestSender that a source has gone
> away, I didn't intend to use it as a v8 concept --
> PepperExtensionsCommonHost is a source but doesn't have anything to do with
> v8. (Maybe that is a poor usage.)
> If more changes are made as we talked above, this problem will go away.
> Otherwise, maybe I could change it to RemoveSource().

What I mean to say is that the reason InvalidateContext exists is due to a v8
implementation detail where v8::Contexts can outlive the ChromeV8Context
instances. Does pepper have the same problem?

Oops that wasn't a quick response after all. At first I did think that fixing
this problem would be simpler than all that, but the v8-ness of it all is rather
baked in. The high level story is that this stuff is all very old code.

[yzshen, 2013-04-03 19:10:16]

Thanks for the detailed reply! Please see my inline comments.


On Tue, Apr 2, 2013 at 1:29 AM, <kalman@chromium.org> wrote:

> Cool, quick response to some of your questions, and then I'll try to read
> the
> documents tomorrow (I was on vacation for a few days and struggling to
> keep up
> with things)
>
>
>  (The following explains why I came to this design, but I would be happy to
>> listen to your suggestions and make it nicer.)
>> I am adding one more client to use RequestSender. How each client handles
>> responses should be separated from RequestSender itself. That is why I
>> added an interface RequestSender::Source. Comparing with ChromeV8Context,
>> I
>> think it may be better to let RequestSenderNatives inherit
>> RequestSender::Source. However, that requires a little more changes:
>> where/when to notify RequestSender to abandon pending requests. we should
>> also change SetIconNatives because it uses RequestSender too.
>>
>
> Right, use case makes sense. I am not exactly sure what we do about
> cancelling
> pending requests, i.e. where that comes into play, I'd need to refresh my
> memory
> there.


>
>  - When calling from RequestSenderNatives, do you mean binding a callback
>> using a weak ptr of RequestSenderNatives?
>>
>
> Yep.
>
>
>  - SetIconNatives also needs to handle responses, how does it share code
>> (the response callback) with RequestSenderNatives?
>>
>
> If RequestSender's interface accepted a base::Callback then I think this
> wouldn't really be a problem?

I mean this function ChromeV8Context::OnResponseReceived():
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/renderer/ex...
(I moved it to ChromeV8Context from RequestSender.)

This handler should be shared by all JS users of RequestSender, right?
For example, when calling RequestSender::StartRequest() from
SendRequestNatives and SetIconNatives:

request_sender_->StartRequest(....
base::Bind(&SendRequestNatives::OnResponseReceived, this->AsWeakPtr()));
request_sender_->StartRequest(....
base::Bind(&SetIconNatives::OnResponseReceived, this->AsWeakPtr()));

The logic of this two methods are the same:
SendRequestNatives::OnResponseReceived
SetIconNatives::OnResponseReceived

The best place to put this common OnResponseReceived() seems to be
ChromeV8Extension, because it is a base class shared by
SendRequestNatives/SetIconNatives and it is aware of ChromeV8Context.
But that comment says that the class should be deprecated.


>
>  I think I should put the
>> share code in their base class ChromeV8Extension, because that is where
>> ChromeV8Context can be accessed, but that class has comment saying that it
>> should be deprecated too.
>>
>
> Yes, ChromeV8Extension shouldn't be used.
>
> ---
>
> As for RequestSender - it should actually do all its own IPC handling and
> not be
> funnelled through the ExtensionHelper->Dispatcher at all.
>
> 1. It should generate its own IDs
> 2. It should accept a callback rather than funnelling things into the JS
> 3. It should be per render view rather than global
> 4. The stuff that deals with contexts and v8 etc should be handled by the
> SendRequestNatives/**SetIconNatives things
>
> The interface should really have just a single StartRequest method (or
> better,
> SendRequest) with (name, has_callback, for_io_thread, ListValue*,
> callback). If
> the sender is per render view then passing through the context is
> unnecessary
> (everything can be determined that way), and if it generates its request
> IDs
> internally then obviously the request_id parameter isn't necessary.
>
> Basically, it should follow the model that ScriptExecutor uses, see:
> https://code.google.com/p/**chromium/codesearch#chromium/**
> src/chrome/browser/extensions/**script_executor.h&q=script_**
>
executor.h&sq=package:**chromium&type=cs&l=5<https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/extensions/script_executor.h&q=script_executor.h&sq=package:chromium&type=cs&l=5>
>
> (though it wouldn't need to do the funny unpacking thing since it'd be per
> render view)
>
> That sounds to be a nice plan.
One question: I think some information still needs to be retrieved from
context instead of render_view, according to the current StartRequest()
logic.
For example, the source origin is deduced from the frame of the context,
and also CheckContextAccessToExtensionAPI().

Do you mean I should also make the per-render-view change? I am afraid I
don't have enough time to make that happen in M28. :)


>
>
>  I used InvalidateSource() to notify RequestSender that a source has gone
>> away, I didn't intend to use it as a v8 concept --
>> PepperExtensionsCommonHost is a source but doesn't have anything to do
>> with
>> v8. (Maybe that is a poor usage.)
>> If more changes are made as we talked above, this problem will go away.
>> Otherwise, maybe I could change it to RemoveSource().
>>
>
> What I mean to say is that the reason InvalidateContext exists is due to a
> v8
> implementation detail where v8::Contexts can outlive the ChromeV8Context
> instances. Does pepper have the same problem?


The call InvalidateSource() is used when a user of RequestSender dies
earlier than RequestSender.
That happens for both ChromeV8Context and PepperExtensionsCommonHost.

I don't understand why it is related to the relationship between
v8::Context and ChromeV8Context. (Have I misunderstood anything?)

>


> Oops that wasn't a quick response after all. At first I did think that
> fixing
> this problem would be simpler than all that, but the v8-ness of it all is
> rather
> baked in. The high level story is that this stuff is all very old code.
>
>
https://codereview.chromium.**org/12567028/<https://codereview.chromium.org/1...
>


-- 
Best regards,
Yuzhu Shen.

[not at google - send to devlin, 2013-04-04 05:40:28]

I'll preface this with a few high-level things that may be getting lost here:
- a v8::Context is a JavaScript context (background page, content script, web
page, etc), and a ChromeV8Context is intended to be a little wrapper around this
that stores some extra Chrome-specific information for that context, such as the
extension.
- NativeHandler is an abstract way for JS code to hook into C++ code, and
ChromeV8Extension is the old way of doing that. It doesn't have any more
specific knowledge than that.
- RequestSender is a very specific process in which API calls are sent to the
browser, and then expects a response.

Neither of these 3 things are conceptually related, which is why e.g. putting
request interception logic in ChromeV8Context is not the right place to do it.

> 
> I mean this function ChromeV8Context::OnResponseReceived():
>
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/renderer/ex...
> (I moved it to ChromeV8Context from RequestSender.)
> 
> This handler should be shared by all JS users of RequestSender, right?
> For example, when calling RequestSender::StartRequest() from
> SendRequestNatives and SetIconNatives:
> 
> request_sender_->StartRequest(....
> base::Bind(&SendRequestNatives::OnResponseReceived, this->AsWeakPtr()));
> request_sender_->StartRequest(....
> base::Bind(&SetIconNatives::OnResponseReceived, this->AsWeakPtr()));
> 
> The logic of this two methods are the same:
> SendRequestNatives::OnResponseReceived
> SetIconNatives::OnResponseReceived

Another solution to that is an object/function/whatever to share between those
classes, or worst case a targeted superclass of SendRequestNatives and
SetIconNatives.

> 
> The best place to put this common OnResponseReceived() seems to be
> ChromeV8Extension, because it is a base class shared by
> SendRequestNatives/SetIconNatives and it is aware of ChromeV8Context.
> But that comment says that the class should be deprecated.

See comment at top. I should have explained that earlier.

> One question: I think some information still needs to be retrieved from
> context instead of render_view, according to the current StartRequest()
> logic.
> For example, the source origin is deduced from the frame of the context,
> and also CheckContextAccessToExtensionAPI().

Sure, pass whatever is needed into StartRequest.

> 
> Do you mean I should also make the per-render-view change? I am afraid I
> don't have enough time to make that happen in M28. :)

The per-render-view change would be nice but I think that the info can still be
passed in via StartRequest?

Incidentally, while you're talking about M28; what are the further changes going
to be to this stuff?

> The call InvalidateSource() is used when a user of RequestSender dies
> earlier than RequestSender.
> That happens for both ChromeV8Context and PepperExtensionsCommonHost.
> 
> I don't understand why it is related to the relationship between
> v8::Context and ChromeV8Context. (Have I misunderstood anything?)

Or maybe I'm misunderstanding something...

The reason the invalidation stuff happens is because a v8 context can outlive
all of the native handler / ChromeV8Context machinery. RequestSender needs to
know about that because it holds onto the raw ChromeV8Contexts which are no
longer valid after that. It's a C++ concern not a logical correctness concern,
if that makes sense. To put it another way... it's implementing weak ptrs, but
if this were using base::Bind then we'd get that automatically.

[yzshen, 2013-04-04 17:56:42]

Thanks Kalman for the detailed explanation.

I will come up with a patch and send to you for review. It will probably in
the next week or the week after that.

> Incidentally, while you're talking about M28; what are the further
changes going
> to be to this stuff?
For M28, the only changes at the renderer side that I would like to make is
to make it look nicer according to your suggestions.

I will do some other work at the browser side to relay requests coming
directly from the plugin process to the extension system. This is for those
performance-sensitive APIs, for example, chrome.socket. We don't want the
requests/responses to travel by way of the renderer process.

In M29 or later, I will need to come up with a solution for
PepperExtensionsCommonHost to call into the JS custom bindings. (As you
know, some APIs have part of the implementation as JS code.)
I will come up with a design and ask for comments from extension experts
then.


On Wed, Apr 3, 2013 at 10:40 PM, <kalman@chromium.org> wrote:

> I'll preface this with a few high-level things that may be getting lost
> here:
> - a v8::Context is a JavaScript context (background page, content script,
> web
> page, etc), and a ChromeV8Context is intended to be a little wrapper
> around this
> that stores some extra Chrome-specific information for that context, such
> as the
> extension.
> - NativeHandler is an abstract way for JS code to hook into C++ code, and
> ChromeV8Extension is the old way of doing that. It doesn't have any more
> specific knowledge than that.
> - RequestSender is a very specific process in which API calls are sent to
> the
> browser, and then expects a response.
>
> Neither of these 3 things are conceptually related, which is why e.g.
> putting
> request interception logic in ChromeV8Context is not the right place to do
> it.
>
>
>
>  I mean this function ChromeV8Context::**OnResponseReceived():
>>
>
> https://code.google.com/p/**chromium/codesearch#chromium/**
> src/chrome/renderer/**extensions/chrome_v8_context.**
>
cc&sq=package:chromium&rcl=**1364977939&l=197<https://code.google.com/p/chromium/codesearch#chromium/src/chrome/renderer/extensions/chrome_v8_context.cc&sq=package:chromium&rcl=1364977939&l=197>
>
>> (I moved it to ChromeV8Context from RequestSender.)
>>
>
>  This handler should be shared by all JS users of RequestSender, right?
>> For example, when calling RequestSender::StartRequest() from
>> SendRequestNatives and SetIconNatives:
>>
>
>  request_sender_->StartRequest(**....
>> base::Bind(&**SendRequestNatives::**OnResponseReceived,
>> this->AsWeakPtr()));
>> request_sender_->StartRequest(**....
>> base::Bind(&SetIconNatives::**OnResponseReceived, this->AsWeakPtr()));
>>
>
>  The logic of this two methods are the same:
>> SendRequestNatives::**OnResponseReceived
>> SetIconNatives::**OnResponseReceived
>>
>
> Another solution to that is an object/function/whatever to share between
> those
> classes, or worst case a targeted superclass of SendRequestNatives and
> SetIconNatives.
>
>
>
>  The best place to put this common OnResponseReceived() seems to be
>> ChromeV8Extension, because it is a base class shared by
>> SendRequestNatives/**SetIconNatives and it is aware of ChromeV8Context.
>> But that comment says that the class should be deprecated.
>>
>
> See comment at top. I should have explained that earlier.
>
>
>  One question: I think some information still needs to be retrieved from
>> context instead of render_view, according to the current StartRequest()
>> logic.
>> For example, the source origin is deduced from the frame of the context,
>> and also CheckContextAccessToExtensionA**PI().
>>
>
> Sure, pass whatever is needed into StartRequest.
>
>
>
>  Do you mean I should also make the per-render-view change? I am afraid I
>> don't have enough time to make that happen in M28. :)
>>
>
> The per-render-view change would be nice but I think that the info can
> still be
> passed in via StartRequest?
>
> Incidentally, while you're talking about M28; what are the further changes
> going
> to be to this stuff?
>
>
>  The call InvalidateSource() is used when a user of RequestSender dies
>> earlier than RequestSender.
>> That happens for both ChromeV8Context and PepperExtensionsCommonHost.
>>
>
>  I don't understand why it is related to the relationship between
>> v8::Context and ChromeV8Context. (Have I misunderstood anything?)
>>
>
> Or maybe I'm misunderstanding something...
>
> The reason the invalidation stuff happens is because a v8 context can
> outlive
> all of the native handler / ChromeV8Context machinery. RequestSender needs
> to
> know about that because it holds onto the raw ChromeV8Contexts which are no
> longer valid after that. It's a C++ concern not a logical correctness
> concern,
> if that makes sense. To put it another way... it's implementing weak ptrs,
> but
> if this were using base::Bind then we'd get that automatically.
>
>
https://codereview.chromium.**org/12567028/<https://codereview.chromium.org/1...
>



-- 
Best regards,
Yuzhu Shen.

[not at google - send to devlin, 2013-04-05 01:07:39]

> 
> I will come up with a patch and send to you for review. It will probably in
> the next week or the week after that.
>
> For M28, the only changes at the renderer side that I would like to make is
> to make it look nicer according to your suggestions.

Thanks. There isn't any rush (i.e. milestone requirement) though I'd like to
simplify the code before make any further changes, if possible.

> 
> I will do some other work at the browser side to relay requests coming
> directly from the plugin process to the extension system. This is for those
> performance-sensitive APIs, for example, chrome.socket. We don't want the
> requests/responses to travel by way of the renderer process.
> 
> In M29 or later, I will need to come up with a solution for
> PepperExtensionsCommonHost to call into the JS custom bindings. (As you
> know, some APIs have part of the implementation as JS code.)
> I will come up with a design and ask for comments from extension experts
> then.

Ok. Could you point me at it when you're done?

Thanks again!

[yzshen, 2013-04-05 16:18:22]

On Thu, Apr 4, 2013 at 6:07 PM, <kalman@chromium.org> wrote:

>
>
>  I will come up with a patch and send to you for review. It will probably
>> in
>> the next week or the week after that.
>>
>
>  For M28, the only changes at the renderer side that I would like to make
>> is
>> to make it look nicer according to your suggestions.
>>
>
> Thanks. There isn't any rush (i.e. milestone requirement) though I'd like
> to
> simplify the code before make any further changes, if possible.
>
>
>
>  I will do some other work at the browser side to relay requests coming
>> directly from the plugin process to the extension system. This is for
>> those
>> performance-sensitive APIs, for example, chrome.socket. We don't want the
>> requests/responses to travel by way of the renderer process.
>>
>
>  In M29 or later, I will need to come up with a solution for
>> PepperExtensionsCommonHost to call into the JS custom bindings. (As you
>> know, some APIs have part of the implementation as JS code.)
>> I will come up with a design and ask for comments from extension experts
>> then.
>>
>
> Ok. Could you point me at it when you're done?
>
Sure! :)


>
> Thanks again!
>
>
https://codereview.chromium.**org/12567028/<https://codereview.chromium.org/1...
>



-- 
Best regards,
Yuzhu Shen.