Automatically generate EBreakBetween & EBreakInside

Automatically generate EBreakBetween & EBreakInside

The change updates CSSProperties.json5 to cause the EBreakBetween &
EBreakInside enums to be automatically generated in
ComputedStyleBase.cpp.

The manual definition of these enums and any relevant getters, setters,
initializers, and comparitors in ComputedStyle.h/cpp are removed.

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

[amoylan, 2017-02-08 02:28:57]

Description was changed from

==========
Automatically generate EBreakBetween & EBreakInside

The change updates CSSProperties.json5 to cause the EBreakBetween &
EBreakInside enums to be automatically generated in
ComputedStyleBase.cpp.

The manual definition of these enums and any relevant getters, setters,
initializers, and comparitors in ComputedStyle.h/cpp are removed.

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

to

==========
Automatically generate EBreakBetween & EBreakInside

The change updates CSSProperties.json5 to cause the EBreakBetween &
EBreakInside enums to be automatically generated in
ComputedStyleBase.cpp.

The manual definition of these enums and any relevant getters, setters,
initializers, and comparitors in ComputedStyle.h/cpp are removed.

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

[amoylan, 2017-02-08 02:28:57]

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

[amoylan, 2017-02-08 02:29:45]

Hi Darren, could you please take the first look?

CC Sasha for comment on my comments about comments.

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

https://codereview.chromium.org/2681833004/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/style/ComputedStyle.h:766: // break-after
(shorthand for page-break-after and -webkit-column-break-after)
This is quite interesting. Lost comments. I wonder if we should allow the
generator to generate comments. Particularly if we consider moving the generated
files from out/ into the checked-in files.

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

https://codereview.chromium.org/2681833004/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/style/ComputedStyle.h:231: // hasViewportUnits
Formatter was pretty eager to de-indent this // hasViewportUnits.
To be honest I don't know what these comments are.

https://codereview.chromium.org/2681833004/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/style/ComputedStyleConstants.h (left):

https://codereview.chromium.org/2681833004/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/style/ComputedStyleConstants.h:320: // Values
applicable to the break-before, break-after properties.
Another lost comment.

[sashab, 2017-02-08 02:32:15]

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

[sashab, 2017-02-08 02:32:15]

Commented on comments about comments. :^)

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

https://codereview.chromium.org/2681833004/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/style/ComputedStyle.h:766: // break-after
(shorthand for page-break-after and -webkit-column-break-after)
On 2017/02/08 at 02:29:45, amoylan wrote:
> This is quite interesting. Lost comments. I wonder if we should allow the
generator to generate comments. Particularly if we consider moving the generated
files from out/ into the checked-in files.

Move the comments to the CSSProperties json5 file.

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

https://codereview.chromium.org/2681833004/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/style/ComputedStyle.h:231: // hasViewportUnits
On 2017/02/08 at 02:29:45, amoylan wrote:
> Formatter was pretty eager to de-indent this // hasViewportUnits.
> To be honest I don't know what these comments are.

Haha, good question... Get rid of them all. They probably used to refer to
fields that are now generated.

https://codereview.chromium.org/2681833004/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/style/ComputedStyleConstants.h (left):

https://codereview.chromium.org/2681833004/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/style/ComputedStyleConstants.h:320: // Values
applicable to the break-before, break-after properties.
On 2017/02/08 at 02:29:45, amoylan wrote:
> Another lost comment.

This is now intrinsic to the json5 file - they share the enum.

We *could* add a comment above the generated enums with a list of properties
that use the enum. Happy for you to do this in a follow up patch. :D

[amoylan, 2017-02-08 02:48:47]

Thanks

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

https://codereview.chromium.org/2681833004/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/style/ComputedStyle.h:766: // break-after
(shorthand for page-break-after and -webkit-column-break-after)
On 2017/02/08 02:32:15, sashab wrote:
> On 2017/02/08 at 02:29:45, amoylan wrote:
> > This is quite interesting. Lost comments. I wonder if we should allow the
> generator to generate comments. Particularly if we consider moving the
generated
> files from out/ into the checked-in files.
> 
> Move the comments to the CSSProperties json5 file.

Good idea. Done. PTAL.

https://codereview.chromium.org/2681833004/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/style/ComputedStyleConstants.h (left):

https://codereview.chromium.org/2681833004/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/style/ComputedStyleConstants.h:320: // Values
applicable to the break-before, break-after properties.
On 2017/02/08 02:32:15, sashab wrote:
> On 2017/02/08 at 02:29:45, amoylan wrote:
> > Another lost comment.
> 
> This is now intrinsic to the json5 file - they share the enum.
> 
> We *could* add a comment above the generated enums with a list of properties
> that use the enum. Happy for you to do this in a follow up patch. :D

