I felt like doing it today.

emilio++

The second review request contains a bug in the handling of :any-link. There's still a whole lot of unexpected pass orange: https://treeherder.mozilla.org/#/jobs?repo=try&revision=e105dd09ebbe87687edc17f9f828724430e9077a This one is with the :any-link issue fixed: https://treeherder.mozilla.org/#/jobs?repo=try&revision=044fa37988243c68c6148ba005f6ffcbdf0e3292

Comment on attachment 8850768 [details] Bug 1350140: stylo: Implement all the remaining state pseudo-classes. https://reviewboard.mozilla.org/r/123284/#review125744 Nice! ::: dom/events/EventStates.h:294 (Diff revision 3) > -// #define NS_EVENT_STATE_?????????? NS_DEFINE_EVENT_STATE_MACRO(44) > -// Element is highlighted (devtools inspector) > -#define NS_EVENT_STATE_DEVTOOLS_HIGHLIGHTED NS_DEFINE_EVENT_STATE_MACRO(45) > -// Element is an unresolved custom element candidate > -#define NS_EVENT_STATE_UNRESOLVED NS_DEFINE_EVENT_STATE_MACRO(46) > -// Element is transitioning for rules changed by style editor > +/* > + * Bits below here do not have Servo-related ordering constraints. > + * > + * Remember to change NS_EVENT_STATE_HIGHEST_SERVO_BIT at the top of the file if > + * this changes! > + */ I guess any new event state bits we have will need to be handled in Servo too, so I guess we can just get rid of this comment (and NS_EVENT_STATE_HIGHEST_SERVO_BIT). ::: servo/components/style/element_state.rs:15 (Diff revision 3) > + /// TODO(emilio): We really really want to use the NS_EVENT_STATE bindings > + /// for this. In the meantime, could we add assertions that the values are the same, like we do for the RestyleHint flags?

Quick update with this: This patch uncovered a bug with how outline-width is inherited. With that (and border inheritance also fixed), I'm still running into some test failures that I need to look into.

Comment on attachment 8851760 [details] Bug 1350140: Fix computation of border and outline-width when the border or outline style change. https://reviewboard.mozilla.org/r/123986/#review126586 ::: servo/components/style/properties/gecko.mako.rs:440 (Diff revision 1) > #[allow(non_snake_case)] > pub fn set_${ident}(&mut self, v: longhands::${ident}::computed_value::T) { > % if round_to_pixels: > let au_per_device_px = Au(self.gecko.mTwipsPerPixel); > + % if save_specified_in: > + self.gecko.${save_specified_in} = v.0; Nit: the Mako gunk around here doesn't seem to affect the indenting of the Rust code, so I would deindent this line. Well, I guess we're a bit inconsistent throughout the file, but let's be consistent within the function at least. :-) ::: servo/components/style/properties/gecko.mako.rs:450 (Diff revision 1) > + % if save_specified_in: > + self.set_${ident}(Au(other.gecko.${save_specified_in})); > + % else: > + self.gecko.${gecko_ffi_name} = other.gecko.${gecko_ffi_name}; > + % endif Can this just be: % if save_specified_in: self.gecko.${save_specified_in} = other.gecko.${save_specified_in}; % endif self.gecko.${gecko_ffi_name} = other.gecko.${gecko_ffi_name}; ? Or do we need to call set_${ident} because we might have a different mTwipsPerPixelValue? And if that's true, why don't we have to call set_${ident} when save_specified_in wasn't specified, but round_to_pixels is true? ::: servo/components/style/properties/properties.mako.rs:2324 (Diff revision 1) > // The initial value of outline width may be changed at computed value time. > + // > + // FIXME(emilio): Is there a spec backing this out? s/out/up/ But yes, the initial value of outline-width is medium, but if outline-style is none, then outline-width computes to 0. Given hidden isn't a valid value of outline-style, it might be worth changing this to check for none explicitly, or to assert or something.

Comment on attachment 8851761 [details] Bug 1350140: Avoid dumb copy-constructor. https://reviewboard.mozilla.org/r/123988/#review126598

Comment on attachment 8851762 [details] Bug 1350140: Flush the overflow changed tracker when done with restyles. https://reviewboard.mozilla.org/r/123990/#review126600

Comment on attachment 8851760 [details] Bug 1350140: Fix computation of border and outline-width when the border or outline style change. Requesting re-review on this one since it's significantly different from the initial patch.

Comment on attachment 8851760 [details] Bug 1350140: Fix computation of border and outline-width when the border or outline style change. https://reviewboard.mozilla.org/r/123986/#review127118 ::: servo/components/style/properties/gecko.mako.rs:828 (Diff revision 5) > + "mBorderStyle[%s]" % side.index, > + border_style_keyword, > + on_set="update_border_%s" % side.ident, > need_clone=True) %> > > + // This is needed because the initial mComputedBorder value is set to zero. Thanks for adding/expanding this comment.

Comment on attachment 8851988 [details] Bug 1350140: more test expectation updates. https://reviewboard.mozilla.org/r/124246/#review127120

Comment on attachment 8851759 [details] Bug 1350140: Update test expectations. https://reviewboard.mozilla.org/r/123984/#review127214

Comment on attachment 8850768 [details] Bug 1350140: stylo: Implement all the remaining state pseudo-classes. https://reviewboard.mozilla.org/r/123284/#review125744 > In the meantime, could we add assertions that the values are the same, like we do for the RestyleHint flags? We can't (at least without a bit more work and some macro hackery), because I hadn't realised the macros don't define plain integers.

Pushed by ecoal95@gmail.com: https://hg.mozilla.org/integration/autoland/rev/fcad8df0a68b stylo: Implement all the remaining state pseudo-classes. r=heycam https://hg.mozilla.org/integration/autoland/rev/52fff0dd38c1 Update test expectations. r=emilio https://hg.mozilla.org/integration/autoland/rev/1d26488c7af2 Fix computation of border and outline-width when the border or outline style change. r=heycam https://hg.mozilla.org/integration/autoland/rev/ebeac2783f24 Avoid dumb copy-constructor. r=heycam https://hg.mozilla.org/integration/autoland/rev/43cff735bb7c Flush the overflow changed tracker when done with restyles. r=heycam https://hg.mozilla.org/integration/autoland/rev/ea75131f2054 more test expectation updates. r=heycam

https://hg.mozilla.org/mozilla-central/rev/fcad8df0a68b https://hg.mozilla.org/mozilla-central/rev/52fff0dd38c1 https://hg.mozilla.org/mozilla-central/rev/1d26488c7af2 https://hg.mozilla.org/mozilla-central/rev/ebeac2783f24 https://hg.mozilla.org/mozilla-central/rev/43cff735bb7c https://hg.mozilla.org/mozilla-central/rev/ea75131f2054