Add Jinja2 macro for adding trailing strings.

Add Jinja2 macro for adding trailing strings.

There's a lot of repetitive code in ComputedStyleBase.h.tmpl to handle
formatting. For example, the member initialiser list in the generated
constructor needs a trailing comma for every member except the last one.
This patch moves such code into a generic macro in macros.tmpl, which
which makes it available for other templates.

This will be used more frequently when we start introducing more
complexity to the ComputedStyle templates.

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

[shend, 2017-02-08 22:18:27]

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

[shend, 2017-02-08 22:18:28]

Hi Bugs, PTAL

[Bugs Nash, 2017-02-09 02:46:43]

lgtm

[shend, 2017-02-09 21:26:23]

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

[shend, 2017-02-09 21:26:24]

Hi Eddy, PTAL :)

[Bugs Nash, 2017-02-12 22:48:20]

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

[Bugs Nash, 2017-02-12 22:48:20]

lgtm

[Bugs Nash, 2017-02-12 22:50:24]

On 2017/02/12 at 22:48:20, Bugs Nash wrote:
> lgtm

I swear I didn't just do that lgtm :s

[shend, 2017-02-13 00:36:49]

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

[shend, 2017-02-13 00:36:50]

Hi Eddy, PTAL

[meade_UTC10, 2017-02-13 06:19:59]

lgtm

Nice fix! One small comment.

https://codereview.chromium.org/2680123002/diff/1/third_party/WebKit/Source/b...
File third_party/WebKit/Source/build/scripts/templates/macros.tmpl (right):

https://codereview.chromium.org/2680123002/diff/1/third_party/WebKit/Source/b...
third_party/WebKit/Source/build/scripts/templates/macros.tmpl:60: {% macro
trailing(loop, str) -%}
Would it make sense to pass in loop.last here instead of passing in loop? It
made me double take a little on why loop is being passed into trailing()
above...

[shend, 2017-02-13 06:40:37]

https://codereview.chromium.org/2680123002/diff/1/third_party/WebKit/Source/b...
File third_party/WebKit/Source/build/scripts/templates/macros.tmpl (right):

https://codereview.chromium.org/2680123002/diff/1/third_party/WebKit/Source/b...
third_party/WebKit/Source/build/scripts/templates/macros.tmpl:60: {% macro
trailing(loop, str) -%}
On 2017/02/13 at 06:19:58, Eddy wrote:
> Would it make sense to pass in loop.last here instead of passing in loop? It
made me double take a little on why loop is being passed into trailing()
above...

Hmm, my intention for this macro is to print a string based on the current loop
state. If I make it take in loop.last (i.e. a bool), then the macro kinda
becomes print_if_true(bool, str). It's also less typing for the caller :P. Would
it help if I added an explanatory comment here?

[meade_UTC10, 2017-02-13 06:51:15]

https://codereview.chromium.org/2680123002/diff/1/third_party/WebKit/Source/b...
File third_party/WebKit/Source/build/scripts/templates/macros.tmpl (right):

https://codereview.chromium.org/2680123002/diff/1/third_party/WebKit/Source/b...
third_party/WebKit/Source/build/scripts/templates/macros.tmpl:60: {% macro
trailing(loop, str) -%}
On 2017/02/13 06:40:37, shend wrote:
> On 2017/02/13 at 06:19:58, Eddy wrote:
> > Would it make sense to pass in loop.last here instead of passing in loop? It
> made me double take a little on why loop is being passed into trailing()
> above...
> 
> Hmm, my intention for this macro is to print a string based on the current
loop
> state. If I make it take in loop.last (i.e. a bool), then the macro kinda
> becomes print_if_true(bool, str). It's also less typing for the caller :P.
Would
> it help if I added an explanatory comment here?

Hmm, it's really print_if_false right? I think it's actually clearer to name it
what it does - which is clearer about what's happening?


{% for field in fields %}
  {{field.name}}({{default_value(field)}}){{trailing(loop, ',')}}
{% endfor %}



{% for field in fields %}
  {{field.name}}({{default_value(field)}}){{print_if_false(loop.last, ',')}}
{% endfor %}

[shend, 2017-02-13 06:59:39]

