Store border-*-style on SurroundData in ComputedStyle

Store border-*-style on SurroundData in ComputedStyle

This patch moves the storage of border-*-style away from BorderStyle.
We instead store them on the generated SurroundData group on
ComputedStyle instead.

We generate a BorderValue to pass to Layout in
ComputedStyle from all the border information stored
there.

Diff: https://gist.github.com/nainar/fd091fc658aebc3f5676f16e545e975a/revisions

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

[nainar, 2017-05-11 12:11:49]

Description was changed from

==========
Store border-*-style on SurroundData in ComputedStyle

This patch moves the storage of border-*-style away from BorderStyle.
We instead store them on the generated SurroundData group on
ComputedStyle instead.

This patch also generates the enum EBOrderStyle and changes.

Diff:


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

to

==========
Store border-*-style on SurroundData in ComputedStyle

This patch moves the storage of border-*-style away from BorderStyle.
We instead store them on the generated SurroundData group on
ComputedStyle instead.

This patch also generates the enum EBOrderStyle and changes.

Diff:


BUG=628043
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2
==========

[nainar, 2017-05-16 04:44:26]

Description was changed from

==========
Store border-*-style on SurroundData in ComputedStyle

This patch moves the storage of border-*-style away from BorderStyle.
We instead store them on the generated SurroundData group on
ComputedStyle instead.

This patch also generates the enum EBOrderStyle and changes.

Diff:


BUG=628043
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2
==========

to

==========
Store border-*-style on SurroundData in ComputedStyle

This patch moves the storage of border-*-style away from BorderStyle.
We instead store them on the generated SurroundData group on
ComputedStyle instead.

We generate a BorderValue to pass to Layout in
ComputedStyle from all the border information stored
there.

This patch also generates the enum EBOrderStyle and changes.

Diff:


BUG=628043
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2
==========

[nainar, 2017-05-16 04:45:06]

Description was changed from

==========
Store border-*-style on SurroundData in ComputedStyle

This patch moves the storage of border-*-style away from BorderStyle.
We instead store them on the generated SurroundData group on
ComputedStyle instead.

We generate a BorderValue to pass to Layout in
ComputedStyle from all the border information stored
there.

This patch also generates the enum EBOrderStyle and changes.

Diff:


BUG=628043
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2
==========

to

==========
Store border-*-style on SurroundData in ComputedStyle

This patch moves the storage of border-*-style away from BorderStyle.
We instead store them on the generated SurroundData group on
ComputedStyle instead.

We generate a BorderValue to pass to Layout in
ComputedStyle from all the border information stored
there.

This patch also generates the enum EBOrderStyle and changes.

Diff: https://gist.github.com/nainar/bfad9ac65b58dc59dc9871189b63eba8/revisions

BUG=628043
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2
==========

[nainar, 2017-05-16 04:46:02]

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

[nainar, 2017-05-16 04:46:03]

Can you take a first pass please? 
Thank you!

Sorry for the ginormous CL.

[nainar, 2017-05-16 04:47:30]

Description was changed from

==========
Store border-*-style on SurroundData in ComputedStyle

This patch moves the storage of border-*-style away from BorderStyle.
We instead store them on the generated SurroundData group on
ComputedStyle instead.

We generate a BorderValue to pass to Layout in
ComputedStyle from all the border information stored
there.

This patch also generates the enum EBOrderStyle and changes.

Diff: https://gist.github.com/nainar/bfad9ac65b58dc59dc9871189b63eba8/revisions

BUG=628043
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2
==========

to

==========
Store border-*-style on SurroundData in ComputedStyle

This patch moves the storage of border-*-style away from BorderStyle.
We instead store them on the generated SurroundData group on
ComputedStyle instead.

We generate a BorderValue to pass to Layout in
ComputedStyle from all the border information stored
there.

This patch also generates the enum EBOrderStyle and changes.

Diff: https://gist.github.com/nainar/bfad9ac65b58dc59dc9871189b63eba8/revisions

BUG=628043
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_layout_ng;master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2
==========

[nainar, 2017-05-16 04:47:35]

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

[commit-bot: I haz the power, 2017-05-16 04:48:13]

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-16 04:53:22]

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

[commit-bot: I haz the power, 2017-05-16 04:53:22]

Dry run: Try jobs failed on following builders:
  android_clang_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_clan...)
  cast_shell_android on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/cast_shell_a...)

[nainar, 2017-05-16 06:30:54]

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

[commit-bot: I haz the power, 2017-05-16 06:31:06]

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-16 06:34:43]

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

[commit-bot: I haz the power, 2017-05-16 06:34:44]

Dry run: Try jobs failed on following builders:
  android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)

[shend, 2017-05-17 06:49:51]

Sorry but could you split this into two patches:
1. Make BorderStyle an enum class.
2. The rest of this patch.

It would make it much easier to review :)

https://codereview.chromium.org/2880573002/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/layout/LayoutTable.cpp (right):

