Move field-dependent code in ComputedStyleBase to Jinja macros.

Move field-dependent code in ComputedStyleBase to Jinja macros.

When we're generating fields in ComputedStyleBase, there are cases
where the generated code depends on the field template. For example,
keyword fields may have different getters than platform type fields.
Instead of doing if-else statements inside the template which makes
it quite ugly, this patch moves that logic out into their own files
as Jinja macros. Each file defines Jinja macros that specify how a
field should be generated.

This patch adds a 'keyword' template for enums, 'flag' template for
bools, and 'monotonic_flag' for nonproperty flags that can't be set
to false.

Link to generated code:
https://gist.github.com/anonymous/93f5d3907e3515d0b50adf0107f3000e

Link to diff (see differences in 'initialUnique' and 'resetUnique'):
https://www.diffchecker.com/mikllVbf

BUG=628043
Review-Url: https://codereview.chromium.org/2693933003
Cr-Commit-Position: refs/heads/master@{#456095}
Committed: https://chromium.googlesource.com/chromium/src/+/6ce2feb35c970fe7d9557c851c016c6d6c8b1d39

[shend, 2017-02-16 05:59:34]

Description was changed from

==========
Move field-dependent template code from ComputedStyleBase to macros.

When we're generating fields in ComputedStyleBase, there are cases
where the generated code depends on the field template. For example,
keyword fields may have different getters than platform type fields.
Instead of doing if-else statements inside the template which makes
it quite ugly, this patch moves that logic out into their own files
as macros. Each file defines macros that specify how a field should
be generated.

This patch does not change behaviour.

BUG=628043
==========

to

==========
Move field-dependent template code from ComputedStyleBase to macros.

When we're generating fields in ComputedStyleBase, there are cases
where the generated code depends on the field template. For example,
keyword fields may have different getters than platform type fields.
Instead of doing if-else statements inside the template which makes
it quite ugly, this patch moves that logic out into their own files
as macros. Each file defines macros that specify how a field should
be generated.

This patch adds a 'keyword' template for enums, 'flag' template for
bools, and 'monotonic_flag' for nonproperty flags that can't be set
to false.

This patch does not change behaviour.

BUG=628043
==========

[shend, 2017-02-16 05:59:47]

The CQ bit was checked by shend@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-02-16 06:00:27]

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

[shend, 2017-02-16 06:00:51]

The CQ bit was unchecked by shend@chromium.org

[shend, 2017-02-16 06:01:57]

The CQ bit was checked by shend@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-02-16 06:03:29]

Dry run: 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-02-16 06:07:24]

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

[commit-bot: I haz the power, 2017-02-16 06:07:25]

Dry run: Try jobs failed on following builders:
  ios-device on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...)
  ios-device-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-xcode-...)
  ios-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...)

[shend, 2017-02-16 06:23:44]

The CQ bit was checked by shend@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-02-16 06:24:41]

Dry run: 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-02-16 06:28:09]

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

[commit-bot: I haz the power, 2017-02-16 06:28:10]

Dry run: Try jobs failed on following builders:
  ios-device on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...)
  ios-device-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-xcode-...)
  ios-simulator on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
  ios-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...)

[shend, 2017-02-16 06:40:58]

The CQ bit was checked by shend@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-02-16 06:41:42]

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

[shend, 2017-02-16 06:49:21]

shend@chromium.org changed reviewers:
+ bugsnash@chromium.org

[shend, 2017-02-16 06:49:22]

Hi Bugs, PTAL :)

[commit-bot: I haz the power, 2017-02-16 10:35:16]

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

[commit-bot: I haz the power, 2017-02-16 10:35:16]

Dry run: This issue passed the CQ dry run.

[shend, 2017-02-16 21:50:07]

On 2017/02/16 at 06:49:22, shend wrote:
> Hi Bugs, PTAL :)

Forgot to mention, I'm open to better names than 'monotonic_flag'.

[Bugs Nash, 2017-02-19 23:39:20]

On 2017/02/16 at 21:50:07, shend wrote:
> On 2017/02/16 at 06:49:22, shend wrote:
> > Hi Bugs, PTAL :)
> 
> Forgot to mention, I'm open to better names than 'monotonic_flag'.

Yeah, 'monotonic' isn't exactly an accurate description of the flag. Although I
can't think of a name that is exactly accurate. The only things I can think of
are things like 'isset' and 'unidirectional'.

