style: Stop using PseudoElement::inherits_all. #21946

emilio · 2018-10-14T22:40:08Z

This was done that way just because Servo didn't support the all property at
the time.

We should do it this way and optimize it if it's slow. Though I suspect that
most of stuff doesn't actually need to be inherited, my patch at bug 1498943
should make it much faster than what it would otherwise be.

This change is

highfive · 2018-10-14T22:40:13Z

Warning

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

emilio · 2018-10-14T22:40:28Z

r? @jdm or @SimonSapin or @nox or @mbrubeck

emilio · 2018-10-14T22:42:57Z

@bors-servo try

bors-servo · 2018-10-14T22:43:02Z

⌛ Trying commit 5dac6a6 with merge 4d4731a...


        Auto merge of #21946 - emilio:remove-inherits-all, r=<try>

style: Stop using PseudoElement::inherits_all. This was done that way just because Servo didn't support the `all` property at the time. We should do it this way and optimize it if it's slow. Though I suspect that most of stuff doesn't actually need to be inherited, my patch at bug 1498943 should make it much faster than what it would otherwise be.  --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/21946)

bors-servo · 2018-10-14T23:03:34Z

💔 Test failed - linux-rel-css


        style: Stop using PseudoElement::inherits_all.

This was done that way just because Servo didn't support the `all` property at the time. We should do it this way and optimize it if it's slow. Though I suspect that most of stuff doesn't actually need to be inherited, my patch at bug 1498943 should make it much faster than what it would otherwise be.


        style: Remove useless declarations in servo.css.

Margin is a reset property, there's no point in setting it to zero since it's zero by default.

emilio · 2018-10-15T00:18:08Z

Pfft, of course there was a typo in the CSS parser. This should be green, we should re-enable the commented-out part in a followup.

@bors-servo try

bors-servo · 2018-10-15T00:18:12Z

⌛ Trying commit 6d7d1e5 with merge 8873fa3...


        Auto merge of #21946 - emilio:remove-inherits-all, r=<try>

style: Stop using PseudoElement::inherits_all. This was done that way just because Servo didn't support the `all` property at the time. We should do it this way and optimize it if it's slow. Though I suspect that most of stuff doesn't actually need to be inherited, my patch at bug 1498943 should make it much faster than what it would otherwise be.  --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/21946)

bors-servo · 2018-10-15T00:53:42Z

💔 Test failed - mac-rel-wpt2

emilio · 2018-10-15T01:03:48Z

That one looks unrelated and similar to #21930.

@bors-servo retry

bors-servo · 2018-10-15T01:03:52Z

⌛ Trying commit 6d7d1e5 with merge 0653e32...


        Auto merge of #21946 - emilio:remove-inherits-all, r=<try>

style: Stop using PseudoElement::inherits_all. This was done that way just because Servo didn't support the `all` property at the time. We should do it this way and optimize it if it's slow. Though I suspect that most of stuff doesn't actually need to be inherited, my patch at bug 1498943 should make it much faster than what it would otherwise be.  --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/21946)

bors-servo · 2018-10-15T01:43:38Z

💔 Test failed - mac-rel-wpt2

CYBAI · 2018-10-15T02:27:47Z

@bors-servo retry

the failures look not related

{"status": "NOTRUN", "group": "default", "message": null, "stack": null, "subtest": "Overall test", "test": "/_webgl/conformance/textures/misc/origin-clean-conformance-offscreencanvas.html", "line": 30546, "action": "test_result", "expected": "PASS"}
{"status": "TIMEOUT", "group": "default", "message": null, "stack": null, "subtest": null, "test": "/_webgl/conformance/textures/misc/origin-clean-conformance-offscreencanvas.html", "line": 30547, "action": "test_result", "expected": "OK"}

bors-servo · 2018-10-15T02:27:52Z

⌛ Trying commit 6d7d1e5 with merge c4e32a6...


        Auto merge of #21946 - emilio:remove-inherits-all, r=<try>

style: Stop using PseudoElement::inherits_all. This was done that way just because Servo didn't support the `all` property at the time. We should do it this way and optimize it if it's slow. Though I suspect that most of stuff doesn't actually need to be inherited, my patch at bug 1498943 should make it much faster than what it would otherwise be.  --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/21946)

bors-servo · 2018-10-15T02:55:58Z

💔 Test failed - linux-rel-wpt

emilio · 2018-10-15T03:03:44Z

Yeah, more WebGL failures and such, I'm pretty sure this should be good to go.

nox · 2018-10-15T08:20:52Z

components/style/servo/selector_parser.rs

-    /// properties...  Also, I guess it just could do all: inherit on the
-    /// stylesheet, though chances are that'd be kinda slow if we don't cache
-    /// them...
+    /// To be removed.


