Generate subgroup StyleSurroundData in ComputedStyle.

Generate subgroup StyleSurroundData in ComputedStyle.

StyleSurroundData is a class that contains the values of the offset,
margin, padding and border properties. This patch generates this class
as a nested class inside ComputedStyle. We add an extra key in
CSSProperties.json5 called 'field_group' that specifies the name of the
group that the field is in. Fields with the same field_group are grouped
together into a nested class.

A few caveats:
- Causes perf regression because it doesn't exploit sharing of groups.
  The fix is in a dependent patch (to avoid bloating this CL). We
  will land the fix right after this patch is landed.
  https://codereview.chromium.org/2826633002
- We generate 'border' as 'storage_only', because it is a shorthand
  and not a CSS property.
- Subgroups are currently generated as nested classes of ComputedStyle,
  but this makes the code difficult to read. This may change in the
  future (such as putting subgroups in a ::detail namespace).
- We don't ASSERT_SIZE on subgroups yet.

Diff of generated files:
https://gist.github.com/darrnshn/876c8171fd134b373f28ca23e821d28f/revisions

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

[shend, 2017-04-03 01:39:40]

Description was changed from

==========
Generate subgroup StyleSurroundData in ComputedStyle.

StyleSurroundData is a class that contains the values of the offset,
margin, padding and border properties. This patch generates this class
as a nested class inside ComputedStyle. We add an extra key in
CSSProperties.json5 called 'field_group' that specifies the name of the
group that the field is in. Fields with the same field_group are grouped
together into a nested class.

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

to

==========
Generate subgroup StyleSurroundData in ComputedStyle.

StyleSurroundData is a class that contains the values of the offset,
margin, padding and border properties. This patch generates this class
as a nested class inside ComputedStyle. We add an extra key in
CSSProperties.json5 called 'field_group' that specifies the name of the
group that the field is in. Fields with the same field_group are grouped
together into a nested class.

A caveat on shorthands:
For shorthand properties, we generate the shorthand as if it was a
CSS property and leave the longhands unchanged while we come up with a
design for shorthands. This means that the getters/setters for longhands
such as margin-top are still handwritten in ComputedStyle.h

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

[shend, 2017-04-03 01:57:19]

Description was changed from

==========
Generate subgroup StyleSurroundData in ComputedStyle.

StyleSurroundData is a class that contains the values of the offset,
margin, padding and border properties. This patch generates this class
as a nested class inside ComputedStyle. We add an extra key in
CSSProperties.json5 called 'field_group' that specifies the name of the
group that the field is in. Fields with the same field_group are grouped
together into a nested class.

A caveat on shorthands:
For shorthand properties, we generate the shorthand as if it was a
CSS property and leave the longhands unchanged while we come up with a
design for shorthands. This means that the getters/setters for longhands
such as margin-top are still handwritten in ComputedStyle.h

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

to

==========
Generate subgroup StyleSurroundData in ComputedStyle.

StyleSurroundData is a class that contains the values of the offset,
margin, padding and border properties. This patch generates this class
as a nested class inside ComputedStyle. We add an extra key in
CSSProperties.json5 called 'field_group' that specifies the name of the
group that the field is in. Fields with the same field_group are grouped
together into a nested class.

A few caveats:
- For shorthand properties, we generate the shorthand as if it was a
  CSS property and leave the longhands unchanged while we come up with a
  design for shorthands. This means that the accessors for longhands
  such as margin-top are still handwritten in ComputedStyle.h
- Subgroups are currently generated as nested classes of ComputedStyle,
  but this makes the code difficult to read. This may change in the
  future (such as putting subgroups in a ::detail namespace).
- We don't ASSERT_SIZE on subgroups yet.

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

[shend, 2017-04-03 02:57:16]

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

[commit-bot: I haz the power, 2017-04-03 02:57:32]

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

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

[commit-bot: I haz the power, 2017-04-03 02:59:46]

Dry run: Try jobs failed on following builders:
  mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)

[shend, 2017-04-03 04:17:09]

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

[commit-bot: I haz the power, 2017-04-03 04:17: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-04-03 04:24:58]

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

[commit-bot: I haz the power, 2017-04-03 04:24:59]

Dry run: Try jobs failed on following builders:
  mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[shend, 2017-04-03 04:41:17]

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

[commit-bot: I haz the power, 2017-04-03 04:41: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-04-03 05:01:00]

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

[commit-bot: I haz the power, 2017-04-03 05:01:01]

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

[shend, 2017-04-03 05:12:44]

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

[commit-bot: I haz the power, 2017-04-03 05:12:59]

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-04-03 06:25:03]

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

[commit-bot: I haz the power, 2017-04-03 06:25:03]

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

[shend, 2017-04-03 07:02:40]

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

[commit-bot: I haz the power, 2017-04-03 07:02:51]

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-04-03 13:03:28]

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

[commit-bot: I haz the power, 2017-04-03 13:03:29]

Dry run: Try jobs failed on following builders:
  chromeos_amd64-generic_chromium_compile_only_ng on
master.tryserver.chromium.linux (JOB_TIMED_OUT,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_amd64-...)

[shend, 2017-04-03 22:24:47]

Description was changed from

==========
Generate subgroup StyleSurroundData in ComputedStyle.

StyleSurroundData is a class that contains the values of the offset,
margin, padding and border properties. This patch generates this class
as a nested class inside ComputedStyle. We add an extra key in
CSSProperties.json5 called 'field_group' that specifies the name of the
group that the field is in. Fields with the same field_group are grouped
together into a nested class.

A few caveats:
- For shorthand properties, we generate the shorthand as if it was a
  CSS property and leave the longhands unchanged while we come up with a
  design for shorthands. This means that the accessors for longhands
  such as margin-top are still handwritten in ComputedStyle.h
