Check if value changed when setting nested fields in ComputedStyleBase.

Check if value changed when setting nested fields in ComputedStyleBase.

Many of the setters in ComputedStyle use a macro called SET_VAR, which
sets the value of a field stored in a subgroup on ComputedStyle. SET_VAR
first checks if the new value is different to the old value. Only when
the value changes does it modify the field using the Access() method on
the subgroup. Since Access() could potentially do a copy, we would like
to call it as rarely as possible. Hence, SET_VAR is an optimisation that
skips a call to Access() when it is not necessary.

For example, the setter for the 'left' property might look like:

void SetLeft(const Length& v) {
  if (surround_data_->left_ != v)
    surround_data_->Access()->left_ = v;
}

As we can see, we only call Access() when the values are different,
potentially saving a copy.

However, the generated setters on ComputedStyleBase do not perform this
optimisation. This patch changes the generator to use the same
optimisation in ComputedStyleBase.

No behavioural changes are expected, but there should be less memory
allocations in ComputedStyle.

Diff of generated files:
https://gist.github.com/darrnshn/47e96b81c2ade55cb337fd658f0d6785/revisions

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

[shend, 2017-05-10 23:27:58]

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

[commit-bot: I haz the power, 2017-05-10 23:29:15]

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-05-10 23:42:41]

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

[commit-bot: I haz the power, 2017-05-10 23:42:42]

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-05-11 00:43:27]

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

[shend, 2017-05-11 00:44:10]

Description was changed from

==========
Check if value changed when setting nested fields in ComputedStyleBase.

Many of the setters in ComputedStyle use a macro called SET_VAR, which
sets the value of a field stored in a subgroup on ComputedStyle. SET_VAR
first checks if the new value is different to the old value. Only when
the value changes does it modify the field using the Access() method on
the subgroup. Since Access() could potentially do a copy, we would like
to call it as rarely as possible. Hence, SET_VAR is an optimisation that
skips a call to Access() when it is not necessary.

However, the generated setters on ComputedStyleBase do not perform this
optimisation. This patch changes the generator to use the same
optimisation in ComputedStyleBase.

No behavioural changes are expected, but there should be less memory
allocations in ComputedStyle.

Diff of generated files:
https://gist.github.com/darrnshn/141e42763b233fbccd8327ba50dfcbac/revisions

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

to

==========
Check if value changed when setting nested fields in ComputedStyleBase.

Many of the setters in ComputedStyle use a macro called SET_VAR, which
sets the value of a field stored in a subgroup on ComputedStyle. SET_VAR
first checks if the new value is different to the old value. Only when
the value changes does it modify the field using the Access() method on
the subgroup. Since Access() could potentially do a copy, we would like
to call it as rarely as possible. Hence, SET_VAR is an optimisation that
skips a call to Access() when it is not necessary.

However, the generated setters on ComputedStyleBase do not perform this
optimisation. This patch changes the generator to use the same
optimisation in ComputedStyleBase.

No behavioural changes are expected, but there should be less memory
allocations in ComputedStyle.

Diff of generated files:
https://gist.github.com/ad2f36943a8c11dbd33d45213011fc77/revisions

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

[commit-bot: I haz the power, 2017-05-11 00:45:18]

Dry run: CQ is trying da patch.

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

[shend, 2017-05-11 00:45:25]

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

[shend, 2017-05-11 00:45:26]

Hi Naina, PTAL

[nainar, 2017-05-11 01:02:16]

lgtm. nit: to make it clearer maybe add an example inline in the CL desciption?

[shend, 2017-05-11 01:50:02]

Description was changed from

==========
Check if value changed when setting nested fields in ComputedStyleBase.

Many of the setters in ComputedStyle use a macro called SET_VAR, which
sets the value of a field stored in a subgroup on ComputedStyle. SET_VAR
first checks if the new value is different to the old value. Only when
the value changes does it modify the field using the Access() method on
the subgroup. Since Access() could potentially do a copy, we would like
to call it as rarely as possible. Hence, SET_VAR is an optimisation that
skips a call to Access() when it is not necessary.

However, the generated setters on ComputedStyleBase do not perform this
optimisation. This patch changes the generator to use the same
optimisation in ComputedStyleBase.

No behavioural changes are expected, but there should be less memory
allocations in ComputedStyle.

Diff of generated files:
https://gist.github.com/ad2f36943a8c11dbd33d45213011fc77/revisions

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

to

==========
Check if value changed when setting nested fields in ComputedStyleBase.

Many of the setters in ComputedStyle use a macro called SET_VAR, which
sets the value of a field stored in a subgroup on ComputedStyle. SET_VAR
first checks if the new value is different to the old value. Only when
the value changes does it modify the field using the Access() method on
the subgroup. Since Access() could potentially do a copy, we would like
to call it as rarely as possible. Hence, SET_VAR is an optimisation that
skips a call to Access() when it is not necessary.

