style: Add infrastructure to support lazy pseudo-elements #10934

emilio · 2016-04-29T21:02:54Z

This builds on top of #10815, so it's really just the last commit the one that should be reviewed.

I tried to apply the new infrastructure to servo, but failed (for now?).

The problem with it is that it'd require ThreadSafeLayoutElement to implement selectors::Element, which is a lot of work and might be racy (not totally sure about it though). Thus, I prefered to keep selectors eager until knowing that it's safe to do it.

r? @mbrubeck for style changes, @bholley for the geckolib changes (minimal for now, glue + a list of lazy PEs must be added)

This change is

highfive · 2016-04-29T21:03:28Z

Heads up! This PR modifies the following files:

@bholley: components/style/selector_matching.rs, components/style/servo.rs, components/style/traversal.rs, components/style/matching.rs, components/style/media_queries.rs, components/style/restyle_hints.rs, components/style/dom.rs, components/style/context.rs, components/style/selector_impl.rs

highfive · 2016-04-29T21:03:29Z

Warning

These commits modify unsafe code. Please review it carefully!

emilio · 2016-04-29T21:04:06Z

components/layout/wrapper.rs

+                    PseudoElementCascadeType::Lazy => {
+                        panic!("Lazy pseudo-elements can't be used in Servo \
+                               since accessing the DOM tree during layout \
+                               could be unsafe.")


Not totally sure about this, @mbrubeck, do you know if it'd be safe for ServoThreadSafeLayoutElement to implement selectors::Element? (that is, getting parent, siblings...)

I'm pretty sure it would not be, but I'll let @mbrubeck confirm. Might need to think a bit on how this fits into the Servo model.

Right, it is unsafe for a ThreadSafeLayoutNode to access its parents or siblings.

We could still use a subset of selector-matching though (attributes + tag name at least), enough for avoiding doing the whole cascade for some private pseudos, like ::-servo-details-summary, which only needs access to the details tag and the open attribute.

Would this be desirable (with appropriate docs of course), or is it just a lot of overhead? I think it can be worth it but I'd like to know your opinion.

That seems reasonable to me; I guess all it would require is an impl of Element that just returns None for things like parent_element.

Yup, just did that and cleaned it up a bit, feel free to re-review the changes :)

bholley · 2016-04-29T21:10:25Z

ports/geckolib/selector_impl.rs

+            MozTreeDropFeedback | MozSVGMarkerAnonChild |
+            MozSVGOuterSVGAnonChild | MozSVGForeignContent |
+            MozSVGText => true,
+            _ => false,


Seems like we should make this an ADT?

bholley · 2016-04-29T21:10:50Z

geckolib changes look good modulo that one change.

mbrubeck · 2016-04-29T23:26:48Z

components/layout/wrapper.rs

-/// A wrapper around elements that ensures layout can only ever access safe properties and cannot
-/// race on elements.
+/// A wrapper around elements that ensures layout can only
+/// ever access safe properties and cannot race on elements.


Nit: Rust style is to wrap at 99 columns. I don't care too deeply about exactly where comments wrap, but if you can configure your editor not to re-format things to 80 columns, it would make reviews a little easier.

mbrubeck · 2016-04-29T23:40:12Z

components/layout/wrapper.rs

+                        //     data.style_data.per_pseudo
+                        //         .insert(style_pseudo.clone(), new_style.unwrap())
+                        // }
+                    }


Please remove the commented-out code before merging.

mbrubeck · 2016-04-29T23:45:50Z

The Servo changes are good; r=mbrubeck with the commented-out code removed. Note: I left a couple of other review comments but then deleted them because they were incorrect. (This is what I get for using GitHub comments instead of Reviewable.) Please disregard any comments that no longer appear in the GitHub UI.

notriddle · 2016-04-30T01:37:08Z

Now that #10815 is merged, should this commit be rebased on top of it (so that GitHub only lists one commit for this PR)?

bholley · 2016-04-30T02:16:16Z

Yes. I'm guessing @emilio will do that sometime when he's awake. :-)

highfive · 2016-05-03T17:01:44Z

New code was committed to pull request.


        style: Support lazy pseudo-elements

These can't be supported in Servo as of right now, because I'm not totally sure the accesses that should be done in layout would be thread-safe. It can be revisited later though.


        geckolib: Make anon-box PEs an ADT.