Acknowledged.

[shend, 2017-02-08 03:32:26]

lgtm, nice work! Looks much cleaner.

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

https://codereview.chromium.org/2681833004/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/style/ComputedStyle.h:231: // hasViewportUnits
On 2017/02/08 at 02:32:15, sashab wrote:
> On 2017/02/08 at 02:29:45, amoylan wrote:
> > Formatter was pretty eager to de-indent this // hasViewportUnits.
> > To be honest I don't know what these comments are.
> 
> Haha, good question... Get rid of them all. They probably used to refer to
fields that are now generated.

I think the comments were used to explicitly indicate that these fields do not
participate in == comparisons, so it doesn't just look like we forgot those
fields (there's a comment above that says "differences in inherited bits or
other flags should not cause an inequality").

[sashab, 2017-02-09 03:34:34]

Oh, I think darren is right. Ok sorry, please add those comments back to the
comparison operator, and add another comment above them like // Differences in
the following fields do not cause inequality:

It'll go out of date any day now, but, let's push that problem to a future us.
8-)

LGTM

[amoylan, 2017-02-09 04:26:33]

On 2017/02/09 03:34:34, sashab wrote:
> Oh, I think darren is right. Ok sorry, please add those comments back to the
> comparison operator, and add another comment above them like // Differences in
> the following fields do not cause inequality:
> 
> It'll go out of date any day now, but, let's push that problem to a future us.
> 8-)
> 
> LGTM

Roger that! Done.

[amoylan, 2017-02-13 00:50:05]

The CQ bit was checked by amoylan@chromium.org

[amoylan, 2017-02-13 00:50:06]

The patchset sent to the CQ was uploaded after l-g-t-m from sashab@chromium.org,
shend@chromium.org
Link to the patchset: https://codereview.chromium.org/2681833004/#ps60001
(title: "Add clarifying comment about fields that do not impact operator==.")

[commit-bot: I haz the power, 2017-02-13 00:50: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:51:41]

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

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

Try jobs failed on following builders:
  ios-device on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...)
  ios-device-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-xcode-...)
  ios-simulator on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
  ios-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...)

[amoylan, 2017-02-13 02:04:27]

The CQ bit was checked by amoylan@chromium.org

[amoylan, 2017-02-13 02:04:27]

The patchset sent to the CQ was uploaded after l-g-t-m from sashab@chromium.org,
shend@chromium.org
Link to the patchset: https://codereview.chromium.org/2681833004/#ps80001
(title: "Rebase")

[commit-bot: I haz the power, 2017-02-13 02:04:43]

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 08:05:22]

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

[commit-bot: I haz the power, 2017-02-13 08:05:23]

Try jobs failed on following builders:
  linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  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-13 20:54:40]

The CQ bit was checked by amoylan@chromium.org

[commit-bot: I haz the power, 2017-02-13 20:55:44]

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 22:37:01]

CQ is committing da patch.

Bot data: {"patchset_id": 80001, "attempt_start_ts": 1487019280533210,
"parent_rev": "8cc82777e38c6e39cfcbbdfccce6216a222251e1", "commit_rev":
"1b4d28430e901f64aa5727490a3768bb25f3139e"}

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

Description was changed from

==========
Automatically generate EBreakBetween & EBreakInside

The change updates CSSProperties.json5 to cause the EBreakBetween &
EBreakInside enums to be automatically generated in
ComputedStyleBase.cpp.

The manual definition of these enums and any relevant getters, setters,
initializers, and comparitors in ComputedStyle.h/cpp are removed.

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

to

==========
Automatically generate EBreakBetween & EBreakInside

The change updates CSSProperties.json5 to cause the EBreakBetween &
EBreakInside enums to be automatically generated in
ComputedStyleBase.cpp.

The manual definition of these enums and any relevant getters, setters,
initializers, and comparitors in ComputedStyle.h/cpp are removed.

BUG=628043

Review-Url: https://codereview.chromium.org/2681833004
Cr-Commit-Position: refs/heads/master@{#450124}
Committed:
https://chromium.googlesource.com/chromium/src/+/1b4d28430e901f64aa5727490a37...
==========

[commit-bot: I haz the power, 2017-02-13 22:37:44]

Committed patchset #5 (id:80001) as
https://chromium.googlesource.com/chromium/src/+/1b4d28430e901f64aa5727490a37...