bindings: Supports per-member [Exposed] for attributes on prototype.

bindings: Supports per-member [Exposed] for attributes on prototype.

Support per-member [Exposed] extended attribute on the prototype object.
This CL does NOT support ones on the instance or interface object.

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

[Yuki, 2015-06-23 07:51:32]

yukishiino@chromium.org changed reviewers:
+ bashi@chromium.org, haraken@chromium.org

[Yuki, 2015-06-23 07:51:32]

Could you guys review this CL?

[bashi, 2015-06-23 08:21:03]

Do we have a crbug entry for this? It would be nice to have BUG=[actual issue
number].

https://codereview.chromium.org/1202683002/diff/1/Source/bindings/core/v8/V8D...
File Source/bindings/core/v8/V8DOMConfiguration.cpp (right):

https://codereview.chromium.org/1202683002/diff/1/Source/bindings/core/v8/V8D...
Source/bindings/core/v8/V8DOMConfiguration.cpp:128:
instanceOrTemplate->SetAccessorProperty(name, getter, setter,
accessor.attribute, accessor.settings);
Is it safe to pass empty |getter| or |setter|? If not (or unclear), I'd prefer
immediate crash in createAccessorFunctionOrTemplate() on V8 API failures.

https://codereview.chromium.org/1202683002/diff/1/Source/bindings/scripts/v8_...
File Source/bindings/scripts/v8_interface.py (right):

https://codereview.chromium.org/1202683002/diff/1/Source/bindings/scripts/v8_...
Source/bindings/scripts/v8_interface.py:575: unscopeables or
It seems that |unscopeables| do nothing with conditionally enabled members
(apart from how we handle them). It might be better to keep them be separated?

https://codereview.chromium.org/1202683002/diff/1/Source/bindings/templates/i...
File Source/bindings/templates/interface.cpp (right):

https://codereview.chromium.org/1202683002/diff/1/Source/bindings/templates/i...
Source/bindings/templates/interface.cpp:803: #error TODO(yukishiino): Rename
this function to prepareInstanceObject (c.f. preparePrototypeObject) and
implement this function if necessary.
Crbug number?

[Yuki, 2015-06-23 08:36:05]

https://codereview.chromium.org/1202683002/diff/1/Source/bindings/core/v8/V8D...
File Source/bindings/core/v8/V8DOMConfiguration.cpp (right):

https://codereview.chromium.org/1202683002/diff/1/Source/bindings/core/v8/V8D...
Source/bindings/core/v8/V8DOMConfiguration.cpp:128:
instanceOrTemplate->SetAccessorProperty(name, getter, setter,
accessor.attribute, accessor.settings);
On 2015/06/23 08:21:03, bashi1 wrote:
> Is it safe to pass empty |getter| or |setter|? If not (or unclear), I'd prefer
> immediate crash in createAccessorFunctionOrTemplate() on V8 API failures.

Yes, it's safe and expected.

https://codereview.chromium.org/1202683002/diff/1/Source/bindings/scripts/v8_...
File Source/bindings/scripts/v8_interface.py (right):

https://codereview.chromium.org/1202683002/diff/1/Source/bindings/scripts/v8_...
Source/bindings/scripts/v8_interface.py:575: unscopeables or
On 2015/06/23 08:21:03, bashi1 wrote:
> It seems that |unscopeables| do nothing with conditionally enabled members
> (apart from how we handle them). It might be better to keep them be separated?

Good catch!  Done.

https://codereview.chromium.org/1202683002/diff/1/Source/bindings/templates/i...
File Source/bindings/templates/interface.cpp (right):

https://codereview.chromium.org/1202683002/diff/1/Source/bindings/templates/i...
Source/bindings/templates/interface.cpp:803: #error TODO(yukishiino): Rename
this function to prepareInstanceObject (c.f. preparePrototypeObject) and
implement this function if necessary.
On 2015/06/23 08:21:03, bashi1 wrote:
> Crbug number?

Done.

[bashi, 2015-06-23 08:43:49]

LGTM from my side.

[haraken, 2015-06-23 09:14:07]

Mostly looks good.

https://codereview.chromium.org/1202683002/diff/40001/Source/bindings/core/v8...
File Source/bindings/core/v8/V8DOMConfiguration.cpp (right):

https://codereview.chromium.org/1202683002/diff/40001/Source/bindings/core/v8...
Source/bindings/core/v8/V8DOMConfiguration.cpp:134:
v8::Local<FunctionOrTemplate> setter =
createAccessorFunctionOrTemplate<FunctionOrTemplate>(isolate, setterCallback,
data, v8::Local<v8::Signature>(), 1);

Just help me understand: Are you intentionally passing
v8::Local<v8::Signature>(), instead of signature? What happens if we pass
signature?

https://codereview.chromium.org/1202683002/diff/40001/Source/bindings/templat...
File Source/bindings/templates/interface.cpp (right):

https://codereview.chromium.org/1202683002/diff/40001/Source/bindings/templat...
Source/bindings/templates/interface.cpp:821: ExecutionContext* context =
toExecutionContext(prototypeObject->CreationContext());

What is the ExecutionContext for?

https://codereview.chromium.org/1202683002/diff/40001/Source/bindings/templat...
File Source/bindings/templates/partial_interface.cpp (right):

https://codereview.chromium.org/1202683002/diff/40001/Source/bindings/templat...
Source/bindings/templates/partial_interface.cpp:5: {% block
prepare_prototype_object %}