- Subgroups are currently generated as nested classes of ComputedStyle,
  but this makes the code difficult to read. This may change in the
  future (such as putting subgroups in a ::detail namespace).
- We don't ASSERT_SIZE on subgroups yet.

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

to

==========
Generate subgroup StyleSurroundData in ComputedStyle.

StyleSurroundData is a class that contains the values of the offset,
margin, padding and border properties. This patch generates this class
as a nested class inside ComputedStyle. We add an extra key in
CSSProperties.json5 called 'field_group' that specifies the name of the
group that the field is in. Fields with the same field_group are grouped
together into a nested class.

A few caveats:
- For shorthand properties, we generate the shorthand as if it was a
  CSS property and leave the longhands unchanged while we come up with a
  design for shorthands. This means that the accessors for longhands
  such as margin-top are still handwritten in ComputedStyle.h
- Subgroups are currently generated as nested classes of ComputedStyle,
  but this makes the code difficult to read. This may change in the
  future (such as putting subgroups in a ::detail namespace).
- We don't ASSERT_SIZE on subgroups yet.

Diff of generated files:
https://gist.github.com/darrnshn/1406fa90407aebbde6f8a078443627f7/revisions

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

[shend, 2017-04-03 22:25:10]

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

[shend, 2017-04-03 22:25:11]

Hi Eddy, PTAL.

[shend, 2017-04-04 07:19:05]

Description was changed from

==========
Generate subgroup StyleSurroundData in ComputedStyle.

StyleSurroundData is a class that contains the values of the offset,
margin, padding and border properties. This patch generates this class
as a nested class inside ComputedStyle. We add an extra key in
CSSProperties.json5 called 'field_group' that specifies the name of the
group that the field is in. Fields with the same field_group are grouped
together into a nested class.

A few caveats:
- For shorthand properties, we generate the shorthand as if it was a
  CSS property and leave the longhands unchanged while we come up with a
  design for shorthands. This means that the accessors for longhands
  such as margin-top are still handwritten in ComputedStyle.h
- Subgroups are currently generated as nested classes of ComputedStyle,
  but this makes the code difficult to read. This may change in the
  future (such as putting subgroups in a ::detail namespace).
- We don't ASSERT_SIZE on subgroups yet.

Diff of generated files:
https://gist.github.com/darrnshn/1406fa90407aebbde6f8a078443627f7/revisions

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

to

==========
Generate subgroup StyleSurroundData in ComputedStyle.

StyleSurroundData is a class that contains the values of the offset,
margin, padding and border properties. This patch generates this class
as a nested class inside ComputedStyle. We add an extra key in
CSSProperties.json5 called 'field_group' that specifies the name of the
group that the field is in. Fields with the same field_group are grouped
together into a nested class.

A few caveats:
- For shorthand properties, we generate the shorthand as if it was a
  CSS property and leave the longhands unchanged while we come up with a
  design for shorthands. This means that the accessors for longhands
  such as margin-top are still handwritten in ComputedStyle.h. The
  shorthands are generated as 'storage_only' fields so that we know
  that they shouldn't be generated as CSS properties.
- Subgroups are currently generated as nested classes of ComputedStyle,
  but this makes the code difficult to read. This may change in the
  future (such as putting subgroups in a ::detail namespace).
- We don't ASSERT_SIZE on subgroups yet.

Diff of generated files:
https://gist.github.com/darrnshn/1406fa90407aebbde6f8a078443627f7/revisions

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

[shend, 2017-04-04 07:37:10]

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

[commit-bot: I haz the power, 2017-04-04 07:37:19]

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-04-04 07:46:50]

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

[commit-bot: I haz the power, 2017-04-04 07:46:51]

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

[shend, 2017-04-04 23:43:46]

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

[commit-bot: I haz the power, 2017-04-04 23:44:34]

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-04-05 03:35:22]

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

[commit-bot: I haz the power, 2017-04-05 03:35:23]

Dry run: This issue passed the CQ dry run.

[shend, 2017-04-05 05:06:05]

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

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

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

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

[commit-bot: I haz the power, 2017-04-05 05:08:40]

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

[shend, 2017-04-05 22:57:23]

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

[commit-bot: I haz the power, 2017-04-05 22:58:02]

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-04-06 01:02:09]

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

[commit-bot: I haz the power, 2017-04-06 01:02:10]

Dry run: This issue passed the CQ dry run.

[meade_UTC10, 2017-04-06 03:29:58]

Mostly small comments...

https://codereview.chromium.org/2786883002/diff/260001/third_party/WebKit/Sou...
File third_party/WebKit/Source/build/scripts/make_computed_style_base.py
(right):

https://codereview.chromium.org/2786883002/diff/260001/third_party/WebKit/Sou...
third_party/WebKit/Source/build/scripts/make_computed_style_base.py:78: def
_num_bit_field_buckets(bit_fields):
would this be better as something like

_num_32_bit_words_for_bit_fields

?

https://codereview.chromium.org/2786883002/diff/260001/third_party/WebKit/Sou...
third_party/WebKit/Source/build/scripts/make_computed_style_base.py:91:
"""Represents a group of fields, which may contain subgroups that are
dynamically allocated."""
Could you please document the types of the parameters? This will get more and
more important as the Python code gets bigger. 