For example, the setter for the 'left' property might look like:

void SetLeft(const Length& v) {
  if (surround_data_->left_ != v)
    surround_data_->Access()->left_ = v;
}

As we can see, we only call Access() when the values are different,
potentially saving a copy.

However, the generated setters on ComputedStyleBase do not perform this
optimisation. This patch changes the generator to use the same
optimisation in ComputedStyleBase.

No behavioural changes are expected, but there should be less memory
allocations in ComputedStyle.

Diff of generated files:
https://gist.github.com/ad2f36943a8c11dbd33d45213011fc77/revisions

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

[shend, 2017-05-11 01:50:17]

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

[shend, 2017-05-11 01:50:18]

done. Eddy, PTAL :)

[shend, 2017-05-11 01:53:25]

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

[shend, 2017-05-11 01:53:25]

Redirecting to Alan. PTAL :)

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

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

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

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_...)

[alancutter (OOO until 2018), 2017-05-11 04:03:24]

Looking at the generated code this looks very wrong:

> surround_data_->border_bottom_left_radius_ == std::move(v)

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

On 2017/05/11 at 04:03:24, alancutter wrote:
> Looking at the generated code this looks very wrong:
> 
> > surround_data_->border_bottom_left_radius_ == std::move(v)

Oh yeah, I agree that's really weird looking, but it should still work as long
as operator== doesn't take a &&. Alternatively, I could just not use the
get_if_changed macro for the move setter? Just means a bit more duplicate code.

[alancutter (OOO until 2018), 2017-05-11 04:14:51]

On 2017/05/11 at 04:11:11, shend wrote:
> On 2017/05/11 at 04:03:24, alancutter wrote:
> > Looking at the generated code this looks very wrong:
> > 
> > > surround_data_->border_bottom_left_radius_ == std::move(v)
> 
> Oh yeah, I agree that's really weird looking, but it should still work as long
as operator== doesn't take a &&. Alternatively, I could just not use the
get_if_changed macro for the move setter? Just means a bit more duplicate code.

True though I'm still not comfortable with == std::move. I think it should be
changed.

[shend, 2017-05-11 05:34:13]

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

[commit-bot: I haz the power, 2017-05-11 05:35:02]

Dry run: CQ is trying da patch.

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

[shend, 2017-05-11 05:35:02]

On 2017/05/11 at 04:14:51, alancutter wrote:
> On 2017/05/11 at 04:11:11, shend wrote:
> > On 2017/05/11 at 04:03:24, alancutter wrote:
> > > Looking at the generated code this looks very wrong:
> > > 
> > > > surround_data_->border_bottom_left_radius_ == std::move(v)
> > 
> > Oh yeah, I agree that's really weird looking, but it should still work as
long as operator== doesn't take a &&. Alternatively, I could just not use the
get_if_changed macro for the move setter? Just means a bit more duplicate code.
> 
> True though I'm still not comfortable with == std::move. I think it should be
changed.

tis done

[shend, 2017-05-11 05:36:02]

Description was changed from

==========
Check if value changed when setting nested fields in ComputedStyleBase.

Many of the setters in ComputedStyle use a macro called SET_VAR, which
sets the value of a field stored in a subgroup on ComputedStyle. SET_VAR
first checks if the new value is different to the old value. Only when
the value changes does it modify the field using the Access() method on
the subgroup. Since Access() could potentially do a copy, we would like
to call it as rarely as possible. Hence, SET_VAR is an optimisation that
skips a call to Access() when it is not necessary.

For example, the setter for the 'left' property might look like:

void SetLeft(const Length& v) {
  if (surround_data_->left_ != v)
    surround_data_->Access()->left_ = v;
}

As we can see, we only call Access() when the values are different,
potentially saving a copy.

However, the generated setters on ComputedStyleBase do not perform this
optimisation. This patch changes the generator to use the same
optimisation in ComputedStyleBase.

No behavioural changes are expected, but there should be less memory
allocations in ComputedStyle.

Diff of generated files:
https://gist.github.com/ad2f36943a8c11dbd33d45213011fc77/revisions

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

to

==========
Check if value changed when setting nested fields in ComputedStyleBase.

Many of the setters in ComputedStyle use a macro called SET_VAR, which
sets the value of a field stored in a subgroup on ComputedStyle. SET_VAR
first checks if the new value is different to the old value. Only when
the value changes does it modify the field using the Access() method on
the subgroup. Since Access() could potentially do a copy, we would like
to call it as rarely as possible. Hence, SET_VAR is an optimisation that
skips a call to Access() when it is not necessary.

For example, the setter for the 'left' property might look like:

