Remove BreakAlways from EBreak enum

Remove BreakAlways from EBreak enum

This change is the first step in preparing EBreak for automatic
generation.

The EBreak enum currently has three kinds of values:
* Values applicable to break-before & break-after property
* A subset of these applicable to break-inline
* A single value BreakAlways applicable to neither and only used
  temporarily in the code path for computing values for the properties
  page-break-before|after|inline and
  -webkit-column-break-before|after|inline.

This change removes the last bullet point. The next stage will split the
other two into separate enum classes with standardized naming.

Reasons to feel confident about removing BreakAlways:
* There are existing DCHECKs ensuring that it is never set in
  ComputedStyle (removed in this CL).
* CSSValueAlways (the only thing that maps to it) is excluded from the
  list of allowed values for break-before|after|inline in
  CSSParserFastPaths.cpp.
* For the legacy properties (page|-webkit-column)-break-(before|after),
  CSSPropertyParser.cpp converts CSSValueAlways to either CSSValuePage
  or CSSValueColumn as appropriate (and CSSValueInvalid for
  *-inside properties).

I have replaced the temporary use of BreakAlways within
ComputedStyleCSSValueMapping.cpp with an equivalent direct conversion to
appropriate CSSValues. (This seems more consistent with the rest of
ComputedStyleCSSValueMapping.cpp anyway.)


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

[amoylan, 2017-02-06 05:57:29]

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

[commit-bot: I haz the power, 2017-02-06 05:57:42]

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

[amoylan, 2017-02-06 06:06:29]

Description was changed from

==========
Remove BreakAlways from EBreak enum

This change is the first step in preparing EBreak for automatic
generation.

The EBreak enum currently has three kinds of values:
* Values applicable to break-before & break-after property
* A subset of these applicable to break-inline
* A single value BreakAlways applicable to neither and only used
  temporarily in the code path for computing values for the properties
  page-break-before|after|inline and
  -webkit-column-break-before|after|inline.

This change removes the last bullet point. The next stage will split the
other two into separate enum classes with standardized naming.

Reasons to feel confident about removing BreakAlways:
* There are existing DCHECKs ensuring that it is never set in
  ComputedStyle (removed in this CL).
* CSSValueAlways (the only thing that maps to it) is excluded from the
  list of allowed values for break-before|after|inline in
  CSSParserFastPaths.cpp.

I have replaced the temporary use of BreakAlways within
ComputedStyleCSSValueMapping.cpp with an equivalent direct conversion to
appropriate CSSValues. (This seems more consistent with the rest of
ComputedStyleCSSValueMapping.cpp anyway.)


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

to

==========
Remove BreakAlways from EBreak enum

This change is the first step in preparing EBreak for automatic
generation.

The EBreak enum currently has three kinds of values:
* Values applicable to break-before & break-after property
* A subset of these applicable to break-inline
* A single value BreakAlways applicable to neither and only used
  temporarily in the code path for computing values for the properties
  page-break-before|after|inline and
  -webkit-column-break-before|after|inline.

This change removes the last bullet point. The next stage will split the
other two into separate enum classes with standardized naming.

Reasons to feel confident about removing BreakAlways:
* There are existing DCHECKs ensuring that it is never set in
  ComputedStyle (removed in this CL).
* CSSValueAlways (the only thing that maps to it) is excluded from the
  list of allowed values for break-before|after|inline in
  CSSParserFastPaths.cpp.
* For the legacy properties (page|-webkit-column)-break-(before|after),
  CSSPropertyParser.cpp converts CSSValueAlways to either CSSValuePage
  or CSSValueColumn as appropriate (and CSSValueInvalid for
  *-inside properties).

I have replaced the temporary use of BreakAlways within
ComputedStyleCSSValueMapping.cpp with an equivalent direct conversion to
appropriate CSSValues. (This seems more consistent with the rest of
ComputedStyleCSSValueMapping.cpp anyway.)


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

[amoylan, 2017-02-06 06:16:04]

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

[amoylan, 2017-02-06 06:16:55]

Hi Sasha, please let me know if I should direct this to someone else for a first
review, I wasn't sure.

[commit-bot: I haz the power, 2017-02-06 07:47:00]

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

[commit-bot: I haz the power, 2017-02-06 07:47:00]

Dry run: This issue passed the CQ dry run.

[sashab, 2017-02-07 01:44:58]

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

[sashab, 2017-02-07 01:44:58]

Great CL description! This change sounds great. I love the "reasons to feel
confident" bit :D

Hmm who's a good person to do a first pass review of this... Maybe alancutter@,
I've added him, alan ptal :)

[alancutter (OOO until 2018), 2017-02-08 23:37:09]

