Moved nonproperty 'unique' to be generated in ComputedStyleBase.

Moved nonproperty 'unique' to be generated in ComputedStyleBase.

Currently we are generating some CSS property fields in ComputedStyle.
This patch allows the code generator to generate nonproperty fields as
well. Nonproperty fields are member variables in ComputedStyle that
are not actually CSS properties (e.g. m_isLink, m_unique). Property
fields are mostly enums, whereas most nonproperty fields are boolean
flags.

To show that this is working, this patch move the nonproperty field
'unique' to be generated in ComputedStyleBase. Also moved getter /
setter methods to be generated in ComputedStyleBase.

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

[shend, 2017-02-02 03:03:31]

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

[shend, 2017-02-02 03:03:44]

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

[shend, 2017-02-02 03:03:44]

Hi Naina, PTAL :)

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

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-02-02 03:05:49]

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

[commit-bot: I haz the power, 2017-02-02 03:05:50]

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

[shend, 2017-02-02 03:07:36]

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

[commit-bot: I haz the power, 2017-02-02 03:08: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-02-02 04:40:21]

lgtm

[shend, 2017-02-02 04:47:00]

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

[shend, 2017-02-02 04:47:01]

Hi Sasha, PTAL

[commit-bot: I haz the power, 2017-02-02 06:35:49]

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

[commit-bot: I haz the power, 2017-02-02 06:35:50]

Dry run: This issue passed the CQ dry run.

[shend, 2017-02-03 04:18:18]

Description was changed from

==========
Moved nonproperty 'unique' to be generated in ComputedStyleBase.

Move the nonproperty field unique to be generated in ComputedStyleBase.
Also moved getter / setter methods to be generated in ComputedStyleBase.

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

to

==========
Moved nonproperty 'unique' to be generated in ComputedStyleBase.

Currently we are generating some CSS property fields in ComputedStyle.
This patch allows the code generator to generate nonproperty fields as
well. Nonproperty fields are member variables in ComputedStyle that
are not actually CSS properties (e.g. m_isLink, m_unique). Property
fields are mostly enums, whereas most nonproperty fields are boolean
flags. Differences in generated code:
- nonproperty fields don't have initial value static methods
- nonproperty field's setter takes no arguments and just sets the field
  to true
- nonproperty fields do not appear in inheritance or comparison code

To show that this is working, this patch move the nonproperty field
'unique' to be generated in ComputedStyleBase. Also moved getter /
setter methods to be generated in ComputedStyleBase.

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

[shend, 2017-02-03 04:18:42]

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

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

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

[sashab, 2017-02-03 04:53:06]

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

https://codereview.chromium.org/2667543002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/build/scripts/templates/ComputedStyleBase.h.tmpl:106:
{% if field.is_property or field.is_inherited_flag %}
Do you need this? Why not have a way to get the default value for nonproperty
fields?

https://codereview.chromium.org/2667543002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/build/scripts/templates/ComputedStyleBase.h.tmpl:111:
void {{field.setter_method_name}}() { {{field.name}} = true; }
Ahh, this is kind of annoying! Would be great to keep this template consistent
for all fields.

Can we refactor non-property setters to take a bool? Then the regular setter:
void {{field.setter_method_name}}({{field.storage_type}} v) { {{field.name}} =
static_cast<unsigned>(v); }

Would be like:

void setIsFoo(bool v) { m_isFoo = v; }

Technically non-property fields *should* be able to be set to both true and
false (it's just a regular field), so I think we should allow this functionality
anyway.

https://codereview.chromium.org/2667543002/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/style/ComputedStyle.h (left):

https://codereview.chromium.org/2667543002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/style/ComputedStyle.h:2624: void setUnique() {
m_nonInheritedData.m_unique = true; }
Maybe just do one prework patch to change setUnique to

setUnique(bool isUnique)

This way its consistent with property fields, which I think is better.

[shend, 2017-02-03 05:11:33]

Hi Sasha, very interesting points. I replied to your comments, let me know what
you think.

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

https://codereview.chromium.org/2667543002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/build/scripts/templates/ComputedStyleBase.h.tmpl:106:
{% if field.is_property or field.is_inherited_flag %}
On 2017/02/03 at 04:53:06, sashab wrote:
> Do you need this? Why not have a way to get the default value for nonproperty
fields?

I was trying to keep the generated code the same as the original, but yeah we
don't need this.

https://codereview.chromium.org/2667543002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/build/scripts/templates/ComputedStyleBase.h.tmpl:111:
void {{field.setter_method_name}}() { {{field.name}} = true; }
On 2017/02/03 at 04:53:06, sashab wrote:
> Ahh, this is kind of annoying! Would be great to keep this template consistent
for all fields.
> 
> Can we refactor non-property setters to take a bool? Then the regular setter:
> void {{field.setter_method_name}}({{field.storage_type}} v) { {{field.name}} =
static_cast<unsigned>(v); }
> 
> Would be like:
> 
> void setIsFoo(bool v) { m_isFoo = v; }
> 
> Technically non-property fields *should* be able to be set to both true and
false (it's just a regular field), so I think we should allow this functionality
anyway.

Yeah, so all nonproperty fields I've seen use the setIsFoo() style. I kept it
because I assumed it was like that for a reason (prevent people from setting it
to false or something). I would be hesitant about changing it without knowing
the rationale behind the setIsFoo() style (maybe it's really important). But if
there's no particular reason, then yeah your suggestion is much better.

https://codereview.chromium.org/2667543002/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/style/ComputedStyle.h (left):

https://codereview.chromium.org/2667543002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/style/ComputedStyle.h:2624: void setUnique() {
m_nonInheritedData.m_unique = true; }
On 2017/02/03 at 04:53:06, sashab wrote:
> Maybe just do one prework patch to change setUnique to
> 
> setUnique(bool isUnique)
> 
> This way its consistent with property fields, which I think is better.

See the other comment :)