void SetLeft(const Length& v) {
  if (surround_data_->left_ != v)
    surround_data_->Access()->left_ = v;
}

As we can see, we only call Access() when the values are different,
potentially saving a copy.

However, the generated setters on ComputedStyleBase do not perform this
optimisation. This patch changes the generator to use the same
optimisation in ComputedStyleBase.

No behavioural changes are expected, but there should be less memory
allocations in ComputedStyle.

Diff of generated files:
https://gist.github.com/darrnshn/47e96b81c2ade55cb337fd658f0d6785/revisions

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

[alancutter (OOO until 2018), 2017-05-11 05:56:38]

https://codereview.chromium.org/2879493003/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/build/scripts/templates/fields/external.tmpl
(right):

https://codereview.chromium.org/2879493003/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/build/scripts/templates/fields/external.tmpl:13:
{{field.name}} = std::move({{value}});
Should this be referencing v instead of {{value}}?

[shend, 2017-05-11 06:02:30]

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

[shend, 2017-05-11 06:03:00]

https://codereview.chromium.org/2879493003/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/build/scripts/templates/fields/external.tmpl
(right):

https://codereview.chromium.org/2879493003/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/build/scripts/templates/fields/external.tmpl:13:
{{field.name}} = std::move({{value}});
On 2017/05/11 at 05:56:38, alancutter wrote:
> Should this be referencing v instead of {{value}}?

Oops, good catch.

[commit-bot: I haz the power, 2017-05-11 06:03:17]

Dry run: CQ is trying da patch.

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

[alancutter (OOO until 2018), 2017-05-11 06:26:51]

lgtm

[commit-bot: I haz the power, 2017-05-11 07:33:04]

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

[commit-bot: I haz the power, 2017-05-11 07:33:04]

Dry run: This issue passed the CQ dry run.

[shend, 2017-05-11 08:08:07]

The CQ bit was checked by shend@chromium.org

[shend, 2017-05-11 08:08:08]

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/2879493003/#ps60001
(title: "Rebase")

[commit-bot: I haz the power, 2017-05-11 08:08: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-05-11 08:13:39]

CQ is committing da patch.

Bot data: {"patchset_id": 60001, "attempt_start_ts": 1494490087707360,
"parent_rev": "e6be43e51ab2b92ae4fa55745bd54c8ee09f155d", "commit_rev":
"e8e5df6b838075a0dc6e584853f1f795ac5af0b3"}

[commit-bot: I haz the power, 2017-05-11 08:13:49]

Description was changed from

==========
Check if value changed when setting nested fields in ComputedStyleBase.

Many of the setters in ComputedStyle use a macro called SET_VAR, which
sets the value of a field stored in a subgroup on ComputedStyle. SET_VAR
first checks if the new value is different to the old value. Only when
the value changes does it modify the field using the Access() method on
the subgroup. Since Access() could potentially do a copy, we would like
to call it as rarely as possible. Hence, SET_VAR is an optimisation that
skips a call to Access() when it is not necessary.

For example, the setter for the 'left' property might look like:

void SetLeft(const Length& v) {
  if (surround_data_->left_ != v)
    surround_data_->Access()->left_ = v;
}

As we can see, we only call Access() when the values are different,
potentially saving a copy.

However, the generated setters on ComputedStyleBase do not perform this
optimisation. This patch changes the generator to use the same
optimisation in ComputedStyleBase.

No behavioural changes are expected, but there should be less memory
allocations in ComputedStyle.

Diff of generated files:
https://gist.github.com/darrnshn/47e96b81c2ade55cb337fd658f0d6785/revisions

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

to

==========
Check if value changed when setting nested fields in ComputedStyleBase.

Many of the setters in ComputedStyle use a macro called SET_VAR, which
sets the value of a field stored in a subgroup on ComputedStyle. SET_VAR
first checks if the new value is different to the old value. Only when
the value changes does it modify the field using the Access() method on
the subgroup. Since Access() could potentially do a copy, we would like
to call it as rarely as possible. Hence, SET_VAR is an optimisation that
skips a call to Access() when it is not necessary.

For example, the setter for the 'left' property might look like:

void SetLeft(const Length& v) {
  if (surround_data_->left_ != v)
    surround_data_->Access()->left_ = v;
}

As we can see, we only call Access() when the values are different,
potentially saving a copy.

However, the generated setters on ComputedStyleBase do not perform this
optimisation. This patch changes the generator to use the same
optimisation in ComputedStyleBase.

No behavioural changes are expected, but there should be less memory
allocations in ComputedStyle.

Diff of generated files:
https://gist.github.com/darrnshn/47e96b81c2ade55cb337fd658f0d6785/revisions

BUG=628043

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

[commit-bot: I haz the power, 2017-05-11 08:13:50]

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