Perhaps a followup CL could improve the documentation in this file (including
the parameter types, and things like below in field.__init__ the doc is "creates
a new field", which is completely useless).

https://codereview.chromium.org/2786883002/diff/260001/third_party/WebKit/Sou...
third_party/WebKit/Source/build/scripts/make_computed_style_base.py:296: def
_reorder_fields(fields):
You're still doing packing though? I don't think this is a helpful renaming...

maybe something like _reorder_and_pack_members?

https://codereview.chromium.org/2786883002/diff/260001/third_party/WebKit/Sou...
third_party/WebKit/Source/build/scripts/make_computed_style_base.py:369: # Get
include paths
Is this really a helpful comment? :p

https://codereview.chromium.org/2786883002/diff/260001/third_party/WebKit/Sou...
File
third_party/WebKit/Source/build/scripts/templates/ComputedStyleBase.cpp.tmpl
(right):

https://codereview.chromium.org/2786883002/diff/260001/third_party/WebKit/Sou...
third_party/WebKit/Source/build/scripts/templates/ComputedStyleBase.cpp.tmpl:10:
struct SameSizeAsComputedStyleBase {
Did we end up making a decision about whether or not to keep this, and instead
rely on the assert in the generator?

https://codereview.chromium.org/2786883002/diff/260001/third_party/WebKit/Sou...
File third_party/WebKit/Source/build/scripts/templates/fields/field.tmpl
(right):

https://codereview.chromium.org/2786883002/diff/260001/third_party/WebKit/Sou...
third_party/WebKit/Source/build/scripts/templates/fields/field.tmpl:17: {% macro
get_expr(field) %}
Can we name this better (and use full words)? e.g. 

field_getter(field)
getter_expression(field)

https://codereview.chromium.org/2786883002/diff/260001/third_party/WebKit/Sou...
third_party/WebKit/Source/build/scripts/templates/fields/field.tmpl:25: {% macro
set_expr(field) %}
ditto

https://codereview.chromium.org/2786883002/diff/260001/third_party/WebKit/Sou...
File third_party/WebKit/Source/build/scripts/templates/fields/group.tmpl
(right):

https://codereview.chromium.org/2786883002/diff/260001/third_party/WebKit/Sou...
third_party/WebKit/Source/build/scripts/templates/fields/group.tmpl:3: {% macro
def_group(group): -%}
I think this can be named more clearly, group_class_def? class_def_for_group?

https://codereview.chromium.org/2786883002/diff/260001/third_party/WebKit/Sou...
third_party/WebKit/Source/build/scripts/templates/fields/group.tmpl:13: bool
operator==(const {{group.type_name}}& o) const {
Not sure if the other generators you've written missed this, but blink style is
to not use abbreviations: https://www.chromium.org/blink/coding-style

o -> other

Now that I think about it, even though tmpl files aren't really C++, we should
probably use that for generator function names too...

https://codereview.chromium.org/2786883002/diff/260001/third_party/WebKit/Sou...
third_party/WebKit/Source/build/scripts/templates/fields/group.tmpl:23: {% if
field.is_bit_field %}
How come some of the if field.is_bit_field is abstracted into field.tmpl, but
this one isn't? Should there be a macro like this:

{% macro define_field_storage(field) %}
{% if field.is_bit_field %}
unsigned {{field.name}} : {{field.size}}; // {{field.type_name}}
{% else %}
{{field.type_name}} {{field.name}};
{% endif %}
{% endmacro %}

then this just becomes

{% for field in group.fields %}
{{define_field_storage(field)}}
{% endfor %}

[shend, 2017-04-06 04:39:40]

Thanks for the comments! I've fixed a few, but had a few questions in the
replies.

Also, yes I will be tidying up the generator script with more documentation :)

https://codereview.chromium.org/2786883002/diff/260001/third_party/WebKit/Sou...
File third_party/WebKit/Source/build/scripts/make_computed_style_base.py
(right):

https://codereview.chromium.org/2786883002/diff/260001/third_party/WebKit/Sou...
third_party/WebKit/Source/build/scripts/make_computed_style_base.py:78: def
_num_bit_field_buckets(bit_fields):
On 2017/04/06 at 03:29:57, Eddy_UTC10 wrote:
> would this be better as something like
> 
> _num_32_bit_words_for_bit_fields
> 
> ?

Done.

https://codereview.chromium.org/2786883002/diff/260001/third_party/WebKit/Sou...
third_party/WebKit/Source/build/scripts/make_computed_style_base.py:91:
"""Represents a group of fields, which may contain subgroups that are
dynamically allocated."""
On 2017/04/06 at 03:29:57, Eddy_UTC10 wrote:
> Could you please document the types of the parameters? This will get more and
more important as the Python code gets bigger. 
> 
> Perhaps a followup CL could improve the documentation in this file (including
the parameter types, and things like below in field.__init__ the doc is "creates
a new field", which is completely useless).

Agreed, the documentation in this file could be significantly improved.

https://codereview.chromium.org/2786883002/diff/260001/third_party/WebKit/Sou...
third_party/WebKit/Source/build/scripts/make_computed_style_base.py:296: def
_reorder_fields(fields):
On 2017/04/06 at 03:29:57, Eddy_UTC10 wrote:
> You're still doing packing though? I don't think this is a helpful renaming...
> 
> maybe something like _reorder_and_pack_members?

For me, packing means the compiler "packing" the struct. What we're doing is
"reordering" so that the compiler can "pack" the struct.

https://codereview.chromium.org/2786883002/diff/260001/third_party/WebKit/Sou...
third_party/WebKit/Source/build/scripts/make_computed_style_base.py:369: # Get
include paths
On 2017/04/06 at 03:29:57, Eddy_UTC10 wrote:
> Is this really a helpful comment? :p

lol, removed.

https://codereview.chromium.org/2786883002/diff/260001/third_party/WebKit/Sou...
File
third_party/WebKit/Source/build/scripts/templates/ComputedStyleBase.cpp.tmpl
(right):

https://codereview.chromium.org/2786883002/diff/260001/third_party/WebKit/Sou...
third_party/WebKit/Source/build/scripts/templates/ComputedStyleBase.cpp.tmpl:10:
struct SameSizeAsComputedStyleBase {
On 2017/04/06 at 03:29:57, Eddy_UTC10 wrote:
> Did we end up making a decision about whether or not to keep this, and instead
rely on the assert in the generator?

I think the decision with this was to make a CL to remove it and we can discuss
there.

For the python assert, I now remember why I deleted it. Because of groups, this
assert now needs to be on a per group basis. This then means that the
"extra_padding" variable (we set it to nonzero in case the packing is not
optimal) has to be per group as well, which is kind of ugly imo. I guess we
could warn instead of hard erroring, but that could get annoying real quick. I
don't know if it's worth keeping the python assert.

https://codereview.chromium.org/2786883002/diff/260001/third_party/WebKit/Sou...
File third_party/WebKit/Source/build/scripts/templates/fields/field.tmpl
(right):

https://codereview.chromium.org/2786883002/diff/260001/third_party/WebKit/Sou...
third_party/WebKit/Source/build/scripts/templates/fields/field.tmpl:17: {% macro
get_expr(field) %}
On 2017/04/06 at 03:29:57, Eddy_UTC10 wrote:
> Can we name this better (and use full words)? e.g. 
> 
> field_getter(field)
> getter_expression(field)

For these macros, should the naming be treated as if they were methods of field
(e.g. field.getter_expression) or as if they were free functions
(field_getter_expression)? Technically callers can use both notations (the
latter being slightly more verbose), but I would like to be consistent.

https://codereview.chromium.org/2786883002/diff/260001/third_party/WebKit/Sou...
File third_party/WebKit/Source/build/scripts/templates/fields/group.tmpl
(right):

https://codereview.chromium.org/2786883002/diff/260001/third_party/WebKit/Sou...
third_party/WebKit/Source/build/scripts/templates/fields/group.tmpl:13: bool
operator==(const {{group.type_name}}& o) const {
On 2017/04/06 at 03:29:57, Eddy_UTC10 wrote:
> Not sure if the other generators you've written missed this, but blink style
is to not use abbreviations: https://www.chromium.org/blink/coding-style
> 
> o -> other
> 
> Now that I think about it, even though tmpl files aren't really C++, we should
probably use that for generator function names too...

Ah ok, I was merely following the handwritten code here:
https://cs.chromium.org/chromium/src/third_party/WebKit/Source/core/style/Sty...

[nainar, 2017-04-15 00:39:03]

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

[nainar, 2017-04-15 00:39:04]

Just as an FYI. 

I need the group.tmpl file committed so that I can start my work. Unless you
want me to add the file. Which can work too depending on our timelines.

[shend, 2017-04-18 06:51:30]

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

[commit-bot: I haz the power, 2017-04-18 06:51:57]

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-04-18 07:24:47]

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

[commit-bot: I haz the power, 2017-04-18 07:24:48]

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-04-18 08:20:20]

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

[commit-bot: I haz the power, 2017-04-18 08:20:34]

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-04-18 10:22:46]

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

[commit-bot: I haz the power, 2017-04-18 10:22:47]

Dry run: 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_...)

[shend, 2017-04-18 22:57:13]

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

[commit-bot: I haz the power, 2017-04-18 22:57:53]

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-04-19 00:57:14]

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

