Implement an infrastructure of Blink-in-JS

Implement an infrastructure of Blink-in-JS

* Design doc is here: https://docs.google.com/a/google.com/document/d/13cT9Klgvt_ciAR3ONGvzKvw6fz9-f6E0FrqYFqfoc8Y/edit

* Developers can use Blink-in-JS in the following steps:

(1) Implement a DOM feature in XXX.js.

installClass("XXX", function() {
  function func1(int value) { ... }
  function func2(Node node) { ... }
  return {func1: func1, func2: func2};
});

(2) Add XXX.idl that defines an interface from C++ to the JS functions.

[BlinkInJSInterface] interface XXX {
  void func1(int value);
  DOMString func2(Node node);
};

(3) Then developers can use V8XXX::func1(int) and V8XXX::func2(Node*) in C++.

* For more details, see what BlinkInJSTest.js, BlinkInJSTest.idl and BlinkInJSTest.cpp are doing.

* Important files in this CL are as follows:

- BlinkInJSTest.js, BlinkInJSTest.idl and BlinkInJSTest.cpp: Test cases for Blink-in-JS.

- v8_blink_in_js.py, blink_in_js_interface.{h,cpp}: The code generator for Blink-in-JS' IDL files. You can see the generated code in V8TestBlinkInJSInterface.{h,cpp}.

- make_blink_in_js.py: Converts the contents of Blink-in-JS' JS files into byte stream. The byte stream is generated to BlinkInJSSources.h.

- BlinkInJSController.{h,cpp}: Compiles the byte stream and invokes the JS code.


BUG=341031
TEST=BlinkInJSTest

[haraken, 2014-06-20 14:04:05]

abarth@: Would you take a look at this CL when you have time?

nbarth@: Would you take a look at the IDL compiler and GYP files?

adamk@, jochen@, dcarney@, yosin@, tasak@: FYI.

This CL just implements an infrastructure of Blink-in-JS and add test cases (See
BlinkInJSTest.cpp). There is no real customers yet. Once we land this CL,
developers can start experimenting Blink-in-JS locally.

[arv (Not doing code reviews), 2014-06-20 15:06:22]

FYI

https://codereview.chromium.org/345893002/diff/40001/Source/bindings/v8/Blink...
File Source/bindings/v8/BlinkInJSController.js (right):

https://codereview.chromium.org/345893002/diff/40001/Source/bindings/v8/Blink...
Source/bindings/v8/BlinkInJSController.js:13: (installedClasses);
useless parens...

This could use a comment that you are depending in the completion value since
that is something no one ever does ;-)

https://codereview.chromium.org/345893002/diff/40001/Source/bindings/v8/Blink...
File Source/bindings/v8/BlinkInJSTest.js (right):

https://codereview.chromium.org/345893002/diff/40001/Source/bindings/v8/Blink...
Source/bindings/v8/BlinkInJSTest.js:49: function setInnerHTML(node, string) {
Is there a plan to support getters and setters?

https://codereview.chromium.org/345893002/diff/40001/Source/bindings/v8/Blink...
Source/bindings/v8/BlinkInJSTest.js:58: var event =
document.createEvent("MouseEvents");
var event = new MouseEvent('click', {
  bubbles: true,
  cancelable: true,
  view: window
});

[abarth-chromium, 2014-06-20 15:17:43]

This CL looks very interesting, but it's a lot to digest.  Can we break it into
a few pieces?

1) Code generator support
2) PrivateScriptController
3) PrivateScriptTest.js

Also, I wonder if we should use a different name in the implementation. 
Blink-in-JS is a good name for the project and design, but in the implementation
it is a bit awkward.  Perhaps "PrivateScript" is a better name for this concept
because the script is private to the implementation.  Another option is
"InternalScript", but we've already got the notion of running some internal
scripts in the main world (e.g., to set up inline event handlers).

Should we limit the types we allow in ImplementedWith=JavaScript interfaces? 
For example, we can't support "any" or "Dictionary" yet.  For example, |any| is
a security vulnerability because it would pass the JS object straight through
the C++ trampoline.  Maybe the RELEASE_ASSERT you added for content script
security already catches these cases?

https://codereview.chromium.org/345893002/diff/40001/Source/bindings/IDLExten...
File Source/bindings/IDLExtendedAttributes.txt (right):

https://codereview.chromium.org/345893002/diff/40001/Source/bindings/IDLExten...
Source/bindings/IDLExtendedAttributes.txt:37: BlinkInJSInterface
Would you be willing to consider a new name?  Perhaps ImplementedWith=JavaScript
?

