Store border-*-color on SurroundData in ComputedStyle not BorderColorAndStyle

Store border-*-color on SurroundData in ComputedStyle not BorderColorAndStyle

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

Since BorderValue is used by other classes such as OutlineValue and
MultiColData we have changed BorderData to store the
border-*-style information on BorderStyle which is the
BorderValue class without the border-*-(width/color) information.

The long term intention is to move all information away from
BorderStyle - it is a temporary class to store information as
we generate border-*-* iteratively.

Please note that we convert BorderStyle to a BorderValue when we
send information over to the Layout system to make the interfacing easy.
This will be changed to creating a new BorderValue from the individual
values in the long run or something along those lines
(pending discussion.)

Diff: https://gist.github.com/cf8735baa30e27a2dc945b6485c85a74/revisions


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

[nainar, 2017-05-09 06:37:24]

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

[commit-bot: I haz the power, 2017-05-09 06:37:51]

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-09 07:01:08]

The CQ bit was unchecked by nainar@chromium.org

[shend, 2017-05-09 07:24:40]

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

[shend, 2017-05-09 07:24:41]

https://codereview.chromium.org/2869043002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/style/CachedUAStyle.h (right):

https://codereview.chromium.org/2869043002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/style/CachedUAStyle.h:53: Color
border_left_color;
Probably won't make a difference, but we can rearrange these in order of
alignment size: LengthSize, float, Color, unsigned bitfields.

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

https://codereview.chromium.org/2869043002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/style/ComputedStyle.h:581:
SET_BORDERVALUE_COLOR(surround_data_, border_.bottom_, v);
Can we get rid of this macro now?

[nainar, 2017-05-09 07:38:44]

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

[commit-bot: I haz the power, 2017-05-09 07:39:08]

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-09 07:40:39]

shend@, 

PTAL? Thanks!

https://codereview.chromium.org/2869043002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/style/CachedUAStyle.h (right):

https://codereview.chromium.org/2869043002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/style/CachedUAStyle.h:53: Color
border_left_color;
On 2017/05/09 at 07:24:41, shend wrote:
> Probably won't make a difference, but we can rearrange these in order of
alignment size: LengthSize, float, Color, unsigned bitfields.

Will do this in a separate patch

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

https://codereview.chromium.org/2869043002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/style/ComputedStyle.h:581:
SET_BORDERVALUE_COLOR(surround_data_, border_.bottom_, v);
On 2017/05/09 at 07:24:41, shend wrote:
> Can we get rid of this macro now?

Nope. Needed by BorderValue.

[shend, 2017-05-09 07:54:43]

lgtm

[commit-bot: I haz the power, 2017-05-09 08:22:02]

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

[commit-bot: I haz the power, 2017-05-09 08:22:02]

Dry run: Try jobs failed on following builders:
  win_chromium_compile_dbg_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_comp...)

[nainar, 2017-05-09 08:57:42]

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

[commit-bot: I haz the power, 2017-05-09 08:58:14]

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-09 09:47:40]

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

[commit-bot: I haz the power, 2017-05-09 09:47:41]

Dry run: Try jobs failed on following builders:
  win_chromium_compile_dbg_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_comp...)

[nainar, 2017-05-10 09:12:48]

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

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

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 09:51:21]

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

[commit-bot: I haz the power, 2017-05-10 09:51:22]

Dry run: Try jobs failed on following builders:
  win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)

[shend, 2017-05-11 00:41:13]

A few things

https://codereview.chromium.org/2869043002/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/style/BorderValue.h (right):

