Implement Blink-in-JS for DOM attributes

Implement Blink-in-JS for DOM attributes

* This CL implements the core infrastructure of Blink-in-JS to support DOM attributes.

* Developers can use DOM attributes in Blink-in-JS as follows:

(1) Implement DOM attribute getters/setters in JavaScript.

// Foo.js
installClass("Foo", function(window) {
  var FooPrototype = {};
  FooPrototype.initialize = function() {  // Initialize properties of |this| object.
    this.m_attr = "initial value";
  }
  Object.defineProperty(FooPrototype, "attr", {
    get: function() { return this.m_attr; },  // DOM attribute getter
    set: function(value) { return this.m_attr = attr; }  // DOM attribute setter
  })
  return FooPrototype;
});

(2) Add an [ImplementedInPrivateScript] IDL attribute to Foo.idl.

// Foo.idl
interface Foo {
  [ImplementedInPrivateScript] attribute DOMString attr;
};

* For more details, see changes to Internals.js and Internals.idl.

* bindings/scripts/v8_attributes.py and bindings/templates/attributes.cpp implement the binding layer that connects DOM attributes with private scripts. Only primitive types and DOM wrappers are allowed as types of attribute values used in Blink-in-JS.

BUG=341031
TEST=fast/dom/private_script_unittest.html

Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=177511

[haraken, 2014-07-02 14:16:07]

This is still work-in-progress. Not ready for review.

[abarth-chromium, 2014-07-02 14:26:31]

https://codereview.chromium.org/362993004/diff/1/Source/core/testing/Internal...
File Source/core/testing/Internals.js (right):

https://codereview.chromium.org/362993004/diff/1/Source/core/testing/Internal...
Source/core/testing/Internals.js:114: }
This is pretty ugly.  It would be much nicer to be able to implement getters and
setters as getters and setters...  Maybe we should write a CL for V8 to expose
getPropertyDescriptor via V8's API?

[haraken, 2014-07-02 15:47:20]

https://codereview.chromium.org/362993004/diff/1/Source/core/testing/Internal...
File Source/core/testing/Internals.js (right):

https://codereview.chromium.org/362993004/diff/1/Source/core/testing/Internal...
Source/core/testing/Internals.js:114: }
On 2014/07/02 14:26:31, abarth wrote:
> This is pretty ugly.  It would be much nicer to be able to implement getters
and
> setters as getters and setters...  Maybe we should write a CL for V8 to expose
> getPropertyDescriptor via V8's API?

Yeah, I'm on the same page.

Even if it's hard to implement a V8 API for getPropertyDescriptor, we can
implement getPropertyDescriptor in JavaScript and get the result from C++ using
v8::Function::Call. This is weird but will work. Either way we should somehow
support normal getters and setters in Blink-in-JS.

[haraken, 2014-07-03 11:10:25]

abarth@, arv@: I supported formal getters and setters of DOM attributes. PTAL at
PrivateScriptRunner.{h,cpp} and Internals.{idl,js}.

jl@: PTAL at the IDL compiler.

adamk@, dcarney@, tasak@, nbarth@: FYI.

https://codereview.chromium.org/362993004/diff/60001/Source/bindings/core/v8/...
File Source/bindings/core/v8/PrivateScriptRunner.cpp (right):

https://codereview.chromium.org/362993004/diff/60001/Source/bindings/core/v8/...
Source/bindings/core/v8/PrivateScriptRunner.cpp:68: static
v8::Handle<v8::Object> getOwnPropertyDescriptor(ScriptState* scriptState,
v8::Handle<v8::Object> classObject, String name)

dcarney@ implemented a V8 API for getOwnPropertyDescriptor. Once it's rolled,
I'll replace this method with the V8 API.

