Split EBreak enum into EBreakBetween & EBreakInside

Split EBreak enum into EBreakBetween & EBreakInside

This change splits EBreak into two enums that are ready for automatic
generation.

Before this change:
* The EBreak enum includes all values applicable to break-before &
  break-after properties.
* A subset of these values are applicable to break-inside & this
  subset is enforced with some asserts.

After this change:
* EBreakBetween includes all values applicable to break-before &
  break-after.
* EBreakInside includes values applicable to break-inside.

Client code working with EBreak has been made specific to the
appropriate new enum. I burrowed upwards through the call chain using
code search to find out / verify whether each client was using EBreak to
store EBreakBetween-like values (most often), EBreakInside-like values
(sometimes), or both (rarely).

I chose the name "EBreakBetween" because the main clients in /layout use
"Between" for the same thing.

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

[amoylan, 2017-02-07 04:20:56]

Description was changed from

==========
Split EBreak enum into EBreakBetween & EBreakInside

This change splits EBreak into two enums that are ready for automatic
generation.

Before this change:
* The EBreak enum includes all values applicable to break-before &
  break-after properties.
* A subset of these values are applicable to break-inline & this
  subset is enforced with some asserts.

After this change:
* EBreakBetween includes all values applicable to break-before &
  break-after.
* EBreakInside includes values applicable to break-inline.

Client code working with EBreak has been made specific to the
appropriate new enum. For the most part this was straightforward as very
little client code was making use of the fact that break-before|after &
break-inside were using the same data type.

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

to

==========
Split EBreak enum into EBreakBetween & EBreakInside

This change splits EBreak into two enums that are ready for automatic
generation.

Before this change:
* The EBreak enum includes all values applicable to break-before &
  break-after properties.
* A subset of these values are applicable to break-inline & this
  subset is enforced with some asserts.

After this change:
* EBreakBetween includes all values applicable to break-before &
  break-after.
* EBreakInside includes values applicable to break-inline.

Client code working with EBreak has been made specific to the
appropriate new enum. For the most part this was straightforward: I
burrowed upwards through the call chain using code search to find out /
verify whether each client was using EBreak to store EBreakBetween-like
values (most often), EBreakInside-like values (sometimes), or both
(rarely).

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

[amoylan, 2017-02-07 04:23:47]

Description was changed from

==========
Split EBreak enum into EBreakBetween & EBreakInside

This change splits EBreak into two enums that are ready for automatic
generation.

Before this change:
* The EBreak enum includes all values applicable to break-before &
  break-after properties.
* A subset of these values are applicable to break-inline & this
  subset is enforced with some asserts.

After this change:
* EBreakBetween includes all values applicable to break-before &
  break-after.
* EBreakInside includes values applicable to break-inline.

Client code working with EBreak has been made specific to the
appropriate new enum. For the most part this was straightforward: I
burrowed upwards through the call chain using code search to find out /
verify whether each client was using EBreak to store EBreakBetween-like
values (most often), EBreakInside-like values (sometimes), or both
(rarely).

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

to

==========
Split EBreak enum into EBreakBetween & EBreakInside

This change splits EBreak into two enums that are ready for automatic
generation.

Before this change:
* The EBreak enum includes all values applicable to break-before &
  break-after properties.
* A subset of these values are applicable to break-inline & this
  subset is enforced with some asserts.

After this change:
* EBreakBetween includes all values applicable to break-before &
  break-after.
* EBreakInside includes values applicable to break-inline.

Client code working with EBreak has been made specific to the
appropriate new enum. I burrowed upwards through the call chain using
code search to find out / verify whether each client was using EBreak to
store EBreakBetween-like values (most often), EBreakInside-like values
(sometimes), or both (rarely).

I chose the name "EBreakBetween" because the main clients in /layout use
"Between" for the same thing.

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

[amoylan, 2017-02-07 04:32:36]

Description was changed from

==========
Split EBreak enum into EBreakBetween & EBreakInside

This change splits EBreak into two enums that are ready for automatic
generation.

Before this change:
* The EBreak enum includes all values applicable to break-before &
  break-after properties.
* A subset of these values are applicable to break-inline & this
  subset is enforced with some asserts.

After this change:
* EBreakBetween includes all values applicable to break-before &
  break-after.
* EBreakInside includes values applicable to break-inline.

Client code working with EBreak has been made specific to the
appropriate new enum. I burrowed upwards through the call chain using
code search to find out / verify whether each client was using EBreak to
store EBreakBetween-like values (most often), EBreakInside-like values
(sometimes), or both (rarely).