highfive · 2016-05-03T17:02:10Z

New code was committed to pull request.

highfive · 2016-05-03T17:27:21Z

New code was committed to pull request.

mbrubeck · 2016-05-03T18:22:14Z

-S-awaiting-review +S-needs-code-changes

Reviewed 6 of 6 files at r1, 2 of 11 files at r2, 3 of 7 files at r19, 3 of 6 files at r23, 3 of 3 files at r24.
Review status: all files reviewed at latest revision, 6 unresolved discussions.

components/layout/wrapper.rs, line 1360 [r24] (raw file):
Is it a programming error if this is ever called? If so, maybe change these from warn! to panic! or debug_assert!(false, ...).

components/style/selector_impl.rs, line 117 [r24] (raw file):
This TODO comment is done. \o/ (A note about ::selection would still be appropriate.)

Comments from Reviewable


        layout: use `debug!` instead of `warn!` for the expected ThreadSafeLa…

…youtElement methods


        docs: Ammend the lazy pseudo-element section to be clearer

Thanks to Simon for the feedback :)


        geckolib: Implement Servo_GetComputedValuesForPseudoElement

highfive · 2016-05-03T22:44:04Z

New code was committed to pull request.

emilio · 2016-05-03T22:46:08Z

Ok, rebased, typo fixed, and implemented Servo_GetComputedValuesForPseudoElement for stylo.

r? @bholley (for the stylo changes, the rest is reviewed)

Review status: 9 of 13 files reviewed at latest revision, 3 unresolved discussions.

docs/components/style.md, line 126 [r26] (raw file):
Done.

Comments from Reviewable

bholley · 2016-05-04T00:02:09Z

ports/geckolib/glue.rs