[Bugs Nash, 2017-02-19 23:39:40]

bugsnash@chromium.org changed reviewers:
+ sashab@chromium.org

[Bugs Nash, 2017-02-19 23:39:46]

This patch introduces some duplication of templated code, which could make it
difficult to maintain. I feel like it was better before, what do you think
Sasha?

[Bugs Nash, 2017-02-20 00:22:42]

bugsnash@chromium.org changed reviewers:
+ meade@chromium.org
- sashab@chromium.org

[Bugs Nash, 2017-02-20 00:22:43]

Forwarding this question to Eddy as Sasha is handing this project over to style
team

[shend, 2017-02-20 06:43:28]

On 2017/02/19 at 23:39:46, bugsnash wrote:
> This patch introduces some duplication of templated code, which could make it
difficult to maintain. I feel like it was better before, what do you think
Sasha?

I could reduce duplication by using template inheritance:
http://flask.pocoo.org/docs/0.12/patterns/templateinheritance/
So I would create a base template with getters and setters and then extend that
in the other templates.

It might not look like we're doing much here, but a future patch
https://codereview.chromium.org/2697953004 would look quite ugly if we don't
split the logic up somehow.

[shend, 2017-02-20 07:03:29]

On 2017/02/20 at 06:43:28, shend wrote:
> On 2017/02/19 at 23:39:46, bugsnash wrote:
> > This patch introduces some duplication of templated code, which could make
it difficult to maintain. I feel like it was better before, what do you think
Sasha?
> 
> I could reduce duplication by using template inheritance:
http://flask.pocoo.org/docs/0.12/patterns/templateinheritance/
> So I would create a base template with getters and setters and then extend
that in the other templates.
> 
> It might not look like we're doing much here, but a future patch
https://codereview.chromium.org/2697953004 would look quite ugly if we don't
split the logic up somehow.

Oops, can't use template inheritance since I'm using a macro, but essentially I
can create a base template that other macros call:

{% import 'base.tmpl' as base %}
{% macro decl_methods(field) -%}
{% base.decl_methods(field) %}
// other methods
{%- endmacro %}

[Bugs Nash, 2017-02-20 20:41:44]

On 2017/02/20 at 07:03:29, shend wrote:
> On 2017/02/20 at 06:43:28, shend wrote:
> > On 2017/02/19 at 23:39:46, bugsnash wrote:
> > > This patch introduces some duplication of templated code, which could make
it difficult to maintain. I feel like it was better before, what do you think
Sasha?
> > 
> > I could reduce duplication by using template inheritance:
http://flask.pocoo.org/docs/0.12/patterns/templateinheritance/
> > So I would create a base template with getters and setters and then extend
that in the other templates.
> > 
> > It might not look like we're doing much here, but a future patch
https://codereview.chromium.org/2697953004 would look quite ugly if we don't
split the logic up somehow.
> 
> Oops, can't use template inheritance since I'm using a macro, but essentially
I can create a base template that other macros call:
> 
> {% import 'base.tmpl' as base %}
> {% macro decl_methods(field) -%}
> {% base.decl_methods(field) %}
> // other methods
> {%- endmacro %}

I would be ok with calling base templates here to get rid of the duplication

[shend, 2017-02-22 03:29:49]

On 2017/02/20 at 20:41:44, bugsnash wrote:
> On 2017/02/20 at 07:03:29, shend wrote:
> > On 2017/02/20 at 06:43:28, shend wrote:
> > > On 2017/02/19 at 23:39:46, bugsnash wrote:
> > > > This patch introduces some duplication of templated code, which could
make it difficult to maintain. I feel like it was better before, what do you
think Sasha?
> > > 
> > > I could reduce duplication by using template inheritance:
http://flask.pocoo.org/docs/0.12/patterns/templateinheritance/
> > > So I would create a base template with getters and setters and then extend
that in the other templates.
> > > 
> > > It might not look like we're doing much here, but a future patch
https://codereview.chromium.org/2697953004 would look quite ugly if we don't
split the logic up somehow.
> > 
> > Oops, can't use template inheritance since I'm using a macro, but
essentially I can create a base template that other macros call:
> > 
> > {% import 'base.tmpl' as base %}
> > {% macro decl_methods(field) -%}
> > {% base.decl_methods(field) %}
> > // other methods
> > {%- endmacro %}
> 
> I would be ok with calling base templates here to get rid of the duplication

