Use Parser::skip_whitespace in a few places to make Parser::try rewind less #18171

SimonSapin · 2017-08-21T14:22:02Z

Do not merge yet. This pulls in unrelated cssparser changes which add a dependency to dota, and we’d like to resolve dtolnay/dtoa#9 before landing dtoa in mozilla-central. Also, the dependency change may require manually revendoring in mozilla-central.

Gecko’s CSS parsing microbenchmarks before:

  43.437 ±  0.391 ms    Stylo.Servo_StyleSheet_FromUTF8Bytes_Bench
  29.244 ±  0.042 ms    Stylo.Gecko_nsCSSParser_ParseSheet_Bench
 281.884 ±  0.028 ms    Stylo.Servo_DeclarationBlock_SetPropertyById_Bench
 426.242 ±  0.008 ms    Stylo.Servo_DeclarationBlock_SetPropertyById_WithInitialSpace_Bench

After:

  29.779 ±  0.254 ms    Stylo.Servo_StyleSheet_FromUTF8Bytes_Bench
  28.841 ±  0.031 ms    Stylo.Gecko_nsCSSParser_ParseSheet_Bench
 296.240 ±  4.744 ms    Stylo.Servo_DeclarationBlock_SetPropertyById_Bench
 293.855 ±  4.304 ms    Stylo.Servo_DeclarationBlock_SetPropertyById_WithInitialSpace_Bench

This change is

emilio

Looks good, r=me when it's ready.

emilio · 2017-08-21T14:30:02Z

components/style/properties/properties.mako.rs

