Skip to content
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 border shorthand serialization #15416

Merged
merged 1 commit into from Mar 1, 2017

Conversation

@karlding
Copy link
Contributor

commented Feb 7, 2017

Fix border shorthand serialization when sides are not identical. I think I managed to get the serialization to work as expected.

I added a check to ensure that the border-width, border-style and border-color were the same, before performing the serialization.

I'm still a Rust newbie, so if there's a more idiomatic way of doing things (or any critiques in general), please let me know!


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

This change is Reviewable

@highfive

This comment has been minimized.

Copy link

commented Feb 7, 2017

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

This comment has been minimized.

Copy link

commented Feb 7, 2017

Heads up! This PR modifies the following files:

  • @bholley: components/style/properties/shorthand/border.mako.rs
  • @emilio: components/style/properties/shorthand/border.mako.rs
self.border_${side}_color
)
% for side in PHYSICAL_SIDES:
let border_${side}_width = self.border_${side}_width.clone();

This comment has been minimized.

Copy link
@emilio

emilio Feb 8, 2017

Member

are the clone needed? I'm pretty sure you can avoid it using something like:

let all_equal = {
    % for ...:
        let border_${...}_style = &self.border_${...}_style;
    % endfor
    border_top_width == border_right_width &&
    // ...
};

Or just using self everywhere, whatever you prefer.

This comment has been minimized.

Copy link
@karlding

karlding Feb 9, 2017

Author Contributor

Cool, didn't know about blocks!

@emilio

This comment has been minimized.

Copy link
Member

commented Feb 8, 2017

@bors-servo

This comment has been minimized.

Copy link
Contributor

commented Feb 8, 2017

⌛️ Trying commit 4c4d959 with merge e988f79...

bors-servo added a commit that referenced this pull request Feb 8, 2017
…try>

Fix border shorthand serialization

Fix border shorthand serialization when sides are not identical. I think I managed to get the serialization to work as expected.

I added a check to ensure that the **border-width**, **border-style** and **border-color** were the same, before performing the serialization.

I'm still a Rust newbie, so if there's a more idiomatic way of doing things (or any critiques in general), please let me know!

---
<!-- 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 #15395

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

<!-- 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/15416)
<!-- Reviewable:end -->
@bors-servo

This comment has been minimized.

Copy link
Contributor

commented Feb 8, 2017

@karlding

This comment has been minimized.

Copy link
Contributor Author

commented Feb 11, 2017

Hmm, do I need to rebase my branch or something? I'm not getting those errors on my branch.

@jdm

This comment has been minimized.

Copy link
Member

commented Feb 11, 2017

Yes, that comes from #15411.

properties.push(PropertyDeclaration::BorderBottomWidth(px_30.clone()));
properties.push(PropertyDeclaration::BorderLeftWidth(px_30.clone()));