Ok, done. Let me know what you think.

[Bugs Nash, 2017-02-23 00:08:55]

On 2017/02/22 at 03:29:49, shend wrote:
> On 2017/02/20 at 20:41:44, bugsnash wrote:
> > On 2017/02/20 at 07:03:29, shend wrote:
> > > On 2017/02/20 at 06:43:28, shend wrote:
> > > > On 2017/02/19 at 23:39:46, bugsnash wrote:
> > > > > This patch introduces some duplication of templated code, which could
make it difficult to maintain. I feel like it was better before, what do you
think Sasha?
> > > > 
> > > > I could reduce duplication by using template inheritance:
http://flask.pocoo.org/docs/0.12/patterns/templateinheritance/
> > > > So I would create a base template with getters and setters and then
extend that in the other templates.
> > > > 
> > > > It might not look like we're doing much here, but a future patch
https://codereview.chromium.org/2697953004 would look quite ugly if we don't
split the logic up somehow.
> > > 
> > > Oops, can't use template inheritance since I'm using a macro, but
essentially I can create a base template that other macros call:
> > > 
> > > {% import 'base.tmpl' as base %}
> > > {% macro decl_methods(field) -%}
> > > {% base.decl_methods(field) %}
> > > // other methods
> > > {%- endmacro %}
> > 
> > I would be ok with calling base templates here to get rid of the duplication
> 
> Ok, done. Let me know what you think.

Could you please link to sample generated code in the description?

[Bugs Nash, 2017-02-23 00:13:34]

https://codereview.chromium.org/2693933003/diff/140001/third_party/WebKit/Sou...
File third_party/WebKit/Source/build/scripts/templates/fields/keyword.tmpl
(right):

https://codereview.chromium.org/2693933003/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/build/scripts/templates/fields/keyword.tmpl:1: {%
import 'fields/base.tmpl' as base %}
This template is identical to the flag template. How do we determine which
methods are to be excluded?

[shend, 2017-02-23 00:17:19]

On 2017/02/23 at 00:08:55, bugsnash wrote:
> On 2017/02/22 at 03:29:49, shend wrote:
> > On 2017/02/20 at 20:41:44, bugsnash wrote:
> > > On 2017/02/20 at 07:03:29, shend wrote:
> > > > On 2017/02/20 at 06:43:28, shend wrote:
> > > > > On 2017/02/19 at 23:39:46, bugsnash wrote:
> > > > > > This patch introduces some duplication of templated code, which
could make it difficult to maintain. I feel like it was better before, what do
you think Sasha?
> > > > > 
> > > > > I could reduce duplication by using template inheritance:
http://flask.pocoo.org/docs/0.12/patterns/templateinheritance/
> > > > > So I would create a base template with getters and setters and then
extend that in the other templates.
> > > > > 
> > > > > It might not look like we're doing much here, but a future patch
https://codereview.chromium.org/2697953004 would look quite ugly if we don't
split the logic up somehow.
> > > > 
> > > > Oops, can't use template inheritance since I'm using a macro, but
essentially I can create a base template that other macros call:
> > > > 
> > > > {% import 'base.tmpl' as base %}
> > > > {% macro decl_methods(field) -%}
> > > > {% base.decl_methods(field) %}
> > > > // other methods
> > > > {%- endmacro %}
> > > 
> > > I would be ok with calling base templates here to get rid of the
duplication
> > 
> > Ok, done. Let me know what you think.
> 
> Could you please link to sample generated code in the description?

Will do!

[shend, 2017-02-23 00:19:15]

On 2017/02/23 at 00:13:34, bugsnash wrote:
>
https://codereview.chromium.org/2693933003/diff/140001/third_party/WebKit/Sou...
> File third_party/WebKit/Source/build/scripts/templates/fields/keyword.tmpl
(right):
> 
>
https://codereview.chromium.org/2693933003/diff/140001/third_party/WebKit/Sou...
> third_party/WebKit/Source/build/scripts/templates/fields/keyword.tmpl:1: {%
import 'fields/base.tmpl' as base %}
> This template is identical to the flag template. How do we determine which
methods are to be excluded?

Yes, I'll eventually be adding more methods to the keywords template (if not
I'll just merge the two templates). I'm not sure what you mean by which methods
are excluded. With this setup, you can't really "exclude" methods. It is
possible to split the "base.decl_methods" macro into "decl_getter" and
"decl_setter" etc. so you can cherry pick what methods to include. Would that
help?

