Make ComputedStyleBase a templated class to allow it to use functions on ComputedStyle

Make ComputedStyleBase a templated class to allow it to use functions on ComputedStyle

Make ComputedStyleBase a templated class to allow it to use functions
on ComputedStyle. This allows us to diff functions on ComputedStyle that
ComputedStyleBase would otherwise not know about.

Diff: https://gist.github.com/nainar/cced45292622ae9fb1d8c1185be9399e/revisions
Review-Url: https://codereview.chromium.org/2879813002
Cr-Commit-Position: refs/heads/master@{#471667}
Committed: https://chromium.googlesource.com/chromium/src/+/8ace54b1623362bd0720adf22811f237d93344a0

[nainar, 2017-05-12 05:42:59]

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

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

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 05:44:39]

Description was changed from

==========
Make ComputedStyleBase a templated class to allow it to use functions on
ComputedStyle

Make ComputedStyleBase a templated class to allow it to use functions
on ComputedStyle. This allows us to diff functions on ComputedStyle that
ComputedStyleBase would otherwise not know about.

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

to

==========
Make ComputedStyleBase a templated class to allow it to use functions on
ComputedStyle

Make ComputedStyleBase a templated class to allow it to use functions
on ComputedStyle. This allows us to diff functions on ComputedStyle that
ComputedStyleBase would otherwise not know about.

Also remove size assertions because:
1. We can't use templated ComputedStyleBase here since we don't know
about ComputedStyle. 
2. These size assertions will always be true since we are generating
both the class and the expected class size

Diff: https://gist.github.com/nainar/cced45292622ae9fb1d8c1185be9399e/revisions
BUG=710938
==========

[nainar, 2017-05-12 05:45:05]

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

[nainar, 2017-05-12 05:45:06]

shend@, 

PTAL? Thanks!

[shend, 2017-05-12 06:26:01]

\o/ lgtm

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

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

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

@alancutter, 

PTAL? Thanks!

[alancutter (OOO until 2018), 2017-05-12 06:45:43]

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

https://codereview.chromium.org/2879813002/diff/1/third_party/WebKit/Source/b...
third_party/WebKit/Source/build/scripts/templates/ComputedStyleBase.h.tmpl:190: 
Put this in ComputedStyle.h

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

https://codereview.chromium.org/2879813002/diff/1/third_party/WebKit/Source/b...
third_party/WebKit/Source/build/scripts/templates/ComputedStyleBase.h.tmpl:39:
template <class T>
Rename T to ComputedStyleFinal or something similar.

https://codereview.chromium.org/2879813002/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/css/resolver/StyleResolver.cpp (right):

https://codereview.chromium.org/2879813002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/css/resolver/StyleResolver.cpp:700: :
ComputedStyleBase<ComputedStyle>::kNotAtShadowBoundary);
Can this just be ComputedStyle::kEnumValue?

[nainar, 2017-05-12 06:50:57]

Description was changed from

==========
Make ComputedStyleBase a templated class to allow it to use functions on
ComputedStyle

Make ComputedStyleBase a templated class to allow it to use functions
on ComputedStyle. This allows us to diff functions on ComputedStyle that
ComputedStyleBase would otherwise not know about.

Also remove size assertions because:
1. We can't use templated ComputedStyleBase here since we don't know
about ComputedStyle. 
2. These size assertions will always be true since we are generating
both the class and the expected class size

Diff: https://gist.github.com/nainar/cced45292622ae9fb1d8c1185be9399e/revisions
BUG=710938
==========

to

==========
Make ComputedStyleBase a templated class to allow it to use functions on
ComputedStyle

Make ComputedStyleBase a templated class to allow it to use functions
on ComputedStyle. This allows us to diff functions on ComputedStyle that
ComputedStyleBase would otherwise not know about.

Diff: https://gist.github.com/nainar/cced45292622ae9fb1d8c1185be9399e/revisions
BUG=710938
==========

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

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:59:03]

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:59:19]