https://codereview.chromium.org/362993004/diff/60001/Source/bindings/core/v8/...
Source/bindings/core/v8/PrivateScriptRunner.cpp:93: if
(!initializeFunction.IsEmpty()) {

abarth@, arv@: If initialize() is not yet called for the holder object, we call
it here.

https://codereview.chromium.org/362993004/diff/60001/Source/core/testing/Inte...
File Source/core/testing/Internals.js (right):

https://codereview.chromium.org/362993004/diff/60001/Source/core/testing/Inte...
Source/core/testing/Internals.js:10: InternalsPrototype.initialize = function()
{

abarth@, arv@: We need to provide a way to initialize properties of objects that
use InternalsPrototype.

[Jens Widell, 2014-07-03 11:33:14]

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

https://codereview.chromium.org/362993004/diff/60001/Source/bindings/scripts/...
Source/bindings/scripts/v8_attributes.py:138: 'argument_cpp_type':
idl_type.cpp_type_args(used_as_argument=True),
Alphabetical order. :-)

https://codereview.chromium.org/362993004/diff/60001/Source/bindings/scripts/...
Source/bindings/scripts/v8_attributes.py:194: if 'ImplementedInPrivateScript' in
extended_attributes:
Both this branch and the one before it pushes cpp_value into
'cpp_value_original' and changes cpp_value to something else. Would it work for
both branches to do that for one attribute, or are they in fact mutually
exclusive? If the latter, maybe make this "elif" instead?

https://codereview.chromium.org/362993004/diff/60001/Source/bindings/scripts/...
Source/bindings/scripts/v8_attributes.py:341: 'cpp_value_to_v8_value':
idl_type.cpp_value_to_v8_value(
This is set to a different value in getter_context(). Won't these conflict?

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

https://codereview.chromium.org/362993004/diff/60001/Source/bindings/scripts/...
Source/bindings/scripts/v8_methods.py:169: 'raw_cpp_type':
idl_type.cpp_type_args(raw_type=True),
This becomes unused now, I think.

https://codereview.chromium.org/362993004/diff/60001/Source/bindings/tests/re...
File Source/bindings/tests/results/V8TestObject.cpp (right):

https://codereview.chromium.org/362993004/diff/60001/Source/bindings/tests/re...
Source/bindings/tests/results/V8TestObject.cpp:1884:
V8HiddenValue::setHiddenValue(info.GetIsolate(), holder, propertyName,
v8Array(cppValue, scriptState->context()->Global(), scriptState->isolate()));
This change (and several below) looks wrong; there is no 'scriptState' declared
in this function.

[haraken, 2014-07-03 11:56:09]

Thanks for review!

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

https://codereview.chromium.org/362993004/diff/60001/Source/bindings/scripts/...
Source/bindings/scripts/v8_attributes.py:138: 'argument_cpp_type':
idl_type.cpp_type_args(used_as_argument=True),
On 2014/07/03 11:33:14, Jens Lindström wrote:
> Alphabetical order. :-)

Done.

https://codereview.chromium.org/362993004/diff/60001/Source/bindings/scripts/...
Source/bindings/scripts/v8_attributes.py:194: if 'ImplementedInPrivateScript' in
extended_attributes:
On 2014/07/03 11:33:14, Jens Lindström wrote:
> Both this branch and the one before it pushes cpp_value into
> 'cpp_value_original' and changes cpp_value to something else. Would it work
for
> both branches to do that for one attribute, or are they in fact mutually
> exclusive? If the latter, maybe make this "elif" instead?

Yes, changed it to elif.

https://codereview.chromium.org/362993004/diff/60001/Source/bindings/scripts/...
Source/bindings/scripts/v8_attributes.py:341: 'cpp_value_to_v8_value':
idl_type.cpp_value_to_v8_value(
On 2014/07/03 11:33:14, Jens Lindström wrote:
> This is set to a different value in getter_context(). Won't these conflict?

Nice catch, renamed it to private_script_cpp_value_to_v8_value.

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

https://codereview.chromium.org/362993004/diff/60001/Source/bindings/scripts/...
Source/bindings/scripts/v8_methods.py:169: 'raw_cpp_type':
idl_type.cpp_type_args(raw_type=True),
On 2014/07/03 11:33:14, Jens Lindström wrote:
> This becomes unused now, I think.

Removed.

https://codereview.chromium.org/362993004/diff/60001/Source/bindings/tests/re...
File Source/bindings/tests/results/V8TestObject.cpp (right):

https://codereview.chromium.org/362993004/diff/60001/Source/bindings/tests/re...
Source/bindings/tests/results/V8TestObject.cpp:1884:
V8HiddenValue::setHiddenValue(info.GetIsolate(), holder, propertyName,
v8Array(cppValue, scriptState->context()->Global(), scriptState->isolate()));
On 2014/07/03 11:33:14, Jens Lindström wrote:
> This change (and several below) looks wrong; there is no 'scriptState'
declared
> in this function.

Fixed (by resolving the conflict for cpp_value_to_v8_value).

[Jens Widell, 2014-07-03 13:23:26]

IDL compiler LGTM.

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

https://codereview.chromium.org/362993004/diff/80001/Source/bindings/scripts/...
Source/bindings/scripts/v8_types.py:484: macro =
'TONATIVE_DEFAULT_EXCEPTIONSTATE' if used_in_private_script else
'TONATIVE_VOID_EXCEPTIONSTATE'
This might be somewhat nicer if we had
"TONATIVE_(|EXCEPTIONSTATE)_(VOID|DEFAULT)" instead, in which case we would have

  macro = 'TONATIVE' / 'TONATIVE_EXCEPTIONSTATE' / 'TOSTRING'

first, then

  if used_in_private_script:
      macro += '_DEFAULT'
      args.append('false')
  else:
      macro += '_VOID'

And also, at least in this context, "DEFAULT" is a bit strange. "WITH_RETVAL"
maybe? Not overwhelmingly perfect either, but a bit less confusing. (Also
unrelated to this CL, of course.)

[haraken, 2014-07-04 00:44:00]

On 2014/07/03 13:23:26, Jens Lindström wrote:
> IDL compiler LGTM.
> 
>
https://codereview.chromium.org/362993004/diff/80001/Source/bindings/scripts/...
> File Source/bindings/scripts/v8_types.py (right):
> 
>
https://codereview.chromium.org/362993004/diff/80001/Source/bindings/scripts/...
> Source/bindings/scripts/v8_types.py:484: macro =
> 'TONATIVE_DEFAULT_EXCEPTIONSTATE' if used_in_private_script else
> 'TONATIVE_VOID_EXCEPTIONSTATE'
> This might be somewhat nicer if we had
> "TONATIVE_(|EXCEPTIONSTATE)_(VOID|DEFAULT)" instead, in which case we would
have
> 
>   macro = 'TONATIVE' / 'TONATIVE_EXCEPTIONSTATE' / 'TOSTRING'
> 
> first, then
> 
>   if used_in_private_script:
>       macro += '_DEFAULT'
>       args.append('false')
>   else:
>       macro += '_VOID'
> 
> And also, at least in this context, "DEFAULT" is a bit strange. "WITH_RETVAL"
> maybe? Not overwhelmingly perfect either, but a bit less confusing. (Also
> unrelated to this CL, of course.)

Sounds like a nice suggestion. Let me address it in a follow-up :)

[haraken, 2014-07-04 00:44:24]

abarth@: Would you take a look at PrivateScriptRunner.{h,cpp} and
Internals.{idl,js} ?

[abarth-chromium, 2014-07-04 01:28:15]

A thing of beauty is a joy forever.  LGTM

https://codereview.chromium.org/362993004/diff/80001/Source/bindings/core/v8/...
File Source/bindings/core/v8/PrivateScriptRunner.cpp (right):

https://codereview.chromium.org/362993004/diff/80001/Source/bindings/core/v8/...
Source/bindings/core/v8/PrivateScriptRunner.cpp:92: v8::Handle<v8::Value>
initializeFunction = classObject->Get(v8String(isolate, "initialize"));
s/initialize/constructor/ right?

http://dmitrysoshnikov.com/ecmascript/chapter-7-2-oop-ecmascript-implementati...

[haraken, 2014-07-04 01:32:22]

https://codereview.chromium.org/362993004/diff/80001/Source/bindings/core/v8/...
File Source/bindings/core/v8/PrivateScriptRunner.cpp (right):

https://codereview.chromium.org/362993004/diff/80001/Source/bindings/core/v8/...
Source/bindings/core/v8/PrivateScriptRunner.cpp:92: v8::Handle<v8::Value>
initializeFunction = classObject->Get(v8String(isolate, "initialize"));
On 2014/07/04 01:28:15, abarth wrote:
> s/initialize/constructor/ right?
> 
>
http://dmitrysoshnikov.com/ecmascript/chapter-7-2-oop-ecmascript-implementati...

Done.

At first I avoided to use 'constructor', since the 'constructor' in Blink-in-JS
is a bit different from the 'constructor' in ECMAScript. They are the same in a
sense that it's used for constructing an object, but they are different in a
sense that the 'constructor' in Blink-in-JS is not pointing to the prototype
object that created the object.

[haraken, 2014-07-04 01:33:44]

After landing this CL, I'll try to make your marquee element workable. I guess
it won't work for unexpected reasons, so I'll make it workable :)

