Emilio Cobos Álvarez (:emilio)
	Comment 3 • 5 years ago • Edited

Comment on attachment 9034771 [details] [diff] [review]
[css-lists] Implement the counter-set property.

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.

	Emilio Cobos Álvarez (:emilio)
	Comment 4 • 5 years ago • Edited

Comment on attachment 9034772 [details] [diff] [review]
[css-lists] Add some basic tests for the counter-set property.

Review of attachment 9034772 [details] [diff] [review]:
-----------------------------------------------------------------

This looks great, thanks! :)

It may be useful to have some tests for dynamic changes to the property if you want, though given the previous patch is straight-forward, and that we don't have pre-existing tests for the other properties in WPT, looks like, I don't think I'd feel good enforcing it.

```patch
::: testing/web-platform/tests/css/css-lists/counter-reset-increment-set-display-contents.html
@@ -4,1 @@
>  <link rel="author" title="Rune Lillesveen" href="mailto:futhark@chromium.org">
```

Maybe we should rename the file to `counter-set-reset-increment-*`? Same for the other existing test.

No big deal if not though.

	Pulsebot
	Comment 12 • 5 years ago

Pushed by mpalmgren@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/467284f16c74
[css-lists] Implement the counter-set property.  r=emilio
https://hg.mozilla.org/integration/mozilla-inbound/rev/9535b36adb21
[css-lists] Add some basic tests for the counter-set property.  r=emilio

	Web Platform Test Sync Bot (Matrix: #interop:mozilla.org)
	Comment 14 • 5 years ago

Created web-platform-tests PR https://github.com/web-platform-tests/wpt/pull/16096 for changes under testing/web-platform/tests

	Web Platform Test Sync Bot (Matrix: #interop:mozilla.org)
	Comment 15 • 5 years ago

Can't merge web-platform-tests PR due to failing upstream checks:
Github PR https://github.com/web-platform-tests/wpt/pull/16096
* Taskcluster (pull_request) (https://tools.taskcluster.net/task-group-inspector/#/G3Pi1-yLRr-21WJFcozmzA)