Enable the WebIDL [Exposed] annotation on an interface's members.

Enable the WebIDL [Exposed] annotation on an interface's members.

Blink only supports [Exposed] as an interface-level annotation right now,
but it also makes sense as a member-level annotation in case certain members
should only be available in a subset of the interface's exposure set.

The first user for this will be the Web Notifications API, which now
defines requestPermission() to only be exposed on Document, whereas the rest
of the API will be available in both windows and workers.

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

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

[Peter Beverloo, 2014-07-29 17:40:26]

First draft with some questions.

It'd be great if you'd have some time to look at this! I'll continue working on
the patch tomorrow.

https://codereview.chromium.org/424163002/diff/20001/Source/bindings/scripts/...
File Source/bindings/scripts/v8_utilities.py (right):

https://codereview.chromium.org/424163002/diff/20001/Source/bindings/scripts/...
Source/bindings/scripts/v8_utilities.py:235: # as |ExtendedAttributeIdentList|
per the WebIDL parsing rules.
Note: This was resolved 12 hours ago, and hasn't been implemented in Blink yet.

https://github.com/heycam/webidl/commit/68ebd7772eb5b4b2e10bd8bdee56145c77e318cf

https://codereview.chromium.org/424163002/diff/20001/Source/bindings/scripts/...
Source/bindings/scripts/v8_utilities.py:249:
exposure_checks.append('context->is%s()' % environment)
Do we have a canonical way of checking what kind of environment |context| (EC)
is? There are the isDocument and isWorkerGlobalScope checks, but these don't
have the granularity of different kinds of workers.

https://codereview.chromium.org/424163002/diff/20001/Source/bindings/scripts/...
Source/bindings/scripts/v8_utilities.py:251: return ' || '.join(exposure_checks)
Is this the correct layer to join the checks together? I couldn't find much
precedence for this in the templates.

https://codereview.chromium.org/424163002/diff/20001/Source/bindings/tests/id...
File Source/bindings/tests/idls/TestExposedInterface.idl (right):

https://codereview.chromium.org/424163002/diff/20001/Source/bindings/tests/id...
Source/bindings/tests/idls/TestExposedInterface.idl:20:
[Exposed=Window&ServiceWorker] void windowAndServiceWorkerExposedMethod();
Note: We need some way to recognize that "ServiceWorker" (and "SharedWorker",
"DedicatedWorker") are part of "Worker", which is a group definition.

https://codereview.chromium.org/424163002/diff/20001/Source/bindings/tests/re...
File Source/bindings/tests/results/V8TestExposedInterface.cpp (right):

https://codereview.chromium.org/424163002/diff/20001/Source/bindings/tests/re...
Source/bindings/tests/results/V8TestExposedInterface.cpp:296: if (context &&
(context->isWorker()))
Note: It'd be good to group methods and attributes sharing conditional rules
together, to avoid excess calls and branches.

[Jens Widell, 2014-07-29 18:24:30]

The design looks good to me; doesn't seem to add much complexity, which is nice.

Some comments, mostly on minor style issues.

https://codereview.chromium.org/424163002/diff/20001/Source/bindings/scripts/...
File Source/bindings/scripts/v8_interface.py (right):

https://codereview.chromium.org/424163002/diff/20001/Source/bindings/scripts/...
Source/bindings/scripts/v8_interface.py:244: 'has_conditional_attributes':
any(attribute['per_context_enabled_function'] or
attribute['custom_exposed_rules'] for attribute in attributes),
Should be moved up one line (alphabetical order).

https://codereview.chromium.org/424163002/diff/20001/Source/bindings/scripts/...
Source/bindings/scripts/v8_interface.py:322: 'has_conditional_methods':
per_context_enabled_methods or conditionally_exposed_methods,
These dictionary literals should be sorted alphabetically.

https://codereview.chromium.org/424163002/diff/20001/Source/bindings/scripts/...
Source/bindings/scripts/v8_interface.py:456: 'custom_exposed_rules_all':
common_value(overloads, 'custom_exposed_rules'),  # [Exposed]
Alphabetical order.

https://codereview.chromium.org/424163002/diff/20001/Source/bindings/scripts/...
File Source/bindings/scripts/v8_utilities.py (right):