[haraken, 2014-07-04 01:36:30]

The CQ bit was checked by haraken@chromium.org

[commit-bot: I haz the power, 2014-07-04 01:37:27]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/haraken@chromium.org/362993004/100001

[abarth-chromium, 2014-07-04 02:04:06]

On 2014/07/04 at 01:32:22, haraken wrote:
> At first I avoided to use 'constructor', since the 'constructor' in
Blink-in-JS is a bit different from the 'constructor' in ECMAScript. They are
the same in a sense that it's used for constructing an object, but they are
different in a sense that the 'constructor' in Blink-in-JS is not pointing to
the prototype object that created the object.

Yeah, we're creating a bit of a fiction.  However, if you define things in the
normal way, it should work automagically:

installClass("Foo", function(global) {
  function Foo() {
    this.bar = true;
  }

  Foo.prototype.alertBar = function() {
    alert(this.bar);
  }

  return Foo.prototype;
});

I'm sure we'll need to iterate on the system as we see how people use it.

[abarth-chromium, 2014-07-04 02:05:31]

On 2014/07/04 at 01:33:44, haraken wrote:
> After landing this CL, I'll try to make your marquee element workable. I guess
it won't work for unexpected reasons, so I'll make it workable :)

One trick is to figure out when to call document.register...  I was think about
hacking into HTMLElementFactory so that we call document.register only when
someone actually tries to create a marquee element.

