Generate diffs for fields in ComputedStyle that use their public getters

Generate diffs for fields in ComputedStyle that use their public getters

This patch allows us to diff fields using their public getters when
these getters do added work.

We specify the getters in a map of expressions in
ComputedStyleDiffFunctions.json5.

This can be used to diff not just public getter but any function that
uses the field. Hence the name map_of_expressions.

Diff: https://gist.github.com/ae54c525ef49c24b7707848546e78c73/revisions

BUG=710938
Review-Url: https://codereview.chromium.org/2876803003
Cr-Commit-Position: refs/heads/master@{#472605}
Committed: https://chromium.googlesource.com/chromium/src/+/1f05e0767786c4b551c6cd3846dce6c183500747

[nainar, 2017-05-12 06:46:58]

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

[commit-bot: I haz the power, 2017-05-12 06:47:16]

Dry run: CQ is trying da patch.

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

[nainar, 2017-05-12 06:48:26]

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

[nainar, 2017-05-12 06:48:27]

shend@, 

PTAL? Thanks!

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

https://codereview.chromium.org/2876803003/diff/1/third_party/WebKit/Source/b...
third_party/WebKit/Source/build/scripts/templates/ComputedStyleBase.h.tmpl:117:
const T& self = static_cast<const T&>(*this);
Can't decide if this should be in this tmpl file or field.tmpl

[shend, 2017-05-12 06:59:17]

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

https://codereview.chromium.org/2876803003/diff/1/third_party/WebKit/Source/b...
third_party/WebKit/Source/build/scripts/templates/ComputedStyleBase.h.tmpl:117:
const T& self = static_cast<const T&>(*this);
On 2017/05/12 at 06:48:27, nainar wrote:
> Can't decide if this should be in this tmpl file or field.tmpl

Should be here. If we want to use multiple fieldwise_diffs, then we don't want
to generate this code multiple times.

Nit: To do this super-properly, we would pass "self" to fieldwise_diff as a
parameter, so fieldwise_diff doesn't assume that "self" is always called "self".
No strong opinion about this though, as other Jinja macros violate this as well.

https://codereview.chromium.org/2876803003/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/css/ComputedStyleDiffFunctions.json5
(right):

https://codereview.chromium.org/2876803003/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/css/ComputedStyleDiffFunctions.json5:13: //
functions that use fields to determine a value).
Maybe explain what the keys and what the corresponding values should be?

https://codereview.chromium.org/2876803003/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/style/ComputedStyle.cpp (left):

https://codereview.chromium.org/2876803003/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/style/ComputedStyle.cpp:592: if
(!(BorderWidthEquals(BorderLeftWidth(), other.BorderLeftWidth())) ||
The generated code does == directly, instead of BorderWidthEquals which calls
WidthToFixedPoint. Is there a change in behaviour here?

[nainar, 2017-05-12 07:06:58]

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

[commit-bot: I haz the power, 2017-05-12 07:07:09]

Dry run: CQ is trying da patch.

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

[nainar, 2017-05-12 07:07:47]

Made the changes you asked for, 

PTAL? Thanks!

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

https://codereview.chromium.org/2876803003/diff/1/third_party/WebKit/Source/b...
third_party/WebKit/Source/build/scripts/templates/ComputedStyleBase.h.tmpl:117:
const T& self = static_cast<const T&>(*this);
On 2017/05/12 at 06:59:17, shend wrote:
> On 2017/05/12 at 06:48:27, nainar wrote:
> > Can't decide if this should be in this tmpl file or field.tmpl
> 
> Should be here. If we want to use multiple fieldwise_diffs, then we don't want
to generate this code multiple times.
> 
> Nit: To do this super-properly, we would pass "self" to fieldwise_diff as a
parameter, so fieldwise_diff doesn't assume that "self" is always called "self".
No strong opinion about this though, as other Jinja macros violate this as well.

yup, passing self into fieldwise_diff

https://codereview.chromium.org/2876803003/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/css/ComputedStyleDiffFunctions.json5
(right):

https://codereview.chromium.org/2876803003/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/css/ComputedStyleDiffFunctions.json5:13: //
functions that use fields to determine a value).
Done.

https://codereview.chromium.org/2876803003/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/style/ComputedStyle.cpp (left):