https://codereview.chromium.org/345893002/diff/40001/Source/bindings/template...
File Source/bindings/templates/blink_in_js_interface.cpp (right):

https://codereview.chromium.org/345893002/diff/40001/Source/bindings/template...
Source/bindings/templates/blink_in_js_interface.cpp:20: v8::Handle<v8::Context>
context = toV8Context(frame, DOMWrapperWorld::blinkInJSWorld());
DOMWrapperWorld::blinkInJSWorld -> DOMWrapperWorld::PrivateScriptWorld ?

https://codereview.chromium.org/345893002/diff/40001/Source/bindings/template...
Source/bindings/templates/blink_in_js_interface.cpp:41: v8::Handle<v8::Value>
v8Value = BlinkInJSController::run(scriptState->isolate(), "{{cpp_class}}",
"{{method.name}}", windowWrapper, {{method.arguments | length}}, argv);
BlinkInJSController -> PrivateScriptController ?

https://codereview.chromium.org/345893002/diff/40001/Source/bindings/template...
File Source/bindings/templates/templates.gypi (right):

https://codereview.chromium.org/345893002/diff/40001/Source/bindings/template...
Source/bindings/templates/templates.gypi:10: 'blink_in_js_interface.h',
blink_in_js_interface -> implemented_with_javascript_interface ?

https://codereview.chromium.org/345893002/diff/40001/Source/bindings/v8/Blink...
File Source/bindings/v8/BlinkInJSController.cpp (right):

https://codereview.chromium.org/345893002/diff/40001/Source/bindings/v8/Blink...
Source/bindings/v8/BlinkInJSController.cpp:15: static v8::Handle<v8::Value>
compileBlinkInJS(v8::Isolate* isolate, String className)
compilePrivateScript ?

https://codereview.chromium.org/345893002/diff/40001/Source/bindings/v8/Blink...
Source/bindings/v8/BlinkInJSController.cpp:20: for (index = 0; index <
WTF_ARRAY_LENGTH(kBlinkInJSSources); index++) {
kPrivateScriptSources

https://codereview.chromium.org/345893002/diff/40001/Source/bindings/v8/Blink...
Source/bindings/v8/BlinkInJSController.cpp:22: break;
If we end up with a lot of these, we can probably do something smarter, but this
is fine for now.

https://codereview.chromium.org/345893002/diff/40001/Source/bindings/v8/Blink...
File Source/bindings/v8/BlinkInJSController.js (right):

https://codereview.chromium.org/345893002/diff/40001/Source/bindings/v8/Blink...
Source/bindings/v8/BlinkInJSController.js:3: // found in the LICENSE file.
PrivateScriptController.js ?

https://codereview.chromium.org/345893002/diff/40001/Source/bindings/v8/DOMWr...
File Source/bindings/v8/DOMWrapperWorld.h (right):

https://codereview.chromium.org/345893002/diff/40001/Source/bindings/v8/DOMWr...
Source/bindings/v8/DOMWrapperWorld.h:54: BlinkInJSIsolatedWorldId,
PrivateScriptWorldId ?

[haraken, 2014-06-20 15:26:49]

Thanks for comments! I'm happy to break the CL into pieces next week, so please
just leave comments about overall design and naming.

[haraken, 2014-06-20 15:31:11]

> Should we limit the types we allow in ImplementedWith=JavaScript interfaces? 
> For example, we can't support "any" or "Dictionary" yet.  For example, |any|
is
> a security vulnerability because it would pass the JS object straight through
> the C++ trampoline.  Maybe the RELEASE_ASSERT you added for content script
> security already catches these cases?

Good point. I'm planning to limit the types to JS primitive types and DOM
wrappers (even a JS object is dangerous). It's easy to add the verification to
the IDL compiler.

[dcarney, 2014-06-22 12:49:31]

you want everything to be per context, not per isolate.  this implementation
will bleed blinkinjs contexts all over the place, potentially keeping dom trees
alive in unexpected/exciting ways when blinkisjs interacts with the dom.  you
could keep the unbound scripts cached in the isolate if you want and have a
weakmap cache per context for the bound functions.

[haraken, 2014-06-22 13:21:00]

> you want everything to be per context, not per isolate.  this implementation
> will bleed blinkinjs contexts all over the place, potentially keeping dom
trees
> alive in unexpected/exciting ways when blinkisjs interacts with the dom.  you
> could keep the unbound scripts cached in the isolate if you want and have a
> weakmap cache per context for the bound functions.

This is something we've discussed in Munich, and probably I'm not understanding
things correctly.

In the current implementation, a context for Blink-in-JS exists per frame.
Specifically, if you look at V8TestBlinkInJSInterface.cpp, you can find the
following code:

  v8::Handle<v8::Context> context = toV8Context(frame,
DOMWrapperWorld::blinkInJSWorld());
  ScriptState* scriptState = ScriptState::from(context);
  ScriptState::Scope scope(scriptState);
  ...;

As shown in the above, we have a dedicated world for Blink-in-JS, and we have a
dedicated context for each pair of (frame, the dedicated world for Blink-in-JS).

In other words:

If we have no Blink-in-JS in a page where x frames and y worlds are involved,
there are (x*y) contexts. If we use Blink-in-JS in that page, we will have x
frames and (y+1) worlds, and thus x*(y+1) contexts.

Am I missing something?

[dcarney, 2014-06-22 15:15:56]

> Am I missing something?

sorry, i should have been more specific.  i'm only talking about the cache you
added to perisolatedata.  everything else is okay.

[haraken, 2014-06-22 15:27:04]

On 2014/06/22 15:15:56, dcarney wrote:
> > Am I missing something?
> 
> sorry, i should have been more specific.  i'm only talking about the cache you
> added to perisolatedata.  everything else is okay.

Thanks, makes sense. I'll move m_compiledBlinkInJS to V8PerContextData.

[haraken, 2014-06-23 01:32:55]

https://codereview.chromium.org/345893002/diff/40001/Source/bindings/v8/Blink...
File Source/bindings/v8/BlinkInJSController.js (right):

https://codereview.chromium.org/345893002/diff/40001/Source/bindings/v8/Blink...
Source/bindings/v8/BlinkInJSController.js:3: // found in the LICENSE file.
On 2014/06/20 15:17:43, abarth wrote:
> PrivateScriptController.js ?

Renamed all "BlinkInJS" to "PrivateScript".

https://codereview.chromium.org/345893002/diff/40001/Source/bindings/v8/Blink...
Source/bindings/v8/BlinkInJSController.js:13: (installedClasses);
On 2014/06/20 15:06:22, arv wrote:
> useless parens...
> 
> This could use a comment that you are depending in the completion value since
> that is something no one ever does ;-)