[Bugs Nash, 2017-02-23 00:23:08]

On 2017/02/23 at 00:19:15, shend wrote:
> On 2017/02/23 at 00:13:34, bugsnash wrote:
> >
https://codereview.chromium.org/2693933003/diff/140001/third_party/WebKit/Sou...
> > File third_party/WebKit/Source/build/scripts/templates/fields/keyword.tmpl
(right):
> > 
> >
https://codereview.chromium.org/2693933003/diff/140001/third_party/WebKit/Sou...
> > third_party/WebKit/Source/build/scripts/templates/fields/keyword.tmpl:1: {%
import 'fields/base.tmpl' as base %}
> > This template is identical to the flag template. How do we determine which
methods are to be excluded?
> 
> Yes, I'll eventually be adding more methods to the keywords template (if not
I'll just merge the two templates). I'm not sure what you mean by which methods
are excluded. With this setup, you can't really "exclude" methods. It is
possible to split the "base.decl_methods" macro into "decl_getter" and
"decl_setter" etc. so you can cherry pick what methods to include. Would that
help?

What I mean is in the previous design nonpropertys got the setter with no
arguments only, and propertys got the setter with an argument only. With the new
design both get both correct? Which means that we're adding superfluous methods
here.

[shend, 2017-02-23 00:27:15]

On 2017/02/23 at 00:23:08, bugsnash wrote:
> On 2017/02/23 at 00:19:15, shend wrote:
> > On 2017/02/23 at 00:13:34, bugsnash wrote:
> > >
https://codereview.chromium.org/2693933003/diff/140001/third_party/WebKit/Sou...
> > > File third_party/WebKit/Source/build/scripts/templates/fields/keyword.tmpl
(right):
> > > 
> > >
https://codereview.chromium.org/2693933003/diff/140001/third_party/WebKit/Sou...
> > > third_party/WebKit/Source/build/scripts/templates/fields/keyword.tmpl:1:
{% import 'fields/base.tmpl' as base %}
> > > This template is identical to the flag template. How do we determine which
methods are to be excluded?
> > 
> > Yes, I'll eventually be adding more methods to the keywords template (if not
I'll just merge the two templates). I'm not sure what you mean by which methods
are excluded. With this setup, you can't really "exclude" methods. It is
possible to split the "base.decl_methods" macro into "decl_getter" and
"decl_setter" etc. so you can cherry pick what methods to include. Would that
help?
> 
> What I mean is in the previous design nonpropertys got the setter with no
arguments only, and propertys got the setter with an argument only. With the new
design both get both correct? Which means that we're adding superfluous methods
here.

So 'keyword' and 'flag' are for properties and take arguments in their setter.
'monotonic_flag' (urgh) is for nonproperties and they don't take an argument.

[Bugs Nash, 2017-02-23 00:36:27]

On 2017/02/23 at 00:27:15, shend wrote:
> On 2017/02/23 at 00:23:08, bugsnash wrote:
> > On 2017/02/23 at 00:19:15, shend wrote:
> > > On 2017/02/23 at 00:13:34, bugsnash wrote:
> > > >
https://codereview.chromium.org/2693933003/diff/140001/third_party/WebKit/Sou...
> > > > File
third_party/WebKit/Source/build/scripts/templates/fields/keyword.tmpl (right):
> > > > 
> > > >
https://codereview.chromium.org/2693933003/diff/140001/third_party/WebKit/Sou...
> > > > third_party/WebKit/Source/build/scripts/templates/fields/keyword.tmpl:1:
{% import 'fields/base.tmpl' as base %}
> > > > This template is identical to the flag template. How do we determine
which methods are to be excluded?
> > > 
> > > Yes, I'll eventually be adding more methods to the keywords template (if
not I'll just merge the two templates). I'm not sure what you mean by which
methods are excluded. With this setup, you can't really "exclude" methods. It is
possible to split the "base.decl_methods" macro into "decl_getter" and
"decl_setter" etc. so you can cherry pick what methods to include. Would that
help?
> > 
> > What I mean is in the previous design nonpropertys got the setter with no
arguments only, and propertys got the setter with an argument only. With the new
design both get both correct? Which means that we're adding superfluous methods
here.
> 
> So 'keyword' and 'flag' are for properties and take arguments in their setter.
'monotonic_flag' (urgh) is for nonproperties and they don't take an argument.