https://codereview.chromium.org/2869043002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/style/BorderValue.h:49: BorderValue(const
BorderStyle& data, const StyleColor& color, float width) {
Previously, BorderValue took a BorderColorAndStyle, which used Color. Now we
take a StyleColor and resolve it.

https://codereview.chromium.org/2869043002/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/style/ComputedStyle.cpp (right):

https://codereview.chromium.org/2869043002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/style/ComputedStyle.cpp:889:
!BorderColorEquals(other) || !BorderSizeEquals(other) ||
Is this necessary? Originally, we only compared Border with VisualEqual,
RadiiEqual and SizeEqual.

https://codereview.chromium.org/2869043002/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/style/ComputedStyle.h (right):

https://codereview.chromium.org/2869043002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/style/ComputedStyle.h:2853:
VisitedDependentColor(CSSPropertyBorderLeftColor),
Could this be just BorderLeftColorInternal()?

[nainar, 2017-05-11 01:04:51]

https://codereview.chromium.org/2869043002/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/style/BorderValue.h (right):

https://codereview.chromium.org/2869043002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/style/BorderValue.h:49: BorderValue(const
BorderStyle& data, const StyleColor& color, float width) {
Yup.

https://codereview.chromium.org/2869043002/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/style/ComputedStyle.cpp (right):

https://codereview.chromium.org/2869043002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/style/ComputedStyle.cpp:889:
!BorderColorEquals(other) || !BorderSizeEquals(other) ||
On 2017/05/11 at 00:41:13, shend wrote:
> Is this necessary? Originally, we only compared Border with VisualEqual,
RadiiEqual and SizeEqual.

This is needed because VisuallyEqual used the BorderValue::operator== which
checks color and color_is_current_color.

https://codereview.chromium.org/2869043002/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/style/ComputedStyle.h (right):

https://codereview.chromium.org/2869043002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/style/ComputedStyle.h:2853:
VisitedDependentColor(CSSPropertyBorderLeftColor),
On 2017/05/11 at 00:41:13, shend wrote:
> Could this be just BorderLeftColorInternal()?

It's the same

[nainar, 2017-05-11 01:07:50]

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

[nainar, 2017-05-11 01:07:50]

Hi meade@, 

shend@ and I have been staring at this for ages and clearly there are failing
tests. We can't seem to catch why. Can you take a look? A fresh set of eyes
might help.

[nainar, 2017-05-11 02:15:08]

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

[commit-bot: I haz the power, 2017-05-11 02:16: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-05-11 03:38:14]

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

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

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

[meade_UTC10, 2017-05-11 03:39:24]

Hmmmmmmm that's tricky! I didn't see anything that looked like it would cause
extra style invalidation, unless it so happens that the TODO I commented on is
indirectly causing it?

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

https://codereview.chromium.org/2869043002/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/style/ComputedStyle.h:2946: // todo - reset
border colors here
Is this still meant to be here?

[nainar, 2017-05-11 04:24:12]

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

[commit-bot: I haz the power, 2017-05-11 04:25:49]

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-11 04:33:22]

removed the todo - tests still failing.

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

