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

style: Move contain outside of mako #19682

Merged
merged 1 commit into from Jan 4, 2018
Merged

style: Move contain outside of mako #19682

merged 1 commit into from Jan 4, 2018

Conversation

@stevel98
Copy link
Contributor

stevel98 commented Jan 4, 2018 •
edited

Sub-PR for #19015.

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

This change is Reviewable
@highfive
Copy link

highfive commented Jan 4, 2018

Thanks for the pull request, and welcome! The Servo team is excited to review your changes, and you should hear from @emilio (or someone else) soon.
@highfive
Copy link

highfive commented Jan 4, 2018

Heads up! This PR modifies the following files:

  • @bholley: components/style/values/specified/box.rs, components/style/properties/longhand/box.mako.rs, components/style/values/computed/box.rs, components/style/values/specified/mod.rs, components/style/values/computed/mod.rs
  • @canaltinova: components/style/values/specified/box.rs, components/style/properties/longhand/box.mako.rs, components/style/values/computed/box.rs, components/style/values/specified/mod.rs, components/style/values/computed/mod.rs
  • @emilio: components/style/values/specified/box.rs, components/style/properties/longhand/box.mako.rs, components/style/values/computed/box.rs, components/style/values/specified/mod.rs, components/style/values/computed/mod.rs
@highfive
Copy link

highfive commented Jan 4, 2018

warning Warning warning

  • These commits modify style code, but no tests are modified. Please consider adding a test!
@emilio
emilio reviewed Jan 4, 2018
View changes
Copy link
Member

emilio left a comment

Looks great! Just minor nits and suggestions, other than that, r=me

Thanks a lot for doing this!
components/style/values/specified/box.rs Outdated

