Split StyleSurroundData::padding into four individual Lengths.

Split StyleSurroundData::padding into four individual Lengths.

In StyleSurroundData, the padding longhand properties are stored in a
single LengthBox. This makes it difficult to generate because the
generator assumes that properties don't share storage.

This patch splits the LengthBox padding into four Lengths:
padding_{left/right/top/bottom}. This enables us to generate
the padding properties properly.

This patch also adds PaddingEqual helper functions to check if two
margins have the same value and changes the callsites to
ComputedStyle::Padding() to cater for the fact that the LengthBox has
been broken into its individual Length components.

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

[nainar, 2017-04-20 06:55:39]

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

[commit-bot: I haz the power, 2017-04-20 06:55:59]

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

[nainar, 2017-04-20 06:59:41]

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

[nainar, 2017-04-20 06:59:41]

Hi Darren!

PTAL? Thanks!

[shend, 2017-04-20 07:07:20]

lgtm with nits.

https://codereview.chromium.org/2834603003/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/style/ComputedStyle.h (right):

https://codereview.chromium.org/2834603003/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/style/ComputedStyle.h:2917: return PaddingTop()
== other.PaddingTop() &&
nit: could be: PaddingEqual(other.Padding());

https://codereview.chromium.org/2834603003/diff/1/third_party/WebKit/Source/p...
File third_party/WebKit/Source/platform/Theme.cpp (right):

https://codereview.chromium.org/2834603003/diff/1/third_party/WebKit/Source/p...
third_party/WebKit/Source/platform/Theme.cpp:60: return
LengthBox(zoomed_box_top, zoomed_box_right, zoomed_box_bottom,
Does this get called often? If so, there might be a cost of copy and
constructing the LengthBox.

[nainar, 2017-04-20 07:13:09]

Followup comments.

https://codereview.chromium.org/2834603003/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/style/ComputedStyle.h (right):

https://codereview.chromium.org/2834603003/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/style/ComputedStyle.h:2917: return PaddingTop()
== other.PaddingTop() &&
On 2017/04/20 at 07:07:20, shend wrote:
> nit: could be: PaddingEqual(other.Padding());

How about I delete this and pass other.Padding() directly at the callsites?

https://codereview.chromium.org/2834603003/diff/1/third_party/WebKit/Source/p...
File third_party/WebKit/Source/platform/Theme.cpp (right):

https://codereview.chromium.org/2834603003/diff/1/third_party/WebKit/Source/p...
third_party/WebKit/Source/platform/Theme.cpp:60: return
LengthBox(zoomed_box_top, zoomed_box_right, zoomed_box_bottom,
On 2017/04/20 at 07:07:20, shend wrote:
> Does this get called often? If so, there might be a cost of copy and
constructing the LengthBox.

Yes, it gets called up through StyleResolver::StyleForElement()

[commit-bot: I haz the power, 2017-04-20 07:17:49]

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

[commit-bot: I haz the power, 2017-04-20 07:17:50]

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-20 07:20:57]

https://codereview.chromium.org/2834603003/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/style/ComputedStyle.h (right):

https://codereview.chromium.org/2834603003/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/style/ComputedStyle.h:2917: return PaddingTop()
== other.PaddingTop() &&
On 2017/04/20 at 07:13:09, nainar wrote:
> On 2017/04/20 at 07:07:20, shend wrote:
> > nit: could be: PaddingEqual(other.Padding());
> 
> How about I delete this and pass other.Padding() directly at the callsites?

If you want.

https://codereview.chromium.org/2834603003/diff/1/third_party/WebKit/Source/p...
File third_party/WebKit/Source/platform/Theme.cpp (right):

https://codereview.chromium.org/2834603003/diff/1/third_party/WebKit/Source/p...
third_party/WebKit/Source/platform/Theme.cpp:60: return
LengthBox(zoomed_box_top, zoomed_box_right, zoomed_box_bottom,
On 2017/04/20 at 07:13:09, nainar wrote:
> On 2017/04/20 at 07:07:20, shend wrote:
> > Does this get called often? If so, there might be a cost of copy and
constructing the LengthBox.
> 
> Yes, it gets called up through StyleResolver::StyleForElement()

That's annoying. Maybe ask alancutter@ for advice on how to prevent the cost of
constructing a LengthBox?

[nainar, 2017-04-21 00:15:18]

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

[nainar, 2017-04-21 00:15:19]

Alan could you take a look? Specifically at the point Darren raised about the
copy on construct cost in LayoutTheme.cpp? 

Thank you!

https://codereview.chromium.org/2834603003/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/style/ComputedStyle.h (right):

https://codereview.chromium.org/2834603003/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/style/ComputedStyle.h:2917: return PaddingTop()
== other.PaddingTop() &&
On 2017/04/20 at 07:20:56, shend wrote:
> On 2017/04/20 at 07:13:09, nainar wrote:
> > On 2017/04/20 at 07:07:20, shend wrote:
> > > nit: could be: PaddingEqual(other.Padding());
> > 
> > How about I delete this and pass other.Padding() directly at the callsites?
> 
> If you want.

Retaining as is because both the option shend@ and I mentioned require the copy
on construct cost.

https://codereview.chromium.org/2834603003/diff/1/third_party/WebKit/Source/p...
File third_party/WebKit/Source/platform/Theme.cpp (right):

https://codereview.chromium.org/2834603003/diff/1/third_party/WebKit/Source/p...
third_party/WebKit/Source/platform/Theme.cpp:60: return
LengthBox(zoomed_box_top, zoomed_box_right, zoomed_box_bottom,
On 2017/04/20 at 07:20:56, shend wrote:
> On 2017/04/20 at 07:13:09, nainar wrote:
> > On 2017/04/20 at 07:07:20, shend wrote:
> > > Does this get called often? If so, there might be a cost of copy and
constructing the LengthBox.
> > 
> > Yes, it gets called up through StyleResolver::StyleForElement()
> 
> That's annoying. Maybe ask alancutter@ for advice on how to prevent the cost
of constructing a LengthBox?

Adding him for owners and asking his opinion on the matter.

[alancutter (OOO until 2018), 2017-04-21 03:13:32]

lgtm

https://codereview.chromium.org/2834603003/diff/1/third_party/WebKit/Source/p...
File third_party/WebKit/Source/platform/Theme.cpp (right):

https://codereview.chromium.org/2834603003/diff/1/third_party/WebKit/Source/p...
third_party/WebKit/Source/platform/Theme.cpp:60: return
LengthBox(zoomed_box_top, zoomed_box_right, zoomed_box_bottom,
On 2017/04/21 at 00:15:19, nainar wrote:
> On 2017/04/20 at 07:20:56, shend wrote:
> > On 2017/04/20 at 07:13:09, nainar wrote:
> > > On 2017/04/20 at 07:07:20, shend wrote:
> > > > Does this get called often? If so, there might be a cost of copy and
constructing the LengthBox.
> > > 
> > > Yes, it gets called up through StyleResolver::StyleForElement()
> > 
> > That's annoying. Maybe ask alancutter@ for advice on how to prevent the cost
of constructing a LengthBox?
> 
> Adding him for owners and asking his opinion on the matter.

It was doing a copy anyway, I wouldn't worry about it.

[nainar, 2017-04-21 03:16:17]

The CQ bit was checked by nainar@chromium.org

[commit-bot: I haz the power, 2017-04-21 03:16:27]

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-21 03:24:48]

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

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

Try jobs failed on following builders:
  chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[nainar, 2017-04-21 03:28:11]

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

[nainar, 2017-04-21 03:28:11]

haraken@ for OWNERS on platform/

[haraken, 2017-04-21 06:33:55]

On 2017/04/21 03:28:11, nainar wrote:
> haraken@ for OWNERS on platform/

LGTM

[nainar, 2017-04-21 06:34:21]

The CQ bit was checked by nainar@chromium.org

[commit-bot: I haz the power, 2017-04-21 06:34:42]

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-21 06:57:49]

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

[commit-bot: I haz the power, 2017-04-21 06:57:50]

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

[nainar, 2017-04-21 07:11:02]

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

[commit-bot: I haz the power, 2017-04-21 07:11:16]

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

[nainar, 2017-04-21 07:11:58]

The CQ bit was unchecked by nainar@chromium.org

[nainar, 2017-04-21 07:12:09]

The CQ bit was checked by nainar@chromium.org

[nainar, 2017-04-21 07:12:09]

The patchset sent to the CQ was uploaded after l-g-t-m from
haraken@chromium.org, alancutter@chromium.org, shend@chromium.org
Link to the patchset: https://codereview.chromium.org/2834603003/#ps20001
(title: "Change ThemeMac")

[commit-bot: I haz the power, 2017-04-21 07:12:25]

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-21 07:41:34]

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

[commit-bot: I haz the power, 2017-04-21 07:41:36]

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

[nainar, 2017-04-21 08:03:15]

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

[commit-bot: I haz the power, 2017-04-21 08:03:22]

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-21 08:21:50]

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