https://codereview.chromium.org/2876803003/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/style/ComputedStyle.cpp:592: if
(!(BorderWidthEquals(BorderLeftWidth(), other.BorderLeftWidth())) ||
On 2017/05/12 at 06:59:17, shend wrote:
> The generated code does == directly, instead of BorderWidthEquals which calls
WidthToFixedPoint. Is there a change in behaviour here?

So I checked before I landed my CL splitting border-*-width the diffing was the
same as the generated code:
https://codereview.chromium.org/2861773004/diff/100001/third_party/WebKit/Sou...

[shend, 2017-05-12 07:10:41]

lgtm when u get build working

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

https://codereview.chromium.org/2876803003/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/build/scripts/templates/ComputedStyleBase.h.tmpl:132:
{{fieldwise_diff(self, groups_to_diff)|indent(2)}}
might have to be "self" in quotes.

[shend, 2017-05-12 07:10:41]

lgtm when u get build working

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

https://codereview.chromium.org/2876803003/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/build/scripts/templates/ComputedStyleBase.h.tmpl:132:
{{fieldwise_diff(self, groups_to_diff)|indent(2)}}
might have to be "self" in quotes.

[nainar, 2017-05-12 07:14:05]

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

[commit-bot: I haz the power, 2017-05-12 07:14:10]

Dry run: CQ is trying da patch.

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

[nainar, 2017-05-12 07:14:31]

nainar@chromium.org changed reviewers:
+ alancutter@chromium.org

[nainar, 2017-05-12 07:14:31]

alancutter@, 

PTAL? Thanks!

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

https://codereview.chromium.org/2876803003/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/build/scripts/templates/ComputedStyleBase.h.tmpl:132:
{{fieldwise_diff(self, groups_to_diff)|indent(2)}}
yup. lol .done

[commit-bot: I haz the power, 2017-05-12 08:27:14]

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

[commit-bot: I haz the power, 2017-05-12 08:27:15]

Dry run: Try jobs failed on following builders:
  linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)

[nainar, 2017-05-15 02:21:07]

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

[commit-bot: I haz the power, 2017-05-15 02:21:22]

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-05-15 04:23:45]

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

[commit-bot: I haz the power, 2017-05-15 04:23:46]

Dry run: This issue passed the CQ dry run.

[alancutter (OOO until 2018), 2017-05-16 00:43:56]

https://codereview.chromium.org/2876803003/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/css/ComputedStyleDiffFunctions.json5
(right):

https://codereview.chromium.org/2876803003/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/css/ComputedStyleDiffFunctions.json5:9: fields: {
Rename as fields_to_diff to match code and avoid comment.

https://codereview.chromium.org/2876803003/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/css/ComputedStyleDiffFunctions.json5:14:
map_of_expressions: {
methods_to_diff: [
  {
    name: "BorderLeftWidth",
    field_dependencies: ["border-left-width"],
  },
],
WDYT?

[nainar, 2017-05-16 01:24:16]

Description was changed from

==========
Generate diffs for fields in ComputedStyle that use their public getters

This patch allows us to diff fields using their public getters when
these getters do added work.

We specify the getters in a map of expressions in
ComputedStyleDiffFunctions.json5.

This can be used to diff not just public getter but any function that
uses the field. Hence the name map_of_expressions.

Diff: https://gist.github.com/51fe19c7f597836deab1e0099a5187d0/revisions

BUG=710938
==========

to

==========
Generate diffs for fields in ComputedStyle that use their public getters

This patch allows us to diff fields using their public getters when
these getters do added work.

We specify the getters in a map of expressions in
ComputedStyleDiffFunctions.json5.

This can be used to diff not just public getter but any function that
uses the field. Hence the name map_of_expressions.

Diff: https://gist.github.com/ae54c525ef49c24b7707848546e78c73/revisions

BUG=710938
==========

[nainar, 2017-05-16 01:24:22]

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

[nainar, 2017-05-16 01:24:44]

alancutter@, 

Made the changes you asked for. PTAL? Thanks!

https://codereview.chromium.org/2876803003/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/css/ComputedStyleDiffFunctions.json5
(right):

https://codereview.chromium.org/2876803003/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/css/ComputedStyleDiffFunctions.json5:9: fields: {
Done.

https://codereview.chromium.org/2876803003/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/css/ComputedStyleDiffFunctions.json5:14:
map_of_expressions: {
On 2017/05/16 at 00:43:56, alancutter wrote:
> methods_to_diff: [
>   {
>     name: "BorderLeftWidth",
>     field_dependencies: ["border-left-width"],
>   },
> ],
> WDYT?

Done as the following:

methods_to_diff: [
  {
    method: "BorderLeftWidth",
    field_dependencies: ["border-left-width"],
  },
],

[commit-bot: I haz the power, 2017-05-16 01:25:23]

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-05-16 01:48:07]

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

[commit-bot: I haz the power, 2017-05-16 01:48:08]

Dry run: Try jobs failed on following builders:
  chromeos_amd64-generic_chromium_compile_only_ng on
master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_amd64-...)
  linux_chromium_compile_dbg_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  win_chromium_compile_dbg_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_comp...)
  win_clang on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_clang/builds/...)

[alancutter (OOO until 2018), 2017-05-16 03:28:38]

https://codereview.chromium.org/2876803003/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/build/scripts/make_computed_style_base.py
(right):

https://codereview.chromium.org/2876803003/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/build/scripts/make_computed_style_base.py:200: return
field_dependencies
This looks like it's creating a list of lists.