On 2017/02/13 at 06:51:15, meade wrote:
>
https://codereview.chromium.org/2680123002/diff/1/third_party/WebKit/Source/b...
> File third_party/WebKit/Source/build/scripts/templates/macros.tmpl (right):
> 
>
https://codereview.chromium.org/2680123002/diff/1/third_party/WebKit/Source/b...
> third_party/WebKit/Source/build/scripts/templates/macros.tmpl:60: {% macro
trailing(loop, str) -%}
> On 2017/02/13 06:40:37, shend wrote:
> > On 2017/02/13 at 06:19:58, Eddy wrote:
> > > Would it make sense to pass in loop.last here instead of passing in loop?
It
> > made me double take a little on why loop is being passed into trailing()
> > above...
> > 
> > Hmm, my intention for this macro is to print a string based on the current
loop
> > state. If I make it take in loop.last (i.e. a bool), then the macro kinda
> > becomes print_if_true(bool, str). It's also less typing for the caller :P.
Would
> > it help if I added an explanatory comment here?
> 
> Hmm, it's really print_if_false right? I think it's actually clearer to name
it what it does - which is clearer about what's happening?
> 
> 
> {% for field in fields %}
>   {{field.name}}({{default_value(field)}}){{trailing(loop, ',')}}
> {% endfor %}
> 
> 
> 
> {% for field in fields %}
>   {{field.name}}({{default_value(field)}}){{print_if_false(loop.last, ',')}}
> {% endfor %}

Yeah 'trailing' does look mysterious in comparison. Instead of
print_if_false(loop.last), what do you think of print_if_not(loop.last)? I feel
like the latter reads better, and it's equivalent to print_if(not loop.last).

[shend, 2017-02-13 07:03:09]

On 2017/02/13 at 06:59:39, shend wrote:
> On 2017/02/13 at 06:51:15, meade wrote:
> >
https://codereview.chromium.org/2680123002/diff/1/third_party/WebKit/Source/b...
> > File third_party/WebKit/Source/build/scripts/templates/macros.tmpl (right):
> > 
> >
https://codereview.chromium.org/2680123002/diff/1/third_party/WebKit/Source/b...
> > third_party/WebKit/Source/build/scripts/templates/macros.tmpl:60: {% macro
trailing(loop, str) -%}
> > On 2017/02/13 06:40:37, shend wrote:
> > > On 2017/02/13 at 06:19:58, Eddy wrote:
> > > > Would it make sense to pass in loop.last here instead of passing in
loop? It
> > > made me double take a little on why loop is being passed into trailing()
> > > above...
> > > 
> > > Hmm, my intention for this macro is to print a string based on the current
loop
> > > state. If I make it take in loop.last (i.e. a bool), then the macro kinda
> > > becomes print_if_true(bool, str). It's also less typing for the caller :P.
Would
> > > it help if I added an explanatory comment here?
> > 
> > Hmm, it's really print_if_false right? I think it's actually clearer to name
it what it does - which is clearer about what's happening?
> > 
> > 
> > {% for field in fields %}
> >   {{field.name}}({{default_value(field)}}){{trailing(loop, ',')}}
> > {% endfor %}
> > 
> > 
> > 
> > {% for field in fields %}
> >   {{field.name}}({{default_value(field)}}){{print_if_false(loop.last, ',')}}
> > {% endfor %}
> 
> Yeah 'trailing' does look mysterious in comparison. Instead of
print_if_false(loop.last), what do you think of print_if_not(loop.last)? I feel
like the latter reads better, and it's equivalent to print_if(not loop.last).

actually, why not just have print_if and do print_if(not loop.last)?

[blink-reviews, 2017-02-13 07:07:19]

Both sound good!
print_if_not(foo) vs print_if(not foo) seem so similar that I don't mind :p

On 13 Feb. 2017 6:03 pm, <shend@chromium.org> wrote:

On 2017/02/13 at 06:59:39, shend wrote:
> On 2017/02/13 at 06:51:15, meade wrote:
> >
https://codereview.chromium.org/2680123002/diff/1/third_
party/WebKit/Source/build/scripts/templates/macros.tmpl
> > File third_party/WebKit/Source/build/scripts/templates/macros.tmpl
(right):
> >
> >
https://codereview.chromium.org/2680123002/diff/1/third_
party/WebKit/Source/build/scripts/templates/macros.tmpl#newcode60
> > third_party/WebKit/Source/build/scripts/templates/macros.tmpl:60: {%
macro
trailing(loop, str) -%}
> > On 2017/02/13 06:40:37, shend wrote:
> > > On 2017/02/13 at 06:19:58, Eddy wrote:
> > > > Would it make sense to pass in loop.last here instead of passing in
loop? It
> > > made me double take a little on why loop is being passed into
trailing()
> > > above...
> > >
> > > Hmm, my intention for this macro is to print a string based on the
current
loop
> > > state. If I make it take in loop.last (i.e. a bool), then the macro
kinda
> > > becomes print_if_true(bool, str). It's also less typing for the
caller :P.
Would
> > > it help if I added an explanatory comment here?
> >
> > Hmm, it's really print_if_false right? I think it's actually clearer to
name
it what it does - which is clearer about what's happening?
> >
> >
> > {% for field in fields %}
> > {{field.name}}({{default_value(field)}}){{trailing(loop, ',')}}
> > {% endfor %}
> >
> >
> >
> > {% for field in fields %}
> > {{field.name}}({{default_value(field)}}){{print_if_false(loop.last,
',')}}
> > {% endfor %}
>
> Yeah 'trailing' does look mysterious in comparison. Instead of
print_if_false(loop.last), what do you think of print_if_not(loop.last)? I
feel
like the latter reads better, and it's equivalent to print_if(not
loop.last).