[commit-bot: I haz the power, 2017-04-21 08:21:51]

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

[nainar, 2017-04-23 23:15:50]

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

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

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-24 00:59:59]

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

[commit-bot: I haz the power, 2017-04-24 01:00:00]

Dry run: This issue passed the CQ dry run.

[nainar, 2017-04-24 02:54:44]

The CQ bit was checked by nainar@chromium.org

[nainar, 2017-04-24 02:54:45]

The patchset sent to the CQ was uploaded after l-g-t-m from
haraken@chromium.org, alancutter@chromium.org, shend@chromium.org
Link to the patchset: https://codereview.chromium.org/2834603003/#ps60001
(title: "Split StyleSurroundData::padding into four individual Lengths.")

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

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-24 02:58:26]

CQ is committing da patch.

Bot data: {"patchset_id": 60001, "attempt_start_ts": 1493002484952330,
"parent_rev": "3df03ef75837bd7664e17fa0daab5ea2f0bef0da", "commit_rev":
"7d5e129a2deae3313aa912575ff90faba8583b61"}

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

Description was changed from

==========
Split StyleSurroundData::padding into four individual Lengths.

In StyleSurroundData, the padding longhand properties are stored in a
single LengthBox. This makes it difficult to generate because the
generator assumes that properties don't share storage.

This patch splits the LengthBox padding into four Lengths:
padding_{left/right/top/bottom}. This enables us to generate
the padding properties properly.

This patch also adds PaddingEqual helper functions to check if two
margins have the same value and changes the callsites to
ComputedStyle::Padding() to cater for the fact that the LengthBox has
been broken into its individual Length components.

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

to

==========
Split StyleSurroundData::padding into four individual Lengths.

In StyleSurroundData, the padding longhand properties are stored in a
single LengthBox. This makes it difficult to generate because the
generator assumes that properties don't share storage.

This patch splits the LengthBox padding into four Lengths:
padding_{left/right/top/bottom}. This enables us to generate
the padding properties properly.

This patch also adds PaddingEqual helper functions to check if two
margins have the same value and changes the callsites to
ComputedStyle::Padding() to cater for the fact that the LengthBox has
been broken into its individual Length components.

BUG=628043

Review-Url: https://codereview.chromium.org/2834603003
Cr-Commit-Position: refs/heads/master@{#466582}
Committed:
https://chromium.googlesource.com/chromium/src/+/7d5e129a2deae3313aa912575ff9...
==========

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

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