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

stylo: Bug 1392161 - Introduce CSSPixelLength as computed::Length #18381

Merged
merged 4 commits into from Sep 13, 2017

Conversation

Projects
None yet
6 participants
@BorisChiou
Copy link
Contributor

commented Sep 5, 2017

These are the inter-dependent patches of bug 1392161. We want to handle
extreme small lengths carefully for some properties, such as transform, so we
shouldn't use |Au| as the computed value of specified::Length. Now, we introduce
a new type, CSSPixelLength, which is a wrapper of CSSFloat, and it is the
computed value of specified::Length, so we can keep the fractional part
of computed::Length.


  • ./mach build -d does not report any errors
  • ./mach test-tidy does not report any errors
  • These changes fix Bug 1392161.
  • These changes do not require tests because there is a wpt test for this, and I also add some others in Gecko.

This change is Reviewable

@highfive

This comment has been minimized.

Copy link

commented Sep 5, 2017

Heads up! This PR modifies the following files:

  • @bholley: components/style/gecko/generated/bindings.rs, components/style/values/specified/transform.rs, components/style/values/specified/mod.rs, components/style/properties/gecko.mako.rs, components/style/values/computed/mod.rs and 6 more
  • @canaltinova: components/style/gecko/generated/bindings.rs, components/style/values/specified/transform.rs, components/style/values/specified/mod.rs, components/style/properties/gecko.mako.rs, components/style/values/computed/mod.rs and 6 more
  • @emilio: components/layout/fragment.rs, components/style/gecko/generated/bindings.rs, components/style/values/specified/transform.rs, components/style/values/specified/mod.rs, components/style/properties/gecko.mako.rs and 7 more
@highfive

This comment has been minimized.

Copy link

commented Sep 5, 2017

warning Warning warning

  • These commits modify unsafe code. Please review it carefully!
@BorisChiou

This comment has been minimized.

Copy link
Contributor Author

commented Sep 5, 2017

r?@emilio

Assume handling extremely small lengths for transform is necessary, so we should avoid using Au as the computed lengths of transform. The current implementation has many duplicated codes, but I don't have a good idea to reduce them. @nox has a good idea?

also cc @upsuper

@highfive highfive assigned emilio and unassigned nox Sep 5, 2017

@nox

This comment has been minimized.

Copy link
Member

commented Sep 5, 2017

As long as it works it's fine I guess. I don't currently have the mental bandwidth to think about how to derive more of this.

@BorisChiou

This comment has been minimized.

Copy link
Contributor Author

commented Sep 6, 2017

bors-servo added a commit that referenced this pull request Sep 6, 2017

Auto merge of #18381 - BorisChiou:stylo/transform/rounding, r=<try>
stylo: Bug 1392161 - Length values should not be rounded to Au for computed transform

These are the inter-dependent patches of bug 1392161. We should handle
extreme small lengths carefully for transform, so we shouldn't use |Au|
as the computed value, and make sure the computed value of transform
is the same as the specified value, but with relative lengths converted
into absolute lengths.