+        }
+    };
+
+    let pseudo = match pseudo_element_from_atom(pseudo_tag, /* ua_stylesheet = */ true) {


We'll want to pass the appropriate value here from the caller right? I'll add a TODO.

bholley · 2016-05-04T00:04:40Z

Looks great!

@bors-servo r=bholley,mbrubeck

bors-servo · 2016-05-04T00:04:43Z

📌 Commit 0f7b70c has been approved by bholley,mbrubeck

bors-servo · 2016-05-04T01:38:38Z

⌛ Testing commit 0f7b70c with merge 04012c1...


        Auto merge of #10934 - emilio:other-gecko-pseudos, r=bholley,mbrubeck

@mbrubeck

style: Add infrastructure to support lazy pseudo-elements This builds on top of #10815, so it's really just the last commit the one that should be reviewed. I tried to apply the new infrastructure to servo, but failed (for now?). The problem with it is that it'd require `ThreadSafeLayoutElement` to implement `selectors::Element`, which is a lot of work and might be racy (not totally sure about it though). Thus, I prefered to keep selectors eager until knowing that it's safe to do it. r? @mbrubeck for style changes, @bholley for the geckolib changes (minimal for now, glue + a list of lazy PEs must be added)  --- This change is [<img src="https://reviewable.io/review_button.svg" height="35" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/10934)

bors-servo · 2016-05-04T02:40:07Z

💔 Test failed - linux-rel

highfive · 2016-05-04T02:40:09Z

  ▶ TIMEOUT [expected OK] /html/rendering/replaced-elements/svg-embedded-sizing/svg-in-iframe-auto.html
  │ 
  └ Xlib:  extension &#34;XFree86-VidModeExtension&#34; missing on display &#34;:0&#34;.

  ▶ Unexpected subtest result in /html/rendering/replaced-elements/svg-embedded-sizing/svg-in-iframe-auto.html:
  │ TIMEOUT [expected PASS] placeholder: &#39;iframe&#39;, containerWidthStyle: &#39;400px&#39;, containerHeightStyle: &#39;400px&#39;, placeholderWidthAttr: &#39;100&#39;, svgViewBoxAttr: &#39;0 0 100 200&#39;, svgWidthAttr: &#39;25%&#39;, svgHeightAttr: &#39;25%&#39;, 
  └   → Test timed out

  ▶ TIMEOUT [expected OK] /html/rendering/replaced-elements/svg-embedded-sizing/svg-in-iframe-fixed.html
  │ 
  └ Xlib:  extension &#34;XFree86-VidModeExtension&#34; missing on display &#34;:0&#34;.

  ▶ Unexpected subtest result in /html/rendering/replaced-elements/svg-embedded-sizing/svg-in-iframe-fixed.html:
  │ TIMEOUT [expected PASS] placeholder: &#39;iframe&#39;, containerHeightStyle: &#39;400px&#39;, placeholderWidthAttr: &#39;100&#39;, placeholderHeightAttr: &#39;100px&#39;, svgViewBoxAttr: &#39;0 0 100 200&#39;, svgWidthAttr: &#39;25%&#39;, svgHeightAttr: &#39;25%&#39;, 
  └   → Test timed out

  ▶ TIMEOUT [expected OK] /html/rendering/replaced-elements/svg-embedded-sizing/svg-in-iframe-percentage.html
  │ 
  └ Xlib:  extension &#34;XFree86-VidModeExtension&#34; missing on display &#34;:0&#34;.

  ▶ Unexpected subtest result in /html/rendering/replaced-elements/svg-embedded-sizing/svg-in-iframe-percentage.html:
  │ TIMEOUT [expected PASS] placeholder: &#39;iframe&#39;, containerHeightStyle: &#39;400px&#39;, placeholderWidthAttr: &#39;100&#39;, placeholderHeightAttr: &#39;100%&#39;, svgViewBoxAttr: &#39;0 0 100 200&#39;, svgWidthAttr: &#39;25%&#39;, svgHeightAttr: &#39;25%&#39;, 
  └   → Test timed out

emilio · 2016-05-04T10:21:42Z

@bors-servo: retry

#10457

bors-servo · 2016-05-04T10:42:24Z

⌛ Testing commit 0f7b70c with merge 29823cb...


        Auto merge of #10934 - emilio:other-gecko-pseudos, r=bholley,mbrubeck

@mbrubeck

style: Add infrastructure to support lazy pseudo-elements This builds on top of #10815, so it's really just the last commit the one that should be reviewed. I tried to apply the new infrastructure to servo, but failed (for now?). The problem with it is that it'd require `ThreadSafeLayoutElement` to implement `selectors::Element`, which is a lot of work and might be racy (not totally sure about it though). Thus, I prefered to keep selectors eager until knowing that it's safe to do it. r? @mbrubeck for style changes, @bholley for the geckolib changes (minimal for now, glue + a list of lazy PEs must be added)  --- This change is [<img src="https://reviewable.io/review_button.svg" height="35" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/10934)

bors-servo · 2016-05-04T12:56:58Z

☀️ Test successful - android, arm32, arm64, linux-dev, linux-rel, mac-dev-unit, mac-rel-css, mac-rel-wpt, windows

highfive assigned mbrubeck Apr 29, 2016

highfive added the S-awaiting-review label Apr 29, 2016

emilio reviewed Apr 29, 2016
View changes

bholley reviewed Apr 29, 2016
View changes

mbrubeck reviewed Apr 29, 2016
View changes

components/layout/wrapper.rs Outdated

// data.style_data.per_pseudo

// .insert(style_pseudo.clone(), new_style.unwrap())

// }

}

This comment has been minimized.

Sign in to view

mbrubeck Apr 29, 2016
Contributor

Please remove the commented-out code before merging.

mbrubeck added S-needs-code-changes and removed S-awaiting-review labels Apr 29, 2016

emilio force-pushed the emilio:other-gecko-pseudos branch from 3c3e9ef to b1002cb Apr 30, 2016

highfive added S-awaiting-review and removed S-needs-code-changes labels Apr 30, 2016

emilio force-pushed the emilio:other-gecko-pseudos branch from b1002cb to 1446e24 Apr 30, 2016

emilio added 2 commits Apr 29, 2016

geckolib: Make anon-box PEs an ADT.

Unverified

This user has not uploaded their public key yet.

GPG key ID: 056B727BB9C1027C Learn about signing commits

18c1fee

emilio force-pushed the emilio:other-gecko-pseudos branch from cb98bac to 845b2d9 May 3, 2016

emilio force-pushed the emilio:other-gecko-pseudos branch from 845b2d9 to 1138b89 May 3, 2016

emilio changed the title ~~[WIP] style: Add infrastructure to support lazy pseudo-elements~~ style: Add infrastructure to support lazy pseudo-elements May 3, 2016