[commit-bot: I haz the power, 2017-04-19 00:57:15]

Dry run: This issue passed the CQ dry run.

[shend, 2017-04-19 01:41:30]

Description was changed from

==========
Generate subgroup StyleSurroundData in ComputedStyle.

StyleSurroundData is a class that contains the values of the offset,
margin, padding and border properties. This patch generates this class
as a nested class inside ComputedStyle. We add an extra key in
CSSProperties.json5 called 'field_group' that specifies the name of the
group that the field is in. Fields with the same field_group are grouped
together into a nested class.

A few caveats:
- For shorthand properties, we generate the shorthand as if it was a
  CSS property and leave the longhands unchanged while we come up with a
  design for shorthands. This means that the accessors for longhands
  such as margin-top are still handwritten in ComputedStyle.h. The
  shorthands are generated as 'storage_only' fields so that we know
  that they shouldn't be generated as CSS properties.
- Subgroups are currently generated as nested classes of ComputedStyle,
  but this makes the code difficult to read. This may change in the
  future (such as putting subgroups in a ::detail namespace).
- We don't ASSERT_SIZE on subgroups yet.

Diff of generated files:
https://gist.github.com/darrnshn/1406fa90407aebbde6f8a078443627f7/revisions

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

to

==========
Generate subgroup StyleSurroundData in ComputedStyle.

StyleSurroundData is a class that contains the values of the offset,
margin, padding and border properties. This patch generates this class
as a nested class inside ComputedStyle. We add an extra key in
CSSProperties.json5 called 'field_group' that specifies the name of the
group that the field is in. Fields with the same field_group are grouped
together into a nested class.

A few caveats:
- Causes perf regression because it doesn't exploit sharing of groups.
  The fix is in a dependent patch (to avoid bloating this CL). We
  will land the fix right after this patch is landed.
- For shorthand properties, we generate the shorthand as if it was a
  CSS property and leave the longhands unchanged while we come up with a
  design for shorthands. This means that the accessors for longhands
  such as margin-top are still handwritten in ComputedStyle.h. The
  shorthands are generated as 'storage_only' fields so that we know
  that they shouldn't be generated as CSS properties.
- Subgroups are currently generated as nested classes of ComputedStyle,
  but this makes the code difficult to read. This may change in the
  future (such as putting subgroups in a ::detail namespace).
- We don't ASSERT_SIZE on subgroups yet.

Diff of generated files:
https://gist.github.com/darrnshn/1406fa90407aebbde6f8a078443627f7/revisions

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

[shend, 2017-04-19 05:35:06]

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

[shend, 2017-04-19 23:43:26]

Description was changed from

==========
Generate subgroup StyleSurroundData in ComputedStyle.

StyleSurroundData is a class that contains the values of the offset,
margin, padding and border properties. This patch generates this class
as a nested class inside ComputedStyle. We add an extra key in
CSSProperties.json5 called 'field_group' that specifies the name of the
group that the field is in. Fields with the same field_group are grouped
together into a nested class.

