Add 'priority' key to CSSProperties.in

Add 'priority' key to CSSProperties.in

Add a 'priority' key to CSSProperties.in, rather than relying on the
file order. This removes the requirement that properties defined in this
file have to be in a certain order.

BUG=677884
Review-Url: https://codereview.chromium.org/2620233002
Cr-Commit-Position: refs/heads/master@{#443845}
Committed: https://chromium.googlesource.com/chromium/src/+/be6d3f418dca5f76954e3e04108364fb6ce3be29

[sashab, 2017-01-11 01:39:39]

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

[commit-bot: I haz the power, 2017-01-11 01:39:58]

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

[sashab, 2017-01-11 02:35:29]

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

[ktyliu, 2017-01-11 02:45:43]

lgtm

https://codereview.chromium.org/2620233002/diff/1/third_party/WebKit/Source/b...
File third_party/WebKit/Source/build/scripts/css_properties.py (right):

https://codereview.chromium.org/2620233002/diff/1/third_party/WebKit/Source/b...
third_party/WebKit/Source/build/scripts/css_properties.py:68:
prioritised_properties = {0: [], 1: [], 2: []}
Note you can do this all in one statement by using sorting based on tuple:

# Sort by priority and then by stripped name.
propertie.sort(key = lambda p: (p.priority,
name_utilities.strip_webkit_prefix(p['name']))

[sashab, 2017-01-11 02:50:39]

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

[sashab, 2017-01-11 02:50:45]

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

[sashab, 2017-01-11 02:50:46]

alancutter@ ptal, patch reduced to very little now thanks to some magic python
hAx0Rz from kevin :D

https://codereview.chromium.org/2620233002/diff/1/third_party/WebKit/Source/b...
File third_party/WebKit/Source/build/scripts/css_properties.py (right):

https://codereview.chromium.org/2620233002/diff/1/third_party/WebKit/Source/b...
third_party/WebKit/Source/build/scripts/css_properties.py:68:
prioritised_properties = {0: [], 1: [], 2: []}
On 2017/01/11 at 02:45:43, ktyliu wrote:
> Note you can do this all in one statement by using sorting based on tuple:
> 
> # Sort by priority and then by stripped name.
> propertie.sort(key = lambda p: (p.priority,
name_utilities.strip_webkit_prefix(p['name']))

"Let me do your entire patch in one line kthxbai"

;) Nice one!!!!

[commit-bot: I haz the power, 2017-01-11 02:51:07]

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

[alancutter (OOO until 2018), 2017-01-11 03:24:20]

https://codereview.chromium.org/2620233002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/build/scripts/css_properties.py (right):

https://codereview.chromium.org/2620233002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/build/scripts/css_properties.py:68:
properties.sort(key=lambda p: (int(p['priority']),
name_utilities.strip_webkit_prefix(p['name'])))
This sorting key has collisions between different items resulting in potentially
non-deterministic behaviour.
I wouldn't bother stripping "-webkit-", alternatively just strip the "-" and
assert that no sorting key collisions occur.

https://codereview.chromium.org/2620233002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/css/CSSProperties.in (right):

https://codereview.chromium.org/2620233002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/css/CSSProperties.in:148: // grouped and computed
in alphabetical order. The default value is 2.
I'm not comfortable with using numbers for the priority here when we exclusively
use enum values in the C++ code. We should use priority=Animation/High/Low.
You don't need to worry about ResolveVariables (poorly named IMO), that doesn't
appear in this file.

[sashab, 2017-01-11 03:57:44]

Nice suggestions Alan. Added a 'sorting_key' field. PTAL

https://codereview.chromium.org/2620233002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/build/scripts/css_properties.py (right):

https://codereview.chromium.org/2620233002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/build/scripts/css_properties.py:68:
properties.sort(key=lambda p: (int(p['priority']),
name_utilities.strip_webkit_prefix(p['name'])))
On 2017/01/11 at 03:24:20, alancutter wrote:
> This sorting key has collisions between different items resulting in
potentially non-deterministic behaviour.
> I wouldn't bother stripping "-webkit-", alternatively just strip the "-" and
assert that no sorting key collisions occur.

Done. Added check for sorting key collisions.

https://codereview.chromium.org/2620233002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/css/CSSProperties.in (right):

https://codereview.chromium.org/2620233002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/css/CSSProperties.in:148: // grouped and computed
in alphabetical order. The default value is 2.
On 2017/01/11 at 03:24:20, alancutter wrote:
> I'm not comfortable with using numbers for the priority here when we
exclusively use enum values in the C++ code. We should use
priority=Animation/High/Low.
> You don't need to worry about ResolveVariables (poorly named IMO), that
doesn't appear in this file.

Done, not sure what you meant with ResolveVariables.

[alancutter (OOO until 2018), 2017-01-11 04:20:56]

lgtm. :D

https://codereview.chromium.org/2620233002/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/build/scripts/css_properties.py (right):

https://codereview.chromium.org/2620233002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/build/scripts/css_properties.py:70: priority_numbers =
{'Animation': 0, 'High': 1, 'Low': 2}
Add comment stating that this must match the order in CSSPropertyPriority.h (and
maybe add a TODO to auto generate that file (:).

[sashab, 2017-01-11 23:28:52]

https://codereview.chromium.org/2620233002/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/build/scripts/css_properties.py (right):

https://codereview.chromium.org/2620233002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/build/scripts/css_properties.py:70: priority_numbers =
{'Animation': 0, 'High': 1, 'Low': 2}
On 2017/01/11 at 04:20:56, alancutter wrote:
> Add comment stating that this must match the order in CSSPropertyPriority.h
(and maybe add a TODO to auto generate that file (:).

Added comment, didn't add TODO to generate CSSPropertyPriority.h because I think
it has benefits if its handwritten. If we generate too much we can't check for
errors in our generator. :)

[alancutter (OOO until 2018), 2017-01-11 23:31:16]

On 2017/01/11 at 23:28:52, sashab wrote:
>
https://codereview.chromium.org/2620233002/diff/40001/third_party/WebKit/Sour...
> File third_party/WebKit/Source/build/scripts/css_properties.py (right):
> 
>
https://codereview.chromium.org/2620233002/diff/40001/third_party/WebKit/Sour...
> third_party/WebKit/Source/build/scripts/css_properties.py:70: priority_numbers
= {'Animation': 0, 'High': 1, 'Low': 2}
> On 2017/01/11 at 04:20:56, alancutter wrote:
> > Add comment stating that this must match the order in CSSPropertyPriority.h
(and maybe add a TODO to auto generate that file (:).
> 
> Added comment, didn't add TODO to generate CSSPropertyPriority.h because I
think it has benefits if its handwritten. If we generate too much we can't check
for errors in our generator. :)

Nothing stopping you from having hand written static_asserts as well.

[sashab, 2017-01-11 23:36:16]

Ah, I misunderstood. Yes, we should definitely generate the methods in
CSSPropertyPriority.h. Added TODO for that, thanks.

[sashab, 2017-01-11 23:36:22]

The CQ bit was checked by sashab@chromium.org

[sashab, 2017-01-11 23:36:23]

The patchset sent to the CQ was uploaded after l-g-t-m from ktyliu@chromium.org,
alancutter@chromium.org
Link to the patchset: https://codereview.chromium.org/2620233002/#ps80001
(title: "Added generating todo")

[commit-bot: I haz the power, 2017-01-11 23:37:16]

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-01-11 23:41:12]

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

[commit-bot: I haz the power, 2017-01-11 23:41:14]

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...)
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[sashab, 2017-01-11 23:50:20]

The CQ bit was checked by sashab@chromium.org

[sashab, 2017-01-11 23:50:20]

The patchset sent to the CQ was uploaded after l-g-t-m from ktyliu@chromium.org,
alancutter@chromium.org
Link to the patchset: https://codereview.chromium.org/2620233002/#ps100001
(title: "Rebase")

[commit-bot: I haz the power, 2017-01-11 23:51:08]

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-01-12 00:04:29]

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

