stylo: Stop restyling display: none elements, remove the has_changed hack that made us use ReconstructFrame unconditionally. #12757

emilio · 2016-08-06T21:11:37Z

./mach build -d does not report any errors
./mach test-tidy does not report any errors

r? @bholley

This change is

highfive · 2016-08-06T21:11:42Z

Heads up! This PR modifies the following files:

@bholley: components/style/parallel.rs, components/style/sequential.rs, components/style/dom.rs, components/style/traversal.rs, components/style/matching.rs
@KiChjang: components/script_layout_interface/restyle_damage.rs

highfive · 2016-08-06T21:11:43Z

Warning

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

emilio · 2016-08-07T17:49:14Z

@bors-servo: try

bors-servo · 2016-08-07T17:49:16Z

⌛ Trying commit 1130e7a with merge bf43593...


        Auto merge of #12757 - emilio:stylo, r=<try>

@bholley

stylo: Stop restyling display: none elements, remove the has_changed hack that made us use ReconstructFrame unconditionally.  ---  - [x] `./mach build -d` does not report any errors - [x] `./mach test-tidy` does not report any errors  r? @bholley  --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/12757)

bors-servo · 2016-08-07T18:36:11Z

💔 Test failed - mac-rel-css

emilio · 2016-08-07T22:21:16Z

@bors-servo: try

bors-servo · 2016-08-07T22:21:19Z

⌛ Trying commit 8b3a8fc with merge dbffad1...


        Auto merge of #12757 - emilio:stylo, r=<try>

@bholley

stylo: Stop restyling display: none elements, remove the has_changed hack that made us use ReconstructFrame unconditionally.  ---  - [x] `./mach build -d` does not report any errors - [x] `./mach test-tidy` does not report any errors  r? @bholley  --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/12757)

bors-servo · 2016-08-07T23:09:41Z

💔 Test failed - linux-rel

highfive · 2016-08-07T23:09:43Z

  ▶ Unexpected subtest result in /html/browsers/browsing-the-web/history-traversal/browsing_context_name.html:
  └ PASS [expected FAIL] Retaining window.name on history traversal

emilio · 2016-08-08T00:07:21Z

That's a known intermittent (#12568), cool :)

bholley · 2016-08-08T18:53:07Z

components/style/traversal.rs

+    /// performed.
+    ///
+    /// If it's false, then process_postorder has no effect at all.
+    fn should_traverse_back_up(&self) -> bool { true }


Call this has_postorder_traversal?

bholley · 2016-08-08T18:53:26Z

ports/geckolib/traversal.rs