A few caveats:
- Causes perf regression because it doesn't exploit sharing of groups.
  The fix is in a dependent patch (to avoid bloating this CL). We
  will land the fix right after this patch is landed.
- For shorthand properties, we generate the shorthand as if it was a
  CSS property and leave the longhands unchanged while we come up with a
  design for shorthands. This means that the accessors for longhands
  such as margin-top are still handwritten in ComputedStyle.h. The
  shorthands are generated as 'storage_only' fields so that we know
  that they shouldn't be generated as CSS properties.
- Subgroups are currently generated as nested classes of ComputedStyle,
  but this makes the code difficult to read. This may change in the
  future (such as putting subgroups in a ::detail namespace).
- We don't ASSERT_SIZE on subgroups yet.

Diff of generated files:
https://gist.github.com/darrnshn/1406fa90407aebbde6f8a078443627f7/revisions

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

to

==========
Generate subgroup StyleSurroundData in ComputedStyle.

StyleSurroundData is a class that contains the values of the offset,
margin, padding and border properties. This patch generates this class
as a nested class inside ComputedStyle. We add an extra key in
CSSProperties.json5 called 'field_group' that specifies the name of the
group that the field is in. Fields with the same field_group are grouped
together into a nested class.

A few caveats:
- Causes perf regression because it doesn't exploit sharing of groups.
  The fix is in a dependent patch (to avoid bloating this CL). We
  will land the fix right after this patch is landed.
  https://codereview.chromium.org/2826633002
- For shorthand properties, we generate the shorthand as if it was a
  CSS property and leave the longhands unchanged while we come up with a
  design for shorthands. This means that the accessors for longhands
  such as margin-top are still handwritten in ComputedStyle.h. The
  shorthands are generated as 'storage_only' fields so that we know
  that they shouldn't be generated as CSS properties.
- Subgroups are currently generated as nested classes of ComputedStyle,
  but this makes the code difficult to read. This may change in the
  future (such as putting subgroups in a ::detail namespace).
- We don't ASSERT_SIZE on subgroups yet.

Diff of generated files:
https://gist.github.com/darrnshn/1406fa90407aebbde6f8a078443627f7/revisions

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

[shend, 2017-04-19 23:45:15]

Description was changed from

==========
Generate subgroup StyleSurroundData in ComputedStyle.

StyleSurroundData is a class that contains the values of the offset,
margin, padding and border properties. This patch generates this class
as a nested class inside ComputedStyle. We add an extra key in
CSSProperties.json5 called 'field_group' that specifies the name of the
group that the field is in. Fields with the same field_group are grouped
together into a nested class.

A few caveats:
- Causes perf regression because it doesn't exploit sharing of groups.
  The fix is in a dependent patch (to avoid bloating this CL). We
  will land the fix right after this patch is landed.
  https://codereview.chromium.org/2826633002
- For shorthand properties, we generate the shorthand as if it was a
  CSS property and leave the longhands unchanged while we come up with a
  design for shorthands. This means that the accessors for longhands
  such as margin-top are still handwritten in ComputedStyle.h. The
  shorthands are generated as 'storage_only' fields so that we know
  that they shouldn't be generated as CSS properties.
- Subgroups are currently generated as nested classes of ComputedStyle,
  but this makes the code difficult to read. This may change in the
  future (such as putting subgroups in a ::detail namespace).
- We don't ASSERT_SIZE on subgroups yet.

Diff of generated files:
https://gist.github.com/darrnshn/1406fa90407aebbde6f8a078443627f7/revisions

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

to

==========
Generate subgroup StyleSurroundData in ComputedStyle.

StyleSurroundData is a class that contains the values of the offset,
margin, padding and border properties. This patch generates this class
as a nested class inside ComputedStyle. We add an extra key in
CSSProperties.json5 called 'field_group' that specifies the name of the
group that the field is in. Fields with the same field_group are grouped
together into a nested class.

A few caveats:
- Causes perf regression because it doesn't exploit sharing of groups.
  The fix is in a dependent patch (to avoid bloating this CL). We
  will land the fix right after this patch is landed.
  https://codereview.chromium.org/2826633002
- For margin, padding and border, we generate them as 'storage_only'
  extra fields. This is because these are shorthands and not CSS
  properties. We plan on splitting margin and padding into longhands
  so that they can generated properly.
- Subgroups are currently generated as nested classes of ComputedStyle,
  but this makes the code difficult to read. This may change in the
  future (such as putting subgroups in a ::detail namespace).
- We don't ASSERT_SIZE on subgroups yet.

Diff of generated files:
https://gist.github.com/darrnshn/1406fa90407aebbde6f8a078443627f7/revisions

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

[shend, 2017-04-19 23:48:06]

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

[commit-bot: I haz the power, 2017-04-19 23:48:33]

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-04-20 00:00:36]

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

[commit-bot: I haz the power, 2017-04-20 00:00:38]

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

[meade_UTC10, 2017-04-20 03:08:41]

lgtm, but it looks like not all the comments in group.tmpl have been
addressed...

https://codereview.chromium.org/2786883002/diff/260001/third_party/WebKit/Sou...
File third_party/WebKit/Source/build/scripts/templates/fields/field.tmpl
(right):

https://codereview.chromium.org/2786883002/diff/260001/third_party/WebKit/Sou...
third_party/WebKit/Source/build/scripts/templates/fields/field.tmpl:17: {% macro
get_expr(field) %}
On 2017/04/06 04:39:40, shend wrote:
> On 2017/04/06 at 03:29:57, Eddy_UTC10 wrote:
> > Can we name this better (and use full words)? e.g. 
> > 
> > field_getter(field)
> > getter_expression(field)
> 
> For these macros, should the naming be treated as if they were methods of
field
> (e.g. field.getter_expression) or as if they were free functions
> (field_getter_expression)? Technically callers can use both notations (the
> latter being slightly more verbose), but I would like to be consistent.