https://codereview.chromium.org/424163002/diff/20001/Source/bindings/scripts/...
Source/bindings/scripts/v8_utilities.py:237: interface_exposure_set = ('Window')
 # Default value
This ought to be ['Window'] or ('Window',). You're setting the variable to a
plain string here.

https://codereview.chromium.org/424163002/diff/20001/Source/bindings/scripts/...
Source/bindings/scripts/v8_utilities.py:251: return ' || '.join(exposure_checks)
You ought to sort somehow here, otherwise code generation could vary randomly.

Or you could keep exposure_set a list and convert temporarily to a set when used
as such.

https://codereview.chromium.org/424163002/diff/20001/Source/bindings/template...
File Source/bindings/templates/interface.cpp (right):

https://codereview.chromium.org/424163002/diff/20001/Source/bindings/template...
Source/bindings/templates/interface.cpp:1170: {% else %}
Are these really mutually exclusive?

Also, we could use {% filter %} here, like we do for e.g. runtime enabled stuff.
Don't know if there's a policy for when to use it and when to just generate an
if "manually".

https://codereview.chromium.org/424163002/diff/20001/Source/bindings/tests/id...
File Source/bindings/tests/idls/TestExposedInterface.idl (right):

https://codereview.chromium.org/424163002/diff/20001/Source/bindings/tests/id...
Source/bindings/tests/idls/TestExposedInterface.idl:6:
Exposed=Worker&Window&ServiceWorker
We typically prefer to not add additional files here unless necessary. Can this
[Exposed] and the various attributes and methods be added to TestObject.idl
without interfering with anything already there?

[Inactive, 2014-07-29 18:54:47]

https://codereview.chromium.org/424163002/diff/20001/Source/bindings/tests/id...
File Source/bindings/tests/idls/TestExposedInterface.idl (right):

https://codereview.chromium.org/424163002/diff/20001/Source/bindings/tests/id...
Source/bindings/tests/idls/TestExposedInterface.idl:6:
Exposed=Worker&Window&ServiceWorker
On 2014/07/29 18:24:29, Jens Lindström wrote:
> We typically prefer to not add additional files here unless necessary. Can
this
> [Exposed] and the various attributes and methods be added to TestObject.idl
> without interfering with anything already there?

Note that the new spec says this should be:
Exposed=(Worker, Window, ServiceWorker)

[haraken, 2014-07-29 20:13:11]