I'm confused. This change removes the conversion from CSSValue that produces
BreakAlways and doesn't replace it with anything else. Was BreakAlways ever
actually used or is this patch cleaning up dead code?

[amoylan, 2017-02-09 00:18:46]

On 2017/02/08 23:37:09, alancutter wrote:
> I'm confused. This change removes the conversion from CSSValue that produces
> BreakAlways and doesn't replace it with anything else. Was BreakAlways ever
> actually used or is this patch cleaning up dead code?

Yes this was the trickiest part to be sure about. The reasoning goes as follows:

---------------------------------------

For conversion from EBreak to CSSValue:

  CSSIdentifierValue::CSSIdentifierValue(EBreak)
is only called from
  CSSIdentifierValue::create(EBreak)
which is called as either

1. CSSIdentifierValue::create(b), where b is one of the getters from
ComputedStyle e.g. style.breakBefore(). This can't be BreakAlways as existing
DCHECKS prevent ComputedStyle from storing a BreakAlways.

or as

2. CSSIdentifierValue::create(f(b)) where f is one of {mapToPageBreakValue,
mapToColumnBreakValue}. These f are very simple converter functions that map
EBreak values to each other, to turn our stored values for e.g. "break-before"
into a suitable value for legacy properties such as e.g. "page-break-before".
Here BreakAlways always is used very temporarily before being immediately
consumed by CSSIdentifierValue::create(). I resolved this by absorbing the call
to CSSIdentifierValue::create into f (this is the style mainly used in
ComputedStyleCSSValueMapping.cpp anyway).

---------------------------------------

For conversion from CSSValue to EBreak:

Calls to
  EBreak CSSIdentifierValue::convertTo()
are always immediately passed to one of the EBreak setters on ComputedStyle,
which apply DCHECKS to prevent BreakAlways.

---------------------------------------

I believe the above discoveries in code search are sufficient to safely remove
BreakAlways, but I was quite curious why CSSValueAlways somehow never bubbles
down to the function EBreak CSSIdentifierValue::convertTo(), triggering a
DCHECK. The answer to this is contained in my last two bullets in "reasons to
feel confident" in the CL description:
* CSSValueAlways (the only thing that maps to it) is excluded from the
  list of allowed values for break-before|after|inline in
  CSSParserFastPaths.cpp.
* For the legacy properties (page|-webkit-column)-break-(before|after),
  CSSPropertyParser.cpp converts CSSValueAlways to either CSSValuePage
  or CSSValueColumn as appropriate (and CSSValueInvalid for
  *-inside properties).

[amoylan, 2017-02-09 00:24:17]

On 2017/02/09 00:18:46, amoylan wrote:
> On 2017/02/08 23:37:09, alancutter wrote:
> > I'm confused. This change removes the conversion from CSSValue that produces
> > BreakAlways and doesn't replace it with anything else. Was BreakAlways ever
> > actually used or is this patch cleaning up dead code?
> 
> Yes this was the trickiest part to be sure about. The reasoning goes as
follows:
> 
> ---------------------------------------
> 
> For conversion from EBreak to CSSValue:
> 
>   CSSIdentifierValue::CSSIdentifierValue(EBreak)
> is only called from
>   CSSIdentifierValue::create(EBreak)
> which is called as either
> 
> 1. CSSIdentifierValue::create(b), where b is one of the getters from
> ComputedStyle e.g. style.breakBefore(). This can't be BreakAlways as existing
> DCHECKS prevent ComputedStyle from storing a BreakAlways.
> 
> or as
> 
> 2. CSSIdentifierValue::create(f(b)) where f is one of {mapToPageBreakValue,
> mapToColumnBreakValue}. These f are very simple converter functions that map
> EBreak values to each other, to turn our stored values for e.g. "break-before"
> into a suitable value for legacy properties such as e.g. "page-break-before".
> Here BreakAlways always is used very temporarily before being immediately
> consumed by CSSIdentifierValue::create(). I resolved this by absorbing the
call
> to CSSIdentifierValue::create into f (this is the style mainly used in
> ComputedStyleCSSValueMapping.cpp anyway).
> 
> ---------------------------------------
> 
> For conversion from CSSValue to EBreak:
> 
> Calls to
>   EBreak CSSIdentifierValue::convertTo()
> are always immediately passed to one of the EBreak setters on ComputedStyle,
> which apply DCHECKS to prevent BreakAlways.
> 
> ---------------------------------------
> 
> I believe the above discoveries in code search are sufficient to safely remove
> BreakAlways, but I was quite curious why CSSValueAlways somehow never bubbles
> down to the function EBreak CSSIdentifierValue::convertTo(), triggering a
> DCHECK. The answer to this is contained in my last two bullets in "reasons to
> feel confident" in the CL description:
> * CSSValueAlways (the only thing that maps to it) is excluded from the
>   list of allowed values for break-before|after|inline in
>   CSSParserFastPaths.cpp.
> * For the legacy properties (page|-webkit-column)-break-(before|after),
>   CSSPropertyParser.cpp converts CSSValueAlways to either CSSValuePage
>   or CSSValueColumn as appropriate (and CSSValueInvalid for
>   *-inside properties).