Made the changes, PTAL?

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

https://codereview.chromium.org/2879813002/diff/1/third_party/WebKit/Source/b...
third_party/WebKit/Source/build/scripts/templates/ComputedStyleBase.h.tmpl:190: 
Done. Removed the CL description too

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

https://codereview.chromium.org/2879813002/diff/1/third_party/WebKit/Source/b...
third_party/WebKit/Source/build/scripts/templates/ComputedStyleBase.h.tmpl:39:
template <class T>
Done.

https://codereview.chromium.org/2879813002/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/css/resolver/StyleResolver.cpp (right):

https://codereview.chromium.org/2879813002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/css/resolver/StyleResolver.cpp:700: :
ComputedStyleBase<ComputedStyle>::kNotAtShadowBoundary);
Yup, Done.

[alancutter (OOO until 2018), 2017-05-12 07:06:22]

lgtm with nits.

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

https://codereview.chromium.org/2879813002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/build/scripts/templates/ComputedStyleBase.h.tmpl:50:
// ComputedStyleBase is a templated class to allow it to use functions
This should be clearer that this may only be templated with the ComputedStyle
class, no other class is allowed.

https://codereview.chromium.org/2879813002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/build/scripts/templates/ComputedStyleBase.h.tmpl:51:
// on ComputedStyle. This allows us to diff functions on ComputedStyle that
*This allows ComputedStyleBase to use hand written functions...

[nainar, 2017-05-12 07:27:13]

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:27:32]

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:31:33]

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

https://codereview.chromium.org/2879813002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/build/scripts/templates/ComputedStyleBase.h.tmpl:50:
// ComputedStyleBase is a templated class to allow it to use functions
Done.

https://codereview.chromium.org/2879813002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/build/scripts/templates/ComputedStyleBase.h.tmpl:51:
// on ComputedStyle. This allows us to diff functions on ComputedStyle that
Done.

[nainar, 2017-05-12 08:02:59]

alancutter@ had the idea that instead of making a template file. We could
generate the definitions for these custom functions in ComputedStyleBase and
then implement them in ComputedStyle.cpp. 

This solves the problem of ComputedStyleBase knowing about the function. 

You can specify the custom functions in the CSSProperties.json5 file.

[commit-bot: I haz the power, 2017-05-12 08:49:33]

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

[commit-bot: I haz the power, 2017-05-12 08:49:34]

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

[shend, 2017-05-15 00:03:00]

On 2017/05/12 at 08:02:59, nainar wrote:
> alancutter@ had the idea that instead of making a template file. We could
generate the definitions for these custom functions in ComputedStyleBase and
then implement them in ComputedStyle.cpp. 
> 
> This solves the problem of ComputedStyleBase knowing about the function. 
> 
> You can specify the custom functions in the CSSProperties.json5 file.

Interesting idea. So something like this?

// CustomFunctions.json5
// I don't think you can use CSSProperties.json5, since some functions depend on
multiple fields.
{
  data: ["bool HasFilter()", "bool HasTransform()", "float BorderLeftWidth()"]
}

// Generated ComputedStyleBase.h
bool HasFilter() const;
bool HasTransform() const;
float BorderLeftWidth() const;

bool diff(const ComputedStyleBase& o) { return HasFilter() != o.HasFilter(); }

// ComputedStyle.cpp
// Have to use "ComputedStyleBase" instead of "ComputedStyle"
bool ComputedStyleBase::HasFilter() const { return true; }
bool ComputedStyleBase::HasTransform() const { return true; }
float ComputedStyleBase::BorderLeftWidth() const { return 1.0; }

I think it's a great idea. Despite a couple of annoyances (extra file and
implementing ComputedStyleBase in ComputedStyle.cpp), it's easier to understand
what's going on compared to CRTP.

[nainar, 2017-05-15 02:17:57]

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:18: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-15 03:29:36]

