Skip to content

/ servo

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Introduce a PropertyId enum and use it instead of strings of property names #14535

Merged
merged 7 commits into from Dec 10, 2016
Merged

Introduce a PropertyId enum and use it instead of strings of property names #14535

merged 7 commits into from Dec 10, 2016

Conversation

@SimonSapin
Copy link
Member

SimonSapin commented Dec 9, 2016 •
edited

  • LonghandId and ShorthandId are C-like enums
  • Atom is used for the name of custom properties.
  • PropertyDeclarationId is the identifier for PropertyDeclaration,
    after parsing and shorthand expansion. (Longhand or custom property.)
  • PropertyId represents any CSS property, e.g. in CSSOM.
    (Longhand, shorthand, or custom.)

CC @upsuper

  • ./mach build -d does not report any errors
  • ./mach test-tidy does not report any errors
  • These changes fix #__ (github issue number if applicable).
  • There are tests for these changes OR
  • These changes do not require tests because _____

This change is Reviewable
@highfive
Copy link

highfive commented Dec 9, 2016

Heads up! This PR modifies the following files:

  • @bholley: components/style/properties/properties.mako.rs, components/style/Cargo.toml, components/style/lib.rs, components/style/str.rs, components/style/properties/build.py, components/style/properties/declaration_block.rs, components/style/keyframes.rs, components/style/gecko_string_cache/mod.rs, components/style/properties/shorthand/serialize.mako.rs, components/style/build.rs, components/style/properties/helpers.mako.rs
  • @KiChjang: components/script/dom/window.rs, components/script/dom/cssstyledeclaration.rs, components/script/dom/bindings/str.rs, components/script_layout_interface/rpc.rs, components/script_layout_interface/message.rs
  • @fitzgen: components/script/dom/window.rs, components/script/dom/cssstyledeclaration.rs, components/script/dom/bindings/str.rs, components/script_layout_interface/rpc.rs, components/script_layout_interface/message.rs
  • @emilio: ports/geckolib/glue.rs, components/style/properties/properties.mako.rs, components/style/Cargo.toml, components/style/lib.rs, components/style/str.rs, components/style/properties/build.py, components/style/properties/declaration_block.rs, components/style/keyframes.rs, components/style/gecko_string_cache/mod.rs, components/style/properties/shorthand/serialize.mako.rs, components/style/build.rs, components/style/properties/helpers.mako.rs, components/layout/query.rs
SimonSapin added 3 commits Oct 6, 2016
@SimonSapin
cow_to_ascii_lowercase()
97344b1
@SimonSapin
Introduce enums for identifying CSS properties.
137e30b 
* `LonghandId` and `ShorthandId` are C-like enums
* `Atom` is used for the name of custom properties.
* `PropertyDeclarationId` is the identifier for `PropertyDeclaration`,
  after parsing and shorthand expansion. (Longhand or custom property.)
* `PropertyId` represents any CSS property, e.g. in CSSOM.
  (Longhand, shorthand, or custom.)

Using these instead of strings avoids some memory allocations and copies.
@SimonSapin
Use rust-phf to map strings to property IDs
fdc4059
@SimonSapin SimonSapin force-pushed the property-id branch from 3a3a13f to 87c7430 Dec 9, 2016
@SimonSapin
Copy link
Member Author

SimonSapin commented Dec 9, 2016

@upsuper I’ve added a first pass at PropertyId::from_nscsspropertyid, but it’s not pretty.
@SimonSapin SimonSapin force-pushed the property-id branch from 50d2191 to eccea85 Dec 9, 2016
@mbrubeck mbrubeck assigned mbrubeck and unassigned cbrewster Dec 9, 2016
@mbrubeck
Copy link
Contributor

mbrubeck commented Dec 10, 2016

