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

style: Unbox a bunch of color properties. #15518

Merged
merged 1 commit into from Feb 14, 2017
Merged

style: Unbox a bunch of color properties. #15518

merged 1 commit into from Feb 14, 2017

Conversation

@emilio
Copy link
Member

emilio commented Feb 12, 2017

This builds on servo/rust-cssparser#118.


This change is Reviewable

@highfive
Copy link

highfive commented Feb 12, 2017

Heads up! This PR modifies the following files:

  • @bholley: components/style/properties/longhand/background.mako.rs, components/style/properties/longhand/text.mako.rs, components/style/properties/longhand/outline.mako.rs, components/style/properties/longhand/svg.mako.rs, components/style/properties/longhand/inherited_text.mako.rs, components/style/values/specified/mod.rs, components/style/properties/longhand/color.mako.rs, components/style/attr.rs, components/style/properties/longhand/border.mako.rs, components/style/properties/shorthand/serialize.mako.rs, components/style/properties/helpers/animated_properties.mako.rs, components/style/properties/longhand/column.mako.rs
  • @fitzgen: components/script/dom/element.rs, components/script/dom/canvasgradient.rs, components/script/dom/canvasrenderingcontext2d.rs
  • @KiChjang: components/script/dom/element.rs, components/script/dom/canvasgradient.rs, components/script/dom/canvasrenderingcontext2d.rs
@emilio
Copy link
Member Author

emilio commented Feb 12, 2017

@highfive highfive assigned SimonSapin and unassigned nox Feb 12, 2017
@emilio
Copy link
Member Author

emilio commented Feb 12, 2017

@bors-servo
Copy link
Contributor

bors-servo commented Feb 12, 2017

Trying commit 9d66354 with merge 4aa4b4a...

bors-servo added a commit that referenced this pull request Feb 12, 2017
style: Unbox a bunch of color properties.

This builds on servo/rust-cssparser#118.

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

bors-servo commented Feb 12, 2017

💔 Test failed - linux-dev

@emilio emilio force-pushed the emilio:color branch from 9d66354 to 043c6d9 Feb 12, 2017
@emilio
Copy link
Member Author

emilio commented Feb 12, 2017

@bors-servo
Copy link
Contributor

bors-servo commented Feb 12, 2017

Trying commit 043c6d9 with merge 81785a3...

bors-servo added a commit that referenced this pull request Feb 12, 2017
style: Unbox a bunch of color properties.

This builds on servo/rust-cssparser#118.

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

bors-servo commented Feb 12, 2017

💔 Test failed - linux-rel-wpt

@jdm
Copy link
Member

jdm commented Feb 12, 2017

The linux builder suddenly does not like osmesa, apparently :<

@emilio emilio force-pushed the emilio:color branch from 043c6d9 to b6cf998 Feb 13, 2017
@emilio
Copy link
Member Author

emilio commented Feb 13, 2017

@bors-servo
Copy link
Contributor

bors-servo commented Feb 13, 2017

Trying commit b6cf998 with merge d0acb0c...

bors-servo added a commit that referenced this pull request Feb 13, 2017
style: Unbox a bunch of color properties.

This builds on servo/rust-cssparser#118.

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

bors-servo commented Feb 13, 2017

💔 Test failed - linux-rel-wpt

@emilio
Copy link
Member Author

emilio commented Feb 13, 2017

@emilio
Copy link
Member Author

emilio commented Feb 14, 2017

Ouch, these are legit, even though I believe we should be using the stored value? huh.

  ▶ Unexpected subtest result in /cssom/serialize-values.html:
  │ FAIL [expected PASS] background-color: rgba(5, 7, 10, 0.5)
  │   → assert_equals: background-color raw inline style declaration expected "rgba(5, 7, 10, 0.5)" but got "rgba(5, 7, 10, 0.502)"
  │ FAIL [expected PASS] border-top-color: rgba(5, 7, 10, 0.5)
  │   → assert_equals: border-top-color raw inline style declaration expected "rgba(5, 7, 10, 0.5)" but got "rgba(5, 7, 10, 0.502)"
  │ FAIL [expected PASS] border-right-color: rgba(5, 7, 10, 0.5)
  │   → assert_equals: border-right-color raw inline style declaration expected "rgba(5, 7, 10, 0.5)" but got "rgba(5, 7, 10, 0.502)"
  │ FAIL [expected PASS] border-bottom-color: rgba(5, 7, 10, 0.5)
  │   → assert_equals: border-bottom-color raw inline style declaration expected "rgba(5, 7, 10, 0.5)" but got "rgba(5, 7, 10, 0.502)"
  │ FAIL [expected PASS] border-left-color: rgba(5, 7, 10, 0.5)
  │   → assert_equals: border-left-color raw inline style declaration expected "rgba(5, 7, 10, 0.5)" but got "rgba(5, 7, 10, 0.502)"
  │ FAIL [expected PASS] color: rgba(5, 7, 10, 0.5)
  │   → assert_equals: color raw inline style declaration expected "rgba(5, 7, 10, 0.5)" but got "rgba(5, 7, 10, 0.502)"
  │ FAIL [expected PASS] outline-color: rgba(5, 7, 10, 0.5)
  │   → assert_equals: outline-color raw inline style declaration expected "rgba(5, 7, 10, 0.5)" but got "rgba(5, 7, 10, 0.502)"
  │ 
  │ run_individual_test/<@http://web-platform.test:8000/cssom/serialize-values.html:234:9
  │ Test.prototype.step@http://web-platform.test:8000/resources/testharness.js:1409:20
  │ run_individual_test@http://web-platform.test:8000/cssom/serialize-values.html:230:7
  │ test_property@http://web-platform.test:8000/cssom/serialize-values.html:245:14
  └ @http://web-platform.test:8000/cssom/serialize-values.html:593:7