Would it be hard to remove the code duplication with the one in interface.cpp?
To remove the duplication, maybe you'll need to split
has_conditional_member_on_prototype into has_conditional_attributes_on_prototype
and conditionally_enabled_methods.

https://codereview.chromium.org/1202683002/diff/40001/Source/bindings/templat...
Source/bindings/templates/partial_interface.cpp:16: ExecutionContext* context =
toExecutionContext(prototypeObject->CreationContext());

What is the ExecutionContext for?

[Yuki, 2015-06-23 12:41:38]

https://codereview.chromium.org/1202683002/diff/40001/Source/bindings/core/v8...
File Source/bindings/core/v8/V8DOMConfiguration.cpp (right):

https://codereview.chromium.org/1202683002/diff/40001/Source/bindings/core/v8...
Source/bindings/core/v8/V8DOMConfiguration.cpp:134:
v8::Local<FunctionOrTemplate> setter =
createAccessorFunctionOrTemplate<FunctionOrTemplate>(isolate, setterCallback,
data, v8::Local<v8::Signature>(), 1);
On 2015/06/23 09:14:07, haraken wrote:
> 
> Just help me understand: Are you intentionally passing
> v8::Local<v8::Signature>(), instead of signature? What happens if we pass
> signature?

If non-empty Signature is specified, V8 checks if info.Holder() is an instance
of the FunctionTemplate or not.  It's very useful for regular attributes and
operations, but harmful for static attributes and operations (and
[LenientThis]).

If we specify a non-empty Signature, you need to pass an instance of the
interface as |this| in JS when you access to that (static) attribute.

https://codereview.chromium.org/1202683002/diff/40001/Source/bindings/templat...
File Source/bindings/templates/interface.cpp (right):

https://codereview.chromium.org/1202683002/diff/40001/Source/bindings/templat...
Source/bindings/templates/interface.cpp:821: ExecutionContext* context =
toExecutionContext(prototypeObject->CreationContext());
On 2015/06/23 09:14:07, haraken wrote:
> 
> What is the ExecutionContext for?

To check what is the global object.

For [Exposed=Worker], generated code looks like:
    if (context && context->isWorkerGlobalScope()) {
        // register an attribute
    }

This check is the same as before I removed it.

https://codereview.chromium.org/1202683002/diff/40001/Source/bindings/templat...
File Source/bindings/templates/partial_interface.cpp (right):

https://codereview.chromium.org/1202683002/diff/40001/Source/bindings/templat...
Source/bindings/templates/partial_interface.cpp:5: {% block
prepare_prototype_object %}
On 2015/06/23 09:14:07, haraken wrote:
> 
> Would it be hard to remove the code duplication with the one in interface.cpp?
> To remove the duplication, maybe you'll need to split
> has_conditional_member_on_prototype into
has_conditional_attributes_on_prototype
> and conditionally_enabled_methods.

Done.

https://codereview.chromium.org/1202683002/diff/40001/Source/bindings/templat...
Source/bindings/templates/partial_interface.cpp:16: ExecutionContext* context =
toExecutionContext(prototypeObject->CreationContext());
On 2015/06/23 09:14:07, haraken wrote:
> 
> What is the ExecutionContext for?

Done.

[haraken, 2015-06-23 12:52:36]

LGTM

https://codereview.chromium.org/1202683002/diff/60001/Source/bindings/templat...
File Source/bindings/templates/interface.cpp (right):

https://codereview.chromium.org/1202683002/diff/60001/Source/bindings/templat...
Source/bindings/templates/interface.cpp:813: {% if
has_conditional_member_on_prototype or unscopeables %}

Maybe it's clearer to just write:

  unscopeables or has_conditional_attributes_on_prototype or
conditionally_enabled_methods

since the following if statements list up the conditions. (i.e.,
has_conditional_member_on_prototype won't be needed.)

(I think you're planning to rename conditionally_enabled_methods to
has_conditional_methods_on_prototype.)

[Yuki, 2015-06-23 13:10:32]

https://codereview.chromium.org/1202683002/diff/60001/Source/bindings/templat...
File Source/bindings/templates/interface.cpp (right):

https://codereview.chromium.org/1202683002/diff/60001/Source/bindings/templat...
Source/bindings/templates/interface.cpp:813: {% if
has_conditional_member_on_prototype or unscopeables %}
On 2015/06/23 12:52:36, haraken wrote:
> 
> Maybe it's clearer to just write:
> 
>   unscopeables or has_conditional_attributes_on_prototype or
> conditionally_enabled_methods
> 
> since the following if statements list up the conditions. (i.e.,
> has_conditional_member_on_prototype won't be needed.)
> 
> (I think you're planning to rename conditionally_enabled_methods to
> has_conditional_methods_on_prototype.)

Done.

[Yuki, 2015-06-23 13:10:41]

The CQ bit was checked by yukishiino@chromium.org

[Yuki, 2015-06-23 13:10:41]

The patchset sent to the CQ was uploaded after l-g-t-m from bashi@chromium.org,
haraken@chromium.org
Link to the patchset: https://codereview.chromium.org/1202683002/#ps80001
(title: "Addressed a review comment.")

[commit-bot: I haz the power, 2015-06-23 13:10:47]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1202683002/80001

[commit-bot: I haz the power, 2015-06-23 14:42:43]

Committed patchset #5 (id:80001) as
https://src.chromium.org/viewvc/blink?view=rev&revision=197651