Why do we keep it then?

Because I'm removing it in that Gecko bug I point above, which I'll sink in a few..

nox · 2018-10-15T12:32:27Z

@bors-servo r+

bors-servo · 2018-10-15T12:32:28Z

📌 Commit 6d7d1e5 has been approved by nox

bors-servo · 2018-10-15T13:19:08Z

💔 Test failed - mac-rel-wpt4

emilio · 2018-10-15T13:53:22Z

@bors-servo retry

git update failed?

bors-servo · 2018-10-15T13:53:28Z

⌛ Testing commit 6d7d1e5 with merge 2bc0867...


        Auto merge of #21946 - emilio:remove-inherits-all, r=nox

style: Stop using PseudoElement::inherits_all. This was done that way just because Servo didn't support the `all` property at the time. We should do it this way and optimize it if it's slow. Though I suspect that most of stuff doesn't actually need to be inherited, my patch at bug 1498943 should make it much faster than what it would otherwise be.  --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/21946)

bors-servo · 2018-10-15T14:13:21Z

💔 Test failed - linux-rel-css

emilio · 2018-10-15T14:50:53Z

@bors-servo retry

bors-servo · 2018-10-15T15:06:22Z

⚡ Previous build results for android, android-x86, arm32, arm64, linux-dev, linux-rel-wpt, mac-dev-unit, mac-rel-css1, mac-rel-css2, status-taskcluster, windows-msvc-dev are reusable. Rebuilding only linux-rel-css, mac-rel-wpt1, mac-rel-wpt2, mac-rel-wpt3, mac-rel-wpt4...

emilio · 2018-10-15T15:40:59Z

@bors-servo retry

  ▶ CRASH [expected PASS] /css/css-flexbox/flexbox_flex-0-1-N.html
  │ 
  │ VMware, Inc.
  │ softpipe
  │ 3.3 (Core Profile) Mesa 18.1.0-devel
  │ Shutting down the Constellation after generating an output file or exit flag specified
  │ mozilla::detail::MutexImpl::lock: pthread_mutex_lock failed: Invalid argument
  │ VMware, Inc.
  │ softpipe
  │ 3.3 (Core Profile) Mesa 18.1.0-devel
  │ called `Result::unwrap()` on an `Err` value: Io(Custom { kind: ConnectionReset, error: StringError("All senders for this socket closed") }) (thread main, at libcore/result.rs:1009)
  │ stack backtrace:
  │    0:     0x7f740ee95bbc - backtrace::backtrace::trace::h34a5004b5e0f1dc7
  │    1:     0x7f740ee94e52 - <backtrace::capture::Backtrace as core::default::Default>::default::hae829d87a5db9150
  │    2:     0x7f740ee94ec8 - backtrace::capture::Backtrace::new::hcbbf9ea3df342ffc
  │    3:     0x7f740be72309 - servo::main::{{closure}}::h48341614c6b12575
  │    4:     0x7f740eec11f3 - std::panicking::rust_panic_with_hook::hd470ff3b3e7a1b18
  │                         at libstd/panicking.rs:480
  │    5:     0x7f740eec0d59 - std::panicking::continue_panic_fmt::hf0cf39ae1e114602
  │                         at libstd/panicking.rs:390
  │    6:     0x7f740eec0c55 - rust_begin_unwind
  │                         at libstd/panicking.rs:325
  │    7:     0x7f740eee1e5b - core::panicking::panic_fmt::hf53111ba26d35bb0
  │                         at libcore/panicking.rs:77
  │    8:     0x7f740e28b38d - core::result::unwrap_failed::h83bba07137c7fd88
  │    9:     0x7f740e26ffdd - webrender_api::api::RenderApi::get_scroll_node_state::h32b72beb5e0ecf94
  │   10:     0x7f740be1e0b0 - <compositing::compositor::IOCompositor<Window>>::send_viewport_rects::h2769fe1f6baf9c0f
  │   11:     0x7f740be1f91a - _ZN60_$LT$compositing..compositor..IOCompositor$LT$Window$GT$$GT$22handle_browser_message17hddc1adf5e99d47e6E.llvm.12822754012853505132
  │   12:     0x7f740be1c6a1 - <compositing::compositor::IOCompositor<Window>>::receive_messages::hdbc3f9520105790f
  │   13:     0x7f740be5df3d - <servo::Servo<Window>>::handle_events::h4a5ee2d439557967
  │   14:     0x7f740be3d52a - servo::main::{{closure}}::h283e9286ec794512
  │   15:     0x7f740be3949b - servo::glutin_app::window::Window::run::h32ec67ed3ee68147
  │   16:     0x7f740be71cba - servo::main::h5c0c7e5c5d3323c0
  │   17:     0x7f740be30382 - std::rt::lang_start::{{closure}}::h2b24428e26453900
  │   18:     0x7f740eec0bf2 - std::rt::lang_start_internal::{{closure}}::h444bf05eb286eca1
  │                         at libstd/rt.rs:59
  │                          - std::panicking::try::do_call::hb527d54965a8815a
  │                         at libstd/panicking.rs:310
  │   19:     0x7f740eed3069 - __rust_maybe_catch_panic
  │                         at libpanic_unwind/lib.rs:102
  │   20:     0x7f740eead295 - std::panicking::try::h83b5294aab121d1a
  │                         at libstd/panicking.rs:289
  │                          - std::panic::catch_unwind::h15c1ea783c7910d5
  │                         at libstd/panic.rs:392
  │                          - std::rt::lang_start_internal::hcc3e9196a926118d
  │                         at libstd/rt.rs:58
  │   21:     0x7f740be724d3 - main
  │   22:     0x7f740832ef44 - __libc_start_main
  │   23:     0x7f740bdecf06 - <unknown>
  │   24:                0x0 - <unknown>