Right. So with this patch the 'keyword' and 'flag' properties will get both the
setter with an argument and the setter with no argument right? And the the
nonproperties will get both setters too?

[shend, 2017-02-23 00:39:11]

On 2017/02/23 at 00:36:27, bugsnash wrote:
> On 2017/02/23 at 00:27:15, shend wrote:
> > On 2017/02/23 at 00:23:08, bugsnash wrote:
> > > On 2017/02/23 at 00:19:15, shend wrote:
> > > > On 2017/02/23 at 00:13:34, bugsnash wrote:
> > > > >
https://codereview.chromium.org/2693933003/diff/140001/third_party/WebKit/Sou...
> > > > > File
third_party/WebKit/Source/build/scripts/templates/fields/keyword.tmpl (right):
> > > > > 
> > > > >
https://codereview.chromium.org/2693933003/diff/140001/third_party/WebKit/Sou...
> > > > >
third_party/WebKit/Source/build/scripts/templates/fields/keyword.tmpl:1: {%
import 'fields/base.tmpl' as base %}
> > > > > This template is identical to the flag template. How do we determine
which methods are to be excluded?
> > > > 
> > > > Yes, I'll eventually be adding more methods to the keywords template (if
not I'll just merge the two templates). I'm not sure what you mean by which
methods are excluded. With this setup, you can't really "exclude" methods. It is
possible to split the "base.decl_methods" macro into "decl_getter" and
"decl_setter" etc. so you can cherry pick what methods to include. Would that
help?
> > > 
> > > What I mean is in the previous design nonpropertys got the setter with no
arguments only, and propertys got the setter with an argument only. With the new
design both get both correct? Which means that we're adding superfluous methods
here.
> > 
> > So 'keyword' and 'flag' are for properties and take arguments in their
setter. 'monotonic_flag' (urgh) is for nonproperties and they don't take an
argument.
> 
> Right. So with this patch the 'keyword' and 'flag' properties will get both
the setter with an argument and the setter with no argument right? And the the
nonproperties will get both setters too?

No, all the templates have exactly one setter. 'keyword' should get exactly one
setter that takes an argument. 'monotonic_flag' should get exactly one setter
with no arguments. 'flag' is exactly the same as 'keyword' (for now).

[Bugs Nash, 2017-02-23 00:45:36]