(I'm happy to take another look once Jens comments are addressed.)

https://codereview.chromium.org/424163002/diff/20001/Source/bindings/scripts/...
File Source/bindings/scripts/v8_utilities.py (right):

https://codereview.chromium.org/424163002/diff/20001/Source/bindings/scripts/...
Source/bindings/scripts/v8_utilities.py:249:
exposure_checks.append('context->is%s()' % environment)
On 2014/07/29 17:40:26, Peter Beverloo wrote:
> Do we have a canonical way of checking what kind of environment |context| (EC)
> is? There are the isDocument and isWorkerGlobalScope checks, but these don't
> have the granularity of different kinds of workers.

As far as I know, we currently don't have such methods. Probably it's worth
introducing them.

[bashi, 2014-07-30 00:34:34]

https://codereview.chromium.org/424163002/diff/20001/Source/bindings/scripts/...
File Source/bindings/scripts/v8_utilities.py (right):

https://codereview.chromium.org/424163002/diff/20001/Source/bindings/scripts/...
Source/bindings/scripts/v8_utilities.py:236: exposure_set =
set(extended_attributes['Exposed'].split('&'))
extended_attributes['Exposed'].split('[|&]') ?

I understand that the grammar of value list has changed, but we might want to
add a helper function for this.

[Peter Beverloo, 2014-07-30 14:48:16]

Thank you for the great comments so far!!

https://codereview.chromium.org/424163002/diff/20001/Source/bindings/scripts/...
File Source/bindings/scripts/v8_interface.py (right):

https://codereview.chromium.org/424163002/diff/20001/Source/bindings/scripts/...
Source/bindings/scripts/v8_interface.py:244: 'has_conditional_attributes':
any(attribute['per_context_enabled_function'] or
attribute['custom_exposed_rules'] for attribute in attributes),
On 2014/07/29 18:24:29, Jens Lindström wrote:
> Should be moved up one line (alphabetical order).

Done.

https://codereview.chromium.org/424163002/diff/20001/Source/bindings/scripts/...
Source/bindings/scripts/v8_interface.py:322: 'has_conditional_methods':
per_context_enabled_methods or conditionally_exposed_methods,
On 2014/07/29 18:24:29, Jens Lindström wrote:
> These dictionary literals should be sorted alphabetically.

Done.

https://codereview.chromium.org/424163002/diff/20001/Source/bindings/scripts/...
Source/bindings/scripts/v8_interface.py:456: 'custom_exposed_rules_all':
common_value(overloads, 'custom_exposed_rules'),  # [Exposed]
On 2014/07/29 18:24:29, Jens Lindström wrote:
> Alphabetical order.

Done.

https://codereview.chromium.org/424163002/diff/20001/Source/bindings/scripts/...
File Source/bindings/scripts/v8_utilities.py (right):

https://codereview.chromium.org/424163002/diff/20001/Source/bindings/scripts/...
Source/bindings/scripts/v8_utilities.py:236: exposure_set =
set(extended_attributes['Exposed'].split('&'))
On 2014/07/30 00:34:34, bashi1 wrote:
> extended_attributes['Exposed'].split('[|&]') ?
> 
> I understand that the grammar of value list has changed, but we might want to
> add a helper function for this.

I added |sorted_extended_attribute_set| for the general case, and a
specialization for the exposed attribute as we also have to expand some values
(Worker = DedicatedWorker&SharedWorker&ServiceWorker).

https://codereview.chromium.org/424163002/diff/20001/Source/bindings/scripts/...
Source/bindings/scripts/v8_utilities.py:237: interface_exposure_set = ('Window')
 # Default value
On 2014/07/29 18:24:29, Jens Lindström wrote:
> This ought to be ['Window'] or ('Window',). You're setting the variable to a
> plain string here.

Heh, thanks! Done

https://codereview.chromium.org/424163002/diff/20001/Source/bindings/scripts/...
Source/bindings/scripts/v8_utilities.py:249:
exposure_checks.append('context->is%s()' % environment)
On 2014/07/29 20:13:10, haraken wrote:
> On 2014/07/29 17:40:26, Peter Beverloo wrote:
> > Do we have a canonical way of checking what kind of environment |context|
(EC)
> > is? There are the isDocument and isWorkerGlobalScope checks, but these don't
> > have the granularity of different kinds of workers.
> 
> As far as I know, we currently don't have such methods. Probably it's worth
> introducing them.

Done.

https://codereview.chromium.org/424163002/diff/20001/Source/bindings/scripts/...
Source/bindings/scripts/v8_utilities.py:251: return ' || '.join(exposure_checks)
On 2014/07/29 18:24:29, Jens Lindström wrote:
> You ought to sort somehow here, otherwise code generation could vary randomly.
> 
> Or you could keep exposure_set a list and convert temporarily to a set when
used
> as such.

Done.

https://codereview.chromium.org/424163002/diff/20001/Source/bindings/template...
File Source/bindings/templates/interface.cpp (right):

https://codereview.chromium.org/424163002/diff/20001/Source/bindings/template...
Source/bindings/templates/interface.cpp:1170: {% else %}
On 2014/07/29 18:24:29, Jens Lindström wrote:
> Are these really mutually exclusive?
> 
> Also, we could use {% filter %} here, like we do for e.g. runtime enabled
stuff.
> Don't know if there's a policy for when to use it and when to just generate an
> if "manually".

No, they are not. It was just easier to the first draft patch as I have zero
experience with Jinja and have no idea what I'm doing :-).

The |conditional| filter is used for compile-time #ifdefs, so we can't reuse
that. Neither of the other filters looks relevant either. Ideally, I would only
define |context| if any of the attributes have |custom_exposed_rules| as well.

Do you think I should add a |has_custom_exposed_attributes| variable to the
global context to guard definition of |context|? I could group the conditions
together on the Python side, exposing |conditional_attributes| as a list of
[condition, list of attributes it applies to].

https://codereview.chromium.org/424163002/diff/20001/Source/bindings/tests/id...
File Source/bindings/tests/idls/TestExposedInterface.idl (right):

https://codereview.chromium.org/424163002/diff/20001/Source/bindings/tests/id...
Source/bindings/tests/idls/TestExposedInterface.idl:6:
Exposed=Worker&Window&ServiceWorker
On 2014/07/29 18:54:47, Chris Dumez wrote:
> On 2014/07/29 18:24:29, Jens Lindström wrote:
> > We typically prefer to not add additional files here unless necessary. Can
> this
> > [Exposed] and the various attributes and methods be added to TestObject.idl
> > without interfering with anything already there?
> 
> Note that the new spec says this should be:
> Exposed=(Worker, Window, ServiceWorker)

I don't mind updating the syntax, but not as part of this patch :-).

https://codereview.chromium.org/424163002/diff/20001/Source/bindings/tests/id...
Source/bindings/tests/idls/TestExposedInterface.idl:6:
Exposed=Worker&Window&ServiceWorker
On 2014/07/29 18:24:29, Jens Lindström wrote:
> We typically prefer to not add additional files here unless necessary. Can
this
> [Exposed] and the various attributes and methods be added to TestObject.idl
> without interfering with anything already there?

Ack. I'll split it up later, as I'd like to keep a separate file until we get
slightly further in the review, as it helps me verify the changes.

[Jens Widell, 2014-07-30 15:16:18]

https://codereview.chromium.org/424163002/diff/20001/Source/bindings/template...
File Source/bindings/templates/interface.cpp (right):

https://codereview.chromium.org/424163002/diff/20001/Source/bindings/template...
Source/bindings/templates/interface.cpp:1170: {% else %}
On 2014/07/30 14:48:15, Peter Beverloo wrote:
> On 2014/07/29 18:24:29, Jens Lindström wrote:
> > Are these really mutually exclusive?
> > 
> > Also, we could use {% filter %} here, like we do for e.g. runtime enabled
> stuff.
> > Don't know if there's a policy for when to use it and when to just generate
an
> > if "manually".
> 
> No, they are not. It was just easier to the first draft patch as I have zero
> experience with Jinja and have no idea what I'm doing :-).
> 
> The |conditional| filter is used for compile-time #ifdefs, so we can't reuse
> that. Neither of the other filters looks relevant either. Ideally, I would
only
> define |context| if any of the attributes have |custom_exposed_rules| as well.

My thinking was that you would introduce a filters (one for per_context_enabled
and one for custom_exposed) and end up with

  {% filter per_context_enabled(attribute.per_context_enabled_function) %}
  {% filter custom_exposed(attribute.custom_exposed_rules) %}
  // the code
  {% endfilter %}
  {% endfilter %}

where the filters either add nothing, or add an if-statement. See the
runtime_enabled() filter, implemented in code_generator_v8.py (search for
"runtime_enabled_if").

> Do you think I should add a |has_custom_exposed_attributes| variable to the
> global context to guard definition of |context|? I could group the conditions
> together on the Python side, exposing |conditional_attributes| as a list of
> [condition, list of attributes it applies to].

I suppose the 'context' variable could cause a compiler warning if it isn't
used, and thus that a conditional declaration is necessary. (If that wasn't an
issue, having it unconditionally would be fine.)

https://codereview.chromium.org/424163002/diff/40001/Source/bindings/scripts/...
File Source/bindings/scripts/v8_utilities.py (right):

https://codereview.chromium.org/424163002/diff/40001/Source/bindings/scripts/...
Source/bindings/scripts/v8_utilities.py:83: return set(sorted(attribute_values))
This doesn't work. It's the construction of the set that randomizes the order
(since sets are unordered), so it's at the point where you enumerate the items
in the set that you need to sort.

(Note that CPython, without the -R command line switch, probably has stable
order in practice, and thus that this doesn't lead to *varying* random order,
but it's bad practice to rely on this.)

[Peter Beverloo, 2014-07-30 16:03:46]

Addressed. Thanks!

On 2014/07/30 15:16:18, Jens Lindström wrote:
>
https://codereview.chromium.org/424163002/diff/20001/Source/bindings/template...
> File Source/bindings/templates/interface.cpp (right):
> 
>
https://codereview.chromium.org/424163002/diff/20001/Source/bindings/template...
> Source/bindings/templates/interface.cpp:1170: {% else %}
> On 2014/07/30 14:48:15, Peter Beverloo wrote:
> > On 2014/07/29 18:24:29, Jens Lindström wrote:
> > > Are these really mutually exclusive?
> > > 
> > > Also, we could use {% filter %} here, like we do for e.g. runtime enabled
> > stuff.
> > > Don't know if there's a policy for when to use it and when to just
generate
> an
> > > if "manually".
> > 
> > No, they are not. It was just easier to the first draft patch as I have zero
> > experience with Jinja and have no idea what I'm doing :-).
> > 
> > The |conditional| filter is used for compile-time #ifdefs, so we can't reuse
> > that. Neither of the other filters looks relevant either. Ideally, I would
> only
> > define |context| if any of the attributes have |custom_exposed_rules| as
well.
> 
> My thinking was that you would introduce a filters (one for
per_context_enabled
> and one for custom_exposed) and end up with
> 
>   {% filter per_context_enabled(attribute.per_context_enabled_function) %}
>   {% filter custom_exposed(attribute.custom_exposed_rules) %}
>   // the code
>   {% endfilter %}
>   {% endfilter %}
> 
> where the filters either add nothing, or add an if-statement. See the
> runtime_enabled() filter, implemented in code_generator_v8.py (search for
> "runtime_enabled_if").

Ah, that even works with indentation. Done, and generalized the code a little
bit there.
 
> > Do you think I should add a |has_custom_exposed_attributes| variable to the
> > global context to guard definition of |context|? I could group the
conditions
> > together on the Python side, exposing |conditional_attributes| as a list of
> > [condition, list of attributes it applies to].
> 
> I suppose the 'context' variable could cause a compiler warning if it isn't
> used, and thus that a conditional declaration is necessary. (If that wasn't an
> issue, having it unconditionally would be fine.)
> 
>
https://codereview.chromium.org/424163002/diff/40001/Source/bindings/scripts/...
> File Source/bindings/scripts/v8_utilities.py (right):
> 
>
https://codereview.chromium.org/424163002/diff/40001/Source/bindings/scripts/...
> Source/bindings/scripts/v8_utilities.py:83: return
set(sorted(attribute_values))
> This doesn't work. It's the construction of the set that randomizes the order
> (since sets are unordered), so it's at the point where you enumerate the items
> in the set that you need to sort.
> 
> (Note that CPython, without the -R command line switch, probably has stable
> order in practice, and thus that this doesn't lead to *varying* random order,
> but it's bad practice to rely on this.)

Changed the code to use lists (which can be sorted.. reflected in the output as
well) except for the |issubset| call.

https://codereview.chromium.org/424163002/diff/60001/Tools/Scripts/webkitpy/b...
File Tools/Scripts/webkitpy/bindings/main.py (right):

https://codereview.chromium.org/424163002/diff/60001/Tools/Scripts/webkitpy/b...
Tools/Scripts/webkitpy/bindings/main.py:202: idl_basenames =
['TestExposedInterface.idl']
Disregard this for now please.

[haraken, 2014-07-30 16:32:03]

Just a couple of nits.

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

https://codereview.chromium.org/424163002/diff/60001/Source/bindings/scripts/...
Source/bindings/scripts/code_generator_v8.py:242: 'custom_exposed':
custom_exposed_if,

"custom" is a bit confusing since "custom" implies custom bindings. I'd rename
this to simply "exposed".

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

https://codereview.chromium.org/424163002/diff/60001/Source/bindings/scripts/...
Source/bindings/scripts/v8_attributes.py:103: 'custom_exposed_rules':
v8_utilities.exposed(attribute, interface),  # [Exposed]

custom_exposed_rules => exposed_rules

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

https://codereview.chromium.org/424163002/diff/60001/Source/bindings/scripts/...
Source/bindings/scripts/v8_methods.py:140: 'custom_exposed_rules':
v8_utilities.exposed(method, interface),  # [Exposed]

custom_exposed_rules => exposed_rules

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

https://codereview.chromium.org/424163002/diff/60001/Source/bindings/scripts/...
Source/bindings/scripts/v8_utilities.py:256: raise Exception('Interface
members\' exposure sets must be a subset of the interface\'s.')

Nit: We normally use ValueError instead of Exception.

https://codereview.chromium.org/424163002/diff/60001/Source/bindings/scripts/...
Source/bindings/scripts/v8_utilities.py:262: raise Exception('Values for the
[Exposed] annotation must reflect to a valid exposure scope.')

Ditto.

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

https://codereview.chromium.org/424163002/diff/60001/Source/bindings/tests/re...
Source/bindings/tests/results/V8TestExposedInterface.cpp:297:
prototypeTemplate->Set(v8AtomicString(isolate, "workerExposedMethod"),
v8::FunctionTemplate::New(isolate,
TestExposedInterfaceV8Internal::workerExposedMethodMethodCallback,
v8Undefined(), defaultSignature, 0)->GetFunction());

bashi@: We might want to improve the IDL compiler so that it uses
V8DOMConfiguration::installMethod() here.

[Jens Widell, 2014-07-30 16:48:45]

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

https://codereview.chromium.org/424163002/diff/60001/Source/bindings/scripts/...
Source/bindings/scripts/code_generator_v8.py:271: return
generate_indented_conditional(code, 'context && (%s)' % custom_exposed_rules)
Can 'context' really be null? When does that happen?

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

https://codereview.chromium.org/424163002/diff/60001/Source/bindings/scripts/...
Source/bindings/scripts/v8_attributes.py:103: 'custom_exposed_rules':
v8_utilities.exposed(attribute, interface),  # [Exposed]
On 2014/07/30 16:32:03, haraken wrote:
> 
> custom_exposed_rules => exposed_rules

Or maybe 'exposed_tests'? Or maybe better yet, 'exposed_test', since this is in
fact a single string containing a C++ expression.

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

https://codereview.chromium.org/424163002/diff/60001/Source/bindings/scripts/...
Source/bindings/scripts/v8_utilities.py:247: extended_attributes =
definition_or_member.extended_attributes
This local is only used once, on the line below, and is thus rather unnecessary.
And the check below could be done on 'exposure_list' instead:

  if not exposure_list:
    return None

https://codereview.chromium.org/424163002/diff/60001/Source/bindings/scripts/...
Source/bindings/scripts/v8_utilities.py:251: exposure_list =
sorted_extended_attribute_list(definition_or_member, 'Exposed')
I think you could have stuck with 'exposure_set' here; it's still logically a
set of something, even if we use a Python list to represent it.

The suffix switch indicates that the suffix was actually just an explicit record
of the type of the stored value, and such are rarely useful IMO, in particular
not for something with a scope this limited.

We could also just call it 'exposure'.

This comment could also just be considered as noise. :-)

[Peter Beverloo, 2014-07-31 19:02:22]

Thank you for your comments and patience!

I merged the test file in TestInterface.idl per Jens' request. TestObject.idl
was not a good choice, given that it explicitly mentions not to put extended
attributes on the interface.

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

https://codereview.chromium.org/424163002/diff/60001/Source/bindings/scripts/...
Source/bindings/scripts/code_generator_v8.py:242: 'custom_exposed':
custom_exposed_if,
On 2014/07/30 16:32:03, haraken wrote:
> 
> "custom" is a bit confusing since "custom" implies custom bindings. I'd rename
> this to simply "exposed".

Done.

https://codereview.chromium.org/424163002/diff/60001/Source/bindings/scripts/...
Source/bindings/scripts/code_generator_v8.py:271: return
generate_indented_conditional(code, 'context && (%s)' % custom_exposed_rules)
On 2014/07/30 16:48:45, Jens Lindström wrote:
> Can 'context' really be null? When does that happen?

I'm not sure. The check existed in installPerContextEnabledMethods already so I
copied the logic.

Looking at v8::Object::CreationContext, there is a bailout case in which an
empty local will be returned, but I don't know enough about v8 to know when
that'd trigger. The toExecutionContext() method handles this case by returning
NULL.

We could ASSERT() on context being non-NULL in these methods, but then I'd
prefer to do that in a follow up patch.

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

https://codereview.chromium.org/424163002/diff/60001/Source/bindings/scripts/...
Source/bindings/scripts/v8_attributes.py:103: 'custom_exposed_rules':
v8_utilities.exposed(attribute, interface),  # [Exposed]
On 2014/07/30 16:48:45, Jens Lindström wrote:
> On 2014/07/30 16:32:03, haraken wrote:
> > 
> > custom_exposed_rules => exposed_rules
> 
> Or maybe 'exposed_tests'? Or maybe better yet, 'exposed_test', since this is
in
> fact a single string containing a C++ expression.

I agree that |exposed_test| represents the contents most accurately. Changed.

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

https://codereview.chromium.org/424163002/diff/60001/Source/bindings/scripts/...
Source/bindings/scripts/v8_methods.py:140: 'custom_exposed_rules':
v8_utilities.exposed(method, interface),  # [Exposed]
On 2014/07/30 16:32:03, haraken wrote:
> 
> custom_exposed_rules => exposed_rules

Acknowledged.

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

https://codereview.chromium.org/424163002/diff/60001/Source/bindings/scripts/...
Source/bindings/scripts/v8_utilities.py:247: extended_attributes =
definition_or_member.extended_attributes
On 2014/07/30 16:48:45, Jens Lindström wrote:
> This local is only used once, on the line below, and is thus rather
unnecessary.
> And the check below could be done on 'exposure_list' instead:
> 
>   if not exposure_list:
>     return None

Done.

https://codereview.chromium.org/424163002/diff/60001/Source/bindings/scripts/...
Source/bindings/scripts/v8_utilities.py:251: exposure_list =
sorted_extended_attribute_list(definition_or_member, 'Exposed')
On 2014/07/30 16:48:45, Jens Lindström wrote:
> I think you could have stuck with 'exposure_set' here; it's still logically a
> set of something, even if we use a Python list to represent it.
> 
> The suffix switch indicates that the suffix was actually just an explicit
record
> of the type of the stored value, and such are rarely useful IMO, in particular
> not for something with a scope this limited.
> 
> We could also just call it 'exposure'.
> 
> This comment could also just be considered as noise. :-)

I'm in dubio. It would indeed describe the contents accurately (a sorted set),
but it could cause some confusion since the Python data type in fact is a list.

Let's go with following the contents -- thanks!

https://codereview.chromium.org/424163002/diff/60001/Source/bindings/scripts/...
Source/bindings/scripts/v8_utilities.py:256: raise Exception('Interface
members\' exposure sets must be a subset of the interface\'s.')
On 2014/07/30 16:32:03, haraken wrote:
> 
> Nit: We normally use ValueError instead of Exception.

Done.

https://codereview.chromium.org/424163002/diff/60001/Source/bindings/scripts/...
Source/bindings/scripts/v8_utilities.py:262: raise Exception('Values for the
[Exposed] annotation must reflect to a valid exposure scope.')
On 2014/07/30 16:32:03, haraken wrote:
> 
> Ditto.

Done.

[Jens Widell, 2014-07-31 19:47:11]

https://codereview.chromium.org/424163002/diff/80001/Source/bindings/template...
File Source/bindings/templates/interface.cpp (right):

https://codereview.chromium.org/424163002/diff/80001/Source/bindings/template...
Source/bindings/templates/interface.cpp:1191: {% for method in
per_context_enabled_methods %}
Should we group these two sets of methods together, and use the filters you
added?

[haraken, 2014-07-31 19:51:46]

LGTM with Jen's comment addressed.

[Peter Beverloo, 2014-08-01 14:04:01]

Thank you for the reviews! Jens, would you mind confirming that this is what you
had in mind?

https://codereview.chromium.org/424163002/diff/80001/Source/bindings/template...
File Source/bindings/templates/interface.cpp (right):

https://codereview.chromium.org/424163002/diff/80001/Source/bindings/template...
Source/bindings/templates/interface.cpp:1191: {% for method in
per_context_enabled_methods %}
On 2014/07/31 19:47:11, Jens Lindström wrote:
> Should we group these two sets of methods together, and use the filters you
> added?

Done. I removed |cpp_class* impl| argument from
installConditionallyEnabledProperties and changed the |exposed| filter to call
the ContextFeatures-check method based on toDocument(context) instead. This also
means that we'll never have an unused local |context| variable in these
functions anymore.

I'll upload a follow-up CL to determine if we need NULL-checks on |context| for
these filters.

[Jens Widell, 2014-08-01 14:21:28]

LGTM, with a small further improvement suggested.

https://codereview.chromium.org/424163002/diff/100001/Source/bindings/scripts...
File Source/bindings/scripts/v8_interface.py (right):

https://codereview.chromium.org/424163002/diff/100001/Source/bindings/scripts...
Source/bindings/scripts/v8_interface.py:268: conditionally_exposed_methods = []
I think merging the lists here would make things simpler (unless there's some
reason to keep them separate that I'm missing.) I.e. replace
per_context_enabled_methods and conditionally_exposed_methods with a
conditionally_enabled_methods. This matches the renamed function that registers
them too, which is nice.

https://codereview.chromium.org/424163002/diff/100001/Source/bindings/scripts...
Source/bindings/scripts/v8_interface.py:319: 'has_conditional_methods':
per_context_enabled_methods or conditionally_exposed_methods,
You could skip this then also, and just use the new list as the condition.

[Peter Beverloo, 2014-08-01 14:30:44]

https://codereview.chromium.org/424163002/diff/100001/Source/bindings/scripts...
File Source/bindings/scripts/v8_interface.py (right):

https://codereview.chromium.org/424163002/diff/100001/Source/bindings/scripts...
Source/bindings/scripts/v8_interface.py:268: conditionally_exposed_methods = []
On 2014/08/01 14:21:27, Jens Lindström wrote:
> I think merging the lists here would make things simpler (unless there's some
> reason to keep them separate that I'm missing.) I.e. replace
> per_context_enabled_methods and conditionally_exposed_methods with a
> conditionally_enabled_methods. This matches the renamed function that
registers
> them too, which is nice.

Done. Fredrik tells me that you're on holiday -- cheers for the reviews!

https://codereview.chromium.org/424163002/diff/100001/Source/bindings/scripts...
Source/bindings/scripts/v8_interface.py:319: 'has_conditional_methods':
per_context_enabled_methods or conditionally_exposed_methods,
On 2014/08/01 14:21:27, Jens Lindström wrote:
> You could skip this then also, and just use the new list as the condition.

Done.

[Jens Widell, 2014-08-01 14:38:29]

On 2014/08/01 14:30:44, Peter Beverloo wrote:
>
https://codereview.chromium.org/424163002/diff/100001/Source/bindings/scripts...
> File Source/bindings/scripts/v8_interface.py (right):
> 
>
https://codereview.chromium.org/424163002/diff/100001/Source/bindings/scripts...
> Source/bindings/scripts/v8_interface.py:268: conditionally_exposed_methods =
[]
> On 2014/08/01 14:21:27, Jens Lindström wrote:
> > I think merging the lists here would make things simpler (unless there's
some
> > reason to keep them separate that I'm missing.) I.e. replace
> > per_context_enabled_methods and conditionally_exposed_methods with a
> > conditionally_enabled_methods. This matches the renamed function that
> registers
> > them too, which is nice.
> 
> Done.

Thanks!

> Fredrik tells me that you're on holiday -- cheers for the reviews!

Yeah, I am. Frees up a lot of time for reviewing, it turns out. :-)

[Peter Beverloo, 2014-08-01 19:42:51]

The CQ bit was checked by peter@chromium.org

[commit-bot: I haz the power, 2014-08-01 19:43:22]

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

[commit-bot: I haz the power, 2014-08-01 20:20:17]

Change committed as 179418

[enne (OOO), 2014-08-01 23:25:07]

A revert of this CL has been created in
https://codereview.chromium.org/426583003/ by enne@chromium.org.

The reason for reverting is: Causes interactive_ui_tests to timeout

Tests:
WebViewInteractiveTest.PopupPositioningBasic
WebViewInteractiveTest.PopupPositioningMoved

See:
http://build.chromium.org/p/chromium.webkit/builders/Linux%20Tests/builds/383....

[Peter Beverloo, 2014-08-05 14:53:10]

Uploaded patch set 8 to fix the interactive_ui_tests case. Rationale in the bug:
  https://code.google.com/p/chromium/issues/detail?id=398446#c11

[Peter Beverloo, 2014-08-11 12:46:51]

On 2014/08/05 14:53:10, Peter Beverloo wrote:
> Uploaded patch set 8 to fix the interactive_ui_tests case. Rationale in the
bug:
>   https://code.google.com/p/chromium/issues/detail?id=398446#c11

Rebased per the other patches which have landed since, including a fix for the
interactive_ui_tests case. Putting on the queue again.

[Peter Beverloo, 2014-08-11 12:46:58]

The CQ bit was checked by peter@chromium.org

[commit-bot: I haz the power, 2014-08-11 12:47:44]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/peter@chromium.org/424163002/180001

[commit-bot: I haz the power, 2014-08-11 13:51:03]

Change committed as 179947