https://codereview.chromium.org/2876803003/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/css/ComputedStyleDiffFunctions.json5
(right):

https://codereview.chromium.org/2876803003/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/css/ComputedStyleDiffFunctions.json5:12: //
functions that use fields to determine a value).
Document the structure of the items here.

https://codereview.chromium.org/2876803003/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/css/ComputedStyleDiffFunctions.json5:29: {
I don't think alignment like this is suitable for arrays with many elements. I
think indentation like data has is more consistent and a better use of
horizontal space.

[nainar, 2017-05-16 03:45:38]

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

[commit-bot: I haz the power, 2017-05-16 03:46:12]

Dry run: CQ is trying da patch.

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

[nainar, 2017-05-16 03:46:13]

alancutter@

Made the changes you asked for. PTAL? Thanks!

https://codereview.chromium.org/2876803003/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/build/scripts/make_computed_style_base.py
(right):

https://codereview.chromium.org/2876803003/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/build/scripts/make_computed_style_base.py:200: return
field_dependencies
On 2017/05/16 at 03:28:38, alancutter wrote:
> This looks like it's creating a list of lists.

Fixed.

https://codereview.chromium.org/2876803003/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/css/ComputedStyleDiffFunctions.json5
(right):

https://codereview.chromium.org/2876803003/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/css/ComputedStyleDiffFunctions.json5:29: {
Done.

[alancutter (OOO until 2018), 2017-05-16 04:08:23]

lgtm

[nainar, 2017-05-16 04:36:25]

The CQ bit was unchecked by nainar@chromium.org

[nainar, 2017-05-16 04:36:28]

The CQ bit was checked by nainar@chromium.org

[nainar, 2017-05-16 04:36:28]

The patchset sent to the CQ was uploaded after l-g-t-m from shend@chromium.org
Link to the patchset: https://codereview.chromium.org/2876803003/#ps100001
(title: "alancutter@'s suggestions")

[commit-bot: I haz the power, 2017-05-16 04:37:00]

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-16 06:36:15]

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

[commit-bot: I haz the power, 2017-05-16 06:36:16]

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_...)

[nainar, 2017-05-17 00:14:29]

The CQ bit was checked by nainar@chromium.org

[commit-bot: I haz the power, 2017-05-17 00:15:36]

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-17 02:44:34]

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

[commit-bot: I haz the power, 2017-05-17 02:44: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_...)

[nainar, 2017-05-17 07:06:28]

The CQ bit was checked by nainar@chromium.org

[commit-bot: I haz the power, 2017-05-17 07:07:05]

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-17 11:20:32]

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

[commit-bot: I haz the power, 2017-05-17 11:20:34]

Try jobs failed on following builders:
  win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

[nainar, 2017-05-17 11:54:04]

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

[commit-bot: I haz the power, 2017-05-17 11:54:21]

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-05-17 13:53:12]

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

[commit-bot: I haz the power, 2017-05-17 13:53:13]

Dry run: This issue passed the CQ dry run.

[nainar, 2017-05-17 23:42:45]

The CQ bit was checked by nainar@chromium.org

[commit-bot: I haz the power, 2017-05-17 23:44:23]

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-17 23:55:31]

CQ is committing da patch.

Bot data: {"patchset_id": 100001, "attempt_start_ts": 1495064565081930,
"parent_rev": "7fa1d036e427d6a1fca97f8d954835d8d7220986", "commit_rev":
"1f05e0767786c4b551c6cd3846dce6c183500747"}

[commit-bot: I haz the power, 2017-05-17 23:55:46]

Description was changed from

==========
Generate diffs for fields in ComputedStyle that use their public getters

This patch allows us to diff fields using their public getters when
these getters do added work.

We specify the getters in a map of expressions in
ComputedStyleDiffFunctions.json5.

This can be used to diff not just public getter but any function that
uses the field. Hence the name map_of_expressions.

Diff: https://gist.github.com/ae54c525ef49c24b7707848546e78c73/revisions

BUG=710938
==========

to

==========
Generate diffs for fields in ComputedStyle that use their public getters

This patch allows us to diff fields using their public getters when
these getters do added work.

We specify the getters in a map of expressions in
ComputedStyleDiffFunctions.json5.

This can be used to diff not just public getter but any function that
uses the field. Hence the name map_of_expressions.

Diff: https://gist.github.com/ae54c525ef49c24b7707848546e78c73/revisions

BUG=710938

Review-Url: https://codereview.chromium.org/2876803003
Cr-Commit-Position: refs/heads/master@{#472605}
Committed:
https://chromium.googlesource.com/chromium/src/+/1f05e0767786c4b551c6cd3846dc...
==========

[commit-bot: I haz the power, 2017-05-17 23:55:47]

Committed patchset #6 (id:100001) as
https://chromium.googlesource.com/chromium/src/+/1f05e0767786c4b551c6cd3846dc...