[Extension ServiceWorkers] Chrome: Passing v8::context to extensions dispatcher

[Extension ServiceWorkers] Chrome: Passing v8::context to extensions dispatcher

Pass the v8::context from the worker thread up to the extensions dispatcher.  In the dispatcher, 42 is binded to "chrome" for testing purposes.

Please view https://codereview.chromium.org/1264913002/ for the implementation done in blink.

BUG=501569
Committed: https://crrev.com/a7fc1264e00391787a5c473255c39d4bfbd4bc90
Cr-Commit-Position: refs/heads/master@{#342864}

[annekao, 2015-07-30 19:51:52]

annekao@google.com changed reviewers:
+ rdevlin.cronin@chromium.org

[annekao, 2015-07-30 21:07:18]

Patchset #1 (id:1) has been deleted

[annekao, 2015-07-30 21:17:18]

For testing the chrome binding, I tried to use post message to post "chrome ==
42" to the service worker and have it post a message back with true or false,
but I couldn't get it to work through a test.  Instead, I just had the service
worker use a fetch event on page.html.

[Devlin, 2015-07-30 21:31:08]

https://codereview.chromium.org/1256323008/diff/20001/chrome/renderer/chrome_...
File chrome/renderer/chrome_content_renderer_client.cc (right):

https://codereview.chromium.org/1256323008/diff/20001/chrome/renderer/chrome_...
chrome/renderer/chrome_content_renderer_client.cc:1637:
extension_dispatcher_->DidInitializeWorkerContext(context, url);
Let's go ahead and make the method on dispatcher static, so that we don't use
any member variables, since this is on a separate thread.  A comment addressing
that would be good, too.

https://codereview.chromium.org/1256323008/diff/20001/content/public/renderer...
File content/public/renderer/content_renderer_client.h (right):

https://codereview.chromium.org/1256323008/diff/20001/content/public/renderer...
content/public/renderer/content_renderer_client.h:88: virtual void
ServiceWorkerCreated() {}
Are these used?

https://codereview.chromium.org/1256323008/diff/20001/content/public/renderer...
content/public/renderer/content_renderer_client.h:305: virtual void
DidInitializeWorkerContext(v8::Local<v8::Context> context,
Comment this method.
Also, I'd rename this to DidInitializeWorkerContextOnWorkerThread(), so that
it's apparent that it's on a different thread.

https://codereview.chromium.org/1256323008/diff/20001/content/renderer/servic...
File content/renderer/service_worker/service_worker_context_client.cc (right):

https://codereview.chromium.org/1256323008/diff/20001/content/renderer/servic...
content/renderer/service_worker/service_worker_context_client.cc:383: void
ServiceWorkerContextClient::didInitializeWorkerContext(
(no need to rename these, because this entire object pretty much runs on the
worker thread.)

https://codereview.chromium.org/1256323008/diff/20001/content/renderer/servic...
File content/renderer/service_worker/service_worker_context_client.h (right):

https://codereview.chromium.org/1256323008/diff/20001/content/renderer/servic...
content/renderer/service_worker/service_worker_context_client.h:40: 
?

https://codereview.chromium.org/1256323008/diff/20001/extensions/renderer/dis...
File extensions/renderer/dispatcher.cc (right):

https://codereview.chromium.org/1256323008/diff/20001/extensions/renderer/dis...
extensions/renderer/dispatcher.cc:317: v8::Local<v8::Number> value =
v8::Number::New(context->GetIsolate(), 42);
While this is fun, let's go ahead and initialize chrome to just be an empty
object instead - that's a little closer to what it'll end up being.  (we can
just test that chrome !== undefined.)

[not at google - send to devlin, 2015-07-30 22:00:14]

kalman@chromium.org changed reviewers:
+ kalman@chromium.org

[not at google - send to devlin, 2015-07-30 22:00:14]

https://codereview.chromium.org/1256323008/diff/20001/extensions/renderer/dis...
File extensions/renderer/dispatcher.cc (right):

https://codereview.chromium.org/1256323008/diff/20001/extensions/renderer/dis...
extensions/renderer/dispatcher.cc:309: void
Dispatcher::DidInitializeWorkerContext(v8::Local<v8::Context> context,
Make sure to disambiguate between workers and service-workers, either by
asserting at the interface level that these are only for service workers, or by
checking within extensions code that |context| is for a service worker.

My preference is for the latter - have blink notify us for every worker context,
but only install bindings onto service worker contexts. Hopefully there is a way
to find out whether a v8::Context is for a service worker.

https://codereview.chromium.org/1256323008/diff/20001/extensions/renderer/dis...
extensions/renderer/dispatcher.cc:315: v8::NewStringType::kNormal)
You can use v8_helpers here (v8_helpers::ToV8String). However
v8_helpers::SetProperty (which I mention below) will do this for you.

https://codereview.chromium.org/1256323008/diff/20001/extensions/renderer/dis...
extensions/renderer/dispatcher.cc:318: if (context->Global()->Set(context, key,
value).IsJust()) {
I presume you did this because it has the V8_WARN_UNUSED_RESULT compiler
directive? Usually you'd solve this by either:
- using ignore_result() from base/macros.h
- putting it inside a CHECK()

however in this case, there is again a v8_helper which does this for you, and
where |key| can be a const char* so you don't need the string conversion. This
entire function can be basically:

if (extension)
  v8_helpers::SetProperty(context, context->Global(), "chrome",
                          v8::Object::New());

[Devlin, 2015-07-30 22:07:24]

https://codereview.chromium.org/1256323008/diff/20001/extensions/renderer/dis...
File extensions/renderer/dispatcher.cc (right):

https://codereview.chromium.org/1256323008/diff/20001/extensions/renderer/dis...
extensions/renderer/dispatcher.cc:309: void
Dispatcher::DidInitializeWorkerContext(v8::Local<v8::Context> context,
On 2015/07/30 22:00:14, kalman wrote:
> Make sure to disambiguate between workers and service-workers, either by
> asserting at the interface level that these are only for service workers, or
by
> checking within extensions code that |context| is for a service worker.
> 
> My preference is for the latter - have blink notify us for every worker
context,
> but only install bindings onto service worker contexts. Hopefully there is a
way
> to find out whether a v8::Context is for a service worker.

Being notified about every worker context is a little more code, because there
are different proxy objects in blink, and I'm not sure how easy it would be to
check if it's a service worker from the context itself.  But you're right about
specifying.

I'd say we should probably, for the time being, just rename the method on the
ContentRendererClient to include ServiceWorker; everywhere else, it's evident
(because each class is specific to service workers, e.g.
ServiceWorkerContextClient).  If we decide to learn about all workers, we can
then rename this method back and include a worker type, and put in the missing
pieces in the other worker proxies.  It's doable, but probably more churn than
we want, when all we need is service workers.

[annekao, 2015-07-30 22:54:18]

Moved the extension check from dispatcher to ChromeContentRendererClient and
checked the url scheme.

https://codereview.chromium.org/1256323008/diff/20001/chrome/renderer/chrome_...
File chrome/renderer/chrome_content_renderer_client.cc (right):

https://codereview.chromium.org/1256323008/diff/20001/chrome/renderer/chrome_...
chrome/renderer/chrome_content_renderer_client.cc:1637:
extension_dispatcher_->DidInitializeWorkerContext(context, url);
On 2015/07/30 21:31:08, Devlin wrote:
> Let's go ahead and make the method on dispatcher static, so that we don't use
> any member variables, since this is on a separate thread.  A comment
addressing
> that would be good, too.

Done.

https://codereview.chromium.org/1256323008/diff/20001/content/public/renderer...
File content/public/renderer/content_renderer_client.h (right):

https://codereview.chromium.org/1256323008/diff/20001/content/public/renderer...
content/public/renderer/content_renderer_client.h:88: virtual void
ServiceWorkerCreated() {}
On 2015/07/30 21:31:08, Devlin wrote:
> Are these used?

Removed.

https://codereview.chromium.org/1256323008/diff/20001/content/public/renderer...
content/public/renderer/content_renderer_client.h:305: virtual void
DidInitializeWorkerContext(v8::Local<v8::Context> context,
On 2015/07/30 21:31:08, Devlin wrote:
> Comment this method.
> Also, I'd rename this to DidInitializeWorkerContextOnWorkerThread(), so that
> it's apparent that it's on a different thread.

Done.

https://codereview.chromium.org/1256323008/diff/20001/extensions/renderer/dis...
File extensions/renderer/dispatcher.cc (right):

https://codereview.chromium.org/1256323008/diff/20001/extensions/renderer/dis...
extensions/renderer/dispatcher.cc:317: v8::Local<v8::Number> value =
v8::Number::New(context->GetIsolate(), 42);
On 2015/07/30 21:31:08, Devlin wrote:
> While this is fun, let's go ahead and initialize chrome to just be an empty
> object instead - that's a little closer to what it'll end up being.  (we can
> just test that chrome !== undefined.)

Done. Tested that chrome was type object.

https://codereview.chromium.org/1256323008/diff/20001/extensions/renderer/dis...
extensions/renderer/dispatcher.cc:318: if (context->Global()->Set(context, key,
value).IsJust()) {
On 2015/07/30 22:00:14, kalman wrote:
> I presume you did this because it has the V8_WARN_UNUSED_RESULT compiler
> directive? Usually you'd solve this by either:
> - using ignore_result() from base/macros.h
> - putting it inside a CHECK()
> 
> however in this case, there is again a v8_helper which does this for you, and
> where |key| can be a const char* so you don't need the string conversion. This
> entire function can be basically:
> 
> if (extension)
>   v8_helpers::SetProperty(context, context->Global(), "chrome",
>                           v8::Object::New());

Done.

[Devlin, 2015-07-30 22:58:52]

https://codereview.chromium.org/1256323008/diff/20001/content/public/renderer...
File content/public/renderer/content_renderer_client.h (right):

https://codereview.chromium.org/1256323008/diff/20001/content/public/renderer...
content/public/renderer/content_renderer_client.h:305: virtual void
DidInitializeWorkerContext(v8::Local<v8::Context> context,
On 2015/07/30 22:54:18, annekao wrote:
> On 2015/07/30 21:31:08, Devlin wrote:
> > Comment this method.
> > Also, I'd rename this to DidInitializeWorkerContextOnWorkerThread(), so that
> > it's apparent that it's on a different thread.
> 
> Done.

The method still has no comment...

https://codereview.chromium.org/1256323008/diff/40001/chrome/renderer/chrome_...
File chrome/renderer/chrome_content_renderer_client.cc (right):

https://codereview.chromium.org/1256323008/diff/40001/chrome/renderer/chrome_...
chrome/renderer/chrome_content_renderer_client.cc:1636: v8::Local<v8::Context>
context,
This indentation looks funny - was it git cl format's doing?

https://codereview.chromium.org/1256323008/diff/40001/chrome/renderer/chrome_...
chrome/renderer/chrome_content_renderer_client.cc:1638: if
(url.SchemeIs(extensions::kExtensionScheme)) {
Eh, I think the dispatcher should handle this check. Best to have as little
extension-y code here as possible.  On that note, we should #ifdef out the body
of this method with ENABLE_EXTENSIONS.

https://codereview.chromium.org/1256323008/diff/40001/chrome/renderer/chrome_...
chrome/renderer/chrome_content_renderer_client.cc:1639:
extension_dispatcher_->DidInitializeServiceWorkerContextOnWorkerThread(
This should be called statically.

https://codereview.chromium.org/1256323008/diff/40001/chrome/test/data/extens...
File chrome/test/data/extensions/api_test/service_worker/bindings/sw.js (right):

https://codereview.chromium.org/1256323008/diff/40001/chrome/test/data/extens...
chrome/test/data/extensions/api_test/service_worker/bindings/sw.js:6:
event.respondWith(new Response(typeof(chrome)));
typeof is a keyword, not a function.

https://codereview.chromium.org/1256323008/diff/40001/content/public/renderer...
File content/public/renderer/content_renderer_client.h (right):

https://codereview.chromium.org/1256323008/diff/40001/content/public/renderer...
content/public/renderer/content_renderer_client.h:303: const GURL& url){};
also, space between ) and {

[annekao, 2015-07-30 23:55:17]

Patchset #3 (id:60001) has been deleted

[annekao, 2015-07-30 23:56:25]

Patchset #4 (id:100001) has been deleted

[annekao, 2015-07-30 23:56:59]

Patchset #3 (id:80001) has been deleted

[annekao, 2015-07-30 23:57:40]

https://codereview.chromium.org/1256323008/diff/20001/content/public/renderer...
File content/public/renderer/content_renderer_client.h (right):

https://codereview.chromium.org/1256323008/diff/20001/content/public/renderer...
content/public/renderer/content_renderer_client.h:305: virtual void
DidInitializeWorkerContext(v8::Local<v8::Context> context,
On 2015/07/30 22:58:51, Devlin wrote:
> On 2015/07/30 22:54:18, annekao wrote:
> > On 2015/07/30 21:31:08, Devlin wrote:
> > > Comment this method.
> > > Also, I'd rename this to DidInitializeWorkerContextOnWorkerThread(), so
that
> > > it's apparent that it's on a different thread.
> > 
> > Done.
> 
> The method still has no comment...

Oops, somehow missed the first line. Done.

https://codereview.chromium.org/1256323008/diff/40001/chrome/renderer/chrome_...
File chrome/renderer/chrome_content_renderer_client.cc (right):

https://codereview.chromium.org/1256323008/diff/40001/chrome/renderer/chrome_...
chrome/renderer/chrome_content_renderer_client.cc:1636: v8::Local<v8::Context>
context,
On 2015/07/30 22:58:51, Devlin wrote:
> This indentation looks funny - was it git cl format's doing?

Yup.

https://codereview.chromium.org/1256323008/diff/40001/chrome/renderer/chrome_...
chrome/renderer/chrome_content_renderer_client.cc:1638: if
(url.SchemeIs(extensions::kExtensionScheme)) {
On 2015/07/30 22:58:51, Devlin wrote:
> Eh, I think the dispatcher should handle this check. Best to have as little
> extension-y code here as possible.  On that note, we should #ifdef out the
body
> of this method with ENABLE_EXTENSIONS.

Done.

https://codereview.chromium.org/1256323008/diff/40001/chrome/renderer/chrome_...
chrome/renderer/chrome_content_renderer_client.cc:1639:
extension_dispatcher_->DidInitializeServiceWorkerContextOnWorkerThread(
On 2015/07/30 22:58:51, Devlin wrote:
> This should be called statically.

Done.

https://codereview.chromium.org/1256323008/diff/40001/chrome/test/data/extens...
File chrome/test/data/extensions/api_test/service_worker/bindings/sw.js (right):

https://codereview.chromium.org/1256323008/diff/40001/chrome/test/data/extens...
chrome/test/data/extensions/api_test/service_worker/bindings/sw.js:6:
event.respondWith(new Response(typeof(chrome)));
On 2015/07/30 22:58:51, Devlin wrote:
> typeof is a keyword, not a function.

Done.

https://codereview.chromium.org/1256323008/diff/40001/content/public/renderer...
File content/public/renderer/content_renderer_client.h (right):

https://codereview.chromium.org/1256323008/diff/40001/content/public/renderer...
content/public/renderer/content_renderer_client.h:303: const GURL& url){};
On 2015/07/30 22:58:52, Devlin wrote:
> also, space between ) and {

Done.

[Devlin, 2015-07-31 16:24:32]

lgtm with nits.

https://codereview.chromium.org/1256323008/diff/120001/chrome/renderer/chrome...
File chrome/renderer/chrome_content_renderer_client.cc (right):

https://codereview.chromium.org/1256323008/diff/120001/chrome/renderer/chrome...
chrome/renderer/chrome_content_renderer_client.cc:1638: #if
defined(ENABLE_PLUGINS)
this should be ENABLE_EXTENSIONS, not ENABLE_PLUGINS

https://codereview.chromium.org/1256323008/diff/120001/chrome/test/data/exten...
File chrome/test/data/extensions/api_test/service_worker/bindings/sw.js (right):

https://codereview.chromium.org/1256323008/diff/120001/chrome/test/data/exten...
chrome/test/data/extensions/api_test/service_worker/bindings/sw.js:7:
event.respondWith(new Response(type));
you can still inline type. ;)

event.RespondWith(new Response(typeof chrome));

should work.

https://codereview.chromium.org/1256323008/diff/120001/content/public/rendere...
File content/public/renderer/content_renderer_client.h (right):

https://codereview.chromium.org/1256323008/diff/120001/content/public/rendere...
content/public/renderer/content_renderer_client.h:301: // Notifies that a
service worker context has been created on the worker
Let's make this say
"Notifies that a service worker context has been created. This function is
called from the worker thread."
The way it reads right now, it's kinda ambiguous, and could mean that the worker
context was created on the worker thread, without implying this function is
called there, too.  Better safe than sorry with threads :)

[Devlin, 2015-07-31 16:24:33]

lgtm with nits.

[annekao, 2015-07-31 17:13:33]

https://codereview.chromium.org/1256323008/diff/120001/chrome/renderer/chrome...
File chrome/renderer/chrome_content_renderer_client.cc (right):

https://codereview.chromium.org/1256323008/diff/120001/chrome/renderer/chrome...
chrome/renderer/chrome_content_renderer_client.cc:1638: #if
defined(ENABLE_PLUGINS)
On 2015/07/31 16:24:32, Devlin wrote:
> this should be ENABLE_EXTENSIONS, not ENABLE_PLUGINS

Don't know how I missed that. Done.

https://codereview.chromium.org/1256323008/diff/120001/chrome/test/data/exten...
File chrome/test/data/extensions/api_test/service_worker/bindings/sw.js (right):

https://codereview.chromium.org/1256323008/diff/120001/chrome/test/data/exten...
chrome/test/data/extensions/api_test/service_worker/bindings/sw.js:7:
event.respondWith(new Response(type));
On 2015/07/31 16:24:32, Devlin wrote:
> you can still inline type. ;)
> 
> event.RespondWith(new Response(typeof chrome));
> 
> should work.

I could've sworn this didn't work yesterday.  It works now though! Done.

https://codereview.chromium.org/1256323008/diff/120001/content/public/rendere...
File content/public/renderer/content_renderer_client.h (right):

https://codereview.chromium.org/1256323008/diff/120001/content/public/rendere...
content/public/renderer/content_renderer_client.h:301: // Notifies that a
service worker context has been created on the worker
On 2015/07/31 16:24:32, Devlin wrote:
> Let's make this say
> "Notifies that a service worker context has been created. This function is
> called from the worker thread."
> The way it reads right now, it's kinda ambiguous, and could mean that the
worker
> context was created on the worker thread, without implying this function is
> called there, too.  Better safe than sorry with threads :)

Done.

[annekao, 2015-07-31 20:34:30]

annekao@google.com changed reviewers:
+ kinuko@chromium.org, thestig@chromium.org

[annekao, 2015-07-31 20:37:11]

kinuko@, can you view the changes in content?  

thestig@, can you view the changes in chrome?

The v8::context for a service worker will be passed up from the worker thread
into the extensions dispatcher in order to register extension bindings onto it. 
This patch starts from blink here:
https://codereview.chromium.org/1264913002/.

[Lei Zhang, 2015-07-31 21:52:28]

chrome/ lgtm with a formatting nit

https://codereview.chromium.org/1256323008/diff/140001/chrome/renderer/chrome...
File chrome/renderer/chrome_content_renderer_client.cc (right):

https://codereview.chromium.org/1256323008/diff/140001/chrome/renderer/chrome...
chrome/renderer/chrome_content_renderer_client.cc:1634: void
ChromeContentRendererClient::
nit: I'd rather break the line after void, like on line 1612 and 1585.

https://codereview.chromium.org/1256323008/diff/140001/chrome/renderer/chrome...
chrome/renderer/chrome_content_renderer_client.cc:1636: v8::Local<v8::Context>
context,
And then 4 space indent this.

[kinuko, 2015-08-03 07:10:48]

content/ lgtm

[annekao, 2015-08-04 17:01:50]

Patchset #5 (id:160001) has been deleted

[annekao, 2015-08-04 17:02:08]

https://codereview.chromium.org/1256323008/diff/140001/chrome/renderer/chrome...
File chrome/renderer/chrome_content_renderer_client.cc (right):

https://codereview.chromium.org/1256323008/diff/140001/chrome/renderer/chrome...
chrome/renderer/chrome_content_renderer_client.cc:1634: void
ChromeContentRendererClient::
On 2015/07/31 21:52:28, Lei Zhang wrote:
> nit: I'd rather break the line after void, like on line 1612 and 1585.

Done.

https://codereview.chromium.org/1256323008/diff/140001/chrome/renderer/chrome...
chrome/renderer/chrome_content_renderer_client.cc:1636: v8::Local<v8::Context>
context,
On 2015/07/31 21:52:28, Lei Zhang wrote:
> And then 4 space indent this.

Done.

[annekao, 2015-08-04 17:34:47]

Patchset #5 (id:180001) has been deleted

[annekao, 2015-08-11 18:20:54]

The CQ bit was checked by annekao@google.com

[annekao, 2015-08-11 18:20:54]

The patchset sent to the CQ was uploaded after l-g-t-m from
rdevlin.cronin@chromium.org, kinuko@chromium.org, thestig@chromium.org
Link to the patchset: https://codereview.chromium.org/1256323008/#ps200001
(title: " ")

[commit-bot: I haz the power, 2015-08-11 18:21:17]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1256323008/200001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1256323008/200001

[commit-bot: I haz the power, 2015-08-11 19:25:34]

Committed patchset #5 (id:200001)

[commit-bot: I haz the power, 2015-08-11 19:26:10]

Patchset 5 (id:??) landed as
https://crrev.com/a7fc1264e00391787a5c473255c39d4bfbd4bc90
Cr-Commit-Position: refs/heads/master@{#342864}