TLDR: BreakAlways was *almost* dead code and the changes in
ComputedStyleCSSValueMapping.cpp make it dead code.

[alancutter (OOO until 2018), 2017-02-09 04:47:17]

Thanks for the extra clarification. It probably was useful once but got changed
along the way. The conversion from EBreak to CSSValue in
ComputedStyleCSSValueMapping is just serialisation logic, it doesn't suggest
that the value actually gets used.

I just wanted to double check as your description didn't seem to suggest that it
was an unused value, I think it's safe to say that this value is not used in the
engine.

lgtm

[sashab, 2017-02-09 04:58:42]

lgtm

[amoylan, 2017-02-09 22:10:17]

The CQ bit was checked by amoylan@chromium.org

[commit-bot: I haz the power, 2017-02-09 22:10:54]

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-09 23:56:33]

CQ is committing da patch.

Bot data: {"patchset_id": 1, "attempt_start_ts": 1486678217043560, "parent_rev":
"cbf28490c8aaa6e4809efe3c321ced99e60f388d", "commit_rev":
"446e9444ff7dd64a3afff530b5224069d14f72a5"}

[commit-bot: I haz the power, 2017-02-09 23:57:08]

Description was changed from

==========
Remove BreakAlways from EBreak enum

This change is the first step in preparing EBreak for automatic
generation.

The EBreak enum currently has three kinds of values:
* Values applicable to break-before & break-after property
* A subset of these applicable to break-inline
* A single value BreakAlways applicable to neither and only used
  temporarily in the code path for computing values for the properties
  page-break-before|after|inline and
  -webkit-column-break-before|after|inline.

This change removes the last bullet point. The next stage will split the
other two into separate enum classes with standardized naming.

Reasons to feel confident about removing BreakAlways:
* There are existing DCHECKs ensuring that it is never set in
  ComputedStyle (removed in this CL).
* CSSValueAlways (the only thing that maps to it) is excluded from the
  list of allowed values for break-before|after|inline in
  CSSParserFastPaths.cpp.
* For the legacy properties (page|-webkit-column)-break-(before|after),
  CSSPropertyParser.cpp converts CSSValueAlways to either CSSValuePage
  or CSSValueColumn as appropriate (and CSSValueInvalid for
  *-inside properties).

I have replaced the temporary use of BreakAlways within
ComputedStyleCSSValueMapping.cpp with an equivalent direct conversion to
appropriate CSSValues. (This seems more consistent with the rest of
ComputedStyleCSSValueMapping.cpp anyway.)


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

to

==========
Remove BreakAlways from EBreak enum

This change is the first step in preparing EBreak for automatic
generation.

The EBreak enum currently has three kinds of values:
* Values applicable to break-before & break-after property
* A subset of these applicable to break-inline
* A single value BreakAlways applicable to neither and only used
  temporarily in the code path for computing values for the properties
  page-break-before|after|inline and
  -webkit-column-break-before|after|inline.

This change removes the last bullet point. The next stage will split the
other two into separate enum classes with standardized naming.

Reasons to feel confident about removing BreakAlways:
* There are existing DCHECKs ensuring that it is never set in
  ComputedStyle (removed in this CL).
* CSSValueAlways (the only thing that maps to it) is excluded from the
  list of allowed values for break-before|after|inline in
  CSSParserFastPaths.cpp.
* For the legacy properties (page|-webkit-column)-break-(before|after),
  CSSPropertyParser.cpp converts CSSValueAlways to either CSSValuePage
  or CSSValueColumn as appropriate (and CSSValueInvalid for
  *-inside properties).

I have replaced the temporary use of BreakAlways within
ComputedStyleCSSValueMapping.cpp with an equivalent direct conversion to
appropriate CSSValues. (This seems more consistent with the rest of
ComputedStyleCSSValueMapping.cpp anyway.)


BUG=628043

Review-Url: https://codereview.chromium.org/2677033002
Cr-Commit-Position: refs/heads/master@{#449474}
Committed:
https://chromium.googlesource.com/chromium/src/+/446e9444ff7dd64a3afff530b522...
==========

[commit-bot: I haz the power, 2017-02-09 23:57:09]

Committed patchset #1 (id:1) as
https://chromium.googlesource.com/chromium/src/+/446e9444ff7dd64a3afff530b522...