impl Parse for Contain {
/// none | strict | content | [ size || layout || style || paint ]
fn parse<'i, 't>(_context: &ParserContext, input: &mut Parser<'i, 't>)

This comment has been minimized.

Sign in to view
@emilio

emilio Jan 4, 2018

Member

nit: Indentation is off, let's do:

fn parse<'i, 't>(
    _context: &ParserContext,
    input: &mut Parser<'i, 't>,
) -> Result<Contain, ParseError<'i>> {
components/style/values/specified/box.rs Outdated
};
let flag = match flag {
Some(flag) if !result.contains(flag) => flag,
_ => return Err(input.new_custom_error(SelectorParseErrorKind::UnexpectedIdent(name.clone())))

This comment has been minimized.

Sign in to view
@emilio

emilio Jan 4, 2018

Member

I'm 99% sure this clone() call is not necessary.
components/style/values/specified/box.rs Outdated
/// none | strict | content | [ size || layout || style || paint ]
fn parse<'i, 't>(_context: &ParserContext, input: &mut Parser<'i, 't>)
-> Result<Contain, ParseError<'i>> {
let mut result = Contain::empty();

This comment has been minimized.

Sign in to view
@emilio

emilio Jan 4, 2018

Member

This function is kinda dumb, and could be rewritten to be faster / clearer. Can you file a followup for it?

In particular, I'm thinking of something like:

let mut result = Contain::empty();
while let Ok(name) = input.try(|i| i.expect_ident_cloned()) {
    match_ignore_ascii_case! { &name,
        "layout" => Some(Contain::LAYOUT),
        "style" => Some(Contain::STYLE),
        "paint" => Some(Contain::PAINT),
        // The following are only valid as the first value, thus the is_empty()
        // checks.
        "strict" => {
            if result.is_empty() {
                return Ok(Contain::STRICT | Contain::STRICT_BITS)
            }
            None
        }
        "none" => {
            if result.is_empty() {
                return Ok(result);
            }
            None
        }
        _ => None
    };

    // Same code as now.
}

Also fine to do it in a separate commit in this same PR.

That avoids having to tokenize twice for none and strict just to tokenize an identifier again below.

This comment has been minimized.

Sign in to view
@stevel98

stevel98 Jan 4, 2018 •
edited

Author Contributor

Thanks for the feedback, I will file a followup commit soon.
components/style/values/specified/box.rs Outdated
}
if input.try(|input| input.expect_ident_matching("strict")).is_ok() {
result.insert(Contain::STRICT | Contain::STRICT_BITS);
return Ok(result)

This comment has been minimized.

Sign in to view
@emilio

emilio Jan 4, 2018

Member

nit: just return Ok(Contain::STRICT | Contain::STRICT_BITS) may be cleaner?
components/style/values/specified/box.rs Outdated
const PAINT = 0x04;
/// `strict` variant, turns on all types of containment
const STRICT = 0x8;
/// `strict bits` variant

This comment has been minimized.

Sign in to view
@emilio

emilio Jan 4, 2018

Member

Just /// variant with all the bits that contain: strict turns on?
@stevel98
Copy link
Contributor Author

stevel98 commented Jan 4, 2018

I just pushed the changes, let me know if there need to be any more!
@CYBAI
CYBAI reviewed Jan 4, 2018
View changes
components/style/values/specified/box.rs Outdated
"style" => Some(Contain::STYLE),
"paint" => Some(Contain::PAINT),
"strict" if result.is_empty() => return Ok(Contain::STRICT | Contain::STRICT_BITS),
"none" if result.is_empty() => return Ok(result),

This comment has been minimized.

Sign in to view
@CYBAI

CYBAI Jan 4, 2018

Collaborator

I think you'll need to check if result.is_empty() inside the pattern instead of using it as a pattern matching guard. If putting if result.is_empty() as guard here, I think it will only get into the strict one?

IMO, just like @emilio commented in the comment, it might be fine to be like following

"strict" => {
    if result.is_empty() {
        return Ok(Contain::STRICT | Contain::STRICT_BITS)
     }
     None
},
"none" => {
    if result.is_empty() {
        return Ok(result);
    }
    None
}

This comment has been minimized.

Sign in to view
@stevel98

stevel98 Jan 4, 2018

Author Contributor

Thanks, I changed it back to the previous version. I don't really understand why it won't get into the none arm though; how would rust evaluate the guard?

This comment has been minimized.

Sign in to view
@emilio

emilio Jan 4, 2018

Member

FWIW the guard should work just fine, and it's more elegant indeed.

Wanna open a PR after this one merges simplifying it? Thanks!

This comment has been minimized.

Sign in to view
@stevel98

stevel98 Jan 4, 2018

Author Contributor

Sure, I'd be glad to, assuming this one passes the tests.

This comment has been minimized.

Sign in to view
@CYBAI

CYBAI Jan 4, 2018

Collaborator

Ahhh... yes... sorry, I think it in a wrong way so using guard here should work fine.
Sorry for the misleading 😣
@CYBAI
Copy link
Collaborator

CYBAI commented Jan 4, 2018

Besides, I think it would be great to squash the two commits into one 😀
@stevel98 stevel98 force-pushed the stevel98:moveContain branch from 63e7b3f to 3181809 Jan 4, 2018
@stevel98
Copy link
Contributor Author

stevel98 commented Jan 4, 2018

I'm not sure why this test failed after such a small change. Could someone please re-run it?
@CYBAI
Copy link
Collaborator

CYBAI commented Jan 4, 2018

@stevel98 I think you can try to commit --amend and push -f to force Travis to re-run :)
@stevel98
style: Move contain outside of mako
Loading status checks…
4c23f09
@stevel98
Copy link
Contributor Author

stevel98 commented Jan 4, 2018

Thanks!
@stevel98 stevel98 force-pushed the stevel98:moveContain branch from 3181809 to 4c23f09 Jan 4, 2018
@emilio
emilio approved these changes Jan 4, 2018
View changes
Copy link
Member

emilio left a comment

LGTM, thanks a lot!
@emilio
Copy link
Member

emilio commented Jan 4, 2018

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

bors-servo commented Jan 4, 2018

📌 Commit 4c23f09 has been approved by emilio
@bors-servo
Copy link
Contributor

bors-servo commented Jan 4, 2018