@@ -1552,6 +1552,9 @@ impl PropertyDeclaration {
        assert!(declarations.is_empty());
        match id {
            PropertyId::Custom(name) => {
+                // FIXME: fully implement https://github.com/w3c/csswg-drafts/issues/774
+                // before adding skip_whitespace here.
+                // This probably affects some test results.


This would also imply we should start adding spaces after the colon for a custom property declaration, perhaps worth leaving a note near the relevant code in declaration_block.rs.

Correct. I assume tests will catch this.

emilio · 2017-08-21T14:30:02Z

components/style_traits/values.rs

-            results.push(item);
+        loop {
+            input.skip_whitespace();  // Unnecessary for correctness, but may help try() rewind less.
+            if let Ok(item) = input.try(&mut parse_one) {


nit: I'd just use match, but no big deal.

SimonSapin · 2017-08-23T03:29:14Z

@bors-servo r=emilio

cssparser 0.19.3 is in autoland, with dtoa 0.4.2 without the unnecessary PNG file.

bors-servo · 2017-08-23T03:29:16Z

📌 Commit 9dbeabf has been approved by emilio

bors-servo · 2017-08-23T04:10:19Z

⌛ Testing commit 9dbeabf with merge 93d2073...


        Auto merge of #18171 - servo:skip_whitespace, r=emilio

Use Parser::skip_whitespace in a few places to make Parser::try rewind less **Do not merge yet.** This pulls in unrelated cssparser changes which add a dependency to `dota`, and we’d like to resolve dtolnay/dtoa#9 before landing dtoa in mozilla-central. Also, the dependency change may require manually revendoring in mozilla-central. Gecko’s CSS parsing microbenchmarks before: ``` 43.437 ± 0.391 ms Stylo.Servo_StyleSheet_FromUTF8Bytes_Bench 29.244 ± 0.042 ms Stylo.Gecko_nsCSSParser_ParseSheet_Bench 281.884 ± 0.028 ms Stylo.Servo_DeclarationBlock_SetPropertyById_Bench 426.242 ± 0.008 ms Stylo.Servo_DeclarationBlock_SetPropertyById_WithInitialSpace_Bench ``` After: ``` 29.779 ± 0.254 ms Stylo.Servo_StyleSheet_FromUTF8Bytes_Bench 28.841 ± 0.031 ms Stylo.Gecko_nsCSSParser_ParseSheet_Bench 296.240 ± 4.744 ms Stylo.Servo_DeclarationBlock_SetPropertyById_Bench 293.855 ± 4.304 ms Stylo.Servo_DeclarationBlock_SetPropertyById_WithInitialSpace_Bench ```  --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/18171)

upsuper · 2017-08-23T04:31:43Z

@bors-servo r-

It seems there are multiple failures caused from 0.19.3 which is not fixed by 0.19.4. I suppose this PR doesn't fix them either. In that case, we probably shouldn't land it at this moment.

https://treeherder.mozilla.org/#/jobs?repo=autoland&revision=a203d42085eae79a6ef106a178a8df4a9e36478e&selectedJob=125065270

bors-servo · 2017-08-23T04:43:22Z

💔 Test failed - linux-rel-wpt

bors-servo · 2017-08-23T15:02:51Z

☔ The latest upstream changes (presumably #18201) made this pull request unmergeable. Please resolve the merge conflicts.

SimonSapin · 2017-08-23T15:05:19Z

@bors-servo try

bors-servo · 2017-08-23T15:05:23Z

⌛ Trying commit 45b8cd0 with merge c04c77e...


        Auto merge of #18171 - servo:skip_whitespace, r=<try>

Use Parser::skip_whitespace in a few places to make Parser::try rewind less **Do not merge yet.** This pulls in unrelated cssparser changes which add a dependency to `dota`, and we’d like to resolve dtolnay/dtoa#9 before landing dtoa in mozilla-central. Also, the dependency change may require manually revendoring in mozilla-central. Gecko’s CSS parsing microbenchmarks before: ``` 43.437 ± 0.391 ms Stylo.Servo_StyleSheet_FromUTF8Bytes_Bench 29.244 ± 0.042 ms Stylo.Gecko_nsCSSParser_ParseSheet_Bench 281.884 ± 0.028 ms Stylo.Servo_DeclarationBlock_SetPropertyById_Bench 426.242 ± 0.008 ms Stylo.Servo_DeclarationBlock_SetPropertyById_WithInitialSpace_Bench ``` After: ``` 29.779 ± 0.254 ms Stylo.Servo_StyleSheet_FromUTF8Bytes_Bench 28.841 ± 0.031 ms Stylo.Gecko_nsCSSParser_ParseSheet_Bench 296.240 ± 4.744 ms Stylo.Servo_DeclarationBlock_SetPropertyById_Bench 293.855 ± 4.304 ms Stylo.Servo_DeclarationBlock_SetPropertyById_WithInitialSpace_Bench ```  --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/18171)

bors-servo · 2017-08-23T15:46:30Z

💔 Test failed - mac-rel-css2


        Fix interaction of skip_whitespace and nested blocks

This fixes (at least some of) the test failures at servo/servo#18171

SimonSapin · 2017-08-23T21:09:30Z

@bors-servo try

bors-servo · 2017-08-23T21:09:35Z

⌛ Trying commit 13035c1 with merge 0623e31...


        Auto merge of #18171 - servo:skip_whitespace, r=<try>

Use Parser::skip_whitespace in a few places to make Parser::try rewind less **Do not merge yet.** This pulls in unrelated cssparser changes which add a dependency to `dota`, and we’d like to resolve dtolnay/dtoa#9 before landing dtoa in mozilla-central. Also, the dependency change may require manually revendoring in mozilla-central. Gecko’s CSS parsing microbenchmarks before: ``` 43.437 ± 0.391 ms Stylo.Servo_StyleSheet_FromUTF8Bytes_Bench 29.244 ± 0.042 ms Stylo.Gecko_nsCSSParser_ParseSheet_Bench 281.884 ± 0.028 ms Stylo.Servo_DeclarationBlock_SetPropertyById_Bench 426.242 ± 0.008 ms Stylo.Servo_DeclarationBlock_SetPropertyById_WithInitialSpace_Bench ``` After: ``` 29.779 ± 0.254 ms Stylo.Servo_StyleSheet_FromUTF8Bytes_Bench 28.841 ± 0.031 ms Stylo.Gecko_nsCSSParser_ParseSheet_Bench 296.240 ± 4.744 ms Stylo.Servo_DeclarationBlock_SetPropertyById_Bench 293.855 ± 4.304 ms Stylo.Servo_DeclarationBlock_SetPropertyById_WithInitialSpace_Bench ```  --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/18171)

bors-servo · 2017-08-23T21:45:28Z

💔 Test failed - mac-rel-wpt3


        Auto merge of #185 - servo:…, r=emilio

Fix interaction of skip_whitespace and nested blocks This fixes (at least some of) the test failures at servo/servo#18171 In code like this: ```css unsupported selector ! { stuff } valid selector { color: green } ``` `rules_and_declarations::parse_qualified_rule` would leave the parser just after the first `{` with `Parser::at_start_of == Some(BlockType::CurlyBracket)`. The latter means that, unless `Parser::parse_nested_block` is called, the parser needs to skip over this block until the matching `}` before doing anything else. This PR makes `Parser::skip_whitespace` and `Parser::skip_cdc_and_cdo` (both added recently in #181) take care of `Parser::at_start_of` correctly the same way `Parser::next` does.  --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/rust-cssparser/185)


        Auto merge of #185 - servo:…, r=emilio

Fix interaction of skip_whitespace and nested blocks This fixes (at least some of) the test failures at servo/servo#18171 In code like this: ```css unsupported selector ! { stuff } valid selector { color: green } ``` `rules_and_declarations::parse_qualified_rule` would leave the parser just after the first `{` with `Parser::at_start_of == Some(BlockType::CurlyBracket)`. The latter means that, unless `Parser::parse_nested_block` is called, the parser needs to skip over this block until the matching `}` before doing anything else. This PR makes `Parser::skip_whitespace` and `Parser::skip_cdc_and_cdo` (both added recently in #181) take care of `Parser::at_start_of` correctly the same way `Parser::next` does.  --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/rust-cssparser/185)

bors-servo · 2017-08-25T20:27:17Z

💔 Test failed - linux-rel-wpt

SimonSapin · 2017-08-25T20:31:27Z

Legit failures:

  ▶ Unexpected subtest result in /cssom/serialize-variable-reference.html:
  │ FAIL [expected PASS] Shorthand with variable preserves original serialization: with withespace
  │   → assert_equals: expected "font: var(--a);" but got "font:var(--a);"
  │ 
  │ @http://web-platform.test:8000/cssom/serialize-variable-reference.html:20:9
  │ Test.prototype.step@http://web-platform.test:8000/resources/testharness.js:1406:20
  │ test@http://web-platform.test:8000/resources/testharness.js:497:9
  └ @http://web-platform.test:8000/cssom/serialize-variable-reference.html:17:5

  ▶ Unexpected subtest result in /cssom/serialize-variable-reference.html:
  │ FAIL [expected PASS] Longhand with variable preserves original serialization: with withespace
  │   → assert_equals: expected "font-size: var(--a);" but got "font-size:var(--a);"
  │ 
  │ @http://web-platform.test:8000/cssom/serialize-variable-reference.html:14:9
  │ Test.prototype.step@http://web-platform.test:8000/resources/testharness.js:1406:20
  │ test@http://web-platform.test:8000/resources/testharness.js:497:9
  └ @http://web-platform.test:8000/cssom/serialize-variable-reference.html:11:5


        Use Parser::skip_whitespace in a few places to make Parser::try rewin…

…d less. Gecko’s CSS parsing microbenchmarks before: ``` 43.437 ± 0.391 ms Stylo.Servo_StyleSheet_FromUTF8Bytes_Bench 29.244 ± 0.042 ms Stylo.Gecko_nsCSSParser_ParseSheet_Bench 281.884 ± 0.028 ms Stylo.Servo_DeclarationBlock_SetPropertyById_Bench 426.242 ± 0.008 ms Stylo.Servo_DeclarationBlock_SetPropertyById_WithInitialSpace_Bench ``` After: ``` 29.779 ± 0.254 ms Stylo.Servo_StyleSheet_FromUTF8Bytes_Bench 28.841 ± 0.031 ms Stylo.Gecko_nsCSSParser_ParseSheet_Bench 296.240 ± 4.744 ms Stylo.Servo_DeclarationBlock_SetPropertyById_Bench 293.855 ± 4.304 ms Stylo.Servo_DeclarationBlock_SetPropertyById_WithInitialSpace_Bench ```

SimonSapin · 2017-08-25T23:45:08Z

@bors-servo r=emilio p=1

bors-servo · 2017-08-25T23:45:08Z

📌 Commit dc5dfaf has been approved by emilio

bors-servo · 2017-08-25T23:46:22Z

💡 This pull request was already approved, no need to approve it again.

There's another pull request that is currently being tested, blocking this pull request: #18244

bors-servo · 2017-08-25T23:46:23Z

📌 Commit dc5dfaf has been approved by emilio

bors-servo · 2017-08-26T00:09:04Z

⌛ Testing commit dc5dfaf with merge 0193b2c...


        Auto merge of #18171 - servo:skip_whitespace, r=emilio

Use Parser::skip_whitespace in a few places to make Parser::try rewind less **Do not merge yet.** This pulls in unrelated cssparser changes which add a dependency to `dota`, and we’d like to resolve dtolnay/dtoa#9 before landing dtoa in mozilla-central. Also, the dependency change may require manually revendoring in mozilla-central. Gecko’s CSS parsing microbenchmarks before: ``` 43.437 ± 0.391 ms Stylo.Servo_StyleSheet_FromUTF8Bytes_Bench 29.244 ± 0.042 ms Stylo.Gecko_nsCSSParser_ParseSheet_Bench 281.884 ± 0.028 ms Stylo.Servo_DeclarationBlock_SetPropertyById_Bench 426.242 ± 0.008 ms Stylo.Servo_DeclarationBlock_SetPropertyById_WithInitialSpace_Bench ``` After: ``` 29.779 ± 0.254 ms Stylo.Servo_StyleSheet_FromUTF8Bytes_Bench 28.841 ± 0.031 ms Stylo.Gecko_nsCSSParser_ParseSheet_Bench 296.240 ± 4.744 ms Stylo.Servo_DeclarationBlock_SetPropertyById_Bench 293.855 ± 4.304 ms Stylo.Servo_DeclarationBlock_SetPropertyById_WithInitialSpace_Bench ```  --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/18171)

bors-servo · 2017-08-26T00:09:30Z

💔 Test failed - mac-rel-wpt2

SimonSapin · 2017-08-26T00:27:16Z

@bors-servo retry

exceptions.Exception: Actual commit (f3f80c0dca7408863cfa1a6276862f0927b7947a) differs from requested commit (0193b2c2ecb79edee62d1b06b0f1f2b363e009b7)

bors-servo · 2017-08-26T01:25:00Z

⌛ Testing commit dc5dfaf with merge 8812422...


        Auto merge of #18171 - servo:skip_whitespace, r=emilio

Use Parser::skip_whitespace in a few places to make Parser::try rewind less **Do not merge yet.** This pulls in unrelated cssparser changes which add a dependency to `dota`, and we’d like to resolve dtolnay/dtoa#9 before landing dtoa in mozilla-central. Also, the dependency change may require manually revendoring in mozilla-central. Gecko’s CSS parsing microbenchmarks before: ``` 43.437 ± 0.391 ms Stylo.Servo_StyleSheet_FromUTF8Bytes_Bench 29.244 ± 0.042 ms Stylo.Gecko_nsCSSParser_ParseSheet_Bench 281.884 ± 0.028 ms Stylo.Servo_DeclarationBlock_SetPropertyById_Bench 426.242 ± 0.008 ms Stylo.Servo_DeclarationBlock_SetPropertyById_WithInitialSpace_Bench ``` After: ``` 29.779 ± 0.254 ms Stylo.Servo_StyleSheet_FromUTF8Bytes_Bench 28.841 ± 0.031 ms Stylo.Gecko_nsCSSParser_ParseSheet_Bench 296.240 ± 4.744 ms Stylo.Servo_DeclarationBlock_SetPropertyById_Bench 293.855 ± 4.304 ms Stylo.Servo_DeclarationBlock_SetPropertyById_WithInitialSpace_Bench ```  --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/18171)

bors-servo · 2017-08-26T02:47:04Z

☀️ Test successful - android, arm32, arm64, linux-dev, linux-rel-css, linux-rel-wpt, mac-dev-unit, mac-rel-css1, mac-rel-css2, mac-rel-wpt1, mac-rel-wpt2, mac-rel-wpt3, mac-rel-wpt4, windows-msvc-dev
Approved by: emilio
Pushing 8812422 to master...

emilio approved these changes Aug 21, 2017

View changes

SimonSapin mentioned this pull request Aug 22, 2017

Have at-rule without block handled in two stages as well servo/rust-cssparser#180

Merged

highfive assigned emilio Aug 23, 2017

highfive added the S-awaiting-merge label Aug 23, 2017

highfive added S-tests-failed and removed S-awaiting-merge labels Aug 23, 2017

highfive added the S-needs-rebase label Aug 23, 2017

SimonSapin force-pushed the skip_whitespace branch from 9dbeabf to 45b8cd0 Aug 23, 2017

highfive added S-awaiting-review and removed S-tests-failed labels Aug 23, 2017

highfive added the S-tests-failed label Aug 23, 2017

SimonSapin added a commit to servo/rust-cssparser that referenced this pull request Aug 23, 2017

Fix interaction of skip_whitespace and nested blocks

Loading status checks…

23a44b0

This fixes (at least some of) the test failures at servo/servo#18171

SimonSapin force-pushed the skip_whitespace branch from 45b8cd0 to 13035c1 Aug 23, 2017

highfive removed the S-tests-failed label Aug 23, 2017

highfive added the S-tests-failed label Aug 23, 2017

highfive added the S-tests-failed label Aug 25, 2017

SimonSapin force-pushed the skip_whitespace branch from 4cd6ec7 to dc5dfaf Aug 25, 2017

highfive removed the S-tests-failed label Aug 25, 2017

highfive added S-awaiting-merge and removed S-awaiting-review S-needs-rebase labels Aug 25, 2017

highfive added S-tests-failed and removed S-awaiting-merge labels Aug 26, 2017

highfive added S-awaiting-merge and removed S-tests-failed labels Aug 26, 2017

highfive removed the S-awaiting-merge label Aug 26, 2017

bors-servo mentioned this pull request Aug 26, 2017

[WIP] Chrome DevTools Protocol implementation #18133

Closed

2 of 12 tasks complete

SimonSapin deleted the skip_whitespace branch Aug 26, 2017

servo / servo

Use Parser::skip_whitespace in a few places to make Parser::try rewind less #18171

Use Parser::skip_whitespace in a few places to make Parser::try rewind less #18171

SimonSapin commented Aug 21, 2017 •

edited

emilio left a comment

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

SimonSapin commented Aug 23, 2017

bors-servo commented Aug 23, 2017

bors-servo commented Aug 23, 2017

upsuper commented Aug 23, 2017 •

edited

bors-servo commented Aug 23, 2017

bors-servo commented Aug 23, 2017

SimonSapin commented Aug 23, 2017

bors-servo commented Aug 23, 2017

bors-servo commented Aug 23, 2017

SimonSapin commented Aug 23, 2017

bors-servo commented Aug 23, 2017

bors-servo commented Aug 23, 2017

bors-servo commented Aug 25, 2017

SimonSapin commented Aug 25, 2017

SimonSapin commented Aug 25, 2017 •

edited

bors-servo commented Aug 25, 2017

bors-servo commented Aug 25, 2017

bors-servo commented Aug 25, 2017

bors-servo commented Aug 26, 2017

bors-servo commented Aug 26, 2017

SimonSapin commented Aug 26, 2017

bors-servo commented Aug 26, 2017

bors-servo commented Aug 26, 2017

servo / servo

Join GitHub today

Use Parser::skip_whitespace in a few places to make Parser::try rewind less #18171

Use Parser::skip_whitespace in a few places to make Parser::try rewind less #18171

Conversation

SimonSapin commented Aug 21, 2017 • edited

emilio left a comment

This comment has been minimized.

emilio Aug 21, 2017

This comment has been minimized.

SimonSapin Aug 21, 2017

This comment has been minimized.

emilio Aug 21, 2017

SimonSapin commented Aug 23, 2017

bors-servo commented Aug 23, 2017

bors-servo commented Aug 23, 2017

upsuper commented Aug 23, 2017 • edited

bors-servo commented Aug 23, 2017

bors-servo commented Aug 23, 2017

SimonSapin commented Aug 23, 2017

bors-servo commented Aug 23, 2017

bors-servo commented Aug 23, 2017

SimonSapin commented Aug 23, 2017

bors-servo commented Aug 23, 2017

bors-servo commented Aug 23, 2017

bors-servo commented Aug 25, 2017

SimonSapin commented Aug 25, 2017

SimonSapin commented Aug 25, 2017 • edited

bors-servo commented Aug 25, 2017

bors-servo commented Aug 25, 2017

bors-servo commented Aug 25, 2017

bors-servo commented Aug 26, 2017

bors-servo commented Aug 26, 2017

SimonSapin commented Aug 26, 2017

bors-servo commented Aug 26, 2017

bors-servo commented Aug 26, 2017

SimonSapin commented Aug 21, 2017 •

edited

upsuper commented Aug 23, 2017 •

edited

SimonSapin commented Aug 25, 2017 •

edited