[commit-bot: I haz the power, 2017-01-12 00:04:30]

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

[sashab, 2017-01-16 02:35:55]

The CQ bit was checked by sashab@chromium.org

[sashab, 2017-01-16 02:35:56]

The patchset sent to the CQ was uploaded after l-g-t-m from ktyliu@chromium.org,
alancutter@chromium.org
Link to the patchset: https://codereview.chromium.org/2620233002/#ps120001
(title: "Removed version control conflict markers")

[commit-bot: I haz the power, 2017-01-16 02:36:17]

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-01-16 02:48:06]

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

[commit-bot: I haz the power, 2017-01-16 02:48:07]

Try jobs failed on following builders:
  android_compile_dbg on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_comp...)
  linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[sashab, 2017-01-16 02:54:27]

The CQ bit was checked by sashab@chromium.org

[sashab, 2017-01-16 02:54:28]

The patchset sent to the CQ was uploaded after l-g-t-m from ktyliu@chromium.org,
alancutter@chromium.org
Link to the patchset: https://codereview.chromium.org/2620233002/#ps140001
(title: "Rebase")

[commit-bot: I haz the power, 2017-01-16 02:54:39]

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-01-16 03:02:58]

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

[commit-bot: I haz the power, 2017-01-16 03:02:59]

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

[sashab, 2017-01-16 03:09:03]

The CQ bit was checked by sashab@chromium.org

[sashab, 2017-01-16 03:09:04]

The patchset sent to the CQ was uploaded after l-g-t-m from ktyliu@chromium.org,
alancutter@chromium.org
Link to the patchset: https://codereview.chromium.org/2620233002/#ps160001
(title: "Small fix from bad rebsae")

[commit-bot: I haz the power, 2017-01-16 03:09: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-01-16 04:32:21]

CQ is committing da patch.

Bot data: {"patchset_id": 160001, "attempt_start_ts": 1484536143880050,
"parent_rev": "fd7c54b5c2775dc8d87c634e402b82d83aca2da1", "commit_rev":
"be6d3f418dca5f76954e3e04108364fb6ce3be29"}

[commit-bot: I haz the power, 2017-01-16 04:33:23]

Description was changed from

==========
Add 'priority' key to CSSProperties.in

Add a 'priority' key to CSSProperties.in, rather than relying on the
file order. This removes the requirement that properties defined in this
file have to be in a certain order.

BUG=677884
==========

to

==========
Add 'priority' key to CSSProperties.in

Add a 'priority' key to CSSProperties.in, rather than relying on the
file order. This removes the requirement that properties defined in this
file have to be in a certain order.

BUG=677884

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

[commit-bot: I haz the power, 2017-01-16 04:33:24]

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