On 2017/02/23 at 00:39:11, shend wrote:
> On 2017/02/23 at 00:36:27, bugsnash wrote:
> > On 2017/02/23 at 00:27:15, shend wrote:
> > > On 2017/02/23 at 00:23:08, bugsnash wrote:
> > > > On 2017/02/23 at 00:19:15, shend wrote:
> > > > > On 2017/02/23 at 00:13:34, bugsnash wrote:
> > > > > >
https://codereview.chromium.org/2693933003/diff/140001/third_party/WebKit/Sou...
> > > > > > File
third_party/WebKit/Source/build/scripts/templates/fields/keyword.tmpl (right):
> > > > > > 
> > > > > >
https://codereview.chromium.org/2693933003/diff/140001/third_party/WebKit/Sou...
> > > > > >
third_party/WebKit/Source/build/scripts/templates/fields/keyword.tmpl:1: {%
import 'fields/base.tmpl' as base %}
> > > > > > This template is identical to the flag template. How do we determine
which methods are to be excluded?
> > > > > 
> > > > > Yes, I'll eventually be adding more methods to the keywords template
(if not I'll just merge the two templates). I'm not sure what you mean by which
methods are excluded. With this setup, you can't really "exclude" methods. It is
possible to split the "base.decl_methods" macro into "decl_getter" and
"decl_setter" etc. so you can cherry pick what methods to include. Would that
help?
> > > > 
> > > > What I mean is in the previous design nonpropertys got the setter with
no arguments only, and propertys got the setter with an argument only. With the
new design both get both correct? Which means that we're adding superfluous
methods here.
> > > 
> > > So 'keyword' and 'flag' are for properties and take arguments in their
setter. 'monotonic_flag' (urgh) is for nonproperties and they don't take an
argument.
> > 
> > Right. So with this patch the 'keyword' and 'flag' properties will get both
the setter with an argument and the setter with no argument right? And the the
nonproperties will get both setters too?
> 
> No, all the templates have exactly one setter. 'keyword' should get exactly
one setter that takes an argument. 'monotonic_flag' should get exactly one
setter with no arguments. 'flag' is exactly the same as 'keyword' (for now).

Oh I see, ignore me. So keyword and flag properties do have identical methods
currently, but you're planning in the future to have different methods for them.
It does seem to leave the code in a state that doesn't make much sense to have
separate but identical templates for them here. However if it's most likely that
they will diverge then the alternative of merging them here then splitting them
out later seems to create unnecessary work. Please add a comment to each of the
identical templates stating that they are currently identical but they will
diverge soon. or possibly a todo for adding extra unique methods. lgtm

[shend, 2017-02-23 00:53:19]

On 2017/02/23 at 00:45:36, bugsnash wrote:
> On 2017/02/23 at 00:39:11, shend wrote:
> > On 2017/02/23 at 00:36:27, bugsnash wrote:
> > > On 2017/02/23 at 00:27:15, shend wrote:
> > > > On 2017/02/23 at 00:23:08, bugsnash wrote:
> > > > > On 2017/02/23 at 00:19:15, shend wrote:
> > > > > > On 2017/02/23 at 00:13:34, bugsnash wrote:
> > > > > > >
https://codereview.chromium.org/2693933003/diff/140001/third_party/WebKit/Sou...
> > > > > > > File
third_party/WebKit/Source/build/scripts/templates/fields/keyword.tmpl (right):
> > > > > > > 
> > > > > > >
https://codereview.chromium.org/2693933003/diff/140001/third_party/WebKit/Sou...
> > > > > > >
third_party/WebKit/Source/build/scripts/templates/fields/keyword.tmpl:1: {%
import 'fields/base.tmpl' as base %}
> > > > > > > This template is identical to the flag template. How do we
determine which methods are to be excluded?
> > > > > > 
> > > > > > Yes, I'll eventually be adding more methods to the keywords template
(if not I'll just merge the two templates). I'm not sure what you mean by which
methods are excluded. With this setup, you can't really "exclude" methods. It is
possible to split the "base.decl_methods" macro into "decl_getter" and
"decl_setter" etc. so you can cherry pick what methods to include. Would that
help?
> > > > > 
> > > > > What I mean is in the previous design nonpropertys got the setter with
no arguments only, and propertys got the setter with an argument only. With the
new design both get both correct? Which means that we're adding superfluous
methods here.
> > > > 
> > > > So 'keyword' and 'flag' are for properties and take arguments in their
setter. 'monotonic_flag' (urgh) is for nonproperties and they don't take an
argument.
> > > 
> > > Right. So with this patch the 'keyword' and 'flag' properties will get
both the setter with an argument and the setter with no argument right? And the
the nonproperties will get both setters too?
> > 
> > No, all the templates have exactly one setter. 'keyword' should get exactly
one setter that takes an argument. 'monotonic_flag' should get exactly one
setter with no arguments. 'flag' is exactly the same as 'keyword' (for now).
> 
> Oh I see, ignore me. So keyword and flag properties do have identical methods
currently, but you're planning in the future to have different methods for them.
It does seem to leave the code in a state that doesn't make much sense to have
separate but identical templates for them here. However if it's most likely that
they will diverge then the alternative of merging them here then splitting them
out later seems to create unnecessary work. Please add a comment to each of the
identical templates stating that they are currently identical but they will
diverge soon. or possibly a todo for adding extra unique methods. lgtm

Cool, sorry for the confusion. Yes I agree. I'll either add a TODO or just add
the extra methods in now.

[Bugs Nash, 2017-02-23 01:01:21]

On 2017/02/23 at 00:53:19, shend wrote:
> On 2017/02/23 at 00:45:36, bugsnash wrote:
> > On 2017/02/23 at 00:39:11, shend wrote:
> > > On 2017/02/23 at 00:36:27, bugsnash wrote:
> > > > On 2017/02/23 at 00:27:15, shend wrote:
> > > > > On 2017/02/23 at 00:23:08, bugsnash wrote:
> > > > > > On 2017/02/23 at 00:19:15, shend wrote:
> > > > > > > On 2017/02/23 at 00:13:34, bugsnash wrote:
> > > > > > > >
https://codereview.chromium.org/2693933003/diff/140001/third_party/WebKit/Sou...
> > > > > > > > File
third_party/WebKit/Source/build/scripts/templates/fields/keyword.tmpl (right):
> > > > > > > > 
> > > > > > > >
https://codereview.chromium.org/2693933003/diff/140001/third_party/WebKit/Sou...
> > > > > > > >
third_party/WebKit/Source/build/scripts/templates/fields/keyword.tmpl:1: {%
import 'fields/base.tmpl' as base %}
> > > > > > > > This template is identical to the flag template. How do we
determine which methods are to be excluded?
> > > > > > > 
> > > > > > > Yes, I'll eventually be adding more methods to the keywords
template (if not I'll just merge the two templates). I'm not sure what you mean
by which methods are excluded. With this setup, you can't really "exclude"
methods. It is possible to split the "base.decl_methods" macro into
"decl_getter" and "decl_setter" etc. so you can cherry pick what methods to
include. Would that help?
> > > > > > 
> > > > > > What I mean is in the previous design nonpropertys got the setter
with no arguments only, and propertys got the setter with an argument only. With
the new design both get both correct? Which means that we're adding superfluous
methods here.
> > > > > 
> > > > > So 'keyword' and 'flag' are for properties and take arguments in their
setter. 'monotonic_flag' (urgh) is for nonproperties and they don't take an
argument.
> > > > 
> > > > Right. So with this patch the 'keyword' and 'flag' properties will get
both the setter with an argument and the setter with no argument right? And the
the nonproperties will get both setters too?
> > > 
> > > No, all the templates have exactly one setter. 'keyword' should get
exactly one setter that takes an argument. 'monotonic_flag' should get exactly
one setter with no arguments. 'flag' is exactly the same as 'keyword' (for now).
> > 
> > Oh I see, ignore me. So keyword and flag properties do have identical
methods currently, but you're planning in the future to have different methods
for them. It does seem to leave the code in a state that doesn't make much sense
to have separate but identical templates for them here. However if it's most
likely that they will diverge then the alternative of merging them here then
splitting them out later seems to create unnecessary work. Please add a comment
to each of the identical templates stating that they are currently identical but
they will diverge soon. or possibly a todo for adding extra unique methods. lgtm
> 
> Cool, sorry for the confusion. Yes I agree. I'll either add a TODO or just add
the extra methods in now.

I think a todo is more appropriate, keep the patches silo'd

[shend, 2017-02-23 22:20:37]

Description was changed from

==========
Move field-dependent template code from ComputedStyleBase to macros.

When we're generating fields in ComputedStyleBase, there are cases
where the generated code depends on the field template. For example,
keyword fields may have different getters than platform type fields.
Instead of doing if-else statements inside the template which makes
it quite ugly, this patch moves that logic out into their own files
as macros. Each file defines macros that specify how a field should
be generated.

This patch adds a 'keyword' template for enums, 'flag' template for
bools, and 'monotonic_flag' for nonproperty flags that can't be set
to false.

This patch does not change behaviour.

BUG=628043
==========

to

==========
Move field-dependent template code from ComputedStyleBase to macros.

When we're generating fields in ComputedStyleBase, there are cases
where the generated code depends on the field template. For example,
keyword fields may have different getters than platform type fields.
Instead of doing if-else statements inside the template which makes
it quite ugly, this patch moves that logic out into their own files
as macros. Each file defines macros that specify how a field should
be generated.

This patch adds a 'keyword' template for enums, 'flag' template for
bools, and 'monotonic_flag' for nonproperty flags that can't be set
to false.

Link to generated code:
https://gist.github.com/anonymous/93f5d3907e3515d0b50adf0107f3000e

Link to diff (see differences in 'initialUnique' and 'resetUnique'):
https://www.diffchecker.com/mikllVbf

BUG=628043
==========

[meade_UTC10, 2017-03-01 06:34:15]

lgtm

This seems good, but at first I thought you were generating C++ macros, lol.
It's probably worth specifying that you mean jinja macros :p

[shend, 2017-03-01 22:31:40]

Description was changed from

==========
Move field-dependent template code from ComputedStyleBase to macros.

When we're generating fields in ComputedStyleBase, there are cases
where the generated code depends on the field template. For example,
keyword fields may have different getters than platform type fields.
Instead of doing if-else statements inside the template which makes
it quite ugly, this patch moves that logic out into their own files
as macros. Each file defines macros that specify how a field should
be generated.

This patch adds a 'keyword' template for enums, 'flag' template for
bools, and 'monotonic_flag' for nonproperty flags that can't be set
to false.

Link to generated code:
https://gist.github.com/anonymous/93f5d3907e3515d0b50adf0107f3000e

Link to diff (see differences in 'initialUnique' and 'resetUnique'):
https://www.diffchecker.com/mikllVbf

BUG=628043
==========

to

==========
Move field-dependent code in ComputedStyleBase to Jinja macros.

When we're generating fields in ComputedStyleBase, there are cases
where the generated code depends on the field template. For example,
keyword fields may have different getters than platform type fields.
Instead of doing if-else statements inside the template which makes
it quite ugly, this patch moves that logic out into their own files
as Jinja macros. Each file defines Jinja macros that specify how a
field should be generated.

This patch adds a 'keyword' template for enums, 'flag' template for
bools, and 'monotonic_flag' for nonproperty flags that can't be set
to false.

Link to generated code:
https://gist.github.com/anonymous/93f5d3907e3515d0b50adf0107f3000e

Link to diff (see differences in 'initialUnique' and 'resetUnique'):
https://www.diffchecker.com/mikllVbf

BUG=628043
==========

[shend, 2017-03-09 21:58:31]

The CQ bit was checked by shend@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-03-09 21:58:51]

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

[shend, 2017-03-09 22:00:44]

The CQ bit was checked by shend@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-03-09 22:01:20]

Dry run: 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-03-09 23:48:17]

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

