Add 'storage_only' template to make_computed_style_base.py.

Add 'storage_only' template to make_computed_style_base.py.

As part of generating ComputedStyle, we would like to move all fields to
from ComputedStyle to ComputedStyleBase. Currently there are several
fields which remain in ComputedStyle because their interfaces are
difficult to generate.

The problem is that the current state of ComputedStyle is undesirable.
For example, the fact that some fields are in ComputedStyle and some are
in ComputedStyleBase can increase memory usage. Although we would like
to generate the interfaces for those fields properly, this will take a
long time. So trying to generate the interfaces for the remaining fields
may block progress.

This patch introduces the 'storage_only' field template, which only
generates the storage part of the field. This allows us to move the
remaining fields to ComputedStyleBase and have handwritten interfaces.

This is temporary. We would like to eventually replace 'storage_only'
with proper templates that generate actual interfaces. But for the
time being, moving all the storage to ComputedStyleBase first is a
priority.

To test this, we move the storage of 'm_emptyState' to be generated in
ComputedStyleBase, but keeping the handwritten interface in
ComputedStyle. This property is hard to generate because it has custom
logic in its setter.

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

[shend, 2017-03-13 06:38:55]

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

[shend, 2017-03-13 06:38:56]

Hi Sasha, PTAL

[sashab, 2017-03-13 07:57:12]

Specific thoughts on this patch inline.

Generally, I'm wary of patches like this because they have the problem of never
being fixed if you get busy or people are transferred off the project. I am OK
with this choice of approach (thanks for the great justification in the CL
description), but I'd just be careful to make sure the code is generated with
lots of TODOs making it clear this is temporary, and also put this in the design
doc somewhere as a temporary stage. We need to make sure the next person working
on this doesn't see storage_only and go "huh? why arent all fields storage only?
let me just..." and undo all your hard work :'(

Unfortunately, maybe the reality is that stages like this are inevitable. I'd
also be keen to see (in design doc or TODOs or wherever) your plan for moving
out of this stage later, even if you have multiple options and you haven't
chosen one yet, just so we know where this is eventually going. Then I might
feel better endorsing it. :)

Well done Darren! Not an easy problem to solve.

https://codereview.chromium.org/2742263002/diff/1/third_party/WebKit/Source/b...
File third_party/WebKit/Source/build/scripts/make_computed_style_base.py
(right):

https://codereview.chromium.org/2742263002/diff/1/third_party/WebKit/Source/b...
third_party/WebKit/Source/build/scripts/make_computed_style_base.py:30: {'name':
'emptyState', 'field': {'template': 'storage_only', 'size': 1}},
# These properties only have generated storage, and their methods are
handwritten in ComputedStyle.
# TODO(shend): Remove these fields and delete the 'storage_only' template.

https://codereview.chromium.org/2742263002/diff/1/third_party/WebKit/Source/b...
third_party/WebKit/Source/build/scripts/make_computed_style_base.py:220:
size=property_['field'].get('size', 1),
Can you document somewhere (like at the top of the json5 file where it's
defined) that if this is not specified, it defaults to 1? Otherwise I think it
should fail, "a size must be specified" kind of thing

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

https://codereview.chromium.org/2742263002/diff/1/third_party/WebKit/Source/b...
third_party/WebKit/Source/build/scripts/templates/ComputedStyleBase.h.tmpl:21:
'keyword': keyword, 'flag': flag, 'monotonic_flag': monotonic_flag,
'storage_only': storage_only
To think about -- these are starting to repeat all over the place (py file, this
file, json5 file, etc). May be worth defining elsewhere so there's a globally
accessible list, especially if they have to be consistent.

[shend, 2017-03-14 22:20:05]

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

[shend, 2017-03-14 22:20:06]

Moved sashab to cc.
Eddy, PTAL :)

https://codereview.chromium.org/2742263002/diff/1/third_party/WebKit/Source/b...
File third_party/WebKit/Source/build/scripts/make_computed_style_base.py
(right):

https://codereview.chromium.org/2742263002/diff/1/third_party/WebKit/Source/b...
third_party/WebKit/Source/build/scripts/make_computed_style_base.py:30: {'name':
'emptyState', 'field': {'template': 'storage_only', 'size': 1}},
On 2017/03/13 at 07:57:11, sashab wrote:
> # These properties only have generated storage, and their methods are
handwritten in ComputedStyle.
> # TODO(shend): Remove these fields and delete the 'storage_only' template.

Done.

https://codereview.chromium.org/2742263002/diff/1/third_party/WebKit/Source/b...
third_party/WebKit/Source/build/scripts/make_computed_style_base.py:220:
size=property_['field'].get('size', 1),
On 2017/03/13 at 07:57:11, sashab wrote:
> Can you document somewhere (like at the top of the json5 file where it's
defined) that if this is not specified, it defaults to 1? Otherwise I think it
should fail, "a size must be specified" kind of thing

