|
|
DescriptionMoved 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
Patch Set 1 #Patch Set 2 : Add comment #Patch Set 3 : Rebase onto master #
Total comments: 6
Patch Set 4 : Rebase #Patch Set 5 : Rebase #
Messages
Total messages: 39 (24 generated)
The CQ bit was checked by shend@chromium.org to run a CQ dry run
shend@chromium.org changed reviewers: + nainar@chromium.org
Hi Naina, PTAL :)
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
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...)
The CQ bit was checked by shend@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
lgtm
shend@chromium.org changed reviewers: + sashab@chromium.org
Hi Sasha, PTAL
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
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 ==========
The CQ bit was checked by shend@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
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.
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 :)
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. :)
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.
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.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Sasha PTAL again, I've refactored the nonproperty setters to match the property ones.
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 ==========
lgtm
The CQ bit was checked by shend@chromium.org
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")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by shend@chromium.org
shend@chromium.org changed reviewers: + meade@chromium.org
Hi Eddy, PTAL :)
lgtm
The CQ bit was checked by shend@chromium.org
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")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch. Bot data: {"patchset_id": 80001, "attempt_start_ts": 1487037324014430, "parent_rev": "433bf5f0aac030d053f83e61ca016d90c8599fb7", "commit_rev": "dfb72e700f9603d638fb9510d9c2f015c26cb8e8"}
Message was sent while issue was closed.
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... ==========
Message was sent while issue was closed.
Committed patchset #5 (id:80001) as https://chromium.googlesource.com/chromium/src/+/dfb72e700f9603d638fb9510d9c2... |