Issue 27638002: IDL compiler: Basic setters

Issue 27638002: IDL compiler: Basic setters (Closed)

Created:
7 years, 2 months ago by Nils Barth (inactive)

Modified:
7 years, 2 months ago

Reviewers:
haraken

CC:
blink-reviews, kojih, jsbell+bindings_chromium.org, abarth-chromium, marja+watch_chromium.org, adamk+blink_chromium.org, haraken, Nate Chapin, Inactive, kouhei (in TOK)

Base URL:
svn://svn.chromium.org/blink/trunk

Visibility:
Public.

More Reviews

Description

IDL compiler: Basic setters Initial CL for setters, supports primitive types and any. Test cases for setters are easy: just remove 'readonly' from a getter-only test case (and 'Readonly' from the variable name)! (In some cases a read only test case is also needed and should be added.) Also doc fix re: PerContextEnabled per: https://code.google.com/p/chromium/issues/detail?id=307814 BUG=239771 NOTRY=true Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=159866

Patch Set 1 #

Patch Set 2 : Reupload #

Total comments: 14

Patch Set 3 : Revised #

Patch Set 4 : PerContextEnabled doc fix #

Patch Set 5 : Comment formatting #

Total comments: 16

Patch Set 6 : Revised #

Patch Set 7 : Fix ; #

Created: 7 years, 2 months ago

Download [raw] [tar.bz2]

	Unified diffs	Side-by-side diffs	Delta from patch set	Stats (+415 lines, -126 lines)			Patch
M	Source/bindings/scripts/IDLAttributes.txt	View	1 2 3 4	2 chunks	+4 lines, -2 lines	0 comments	Download
M	Source/bindings/scripts/unstable/v8_attributes.py	View	1 2 3 4 5 6	6 chunks	+34 lines, -12 lines	0 comments	Download
M	Source/bindings/scripts/unstable/v8_types.py	View	1 2 3 4 5 6	3 chunks	+31 lines, -33 lines	0 comments	Download
M	Source/bindings/scripts/unstable/v8_utilities.py	View	1 2 3 4 5	1 chunk	+17 lines, -0 lines	0 comments	Download
M	Source/bindings/templates/attributes.cpp	View	1 2 3 4 5 6	3 chunks	+25 lines, -3 lines	0 comments	Download
M	Source/bindings/templates/interface.h	View		1 chunk	+1 line, -1 line	0 comments	Download
M	Source/bindings/templates/interface.cpp	View	1 2	1 chunk	+1 line, -1 line	0 comments	Download
M	Source/bindings/templates/interface_base.cpp	View		1 chunk	+7 lines, -2 lines	0 comments	Download
M	Source/bindings/tests/idls/TestObjectPython.idl	View	1 2 3 4 5 6	1 chunk	+17 lines, -12 lines	0 comments	Download
M	Source/bindings/tests/results/V8TestObjectPython.cpp	View	1 2 3 4 5 6	5 chunks	+278 lines, -60 lines	0 comments	Download

Messages

Total messages: 12 (0 generated)

Expand Messages | Collapse Messages

haraken

https://codereview.chromium.org/27638002/diff/5001/Source/bindings/scripts/unstable/v8_attributes.py File Source/bindings/scripts/unstable/v8_attributes.py (right): https://codereview.chromium.org/27638002/diff/5001/Source/bindings/scripts/unstable/v8_attributes.py#newcode84 Source/bindings/scripts/unstable/v8_attributes.py:84: 'setter': setter_name(interface, attribute), setter => setter_callback_name setter_name => setter_callback_name ...

7 years, 2 months ago (2013-10-17 03:54:05 UTC) #2

Nils Barth (inactive)

Revised; main question is what to call the "convert and assign to local variable" template ...

7 years, 2 months ago (2013-10-17 05:02:41 UTC) #3

Revised; main question is what to call the "convert and assign to local
variable" template parameter.
I've renamed:
|assign_js_value_to_local_variable_expression|
to:
|v8_value_to_local_cpp_value|
...which is terse but hopefully understandable.
Thoughts?

(Will update both CGs to always use local variables in getters in followup CL.)

https://codereview.chromium.org/27638002/diff/5001/Source/bindings/scripts/un...
File Source/bindings/scripts/unstable/v8_attributes.py (right):

https://codereview.chromium.org/27638002/diff/5001/Source/bindings/scripts/un...
Source/bindings/scripts/unstable/v8_attributes.py:84: 'setter':
setter_name(interface, attribute),
On 2013/10/17 03:54:06, haraken wrote:
> setter => setter_callback_name
> setter_name => setter_callback_name

Fixed, renamed also in _for_main_world.

