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

extensions/renderer/dispatcher.cc

Index: extensions/renderer/dispatcher.cc
diff --git a/extensions/renderer/dispatcher.cc b/extensions/renderer/dispatcher.cc
index fb5d507348b8687139e447855d8752ab2bf9612f..63b9d436ac5a2027e511549dfb3963d26df90d8f 100644
--- a/extensions/renderer/dispatcher.cc
+++ b/extensions/renderer/dispatcher.cc
@@ -306,6 +306,21 @@ void Dispatcher::DidCreateScriptContext(
   VLOG(1) << "Num tracked contexts: " << script_context_set_->size();
 }
 
+void Dispatcher::DidInitializeWorkerContext(v8::Local<v8::Context> context,

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

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.

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

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.

+                                            const GURL& url) {
+  const Extension* extension = extensions_.GetExtensionOrAppByURL(url);
+  if (extension) {
+    v8::Local<v8::String> key;
+    if (v8::String::NewFromUtf8(context->GetIsolate(), "chrome",
+                                v8::NewStringType::kNormal)

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

You can use v8_helpers here (v8_helpers::ToV8String). However
v8_helpers::SetProperty (which I mention below) will do this for you.

+            .ToLocal(&key)) {
+      v8::Local<v8::Number> value = v8::Number::New(context->GetIsolate(), 42);

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

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.)

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

Done. Tested that chrome was type object.

+      if (context->Global()->Set(context, key, value).IsJust()) {

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

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());

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

Done.

+      }
+    }
+  }
+}
+
 void Dispatcher::WillReleaseScriptContext(
     blink::WebLocalFrame* frame,
     const v8::Local<v8::Context>& v8_context,