https://codereview.chromium.org/2880573002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/LayoutTable.cpp:1195: if
(table_start_border.Style() > EBorderStyle::kHidden)
I would change this to OutlineStyle() != EBorderStyle::kNone && OutlineValue()
!= EBorderStyle::kHidden instead of using >. Since this logic is repeated a lot,
maybe a helper function?

[nainar, 2017-05-17 08:35:58]

Description was changed from

==========
Store border-*-style on SurroundData in ComputedStyle

This patch moves the storage of border-*-style away from BorderStyle.
We instead store them on the generated SurroundData group on
ComputedStyle instead.

We generate a BorderValue to pass to Layout in
ComputedStyle from all the border information stored
there.

This patch also generates the enum EBOrderStyle and changes.

Diff: https://gist.github.com/nainar/bfad9ac65b58dc59dc9871189b63eba8/revisions

BUG=628043
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_layout_ng;master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2
==========

to

==========
Store border-*-style on SurroundData in ComputedStyle

This patch moves the storage of border-*-style away from BorderStyle.
We instead store them on the generated SurroundData group on
ComputedStyle instead.

We generate a BorderValue to pass to Layout in
ComputedStyle from all the border information stored
there.

Diff: https://gist.github.com/nainar/bfad9ac65b58dc59dc9871189b63eba8/revisions

BUG=628043
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_layout_ng;master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2
==========

[nainar, 2017-05-17 08:45:24]

Description was changed from

==========
Store border-*-style on SurroundData in ComputedStyle

This patch moves the storage of border-*-style away from BorderStyle.
We instead store them on the generated SurroundData group on
ComputedStyle instead.

We generate a BorderValue to pass to Layout in
ComputedStyle from all the border information stored
there.

Diff: https://gist.github.com/nainar/bfad9ac65b58dc59dc9871189b63eba8/revisions

BUG=628043
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_layout_ng;master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2
==========

to

==========
Store border-*-style on SurroundData in ComputedStyle

This patch moves the storage of border-*-style away from BorderStyle.
We instead store them on the generated SurroundData group on
ComputedStyle instead.

We generate a BorderValue to pass to Layout in
ComputedStyle from all the border information stored
there.

Diff: https://gist.github.com/nainar/fd091fc658aebc3f5676f16e545e975a/revisions

BUG=628043
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_layout_ng;master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2
==========

[nainar, 2017-05-17 08:49:08]

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

[nainar, 2017-05-17 08:49:20]

shend@, 

Split the CLs. PTAL?

[commit-bot: I haz the power, 2017-05-17 08:49:42]

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-17 09:05:48]

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

[commit-bot: I haz the power, 2017-05-17 09:05:49]

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...)
  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-05-17 09:26:44]

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

[commit-bot: I haz the power, 2017-05-17 09:27:20]

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-17 09:52:09]

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

[commit-bot: I haz the power, 2017-05-17 09:52:10]

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-05-17 22:18:34]

https://codereview.chromium.org/2880573002/diff/100001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/style/BorderValue.h (left):

https://codereview.chromium.org/2880573002/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/style/BorderValue.h:52: SetIsAuto(data.IsAuto());
Was this deletion intentional?

https://codereview.chromium.org/2880573002/diff/100001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/style/ComputedStyle.h (right):