> Shall we also implement getter_callback_name ? It's a bit inconsistent that we
> have setter_callback_name but don't have getter_callback_name.

Done.
(These could all be inlined in the template, but that's a bit long.)

https://codereview.chromium.org/27638002/diff/5001/Source/bindings/scripts/un...
Source/bindings/scripts/unstable/v8_attributes.py:91: 'setter_for_main_world':
setter_for_main_world(interface, attribute),
On 2013/10/17 03:54:06, haraken wrote:
> Alphabetical order please.

Fixed.

https://codereview.chromium.org/27638002/diff/5001/Source/bindings/scripts/un...
Source/bindings/scripts/unstable/v8_attributes.py:151:
'assign_js_value_to_local_variable_expression':
v8_types.v8_value_to_cpp_value_expression(idl_type,
attribute.extended_attributes, 'value', 'v', includes, 'info.GetIsolate()'),
On 2013/10/17 03:54:06, haraken wrote:
> 
> value => jsValue
> v => cppValue
> 
> This will require a lot of (but trivial) changes to the Perl generator, so you
> can do it in a follow-up.

Will do!

https://codereview.chromium.org/27638002/diff/5001/Source/bindings/scripts/un...
Source/bindings/scripts/unstable/v8_attributes.py:228: def
setter_name(interface, attribute):
On 2013/10/17 03:54:06, haraken wrote:
> 
> setter_name => setter_callback_name

Done.

https://codereview.chromium.org/27638002/diff/5001/Source/bindings/scripts/un...
File Source/bindings/scripts/unstable/v8_types.py (right):

https://codereview.chromium.org/27638002/diff/5001/Source/bindings/scripts/un...
Source/bindings/scripts/unstable/v8_types.py:349: includes.update(new_includes)
On 2013/10/17 03:54:06, haraken wrote:
> hmm, looks like we have to pass |includes| around in a lot of places. Let's
make
> |includes| a global variable at some point.

Yup; let's do it after finishing attributes!

https://codereview.chromium.org/27638002/diff/5001/Source/bindings/scripts/un...
File Source/bindings/scripts/unstable/v8_utilities.py (right):

https://codereview.chromium.org/27638002/diff/5001/Source/bindings/scripts/un...
Source/bindings/scripts/unstable/v8_utilities.py:62: if
name.startswith('CssText') or re.match('Xml[a-z]', name):
On 2013/10/17 03:54:06, haraken wrote:
> Can we remove these special casing by renaming the corresponding methods in
> core/ ?

Yup; will do in follow-up CL.

https://codereview.chromium.org/27638002/diff/5001/Source/bindings/templates/...
File Source/bindings/templates/attributes.cpp (right):

https://codereview.chromium.org/27638002/diff/5001/Source/bindings/templates/...
Source/bindings/templates/attributes.cpp:105:
{{attribute.set_value_expression}};
On 2013/10/17 03:54:06, haraken wrote:
> I'm not quite sure if |assign_js_value_to_local_variable_expression| and
> |set_value_expression| are good. We don't have these variables in the above
> AttributeGetter() code. We want to make the implementation consistent between
> getter code and setter code.

Discussed offline:
* need these expressions b/c different steps in getter/setter
* will move to getter always using a local variable (consistent, simplifies CG)
  (but that can be inlined in the template, so doesn't need a name)

Renamed:
|assign_js_value_to_local_variable_expression|
to:
|v8_value_to_local_cpp_value|
How's that?

Nils Barth (inactive)

BTW, just added a doc fix re: PerContextEnabled (ContextFeatures are quite different from RuntimeEnabledFeatures).

7 years, 2 months ago (2013-10-17 05:09:18 UTC) #4

haraken

https://codereview.chromium.org/27638002/diff/20001/Source/bindings/scripts/unstable/v8_attributes.py File Source/bindings/scripts/unstable/v8_attributes.py (right): https://codereview.chromium.org/27638002/diff/20001/Source/bindings/scripts/unstable/v8_attributes.py#newcode80 Source/bindings/scripts/unstable/v8_attributes.py:80: 'has_setter': has_setter, Can we remove has_setter? The fact that ...

7 years, 2 months ago (2013-10-17 05:37:21 UTC) #5

Nils Barth (inactive)

Inline replies; summary: 1. Jinja parameters make the templates simpler 2. For getter/setter expressions, already ...

7 years, 2 months ago (2013-10-17 06:24:01 UTC) #6

Inline replies; summary:
1. Jinja parameters make the templates simpler
2. For getter/setter expressions, already have pure conversion functions, so
need other name for "convert then do something" (ideas below)
3. 2 followup CLs:
* CssText/Xmllang etc., and 
* Always use a local variable

https://codereview.chromium.org/27638002/diff/20001/Source/bindings/scripts/u...
File Source/bindings/scripts/unstable/v8_attributes.py (right):

https://codereview.chromium.org/27638002/diff/20001/Source/bindings/scripts/u...
Source/bindings/scripts/unstable/v8_attributes.py:80: 'has_setter': has_setter,
On 2013/10/17 05:37:21, haraken wrote:
> Can we remove has_setter? The fact that setter_callback_name is 0 implies that
> has_setter is false.

|has_setter| is not necessary (as you say, it can be deduced), but it
makes the template clearer.
Also in future we'll need more special cases for omitting setter callbacks,
so the template's clearer if we have a simple flag.

Compare:
{% if attribute.has_setter %}
{{attribute_setter(attribute, world_suffix)}}
{{attribute_setter_callback(attribute, world_suffix)}}
{% endif %}

{% if attribute.setter_callback_name != '0' %}
{{attribute_setter(attribute, world_suffix)}}
{{attribute_setter_callback(attribute, world_suffix)}}
{% endif %}

https://codereview.chromium.org/27638002/diff/20001/Source/bindings/scripts/u...
Source/bindings/scripts/unstable/v8_attributes.py:91: 'world_suffixes': ['',
'ForMainWorld'] if 'PerWorldBindings' in attribute.extended_attributes else
[''],  # [PerWorldBindings]
On 2013/10/17 05:37:21, haraken wrote:
> Can we remove world_suffixes?

The template's clearer with this parameter,
since we need a loop, and thus need a list.

Compare:
{% for world_suffix in attribute.world_suffixes %}
with:
{% for world_suffix in ['', 'ForMainWorld'] if
attribute.getter_callback_name_for_main_world != '0' else [''] %}

The philosophy is "format in the template, logic in the Python".

https://codereview.chromium.org/27638002/diff/20001/Source/bindings/scripts/u...
File Source/bindings/scripts/unstable/v8_types.py (right):

https://codereview.chromium.org/27638002/diff/20001/Source/bindings/scripts/u...
Source/bindings/scripts/unstable/v8_types.py:372: def
v8_value_to_local_cpp_value(idl_type, extended_attributes, v8_value,
variable_name, includes, isolate):
On 2013/10/17 05:37:21, haraken wrote:
> cpp_value is always local. So v8_value_to_cpp_value might be better.
> 
> The rule is:
> 
> - v8_value_to_cpp_value converts a v8 value to a cpp value and assigns it to a
> local variable.
> - cpp_value_to_v8_value converts a cpp value to a v8 value and assigns it to a
> local variable.
> WDYT?

This doesn't work; we already have functions that just do the conversion:
v8_value_to_cpp_value
cpp_value_to_v8_value
...which are used by getters, setters, and will be used by methods,
and these seem like clear names (just convert), so I don't think we
should change these.

For getters & setters we need "convert *and* do something":
Getters convert and set return value (they don't assign to local AFAICT);
setters convert and assign to local

Currently these are:
v8_set_return_value
v8_value_to_local_cpp_value

Very explicit would be:
cpp_value_to_v8_value_and_set_return_value
v8_value_to_cpp_value_and_assign_to_local_variable

Very terse would be:
v8_set_return_value
store_local_cpp_value

Other thoughts?

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

https://codereview.chromium.org/27638002/diff/20001/Source/bindings/scripts/u...
Source/bindings/scripts/unstable/v8_utilities.py:62: if
name.startswith('CssText') or re.match('Xml[a-z]', name):
On 2013/10/17 05:37:21, haraken wrote:
> Add a FIXME.

Done!

https://codereview.chromium.org/27638002/diff/20001/Source/bindings/templates...
File Source/bindings/templates/attributes.cpp (right):

https://codereview.chromium.org/27638002/diff/20001/Source/bindings/templates...
Source/bindings/templates/attributes.cpp:24: {# FIXME: consider using a local
variable to not call getter twice #}
On 2013/10/17 05:37:21, haraken wrote:
> I think it's worth fixing the FIXME now. Let's see how it affects performance.
I
> hope there is no regression.

Got it; will do in separate CL, b/c wide-ranging impact.

https://codereview.chromium.org/27638002/diff/20001/Source/bindings/templates...
Source/bindings/templates/attributes.cpp:61:
{{attribute.v8_set_return_value_statement}}
On 2013/10/17 05:37:21, haraken wrote:
> "_expression" or "_statement" isn't that helpful. Shall we remove the suffix?

Done.
(This is to track whether there's a ; appended or not,
 but I'll make a fix to the Perl CG so the semi can always
 be in the template.)

https://codereview.chromium.org/27638002/diff/20001/Source/bindings/templates...
Source/bindings/templates/attributes.cpp:105:
{{attribute.set_value_expression}};
On 2013/10/17 05:37:21, haraken wrote:
> 
> set_value_expression => setter_method ?

How about |cpp_setter|?
|setter_method| is a bit confusing b/c:
* this expression also has the arguments, and
* we want to distinguish this from the attribute setter itself
(What do we call 'imp->setBooleanAttribute(v)' ?)

> - Probably we might want to use "getter_method" in the getter generation.

We use |cpp_value| for the result, and we need that b/c sometimes
it's the C++ getter itself, sometimes it's a local variable.
When I switch to "always use local variables", let's see how naming works.

> - "_expression" or "_statement" wouldn't make sense, so let's remove the
suffix.

Removed!

haraken

LGTM. I'm not fully satisfied with some inconsistent naming between getters and setters, but you ...

7 years, 2 months ago (2013-10-17 09:07:47 UTC) #7

LGTM. I'm not fully satisfied with some inconsistent naming between getters and
setters, but you can make them consistent when making getters always use local
variables.

> 3. 2 followup CLs:
> * CssText/Xmllang etc., and 
> * Always use a local variable

* 'v' => 'cppValue', 'value' => 'jsValue'

https://codereview.chromium.org/27638002/diff/20001/Source/bindings/scripts/u...
File Source/bindings/scripts/unstable/v8_types.py (right):

https://codereview.chromium.org/27638002/diff/20001/Source/bindings/scripts/u...
Source/bindings/scripts/unstable/v8_types.py:372: def
v8_value_to_local_cpp_value(idl_type, extended_attributes, v8_value,
variable_name, includes, isolate):
On 2013/10/17 06:24:02, Nils Barth wrote:
> On 2013/10/17 05:37:21, haraken wrote:
> > cpp_value is always local. So v8_value_to_cpp_value might be better.
> > 
> > The rule is:
> > 
> > - v8_value_to_cpp_value converts a v8 value to a cpp value and assigns it to
a
> > local variable.
> > - cpp_value_to_v8_value converts a cpp value to a v8 value and assigns it to
a
> > local variable.
> > WDYT?
> 
> This doesn't work; we already have functions that just do the conversion:
> v8_value_to_cpp_value
> cpp_value_to_v8_value
> ...which are used by getters, setters, and will be used by methods,
> and these seem like clear names (just convert), so I don't think we
> should change these.
> 
> For getters & setters we need "convert *and* do something":
> Getters convert and set return value (they don't assign to local AFAICT);
> setters convert and assign to local
> 
> Currently these are:
> v8_set_return_value
> v8_value_to_local_cpp_value
> 
> Very explicit would be:
> cpp_value_to_v8_value_and_set_return_value
> v8_value_to_cpp_value_and_assign_to_local_variable
> 
> Very terse would be:
> v8_set_return_value
> store_local_cpp_value
> 
> Other thoughts?

Let's get back to this issue after making getters always use local variables.

I guess something like the following methods will work (sorry for reviving
"_statement"):

(for getters)
cpp_value_to_v8_value: converts a cpp value to a v8 value
cpp_value_to_v8_value_statement: assigns the cpp=>V8 conversion result to a
local variable
v8_set_return_value: sets the local variable to v8SetReturnValue

(for setters)
v8_value_to_cpp_value: converts a v8 value to a cpp value
v8_value_to_cpp_value_statement: assigns the v8=>cpp conversion result to a
local variable
cpp_setter: calls setter with the local variable.

Nils Barth (inactive)

On 2013/10/17 09:07:47, haraken wrote: > LGTM. I'm not fully satisfied with some inconsistent naming ...

7 years, 2 months ago (2013-10-17 09:23:17 UTC) #8

Nils Barth (inactive)

https://codereview.chromium.org/27638002/diff/20001/Source/bindings/scripts/unstable/v8_types.py File Source/bindings/scripts/unstable/v8_types.py (right): https://codereview.chromium.org/27638002/diff/20001/Source/bindings/scripts/unstable/v8_types.py#newcode372 Source/bindings/scripts/unstable/v8_types.py:372: def v8_value_to_local_cpp_value(idl_type, extended_attributes, v8_value, variable_name, includes, isolate): On 2013/10/17 ...

7 years, 2 months ago (2013-10-17 09:23:25 UTC) #9

Nils Barth (inactive)

BTW, I fixed the ; handling (so ; all in template, instead of everywhere in ...

7 years, 2 months ago (2013-10-17 09:25:47 UTC) #10

commit-bot: I haz the power

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

7 years, 2 months ago (2013-10-17 09:28:36 UTC) #11

Message was sent while issue was closed.

Change committed as 159866

Expand Messages | Collapse Messages