On 2017/05/15 at 00:03:00, shend wrote:
> On 2017/05/12 at 08:02:59, nainar wrote:
> > alancutter@ had the idea that instead of making a template file. We could
generate the definitions for these custom functions in ComputedStyleBase and
then implement them in ComputedStyle.cpp. 
> > 
> > This solves the problem of ComputedStyleBase knowing about the function. 
> > 
> > You can specify the custom functions in the CSSProperties.json5 file.
> 
> Interesting idea. So something like this?
> 
> // CustomFunctions.json5
> // I don't think you can use CSSProperties.json5, since some functions depend
on multiple fields.
> {
>   data: ["bool HasFilter()", "bool HasTransform()", "float BorderLeftWidth()"]
> }
> 
> // Generated ComputedStyleBase.h
> bool HasFilter() const;
> bool HasTransform() const;
> float BorderLeftWidth() const;
> 
> bool diff(const ComputedStyleBase& o) { return HasFilter() != o.HasFilter(); }
> 
> // ComputedStyle.cpp
> // Have to use "ComputedStyleBase" instead of "ComputedStyle"
> bool ComputedStyleBase::HasFilter() const { return true; }
> bool ComputedStyleBase::HasTransform() const { return true; }
> float ComputedStyleBase::BorderLeftWidth() const { return 1.0; }
> 
> I think it's a great idea. Despite a couple of annoyances (extra file and
implementing ComputedStyleBase in ComputedStyle.cpp), it's easier to understand
what's going on compared to CRTP.

The conclusion as per dicussion IRL is that we are going to go with the current
implemented idea of templating ComputedStyleBase and templating it on
ComputedStyle. 

This is as the suggested solution pushes too much C++ code into Python which
neither the CL author or reviewers prefer. 

Future work will include ensuring (if possible) that the templated type
ComputedStyleFinal is only ever ComputedStyle.

[commit-bot: I haz the power, 2017-05-15 04:09:56]

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

[commit-bot: I haz the power, 2017-05-15 04:09:57]

Dry run: This issue passed the CQ dry run.

[nainar, 2017-05-15 04:31:56]

The CQ bit was checked by nainar@chromium.org

[nainar, 2017-05-15 04:31:56]

The patchset sent to the CQ was uploaded after l-g-t-m from shend@chromium.org,
alancutter@chromium.org
Link to the patchset: https://codereview.chromium.org/2879813002/#ps60001
(title: "Formatting merge")

[commit-bot: I haz the power, 2017-05-15 04:32: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-05-15 04:36:28]

CQ is committing da patch.

Bot data: {"patchset_id": 60001, "attempt_start_ts": 1494822716035330,
"parent_rev": "0066ff300549155904a9d1708488fc617f705404", "commit_rev":
"8ace54b1623362bd0720adf22811f237d93344a0"}

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

Description was changed from

==========
Make ComputedStyleBase a templated class to allow it to use functions on
ComputedStyle

Make ComputedStyleBase a templated class to allow it to use functions
on ComputedStyle. This allows us to diff functions on ComputedStyle that
ComputedStyleBase would otherwise not know about.

Diff: https://gist.github.com/nainar/cced45292622ae9fb1d8c1185be9399e/revisions
BUG=710938
==========

to

==========
Make ComputedStyleBase a templated class to allow it to use functions on
ComputedStyle

Make ComputedStyleBase a templated class to allow it to use functions
on ComputedStyle. This allows us to diff functions on ComputedStyle that
ComputedStyleBase would otherwise not know about.

Diff: https://gist.github.com/nainar/cced45292622ae9fb1d8c1185be9399e/revisions
Review-Url: https://codereview.chromium.org/2879813002
Cr-Commit-Position: refs/heads/master@{#471667}
Committed:
https://chromium.googlesource.com/chromium/src/+/8ace54b1623362bd0720adf22811...
==========

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

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