Expose Quirks Mode information in the layout data and code #14430

julienw · 2016-12-01T11:28:12Z

This patch exposes the Quirks (NoQuirks/LimitedQuirks/Quirks) state to the layout subsystem.

./mach build -d does not report any errors
./mach test-tidy does not report any errors
These changes fix #__ (github issue number if applicable).
Prelimary work for issue Implement line-height quirks #11704.

There are tests for these changes OR
These changes do not require tests because _____
(Waiting for guidance of where/which tests I could do here)

This change is

highfive · 2016-12-01T11:28:19Z

Thanks for the pull request, and welcome! The Servo team is excited to review your changes, and you should hear from @asajeffrey (or someone else) soon.

highfive · 2016-12-01T11:28:19Z

Heads up! This PR modifies the following files:

@KiChjang: components/script/dom/document.rs, components/script/layout_wrapper.rs, components/script_layout_interface/lib.rs, components/script_layout_interface/wrapper_traits.rs, components/script_layout_interface/Cargo.toml
@fitzgen: components/script/dom/document.rs, components/script/layout_wrapper.rs, components/script_layout_interface/lib.rs, components/script_layout_interface/wrapper_traits.rs, components/script_layout_interface/Cargo.toml
@emilio: components/layout/construct.rs, components/layout/fragment.rs, components/layout/Cargo.toml, components/layout/table_colgroup.rs, components/layout/generated_content.rs, components/layout/block.rs, components/layout/lib.rs, components/layout/inline.rs, components/layout/flow.rs

highfive · 2016-12-01T11:28:20Z

Warning

These commits modify unsafe code. Please review it carefully!
These commits modify layout and script code, but no tests are modified. Please consider adding a test!

julienw · 2016-12-01T11:30:00Z

components/layout/flow.rs

@@ -938,6 +939,9 @@ pub struct BaseFlow {
    pub stacking_context_id: StackingContextId,

    pub scroll_root_id: ScrollRootId,
+
+    // Mode for the current document.
+    pub quirks_mode: QuirksMode,


discussed with @SimonSapin, we were not sure whether integrating this into FlowFlags would be a good idea.

julienw · 2016-12-01T11:31:05Z

components/layout/fragment.rs

+        const IS_QUIRKS_MODE = 0b0000_0100,
+        /// Which quirks mode we're in: 0 is quirks, 1 is limited quirks
+        const QUIRKS_MODE = 0b0000_1000,
+    }


here I decided to integrate it in the bitflags, because I thought fragments are everywhere and I didn't want to use more memory when not needed.

julienw · 2016-12-01T11:31:24Z

components/layout/generated_content.rs

@@ -442,7 +442,8 @@ fn render_text(layout_context: &LayoutContext,
                                                             style.clone(),
                                                             style,
                                                             RestyleDamage::rebuild_and_reflow(),
-                                                             info));
+                                                             info,
+                                                             None));


quirksmode information is actually useless here

julienw · 2016-12-01T11:31:54Z

components/layout/inline.rs