r=mbrubeck with this test-unit error fixed:

   Compiling style_tests v0.0.1 (file:///home/travis/build/servo/servo/tests/unit/style)
error: no rules expected the token `"top"`
   --> /home/travis/build/servo/servo/tests/unit/style/stylist.rs:104:51
    |
104 |     assert_eq!(1, selector_map.id_hash.get(&atom!("top")).unwrap()[0].source_order);
    |                                                   ^^^^^
error: Could not compile `style_tests`.
@mbrubeck
Copy link
Contributor

mbrubeck commented Dec 10, 2016

-S-awaiting-review +S-needs-code-changes

Reviewed 5 of 5 files at r1, 10 of 10 files at r2, 7 of 7 files at r3, 8 of 8 files at r4, 6 of 6 files at r5, 2 of 2 files at r6, 1 of 1 files at r7.
Review status: all files reviewed at latest revision, all discussions resolved, some commit checks failed.

Comments from Reviewable
SimonSapin added 4 commits Dec 9, 2016
@SimonSapin
Use PropertyId instead of Atom for CSSStyleDeclaration::get_computed_…
58d452f 
…style
@SimonSapin
Match on PropertyId (not strings) to parse a declaration.
433c33c
@SimonSapin
Use PropertyId in per-property CSSStyleDeclaration accessors.
dbc9fcc
@SimonSapin
Add PropertyId::from_nscsspropertyid
Loading status checks…
9856c61
@SimonSapin SimonSapin force-pushed the property-id branch from eccea85 to 9856c61 Dec 10, 2016
@SimonSapin
Copy link
Member Author

SimonSapin commented Dec 10, 2016

Fixed.

@bors-servo r=mbrubeck
@bors-servo
Copy link
Contributor

bors-servo commented Dec 10, 2016

📌 Commit 9856c61 has been approved by mbrubeck
@bors-servo
Copy link
Contributor

bors-servo commented Dec 10, 2016

⌛ Testing commit 9856c61 with merge 6dd4b48...
bors-servo added a commit that referenced this pull request Dec 10, 2016
@bors-servo
Auto merge of #14535 - servo:property-id, r=mbrubeck
Loading status checks…
6dd4b48 
Introduce a PropertyId enum and use it instead of strings of property names

<!-- Please describe your changes on the following line: -->

* `LonghandId` and `ShorthandId` are C-like enums
* `Atom` is used for the name of custom properties.
* `PropertyDeclarationId` is the identifier for `PropertyDeclaration`,
  after parsing and shorthand expansion. (Longhand or custom property.)
* `PropertyId` represents any CSS property, e.g. in CSSOM.
  (Longhand, shorthand, or custom.)

CC @upsuper

---
<!-- Thank you for contributing to Servo! Please replace each `[ ]` by `[X]` when the step is complete, and replace `__` with appropriate data: -->
- [x] `./mach build -d` does not report any errors
- [x] `./mach test-tidy` does not report any errors
- [ ] These changes fix #__ (github issue number if applicable).

<!-- Either: -->
- [ ] There are tests for these changes OR
- [ ] These changes do not require tests because _____

<!-- Pull requests that do not address these steps are welcome, but they will require additional verification as part of the review process. -->

<!-- Reviewable:start -->
---
This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/14535)
<!-- Reviewable:end -->
@bors-servo
Copy link
Contributor

bors-servo commented Dec 10, 2016

☀️ Test successful - arm32, arm64, linux-dev, linux-rel-css, linux-rel-wpt, mac-dev-unit, mac-rel-css, mac-rel-wpt1, mac-rel-wpt2, windows-dev
@bors-servo bors-servo merged commit 9856c61 into master Dec 10, 2016
4 checks passed
4 checks passed
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
dependency-ci Dependencies checked
Details
homu Test successful
Details
This was referenced Dec 10, 2016
Merged
Merged
Merged
@SimonSapin SimonSapin deleted the property-id branch Dec 10, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees

@mbrubeck mbrubeck

Labels
None yet
Projects
None yet
Milestone
No milestone
Linked issues

Successfully merging this pull request may close these issues.

None yet

5 participants
@SimonSapin @highfive @mbrubeck @bors-servo @cbrewster
You can’t perform that action at this time.