[sashab, 2017-02-03 05:21:07]

How many nonproperty fields are there? Could we write a quick rename patch that
just replaces

setIsInherited()

with

setIsInherited(true)

and replaces the defn with the bool parameter, for all nonproperty fields?

If you could try this and see if it compiles, let's go for it. I'm 99% sure the
fields can all be set to both true and false, if they can't thats a bug we can
deal with later.

But yeah, I reckon you can do all the renames for these in a single patch as
prework to this one. :)

[sashab, 2017-02-03 05:22:43]

So I guess to answer your question: I don't know if there's a particular reason
they are all set this way, but there's no easy way to figure it out, so without
any code comments or documentation, I think we should just go for it :) We're
redesigning the class anyway so we should design it so fields can be set to
whatever they need to be.

[shend, 2017-02-03 05:39:22]

On 2017/02/03 at 05:22:43, sashab wrote:
> So I guess to answer your question: I don't know if there's a particular
reason they are all set this way, but there's no easy way to figure it out, so
without any code comments or documentation, I think we should just go for it :)
We're redesigning the class anyway so we should design it so fields can be set
to whatever they need to be.

Fair enough, will do.

[commit-bot: I haz the power, 2017-02-03 06:01:44]

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

[commit-bot: I haz the power, 2017-02-03 06:01:45]

Dry run: This issue passed the CQ dry run.

[shend, 2017-02-06 04:03:08]

Sasha PTAL again, I've refactored the nonproperty setters to match the property
ones.

[shend, 2017-02-06 04:03:30]

Description was changed from

==========
Moved nonproperty 'unique' to be generated in ComputedStyleBase.

Currently we are generating some CSS property fields in ComputedStyle.
This patch allows the code generator to generate nonproperty fields as
well. Nonproperty fields are member variables in ComputedStyle that
are not actually CSS properties (e.g. m_isLink, m_unique). Property
fields are mostly enums, whereas most nonproperty fields are boolean
flags. Differences in generated code:
- nonproperty fields don't have initial value static methods
- nonproperty field's setter takes no arguments and just sets the field
  to true
- nonproperty fields do not appear in inheritance or comparison code

To show that this is working, this patch move the nonproperty field
'unique' to be generated in ComputedStyleBase. Also moved getter /
setter methods to be generated in ComputedStyleBase.

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

to

==========
Moved nonproperty 'unique' to be generated in ComputedStyleBase.

Currently we are generating some CSS property fields in ComputedStyle.
This patch allows the code generator to generate nonproperty fields as
well. Nonproperty fields are member variables in ComputedStyle that
are not actually CSS properties (e.g. m_isLink, m_unique). Property
fields are mostly enums, whereas most nonproperty fields are boolean
flags.

To show that this is working, this patch move the nonproperty field
'unique' to be generated in ComputedStyleBase. Also moved getter /
setter methods to be generated in ComputedStyleBase.

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

[sashab, 2017-02-07 02:56:27]

lgtm

[shend, 2017-02-07 23:43:47]

The CQ bit was checked by shend@chromium.org

[shend, 2017-02-07 23:43:48]

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

[commit-bot: I haz the power, 2017-02-07 23:44:37]

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

[shend, 2017-02-08 00:47:57]

The CQ bit was unchecked by shend@chromium.org

[shend, 2017-02-13 22:03:20]

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

[shend, 2017-02-13 22:03:21]

Hi Eddy, PTAL :)

[meade_UTC10, 2017-02-13 23:10:59]

lgtm

[shend, 2017-02-14 01:55:24]

The CQ bit was checked by shend@chromium.org

[shend, 2017-02-14 01:55:24]

The patchset sent to the CQ was uploaded after l-g-t-m from nainar@chromium.org,
sashab@chromium.org
Link to the patchset: https://codereview.chromium.org/2667543002/#ps80001
(title: "Rebase")

[commit-bot: I haz the power, 2017-02-14 01:56:12]

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

[commit-bot: I haz the power, 2017-02-14 04:12:33]

CQ is committing da patch.

Bot data: {"patchset_id": 80001, "attempt_start_ts": 1487037324014430,
"parent_rev": "433bf5f0aac030d053f83e61ca016d90c8599fb7", "commit_rev":
"dfb72e700f9603d638fb9510d9c2f015c26cb8e8"}

[commit-bot: I haz the power, 2017-02-14 04:13:43]

Description was changed from

==========
Moved nonproperty 'unique' to be generated in ComputedStyleBase.

Currently we are generating some CSS property fields in ComputedStyle.
This patch allows the code generator to generate nonproperty fields as
well. Nonproperty fields are member variables in ComputedStyle that
are not actually CSS properties (e.g. m_isLink, m_unique). Property
fields are mostly enums, whereas most nonproperty fields are boolean
flags.

To show that this is working, this patch move the nonproperty field
'unique' to be generated in ComputedStyleBase. Also moved getter /
setter methods to be generated in ComputedStyleBase.

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

to

==========
Moved nonproperty 'unique' to be generated in ComputedStyleBase.

Currently we are generating some CSS property fields in ComputedStyle.
This patch allows the code generator to generate nonproperty fields as
well. Nonproperty fields are member variables in ComputedStyle that
are not actually CSS properties (e.g. m_isLink, m_unique). Property
fields are mostly enums, whereas most nonproperty fields are boolean
flags.

To show that this is working, this patch move the nonproperty field
'unique' to be generated in ComputedStyleBase. Also moved getter /
setter methods to be generated in ComputedStyleBase.

BUG=628043

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

[commit-bot: I haz the power, 2017-02-14 04:13:45]

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