@@ -1727,6 +1731,7 @@ pub struct InlineFragmentNodeInfo {
    pub selected_style: Arc<ServoComputedValues>,
    pub pseudo: PseudoElementType<()>,
    pub flags: InlineFragmentNodeFlags,
+    pub quirks_mode: QuirksMode,


same question here, not sure whether this could go into flags above.

julienw · 2016-12-01T11:33:43Z

components/layout/fragment.rs

+    pub fn from_node_and_style<N: ThreadSafeLayoutNode>(node: &N,
+                                                        style: Arc<ServoComputedValues>,
+                                                        specific: SpecificFragmentInfo)
+                                                        -> Fragment {


I created this to simplify some of the existing code, but it's not really necessary for this patch. Tell me if you'd prefer that I revert the changes around this constructor.

If you decide that we should keep it, maybe new could call it too and cut some code.

julienw · 2016-12-01T11:34:42Z

components/layout/fragment.rs

+        let writing_mode = style.writing_mode;
+
+        let mut restyle_damage = node.restyle_damage();
+        restyle_damage.remove(RECONSTRUCT_FLOW);


If I read prperly, this single line is usually what is different where I changed the calls from from_opaque_node_... to this constructor. I don't know what the consequences are. Maybe it's better because more factorized ? :)

emilio · 2016-12-01T11:53:27Z

Isn't the Quirks mode a document-level thing? If so, shouldn't it go into LayoutContext instead of consuming memory all over the place?

julienw · 2016-12-01T12:06:38Z

Yes it is at the document-level; I'll check why I didn't go with this solution.

Note we don't consume more memory if we use the existing bitflags :)

emilio · 2016-12-01T12:19:47Z

Yes, but we'd use two flags that would be more helpful for other stuff :)

julienw · 2016-12-01T12:33:52Z

Yep, I'll look closer :)
Do you have some ideas about where tests could be added ?

julienw · 2016-12-01T15:10:11Z

So I looked closer and now I remember why I did it this way.

Ultimately we need the information in the flow objects (like InlineFlow or BlockFlow) for most of layout-related issues (I especially looked #11704 so far but I expect other issues to have the same requirements).

At first I only added the information in BaseFlow and modified the constructors for InlineFlow and BlockFlow (we don't even need to change LayoutContext for this as we can directly access the data in the document). InlineFlow was quite easy, but I saw that BlockFlow's constructors were used a lot with a fragment as argument. That's when I thought that having this information in a Fragment could be useful as well after all.

Now I understand how LayoutContext works: we create it once for every thread (with some shared data) and we pass it over as parameter to every call. Do you think it would be a better architecture ?

emilio · 2016-12-01T15:57:11Z

On Thu, Dec 01, 2016 at 07:10:18AM -0800, Julien Wajsberg wrote: Now I understand how LayoutContext works: we create it once for every thread (with some shared data) and we pass it over as parameter to every call. Do you think it would be a better architecture ?

Yes, I think so, makes sense to put in SharedLayoutContext anything that is thread-independent and valid for the whole layout pass.

julienw · 2016-12-01T18:42:53Z

@emilio I just pushed 2 new commits to revert part of my previous changes and add the information in the shared layout context. Can you tell me what you think ?

emilio

Mostly nits, the rest looks sane to me :)

Also, please squash together the commits.

emilio · 2016-12-02T10:25:24Z

components/script/layout_wrapper.rs

@@ -908,6 +913,13 @@ impl<'ln> ThreadSafeLayoutNode for ServoThreadSafeLayoutNode<'ln> {
            self.get_jsmanaged().downcast::<Element>().unwrap().get_colspan()
        }
    }
+
+    #[inline]
+    fn quirks_mode(&self) -> QuirksMode {


nit: This is not needed anymore, right?

yeah, I think you're right. If we ever need it later, we can just add it at that time.

emilio · 2016-12-02T10:26:18Z

components/layout_thread/lib.rs

@@ -515,6 +521,7 @@ impl LayoutThread {
                error_reporter: self.error_reporter.clone(),
                local_context_creation_data: Mutex::new(local_style_context_creation_data),
                timer: self.timer.clone(),
+                quirks_mode: self.quirks_mode.unwrap_or(QuirksMode::NoQuirks),


I think you should be able to unwrap here?

Looking only at this file, I wasn't sure quirks_mode would be always already populated when eg repaint or tick_animations or others would be called. I think it's not a big deal to have a default value here, what do you think ?

I think it'd be wrong to lay out a quirks mode document in no-quirks mode, then laying it out in quirks mode, overall taking into account incremental reflows.

But just unwrapping would panic the thread in the same case; is it wishable ?

I think we do. I don't think we can layout anything without a document anyway. Indeed the sanest stuff would be making build_shared_layout_context taking the document directly, and don't store the quirks mode in the script thread at all.

Er.., in the layout thread, I mean

emilio · 2016-12-02T10:26:50Z

components/layout_thread/lib.rs

@@ -228,6 +230,9 @@ pub struct LayoutThread {
    // Number of layout threads. This is copied from `util::opts`, but we'd
    // rather limit the dependency on that module here.
    layout_threads: usize,
+
+    /// Which quirks mode are we rendering the document in ?


nit: No space before ?.

my french typography ;)

julienw · 2016-12-02T16:17:12Z

I fixed your comments except the last one, I squashed the commits.

I'm not sure of what gives the test issue on Travis. I reproduce also locally, but not on master, so it should be something in this patch. But I don't know how to get more information.

SimonSapin · 2016-12-08T02:20:51Z

components/style/Cargo.toml

@@ -30,6 +30,7 @@ euclid = "0.10.1"
 fnv = "1.0"
 heapsize = "0.3.0"
 heapsize_derive = {version = "0.1", optional = true}
+html5ever = {version = "0.10.2", features = ["heap_size", "unstable"]}


This is a fairly large dependency to add to Stylo, just to use a small enum. And it is also what causes build-geckolib to fail because that uses an older (stable channel) Rust compiler that doesn’t know about new-style procedural macros.

Instead I recommend duplicating the definition of the QuirksMode enum into components/style/context.rs. For consistency, the rest of Servo should be changed to use that enum instead of html5ever::tree_builder::QuirksMode. Namely, these files:

components/script/dom/node.rs components/script/dom/document.rs components/script/dom/bindings/trace.rs components/script/dom/servoparser/html.rs

The latter has a set_quirks_mode method that takes a html5ever::tree_builder::QuirksMode and will need to do the conversion to style::context::QuirksMode.

bors-servo · 2016-12-10T02:12:12Z

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

bors-servo · 2016-12-10T11:13:26Z

🔒 Merge conflict

bors-servo · 2016-12-16T20:31:53Z

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

jdm · 2016-12-16T21:19:43Z

@bors-servo: delegate+
If you rebase again, you can merge it immediately with r=emilio.

bors-servo · 2016-12-16T21:19:44Z

✌️ @julienw can now approve this pull request

julienw · 2016-12-17T20:52:32Z

Just realized how I should add r=emilio :) but afk right now so this will wait a few more hours.

KiChjang · 2016-12-17T21:25:04Z

@bors-servo r=emilio

bors-servo · 2016-12-17T21:25:05Z

📌 Commit d024787 has been approved by emilio

bors-servo · 2016-12-17T21:25:07Z

⌛ Testing commit d024787 with merge 164426a...

Expose Quirks Mode information in the layout data and code  This patch exposes the Quirks (NoQuirks/LimitedQuirks/Quirks) state to the layout subsystem. ---  - [X] `./mach build -d` does not report any errors - [X] `./mach test-tidy` does not report any errors - [ ] These changes fix #__ (github issue number if applicable). Prelimary work for issue #11704.  - [ ] There are tests for these changes OR - [ ] These changes do not require tests because _____ (Waiting for guidance of where/which tests I could do here)   --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/14430)

bors-servo · 2016-12-17T22:24:14Z

☀️ 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-dev

julienw · 2016-12-18T07:31:15Z

thanks @KiChjang !

emilio · 2016-12-18T13:23:19Z

Thank you for the patch @julienw! :)

highfive assigned asajeffrey Dec 1, 2016

highfive added the S-awaiting-review There is new code that needs to be reviewed. label Dec 1, 2016

julienw commented Dec 1, 2016

View reviewed changes

julienw changed the title ~~Expose Quirks Mode information in the layout data~~ Expose Quirks Mode information in the layout data and code Dec 1, 2016

julienw force-pushed the access-quirks-mode-from-layout branch 2 times, most recently from cc66b22 to 3fe7d5b Compare December 1, 2016 18:41

emilio reviewed Dec 2, 2016

View reviewed changes

emilio assigned emilio and unassigned asajeffrey Dec 2, 2016

julienw force-pushed the access-quirks-mode-from-layout branch from 58247ae to ab73ef6 Compare December 2, 2016 14:23

julienw force-pushed the access-quirks-mode-from-layout branch from ab73ef6 to dc1943a Compare December 8, 2016 01:16

SimonSapin reviewed Dec 8, 2016

View reviewed changes

julienw force-pushed the access-quirks-mode-from-layout branch 2 times, most recently from 7f8d2b6 to f26e0bf Compare December 8, 2016 19:27

highfive added S-awaiting-merge The PR is in the process of compiling and running tests on the automated CI. and removed S-awaiting-review There is new code that needs to be reviewed. S-needs-rebase There are merge conflict errors. labels Dec 9, 2016

highfive added S-needs-rebase There are merge conflict errors. and removed S-awaiting-merge The PR is in the process of compiling and running tests on the automated CI. labels Dec 10, 2016

julienw force-pushed the access-quirks-mode-from-layout branch from d1f20de to 6873527 Compare December 13, 2016 04:07

highfive added the S-awaiting-review There is new code that needs to be reviewed. label Dec 13, 2016

julienw force-pushed the access-quirks-mode-from-layout branch from 6873527 to 2265cfb Compare December 13, 2016 08:04

highfive added the S-needs-rebase There are merge conflict errors. label Dec 16, 2016

Expose Quirks Mode information in the style shared context r=emilio

d024787

julienw force-pushed the access-quirks-mode-from-layout branch from 2265cfb to d024787 Compare December 17, 2016 19:15

highfive removed S-awaiting-review There is new code that needs to be reviewed. S-needs-rebase There are merge conflict errors. labels Dec 17, 2016

highfive added the S-awaiting-merge The PR is in the process of compiling and running tests on the automated CI. label Dec 17, 2016

bors-servo merged commit d024787 into servo:master Dec 17, 2016

highfive removed the S-awaiting-merge The PR is in the process of compiling and running tests on the automated CI. label Dec 17, 2016

jdm mentioned this pull request Dec 19, 2016

Panic when sending quirks information to layout thread #14637

Closed

Expose Quirks Mode information in the layout data and code #14430

Expose Quirks Mode information in the layout data and code #14430

Conversation

julienw commented Dec 1, 2016 • edited by larsbergstrom

highfive commented Dec 1, 2016

highfive commented Dec 1, 2016

highfive commented Dec 1, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

julienw Dec 1, 2016 • edited

Choose a reason for hiding this comment

emilio commented Dec 1, 2016

julienw commented Dec 1, 2016

emilio commented Dec 1, 2016

julienw commented Dec 1, 2016

julienw commented Dec 1, 2016

emilio commented Dec 1, 2016 via email

julienw commented Dec 1, 2016

emilio left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

julienw commented Dec 2, 2016

Choose a reason for hiding this comment

bors-servo commented Dec 10, 2016

bors-servo commented Dec 10, 2016

bors-servo commented Dec 16, 2016

jdm commented Dec 16, 2016

bors-servo commented Dec 16, 2016

julienw commented Dec 17, 2016

KiChjang commented Dec 17, 2016

bors-servo commented Dec 17, 2016

bors-servo commented Dec 17, 2016

bors-servo commented Dec 17, 2016

julienw commented Dec 18, 2016

emilio commented Dec 18, 2016

julienw commented Dec 1, 2016 •

edited by larsbergstrom

julienw Dec 1, 2016 •

edited