[haraken, 2014-07-04 02:13:56]

On 2014/07/04 02:05:31, abarth wrote:
> On 2014/07/04 at 01:33:44, haraken wrote:
> > After landing this CL, I'll try to make your marquee element workable. I
guess
> it won't work for unexpected reasons, so I'll make it workable :)
> 
> One trick is to figure out when to call document.register...  I was think
about
> hacking into HTMLElementFactory so that we call document.register only when
> someone actually tries to create a marquee element.

Yeah, that sounds reasonable. When someone tries to create a marquee element, we
can compile & run the private script and call document.register. So we'll need
an API to invoke the private script from C++ (i.e., HTMLElementFactory). This is
doable.

[commit-bot: I haz the power, 2014-07-04 02:40:09]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  linux_blink_rel on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/linux_blink_rel/builds/1...)
  mac_blink_rel on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/mac_blink_rel/builds/14201)
  win_blink_rel on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/win_blink_rel/builds/15379)

[commit-bot: I haz the power, 2014-07-04 03:06:13]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-07-04 03:06:14]

Try jobs failed on following builders:
  linux_blink_rel on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/linux_blink_rel/builds/1...)

[haraken, 2014-07-04 03:13:36]

The CQ bit was checked by haraken@chromium.org

[commit-bot: I haz the power, 2014-07-04 03:14:24]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/haraken@chromium.org/362993004/100001