I don't have strong opinions, other than consistency, and using the one that is
the clearest about what's going on. I guess field.getter_expression is the way
to go :)

https://codereview.chromium.org/2786883002/diff/260001/third_party/WebKit/Sou...
File third_party/WebKit/Source/build/scripts/templates/fields/group.tmpl
(right):

https://codereview.chromium.org/2786883002/diff/260001/third_party/WebKit/Sou...
third_party/WebKit/Source/build/scripts/templates/fields/group.tmpl:13: bool
operator==(const {{group.type_name}}& o) const {
On 2017/04/06 04:39:40, shend wrote:
> On 2017/04/06 at 03:29:57, Eddy_UTC10 wrote:
> > Not sure if the other generators you've written missed this, but blink style
> is to not use abbreviations: https://www.chromium.org/blink/coding-style
> > 
> > o -> other
> > 
> > Now that I think about it, even though tmpl files aren't really C++, we
should
> probably use that for generator function names too...
> 
> Ah ok, I was merely following the handwritten code here:
>
https://cs.chromium.org/chromium/src/third_party/WebKit/Source/core/style/Sty...

Ah yeah I think some of that is old and predates that style guidance :p

https://codereview.chromium.org/2786883002/diff/400001/third_party/WebKit/Sou...
File third_party/WebKit/Source/build/scripts/make_computed_style_base.py
(right):

https://codereview.chromium.org/2786883002/diff/400001/third_party/WebKit/Sou...
third_party/WebKit/Source/build/scripts/make_computed_style_base.py:156:
group_member_name: The class member name representing the group.
This is the first time you mention groups in this class doc - is it the group
the field is inside, or is it a group contained within the field?

https://codereview.chromium.org/2786883002/diff/400001/third_party/WebKit/Sou...
third_party/WebKit/Source/build/scripts/make_computed_style_base.py:346:
normal_fields = [field for field in fields if not field.is_bit_field]
normal -> fields that are some type other than a bit field? bit_fields vs
non_bit_fields maybe?

https://codereview.chromium.org/2786883002/diff/400001/third_party/WebKit/Sou...
File third_party/WebKit/Source/build/scripts/templates/fields/group.tmpl
(right):

https://codereview.chromium.org/2786883002/diff/400001/third_party/WebKit/Sou...
third_party/WebKit/Source/build/scripts/templates/fields/group.tmpl:9:
PassRefPtr<{{group.type_name}}> Copy() const {
Bugs has been working on removing PassRefPtr, perhaps you should talk to him
about a suitable replacement here?

[shend, 2017-04-20 03:22:34]

Thanks Eddy!

https://codereview.chromium.org/2786883002/diff/260001/third_party/WebKit/Sou...
File third_party/WebKit/Source/build/scripts/templates/fields/field.tmpl
(right):

https://codereview.chromium.org/2786883002/diff/260001/third_party/WebKit/Sou...
third_party/WebKit/Source/build/scripts/templates/fields/field.tmpl:17: {% macro
get_expr(field) %}
On 2017/04/20 at 03:08:40, Eddy_UTC10 wrote:
> On 2017/04/06 04:39:40, shend wrote:
> > On 2017/04/06 at 03:29:57, Eddy_UTC10 wrote:
> > > Can we name this better (and use full words)? e.g. 
> > > 
> > > field_getter(field)
> > > getter_expression(field)
> > 
> > For these macros, should the naming be treated as if they were methods of
field
> > (e.g. field.getter_expression) or as if they were free functions
> > (field_getter_expression)? Technically callers can use both notations (the
> > latter being slightly more verbose), but I would like to be consistent.
> 
> I don't have strong opinions, other than consistency, and using the one that
is the clearest about what's going on. I guess field.getter_expression is the
way to go :)

Ok, I did getter_expression(field), since field.getter_expression(field) is
repetitive.

https://codereview.chromium.org/2786883002/diff/260001/third_party/WebKit/Sou...
third_party/WebKit/Source/build/scripts/templates/fields/field.tmpl:25: {% macro
set_expr(field) %}
On 2017/04/06 at 03:29:57, Eddy_UTC10 wrote:
> ditto

Done.

https://codereview.chromium.org/2786883002/diff/260001/third_party/WebKit/Sou...
File third_party/WebKit/Source/build/scripts/templates/fields/group.tmpl
(right):

https://codereview.chromium.org/2786883002/diff/260001/third_party/WebKit/Sou...
third_party/WebKit/Source/build/scripts/templates/fields/group.tmpl:3: {% macro
def_group(group): -%}
On 2017/04/06 at 03:29:57, Eddy_UTC10 wrote:
> I think this can be named more clearly, group_class_def? class_def_for_group?

Changed to define_field_group_class.

https://codereview.chromium.org/2786883002/diff/260001/third_party/WebKit/Sou...
third_party/WebKit/Source/build/scripts/templates/fields/group.tmpl:13: bool
operator==(const {{group.type_name}}& o) const {
On 2017/04/20 at 03:08:40, Eddy_UTC10 wrote:
> On 2017/04/06 04:39:40, shend wrote:
> > On 2017/04/06 at 03:29:57, Eddy_UTC10 wrote:
> > > Not sure if the other generators you've written missed this, but blink
style
> > is to not use abbreviations: https://www.chromium.org/blink/coding-style
> > > 
> > > o -> other
> > > 
> > > Now that I think about it, even though tmpl files aren't really C++, we
should
> > probably use that for generator function names too...
> > 
> > Ah ok, I was merely following the handwritten code here:
> >
https://cs.chromium.org/chromium/src/third_party/WebKit/Source/core/style/Sty...
> 
> Ah yeah I think some of that is old and predates that style guidance :p

Changed to 'other'.

https://codereview.chromium.org/2786883002/diff/260001/third_party/WebKit/Sou...
third_party/WebKit/Source/build/scripts/templates/fields/group.tmpl:23: {% if
field.is_bit_field %}
On 2017/04/06 at 03:29:58, Eddy_UTC10 wrote:
> How come some of the if field.is_bit_field is abstracted into field.tmpl, but
this one isn't? Should there be a macro like this:
> 
> {% macro define_field_storage(field) %}
> {% if field.is_bit_field %}
> unsigned {{field.name}} : {{field.size}}; // {{field.type_name}}
> {% else %}
> {{field.type_name}} {{field.name}};
> {% endif %}
> {% endmacro %}
> 
> then this just becomes
> 
> {% for field in group.fields %}
> {{define_field_storage(field)}}
> {% endfor %}

Done.

https://codereview.chromium.org/2786883002/diff/400001/third_party/WebKit/Sou...
File third_party/WebKit/Source/build/scripts/make_computed_style_base.py
(right):

https://codereview.chromium.org/2786883002/diff/400001/third_party/WebKit/Sou...
third_party/WebKit/Source/build/scripts/make_computed_style_base.py:156:
group_member_name: The class member name representing the group.
On 2017/04/20 at 03:08:40, Eddy_UTC10 wrote:
> This is the first time you mention groups in this class doc - is it the group
the field is inside, or is it a group contained within the field?

Clarified the comment.

https://codereview.chromium.org/2786883002/diff/400001/third_party/WebKit/Sou...
third_party/WebKit/Source/build/scripts/make_computed_style_base.py:346:
normal_fields = [field for field in fields if not field.is_bit_field]
On 2017/04/20 at 03:08:40, Eddy_UTC10 wrote:
> normal -> fields that are some type other than a bit field? bit_fields vs
non_bit_fields maybe?

Changed to non_bit_fields.

https://codereview.chromium.org/2786883002/diff/400001/third_party/WebKit/Sou...
File third_party/WebKit/Source/build/scripts/templates/fields/group.tmpl
(right):

https://codereview.chromium.org/2786883002/diff/400001/third_party/WebKit/Sou...
third_party/WebKit/Source/build/scripts/templates/fields/group.tmpl:9:
PassRefPtr<{{group.type_name}}> Copy() const {
On 2017/04/20 at 03:08:41, Eddy_UTC10 wrote:
> Bugs has been working on removing PassRefPtr, perhaps you should talk to him
about a suitable replacement here?

Good idea. Will do in a separate patch.

[shend, 2017-04-20 03:37:23]

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

[commit-bot: I haz the power, 2017-04-20 03:37:42]

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-04-20 05:20:42]

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

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

Dry run: This issue passed the CQ dry run.

[shend, 2017-04-24 02:02:38]

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

[commit-bot: I haz the power, 2017-04-24 02:02:54]

Dry run: CQ is trying da patch.

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

[shend, 2017-04-24 02:11:42]

The CQ bit was unchecked by shend@chromium.org

[commit-bot: I haz the power, 2017-04-24 02:11:43]

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

[commit-bot: I haz the power, 2017-04-24 02:11:44]

Dry run: Try jobs failed on following builders:
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[shend, 2017-04-24 03:25:59]

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

[commit-bot: I haz the power, 2017-04-24 03:26:06]

Dry run: CQ is trying da patch.

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

[shend, 2017-04-24 03:39:31]

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

[commit-bot: I haz the power, 2017-04-24 03:39:44]

Dry run: CQ is trying da patch.

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

[shend, 2017-04-24 03:44:52]

The CQ bit was unchecked by shend@chromium.org

[shend, 2017-04-24 03:50:41]

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

[commit-bot: I haz the power, 2017-04-24 03:50:57]

Dry run: CQ is trying da patch.

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

[shend, 2017-04-24 03:52:30]

Description was changed from

==========
Generate subgroup StyleSurroundData in ComputedStyle.

StyleSurroundData is a class that contains the values of the offset,
margin, padding and border properties. This patch generates this class
as a nested class inside ComputedStyle. We add an extra key in
CSSProperties.json5 called 'field_group' that specifies the name of the
group that the field is in. Fields with the same field_group are grouped
together into a nested class.

A few caveats:
- Causes perf regression because it doesn't exploit sharing of groups.
  The fix is in a dependent patch (to avoid bloating this CL). We
  will land the fix right after this patch is landed.
  https://codereview.chromium.org/2826633002
- For margin, padding and border, we generate them as 'storage_only'
  extra fields. This is because these are shorthands and not CSS
  properties. We plan on splitting margin and padding into longhands
  so that they can generated properly.
- Subgroups are currently generated as nested classes of ComputedStyle,
  but this makes the code difficult to read. This may change in the
  future (such as putting subgroups in a ::detail namespace).
- We don't ASSERT_SIZE on subgroups yet.

Diff of generated files:
https://gist.github.com/darrnshn/1406fa90407aebbde6f8a078443627f7/revisions

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

to

==========
Generate subgroup StyleSurroundData in ComputedStyle.

StyleSurroundData is a class that contains the values of the offset,
margin, padding and border properties. This patch generates this class
as a nested class inside ComputedStyle. We add an extra key in
CSSProperties.json5 called 'field_group' that specifies the name of the
group that the field is in. Fields with the same field_group are grouped
together into a nested class.

A few caveats:
- Causes perf regression because it doesn't exploit sharing of groups.
  The fix is in a dependent patch (to avoid bloating this CL). We
  will land the fix right after this patch is landed.
  https://codereview.chromium.org/2826633002
- We generate 'border' as 'storage_only', because it is a shorthand
  and not a CSS property.
- Subgroups are currently generated as nested classes of ComputedStyle,
  but this makes the code difficult to read. This may change in the
  future (such as putting subgroups in a ::detail namespace).
- We don't ASSERT_SIZE on subgroups yet.

Diff of generated files:
https://gist.github.com/darrnshn/1406fa90407aebbde6f8a078443627f7/revisions

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

[shend, 2017-04-24 03:57:19]

Description was changed from

==========
Generate subgroup StyleSurroundData in ComputedStyle.

StyleSurroundData is a class that contains the values of the offset,
margin, padding and border properties. This patch generates this class
as a nested class inside ComputedStyle. We add an extra key in
CSSProperties.json5 called 'field_group' that specifies the name of the
group that the field is in. Fields with the same field_group are grouped
together into a nested class.

A few caveats:
- Causes perf regression because it doesn't exploit sharing of groups.
  The fix is in a dependent patch (to avoid bloating this CL). We
  will land the fix right after this patch is landed.
  https://codereview.chromium.org/2826633002
- We generate 'border' as 'storage_only', because it is a shorthand
  and not a CSS property.
- Subgroups are currently generated as nested classes of ComputedStyle,
  but this makes the code difficult to read. This may change in the
  future (such as putting subgroups in a ::detail namespace).
- We don't ASSERT_SIZE on subgroups yet.

Diff of generated files:
https://gist.github.com/darrnshn/1406fa90407aebbde6f8a078443627f7/revisions

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

to

==========
Generate subgroup StyleSurroundData in ComputedStyle.

StyleSurroundData is a class that contains the values of the offset,
margin, padding and border properties. This patch generates this class
as a nested class inside ComputedStyle. We add an extra key in
CSSProperties.json5 called 'field_group' that specifies the name of the
group that the field is in. Fields with the same field_group are grouped
together into a nested class.

A few caveats:
- Causes perf regression because it doesn't exploit sharing of groups.
  The fix is in a dependent patch (to avoid bloating this CL). We
  will land the fix right after this patch is landed.
  https://codereview.chromium.org/2826633002
- We generate 'border' as 'storage_only', because it is a shorthand
  and not a CSS property.
- Subgroups are currently generated as nested classes of ComputedStyle,
  but this makes the code difficult to read. This may change in the
  future (such as putting subgroups in a ::detail namespace).
- We don't ASSERT_SIZE on subgroups yet.

Diff of generated files:
https://gist.github.com/darrnshn/876c8171fd134b373f28ca23e821d28f/revisions

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

[commit-bot: I haz the power, 2017-04-24 05:47:21]

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

[commit-bot: I haz the power, 2017-04-24 05:47:23]

Dry run: This issue passed the CQ dry run.

[shend, 2017-04-25 23:46:28]

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

[commit-bot: I haz the power, 2017-04-25 23:47:14]

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-04-26 04:42:25]

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

[commit-bot: I haz the power, 2017-04-26 04:42:26]

Dry run: This issue passed the CQ dry run.

[shend, 2017-04-26 05:09:20]

The CQ bit was checked by shend@chromium.org

[shend, 2017-04-26 05:09:21]

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

[commit-bot: I haz the power, 2017-04-26 05:09:38]

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-04-26 05:16:24]