---
- [X] `./mach build -d` does not report any errors
- [X] `./mach test-tidy` does not report any errors
- [X] These changes fix [Bug 1392161](https://bugzilla.mozilla.org/show_bug.cgi?id=1392161).
- [X] These changes do not require tests because there is a wpt test for this, and I also add some others in Gecko.

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

This comment has been minimized.

Copy link
Contributor

commented Sep 6, 2017

⌛️ Trying commit 6504187 with merge 33c92bd...

@bors-servo

This comment has been minimized.

Copy link
Contributor

commented Sep 6, 2017

💔 Test failed - linux-rel-wpt

FontMetricsQueryResult::NotAvailable => reference_font_size.scale_by(0.5 * length),
}
FontMetricsQueryResult::NotAvailable => {
FontReferenceSize::Half(reference_font_size)

This comment has been minimized.

Copy link
@emilio

emilio Sep 6, 2017

Member

Why is this needed? Can't we return just reference_font_size * 0.5 here?

This comment has been minimized.

Copy link
@BorisChiou

BorisChiou Sep 7, 2017

Author Contributor

Yes. Thanks. Actually we don't really need it. I just want to use a type to represent the reference size. I will drop it.

} else {
reference_font_size.scale_by(0.5 * length)
FontReferenceSize::Half(reference_font_size)

This comment has been minimized.

Copy link
@emilio

emilio Sep 6, 2017

Member

Ditto, I don't get the point of FontReferenceSize, care to explain?

const PX_PER_MM: CSSFloat = PX_PER_IN / 25.4;
const PX_PER_Q: CSSFloat = PX_PER_MM / 4.;
const PX_PER_PT: CSSFloat = PX_PER_IN / 72.;
const PX_PER_PC: CSSFloat = PX_PER_PT * 12.;

This comment has been minimized.

Copy link
@emilio

emilio Sep 6, 2017

Member

Can we remove the AU_PER_FOO constants and at least reuse this? If not, why not?

@@ -8,6 +8,7 @@ use cssparser::Parser;
use parser::{Parse, ParserContext};
use selectors::parser::SelectorParseError;
use style_traits::{ParseError, StyleParseError};
use super::Either;

This comment has been minimized.

Copy link
@emilio

emilio Sep 6, 2017

Member

Let's not use super::Either, just values::Either.


impl Parse for TransformLengthOrPercentage {
#[inline]
fn parse<'i, 't>(context: &ParserContext,

This comment has been minimized.

Copy link
@emilio

emilio Sep 6, 2017

Member

Let's indent this properly.

impl Parse for TransformLength {
#[inline]
fn parse<'i, 't>(context: &ParserContext,
input: &mut Parser<'i, 't>) -> Result<Self, ParseError<'i>> {

This comment has been minimized.

Copy link
@emilio

emilio Sep 6, 2017

Member

ditto, let's use block indentation here.

#[cfg_attr(feature = "servo", derive(HeapSizeOf))]
#[animate(fallback = "Self::animate_fallback")]
#[derive(Animate, Clone, Copy, Debug, PartialEq, ToAnimatedZero, ToCss)]
pub enum TransformLengthOrPercentage {

This comment has been minimized.

Copy link
@emilio

emilio Sep 6, 2017

Member

This is still a lot of duplicated code :(

@emilio
Copy link
Member

left a comment

After seeing the other changes in https://bugzilla.mozilla.org/show_bug.cgi?id=1396692, I think the right way to do this is just using CSSFloat as the computed value everywhere (maybe with a new type, CSSPixel?), instead of duplicating all this code.

That'd solve the precision problem everywhere, is there any reason that can't be done?

@nox

This comment has been minimized.

Copy link
Member

commented Sep 6, 2017

I agree with @emilio. Maybe we don't need to fix the precision problem everywhere indeed, but if fixing it everywhere means we have less code to maintain, I'm all for it.

@emilio

This comment has been minimized.

Copy link
Member

commented Sep 6, 2017

Furthermore, the only reason stuff operates on Au was because it was simpler for layout, but that conversion can just be done in the style -> layout boundary, so it seems reasonable to just use floating point numbers everywhere in style.

So, a concrete proposal:

  • Create a CSSPixelLength type that wraps CSSFloat, and is the computed value of Length, and serializes with ToCss as {}px (using the appropriate methods: f32::to_css, etc).
  • Implement a way to convert from CSSPixelLength to Au, to be used at the style -> layout, and indeed it'd just call Au::from_f32_px(self.0).
  • Move all our computed values from Au to CSSPixelLength.
  • Use the conversion code to clap from floating point to Au when passing it to style structs.

@BorisChiou, does that sound reasonable? This doesn't need to block https://bugzilla.mozilla.org/show_bug.cgi?id=1396692 (that code can just land without the Au -> CSSFloat change, right?).

I'm happy to help implement this if you want.

@BorisChiou

This comment has been minimized.

Copy link
Contributor Author

commented Sep 7, 2017

Thanks emilio. Thanks. it makes sense, and that's the ideal way to do this. At first, I'm not sure the main purpose of using Au. Thanks for clarification. I will land https://bugzilla.mozilla.org/show_bug.cgi?id=1396692 first (because it is much simpler), and then do replace Length with CSSPixelLength. :)

@birtles

This comment has been minimized.

Copy link
Contributor

commented Sep 7, 2017

I like this proposal. We've already updated some SMIL tests to accept either 'px' or unitless values when calling getComputedStyle() on various stroke-* properties. Feel free to update other tests to do the same, especially where other browsers are already adding 'px' for these properties.

@bors-servo

This comment has been minimized.

Copy link
Contributor

commented Sep 8, 2017

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

@BorisChiou BorisChiou force-pushed the BorisChiou:stylo/transform/rounding branch from 6504187 to 2b0bec5 Sep 13, 2017

@bors-servo

This comment has been minimized.

Copy link
Contributor

commented Sep 13, 2017

⌛️ Trying commit 2b0bec5 with merge bab53c0...

bors-servo added a commit that referenced this pull request Sep 13, 2017

Auto merge of #18381 - BorisChiou:stylo/transform/rounding, r=<try>
stylo: Bug 1392161 - Length values should not be rounded to Au for computed transform

These are the inter-dependent patches of bug 1392161. We want to handle
extreme small lengths carefully for some properties, such as transform, so we
shouldn't use |Au| as the computed value of specified::Length. Now, we introduce
a new type, CSSPixelLength, which is a wrapper of CSSFloat, and it is the
computed value of specified::Length, so we can keep the fractional part
of computed::Length.

---
- [X] `./mach build -d` does not report any errors
- [X] `./mach test-tidy` does not report any errors
- [X] These changes fix [Bug 1392161](https://bugzilla.mozilla.org/show_bug.cgi?id=1392161).
- [X] These changes do not require tests because there is a wpt test for this, and I also add some others in Gecko.

<!-- 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/18381)
<!-- Reviewable:end -->

@BorisChiou BorisChiou force-pushed the BorisChiou:stylo/transform/rounding branch from 2b0bec5 to b9ee1da Sep 13, 2017

@BorisChiou

This comment has been minimized.

Copy link
Contributor Author

commented Sep 13, 2017

@bors-servo

This comment has been minimized.

Copy link
Contributor

commented Sep 13, 2017

⌛️ Trying commit b9ee1da with merge 78696a1...

bors-servo added a commit that referenced this pull request Sep 13, 2017

Auto merge of #18381 - BorisChiou:stylo/transform/rounding, r=<try>
stylo: Bug 1392161 - Length values should not be rounded to Au for computed transform

These are the inter-dependent patches of bug 1392161. We want to handle
extreme small lengths carefully for some properties, such as transform, so we
shouldn't use |Au| as the computed value of specified::Length. Now, we introduce
a new type, CSSPixelLength, which is a wrapper of CSSFloat, and it is the
computed value of specified::Length, so we can keep the fractional part
of computed::Length.

---
- [X] `./mach build -d` does not report any errors
- [X] `./mach test-tidy` does not report any errors
- [X] These changes fix [Bug 1392161](https://bugzilla.mozilla.org/show_bug.cgi?id=1392161).
- [X] These changes do not require tests because there is a wpt test for this, and I also add some others in Gecko.

<!-- 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/18381)
<!-- Reviewable:end -->
if au > 1. {
// See bug 989802. We truncate so that adding multiple viewport units
// that add up to 100 does not overflow due to rounding differences
CSSPixelLength::from(Au::from_f64_au(au.trunc()))

This comment has been minimized.

Copy link
@BorisChiou

BorisChiou Sep 13, 2017

Author Contributor

I added this during rebasing my patch. Manish fixed this for some rounding issues of viewport, but I need more time to check how to combine his patch with mine, so use this if-condition as a work-around for now.

This comment has been minimized.

Copy link
@BorisChiou

BorisChiou Sep 13, 2017

Author Contributor

OK, it seems this breaks other tests, so I should update this now. :(

@BorisChiou BorisChiou changed the title stylo: Bug 1392161 - Length values should not be rounded to Au for computed transform stylo: Bug 1392161 - Introduce CSSPixelLength as computed::Length Sep 13, 2017

@BorisChiou

This comment has been minimized.

Copy link
Contributor Author

commented Sep 13, 2017

If gecko try looks good, I will land this ASAP.

@bors-servo

This comment has been minimized.

Copy link
Contributor

commented Sep 13, 2017

💔 Test failed - linux-rel-css

@BorisChiou BorisChiou force-pushed the BorisChiou:stylo/transform/rounding branch from b9ee1da to 08119d3 Sep 13, 2017

@BorisChiou

This comment has been minimized.

Copy link
Contributor Author

commented Sep 13, 2017

@bors-servo

This comment has been minimized.

Copy link
Contributor

commented Sep 13, 2017

⌛️ Trying commit 08119d3 with merge 24c09e6...

bors-servo added a commit that referenced this pull request Sep 13, 2017

Auto merge of #18381 - BorisChiou:stylo/transform/rounding, r=<try>
stylo: Bug 1392161 - Introduce CSSPixelLength as computed::Length

These are the inter-dependent patches of bug 1392161. We want to handle
extreme small lengths carefully for some properties, such as transform, so we
shouldn't use |Au| as the computed value of specified::Length. Now, we introduce
a new type, CSSPixelLength, which is a wrapper of CSSFloat, and it is the
computed value of specified::Length, so we can keep the fractional part
of computed::Length.

---
- [X] `./mach build -d` does not report any errors
- [X] `./mach test-tidy` does not report any errors
- [X] These changes fix [Bug 1392161](https://bugzilla.mozilla.org/show_bug.cgi?id=1392161).
- [X] These changes do not require tests because there is a wpt test for this, and I also add some others in Gecko.

<!-- 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/18381)
<!-- Reviewable:end -->
@BorisChiou

This comment has been minimized.

Copy link
Contributor Author

commented Sep 13, 2017

BorisChiou added some commits Sep 13, 2017

Introduce CSSPixelLength and update NonNegativeLength.
First, we define computed::CSSPixelLength which contains a CSSFloat, a
pixel value, and then we replace computed::Length with CSSPixelLength.
Therefore, the |ComputedValue| of NoCalcLength, AbsoluteLength,
FontRelativeLength, ViewportPercentageLength, CharacterWidth, and
PhysicalLength is CSSPixelLength.

Besides, we drop NonNegativeAu, and replace computed::NonNegativeLength
with NonNegative<computed::Length>. (i.e. NonNegative<CSSPixelLength>)
Replace Au with CSSPixelLength in CalcLengthOrPercentage.
We replace Au with CSSPixelLength for the length part of
computed::CalcLengthOrPercentage. Therefore, it would be easier to use
CSSPixelLength for all other LengthOrPercentage{*} types.
Use CSSPixelLength in LengthOrPercentage{*}.
Replace Au with CSSPixelLength in LengthOrPercentage,
LengthOrPercentageOrAuto, and LengthOrPercentageOrNone.
Fix extremely small pixel value for transform on Servo.
Sometimes, we want to use extremely small pixel value in transform, e.g.
translate(calc(0.001px)), which should be rendered correctly together
with scale(100000). This patch only fixes this case for Servo because
Stylo still uses Au on the Gecko side, even if we pass pixel value into
FFI directly in the previous patch.

@BorisChiou BorisChiou force-pushed the BorisChiou:stylo/transform/rounding branch from 08119d3 to ce9f1ed Sep 13, 2017

@BorisChiou

This comment has been minimized.

Copy link
Contributor Author

commented Sep 13, 2017

@bors-servo

This comment has been minimized.

Copy link
Contributor

commented Sep 13, 2017

⌛️ Trying commit ce9f1ed with merge ac1b49b...

bors-servo added a commit that referenced this pull request Sep 13, 2017

Auto merge of #18381 - BorisChiou:stylo/transform/rounding, r=<try>
stylo: Bug 1392161 - Introduce CSSPixelLength as computed::Length

These are the inter-dependent patches of bug 1392161. We want to handle
extreme small lengths carefully for some properties, such as transform, so we
shouldn't use |Au| as the computed value of specified::Length. Now, we introduce
a new type, CSSPixelLength, which is a wrapper of CSSFloat, and it is the
computed value of specified::Length, so we can keep the fractional part
of computed::Length.

---
- [X] `./mach build -d` does not report any errors
- [X] `./mach test-tidy` does not report any errors
- [X] These changes fix [Bug 1392161](https://bugzilla.mozilla.org/show_bug.cgi?id=1392161).
- [X] These changes do not require tests because there is a wpt test for this, and I also add some others in Gecko.

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

This comment has been minimized.

Copy link
Contributor

commented Sep 13, 2017

💔 Test failed - mac-rel-wpt3

@BorisChiou

This comment has been minimized.

Copy link
Contributor Author

commented Sep 13, 2017

@bors-servo r=emilio p=1

@bors-servo

This comment has been minimized.

Copy link
Contributor

commented Sep 13, 2017

📌 Commit ce9f1ed has been approved by emilio

@bors-servo

This comment has been minimized.

Copy link
Contributor

commented Sep 13, 2017

@bors-servo

This comment has been minimized.

Copy link
Contributor

commented Sep 13, 2017

@bors-servo bors-servo merged commit ce9f1ed into servo:master Sep 13, 2017

2 of 3 checks passed

continuous-integration/travis-ci/pr The Travis CI build failed
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
homu Test successful
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.