[commit-bot: I haz the power, 2014-07-04 03:39:19]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  linux_blink_rel on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/linux_blink_rel/builds/1...)
  mac_blink_rel on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/mac_blink_rel/builds/14206)
  win_blink_rel on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/win_blink_rel/builds/15388)

[commit-bot: I haz the power, 2014-07-04 04:03:13]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-07-04 04:03:14]

Try jobs failed on following builders:
  linux_blink_rel on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/linux_blink_rel/builds/1...)

[haraken, 2014-07-04 05:11:24]

The CQ bit was checked by haraken@chromium.org

[commit-bot: I haz the power, 2014-07-04 05:11:43]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/haraken@chromium.org/362993004/120001

[commit-bot: I haz the power, 2014-07-04 06:16:24]

Change committed as 177511

[arv (Not doing code reviews), 2014-07-09 15:48:22]

LGTM

This is truly a piece of art.

https://codereview.chromium.org/362993004/diff/120001/Source/bindings/core/v8...
File Source/bindings/core/v8/PrivateScriptRunner.cpp (right):

https://codereview.chromium.org/362993004/diff/120001/Source/bindings/core/v8...
Source/bindings/core/v8/PrivateScriptRunner.cpp:90: v8::Handle<v8::Value>
isInitialized = V8HiddenValue::getHiddenValue(isolate, holderObject,
V8HiddenValue::privateScriptObjectIsInitialized(isolate));
Hidden values are slow. Can we use a private symbol? Do we care about perf here?

[haraken, 2014-07-09 15:59:09]

https://codereview.chromium.org/362993004/diff/120001/Source/bindings/core/v8...
File Source/bindings/core/v8/PrivateScriptRunner.cpp (right):

https://codereview.chromium.org/362993004/diff/120001/Source/bindings/core/v8...
Source/bindings/core/v8/PrivateScriptRunner.cpp:90: v8::Handle<v8::Value>
isInitialized = V8HiddenValue::getHiddenValue(isolate, holderObject,
V8HiddenValue::privateScriptObjectIsInitialized(isolate));
On 2014/07/09 15:48:22, arv wrote:
> Hidden values are slow. Can we use a private symbol? Do we care about perf
here?

We have a plan to replace the implementation of
V8HiddenValue::getHiddenValue/setHiddenValue/deleteHiddenValue so that they use
private names (but I'm not actively working on it at the moment).

(This place is not performance-sensitive.)

[haraken, 2014-07-11 04:29:56]

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

https://codereview.chromium.org/362993004/diff/80001/Source/bindings/scripts/...
Source/bindings/scripts/v8_types.py:484: macro =
'TONATIVE_DEFAULT_EXCEPTIONSTATE' if used_in_private_script else
'TONATIVE_VOID_EXCEPTIONSTATE'
On 2014/07/03 13:23:26, Jens Lindström wrote:
> This might be somewhat nicer if we had
> "TONATIVE_(|EXCEPTIONSTATE)_(VOID|DEFAULT)" instead, in which case we would
have
> 
>   macro = 'TONATIVE' / 'TONATIVE_EXCEPTIONSTATE' / 'TOSTRING'
> 
> first, then
> 
>   if used_in_private_script:
>       macro += '_DEFAULT'
>       args.append('false')
>   else:
>       macro += '_VOID'
> 
> And also, at least in this context, "DEFAULT" is a bit strange. "WITH_RETVAL"
> maybe? Not overwhelmingly perfect either, but a bit less confusing. (Also
> unrelated to this CL, of course.)

I took a look at this. I agree that "TONATIVE_(|EXCEPTIONSTATE)_(VOID|DEFAULT)"
makes the code generator nicer, but in terms of the macro name,
TO*_RETURNTYPE[_ARGTYPE] would make more sense, as commented in
V8BindingMacros.h. So I'm leaning toward keeping the current name as is.