[commit-bot: I haz the power, 2017-03-09 23:48:18]

Dry run: This issue passed the CQ dry run.

[shend, 2017-03-10 17:40:33]

The CQ bit was checked by shend@chromium.org

[shend, 2017-03-10 17:40:33]

The patchset sent to the CQ was uploaded after l-g-t-m from
bugsnash@chromium.org, meade@chromium.org
Link to the patchset: https://codereview.chromium.org/2693933003/#ps180001
(title: "Rebase")

[commit-bot: I haz the power, 2017-03-10 17:40:48]

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-03-10 17:45:38]

CQ is committing da patch.

Bot data: {"patchset_id": 180001, "attempt_start_ts": 1489167633213050,
"parent_rev": "b579d0a779919583b7f77793d2f47a23eab9c1c0", "commit_rev":
"6ce2feb35c970fe7d9557c851c016c6d6c8b1d39"}

[commit-bot: I haz the power, 2017-03-10 17:46:18]

Description was changed from

==========
Move field-dependent code in ComputedStyleBase to Jinja macros.

When we're generating fields in ComputedStyleBase, there are cases
where the generated code depends on the field template. For example,
keyword fields may have different getters than platform type fields.
Instead of doing if-else statements inside the template which makes
it quite ugly, this patch moves that logic out into their own files
as Jinja macros. Each file defines Jinja macros that specify how a
field should be generated.