Done.

https://codereview.chromium.org/345893002/diff/40001/Source/bindings/v8/Blink...
File Source/bindings/v8/BlinkInJSTest.js (right):

https://codereview.chromium.org/345893002/diff/40001/Source/bindings/v8/Blink...
Source/bindings/v8/BlinkInJSTest.js:49: function setInnerHTML(node, string) {
On 2014/06/20 15:06:22, arv wrote:
> Is there a plan to support getters and setters?

Yes, will do that in a follow-up.

https://codereview.chromium.org/345893002/diff/40001/Source/bindings/v8/Blink...
Source/bindings/v8/BlinkInJSTest.js:58: var event =
document.createEvent("MouseEvents");
On 2014/06/20 15:06:22, arv wrote:
> var event = new MouseEvent('click', {
>   bubbles: true,
>   cancelable: true,
>   view: window
> });

Hmm, if I use 'new MouseEvent', it crashes. This looks like a real bug and I'm
investigating.

[Nils Barth (inactive), 2014-06-23 03:14:30]

* Per abarth@, breaking into separate CLs would help.
* Overall CG changes look fine!
* Instead of a build script (make_private_script_source.py), shouldn't we use
resources?
* Lotta nits.

https://codereview.chromium.org/345893002/diff/60001/Source/bindings/IDLExten...
File Source/bindings/IDLExtendedAttributes.txt (right):

https://codereview.chromium.org/345893002/diff/60001/Source/bindings/IDLExten...
Source/bindings/IDLExtendedAttributes.txt:37: PrivateScriptInterface
alpha

https://codereview.chromium.org/345893002/diff/60001/Source/bindings/core/idl...
File Source/bindings/core/idl.gypi (right):

https://codereview.chromium.org/345893002/diff/60001/Source/bindings/core/idl...
Source/bindings/core/idl.gypi:10: 'core.gypi',
? You're not using this here, are you?

https://codereview.chromium.org/345893002/diff/60001/Source/bindings/scripts/...
File Source/bindings/scripts/code_generator_v8.py (right):

https://codereview.chromium.org/345893002/diff/60001/Source/bindings/scripts/...
Source/bindings/scripts/code_generator_v8.py:75: import
v8_private_script_interface
alpha

https://codereview.chromium.org/345893002/diff/60001/Source/bindings/scripts/...
Source/bindings/scripts/code_generator_v8.py:151: if 'PrivateScriptInterface'
not in interface.extended_attributes:
Longer-term we can have private scripts also passed through the global
information computation, so we won't need this special case, but it's fine to
special-case at first.
Also, we'll likely *need* to do this to support dependency information,
though again, probably not necessary at first if keep it simple.
(If you plan to do this, could you add a FIXME?)

https://codereview.chromium.org/345893002/diff/60001/Source/bindings/scripts/...
File Source/bindings/scripts/scripts.gypi (right):

https://codereview.chromium.org/345893002/diff/60001/Source/bindings/scripts/...
Source/bindings/scripts/scripts.gypi:37: 'v8_private_script_interface.py',
alpha

https://codereview.chromium.org/345893002/diff/60001/Source/bindings/scripts/...
File Source/bindings/scripts/v8_callback_interface.py (right):

https://codereview.chromium.org/345893002/diff/60001/Source/bindings/scripts/...
Source/bindings/scripts/v8_callback_interface.py:64: # Callbacks use raw
pointers, so used_as_primitive_type=True
This is confusing, because "primitive type" is a technical term in Web IDL:
"The primitive types are boolean and the numeric types."
http://heycam.github.io/webidl/#dfn-primitive-type

...that's why we're using our own "usual" term.

https://codereview.chromium.org/345893002/diff/60001/Source/bindings/scripts/...
File Source/bindings/scripts/v8_private_script_interface.py (right):

https://codereview.chromium.org/345893002/diff/60001/Source/bindings/scripts/...
Source/bindings/scripts/v8_private_script_interface.py:34:
'forward_declarations': forward_declarations(private_script_interface),
alpha

https://codereview.chromium.org/345893002/diff/60001/Source/bindings/scripts/...
Source/bindings/scripts/v8_private_script_interface.py:59: idl_type,
extended_attributes, 'v8Value', 0, isolate='scriptState->isolate()')
line break?