https://codereview.chromium.org/2869043002/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/style/ComputedStyle.h:2852: return
BorderValue(surround_data_->border_.Left(),
Changed these to use the public getters instead.

https://codereview.chromium.org/2869043002/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/style/ComputedStyle.h:2946: // todo - reset
border colors here
On 2017/05/11 at 03:39:24, meade_UTC10 wrote:
> Is this still meant to be here?

Nope. This is something that should have been removed.

[commit-bot: I haz the power, 2017-05-11 06:45:22]

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

[commit-bot: I haz the power, 2017-05-11 06:45:22]

Dry run: Try jobs failed on following builders:
  win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

[nainar, 2017-05-11 07:01:07]

Description was changed from

==========
Store border-*-color on SurroundData in ComputedStyle not BorderColorAndStyle

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

Since BorderValue is used by other classes such as OutlineValue and
MultiColData we have changed BorderData to store the
border-*-style information on BorderStyle which is the
BorderValue class without the border-*-(width/color) information.

The long term intention is to move all information away from
BorderStyle - it is a temporary class to store information as
we generate border-*-* iteratively.

Please note that we convert BorderStyle to a BorderValue when we
send information over to the Layout system to make the interfacing easy.
This will be changed to creating a new BorderValue from the individual
values in the long run or something along those lines
(pending discussion.)

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

to

==========
Store border-*-color on SurroundData in ComputedStyle not BorderColorAndStyle

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

Since BorderValue is used by other classes such as OutlineValue and
MultiColData we have changed BorderData to store the
border-*-style information on BorderStyle which is the
BorderValue class without the border-*-(width/color) information.

The long term intention is to move all information away from
BorderStyle - it is a temporary class to store information as
we generate border-*-* iteratively.

Please note that we convert BorderStyle to a BorderValue when we
send information over to the Layout system to make the interfacing easy.
This will be changed to creating a new BorderValue from the individual
values in the long run or something along those lines
(pending discussion.)

Diff: https://gist.github.com/cf8735baa30e27a2dc945b6485c85a74/revisions


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

[nainar, 2017-05-11 08:17:52]

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

[nainar, 2017-05-11 08:18:19]

meade@ for OWNERS. Only change is BorderColorEquals()

[commit-bot: I haz the power, 2017-05-11 08:18:28]

Dry run: CQ is trying da patch.

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

[meade_UTC10, 2017-05-11 08:20:07]

lgtm

Nice job finding the issue!

[nainar, 2017-05-11 08:24:51]

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

[commit-bot: I haz the power, 2017-05-11 08:25: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-11 10:32:46]

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

[commit-bot: I haz the power, 2017-05-11 10:32:47]

Dry run: This issue passed the CQ dry run.

[nainar, 2017-05-11 10:38:01]

The CQ bit was checked by nainar@chromium.org

[nainar, 2017-05-11 10:38:01]

The patchset sent to the CQ was uploaded after l-g-t-m from shend@chromium.org,
meade@chromium.org
Link to the patchset: https://codereview.chromium.org/2869043002/#ps160001
(title: "BorderColorVisuallyEquals calls BorderColorEquals")

[commit-bot: I haz the power, 2017-05-11 10:38:29]

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 10:44:16]

CQ is committing da patch.

Bot data: {"patchset_id": 160001, "attempt_start_ts": 1494499081286220,
"parent_rev": "8582d8e8c2d42a9ae105280b77f7f8a34979075c", "commit_rev":
"7201468ffddd08c6893769c4a0616df7e9bedcad"}

[commit-bot: I haz the power, 2017-05-11 10:44:26]

Description was changed from

==========
Store border-*-color on SurroundData in ComputedStyle not BorderColorAndStyle

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

Since BorderValue is used by other classes such as OutlineValue and
MultiColData we have changed BorderData to store the
border-*-style information on BorderStyle which is the
BorderValue class without the border-*-(width/color) information.

The long term intention is to move all information away from
BorderStyle - it is a temporary class to store information as
we generate border-*-* iteratively.

Please note that we convert BorderStyle to a BorderValue when we
send information over to the Layout system to make the interfacing easy.
This will be changed to creating a new BorderValue from the individual
values in the long run or something along those lines
(pending discussion.)

Diff: https://gist.github.com/cf8735baa30e27a2dc945b6485c85a74/revisions


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

to

==========
Store border-*-color on SurroundData in ComputedStyle not BorderColorAndStyle

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

Since BorderValue is used by other classes such as OutlineValue and
MultiColData we have changed BorderData to store the
border-*-style information on BorderStyle which is the
BorderValue class without the border-*-(width/color) information.

The long term intention is to move all information away from
BorderStyle - it is a temporary class to store information as
we generate border-*-* iteratively.

Please note that we convert BorderStyle to a BorderValue when we
send information over to the Layout system to make the interfacing easy.
This will be changed to creating a new BorderValue from the individual
values in the long run or something along those lines
(pending discussion.)

Diff: https://gist.github.com/cf8735baa30e27a2dc945b6485c85a74/revisions


BUG=628043

Review-Url: https://codereview.chromium.org/2869043002
Cr-Commit-Position: refs/heads/master@{#470907}
Committed:
https://chromium.googlesource.com/chromium/src/+/7201468ffddd08c6893769c4a061...
==========

[commit-bot: I haz the power, 2017-05-11 10:44:27]

Committed patchset #9 (id:160001) as
https://chromium.googlesource.com/chromium/src/+/7201468ffddd08c6893769c4a061...