@@ -36,4 +36,7 @@ impl<'lc, 'ln> DomTraversalContext<GeckoNode<'ln>> for RecalcStyleOnly<'lc> {
    }

    fn process_postorder(&self, _: GeckoNode<'ln>) {}


unreacheable!() here.

bholley · 2016-08-08T18:53:56Z

components/style/traversal.rs

@@ -149,6 +149,12 @@ pub trait DomTraversalContext<N: TNode>  {
    /// Process `node` on the way up, after its children have been processed.
    fn process_postorder(&self, node: N);


Add a comment indicating that this is only called if has_postorder_traversal returns true.

bholley · 2016-08-08T20:03:03Z

ports/geckolib/wrapper.rs

    fn compute(source: Option<&nsStyleContext>,
               new_style: &Arc<ComputedValues>) -> Self {
        type Helpers = ArcHelpers<ServoComputedValues, ComputedValues>;
        let context = match source {
            Some(ctx) => ctx as *const nsStyleContext as *mut nsStyleContext,
+            // If there's no style source (that is, no style context), there can


I'd say "if there's no preexisting computed values".

bholley · 2016-08-08T20:03:23Z

ports/geckolib/wrapper.rs

+            // If there's no style source (that is, no style context), there can
+            // be two reasons for it.
+            //
+            // The first one, is that this is not an incremental restyle (so we


Nit: remove the comma.

bholley · 2016-08-08T20:04:44Z

ports/geckolib/wrapper.rs

+            //
+            // The second one is that this is an incremental restyle, but the
+            // node has display: none. In this case, the old computed values
+            // should still be present, and we should be able to compare the new


Emphasize that they're stored on the node by servo, but we don't have a style context.

bholley · 2016-08-08T20:05:02Z

ports/geckolib/wrapper.rs

+            // can do nothing on it because the old and the new display values
+            // are none.
+            //
+            // This is done outside of this method in servo itself, so we


Be more specific as to where.

bholley · 2016-08-08T20:10:57Z

ports/geckolib/wrapper.rs

+        use std::mem;
+        GeckoRestyleDamage(unsafe { mem::transmute(0u32) })
+    }
+
    fn compute(source: Option<&nsStyleContext>,
               new_style: &Arc<ComputedValues>) -> Self {


Per discussion, let's make this take a Non-Option, and then the caller can make stronger assertions about the reason why the style context doesn't exist (i.e. either non-incremental restyle or the display::none case).

bholley · 2016-08-08T20:29:13Z

components/style/matching.rs

+        // the changes, and any change that we could miss would already have
+        // been caught by the parent's change. If for some reason I'm wrong on
+        // this, we'd have to compare computed values in Gecko too.
+        let existing_style =


Please file a followup bug on bugzilla describing the stuff we talked about, and reference that bug here.

bholley · 2016-08-08T20:32:17Z

components/style/sequential.rs

-        context.process_preorder(node);
+        let should_stop = match context.process_preorder(node) {
+            ForceTraversalStop::Force => true,
+            ForceTraversalStop::DontForce => false,


Let's call these Let's call this RestyleResult::{StopTraversal,ContinueTraversal}?

pcwalton · 2016-08-10T17:39:22Z

components/style/sequential.rs

-            context.pre_process_child_hook(node, kid);
-            if context.should_process(kid) {
-                doit::<N, C>(context, kid);
+        if !should_stop {


Same as above: don't convert RestyleResult to a boolean.

pcwalton · 2016-08-10T17:41:24Z

components/style/traversal.rs

@@ -201,6 +211,7 @@ pub fn recalc_style_at<'a, N, C>(context: &'a C,
    let mut bf = take_thread_local_bloom_filter(parent_opt, root, context.shared_context());

    let nonincremental_layout = opts::get().nonincremental_layout;
+    let mut restyle_result = RestyleResult::Continue;
    if nonincremental_layout || node.is_dirty() {


nit: I wonder if this might be clearer as:

if !nonincremental_layout && !node.is_dirty() { return RestyleResult::Continue }

Then you can remove a level of indentation below.

Hmm... I can't see what you mean, you still need to insert the node in the bloom filter, complete animations, etc.

The rest of the comments are addressed or were on outdated commits and no longer apply I think.

(Still needs an squash though)

pcwalton · 2016-08-11T01:23:03Z

r=me


        stylo: Allow bypassing the bottom-up traversal.


        style: Force to stop computing children if we find the parent has dis…

…play: none.


        layout: Fix servo layout to take into account possibly unstyled child…

… nodes in the display: none case.


        layout: Ensure a element's style is up to date when processing queries.


        Make RestyleDamage::compute take no optional argument, delegate the h…

…andling to the caller.


        style: Fix pseudo-element restyling.


        Add a regression test for pseudo-element restyles.


        layout: Factor out the code to ensure a node data is initialized.


        style: Document the value of StyleWasShared's RestyleResult


        Don't create unneeded temporaries in style traversals.

emilio · 2016-08-11T01:27:48Z

@bors-servo: r=bholley,pcwalton

bors-servo · 2016-08-11T01:27:48Z

📌 Commit 9b8eac0 has been approved by bholley,pcwalton

bors-servo · 2016-08-11T02:02:31Z

⌛ Testing commit 9b8eac0 with merge 1b24503...


        Auto merge of #12757 - emilio:stylo, r=bholley,pcwalton

@bholley

stylo: Stop restyling display: none elements, remove the has_changed hack that made us use ReconstructFrame unconditionally.  ---  - [x] `./mach build -d` does not report any errors - [x] `./mach test-tidy` does not report any errors  r? @bholley  --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/12757)

bors-servo · 2016-08-11T03:00:39Z

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

bholley · 2016-08-11T07:27:34Z

@emilio does the gecko-dev branch need to be updated in correspondence with this commit?

emilio · 2016-08-11T07:55:23Z

@bholley no, not necessarily

highfive assigned bholley Aug 6, 2016

highfive added the S-awaiting-review label Aug 6, 2016

emilio force-pushed the emilio:stylo branch from a5b7ed9 to 1130e7a Aug 7, 2016

highfive added the S-tests-failed label Aug 7, 2016

emilio force-pushed the emilio:stylo branch from 1130e7a to 8b3a8fc Aug 7, 2016

highfive removed the S-tests-failed label Aug 7, 2016

highfive added the S-tests-failed label Aug 7, 2016

emilio removed the S-tests-failed label Aug 8, 2016

bholley reviewed Aug 8, 2016
View changes

components/style/traversal.rs Outdated

/// performed.

///

/// If it's false, then process_postorder has no effect at all.

fn should_traverse_back_up(&self) -> bool { true }

This comment has been minimized.

Sign in to view

bholley Aug 8, 2016
Contributor

Call this has_postorder_traversal?

bholley reviewed Aug 8, 2016
View changes

ports/geckolib/traversal.rs Outdated

@@ -36,4 +36,7 @@ impl<'lc, 'ln> DomTraversalContext<GeckoNode<'ln>> for RecalcStyleOnly<'lc> {

}

fn process_postorder(&self, _: GeckoNode<'ln>) {}

This comment has been minimized.

Sign in to view

bholley Aug 8, 2016
Contributor

unreacheable!() here.

bholley reviewed Aug 8, 2016
View changes

pcwalton reviewed Aug 10, 2016
View changes

emilio added 10 commits Aug 5, 2016

stylo: Allow bypassing the bottom-up traversal.

Unverified

This user has not uploaded their public key yet.

GPG key ID: 056B727BB9C1027C Learn about signing commits

436c1b3

style: Force to stop computing children if we find the parent has dis…

Unverified

This user has not uploaded their public key yet.

GPG key ID: 056B727BB9C1027C Learn about signing commits

544a117

…play: none.

layout: Fix servo layout to take into account possibly unstyled child…

Unverified

This user has not uploaded their public key yet.

GPG key ID: 056B727BB9C1027C Learn about signing commits

17772d1

… nodes in the display: none case.

layout: Ensure a element's style is up to date when processing queries.

Unverified

This user has not uploaded their public key yet.

GPG key ID: 056B727BB9C1027C Learn about signing commits

6b60383

Make RestyleDamage::compute take no optional argument, delegate the h…

Unverified

This user has not uploaded their public key yet.

GPG key ID: 056B727BB9C1027C Learn about signing commits

1c322f3

…andling to the caller.

style: Fix pseudo-element restyling.

Unverified

This user has not uploaded their public key yet.

GPG key ID: 056B727BB9C1027C Learn about signing commits

a36762a

Add a regression test for pseudo-element restyles.

Unverified

This user has not uploaded their public key yet.

GPG key ID: 056B727BB9C1027C Learn about signing commits

2c32101

layout: Factor out the code to ensure a node data is initialized.

Unverified

This user has not uploaded their public key yet.

GPG key ID: 056B727BB9C1027C Learn about signing commits

477c3c8

style: Document the value of StyleWasShared's RestyleResult

Unverified

This user has not uploaded their public key yet.

GPG key ID: 056B727BB9C1027C Learn about signing commits

d109006

Don't create unneeded temporaries in style traversals.

Unverified

This user has not uploaded their public key yet.

GPG key ID: 056B727BB9C1027C Learn about signing commits

Loading status checks…

9b8eac0

emilio force-pushed the emilio:stylo branch from 3c9bfa3 to 9b8eac0 Aug 11, 2016

highfive assigned bholley and unassigned pcwalton Aug 11, 2016

highfive added S-awaiting-merge and removed S-awaiting-review S-needs-rebase labels Aug 11, 2016

bors-servo merged commit 9b8eac0 into servo:master Aug 11, 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 Aug 11, 2016

emilio deleted the emilio:stylo branch Aug 11, 2016

bors-servo mentioned this pull request Aug 11, 2016

Inline pseudo elements layout#12367 #12669

Merged

4 of 5 tasks complete

servo / servo

stylo: Stop restyling display: none elements, remove the has_changed hack that made us use ReconstructFrame unconditionally. #12757

stylo: Stop restyling display: none elements, remove the has_changed hack that made us use ReconstructFrame unconditionally. #12757

emilio commented Aug 6, 2016 •

edited by larsbergstrom

highfive commented Aug 6, 2016

highfive commented Aug 6, 2016

emilio commented Aug 7, 2016

bors-servo commented Aug 7, 2016

bors-servo commented Aug 7, 2016

emilio commented Aug 7, 2016

bors-servo commented Aug 7, 2016

bors-servo commented Aug 7, 2016

highfive commented Aug 7, 2016

emilio commented Aug 8, 2016 •

edited

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.

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.

pcwalton commented Aug 11, 2016

emilio commented Aug 11, 2016

bors-servo commented Aug 11, 2016

bors-servo commented Aug 11, 2016

bors-servo commented Aug 11, 2016

bholley commented Aug 11, 2016

emilio commented Aug 11, 2016

		@@ -36,4 +36,7 @@ impl<'lc, 'ln> DomTraversalContext<GeckoNode<'ln>> for RecalcStyleOnly<'lc> {
		}

		fn process_postorder(&self, _: GeckoNode<'ln>) {}

		@@ -149,6 +149,12 @@ pub trait DomTraversalContext<N: TNode> {
		/// Process `node` on the way up, after its children have been processed.
		fn process_postorder(&self, node: N);

servo / servo

Join GitHub today

stylo: Stop restyling display: none elements, remove the has_changed hack that made us use ReconstructFrame unconditionally. #12757

stylo: Stop restyling display: none elements, remove the has_changed hack that made us use ReconstructFrame unconditionally. #12757

Conversation

emilio commented Aug 6, 2016 • edited by larsbergstrom

highfive commented Aug 6, 2016

highfive commented Aug 6, 2016

emilio commented Aug 7, 2016

bors-servo commented Aug 7, 2016

bors-servo commented Aug 7, 2016

emilio commented Aug 7, 2016

bors-servo commented Aug 7, 2016

bors-servo commented Aug 7, 2016

highfive commented Aug 7, 2016

emilio commented Aug 8, 2016 • edited

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.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

pcwalton Aug 10, 2016 • edited

This comment has been minimized.

This comment has been minimized.

pcwalton commented Aug 11, 2016

emilio commented Aug 11, 2016

bors-servo commented Aug 11, 2016

bors-servo commented Aug 11, 2016

bors-servo commented Aug 11, 2016

bholley commented Aug 11, 2016

emilio commented Aug 11, 2016

emilio commented Aug 6, 2016 •

edited by larsbergstrom

emilio commented Aug 8, 2016 •

edited

pcwalton Aug 10, 2016 •

edited