Review of attachment 9034771 [details] [diff] [review]: ----------------------------------------------------------------- Looks good, but the Rust code duplication shouldn't be needed. Just renaming all occurrences of CounterReset for CounterSetOrReset and using that in the new property as well as counter-reset should be enough. Alternatively, you can alias them everywhere using `pub use self::CounterReset as CounterSet;` in the relevant counters.rs files, but that may inhibit some code-sharing optimizations we do for cloning declarations, animations, and other bits (see all the grouby usage in properties.mako.rs and animated_properties.mako.rs), so I'd rather not do that. ::: layout/base/nsCSSFrameConstructor.cpp @@ +4602,5 @@ > RestoreFrameStateFor(aNewFrame, aState.mFrameState); > } > > if (aAllowCounters && > + mCounterManager.AddCounterChanges(aNewFrame)) { I think this fits on one line now. ::: layout/style/GeckoBindings.cpp @@ +1409,5 @@ > aContent->SetCounterResetAt(i, data.mCounter, data.mValue); > } > } > > +void Gecko_ClearAndResizeCounterSets(nsStyleContent* aContent, If you fancy about it, you can refactor this as well so there's a single Clear / Copy function for all three properties that takes an nsTArray<nsStyleCounterData>*, and just call it with `&mut self.gecko.m${counter_property}s` from gecko.mako.rs to reduce the duplication on the C++ side as well (it should allow you to remove the Set / Allocate functions from nsStyleStruct.h. Though if it gets annoying to touch the mako stuff to much (I know the feeling :/), I can do that myself in a followup bug :) ::: layout/style/nsStyleStruct.h @@ +2518,5 @@ > + mSets.SetLength(aCount); > + } > + > + void SetCounterSetAt(uint32_t aIndex, const nsString& aCounter, > + int32_t aValue) { nit: Please clang-format this before landing, indentation seems broken. ::: servo/components/style/values/generics/counters.rs @@ +71,5 @@ > +/// A generic value for the `counter-set` property. > +#[derive( > + Clone, Debug, Default, MallocSizeOf, PartialEq, SpecifiedValueInfo, ToComputedValue, ToCss, > +)] > +pub struct CounterSet<I>(Counters<I>); This should not be needed, since it is exactly the same as `CounterReset` in all regards, including Parse implementation. Instead, let's rename `CounterReset` to `CounterSetOrReset` everywhere, and just use it in both .mako.rs entries.
Bug 1518201 Comment 3 Edit History
Note: The actual edited comment in the bug view page will always show the original commenter’s name and original timestamp.
(Hidden by Administrator)
Review of attachment 9034771 [details] [diff] [review]: ----------------------------------------------------------------- Looks good, but the Rust code duplication shouldn't be needed. Just renaming all occurrences of CounterReset for CounterSetOrReset and using that in the new property as well as counter-reset should be enough. Alternatively, you can alias them everywhere using `pub use self::CounterReset as CounterSet;` in the relevant counters.rs files, but that may inhibit some code-sharing optimizations we do for cloning declarations, animations, and other bits (see all the grouby usage in properties.mako.rs and animated_properties.mako.rs), so I'd rather not do that. ::: layout/base/nsCSSFrameConstructor.cpp @@ +4602,5 @@ > RestoreFrameStateFor(aNewFrame, aState.mFrameState); > } > > if (aAllowCounters && > + mCounterManager.AddCounterChanges(aNewFrame)) { I think this fits on one line now. ::: layout/style/GeckoBindings.cpp @@ +1409,5 @@ > aContent->SetCounterResetAt(i, data.mCounter, data.mValue); > } > } > > +void Gecko_ClearAndResizeCounterSets(nsStyleContent* aContent, If you fancy about it, you can refactor this as well so there's a single Clear / Copy function for all three properties that takes an nsTArray<nsStyleCounterData>*, and just call it with `&mut self.gecko.m${counter_property}s` from gecko.mako.rs to reduce the duplication on the C++ side as well (it should allow you to remove the Set / Allocate functions from nsStyleStruct.h. Though if it gets annoying to touch the mako stuff to much (I know the feeling :/), I can do that myself in a followup bug :) ::: layout/style/nsStyleStruct.h @@ +2518,5 @@ > + mSets.SetLength(aCount); > + } > + > + void SetCounterSetAt(uint32_t aIndex, const nsString& aCounter, > + int32_t aValue) { ``` nit: Please clang-format this before landing, indentation seems broken. ::: servo/components/style/values/generics/counters.rs @@ +71,5 @@ > +/// A generic value for the `counter-set` property. > +#[derive( > + Clone, Debug, Default, MallocSizeOf, PartialEq, SpecifiedValueInfo, ToComputedValue, ToCss, > +)] > +pub struct CounterSet<I>(Counters<I>); This should not be needed, since it is exactly the same as `CounterReset` in all regards, including Parse implementation. Instead, let's rename `CounterReset` to `CounterSetOrReset` everywhere, and just use it in both .mako.rs entries.