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

Fix 9283 - space or comma separated rect() arguments #11495

Merged
merged 1 commit into from May 30, 2016
Merged

Fix 9283 - space or comma separated rect() arguments #11495

merged 1 commit into from May 30, 2016

Conversation

@mitchhentges
Copy link
Contributor

mitchhentges commented May 29, 2016 •
edited by larsbergstrom

Allow either commas or space to separate rect() arguments, but not both. Don't allocate unnecessary Vec

  • ./mach build -d does not report any errors
  • ./mach test-tidy does not report any errors
  • These changes fix #9283
  • There are tests for these changes OR

This change is Reviewable
@highfive
Copy link

highfive commented May 29, 2016

Heads up! This PR modifies the following files:

  • @bholley: components/style/properties/longhand/effects.mako.rs
@highfive
Copy link

highfive commented May 29, 2016

warning Warning warning

  • These commits modify style code, but no tests are modified. Please consider adding a test!
@nox
Copy link
Member

nox commented May 30, 2016

r=me modulo this one nit.

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

Previously, highfive wrote…

warning Warning warning

  • These commits modify style code, but no tests are modified. Please consider adding a test!

Reviewed 1 of 1 files at r1.
Review status: all files reviewed at latest revision, 1 unresolved discussion, some commit checks failed.

components/style/properties/longhand/effects.mako.rs, line 354 [r1] (raw file):

        }

        let parse_argument = |input: &mut Parser| {

Nit: make it a proper nested function.

fn parse_argument(input: &mut Parser) {
    ...
}

This makes it clear that the function does not actually capture anything from its environment.

Comments from Reviewable
@nox nox assigned nox and unassigned emilio May 30, 2016
@mitchhentges mitchhentges force-pushed the mitchhentges:9283-rect-no-comma branch from 385cc6d to aafac46 May 30, 2016
@highfive
Copy link

highfive commented May 30, 2016

New code was committed to pull request.
@nox
Copy link
Member

nox commented May 30, 2016

@bors-servo r+

Previously, highfive wrote…

New code was committed to pull request.

Reviewed 1 of 1 files at r2.
Review status: all files reviewed at latest revision, all discussions resolved, some commit checks failed.

Comments from Reviewable
@bors-servo
Copy link
Contributor

bors-servo commented May 30, 2016

📌 Commit aafac46 has been approved by nox
@bors-servo
Copy link
Contributor

bors-servo commented May 30, 2016

⌛ Testing commit aafac46 with merge adf36c1...
bors-servo added a commit that referenced this pull request May 30, 2016
@bors-servo
Auto merge of #11495 - mitchhentges:9283-rect-no-comma, r=nox
adf36c1 
Fix 9283 - space or comma separated rect() arguments

<!-- Please describe your changes on the following line: -->
Allow either commas or space to separate `rect()` arguments, but not both. Don't allocate unnecessary `Vec`

---
<!-- 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
- [X] These changes fix #9283

<!-- Either: -->
- [X] There are tests for these changes OR

<!-- 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="35" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/11495)
<!-- Reviewable:end -->
components/style/properties/longhand/effects.mako.rs
if input.try(|input| input.expect_ident_matching("auto")).is_ok() {
return Ok(SpecifiedValue(None))
}
if !try!(input.expect_function()).eq_ignore_ascii_case("rect") {
return Err(())
}
let sides = try!(input.parse_nested_block(|input| {

This comment has been minimized.

Sign in to view
@SimonSapin

SimonSapin May 30, 2016

Member

parse_nested_block is still needed. Without it, you’re ignoring the stuff between rect( and ), and then parsing four arguments that come after the function.

To fix this, move the rest of this function (from let top = …; to Ok(…)) into a

try!(input.parse_nested_block(|input| {
    // …
}))`

block.
@nox
Copy link
Member

nox commented May 30, 2016

@bors-servo r-

Oops.
@bors-servo
Copy link
Contributor

bors-servo commented May 30, 2016

💔 Test failed - mac-rel-wpt
@highfive
Copy link

highfive commented May 30, 2016 

  ▶ FAIL [expected PASS] /_mozilla/css/clip_a.html
  └   → /_mozilla/css/clip_a.html fd99d6363df07fc0cec3c258cb712e1dcb5d45e3
/_mozilla/css/clip_ref.html 5232a5d12024c568e03a896086d96b265eb6fd3d
Testing fd99d6363df07fc0cec3c258cb712e1dcb5d45e3 == 5232a5d12024c568e03a896086d96b265eb6fd3d

  ▶ FAIL [expected PASS] /_mozilla/css/inline_absolute_hypothetical_clip_a.html
  └   → /_mozilla/css/inline_absolute_hypothetical_clip_a.html 06e7dce5e1b5196a60dbcc1664d88fbdc7d0c105
/_mozilla/css/inline_absolute_hypothetical_clip_ref.html 9cd155b7137f205c8fe54d2c666bb76d976a0a21
Testing 06e7dce5e1b5196a60dbcc1664d88fbdc7d0c105 == 9cd155b7137f205c8fe54d2c666bb76d976a0a21
@mitchhentges
Copy link
Contributor Author

mitchhentges commented May 30, 2016

Thanks for the tip, totally missed that! I don't think I need the try!() around it though, because I return a Result
@mitchhentges
Allow either comma or space seperated css rect()
Loading status checks…
41d2670
@mitchhentges mitchhentges force-pushed the mitchhentges:9283-rect-no-comma branch from aafac46 to 41d2670 May 30, 2016
@highfive
Copy link

highfive commented May 30, 2016

New code was committed to pull request.
@SimonSapin
Copy link
Member

SimonSapin commented May 30, 2016

Yes, good point about try!.

@bors-servo r+
@bors-servo
Copy link
Contributor

bors-servo commented May 30, 2016

📌 Commit 41d2670 has been approved by SimonSapin
@bors-servo
Copy link
Contributor

bors-servo commented May 30, 2016

⌛ Testing commit 41d2670 with merge 509c04c...
bors-servo added a commit that referenced this pull request May 30, 2016
@bors-servo
Auto merge of #11495 - mitchhentges:9283-rect-no-comma, r=SimonSapin
Loading status checks…
509c04c 
Fix 9283 - space or comma separated rect() arguments

<!-- Please describe your changes on the following line: -->
Allow either commas or space to separate `rect()` arguments, but not both. Don't allocate unnecessary `Vec`

---
<!-- 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
- [X] These changes fix #9283

<!-- Either: -->
- [X] There are tests for these changes OR

<!-- 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="35" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/11495)
<!-- Reviewable:end -->
@bors-servo
Copy link
Contributor

bors-servo commented May 30, 2016

☀️ Test successful - android, arm32, arm64, linux-dev, linux-rel, mac-dev-unit, mac-rel-css, mac-rel-wpt, windows
@bors-servo bors-servo merged commit 41d2670 into servo:master May 30, 2016
2 of 3 checks passed
2 of 3 checks passed
continuous-integration/appveyor/pr AppVeyor build failed
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
homu Test successful
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees

@SimonSapin SimonSapin

Labels
None yet
Projects
None yet
Milestone
No milestone
Linked issues

Successfully merging this pull request may close these issues.

clip: rect(...) only works if the values are comma separated
6 participants
@mitchhentges @highfive @nox @bors-servo @SimonSapin @emilio
You can’t perform that action at this time.