I chose the name "EBreakBetween" because the main clients in /layout use
"Between" for the same thing.

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

to

==========
Split EBreak enum into EBreakBetween & EBreakInside

This change splits EBreak into two enums that are ready for automatic
generation.

Before this change:
* The EBreak enum includes all values applicable to break-before &
  break-after properties.
* A subset of these values are applicable to break-inline & this
  subset is enforced with some asserts.

After this change:
* EBreakBetween includes all values applicable to break-before &
  break-after.
* EBreakInside includes values applicable to break-inline.

Client code working with EBreak has been made specific to the
appropriate new enum. I burrowed upwards through the call chain using
code search to find out / verify whether each client was using EBreak to
store EBreakBetween-like values (most often), EBreakInside-like values
(sometimes), or both (rarely).

I chose the name "EBreakBetween" because the main clients in /layout use
"Between" for the same thing.

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

[amoylan, 2017-02-07 04:32:36]

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

[amoylan, 2017-02-07 04:35:02]

Hi Alan, this is part 2 of https://codereview.chromium.org/2677033002/ which you
also are reviewing.

Once you are happy with the part 1, could you please take the first look at this
CL?

After that I will seek the right reviewer(s) to add for the /layout/ parts of
this CL.

Thanks!

https://codereview.chromium.org/2682573002/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/css/CSSPrimitiveValueMappings.h (right):

https://codereview.chromium.org/2682573002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/css/CSSPrimitiveValueMappings.h:1954: inline
CSSIdentifierValue::CSSIdentifierValue(EBreakBetween e)
Sorted this switch to match enum ordering. Same below.

https://codereview.chromium.org/2682573002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/layout/LayoutBox.cpp (left):

https://codereview.chromium.org/2682573002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/LayoutBox.cpp:2157: if (breakValue ==
BreakAuto)
This ASSERT is not required now that we have split out the smaller EBreakInside.

https://codereview.chromium.org/2682573002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/layout/LayoutBox.h (right):

https://codereview.chromium.org/2682573002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/LayoutBox.h:1634: inline bool
LayoutBox::isForcedFragmentainerBreakValue(
For "Between" type breaks, this function was used to test whether the break was
a "forced"-type break (i.e. not "avoid" or "auto"). This use is retained.

For "Inside" type breaks, this function was used to assert that a given EBreak
only had "Inside"-type values. This use is no longer required now that
EBreakInside is split out.

[mstensho (USE GERRIT), 2017-02-07 10:21:34]

mstensho@opera.com changed reviewers:
+ mstensho@opera.com

[mstensho (USE GERRIT), 2017-02-07 10:21:35]

Typos in the description:

> * A subset of these values are applicable to break-inline & this
                                                     ^^^^^^
Should be "break-inside", not "break-inline".

> * EBreakInside includes values applicable to break-inline.
                                                     ^^^^^^
Should be "break-inside", not "break-inline".

https://codereview.chromium.org/2682573002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/layout/LayoutBox.cpp (left):

https://codereview.chromium.org/2682573002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/LayoutBox.cpp:2157: if (breakValue ==
BreakAuto)
On 2017/02/07 04:35:02, amoylan wrote:
> This ASSERT is not required now that we have split out the smaller
EBreakInside.

Acknowledged. Good stuff!

https://codereview.chromium.org/2682573002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/layout/LayoutBox.h (right):

https://codereview.chromium.org/2682573002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/LayoutBox.h:1634: inline bool
LayoutBox::isForcedFragmentainerBreakValue(
On 2017/02/07 04:35:02, amoylan wrote:
> For "Between" type breaks, this function was used to test whether the break
was
> a "forced"-type break (i.e. not "avoid" or "auto"). This use is retained.
> 
> For "Inside" type breaks, this function was used to assert that a given EBreak
> only had "Inside"-type values. This use is no longer required now that
> EBreakInside is split out.

Acknowledged.

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

https://codereview.chromium.org/2682573002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/style/ComputedStyle.h:266: unsigned m_breakBefore
: 4;  // EBreak
Need to update these comments.

[mstensho (USE GERRIT), 2017-02-07 10:22:06]

I also meant to say lgtm. :)

[amoylan, 2017-02-07 23:27:29]

Description was changed from

==========
Split EBreak enum into EBreakBetween & EBreakInside

This change splits EBreak into two enums that are ready for automatic
generation.

