stylo: use eRestyle_StyleAttribute rather than converting it to eRestyle_Self/Subtree

RESOLVED FIXED in Firefox 55

Status

()

Core
CSS Parsing and Computation
P1
normal
RESOLVED FIXED
6 months ago
5 months ago

People

(Reporter: heycam, Assigned: heycam)

Tracking

unspecified
mozilla55
Points:
---

Firefox Tracking Flags

(firefox55 fixed)

Details

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(1 attachment, 1 obsolete attachment)

(Assignee)

Description

6 months ago
Now that we support processing STYLE_ATTRIBUTE restyle hints on the Servo side, we can pass through eRestyle_StyleAttribute to Servo_NoteExplicitHints without converting it to eRestyle_Self/Subtree.
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Updated

6 months ago
Attachment #8835942 - Flags: review?(bobbyholley)
(Assignee)

Comment 3

6 months ago
https://treeherder.mozilla.org/#/jobs?repo=try&revision=d3dd9260d84e357b65f27a91199d408b2d7c477d&group_state=expanded

Comment 4

6 months ago
mozreview-review
Comment on attachment 8835942 [details]
Align RESTYLE_STYLE_ATTRIBUTE value with eRestyle_StyleAttribute.

https://reviewboard.mozilla.org/r/111492/#review112930

::: servo/components/style/restyle_hints.rs:49
(Diff revision 1)
>          const RESTYLE_LATER_SIBLINGS = 0x08,
>  
>          /// Don't re-run selector-matching on the element, only the style
>          /// attribute has changed, and this change didn't have any other
>          /// dependencies.
> -        const RESTYLE_STYLE_ATTRIBUTE = 0x10,
> +        const RESTYLE_STYLE_ATTRIBUTE = 0x80,

So, this makes me a bit nervous. Please add a comment above RestyleHint that the bit positions matter for gecko compat.


I think we should also add some static assertions in RestyleHint::from below that:
(1) Each RestyleHint corresponds to the appropriate value according to bindgen, and that:
(2) The union of all the bits we check is equal to RestyleHint::all() (to prevent unintentionally allowing garbage bits from Gecko if somebody adds a new restle hint).
Attachment #8835942 - Flags: review?(bobbyholley) → review+

Comment 5

6 months ago
mozreview-review
Comment on attachment 8835943 [details]
Bug 1338461 - Pass eRestyle_StyleAttribute through to Servo_NoteExplicitHints.

https://reviewboard.mozilla.org/r/111494/#review112932
Attachment #8835943 - Flags: review?(bobbyholley) → review+
(Assignee)

Comment 6

6 months ago
> I think we should also add some static assertions in RestyleHint::from below
> that:
> (1) Each RestyleHint corresponds to the appropriate value according to
> bindgen, and that:
> (2) The union of all the bits we check is equal to RestyleHint::all() (to
> prevent unintentionally allowing garbage bits from Gecko if somebody adds a
> new restle hint).

I found existing assertions in tests/unit/stylo/sanity_checks.rs so I will add them there and point to them from the comment.
(Assignee)

Comment 7

6 months ago
https://github.com/servo/servo/pull/15523
Comment hidden (mozreview-request)
(Assignee)

Updated

6 months ago
Attachment #8835942 - Attachment is obsolete: true
(In reply to Cameron McCormack (:heycam) from comment #6)
> > I think we should also add some static assertions in RestyleHint::from below
> > that:
> > (1) Each RestyleHint corresponds to the appropriate value according to
> > bindgen, and that:
> > (2) The union of all the bits we check is equal to RestyleHint::all() (to
> > prevent unintentionally allowing garbage bits from Gecko if somebody adds a
> > new restle hint).
> 
> I found existing assertions in tests/unit/stylo/sanity_checks.rs so I will
> add them there and point to them from the comment.

Would you mind moving those to restyle_hints.rs? In the current world those unit tests will run against the checked-in bindings, which may be arbitrarily out of date. I think it's much better to catch these bugs as soon as the enum on either side is changed.
Flags: needinfo?(cam)
(Assignee)

Comment 10

6 months ago
OK, moving those in https://github.com/servo/servo/pull/15538.
Flags: needinfo?(cam)
(Assignee)

Comment 11

5 months ago
Interestingly, I get some test failures from this:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=9105655834fc
Priority: -- → P1
(Assignee)

Comment 12

5 months ago
https://treeherder.mozilla.org/#/jobs?repo=try&revision=a9b3ea102ed73731599555ba51b7038af43dca5c
Comment hidden (mozreview-request)

Comment 14

5 months ago
Pushed by cmccormack@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/1ac14d8a4ca4
Pass eRestyle_StyleAttribute through to Servo_NoteExplicitHints. r=bholley

Comment 15

5 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/1ac14d8a4ca4
Status: ASSIGNED → RESOLVED
Last Resolved: 5 months ago
status-firefox55: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
You need to log in before you can comment on or make changes to this bug.