⌛ Testing commit 4c23f09 with merge a25c526...
bors-servo added a commit that referenced this pull request Jan 4, 2018
@bors-servo
Auto merge of #19682 - stevel98:moveContain, r=emilio
Loading status checks…
a25c526 
style: Move contain outside of mako

<!-- Please describe your changes on the following line: -->
Sub-PR for #19015.
---
<!-- 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 #19656 (github issue number if applicable).

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

<!-- Also, please make sure that "Allow edits from maintainers" checkbox is checked, so that we can help you if you get stuck somewhere along the way.-->

<!-- 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/19682)
<!-- Reviewable:end -->
@bors-servo
Copy link
Contributor

bors-servo commented Jan 4, 2018

💔 Test failed - mac-dev-unit
@emilio
Copy link
Member

emilio commented Jan 4, 2018

@bors-servo retry
@bors-servo
Copy link
Contributor

bors-servo commented Jan 4, 2018

⌛ Testing commit 4c23f09 with merge 7a9f99e...
bors-servo added a commit that referenced this pull request Jan 4, 2018
@bors-servo
Auto merge of #19682 - stevel98:moveContain, r=emilio
Loading status checks…
7a9f99e 
style: Move contain outside of mako

<!-- Please describe your changes on the following line: -->
Sub-PR for #19015.
---
<!-- 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 #19656 (github issue number if applicable).

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

<!-- Also, please make sure that "Allow edits from maintainers" checkbox is checked, so that we can help you if you get stuck somewhere along the way.-->

<!-- 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/19682)
<!-- Reviewable:end -->
@bors-servo
Copy link
Contributor

bors-servo commented Jan 4, 2018

💔 Test failed - mac-dev-unit
@emilio
Copy link
Member

emilio commented Jan 4, 2018 •
edited

@bors-servo retry

  • Intermittent gecko-media failure.
@bors-servo
Copy link
Contributor

bors-servo commented Jan 4, 2018

⚡ Previous build results for android, arm32, arm64, linux-dev, linux-rel-css, linux-rel-wpt, mac-rel-wpt1, mac-rel-wpt3, windows-msvc-dev are reusable. Rebuilding only mac-dev-unit, mac-rel-css1, mac-rel-css2, mac-rel-wpt2, mac-rel-wpt4...
@bors-servo
Copy link
Contributor

bors-servo commented Jan 4, 2018

⚡ Previous build results for android, arm32, arm64, linux-dev, linux-rel-css, linux-rel-wpt, mac-rel-wpt1, mac-rel-wpt3, windows-msvc-dev are reusable. Rebuilding only mac-dev-unit, mac-rel-css1, mac-rel-css2, mac-rel-wpt2, mac-rel-wpt4...
@bors-servo
Copy link
Contributor

bors-servo commented Jan 4, 2018

☀️ 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 7a9f99e to master...
@bors-servo bors-servo merged commit 4c23f09 into servo:master Jan 4, 2018
3 checks passed
3 checks passed
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
homu Test successful
Details
@stevel98 stevel98 deleted the stevel98:moveContain branch Jan 4, 2018
@stevel98 stevel98 mentioned this pull request Jan 4, 2018
Merged
4 of 5 tasks complete
bors-servo added a commit that referenced this pull request Jan 5, 2018
@bors-servo
Auto merge of #19692 - stevel98:Contain, r=emilio
Loading status checks…
0393f9d 
style: Make Contain::parse simpler

<!-- Please describe your changes on the following line: -->
---
Makes Contain::parse slightly simpler, as discussed in #19682

- [X] `./mach build -d` does not report any errors
- [X] `./mach test-tidy` does not report any errors
- [X] These changes fix #19682 (github issue number if applicable).

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

<!-- 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/19692)
<!-- Reviewable:end -->
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@CYBAI CYBAI

@emilio emilio

Assignees

@emilio emilio

Labels
S-tests-failed
Projects
None yet
Milestone
No milestone
Linked issues

Successfully merging this pull request may close these issues.

Move `contain` outside of mako.
5 participants
@stevel98 @highfive @CYBAI @emilio @bors-servo
You can’t perform that action at this time.