Does anybody know what that is about?

bors-servo · 2018-10-15T15:41:02Z

⚡ Previous build results for android, android-x86, 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-wpt4, status-taskcluster, windows-msvc-dev are reusable. Rebuilding only mac-rel-wpt3...

bors-servo · 2018-10-15T15:42:42Z

☀️ Test successful - android, android-x86, 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, status-taskcluster, windows-msvc-dev
Approved by: nox
Pushing 2bc0867 to master...


        Bug 1500260 - Stop using PseudoElement::inherits_all. r=emilio

This was done that way just because Servo didn't support the `all` property at the time. We should do it this way and optimize it if it's slow. Though I suspect that most of stuff doesn't actually need to be inherited, my patch at bug 1498943 should make it much faster than what it would otherwise be. This cherry-picks servo/servo#21946.


        style: Re-enable the rule that was accidentally disabled by a typo in…

… the CSS parser. In servo#21946 I commented out this rule because before that patch we were accidentally ignoring it because of a typo in the CSS parser, since that PR didn't intend to change behavior. This PR does though, and re-enables the rule.


        Auto merge of #21983 - emilio:inline-absolute, r=<try>

style: Re-enable the rule that was accidentally disabled by a typo in the CSS parser. In #21946 I commented out this rule because before that patch we were accidentally ignoring it because of a typo in the CSS parser, since that PR didn't intend to change behavior. This PR does though, and re-enables the rule.  --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/21983)


        Bug 1500260 - Stop using PseudoElement::inherits_all. r=emilio

This was done that way just because Servo didn't support the `all` property at the time. We should do it this way and optimize it if it's slow. Though I suspect that most of stuff doesn't actually need to be inherited, my patch at bug 1498943 should make it much faster than what it would otherwise be. This cherry-picks servo/servo#21946.


        Auto merge of #21983 - emilio:inline-absolute, r=jdm

style: Re-enable the rule that was accidentally disabled by a typo in the CSS parser. In #21946 I commented out this rule because before that patch we were accidentally ignoring it because of a typo in the CSS parser, since that PR didn't intend to change behavior. This PR does though, and re-enables the rule.  --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/21983)


        Auto merge of #21983 - emilio:inline-absolute, r=jdm

style: Re-enable the rule that was accidentally disabled by a typo in the CSS parser. In #21946 I commented out this rule because before that patch we were accidentally ignoring it because of a typo in the CSS parser, since that PR didn't intend to change behavior. This PR does though, and re-enables the rule.  --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/21983)


        Auto merge of #21983 - emilio:inline-absolute, r=jdm

style: Re-enable the rule that was accidentally disabled by a typo in the CSS parser. In #21946 I commented out this rule because before that patch we were accidentally ignoring it because of a typo in the CSS parser, since that PR didn't intend to change behavior. This PR does though, and re-enables the rule.  --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/21983)


        Auto merge of #21983 - emilio:inline-absolute, r=jdm

style: Re-enable the rule that was accidentally disabled by a typo in the CSS parser. In #21946 I commented out this rule because before that patch we were accidentally ignoring it because of a typo in the CSS parser, since that PR didn't intend to change behavior. This PR does though, and re-enables the rule.  --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/21983)


        Bug 1500260 - Stop using PseudoElement::inherits_all. r=emilio

This was done that way just because Servo didn't support the `all` property at the time. We should do it this way and optimize it if it's slow. Though I suspect that most of stuff doesn't actually need to be inherited, my patch at bug 1498943 should make it much faster than what it would otherwise be. This cherry-picks servo/servo#21946. UltraBlame original commit: 477c044daea3a96ba8ec41641ab934f9ec11deb7


        Bug 1500260 - Stop using PseudoElement::inherits_all. r=emilio