mbrubeck added S-needs-code-changes and removed S-awaiting-review labels May 3, 2016

emilio force-pushed the emilio:other-gecko-pseudos branch from 1138b89 to 1800c23 May 3, 2016

emilio added 3 commits May 3, 2016

layout: use `debug!` instead of `warn!` for the expected ThreadSafeLa…

Unverified

This user has not uploaded their public key yet.

GPG key ID: 056B727BB9C1027C Learn about signing commits

aacf67a

…youtElement methods

docs: Ammend the lazy pseudo-element section to be clearer

Unverified

This user has not uploaded their public key yet.

GPG key ID: 056B727BB9C1027C Learn about signing commits

4429318

Thanks to Simon for the feedback :)

geckolib: Implement Servo_GetComputedValuesForPseudoElement

Unverified

This user has not uploaded their public key yet.

GPG key ID: 056B727BB9C1027C Learn about signing commits

Loading status checks…

0f7b70c

emilio force-pushed the emilio:other-gecko-pseudos branch from c961f77 to 0f7b70c May 3, 2016

highfive removed the S-needs-code-changes label May 3, 2016

highfive added the S-awaiting-review label May 3, 2016

highfive assigned bholley and unassigned mbrubeck May 3, 2016

bholley reviewed May 4, 2016
View changes

ports/geckolib/glue.rs

}

};

let pseudo = match pseudo_element_from_atom(pseudo_tag, /* ua_stylesheet = */ true) {

This comment has been minimized.

Sign in to view

bholley May 4, 2016
Contributor

We'll want to pass the appropriate value here from the caller right? I'll add a TODO.

highfive added S-awaiting-merge and removed S-awaiting-review labels May 4, 2016

highfive added S-tests-failed and removed S-awaiting-merge labels May 4, 2016

highfive added S-awaiting-merge and removed S-tests-failed labels May 4, 2016

bors-servo merged commit 0f7b70c into servo:master May 4, 2016
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

highfive removed the S-awaiting-merge label May 4, 2016

emilio deleted the emilio:other-gecko-pseudos branch May 4, 2016

servo / servo

style: Add infrastructure to support lazy pseudo-elements #10934

style: Add infrastructure to support lazy pseudo-elements #10934

emilio commented Apr 29, 2016 •

edited by larsbergstrom

highfive commented Apr 29, 2016

highfive commented Apr 29, 2016

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

bholley commented Apr 29, 2016

This comment has been minimized.

This comment has been minimized.

mbrubeck commented Apr 29, 2016

notriddle commented Apr 30, 2016 •

edited

bholley commented Apr 30, 2016

highfive commented May 3, 2016

highfive commented May 3, 2016

highfive commented May 3, 2016

mbrubeck commented May 3, 2016

highfive commented May 3, 2016

emilio commented May 3, 2016

This comment has been minimized.

bholley commented May 4, 2016

bors-servo commented May 4, 2016

bors-servo commented May 4, 2016

bors-servo commented May 4, 2016

highfive commented May 4, 2016

emilio commented May 4, 2016

bors-servo commented May 4, 2016

bors-servo commented May 4, 2016

servo / servo

Join GitHub today

style: Add infrastructure to support lazy pseudo-elements #10934

style: Add infrastructure to support lazy pseudo-elements #10934

Conversation

emilio commented Apr 29, 2016 • edited by larsbergstrom

highfive commented Apr 29, 2016

highfive commented Apr 29, 2016

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

bholley commented Apr 29, 2016

This comment has been minimized.

This comment has been minimized.

mbrubeck commented Apr 29, 2016

notriddle commented Apr 30, 2016 • edited

bholley commented Apr 30, 2016

highfive commented May 3, 2016

highfive commented May 3, 2016

highfive commented May 3, 2016

mbrubeck commented May 3, 2016

highfive commented May 3, 2016

emilio commented May 3, 2016

This comment has been minimized.

bholley commented May 4, 2016

bors-servo commented May 4, 2016

bors-servo commented May 4, 2016

bors-servo commented May 4, 2016

highfive commented May 4, 2016

emilio commented May 4, 2016

bors-servo commented May 4, 2016

bors-servo commented May 4, 2016

emilio commented Apr 29, 2016 •

edited by larsbergstrom

notriddle commented Apr 30, 2016 •

edited