actually, why not just have print_if and do print_if(not loop.last)?

https://codereview.chromium.org/2680123002/

-- 
You received this message because you are subscribed to the Google Groups "Blink
Reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to blink-reviews+unsubscribe@chromium.org.

[chromium-reviews, 2017-02-13 07:07:20]

Both sound good!
print_if_not(foo) vs print_if(not foo) seem so similar that I don't mind :p

On 13 Feb. 2017 6:03 pm, <shend@chromium.org> wrote:

On 2017/02/13 at 06:59:39, shend wrote:
> On 2017/02/13 at 06:51:15, meade wrote:
> >
https://codereview.chromium.org/2680123002/diff/1/third_
party/WebKit/Source/build/scripts/templates/macros.tmpl
> > File third_party/WebKit/Source/build/scripts/templates/macros.tmpl
(right):
> >
> >
https://codereview.chromium.org/2680123002/diff/1/third_
party/WebKit/Source/build/scripts/templates/macros.tmpl#newcode60
> > third_party/WebKit/Source/build/scripts/templates/macros.tmpl:60: {%
macro
trailing(loop, str) -%}
> > On 2017/02/13 06:40:37, shend wrote:
> > > On 2017/02/13 at 06:19:58, Eddy wrote:
> > > > Would it make sense to pass in loop.last here instead of passing in
loop? It
> > > made me double take a little on why loop is being passed into
trailing()
> > > above...
> > >
> > > Hmm, my intention for this macro is to print a string based on the
current
loop
> > > state. If I make it take in loop.last (i.e. a bool), then the macro
kinda
> > > becomes print_if_true(bool, str). It's also less typing for the
caller :P.
Would
> > > it help if I added an explanatory comment here?
> >
> > Hmm, it's really print_if_false right? I think it's actually clearer to
name
it what it does - which is clearer about what's happening?
> >
> >
> > {% for field in fields %}
> > {{field.name}}({{default_value(field)}}){{trailing(loop, ',')}}
> > {% endfor %}
> >
> >
> >
> > {% for field in fields %}
> > {{field.name}}({{default_value(field)}}){{print_if_false(loop.last,
',')}}
> > {% endfor %}
>
> Yeah 'trailing' does look mysterious in comparison. Instead of
print_if_false(loop.last), what do you think of print_if_not(loop.last)? I
feel
like the latter reads better, and it's equivalent to print_if(not
loop.last).

actually, why not just have print_if and do print_if(not loop.last)?

https://codereview.chromium.org/2680123002/

-- 
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[shend, 2017-02-13 07:19:56]

On 2017/02/13 at 07:07:20, chromium-reviews wrote:
> Both sound good!
> print_if_not(foo) vs print_if(not foo) seem so similar that I don't mind :p
> 
> On 13 Feb. 2017 6:03 pm, <shend@chromium.org> wrote:
> 
> On 2017/02/13 at 06:59:39, shend wrote:
> > On 2017/02/13 at 06:51:15, meade wrote:
> > >
> https://codereview.chromium.org/2680123002/diff/1/third_
> party/WebKit/Source/build/scripts/templates/macros.tmpl
> > > File third_party/WebKit/Source/build/scripts/templates/macros.tmpl
> (right):
> > >
> > >
> https://codereview.chromium.org/2680123002/diff/1/third_
> party/WebKit/Source/build/scripts/templates/macros.tmpl#newcode60
> > > third_party/WebKit/Source/build/scripts/templates/macros.tmpl:60: {%
> macro
> trailing(loop, str) -%}
> > > On 2017/02/13 06:40:37, shend wrote:
> > > > On 2017/02/13 at 06:19:58, Eddy wrote:
> > > > > Would it make sense to pass in loop.last here instead of passing in
> loop? It
> > > > made me double take a little on why loop is being passed into
> trailing()
> > > > above...
> > > >
> > > > Hmm, my intention for this macro is to print a string based on the
> current
> loop
> > > > state. If I make it take in loop.last (i.e. a bool), then the macro
> kinda
> > > > becomes print_if_true(bool, str). It's also less typing for the
> caller :P.
> Would
> > > > it help if I added an explanatory comment here?
> > >
> > > Hmm, it's really print_if_false right? I think it's actually clearer to
> name
> it what it does - which is clearer about what's happening?
> > >
> > >
> > > {% for field in fields %}
> > > {{field.name}}({{default_value(field)}}){{trailing(loop, ',')}}
> > > {% endfor %}
> > >
> > >
> > >
> > > {% for field in fields %}
> > > {{field.name}}({{default_value(field)}}){{print_if_false(loop.last,
> ',')}}
> > > {% endfor %}
> >
> > Yeah 'trailing' does look mysterious in comparison. Instead of
> print_if_false(loop.last), what do you think of print_if_not(loop.last)? I
> feel
> like the latter reads better, and it's equivalent to print_if(not
> loop.last).
> 
> actually, why not just have print_if and do print_if(not loop.last)?
> 
> https://codereview.chromium.org/2680123002/
> 
> -- 
> You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
> To unsubscribe from this group and stop receiving emails from it, send an
email to chromium-reviews+unsubscribe@chromium.org.
> 