This was done that way just because Servo didn't support the `all` property at the time. We should do it this way and optimize it if it's slow. Though I suspect that most of stuff doesn't actually need to be inherited, my patch at bug 1498943 should make it much faster than what it would otherwise be. This cherry-picks servo/servo#21946. UltraBlame original commit: 477c044daea3a96ba8ec41641ab934f9ec11deb7


        Bug 1500260 - Stop using PseudoElement::inherits_all. r=emilio

This was done that way just because Servo didn't support the `all` property at the time. We should do it this way and optimize it if it's slow. Though I suspect that most of stuff doesn't actually need to be inherited, my patch at bug 1498943 should make it much faster than what it would otherwise be. This cherry-picks servo/servo#21946. UltraBlame original commit: 477c044daea3a96ba8ec41641ab934f9ec11deb7

highfive assigned jdm Oct 14, 2018

highfive added the S-awaiting-review label Oct 14, 2018

highfive added the S-tests-failed label Oct 14, 2018

emilio added 2 commits Oct 14, 2018

emilio force-pushed the emilio:remove-inherits-all branch from 5dac6a6 to 6d7d1e5 Oct 15, 2018

highfive removed the S-tests-failed label Oct 15, 2018

highfive added the S-tests-failed label Oct 15, 2018

nox reviewed Oct 15, 2018

View changes

highfive added S-tests-failed and removed S-awaiting-merge labels Oct 15, 2018

highfive added S-awaiting-merge and removed S-tests-failed labels Oct 15, 2018

highfive added S-tests-failed and removed S-awaiting-merge labels Oct 15, 2018

bors-servo merged commit 6d7d1e5 into servo:master Oct 15, 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

emilio deleted the emilio:remove-inherits-all branch Oct 15, 2018

emilio mentioned this pull request Oct 18, 2018

style: Re-enable the rule that was accidentally disabled by a typo in the CSS parser. #21983

Merged

servo / servo

style: Stop using PseudoElement::inherits_all. #21946

style: Stop using PseudoElement::inherits_all. #21946

emilio commented Oct 14, 2018 •

edited by SimonSapin

highfive commented Oct 14, 2018

emilio commented Oct 14, 2018

emilio commented Oct 14, 2018

bors-servo commented Oct 14, 2018

bors-servo commented Oct 14, 2018

emilio commented Oct 15, 2018

bors-servo commented Oct 15, 2018

bors-servo commented Oct 15, 2018

emilio commented Oct 15, 2018

bors-servo commented Oct 15, 2018

bors-servo commented Oct 15, 2018

CYBAI commented Oct 15, 2018

bors-servo commented Oct 15, 2018

bors-servo commented Oct 15, 2018

emilio commented Oct 15, 2018

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

nox commented Oct 15, 2018

bors-servo commented Oct 15, 2018

bors-servo commented Oct 15, 2018

emilio commented Oct 15, 2018

bors-servo commented Oct 15, 2018

bors-servo commented Oct 15, 2018

emilio commented Oct 15, 2018

bors-servo commented Oct 15, 2018

emilio commented Oct 15, 2018

bors-servo commented Oct 15, 2018

bors-servo commented Oct 15, 2018

servo / servo

Join GitHub today

style: Stop using PseudoElement::inherits_all. #21946

style: Stop using PseudoElement::inherits_all. #21946

Conversation

emilio commented Oct 14, 2018 • edited by SimonSapin

highfive commented Oct 14, 2018

emilio commented Oct 14, 2018

emilio commented Oct 14, 2018

bors-servo commented Oct 14, 2018

bors-servo commented Oct 14, 2018

emilio commented Oct 15, 2018

bors-servo commented Oct 15, 2018

bors-servo commented Oct 15, 2018

emilio commented Oct 15, 2018

bors-servo commented Oct 15, 2018

bors-servo commented Oct 15, 2018

CYBAI commented Oct 15, 2018

bors-servo commented Oct 15, 2018

bors-servo commented Oct 15, 2018

emilio commented Oct 15, 2018

This comment has been minimized.

nox Oct 15, 2018

This comment has been minimized.

emilio Oct 15, 2018

This comment has been minimized.

emilio Oct 15, 2018

nox commented Oct 15, 2018

bors-servo commented Oct 15, 2018

bors-servo commented Oct 15, 2018

emilio commented Oct 15, 2018

bors-servo commented Oct 15, 2018

bors-servo commented Oct 15, 2018

emilio commented Oct 15, 2018

bors-servo commented Oct 15, 2018

emilio commented Oct 15, 2018

bors-servo commented Oct 15, 2018

bors-servo commented Oct 15, 2018

emilio commented Oct 14, 2018 •

edited by SimonSapin