CQ is committing da patch.

Bot data: {"patchset_id": 500001, "attempt_start_ts": 1493183360875490,
"parent_rev": "b546eb97eb9f5c8683f0560a625208d5217a3d16", "commit_rev":
"2692eeb5bf7dd77e68141dd9d80ff40f24c170f2"}

[commit-bot: I haz the power, 2017-04-26 05:17:06]

Description was changed from

==========
Generate subgroup StyleSurroundData in ComputedStyle.

StyleSurroundData is a class that contains the values of the offset,
margin, padding and border properties. This patch generates this class
as a nested class inside ComputedStyle. We add an extra key in
CSSProperties.json5 called 'field_group' that specifies the name of the
group that the field is in. Fields with the same field_group are grouped
together into a nested class.

A few caveats:
- Causes perf regression because it doesn't exploit sharing of groups.
  The fix is in a dependent patch (to avoid bloating this CL). We
  will land the fix right after this patch is landed.
  https://codereview.chromium.org/2826633002
- We generate 'border' as 'storage_only', because it is a shorthand
  and not a CSS property.
- Subgroups are currently generated as nested classes of ComputedStyle,
  but this makes the code difficult to read. This may change in the
  future (such as putting subgroups in a ::detail namespace).
- We don't ASSERT_SIZE on subgroups yet.