@emilio
Copy link
Member Author

emilio commented Feb 14, 2017

Of course those properties still use cssparser::Color instead of the one that preserves serialization, sigh.

@emilio
Copy link
Member Author

emilio commented Feb 14, 2017

Hhmm... Or maybe I'm wrong, huh

@emilio
Copy link
Member Author

emilio commented Feb 14, 2017

@emilio
Copy link
Member Author

emilio commented Feb 14, 2017

@SimonSapin, Gecko rounds a * 255 instead of flooring a * 256. I believe our behavior is better though, but worth keeping in mind. cc @upsuper.

@SimonSapin
Copy link
Member

SimonSapin commented Feb 14, 2017

Interesting. I do think we should copy Gecko’s behavior of rounding to 2 decimal digits if that round-trips to the same u8 value, falling back to 3 if not. Wanna do the cssparser change?

@SimonSapin
Copy link
Member

SimonSapin commented Feb 14, 2017

To be clear, the above is only for serialization, not for gfx APIs that use floats.

@emilio
Copy link
Member Author

emilio commented Feb 14, 2017

Sure, makes sense

@emilio
Copy link
Member Author

emilio commented Feb 14, 2017

@emilio emilio force-pushed the emilio:color branch from 55a060c to 6e9967d Feb 14, 2017
@emilio
Copy link
Member Author

emilio commented Feb 14, 2017

@bors-servo
Copy link
Contributor

bors-servo commented Feb 14, 2017

Trying commit 6e9967d with merge d27079a...

bors-servo added a commit that referenced this pull request Feb 14, 2017
style: Unbox a bunch of color properties.

This builds on servo/rust-cssparser#118.

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

bors-servo commented Feb 14, 2017

💔 Test failed - linux-rel-wpt

@SimonSapin
Copy link
Member

