AccessorConfiguration and AttributeConfiguration no longer need the same fields.

AccessorConfiguration and AttributeConfiguration no longer need the same fields.
This saves a cache_key_name field in every AttributeConfiguration.

run-bindings-tests --reset-results

BUG=667237
Review-Url: https://codereview.chromium.org/2875583002
Cr-Commit-Position: refs/heads/master@{#470876}
Committed: https://chromium.googlesource.com/chromium/src/+/8d7f527777c460aba06cf33eaf5d8d61205a2e09

[nverne, 2017-05-10 04:09:46]

nverne@chromium.org changed reviewers:
+ haraken@chromium.org, peria@chromium.org, slangley@chromium.org,
yukishiino@chromium.org

[nverne, 2017-05-10 04:09:47]

Here's the {Attribute|Accessor}Configuration refactor in a separate Cl, as
peria@ requested!

[haraken, 2017-05-10 04:21:13]

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

https://codereview.chromium.org/2875583002/diff/1/third_party/WebKit/Source/b...
third_party/WebKit/Source/bindings/templates/attributes.cpp.tmpl:412: {% if
attribute.is_lazy_cached_accessor %}

Can we factor out these changes to a later CL?

https://codereview.chromium.org/2875583002/diff/1/third_party/WebKit/Source/b...
third_party/WebKit/Source/bindings/templates/attributes.cpp.tmpl:477: {% macro
build_attribute_or_accessor_configuration(attribute, is_attribute_configuration)
%}

For readability, avoid using a boolean type. You can use 'attribute' or
'accessor'.

https://codereview.chromium.org/2875583002/diff/1/third_party/WebKit/Source/b...
third_party/WebKit/Source/bindings/templates/attributes.cpp.tmpl:513: {% set
config_pre = {

I'd prefer simply emitting the 6 patterns here instead of avoiding code
duplication using config_pre & config_post etc.

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

https://codereview.chromium.org/2875583002/diff/1/third_party/WebKit/Source/b...
third_party/WebKit/Source/bindings/templates/interface_base.cpp.tmpl:717:
{{accessor_configuration(attribute) | indent(2)}}

Not related to this CL, 'accessor', 'attribute' and 'data property' are used
randomly in the code base, which is really confusing. We should rename them...

[nverne, 2017-05-10 05:00:54]

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

https://codereview.chromium.org/2875583002/diff/1/third_party/WebKit/Source/b...
third_party/WebKit/Source/bindings/templates/attributes.cpp.tmpl:412: {% if
attribute.is_lazy_cached_accessor %}
On 2017/05/10 at 04:21:13, haraken wrote:
> Can we factor out these changes to a later CL?

Deleted this code.

https://codereview.chromium.org/2875583002/diff/1/third_party/WebKit/Source/b...
third_party/WebKit/Source/bindings/templates/attributes.cpp.tmpl:477: {% macro
build_attribute_or_accessor_configuration(attribute, is_attribute_configuration)
%}
On 2017/05/10 at 04:21:13, haraken wrote:
> For readability, avoid using a boolean type. You can use 'attribute' or
'accessor'.

Done

https://codereview.chromium.org/2875583002/diff/1/third_party/WebKit/Source/b...
third_party/WebKit/Source/bindings/templates/attributes.cpp.tmpl:513: {% set
config_pre = {
On 2017/05/10 at 04:21:13, haraken wrote:
> I'd prefer simply emitting the 6 patterns here instead of avoiding code
duplication using config_pre & config_post etc.

I think that's likely to be more confusing/error prone. We only one or two of
the six patterns, depending on the args passed in.

[haraken, 2017-05-10 05:18:58]

LGTM

https://codereview.chromium.org/2875583002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/bindings/templates/attributes.cpp.tmpl (right):

https://codereview.chromium.org/2875583002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/bindings/templates/attributes.cpp.tmpl:545:
{{'{'}}{{all_worlds_config_list | join(', ')}}{{'}'}}

Nit: I'd say this would be less readable than just listing the 6 patterns though
:)

[peria, 2017-05-10 05:20:50]

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

https://codereview.chromium.org/2875583002/diff/1/third_party/WebKit/Source/b...
third_party/WebKit/Source/bindings/templates/attributes.cpp.tmpl:513: {% set
config_pre = {
On 2017/05/10 05:00:53, nverne wrote:
> On 2017/05/10 at 04:21:13, haraken wrote:
> > I'd prefer simply emitting the 6 patterns here instead of avoiding code
> duplication using config_pre & config_post etc.
> 
> I think that's likely to be more confusing/error prone. We only one or two of
> the six patterns, depending on the args passed in. 

+1 to merge some duplicates, but this implementation looks still
confusing... :(

https://codereview.chromium.org/2875583002/diff/1/third_party/WebKit/Source/b...
third_party/WebKit/Source/bindings/templates/attributes.cpp.tmpl:543:
{{'{'}}{{main_config_list | join(', ')}}{{'}'}}
no indentation is needed. it'll be adjusted by indent filters in caller side.

https://codereview.chromium.org/2875583002/diff/1/third_party/WebKit/Source/b...
third_party/WebKit/Source/bindings/templates/attributes.cpp.tmpl:544: {{','}}
keep a comma at the tail of the previous line,
and it doesn't need {{' and '}}.

https://codereview.chromium.org/2875583002/diff/1/third_party/WebKit/Source/b...
third_party/WebKit/Source/bindings/templates/attributes.cpp.tmpl:550:
{{'{'}}{{all_worlds_config_list | join(', ')}}{{'}'}}
no indent

https://codereview.chromium.org/2875583002/diff/1/third_party/WebKit/Source/b...
File third_party/WebKit/Source/bindings/templates/interface.cpp.tmpl (left):

https://codereview.chromium.org/2875583002/diff/1/third_party/WebKit/Source/b...
third_party/WebKit/Source/bindings/templates/interface.cpp.tmpl:905: 
keep this empty line.

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

https://codereview.chromium.org/2875583002/diff/1/third_party/WebKit/Source/b...
third_party/WebKit/Source/bindings/templates/interface_base.cpp.tmpl:717:
{{accessor_configuration(attribute) | indent(2)}}
On 2017/05/10 04:21:13, haraken wrote:
> 
> Not related to this CL, 'accessor', 'attribute' and 'data property' are used
> randomly in the code base, which is really confusing. We should rename them...

We have to decide the naming rule about this confusing before renaming.
- IDL files describe 'attributes'.
- Web IDL spec says attributes should be exposed as 'accessor properties' in ES.
- Blink expose some attributes as 'data properties'.
So I feel it not clear which name should be used in the bindings layer of Blink.

[Yuki, 2017-05-10 06:16:08]

https://codereview.chromium.org/2875583002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/bindings/templates/attributes.cpp.tmpl (right):

https://codereview.chromium.org/2875583002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/bindings/templates/attributes.cpp.tmpl:545:
{{'{'}}{{all_worlds_config_list | join(', ')}}{{'}'}}
On 2017/05/10 05:18:58, haraken wrote:
> 
> Nit: I'd say this would be less readable than just listing the 6 patterns
though
> :)

nverne, what do you think about my idea commented at
https://codereview.chromium.org/2873543002/ ?
Can we make the code cleaner with using a helper constexpr function?

https://codereview.chromium.org/2875583002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/bindings/templates/interface.cpp.tmpl (left):

https://codereview.chromium.org/2875583002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/bindings/templates/interface.cpp.tmpl:905: 
Curious, why did you remove this line only?
I don't know why, but other sections are separated with two empty lines.  If you
prefer a single empty line, could you be consistent with all other sections?

[nverne, 2017-05-10 06:50:01]

On 2017/05/10 at 06:16:08, yukishiino wrote:
>
https://codereview.chromium.org/2875583002/diff/20001/third_party/WebKit/Sour...
> File third_party/WebKit/Source/bindings/templates/attributes.cpp.tmpl (right):
> 
>
https://codereview.chromium.org/2875583002/diff/20001/third_party/WebKit/Sour...
> third_party/WebKit/Source/bindings/templates/attributes.cpp.tmpl:545:
{{'{'}}{{all_worlds_config_list | join(', ')}}{{'}'}}
> On 2017/05/10 05:18:58, haraken wrote:
> > 
> > Nit: I'd say this would be less readable than just listing the 6 patterns
though
> > :)
> 
> nverne, what do you think about my idea commented at
> https://codereview.chromium.org/2873543002/ ?
> Can we make the code cleaner with using a helper constexpr function?

This sounds good for a follow-up CL !
> 
>
https://codereview.chromium.org/2875583002/diff/20001/third_party/WebKit/Sour...
> File third_party/WebKit/Source/bindings/templates/interface.cpp.tmpl (left):
> 
>
https://codereview.chromium.org/2875583002/diff/20001/third_party/WebKit/Sour...
> third_party/WebKit/Source/bindings/templates/interface.cpp.tmpl:905: 
> Curious, why did you remove this line only?
> I don't know why, but other sections are separated with two empty lines.  If
you prefer a single empty line, could you be consistent with all other sections?

[nverne, 2017-05-10 06:53:00]

https://codereview.chromium.org/2875583002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/bindings/templates/interface.cpp.tmpl (left):

https://codereview.chromium.org/2875583002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/bindings/templates/interface.cpp.tmpl:905: 
On 2017/05/10 at 06:16:08, Yuki wrote:
> Curious, why did you remove this line only?
> I don't know why, but other sections are separated with two empty lines.  If
you prefer a single empty line, could you be consistent with all other sections?

That was a mistake! Thanks for noticing.

[nverne, 2017-05-10 06:53:58]

The CQ bit was checked by nverne@chromium.org

[nverne, 2017-05-10 06:53:59]

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

[commit-bot: I haz the power, 2017-05-10 06:54:51]

CQ is trying da patch.

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

[Yuki, 2017-05-10 06:58:24]

LGTM.

https://codereview.chromium.org/2875583002/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/bindings/templates/attributes.cpp.tmpl (right):

https://codereview.chromium.org/2875583002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/bindings/templates/attributes.cpp.tmpl:541: {% set
all_worlds_config_list = config_pre["non_main"] + accessor_only_fields +
nit: 80 columns

[commit-bot: I haz the power, 2017-05-10 09:44:34]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-05-10 09:44:35]

Try jobs failed on following builders:
  linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[nverne, 2017-05-11 01:20:23]

The CQ bit was checked by nverne@chromium.org

[commit-bot: I haz the power, 2017-05-11 01:22:10]

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, 2017-05-11 04:15:35]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-05-11 04:15:36]

Try jobs failed on following builders:
  linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[nverne, 2017-05-11 06:34:51]

The CQ bit was checked by nverne@chromium.org

[commit-bot: I haz the power, 2017-05-11 06:35:37]

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, 2017-05-11 08:38:15]

CQ is committing da patch.

Bot data: {"patchset_id": 60001, "attempt_start_ts": 1494484491173720,
"parent_rev": "bff6eb30da83bf8b1277e2bbbb8d55467da2bbb6", "commit_rev":
"8d7f527777c460aba06cf33eaf5d8d61205a2e09"}

[commit-bot: I haz the power, 2017-05-11 08:38:25]

Description was changed from

==========
AccessorConfiguration and AttributeConfiguration no longer need the same fields.
This saves a cache_key_name field in every AttributeConfiguration.

run-bindings-tests --reset-results

BUG=667237
==========

to

==========
AccessorConfiguration and AttributeConfiguration no longer need the same fields.
This saves a cache_key_name field in every AttributeConfiguration.

run-bindings-tests --reset-results

BUG=667237

Review-Url: https://codereview.chromium.org/2875583002
Cr-Commit-Position: refs/heads/master@{#470876}
Committed:
https://chromium.googlesource.com/chromium/src/+/8d7f527777c460aba06cf33eaf5d...
==========

[commit-bot: I haz the power, 2017-05-11 08:38:27]

Committed patchset #4 (id:60001) as
https://chromium.googlesource.com/chromium/src/+/8d7f527777c460aba06cf33eaf5d...