Before this change:
* The EBreak enum includes all values applicable to break-before &
  break-after properties.
* A subset of these values are applicable to break-inline & this
  subset is enforced with some asserts.

After this change:
* EBreakBetween includes all values applicable to break-before &
  break-after.
* EBreakInside includes values applicable to break-inline.

Client code working with EBreak has been made specific to the
appropriate new enum. I burrowed upwards through the call chain using
code search to find out / verify whether each client was using EBreak to
store EBreakBetween-like values (most often), EBreakInside-like values
(sometimes), or both (rarely).

I chose the name "EBreakBetween" because the main clients in /layout use
"Between" for the same thing.

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

to

==========
Split EBreak enum into EBreakBetween & EBreakInside

This change splits EBreak into two enums that are ready for automatic
generation.

Before this change:
* The EBreak enum includes all values applicable to break-before &
  break-after properties.
* A subset of these values are applicable to break-inside & this
  subset is enforced with some asserts.

After this change:
* EBreakBetween includes all values applicable to break-before &
  break-after.
* EBreakInside includes values applicable to break-inside.

Client code working with EBreak has been made specific to the
appropriate new enum. I burrowed upwards through the call chain using
code search to find out / verify whether each client was using EBreak to
store EBreakBetween-like values (most often), EBreakInside-like values
(sometimes), or both (rarely).

I chose the name "EBreakBetween" because the main clients in /layout use
"Between" for the same thing.

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

[amoylan, 2017-02-07 23:29:52]

On 2017/02/07 10:21:35, mstensho wrote:
> Typos in the description:
> 
> > * A subset of these values are applicable to break-inline & this
>                                                      ^^^^^^
> Should be "break-inside", not "break-inline".
> 
> > * EBreakInside includes values applicable to break-inline.
>                                                      ^^^^^^
> Should be "break-inside", not "break-inline".
> 
>
https://codereview.chromium.org/2682573002/diff/20001/third_party/WebKit/Sour...
> File third_party/WebKit/Source/core/layout/LayoutBox.cpp (left):
> 
>
https://codereview.chromium.org/2682573002/diff/20001/third_party/WebKit/Sour...
> third_party/WebKit/Source/core/layout/LayoutBox.cpp:2157: if (breakValue ==
> BreakAuto)
> On 2017/02/07 04:35:02, amoylan wrote:
> > This ASSERT is not required now that we have split out the smaller
> EBreakInside.
> 
> Acknowledged. Good stuff!
> 
>
https://codereview.chromium.org/2682573002/diff/20001/third_party/WebKit/Sour...
> File third_party/WebKit/Source/core/layout/LayoutBox.h (right):
> 
>
https://codereview.chromium.org/2682573002/diff/20001/third_party/WebKit/Sour...
> third_party/WebKit/Source/core/layout/LayoutBox.h:1634: inline bool
> LayoutBox::isForcedFragmentainerBreakValue(
> On 2017/02/07 04:35:02, amoylan wrote:
> > For "Between" type breaks, this function was used to test whether the break
> was
> > a "forced"-type break (i.e. not "avoid" or "auto"). This use is retained.
> > 
> > For "Inside" type breaks, this function was used to assert that a given
EBreak
> > only had "Inside"-type values. This use is no longer required now that
> > EBreakInside is split out.
> 
> Acknowledged.
> 
>
https://codereview.chromium.org/2682573002/diff/20001/third_party/WebKit/Sour...
> File third_party/WebKit/Source/core/style/ComputedStyle.h (right):
> 
>
https://codereview.chromium.org/2682573002/diff/20001/third_party/WebKit/Sour...
> third_party/WebKit/Source/core/style/ComputedStyle.h:266: unsigned
m_breakBefore
> : 4;  // EBreak
> Need to update these comments.

Thanks for the review!

[amoylan, 2017-02-07 23:30:06]

Thanks

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

https://codereview.chromium.org/2682573002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/style/ComputedStyle.h:266: unsigned m_breakBefore
: 4;  // EBreak
On 2017/02/07 10:21:35, mstensho wrote:
> Need to update these comments.

Done.

[sashab, 2017-02-08 01:54:33]

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

[sashab, 2017-02-08 01:54:33]

LGTM this is awesome! :D

+shend fyi

[alancutter (OOO until 2018), 2017-02-08 23:26:04]

"Between" is a strange word for describing the opposite (before & after) but if
it's consistent with the rest of the code base that's more important.

lgtm

[amoylan, 2017-02-08 23:30:20]

On 2017/02/08 23:26:04, alancutter wrote:
> "Between" is a strange word for describing the opposite (before & after) but
if
> it's consistent with the rest of the code base that's more important.

Yes that's what I thought too.

It seems it's because, if break-after|before allows/requires a break, then that
break will be "between" two elements. Specifically, between the element on which
break-after|before is set and an adjacent element.




> 
> lgtm

[mstensho (USE GERRIT), 2017-02-09 09:10:04]

On 2017/02/08 23:30:20, amoylan wrote:
> On 2017/02/08 23:26:04, alancutter wrote:
> > "Between" is a strange word for describing the opposite (before & after) but
> if
> > it's consistent with the rest of the code base that's more important.
> 
> Yes that's what I thought too.
> 
> It seems it's because, if break-after|before allows/requires a break, then
that
> break will be "between" two elements. Specifically, between the element on
which
> break-after|before is set and an adjacent element.
> 
> 
> 
> 
> > 
> > lgtm

It may have been me who introduced the term "between" to Blink, but I think it
makes sense:

https://drafts.csswg.org/css-break/#break-between - It's even in the URL! :)

You typically combine one or more break-before values with one or more
break-after values to figure out whether or not to break between two elements.

[amoylan, 2017-02-10 00:00:22]

The CQ bit was checked by amoylan@chromium.org

[amoylan, 2017-02-10 00:00:22]

The patchset sent to the CQ was uploaded after l-g-t-m from mstensho@opera.com
Link to the patchset: https://codereview.chromium.org/2682573002/#ps40001
(title: "Update references to EBreak in ComputedStyle.h comments")

[commit-bot: I haz the power, 2017-02-10 00:01:20]

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-02-10 01:07:16]

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