SimonSapin commented Feb 14, 2017

  ▶ CRASH [expected OK] /navigation-timing/nav2_test_open_data_uri.html
  │ 
  │ VMware, Inc.
  │ Gallium 0.4 on softpipe
  │ 3.3 (Core Profile) Mesa 12.0.1
  │ ERROR:script::dom::bindings::error: Error at data:text/html;charset=utf-8,%3C%21DOCTYPE%20html%3E%0D%0A%3Ctitle%3Edata%20URL%20source%20for%20navigation-timing%2Fnav2_test_open_data_uri.html%3C%2Ftitle%3E%0D%0A%3C%21--%20NB%3A%20this%20file%20isn%27t%20actually%20used%20any%20where%21%20--%3E%0D%0A%3Clink%20rel%3D%22author%22%20title%3D%22Google%22%20href%3D%22http%3A%2F%2Fwww.google.com%2F%22%20%2F%3E%0D%0A%3Cscript%3E%0D%0Avar%20observer%20%3D%20new%20PerformanceObserver%28%0D%0A%20%20%20%20function%20%28entryList%29%20%7B%0D%0A%20%20%20%20%20%20%20%20parent.postMessage%28%22observed%22%2C%20%22%2A%22%29%3B%0D%0A%20%20%20%20%20%20%20%20observer.disconnect%28%29%3B%0D%0A%20%20%20%20%7D%29%3B%0D%0Aobserver.observe%28%7BentryTypes%3A%20%5B%22navigation%22%5D%7D%29%3B%0D%0A%3C%2Fscript%3E%0D%0A:6:5 PerformanceObserver is not defined
  │ called `Result::unwrap()` on an `Err` value: Error { repr: Custom(Custom { kind: UnexpectedEof, error: StringError("failed to fill whole buffer") }) } (thread RenderBackend, at /buildslave/rust-buildbot/slave/nightly-dist-rustc-linux/build/src/libcore/result.rs:868)
  │ stack backtrace:
  │    0:     0x7fb6b65ebd7d - backtrace::backtrace::trace::h976012eed768c7ff
  │    1:     0x7fb6b65ec262 - backtrace::capture::Backtrace::new::hb5a725a088a2a2fc
  │    2:     0x7fb6b4f64617 - servo::main::{{closure}}::h039ba3c803113243
  │    3:     0x7fb6b6dd06f8 - std::panicking::rust_panic_with_hook
  │                         at /buildslave/rust-buildbot/slave/nightly-dist-rustc-linux/build/src/libstd/panicking.rs:556
  │    4:     0x7fb6b6dd05c4 - std::panicking::begin_panic<collections::string::String>
  │                         at /buildslave/rust-buildbot/slave/nightly-dist-rustc-linux/build/src/libstd/panicking.rs:517
  │    5:     0x7fb6b6dd04e9 - std::panicking::begin_panic_fmt
  │                         at /buildslave/rust-buildbot/slave/nightly-dist-rustc-linux/build/src/libstd/panicking.rs:501
  │    6:     0x7fb6b6dd0477 - std::panicking::rust_begin_panic
  │                         at /buildslave/rust-buildbot/slave/nightly-dist-rustc-linux/build/src/libstd/panicking.rs:477
  │    7:     0x7fb6b6dfdc3d - core::panicking::panic_fmt
  │                         at /buildslave/rust-buildbot/slave/nightly-dist-rustc-linux/build/src/libcore/panicking.rs:69
  │    8:     0x7fb6b66349cd - core::result::unwrap_failed::h4e1dafabda202b02
  │    9:     0x7fb6b6689a9e - webrender::render_backend::RenderBackend::run::h806d46e08283bc83
  │   10:     0x7fb6b66263dc - std::panicking::try::do_call::h7d57ef86fec8f13f
  │   11:     0x7fb6b6dd763a - panic_unwind::__rust_maybe_catch_panic
  │                         at /buildslave/rust-buildbot/slave/nightly-dist-rustc-linux/build/src/libpanic_unwind/lib.rs:98
  │   12:     0x7fb6b663dfc7 - <F as alloc::boxed::FnBox<A>>::call_box::h232db978e3695021
  │   13:     0x7fb6b6dcf384 - alloc::boxed::{{impl}}::call_once<(),()>
  │                         at /buildslave/rust-buildbot/slave/nightly-dist-rustc-linux/build/src/liballoc/boxed.rs:623
  │                          - std::sys_common::thread::start_thread
  │                         at /buildslave/rust-buildbot/slave/nightly-dist-rustc-linux/build/src/libstd/sys_common/thread.rs:21
  │                          - std::sys::imp::thread::{{impl}}::new::thread_start
  │                         at /buildslave/rust-buildbot/slave/nightly-dist-rustc-linux/build/src/libstd/sys/unix/thread.rs:84
  │   14:     0x7fb6b2e32183 - start_thread
  │   15:     0x7fb6b294937c - clone
  │   16:                0x0 - <unknown>
  │ ERROR:servo: called `Result::unwrap()` on an `Err` value: Error { repr: Custom(Custom { kind: UnexpectedEof, error: StringError("failed to fill whole buffer") }) }
  └ Pipeline failed in hard-fail mode.  Crashing!
@emilio
Copy link
Member Author

emilio commented Feb 14, 2017

That is #14323, the rest seems green

This builds on servo/rust-cssparser#118.
@emilio emilio force-pushed the emilio:color branch from 6e9967d to 0c102e2 Feb 14, 2017
@SimonSapin
Copy link
Member

SimonSapin commented Feb 14, 2017

@bors-servo r+


Reviewed 19 of 19 files at r4.
Review status: all files reviewed at latest revision, all discussions resolved.


Comments from Reviewable

@bors-servo
Copy link
Contributor

bors-servo commented Feb 14, 2017

📌 Commit 0c102e2 has been approved by SimonSapin

@bors-servo
Copy link
Contributor

bors-servo commented Feb 14, 2017

Testing commit 0c102e2 with merge 357bf3b...

bors-servo added a commit that referenced this pull request Feb 14, 2017
style: Unbox a bunch of color properties.

This builds on servo/rust-cssparser#118.

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

bors-servo commented Feb 14, 2017

☀️ Test successful - android, arm32, arm64, linux-dev, linux-rel-css, linux-rel-wpt, mac-dev-unit, mac-rel-css, mac-rel-wpt1, mac-rel-wpt2, windows-gnu-dev, windows-msvc-dev
Approved by: SimonSapin
Pushing 357bf3b to master...

@bors-servo bors-servo merged commit 0c102e2 into servo:master Feb 14, 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
Linked issues

Successfully merging this pull request may close these issues.

None yet

6 participants
You can’t perform that action at this time.