let blue = DeclaredValue::Value(CSSColor {

This comment has been minimized.

Copy link
@canova

canova Feb 11, 2017

Member

Sorry for #15411 again. After this pr, we should wrap CSSColor with Box like this:

DeclaredValue::Value(Box::new(CSSColor {
    ....
}))
@jdm

This comment has been minimized.

Copy link
Member

commented Feb 27, 2017

@emilio Review ping!

@emilio

This comment has been minimized.

Copy link
Member

commented Feb 27, 2017

@bors-servo r+

  • Though I think it will need a few tweaks due to recent changes.
@bors-servo

This comment has been minimized.

Copy link
Contributor

commented Feb 27, 2017

📌 Commit 533c13e has been approved by emilio

border_right_color == border_bottom_color &&
border_bottom_color == border_left_color
};

// If all longhands are all present, then all sides should be the same,
// so we can just one set of color/style/width

This comment has been minimized.

Copy link
@emilio

emilio Feb 27, 2017

Member

If this happens not to build, likely because of the unit tests, please remove this comment.

If you're busy or anything and can't rebase (these are minor changes), please feel free to mark the "Allow edits from maintainers" checkbox, and I'll land it for you with the changes (maintaining authorship of course).

Sorry for the extreme delay reviewing :(

This comment has been minimized.

Copy link
@emilio

emilio Feb 27, 2017

Member

And thanks for fixing this! :)

This comment has been minimized.

Copy link
@karlding

karlding Mar 1, 2017

Author Contributor

I've updated the tests.

@bors-servo

This comment has been minimized.

Copy link
Contributor

commented Feb 27, 2017

⌛️ Testing commit 533c13e with merge 7f65e7b...

@bors-servo

This comment has been minimized.

Copy link
Contributor

commented Feb 27, 2017

💔 Test failed - mac-dev-unit

@jdm

This comment has been minimized.

Copy link
Member

commented Feb 27, 2017

   Compiling style_tests v0.0.1 (file:///Users/servo/buildbot/slave/mac-dev-unit/build/tests/unit/style)
error[E0308]: mismatched types
   --> /Users/servo/buildbot/slave/mac-dev-unit/build/tests/unit/style/properties/serialization.rs:209:55
    |
209 |               parsed: ComputedColor::RGBA(RGBA { red: 0f32, green: 0f32, blue: 1f32, alpha: 1f32 }),
    |                                                       ^^^^ expected u8, found f32

error[E0308]: mismatched types
   --> /Users/servo/buildbot/slave/mac-dev-unit/build/tests/unit/style/properties/serialization.rs:209:68
    |
209 |               parsed: ComputedColor::RGBA(RGBA { red: 0f32, green: 0f32, blue: 1f32, alpha: 1f32 }),
    |                                                                    ^^^^ expected u8, found f32

error[E0308]: mismatched types
   --> /Users/servo/buildbot/slave/mac-dev-unit/build/tests/unit/style/properties/serialization.rs:209:80
    |
209 |               parsed: ComputedColor::RGBA(RGBA { red: 0f32, green: 0f32, blue: 1f32, alpha: 1f32 }),
    |                                                                                ^^^^ expected u8, found f32

error[E0308]: mismatched types
   --> /Users/servo/buildbot/slave/mac-dev-unit/build/tests/unit/style/properties/serialization.rs:209:93
    |
209 |               parsed: ComputedColor::RGBA(RGBA { red: 0f32, green: 0f32, blue: 1f32, alpha: 1f32 }),
    |                                                                                             ^^^^ expected u8, found f32

error[E0308]: mismatched types
   --> /Users/servo/buildbot/slave/mac-dev-unit/build/tests/unit/style/properties/serialization.rs:213:63
    |
213 |           properties.push(PropertyDeclaration::BorderTopColor(blue.clone()));
    |                                                               ^^^^^^^^^^^^ expected struct `style::values::specified::CSSColor`, found struct `std::boxed::Box`
    |
    = note: expected type `style::properties::DeclaredValue<style::values::specified::CSSColor>`
               found type `style::properties::DeclaredValue<std::boxed::Box<style::values::specified::CSSColor>>`

error[E0308]: mismatched types
   --> /Users/servo/buildbot/slave/mac-dev-unit/build/tests/unit/style/properties/serialization.rs:214:65
    |
214 |           properties.push(PropertyDeclaration::BorderRightColor(blue.clone()));
    |                                                                 ^^^^^^^^^^^^ expected struct `style::values::specified::CSSColor`, found struct `std::boxed::Box`
    |
    = note: expected type `style::properties::DeclaredValue<style::values::specified::CSSColor>`
               found type `style::properties::DeclaredValue<std::boxed::Box<style::values::specified::CSSColor>>`

error[E0308]: mismatched types
   --> /Users/servo/buildbot/slave/mac-dev-unit/build/tests/unit/style/properties/serialization.rs:215:66
    |
215 |           properties.push(PropertyDeclaration::BorderBottomColor(blue.clone()));
    |                                                                  ^^^^^^^^^^^^ expected struct `style::values::specified::CSSColor`, found struct `std::boxed::Box`
    |
    = note: expected type `style::properties::DeclaredValue<style::values::specified::CSSColor>`
               found type `style::properties::DeclaredValue<std::boxed::Box<style::values::specified::CSSColor>>`

error[E0308]: mismatched types
   --> /Users/servo/buildbot/slave/mac-dev-unit/build/tests/unit/style/properties/serialization.rs:216:64
    |
216 |           properties.push(PropertyDeclaration::BorderLeftColor(blue.clone()));
    |                                                                ^^^^^^^^^^^^ expected struct `style::values::specified::CSSColor`, found struct `std::boxed::Box`
    |
    = note: expected type `style::properties::DeclaredValue<style::values::specified::CSSColor>`
               found type `style::properties::DeclaredValue<std::boxed::Box<style::values::specified::CSSColor>>`

error[E0308]: mismatched types
   --> /Users/servo/buildbot/slave/mac-dev-unit/build/tests/unit/style/properties/serialization.rs:241:55
    |
241 |               parsed: ComputedColor::RGBA(RGBA { red: 0f32, green: 0f32, blue: 1f32, alpha: 1f32 }),
    |                                                       ^^^^ expected u8, found f32

error[E0308]: mismatched types
   --> /Users/servo/buildbot/slave/mac-dev-unit/build/tests/unit/style/properties/serialization.rs:241:68
    |
241 |               parsed: ComputedColor::RGBA(RGBA { red: 0f32, green: 0f32, blue: 1f32, alpha: 1f32 }),
    |                                                                    ^^^^ expected u8, found f32

error[E0308]: mismatched types
   --> /Users/servo/buildbot/slave/mac-dev-unit/build/tests/unit/style/properties/serialization.rs:241:80
    |
241 |               parsed: ComputedColor::RGBA(RGBA { red: 0f32, green: 0f32, blue: 1f32, alpha: 1f32 }),
    |                                                                                ^^^^ expected u8, found f32

error[E0308]: mismatched types
   --> /Users/servo/buildbot/slave/mac-dev-unit/build/tests/unit/style/properties/serialization.rs:241:93
    |
241 |               parsed: ComputedColor::RGBA(RGBA { red: 0f32, green: 0f32, blue: 1f32, alpha: 1f32 }),
    |                                                                                             ^^^^ expected u8, found f32

error[E0308]: mismatched types
   --> /Users/servo/buildbot/slave/mac-dev-unit/build/tests/unit/style/properties/serialization.rs:245:63
    |
245 |           properties.push(PropertyDeclaration::BorderTopColor(blue.clone()));
    |                                                               ^^^^^^^^^^^^ expected struct `style::values::specified::CSSColor`, found struct `std::boxed::Box`
    |
    = note: expected type `style::properties::DeclaredValue<style::values::specified::CSSColor>`
               found type `style::properties::DeclaredValue<std::boxed::Box<style::values::specified::CSSColor>>`

error[E0308]: mismatched types
   --> /Users/servo/buildbot/slave/mac-dev-unit/build/tests/unit/style/properties/serialization.rs:246:65
    |
246 |           properties.push(PropertyDeclaration::BorderRightColor(blue.clone()));
    |                                                                 ^^^^^^^^^^^^ expected struct `style::values::specified::CSSColor`, found struct `std::boxed::Box`
    |
    = note: expected type `style::properties::DeclaredValue<style::values::specified::CSSColor>`
               found type `style::properties::DeclaredValue<std::boxed::Box<style::values::specified::CSSColor>>`

error[E0308]: mismatched types
   --> /Users/servo/buildbot/slave/mac-dev-unit/build/tests/unit/style/properties/serialization.rs:247:66
    |
247 |           properties.push(PropertyDeclaration::BorderBottomColor(blue.clone()));
    |                                                                  ^^^^^^^^^^^^ expected struct `style::values::specified::CSSColor`, found struct `std::boxed::Box`
    |
    = note: expected type `style::properties::DeclaredValue<style::values::specified::CSSColor>`
               found type `style::properties::DeclaredValue<std::boxed::Box<style::values::specified::CSSColor>>`

error[E0308]: mismatched types
   --> /Users/servo/buildbot/slave/mac-dev-unit/build/tests/unit/style/properties/serialization.rs:248:64
    |
248 |           properties.push(PropertyDeclaration::BorderLeftColor(blue.clone()));
    |                                                                ^^^^^^^^^^^^ expected struct `style::values::specified::CSSColor`, found struct `std::boxed::Box`
    |
    = note: expected type `style::properties::DeclaredValue<style::values::specified::CSSColor>`
               found type `style::properties::DeclaredValue<std::boxed::Box<style::values::specified::CSSColor>>`
Fix border shorthand serialization when sides are not identical
@karlding

This comment has been minimized.

Copy link
Contributor Author

commented Mar 1, 2017

Rebased again

@emilio

This comment has been minimized.

Copy link
Member

commented Mar 1, 2017

@bors-servo r+

Thanks once again! :)

@bors-servo

This comment has been minimized.

Copy link
Contributor

commented Mar 1, 2017

📌 Commit ec865c3 has been approved by emilio

@bors-servo

This comment has been minimized.

Copy link
Contributor

commented Mar 1, 2017

⌛️ Testing commit ec865c3 with merge f025736...

bors-servo added a commit that referenced this pull request Mar 1, 2017
…milio

Fix border shorthand serialization

Fix border shorthand serialization when sides are not identical. I think I managed to get the serialization to work as expected.

I added a check to ensure that the **border-width**, **border-style** and **border-color** were the same, before performing the serialization.

I'm still a Rust newbie, so if there's a more idiomatic way of doing things (or any critiques in general), please let me know!

---
<!-- 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 #15395

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

<!-- 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/15416)
<!-- Reviewable:end -->
@bors-servo

This comment has been minimized.

Copy link
Contributor

commented Mar 1, 2017

@bors-servo bors-servo merged commit ec865c3 into servo:master Mar 1, 2017
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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
6 participants
You can’t perform that action at this time.