Changed to print_if(not loop.last) so we don't need to implement both print_if
and print_if_not.

[shend, 2017-02-13 21:41:14]

The CQ bit was checked by shend@chromium.org

[shend, 2017-02-13 21:41:14]

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

[commit-bot: I haz the power, 2017-02-13 21:42:13]

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-14 00:40:06]

CQ is committing da patch.

Bot data: {"patchset_id": 20001, "attempt_start_ts": 1487022074240310,
"parent_rev": "b1230ec0b1aeafd15f311ded99d5c88718495c8d", "commit_rev":
"c0a8bdbcd693f00a522afa34e5bdceb3129bb1da"}

[commit-bot: I haz the power, 2017-02-14 00:40:43]

Description was changed from

==========
Add Jinja2 macro for adding trailing strings.

There's a lot of repetitive code in ComputedStyleBase.h.tmpl to handle
formatting. For example, the member initialiser list in the generated
constructor needs a trailing comma for every member except the last one.
This patch moves such code into a generic macro in macros.tmpl, which
which makes it available for other templates.

This will be used more frequently when we start introducing more
complexity to the ComputedStyle templates.

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

to

==========
Add Jinja2 macro for adding trailing strings.

There's a lot of repetitive code in ComputedStyleBase.h.tmpl to handle
formatting. For example, the member initialiser list in the generated
constructor needs a trailing comma for every member except the last one.
This patch moves such code into a generic macro in macros.tmpl, which
which makes it available for other templates.

This will be used more frequently when we start introducing more
complexity to the ComputedStyle templates.

BUG=628043

Review-Url: https://codereview.chromium.org/2680123002
Cr-Commit-Position: refs/heads/master@{#450173}
Committed:
https://chromium.googlesource.com/chromium/src/+/c0a8bdbcd693f00a522afa34e5bd...
==========

[commit-bot: I haz the power, 2017-02-14 00:40:45]

Committed patchset #2 (id:20001) as
https://chromium.googlesource.com/chromium/src/+/c0a8bdbcd693f00a522afa34e5bd...

[sashab, 2017-02-19 23:17:02]

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

[sashab, 2017-02-19 23:17:03]

Ahh <3 this patch. \o/

https://codereview.chromium.org/2680123002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/build/scripts/templates/ComputedStyleBase.h.tmpl
(right):

https://codereview.chromium.org/2680123002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/build/scripts/templates/ComputedStyleBase.h.tmpl:14:
{% macro default_value(field) -%}
What's the dashes for?

https://codereview.chromium.org/2680123002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/build/scripts/templates/ComputedStyleBase.h.tmpl:27:
{{field.name}}({{default_value(field)}}){{print_if(not loop.last, ',')}}
Woah loop.last awesome!!

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

https://codereview.chromium.org/2680123002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/build/scripts/templates/ComputedStyleBase.h.tmpl
(right):

https://codereview.chromium.org/2680123002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/build/scripts/templates/ComputedStyleBase.h.tmpl:14:
{% macro default_value(field) -%}
On 2017/02/19 at 23:17:03, sashab wrote:
> What's the dashes for?

They strip out leading/trailing whitespace so you can write the {% macro %}
stuff on a separate line without introducing excess new lines in the generated
code.

https://codereview.chromium.org/2680123002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/build/scripts/templates/ComputedStyleBase.h.tmpl:27:
{{field.name}}({{default_value(field)}}){{print_if(not loop.last, ',')}}
On 2017/02/19 at 23:17:03, sashab wrote:
> Woah loop.last awesome!!

\o/