Diff of generated files:
https://gist.github.com/darrnshn/876c8171fd134b373f28ca23e821d28f/revisions

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

to

==========
Generate subgroup StyleSurroundData in ComputedStyle.

StyleSurroundData is a class that contains the values of the offset,
margin, padding and border properties. This patch generates this class
as a nested class inside ComputedStyle. We add an extra key in
CSSProperties.json5 called 'field_group' that specifies the name of the
group that the field is in. Fields with the same field_group are grouped
together into a nested class.

A few caveats:
- Causes perf regression because it doesn't exploit sharing of groups.
  The fix is in a dependent patch (to avoid bloating this CL). We
  will land the fix right after this patch is landed.
  https://codereview.chromium.org/2826633002
- We generate 'border' as 'storage_only', because it is a shorthand
  and not a CSS property.
- Subgroups are currently generated as nested classes of ComputedStyle,
  but this makes the code difficult to read. This may change in the
  future (such as putting subgroups in a ::detail namespace).
- We don't ASSERT_SIZE on subgroups yet.

Diff of generated files:
https://gist.github.com/darrnshn/876c8171fd134b373f28ca23e821d28f/revisions

BUG=628043

Review-Url: https://codereview.chromium.org/2786883002
Cr-Commit-Position: refs/heads/master@{#467239}
Committed:
https://chromium.googlesource.com/chromium/src/+/2692eeb5bf7dd77e68141dd9d80f...
==========

[commit-bot: I haz the power, 2017-04-26 05:17:08]

Committed patchset #26 (id:500001) as
https://chromium.googlesource.com/chromium/src/+/2692eeb5bf7dd77e68141dd9d80f...