[OriginTrials] Allow origin trials to be installed without an instance object.

[OriginTrials] Allow origin trials to be installed without an instance object.

Many origin trial features can be installed on interfaces and prototype
objects without requiring an existing interface object. This creates an
alternate method signature for installing such features, and simplifies
the bindings installation code.

BUG=626435
R=yukishiino@chromium.org

Committed: https://crrev.com/4d48d54439b76ce3736de03206f6c0699d497ec6
Cr-Commit-Position: refs/heads/master@{#404455}

[iclelland, 2016-07-07 21:30:58]

+r Yuki, can you PTAL to make sure this makes sense? I think it should be okay
to get the prototype object in the same manner that we can get the interface
object, but I'm not 100% sure.

Thanks!

[haraken, 2016-07-08 01:59:06]

LGTM on my side.

We need to revisit the issue when someone wants to add [OriginTrialEnabled] on
things that need to be installed on the instance object though (e.g.,
[Unforgeable]). I agree that it's less likely to happen.

[iclelland, 2016-07-08 03:56:33]

On 2016/07/08 01:59:06, haraken wrote:
> LGTM on my side.
> 
> We need to revisit the issue when someone wants to add [OriginTrialEnabled] on
> things that need to be installed on the instance object though (e.g.,
> [Unforgeable]). I agree that it's less likely to happen.

That's true -- we can't really handle OnInstance when there are an arbitrary
number of already-created objects in the world, since we're not modifying the
instance template. This code still handles OnInstance for items such as
interfaces, which need to be installed on the global object.

[Yuki, 2016-07-08 13:08:24]

Mostly, l-g-t-m.

https://codereview.chromium.org/2130883002/diff/1/third_party/WebKit/Source/b...
File third_party/WebKit/Source/bindings/templates/interface_base.cpp (right):

https://codereview.chromium.org/2130883002/diff/1/third_party/WebKit/Source/b...
third_party/WebKit/Source/bindings/templates/interface_base.cpp:372: {% if
origin_trial_feature.needs_instance %}
I'm uneasy with this pattern.  The current pattern seems
    {% if %}
    void foo() {
        do_something();
    {% else %}
    void foo(T) {
        do_something_else();
    {% endif %}
        do_common_thing();
    }
I don't like this pattern because open and close parentheses do not match and
hard to follow the code.

I think this part is not performance-sensitive, so I'd recommend the following
pattern.
    void foo(scriptState, instance) {
        do_common_things();
        if (instance.IsEmpty()) {
            do_something();
        } else {
            do_something_else();
        }
    }
and, if you want, we can optionally have the following thin wrapper.
    void foo(scriptState) {
        foo(scriptState, v8::Local<v8::Object>());
    }

Maybe, something will be redundant, but I don't care much.  I think this way is
easier to read the code.

https://codereview.chromium.org/2130883002/diff/1/third_party/WebKit/Source/b...
third_party/WebKit/Source/bindings/templates/interface_base.cpp:394:
v8::Local<v8::FunctionTemplate> interfaceTemplate =
{{v8_class}}::wrapperTypeInfo.domTemplate(scriptState->isolate(),
scriptState->world());
Just FYI,

If the interface is already defined, you can retrieve it using
perIsolateData->findInterfaceTemplate().

Regardless of whether the interface is already defined or not yet defined, you
can retrieve it using {{v8_class}}::domTemplate().  You can use domTemplate()
for both cases that you have |instance| and you do not have |instance|.

[iclelland, 2016-07-08 14:20:49]

https://codereview.chromium.org/2130883002/diff/1/third_party/WebKit/Source/b...
File third_party/WebKit/Source/bindings/templates/interface_base.cpp (right):

https://codereview.chromium.org/2130883002/diff/1/third_party/WebKit/Source/b...
third_party/WebKit/Source/bindings/templates/interface_base.cpp:372: {% if
origin_trial_feature.needs_instance %}
On 2016/07/08 13:08:24, Yuki wrote:
> I'm uneasy with this pattern.  The current pattern seems
>     {% if %}
>     void foo() {
>         do_something();
>     {% else %}
>     void foo(T) {
>         do_something_else();
>     {% endif %}
>         do_common_thing();
>     }
> I don't like this pattern because open and close parentheses do not match and
> hard to follow the code.
> 

That's a fair criticism; I wasn't completely sure myself, but it seemed to
generate the cleanest code while minimizing duplication within the template.

> I think this part is not performance-sensitive, so I'd recommend the following
> pattern.
>     void foo(scriptState, instance) {
>         do_common_things();
>         if (instance.IsEmpty()) {
>             do_something();
>         } else {
>             do_something_else();
>         }
>     }
> and, if you want, we can optionally have the following thin wrapper.
>     void foo(scriptState) {
>         foo(scriptState, v8::Local<v8::Object>());
>     }
> 
> Maybe, something will be redundant, but I don't care much.  I think this way
is
> easier to read the code.

sgtm; I'll rewrite it with that pattern. We can revisit if it turns out to be
performance-critical at some point.

https://codereview.chromium.org/2130883002/diff/1/third_party/WebKit/Source/b...
third_party/WebKit/Source/bindings/templates/interface_base.cpp:394:
v8::Local<v8::FunctionTemplate> interfaceTemplate =
{{v8_class}}::wrapperTypeInfo.domTemplate(scriptState->isolate(),
scriptState->world());
On 2016/07/08 13:08:24, Yuki wrote:
> Just FYI,
> 
> If the interface is already defined, you can retrieve it using
> perIsolateData->findInterfaceTemplate().

Yes. There are cases where that isn't possible, though -- I believe I used that
originally, and found it didn't work when I tried to retrieve the
OriginTrialsTest interface (this was when testing the static-attributes-cl).
Thanks for the pointer, though.

 
> Regardless of whether the interface is already defined or not yet defined, you
> can retrieve it using {{v8_class}}::domTemplate().  You can use domTemplate()
> for both cases that you have |instance| and you do not have |instance|.

I will switch to use that in both cases, to combine more code.

[iclelland, 2016-07-08 14:48:26]

Yuki, I think this addresses your concerns, can you PTAL?

After refactoring, I realized that I was only using instance.IsEmpty to
determine how to get the prototype object. I think that I can use perContextData
in every case, and them eliminate the condition completely.

https://codereview.chromium.org/2130883002/diff/1/third_party/WebKit/Source/b...
File third_party/WebKit/Source/bindings/templates/interface_base.cpp (right):

https://codereview.chromium.org/2130883002/diff/1/third_party/WebKit/Source/b...
third_party/WebKit/Source/bindings/templates/interface_base.cpp:372: {% if
origin_trial_feature.needs_instance %}
On 2016/07/08 14:20:49, iclelland wrote:
> On 2016/07/08 13:08:24, Yuki wrote:
> > I'm uneasy with this pattern.  The current pattern seems
> >     {% if %}
> >     void foo() {
> >         do_something();
> >     {% else %}
> >     void foo(T) {
> >         do_something_else();
> >     {% endif %}
> >         do_common_thing();
> >     }
> > I don't like this pattern because open and close parentheses do not match
and
> > hard to follow the code.
> > 
> 
> That's a fair criticism; I wasn't completely sure myself, but it seemed to
> generate the cleanest code while minimizing duplication within the template.
> 
> > I think this part is not performance-sensitive, so I'd recommend the
following
> > pattern.
> >     void foo(scriptState, instance) {
> >         do_common_things();
> >         if (instance.IsEmpty()) {
> >             do_something();
> >         } else {
> >             do_something_else();
> >         }
> >     }
> > and, if you want, we can optionally have the following thin wrapper.
> >     void foo(scriptState) {
> >         foo(scriptState, v8::Local<v8::Object>());
> >     }
> > 
> > Maybe, something will be redundant, but I don't care much.  I think this way
> is
> > easier to read the code.
> 
> sgtm; I'll rewrite it with that pattern. We can revisit if it turns out to be
> performance-critical at some point.

Done.

https://codereview.chromium.org/2130883002/diff/1/third_party/WebKit/Source/b...
third_party/WebKit/Source/bindings/templates/interface_base.cpp:394:
v8::Local<v8::FunctionTemplate> interfaceTemplate =
{{v8_class}}::wrapperTypeInfo.domTemplate(scriptState->isolate(),
scriptState->world());
On 2016/07/08 14:20:49, iclelland wrote:
> On 2016/07/08 13:08:24, Yuki wrote:
> > Just FYI,
> > 
> > If the interface is already defined, you can retrieve it using
> > perIsolateData->findInterfaceTemplate().
> 
> Yes. There are cases where that isn't possible, though -- I believe I used
that
> originally, and found it didn't work when I tried to retrieve the
> OriginTrialsTest interface (this was when testing the static-attributes-cl).
> Thanks for the pointer, though.
> 
>  
> > Regardless of whether the interface is already defined or not yet defined,
you
> > can retrieve it using {{v8_class}}::domTemplate().  You can use
domTemplate()
> > for both cases that you have |instance| and you do not have |instance|.
> 
> I will switch to use that in both cases, to combine more code.

Done.

[Yuki, 2016-07-08 16:10:30]

LGTM.  Thanks!

[iclelland, 2016-07-08 17:47:35]

The CQ bit was checked by iclelland@chromium.org

[iclelland, 2016-07-08 17:47:35]

The patchset sent to the CQ was uploaded after l-g-t-m from
haraken@chromium.org, yukishiino@chromium.org
Link to the patchset: https://codereview.chromium.org/2130883002/#ps60001
(title: "Rebase")

[commit-bot: I haz the power, 2016-07-08 17:48:39]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-07-08 19:13:38]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2016-07-08 19:15:40]

Description was changed from

==========
[OriginTrials] Allow origin trials to be installed without an instance object.

Many origin trial features can be installed on interfaces and prototype
objects without requiring an existing interface object. This creates an
alternate method signature for installing such features, and simplifies
the bindings installation code.

BUG=626435
R=yukishiino@chromium.org
==========

to

==========
[OriginTrials] Allow origin trials to be installed without an instance object.

Many origin trial features can be installed on interfaces and prototype
objects without requiring an existing interface object. This creates an
alternate method signature for installing such features, and simplifies
the bindings installation code.

BUG=626435
R=yukishiino@chromium.org

Committed: https://crrev.com/4d48d54439b76ce3736de03206f6c0699d497ec6
Cr-Commit-Position: refs/heads/master@{#404455}
==========

[commit-bot: I haz the power, 2016-07-08 19:15:41]

Patchset 4 (id:??) landed as
https://crrev.com/4d48d54439b76ce3736de03206f6c0699d497ec6
Cr-Commit-Position: refs/heads/master@{#404455}

[Marijn Kruisselbrink, 2016-07-08 19:44:08]

mek@chromium.org changed reviewers:
+ haraken@chromium.org

[Marijn Kruisselbrink, 2016-07-08 19:44:09]

https://codereview.chromium.org/2130883002/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/bindings/scripts/v8_interface.py (right):

https://codereview.chromium.org/2130883002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/bindings/scripts/v8_interface.py:119: members_by_name
= itertools.groupby(origin_trial_members,
itemgetter('origin_trial_feature_name'))
itertools.groupby requires the passed in iterable to be sorted already by the
key you're grouping on. You don't seem to actually sort origin_trial_members,
thus this can result in duplicates ending up in members_by_name (and thus also
in features later on).