[commit-bot: I haz the power, 2017-02-10 01:07:17]

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

[amoylan, 2017-02-10 02:02:10]

The CQ bit was checked by amoylan@chromium.org

[commit-bot: I haz the power, 2017-02-10 02:02:46]

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-02-10 08:04:28]

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

[commit-bot: I haz the power, 2017-02-10 08:04:29]

Try jobs failed on following builders:
  linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[amoylan, 2017-02-12 22:46:06]

The CQ bit was checked by amoylan@chromium.org

[commit-bot: I haz the power, 2017-02-12 22:46:13]

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-02-13 00:39:43]

CQ is committing da patch.

Bot data: {"patchset_id": 40001, "attempt_start_ts": 1486939566047960,
"parent_rev": "2fbfd7490c876078719caf9867575b9a0826b2ba", "commit_rev":
"cc78776e3214ce8cd60288796a20baf7f98e7b52"}

[commit-bot: I haz the power, 2017-02-13 00:40:41]

Description was changed from

==========
Split EBreak enum into EBreakBetween & EBreakInside

This change splits EBreak into two enums that are ready for automatic
generation.

Before this change:
* The EBreak enum includes all values applicable to break-before &
  break-after properties.
* A subset of these values are applicable to break-inside & this
  subset is enforced with some asserts.

After this change:
* EBreakBetween includes all values applicable to break-before &
  break-after.
* EBreakInside includes values applicable to break-inside.

Client code working with EBreak has been made specific to the
appropriate new enum. I burrowed upwards through the call chain using
code search to find out / verify whether each client was using EBreak to
store EBreakBetween-like values (most often), EBreakInside-like values
(sometimes), or both (rarely).

I chose the name "EBreakBetween" because the main clients in /layout use
"Between" for the same thing.

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

to

==========
Split EBreak enum into EBreakBetween & EBreakInside

This change splits EBreak into two enums that are ready for automatic
generation.

Before this change:
* The EBreak enum includes all values applicable to break-before &
  break-after properties.
* A subset of these values are applicable to break-inside & this
  subset is enforced with some asserts.

After this change:
* EBreakBetween includes all values applicable to break-before &
  break-after.
* EBreakInside includes values applicable to break-inside.

Client code working with EBreak has been made specific to the
appropriate new enum. I burrowed upwards through the call chain using
code search to find out / verify whether each client was using EBreak to
store EBreakBetween-like values (most often), EBreakInside-like values
(sometimes), or both (rarely).

I chose the name "EBreakBetween" because the main clients in /layout use
"Between" for the same thing.

BUG=628043

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

[commit-bot: I haz the power, 2017-02-13 00:40:42]

Committed patchset #3 (id:40001) as
https://chromium.googlesource.com/chromium/src/+/cc78776e3214ce8cd60288796a20...