IDL compiler: [ActivityLogging] for methods

IDL compiler: [ActivityLogging] for methods

BUG=239771
NOTRY=true

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

[Nils Barth (inactive), 2013-11-05 05:18:42]

https://codereview.chromium.org/58963002/diff/1/Source/bindings/templates/int...
File Source/bindings/templates/interface_base.cpp (right):

https://codereview.chromium.org/58963002/diff/1/Source/bindings/templates/int...
Source/bindings/templates/interface_base.cpp:96: {% set world_suffix = '' %}
I'll do [PerWorldBindings] later;
using this |set| as a placeholder for a |for| helps minimize
future changes and avoids having a mysterious literal '' in
the code base in the meantime.

[haraken, 2013-11-05 06:27:36]

LGTM.

https://codereview.chromium.org/58963002/diff/1/Source/bindings/scripts/unsta...
File Source/bindings/scripts/unstable/v8_methods.py (right):

https://codereview.chromium.org/58963002/diff/1/Source/bindings/scripts/unsta...
Source/bindings/scripts/unstable/v8_methods.py:56:
'activity_logging_world_list': v8_utilities.activity_logging_world_list(method,
'Access'),  # [ActivityLogging]

What is 'Access'? Shall we rename it to 'Method'?

https://codereview.chromium.org/58963002/diff/1/Source/bindings/templates/met...
File Source/bindings/templates/methods.cpp (right):

https://codereview.chromium.org/58963002/diff/1/Source/bindings/templates/met...
Source/bindings/templates/methods.cpp:78: Vector<v8::Handle<v8::Value> >
loggerArgs = toVectorOfArguments(info);

Shall we use toNativeArguments(info, 0), and then remove toVectorOfArguments()
from V8Bindings.h ? We don't want to have similar methods with different names
in V8Bindings.h.

[Nils Barth (inactive), 2013-11-05 06:40:56]

Thanks, 2 followups to do.

https://codereview.chromium.org/58963002/diff/1/Source/bindings/scripts/unsta...
File Source/bindings/scripts/unstable/v8_methods.py (right):

https://codereview.chromium.org/58963002/diff/1/Source/bindings/scripts/unsta...
Source/bindings/scripts/unstable/v8_methods.py:56:
'activity_logging_world_list': v8_utilities.activity_logging_world_list(method,
'Access'),  # [ActivityLogging]
On 2013/11/05 06:27:37, haraken wrote:
> What is 'Access'? Shall we rename it to 'Method'?

Agreed we should rename; will do in a followup; please choose a name.

Access is *either*:
* 'Getter&Setter' (for attributes)
* ...or the only ok one for methods

Agreed, it definitely would be clearer to not use 'Access' for methods;
'Access' suggests attributes.
We could just omit it for methods, since only one logging type is allowed,
especially since we now require world suffix so we'd have
(ditto for ForIsolatedWorlds):
Attributes:
AccessForAllWorlds, GetterForAllWorlds, SetterForAllWorlds

Methods:
ForAllWorlds
...as in:
[ActivityLogging=ForAllWorlds] void foo();

Alternatively, we could require an explicit 'Method', as in:
MethodForAllWorlds

So for methods, do you prefer:
1. AccessForAllWorlds
2. ForAllWorlds
3. MethodForAllWorlds
?

https://codereview.chromium.org/58963002/diff/1/Source/bindings/templates/met...
File Source/bindings/templates/methods.cpp (right):

https://codereview.chromium.org/58963002/diff/1/Source/bindings/templates/met...
Source/bindings/templates/methods.cpp:78: Vector<v8::Handle<v8::Value> >
loggerArgs = toVectorOfArguments(info);
On 2013/11/05 06:27:37, haraken wrote:
> Shall we use toNativeArguments(info, 0), and then remove toVectorOfArguments()
> from V8Bindings.h ? We don't want to have similar methods with different names
> in V8Bindings.h.

Sounds good; will do in a followup!

[commit-bot: I haz the power, 2013-11-05 06:41:23]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/nbarth@chromium.org/58963002/1

[commit-bot: I haz the power, 2013-11-05 06:44:08]

Change committed as 161311

[haraken, 2013-11-05 06:51:04]

https://codereview.chromium.org/58963002/diff/1/Source/bindings/scripts/unsta...
File Source/bindings/scripts/unstable/v8_methods.py (right):

https://codereview.chromium.org/58963002/diff/1/Source/bindings/scripts/unsta...
Source/bindings/scripts/unstable/v8_methods.py:56:
'activity_logging_world_list': v8_utilities.activity_logging_world_list(method,
'Access'),  # [ActivityLogging]
On 2013/11/05 06:40:57, Nils Barth wrote:
> On 2013/11/05 06:27:37, haraken wrote:
> > What is 'Access'? Shall we rename it to 'Method'?
> 
> Agreed we should rename; will do in a followup; please choose a name.
> 
> Access is *either*:
> * 'Getter&Setter' (for attributes)
> * ...or the only ok one for methods
> 
> Agreed, it definitely would be clearer to not use 'Access' for methods;
> 'Access' suggests attributes.
> We could just omit it for methods, since only one logging type is allowed,
> especially since we now require world suffix so we'd have
> (ditto for ForIsolatedWorlds):
> Attributes:
> AccessForAllWorlds, GetterForAllWorlds, SetterForAllWorlds
> 
> Methods:
> ForAllWorlds
> ...as in:
> [ActivityLogging=ForAllWorlds] void foo();
> 
> Alternatively, we could require an explicit 'Method', as in:
> MethodForAllWorlds
> 
> So for methods, do you prefer:
> 1. AccessForAllWorlds
> 2. ForAllWorlds
> 3. MethodForAllWorlds
> ?

I'd prefer ForAllWorlds.

BTW, is it possible to use ForAllWorlds for DOM attributes to indicate that we
want to log both getter and setter? If so, ForAllWorlds sounds even better to
me. Otherwise, it might also make sense to use MethodForAllWorlds to make the
meaning clearer.

[Nils Barth (inactive), 2013-11-05 06:53:14]

On 2013/11/05 06:51:04, haraken wrote:
> I'd prefer ForAllWorlds.

Got it, agreed.

> BTW, is it possible to use ForAllWorlds for DOM attributes to indicate that we
> want to log both getter and setter? If so, ForAllWorlds sounds even better to
> me.

Yup, so we can simply default to current |Access|!
Will do shortly.