This patch adds a 'keyword' template for enums, 'flag' template for
bools, and 'monotonic_flag' for nonproperty flags that can't be set
to false.

Link to generated code:
https://gist.github.com/anonymous/93f5d3907e3515d0b50adf0107f3000e

Link to diff (see differences in 'initialUnique' and 'resetUnique'):
https://www.diffchecker.com/mikllVbf

BUG=628043
==========

to

==========
Move field-dependent code in ComputedStyleBase to Jinja macros.

When we're generating fields in ComputedStyleBase, there are cases
where the generated code depends on the field template. For example,
keyword fields may have different getters than platform type fields.
Instead of doing if-else statements inside the template which makes
it quite ugly, this patch moves that logic out into their own files
as Jinja macros. Each file defines Jinja macros that specify how a
field should be generated.

This patch adds a 'keyword' template for enums, 'flag' template for
bools, and 'monotonic_flag' for nonproperty flags that can't be set
to false.

Link to generated code:
https://gist.github.com/anonymous/93f5d3907e3515d0b50adf0107f3000e

Link to diff (see differences in 'initialUnique' and 'resetUnique'):
https://www.diffchecker.com/mikllVbf

BUG=628043

Review-Url: https://codereview.chromium.org/2693933003
Cr-Commit-Position: refs/heads/master@{#456095}
Committed:
https://chromium.googlesource.com/chromium/src/+/6ce2feb35c970fe7d9557c851c01...
==========

[commit-bot: I haz the power, 2017-03-10 17:46:19]

Committed patchset #10 (id:180001) as
https://chromium.googlesource.com/chromium/src/+/6ce2feb35c970fe7d9557c851c01...