https://codereview.chromium.org/345893002/diff/60001/Source/bindings/scripts/...
Source/bindings/scripts/v8_private_script_interface.py:60: if idl_type.name !=
'void' else '',
Could you just add a check to 
v8_types.v8_value_to_cpp_value
to have
if idl_type.name == 'void':
    return ''
?

https://codereview.chromium.org/345893002/diff/60001/Source/bindings/scripts/...
Source/bindings/scripts/v8_private_script_interface.py:76:
argument_declarations.extend(['%s %s' %
(argument.idl_type.cpp_type_args(used_as_argument=True), argument.name)
line break?

https://codereview.chromium.org/345893002/diff/60001/Source/bindings/scripts/...
File Source/bindings/scripts/v8_types.py (right):

https://codereview.chromium.org/345893002/diff/60001/Source/bindings/scripts/...
Source/bindings/scripts/v8_types.py:130: used_as_primitive_type:
"raw type" sounds better

Also, generally "primitive data type" means a basic type or built-in type,
not a raw type:
http://en.wikipedia.org/wiki/Primitive_type

https://codereview.chromium.org/345893002/diff/60001/Source/bindings/template...
File Source/bindings/templates/private_script_interface.cpp (right):

https://codereview.chromium.org/345893002/diff/60001/Source/bindings/template...
Source/bindings/templates/private_script_interface.cpp:33: {% if
method.arguments %}
Might be worth reminding people that empty array initializers are illegal, which
is why we're doing this.

https://codereview.chromium.org/345893002/diff/60001/Source/bindings/template...
Source/bindings/templates/private_script_interface.cpp:40: {% if
method.v8_value_to_cpp_value != '' %}
Don't need to compare to '', right?
(Implicit false.)

https://codereview.chromium.org/345893002/diff/60001/Source/bindings/template...
Source/bindings/templates/private_script_interface.cpp:49: {% else %}
{% else %}{# void return type #}
?

https://codereview.chromium.org/345893002/diff/60001/Source/bindings/template...
File Source/bindings/templates/templates.gypi (right):

https://codereview.chromium.org/345893002/diff/60001/Source/bindings/template...
Source/bindings/templates/templates.gypi:9: 'private_script_interface.cpp',
alpha

https://codereview.chromium.org/345893002/diff/60001/Source/bindings/tests/id...
File Source/bindings/tests/idls/TestPrivateScriptInterface.idl (right):

https://codereview.chromium.org/345893002/diff/60001/Source/bindings/tests/id...
Source/bindings/tests/idls/TestPrivateScriptInterface.idl:6:
PrivateScriptInterface,
alpha

https://codereview.chromium.org/345893002/diff/60001/Source/bindings/tests/id...
Source/bindings/tests/idls/TestPrivateScriptInterface.idl:7: NoInterfaceObject
Trailing comma

https://codereview.chromium.org/345893002/diff/60001/Source/bindings/tests/id...
Source/bindings/tests/idls/TestPrivateScriptInterface.idl:9: static void
doNothing();
Usually we've been naming members in test files with generic names reflecting
signature,
so these would be:
static void staticVoidMethod();
static short staticShortMethod();
etc.

https://codereview.chromium.org/345893002/diff/60001/Source/build/scripts/mak...
File Source/build/scripts/make_private_script_source.py (right):

https://codereview.chromium.org/345893002/diff/60001/Source/build/scripts/mak...
Source/build/scripts/make_private_script_source.py:5: """Convert Blink-in-JS
sources to C++ constant strings.
abarth@: should this be using GRIT or the like for resources?

This script is simple enough, but would be clearer as a simple Jinja template.

More importantly, we've generally preferred to have these as resources,
rather than build scripts.
abarth@ or Nico should be able to assist.
https://code.google.com/p/grit-i18n/wiki/GritUsersGuide

https://codereview.chromium.org/345893002/diff/60001/Source/build/scripts/mak...
Source/build/scripts/make_private_script_source.py:18: contents = ''
FWIW, in Python generally it's preferred to build a string by building a list
and then using string.join(), or using StringIO (cStringIO),
as simple implementations of string += are O(n^2)
(b/c strings are immutable and thus this builds a new string at each
concatenation).

This is optimized in CPython 2.5+, so this *should* be ok and not a bottleneck,
and you are using join for the key part, but a heads-up about possible build
performance issues.

https://codereview.chromium.org/345893002/diff/60001/Source/build/scripts/mak...
Source/build/scripts/make_private_script_source.py:26: contents += '\nstruct
PrivateScriptSources {\n    const char* name;\n    const unsigned char*
source;\n    size_t size;\n};\n\n'
This would be rather clearer as a multiline string """

https://codereview.chromium.org/345893002/diff/60001/Source/core/core.gypi
File Source/core/core.gypi (right):

https://codereview.chromium.org/345893002/diff/60001/Source/core/core.gypi#ne...
Source/core/core.gypi:452: '../bindings/v8/PrivateScriptTest.idl',
1. Alphabetical order please (.. is at start)

2. Testing files should go in 'webcore_testing_idl_files' instead

3. 'private_source_idl_files' variable instead?

This is creating a dependency of core on bindings
(later core_bindings_v8);
I assume this is ok, but it's a bit icky-looking.
Assuming we'll have more of these, better would be a variable
like 'private_source_idl_files',
which you could include from bindings.gypi
(later: bindings_core_v8.gypi)
and then have listed at the top via: <(private_source_idl_files)

https://codereview.chromium.org/345893002/diff/60001/Source/core/core_generat...
File Source/core/core_generated.gyp (right):

https://codereview.chromium.org/345893002/diff/60001/Source/core/core_generat...
Source/core/core_generated.gyp:148: # FIXME: The implementation of Blink-in-JS
is not yet mature.
Link to tracking bug?

https://codereview.chromium.org/345893002/diff/60001/Source/core/core_generat...
Source/core/core_generated.gyp:162: '<@(_inputs)'
??
'<@(_inputs)'
expands to start with
'../build/scripts/make_private_script_source.py'
doesn't it?!

You probably want a 'private_script_files' or
'private_script_js_files'
variable instead.

[haraken, 2014-06-23 06:01:46]

nbarth@: thanks for detailed review!

I have a question about where the IDL files for PrivateScripts should be listed
in gyp files (see below).

(I'll split this CL into pieces and reupload.)

https://codereview.chromium.org/345893002/diff/60001/Source/bindings/scripts/...
File Source/bindings/scripts/code_generator_v8.py (right):

https://codereview.chromium.org/345893002/diff/60001/Source/bindings/scripts/...
Source/bindings/scripts/code_generator_v8.py:151: if 'PrivateScriptInterface'
not in interface.extended_attributes:
On 2014/06/23 03:14:29, Nils Barth wrote:
> Longer-term we can have private scripts also passed through the global
> information computation, so we won't need this special case, but it's fine to
> special-case at first.
> Also, we'll likely *need* to do this to support dependency information,
> though again, probably not necessary at first if keep it simple.
> (If you plan to do this, could you add a FIXME?)

Actually I'm not sure what kind of dependency we'll need for PrivateScript
interfaces. So I'll just keep the current code as is in this CL.

https://codereview.chromium.org/345893002/diff/60001/Source/build/scripts/mak...
File Source/build/scripts/make_private_script_source.py (right):

https://codereview.chromium.org/345893002/diff/60001/Source/build/scripts/mak...
Source/build/scripts/make_private_script_source.py:5: """Convert Blink-in-JS
sources to C++ constant strings.
On 2014/06/23 03:14:30, Nils Barth wrote:
> abarth@: should this be using GRIT or the like for resources?
> 
> This script is simple enough, but would be clearer as a simple Jinja template.
> 
> More importantly, we've generally preferred to have these as resources,
> rather than build scripts.
> abarth@ or Nico should be able to assist.
> https://code.google.com/p/grit-i18n/wiki/GritUsersGuide

That's a good point. At the moment I just added a FIXME to rewrite the script to
grit. If I should rewrite it to grit in this CL, I'll do that. Otherwise, I want
to do that after landing Blink-in-JS's infrastructure.

https://codereview.chromium.org/345893002/diff/60001/Source/core/core.gypi
File Source/core/core.gypi (right):

https://codereview.chromium.org/345893002/diff/60001/Source/core/core.gypi#ne...
Source/core/core.gypi:452: '../bindings/v8/PrivateScriptTest.idl',
On 2014/06/23 03:14:30, Nils Barth wrote:
> 1. Alphabetical order please (.. is at start)
> 
> 2. Testing files should go in 'webcore_testing_idl_files' instead
> 
> 3. 'private_source_idl_files' variable instead?
> 
> This is creating a dependency of core on bindings
> (later core_bindings_v8);
> I assume this is ok, but it's a bit icky-looking.
> Assuming we'll have more of these, better would be a variable
> like 'private_source_idl_files',
> which you could include from bindings.gypi
> (later: bindings_core_v8.gypi)
> and then have listed at the top via: <(private_source_idl_files)

This is my question as well --- where the IDL files for PrivateScripts should be
listed.

- The IDL files for PrivateScripts (including PrivateScriptTest.idl) are not a
part of testing IDL files. They are a part of normal core/modules IDL files that
should be aggregated into aggregated binding files. Thus we cannot put them into
webcore_testing_idl_files.

- Almost all IDL files for PrivateScripts that will be added in the future will
be placed in core/ or modules/ (in order to replace the implementation of core/
and modules/ with JavaScript). PrivateScriptTest.idl will be the only IDL file
placed in bindings/.

Given the situation, where do you think I should list up the IDL files?

https://codereview.chromium.org/345893002/diff/60001/Source/core/core_generat...
File Source/core/core_generated.gyp (right):

https://codereview.chromium.org/345893002/diff/60001/Source/core/core_generat...
Source/core/core_generated.gyp:162: '<@(_inputs)'
On 2014/06/23 03:14:30, Nils Barth wrote:
> ??
> '<@(_inputs)'
> expands to start with
> '../build/scripts/make_private_script_source.py'
> doesn't it?!
> 
> You probably want a 'private_script_files' or
> 'private_script_js_files'
> variable instead.

You have a very good eye... I don't understand why this code is working as
expected (but it is actually working as expected).

[Nils Barth (inactive), 2014-06-23 06:21:25]

https://codereview.chromium.org/345893002/diff/60001/Source/bindings/scripts/...
File Source/bindings/scripts/code_generator_v8.py (right):

https://codereview.chromium.org/345893002/diff/60001/Source/bindings/scripts/...
Source/bindings/scripts/code_generator_v8.py:151: if 'PrivateScriptInterface'
not in interface.extended_attributes:
On 2014/06/23 06:01:46, haraken wrote:
> Actually I'm not sure what kind of dependency we'll need for PrivateScript
> interfaces. So I'll just keep the current code as is in this CL.

That's fine; just a heads-up.
Might want to add a comment, like:
# Currently private scripts don't have dependencies.

https://codereview.chromium.org/345893002/diff/60001/Source/build/scripts/mak...
File Source/build/scripts/make_private_script_source.py (right):

https://codereview.chromium.org/345893002/diff/60001/Source/build/scripts/mak...
Source/build/scripts/make_private_script_source.py:5: """Convert Blink-in-JS
sources to C++ constant strings.
On 2014/06/23 06:01:46, haraken wrote:
> That's a good point. At the moment I just added a FIXME to rewrite the script
to
> grit. If I should rewrite it to grit in this CL, I'll do that. Otherwise, I
want
> to do that after landing Blink-in-JS's infrastructure.

Might be ok to do later, but would be better to do sooner.

You should ask abarth: he's been reluctant to accept this sort of added build
script,
and if you're going to have to include JS as resources somewhere *anyway*
(regardless
of how the rest of Blink-in-JS is actually implemented), it doesn't hurt to do
it now rather
than later.

OTOH, if this is really in flux and you don't want to figure out GRIT *now*,
should be ok to do later.

Anyway, do ask abarth@ on this point.

https://codereview.chromium.org/345893002/diff/60001/Source/core/core.gypi
File Source/core/core.gypi (right):

https://codereview.chromium.org/345893002/diff/60001/Source/core/core.gypi#ne...
Source/core/core.gypi:452: '../bindings/v8/PrivateScriptTest.idl',
On 2014/06/23 06:01:46, haraken wrote:
> This is my question as well --- where the IDL files for PrivateScripts should
be
> listed.
> 
> - The IDL files for PrivateScripts (including PrivateScriptTest.idl) are not a
> part of testing IDL files. They are a part of normal core/modules IDL files
that
> should be aggregated into aggregated binding files. Thus we cannot put them
into
> webcore_testing_idl_files.
> 
> - Almost all IDL files for PrivateScripts that will be added in the future
will
> be placed in core/ or modules/ (in order to replace the implementation of
core/
> and modules/ with JavaScript). PrivateScriptTest.idl will be the only IDL file
> placed in bindings/.
> 
> Given the situation, where do you think I should list up the IDL files?

Probably best would be a separate list, say core_private_idl_files?
Putting them in the same core list is saying that they're part of
the web interface, but these are fundamentally implementation details,
and I think we'll want to treat these pretty differently
(we're already having special cases).

Also it's weird to put 1 test file with real files;
presumably we shouldn't be *shipping* test interfaces,
which is exactly why there's separate treatment for existing
test interfaces.

Cleanest would be another category,
core_private_testing_idl_files
...but if it's only going to be one file,
and it's not going to be treated differently,
that seems a bit silly.

Safer would be to make it runtime enabled, say
[RuntimeEnabled=PrivateScript] or something

Anyway, for the test file itself:
* Ok to put in core, I think.
* Otherwise make a separate bindings_idl_files list in bindings or
bindings/core/idl.gypi and then add that to the main core list in
bindings/core/idl.gypi.

https://codereview.chromium.org/345893002/diff/60001/Source/core/core_generat...
File Source/core/core_generated.gyp (right):

https://codereview.chromium.org/345893002/diff/60001/Source/core/core_generat...
Source/core/core_generated.gyp:162: '<@(_inputs)'
On 2014/06/23 06:01:46, haraken wrote:
> You have a very good eye...
Thanks (#^.^#) ...I look at lots of GYP files...

> I don't understand why this code is working as
> expected (but it is actually working as expected).

It's working as expected b/c GYP doesn't allow duplicates in lists.
(I realized this only on thinking about it.)
This is pretty fragile and puzzling though.

[dcarney, 2014-06-23 06:47:18]

On 2014/06/22 15:27:04, haraken wrote:
> On 2014/06/22 15:15:56, dcarney wrote:
> > > Am I missing something?
> > 
> > sorry, i should have been more specific.  i'm only talking about the cache
you
> > added to perisolatedata.  everything else is okay.
> 
> Thanks, makes sense. I'll move m_compiledBlinkInJS to V8PerContextData.

for security, i think you should be setting security origins for these contexts
to

blink_in_js:://actual_url

(or something more appropriate).  This will prevent cross origin dom
manipulation through blinkinjs scripts.

I'm not sure what to do about content scrips that call blinkinjs functions with
that scheme however

[haraken, 2014-06-23 07:15:03]

I think the remaining issue would be how to rewrite
make_private_script_source.py to GRIT. I'll chat with Nils about it.

On 2014/06/23 06:47:18, dcarney wrote:
> On 2014/06/22 15:27:04, haraken wrote:
> > On 2014/06/22 15:15:56, dcarney wrote:
> > > > Am I missing something?
> > > 
> > > sorry, i should have been more specific.  i'm only talking about the cache
> you
> > > added to perisolatedata.  everything else is okay.
> > 
> > Thanks, makes sense. I'll move m_compiledBlinkInJS to V8PerContextData.
> 
> for security, i think you should be setting security origins for these
contexts
> to
> 
> blink_in_js:://actual_url
> 
> (or something more appropriate).  This will prevent cross origin dom
> manipulation through blinkinjs scripts.
> 
> I'm not sure what to do about content scrips that call blinkinjs functions
with
> that scheme however

Good idea, done (in V8WindowShell.cpp).

https://codereview.chromium.org/345893002/diff/40001/Source/bindings/v8/Blink...
File Source/bindings/v8/BlinkInJSTest.js (right):

https://codereview.chromium.org/345893002/diff/40001/Source/bindings/v8/Blink...
Source/bindings/v8/BlinkInJSTest.js:58: var event =
document.createEvent("MouseEvents");
On 2014/06/23 01:32:55, haraken wrote:
> On 2014/06/20 15:06:22, arv wrote:
> > var event = new MouseEvent('click', {
> >   bubbles: true,
> >   cancelable: true,
> >   view: window
> > });
> 
> Hmm, if I use 'new MouseEvent', it crashes. This looks like a real bug and I'm
> investigating.

Fixed. The reason of the crash was that I was caching the complied script per
Isolate, not per Context. The crash is gone by moving m_compiledPrivateScript
from V8PerIsolateData to V8PerContextData.

[haraken, 2014-06-23 08:15:38]

https://codereview.chromium.org/345893002/diff/60001/Source/bindings/scripts/...
File Source/bindings/scripts/code_generator_v8.py (right):

https://codereview.chromium.org/345893002/diff/60001/Source/bindings/scripts/...
Source/bindings/scripts/code_generator_v8.py:151: if 'PrivateScriptInterface'
not in interface.extended_attributes:
On 2014/06/23 06:21:25, Nils Barth wrote:
> On 2014/06/23 06:01:46, haraken wrote:
> > Actually I'm not sure what kind of dependency we'll need for PrivateScript
> > interfaces. So I'll just keep the current code as is in this CL.
> 
> That's fine; just a heads-up.
> Might want to add a comment, like:
> # Currently private scripts don't have dependencies.

Done.

https://codereview.chromium.org/345893002/diff/60001/Source/core/core.gypi
File Source/core/core.gypi (right):

https://codereview.chromium.org/345893002/diff/60001/Source/core/core.gypi#ne...
Source/core/core.gypi:452: '../bindings/v8/PrivateScriptTest.idl',
On 2014/06/23 06:21:25, Nils Barth wrote:
> On 2014/06/23 06:01:46, haraken wrote:
> > This is my question as well --- where the IDL files for PrivateScripts
should
> be
> > listed.
> > 
> > - The IDL files for PrivateScripts (including PrivateScriptTest.idl) are not
a
> > part of testing IDL files. They are a part of normal core/modules IDL files
> that
> > should be aggregated into aggregated binding files. Thus we cannot put them
> into
> > webcore_testing_idl_files.
> > 
> > - Almost all IDL files for PrivateScripts that will be added in the future
> will
> > be placed in core/ or modules/ (in order to replace the implementation of
> core/
> > and modules/ with JavaScript). PrivateScriptTest.idl will be the only IDL
file
> > placed in bindings/.
> > 
> > Given the situation, where do you think I should list up the IDL files?
> 
> Probably best would be a separate list, say core_private_idl_files?
> Putting them in the same core list is saying that they're part of
> the web interface, but these are fundamentally implementation details,
> and I think we'll want to treat these pretty differently
> (we're already having special cases).
> 
> Also it's weird to put 1 test file with real files;
> presumably we shouldn't be *shipping* test interfaces,
> which is exactly why there's separate treatment for existing
> test interfaces.
> 
> Cleanest would be another category,
> core_private_testing_idl_files
> ...but if it's only going to be one file,
> and it's not going to be treated differently,
> that seems a bit silly.
> 
> Safer would be to make it runtime enabled, say
> [RuntimeEnabled=PrivateScript] or something
> 
> Anyway, for the test file itself:
> * Ok to put in core, I think.
> * Otherwise make a separate bindings_idl_files list in bindings or
> bindings/core/idl.gypi and then add that to the main core list in
> bindings/core/idl.gypi.

Done. I kept PrivateScriptTest.idl in bindings/v8/ and added 'binding_idl_files'
to bindings/core/v8/idl.gypi.

[Nils Barth (inactive), 2014-06-23 08:54:11]

https://codereview.chromium.org/345893002/diff/60001/Source/core/core.gypi
File Source/core/core.gypi (right):

https://codereview.chromium.org/345893002/diff/60001/Source/core/core.gypi#ne...
Source/core/core.gypi:452: '../bindings/v8/PrivateScriptTest.idl',
On 2014/06/23 08:15:38, haraken wrote:
> Done. I kept PrivateScriptTest.idl in bindings/v8/ and added
'binding_idl_files'
> to bindings/core/v8/idl.gypi.

Thanks, that's a good way to do it!

[haraken, 2014-06-23 11:51:50]

I started landing this CL splitting into pieces.

abarth@: I have a question about make_private_script_source.py. I guess we want
to replace make_private_script_source.py with grit since we don't want to have
more build scripts. I'm not familiar with grit but would you tell me a couple of
examples where grit is used for build scripts?