stylo: Allow computing change hints during the traversal. #12645

emilio · 2016-07-29T02:40:20Z

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

These changes do not require tests because geckolib :-(

r? @bholley
cc @heycam

This change is

highfive · 2016-07-29T02:40:23Z

Heads up! This PR modifies the following files:

@bholley: components/style/gecko_glue.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-07-29T02:40:24Z

Warning

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

bholley · 2016-07-29T05:24:48Z

components/style/gecko_glue.rs

@@ -47,6 +46,10 @@ impl<GeckoType, ServoType> ArcHelpers<GeckoType, ServoType> {
        unsafe { transmute(owned) }
    }

+    pub unsafe fn borrow(borrowed: &Arc<ServoType>) -> *const &mut GeckoType {


Can we make this a closure-y API like |with| to avoid the unsafety here?

bholley · 2016-07-29T05:25:26Z

Generally seems reasonable, but will need some fiddling in light of https://bugzilla.mozilla.org/show_bug.cgi?id=1290335#c9

bholley · 2016-08-01T17:06:56Z

components/style/dom.rs

+    /// ComputedValues in Servo and a StyleContext in Gecko.
+    ///
+    /// This must be obtained via the node.
+    type CascadeStyleSource;


Let's call this PreExistingComputedValues?

Also, please add a comment explaining why we do this dance with the style context (i.e. the inference from the caching).

bholley · 2016-08-01T18:13:30Z

components/style/dom.rs

+    /// XXX: It's a bit unfortunate we need to pass the current computed values
+    /// as an argument here, but otherwise Servo would crash due to double
+    /// borrows to return it.
+    fn style_source<'a>(&'a self,


existing_style_for_restyle_damage?

bholley · 2016-08-01T18:20:14Z

components/style/animation.rs

@@ -507,7 +507,8 @@ where Damage: TRestyleDamage {
                                                                 frame);
            if updated_style {
                if let Some(damage) = damage {
-                    *damage = *damage | Damage::compute(Some(style), &new_style);
+                    *damage =
+                        *damage | Damage::compute_for_layout(Some(style), &new_style);


Let's return the new style and let the caller handle it (or, if that doesn't work, let the caller provide a callback that gets passed the new style), and then remove the distinction between compute_for_layout and compute_for_cascade.

bholley · 2016-08-01T18:22:23Z

components/style/gecko_glue.rs

@@ -47,6 +46,15 @@ impl<GeckoType, ServoType> ArcHelpers<GeckoType, ServoType> {
        unsafe { transmute(owned) }
    }

+    pub fn borrow<F, Output>(borrowed: &Arc<ServoType>, cb: F) -> Output


with_gecko?

bholley · 2016-08-01T18:29:26Z

ports/geckolib/wrapper.rs

+
+        unsafe {
+            let context_ptr = Gecko_GetStyleContext(self.node);
+            mem::transmute(context_ptr)


unsafe { context_ptr.as_ref() } ?

bholley · 2016-08-01T18:30:31Z

@bors-servo delegate+

r=me with comments.

bors-servo · 2016-08-01T18:30:32Z

✌️ @emilio can now approve this pull request

emilio · 2016-08-01T19:59:54Z

@bors-servo: try

bors-servo · 2016-08-01T19:59:58Z

⌛ Trying commit 769cdc5 with merge 9fc5d76...


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

@bholley

stylo: Allow computing change hints during the traversal.  ---  - [x] `./mach build -d` does not report any errors - [x] `./mach test-tidy` does not report any errors  - [x] These changes do not require tests because geckolib :-(  r? @bholley cc @heycam  --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/12645)

bors-servo · 2016-08-01T21:15:20Z

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


        style: Fix sequential traversal.


        stylo: Allow computing change hints during the traversal.


        style: Refactor TRestyleDamage and TNode to accept/provide a "style s…

…ource" In the Gecko case, this style source would be the style context. In the servo case, it will be always the computed values. We could optimise this further in the case of stylo (from three FFI calls to one) if we use an API of the form CalcAndStore(node, new_cv). But that would imply borrowing the data twice from Servo (we also have borrow_data_unchecked fwiw, but...).

emilio · 2016-08-03T20:49:41Z

@bholley: I'll need you to review the last commit and the regen.py changes in the previous one.

bholley · 2016-08-03T21:11:51Z

ports/geckolib/sanity_checks.rs

+    use gecko_bindings::structs::nsRestyleHint;
+
+    check_enum_value_non_static!(nsRestyleHint::eRestyle_Self, RESTYLE_SELF.bits());
+    // XXX This for servo actually means something like an hipotetical


"a hypothetical"

bholley · 2016-08-03T21:12:08Z

ports/geckolib/sanity_checks.rs

+    // XXX This for servo actually means something like an hipotetical
+    // Restyle_AllDescendants (but without running selector matching on the
+    // element). ServoRestyleManager interprets it like that, but in practice we
+    // should align the behavior with Gecko adding a new restyle hint, maybe?


Reference the bug we have on file for doing that?

bholley · 2016-08-03T21:12:16Z

lgtm


        stylo: Fix and ensure alignment of restyle hints. Test it on Travis.

Turns out eRestyle_LaterSiblings was not really matching, sigh...


        Update bindings

emilio · 2016-08-03T22:19:59Z

@bors-servo: r=bholley

bors-servo · 2016-08-03T22:20:00Z

📌 Commit 444e8f8 has been approved by bholley

bors-servo · 2016-08-04T00:02:27Z

⌛ Testing commit 444e8f8 with merge cbf71a2...


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

@bholley

stylo: Allow computing change hints during the traversal.  ---  - [x] `./mach build -d` does not report any errors - [x] `./mach test-tidy` does not report any errors  - [x] These changes do not require tests because geckolib :-(  r? @bholley cc @heycam  --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/12645)

bholley · 2016-08-04T01:02:13Z

@bors-servo p=2

bors-servo · 2016-08-04T01:20:19Z

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

highfive assigned bholley Jul 29, 2016

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

bholley reviewed Jul 29, 2016
View changes

emilio force-pushed the emilio:stylo branch from 3adbd53 to ca7c2f0 Jul 29, 2016

bholley reviewed Aug 1, 2016
View changes

ports/geckolib/wrapper.rs Outdated

unsafe {

let context_ptr = Gecko_GetStyleContext(self.node);

mem::transmute(context_ptr)

This comment has been minimized.

Sign in to view

bholley Aug 1, 2016
Contributor

unsafe { context_ptr.as_ref() } ?

emilio force-pushed the emilio:stylo branch from ca7c2f0 to 769cdc5 Aug 1, 2016

emilio added 3 commits Jul 28, 2016

style: Fix sequential traversal.

Unverified

This user has not uploaded their public key yet.

GPG key ID: 056B727BB9C1027C Learn about signing commits

67ac81f

stylo: Allow computing change hints during the traversal.

Unverified

This user has not uploaded their public key yet.

GPG key ID: 056B727BB9C1027C Learn about signing commits

1470d5b

emilio force-pushed the emilio:stylo branch from 32f8e49 to 616ab07 Aug 3, 2016

bholley reviewed Aug 3, 2016
View changes

emilio added 2 commits Aug 3, 2016

stylo: Fix and ensure alignment of restyle hints. Test it on Travis.

Unverified

This user has not uploaded their public key yet.

GPG key ID: 056B727BB9C1027C Learn about signing commits

27b2bad

Turns out eRestyle_LaterSiblings was not really matching, sigh...

Update bindings

Unverified

This user has not uploaded their public key yet.

GPG key ID: 056B727BB9C1027C Learn about signing commits

Loading status checks…

444e8f8

emilio force-pushed the emilio:stylo branch from 616ab07 to 444e8f8 Aug 3, 2016

highfive added S-awaiting-merge and removed S-awaiting-review labels Aug 3, 2016

bors-servo merged commit 444e8f8 into servo:master Aug 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 Aug 4, 2016

bors-servo mentioned this pull request Aug 4, 2016

Update to selectors 0.8.2 (generic over atoms) #12571

Merged

4 of 5 tasks complete

servo / servo

stylo: Allow computing change hints during the traversal. #12645

stylo: Allow computing change hints during the traversal. #12645

emilio commented Jul 29, 2016 •

edited by larsbergstrom

highfive commented Jul 29, 2016

highfive commented Jul 29, 2016

This comment has been minimized.

bholley commented Jul 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.

bholley commented Aug 1, 2016

bors-servo commented Aug 1, 2016

emilio commented Aug 1, 2016

bors-servo commented Aug 1, 2016

bors-servo commented Aug 1, 2016

emilio commented Aug 3, 2016

This comment has been minimized.

This comment has been minimized.

bholley commented Aug 3, 2016

emilio commented Aug 3, 2016

bors-servo commented Aug 3, 2016

bors-servo commented Aug 4, 2016

bholley commented Aug 4, 2016

bors-servo commented Aug 4, 2016

servo / servo

Join GitHub today

stylo: Allow computing change hints during the traversal. #12645

stylo: Allow computing change hints during the traversal. #12645

Conversation

emilio commented Jul 29, 2016 • edited by larsbergstrom

highfive commented Jul 29, 2016

highfive commented Jul 29, 2016

This comment has been minimized.

bholley commented Jul 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.

bholley commented Aug 1, 2016

bors-servo commented Aug 1, 2016

emilio commented Aug 1, 2016

bors-servo commented Aug 1, 2016

bors-servo commented Aug 1, 2016

emilio commented Aug 3, 2016

This comment has been minimized.

This comment has been minimized.

bholley commented Aug 3, 2016

emilio commented Aug 3, 2016

bors-servo commented Aug 3, 2016

bors-servo commented Aug 4, 2016

bholley commented Aug 4, 2016

bors-servo commented Aug 4, 2016

emilio commented Jul 29, 2016 •

edited by larsbergstrom