Use the Document rather than the Frame's execution context to install JS-based marquee elements

Source/bindings/core/v8/PrivateScriptRunner.cpp

Index: Source/bindings/core/v8/PrivateScriptRunner.cpp
diff --git a/Source/bindings/core/v8/PrivateScriptRunner.cpp b/Source/bindings/core/v8/PrivateScriptRunner.cpp
index 4b4343aa370ba91785c5250d35c63d564fa965b5..b8314f9c6215d8492aa93de8cf5d25ea4b0a7b8b 100644
--- a/Source/bindings/core/v8/PrivateScriptRunner.cpp
+++ b/Source/bindings/core/v8/PrivateScriptRunner.cpp
@@ -14,6 +14,7 @@
 #ifndef NDEBUG
 #include "core/PrivateScriptSourcesForTesting.h"
 #endif
+#include "core/dom/Document.h"
 #include "core/dom/ExceptionCode.h"
 #include "platform/PlatformResourceLoader.h"
 
@@ -170,12 +171,10 @@ static void initializeHolderIfNeeded(ScriptState* scriptState, v8::Handle<v8::Ob
     }
 }
 
-v8::Handle<v8::Value> PrivateScriptRunner::installClassIfNeeded(LocalFrame* frame, String className)
+v8::Handle<v8::Value> PrivateScriptRunner::installClassIfNeeded(Document* document, String className)

[adamk, 2014/10/22 17:38:45]

Asking for a Document here is odd, there are likely non-DOM Blink-in-JS features
in the future. But I see from below that it really is a Document that you need.

 {
-    if (!frame)
-        return v8::Handle<v8::Value>();
-    v8::HandleScope handleScope(toIsolate(frame));
-    v8::Handle<v8::Context> context = toV8Context(frame, DOMWrapperWorld::privateScriptIsolatedWorld());
+    v8::HandleScope handleScope(toIsolate(document));
+    v8::Handle<v8::Context> context = toV8Context(document->executionContext(), DOMWrapperWorld::privateScriptIsolatedWorld());

[adamk, 2014/10/22 17:38:45]

This is pretty magical-looking. I had to go read through a lot of files to
figure out that this was calling Node::executionContext(), which was then
calling Document::contextDocument(). It makes me think we're in a very strange
place with the ExecutionContext refactor. If this is the right way to get ahold
of the ExecutionContext for a Document, it really seems like Document should
stop subclassing ExecutionContext...

I don't feel like holding up Blink-in-JS on this refactoring issue, but I'm
CCing morrita to get an idea of how close we are to getting rid of Document as
an ExecutionContext.

[Hajime Morrita, 2014/10/22 17:46:22]

I think the function is an leftover of my failed attempt. Document is a
ExecutionContext.

[adamk, 2014/10/22 17:51:35]

And yet it can (and does in this case) sometimes return a _different_ Document
than the one passed-in as |this|.

I think I'd rather see this code be more explicit for now:

document->contextDocument()

reads better to me than

document->executionContext()

rhogan, haraken, how does that sound?

     if (context.IsEmpty())
         return v8::Handle<v8::Value>();

[haraken, 2014/10/20 23:47:54]

Won't your CL hit this branch and just return an empty handle? I'm afraid we'll
just fail to install private scripts.

[rhogan, 2014/10/21 17:32:30]

No, document->executionContext() isn't empty AFAICT. I tried this out with and
got a working marquee:

<button onclick="useIt()">Use me</button>
<div id="container"></div>
<script>
function useIt() {
    var content = document.querySelector('template').content;
    document.querySelector('#container').appendChild(
    document.importNode(content, true));
}
</script>

<template>
<marquee>dsadsa</marquee>
</template>

     ScriptState* scriptState = ScriptState::from(context);