Done.

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

https://codereview.chromium.org/2742263002/diff/1/third_party/WebKit/Source/b...
third_party/WebKit/Source/build/scripts/templates/ComputedStyleBase.h.tmpl:21:
'keyword': keyword, 'flag': flag, 'monotonic_flag': monotonic_flag,
'storage_only': storage_only
On 2017/03/13 at 07:57:11, sashab wrote:
> To think about -- these are starting to repeat all over the place (py file,
this file, json5 file, etc). May be worth defining elsewhere so there's a
globally accessible list, especially if they have to be consistent.

Yeah I agree. It's a bit tricky because they're repeated in different formats.

[meade_UTC10, 2017-03-15 06:26:33]

lgtm once you address the comments on the parent CL

[shend, 2017-03-15 23:40:27]

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

[shend, 2017-03-20 05:48:16]

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

[commit-bot: I haz the power, 2017-03-20 05:48:24]

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

[commit-bot: I haz the power, 2017-03-20 05:56:56]

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

[commit-bot: I haz the power, 2017-03-20 05:56:57]

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

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

[commit-bot: I haz the power, 2017-03-20 06:03:12]

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

[commit-bot: I haz the power, 2017-03-20 08:03:33]

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

[commit-bot: I haz the power, 2017-03-20 08:03:35]

Dry run: This issue passed the CQ dry run.

[shend, 2017-03-20 21:15:24]

The CQ bit was checked by shend@chromium.org

[shend, 2017-03-20 21:15:24]

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/2742263002/#ps80001
(title: "Add missing file")

[commit-bot: I haz the power, 2017-03-20 21:15:57]

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

[commit-bot: I haz the power, 2017-03-20 21:28:22]

CQ is committing da patch.

Bot data: {"patchset_id": 80001, "attempt_start_ts": 1490044524181410,
"parent_rev": "f7f370dba5703b1df78ae7a6bc54b498fcd18589", "commit_rev":
"2b588924dbe2205d5f52f4cb453d8d2dc20afb0b"}

[commit-bot: I haz the power, 2017-03-20 21:29:07]

Description was changed from

==========
Add 'storage_only' template to make_computed_style_base.py.

As part of generating ComputedStyle, we would like to move all fields to
from ComputedStyle to ComputedStyleBase. Currently there are several
fields which remain in ComputedStyle because their interfaces are
difficult to generate.

The problem is that the current state of ComputedStyle is undesirable.
For example, the fact that some fields are in ComputedStyle and some are
in ComputedStyleBase can increase memory usage. Although we would like
to generate the interfaces for those fields properly, this will take a
long time. So trying to generate the interfaces for the remaining fields
may block progress.

This patch introduces the 'storage_only' field template, which only
generates the storage part of the field. This allows us to move the
remaining fields to ComputedStyleBase and have handwritten interfaces.

This is temporary. We would like to eventually replace 'storage_only'
with proper templates that generate actual interfaces. But for the
time being, moving all the storage to ComputedStyleBase first is a
priority.

To test this, we move the storage of 'm_emptyState' to be generated in
ComputedStyleBase, but keeping the handwritten interface in
ComputedStyle. This property is hard to generate because it has custom
logic in its setter.

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

to

==========
Add 'storage_only' template to make_computed_style_base.py.

As part of generating ComputedStyle, we would like to move all fields to
from ComputedStyle to ComputedStyleBase. Currently there are several
fields which remain in ComputedStyle because their interfaces are
difficult to generate.

The problem is that the current state of ComputedStyle is undesirable.
For example, the fact that some fields are in ComputedStyle and some are
in ComputedStyleBase can increase memory usage. Although we would like
to generate the interfaces for those fields properly, this will take a
long time. So trying to generate the interfaces for the remaining fields
may block progress.

This patch introduces the 'storage_only' field template, which only
generates the storage part of the field. This allows us to move the
remaining fields to ComputedStyleBase and have handwritten interfaces.

This is temporary. We would like to eventually replace 'storage_only'
with proper templates that generate actual interfaces. But for the
time being, moving all the storage to ComputedStyleBase first is a
priority.

To test this, we move the storage of 'm_emptyState' to be generated in
ComputedStyleBase, but keeping the handwritten interface in
ComputedStyle. This property is hard to generate because it has custom
logic in its setter.

BUG=628043

Review-Url: https://codereview.chromium.org/2742263002
Cr-Commit-Position: refs/heads/master@{#458193}
Committed:
https://chromium.googlesource.com/chromium/src/+/2b588924dbe2205d5f52f4cb453d...
==========

[commit-bot: I haz the power, 2017-03-20 21:29:10]

Committed patchset #5 (id:80001) as
https://chromium.googlesource.com/chromium/src/+/2b588924dbe2205d5f52f4cb453d...