https://codereview.chromium.org/2880573002/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/style/ComputedStyle.h:2896: if
(BorderRightStyle() == EBorderStyle::kNone &&
For the future, I think it might be possible to combine these two if statements
so that there's only three boolean expressions versus four:

if (BorderRightStyle() == o.BorderRightStyle() && (BorderRightStyle() ==
EBorderStyle::kNone || BorderRightStyle() == EBorderStyle::kHidden))

Not sure if it would be faster though.

https://codereview.chromium.org/2880573002/diff/100001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/style/ComputedStyleConstants.h (left):

https://codereview.chromium.org/2880573002/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/style/ComputedStyleConstants.h:101: enum class
EBorderStyle {
When you generate EBorderStyle, the generator also generates the CSSValueID
mappings:
https://gist.github.com/nainar/fd091fc658aebc3f5676f16e545e975a/revisions#dif...

You can delete the handwritten mappings in CSSPrimitiveValueMappings:
https://cs.chromium.org/chromium/src/third_party/WebKit/Source/core/css/CSSPr...

See this CL for an example: https://codereview.chromium.org/2853433003

[nainar, 2017-05-18 01:58:14]

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

[nainar, 2017-05-18 01:58:35]

shend@, 

Made the suggestions you asked for, PTAL? Thanks!

https://codereview.chromium.org/2880573002/diff/100001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/style/BorderValue.h (left):

https://codereview.chromium.org/2880573002/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/style/BorderValue.h:52: SetIsAuto(data.IsAuto());
Nope it wasn't. Added it back in.

https://codereview.chromium.org/2880573002/diff/100001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/style/ComputedStyle.h (right):

https://codereview.chromium.org/2880573002/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/style/ComputedStyle.h:2896: if
(BorderRightStyle() == EBorderStyle::kNone &&
On 2017/05/17 at 22:18:34, shend wrote:
> For the future, I think it might be possible to combine these two if
statements so that there's only three boolean expressions versus four:
> 
> if (BorderRightStyle() == o.BorderRightStyle() && (BorderRightStyle() ==
EBorderStyle::kNone || BorderRightStyle() == EBorderStyle::kHidden))
> 
> Not sure if it would be faster though.

Made note of this for a future CL. Yeah can't determine offhand what is better.

https://codereview.chromium.org/2880573002/diff/100001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/style/ComputedStyleConstants.h (left):

https://codereview.chromium.org/2880573002/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/style/ComputedStyleConstants.h:101: enum class
EBorderStyle {
Yup took out the relevant two functions.

[commit-bot: I haz the power, 2017-05-18 01:59:30]

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-18 02:26:49]

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

[commit-bot: I haz the power, 2017-05-18 02:26: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-05-18 02:59:28]

lgtm

[nainar, 2017-05-18 03:07:21]

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

[nainar, 2017-05-18 03:07:21]

meade@, 

PTAL? Thanks!

[meade_UTC10, 2017-05-18 03:32:08]

lgtm

[nainar, 2017-05-18 04:17:50]

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

[commit-bot: I haz the power, 2017-05-18 04:19:35]

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-18 05:01:19]

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

[commit-bot: I haz the power, 2017-05-18 05:01:20]

Dry run: Try jobs failed on following builders:
  linux_layout_tests_slimming_paint_v2 on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_layout_te...)

[nainar, 2017-05-18 08:29:00]

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

[commit-bot: I haz the power, 2017-05-18 08:29:20]

Dry run: CQ is trying da patch.

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

[nainar, 2017-05-18 09:57:35]

The CQ bit was unchecked by nainar@chromium.org

[nainar, 2017-05-18 09:57:38]

The CQ bit was checked by nainar@chromium.org

[nainar, 2017-05-18 09:57:38]

The patchset sent to the CQ was uploaded after l-g-t-m from meade@chromium.org,
shend@chromium.org
Link to the patchset: https://codereview.chromium.org/2880573002/#ps140001
(title: "Merge branch 'enumClass' into borderStyle")

[commit-bot: I haz the power, 2017-05-18 09:58:01]

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-18 11:42:45]

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

[commit-bot: I haz the power, 2017-05-18 11:42:47]

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

[nainar, 2017-05-19 00:44:46]

Description was changed from

==========
Store border-*-style on SurroundData in ComputedStyle

This patch moves the storage of border-*-style away from BorderStyle.
We instead store them on the generated SurroundData group on
ComputedStyle instead.

We generate a BorderValue to pass to Layout in
ComputedStyle from all the border information stored
there.

Diff: https://gist.github.com/nainar/fd091fc658aebc3f5676f16e545e975a/revisions

BUG=628043
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_layout_ng;master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2
==========

to

==========
Store border-*-style on SurroundData in ComputedStyle

This patch moves the storage of border-*-style away from BorderStyle.
We instead store them on the generated SurroundData group on
ComputedStyle instead.

We generate a BorderValue to pass to Layout in
ComputedStyle from all the border information stored
there.

Diff: https://gist.github.com/nainar/fd091fc658aebc3f5676f16e545e975a/revisions

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

[nainar, 2017-05-19 00:44:51]

The CQ bit was checked by nainar@chromium.org

[commit-bot: I haz the power, 2017-05-19 00:46:35]

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-19 00:53:04]

CQ is committing da patch.

Bot data: {"patchset_id": 140001, "attempt_start_ts": 1495154691192820,
"parent_rev": "c20d3c0cb24f5e90f9c600c3b43c5eabb85d0ea4", "commit_rev":
"b72ebe3be808b64ca15ebff036511450b4a8fcff"}

[commit-bot: I haz the power, 2017-05-19 00:53:22]

Description was changed from

==========
Store border-*-style on SurroundData in ComputedStyle

This patch moves the storage of border-*-style away from BorderStyle.
We instead store them on the generated SurroundData group on
ComputedStyle instead.

We generate a BorderValue to pass to Layout in
ComputedStyle from all the border information stored
there.

Diff: https://gist.github.com/nainar/fd091fc658aebc3f5676f16e545e975a/revisions

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

to

==========
Store border-*-style on SurroundData in ComputedStyle

This patch moves the storage of border-*-style away from BorderStyle.
We instead store them on the generated SurroundData group on
ComputedStyle instead.

We generate a BorderValue to pass to Layout in
ComputedStyle from all the border information stored
there.

Diff: https://gist.github.com/nainar/fd091fc658aebc3f5676f16e545e975a/revisions

BUG=628043

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

[commit-bot: I haz the power, 2017-05-19 00:53:26]

Committed patchset #8 (id:140001) as
https://chromium.googlesource.com/chromium/src/+/b72ebe3be808b64ca15ebff03651...