1366735 - Use struct rather than nsCSSValue::Array to store counter functions

Status: RESOLVED FIXED

(Core :: CSS Parsing and Computation, enhancement)

Description

[Xidorn Quan [:xidorn] UTC+10]

So that we don't need to resolve counter style for each counter use node, and we can share anonymous counter styles among use nodes.

I make it block bug 1363596 because I feel it would make that one easier to implement.

Comment 1

[Xidorn Quan [:xidorn] UTC+10]

Attachment: Bug 1366735 part 1 - Change counter functions to use struct rather than nsCSSValue::Array.

Review commit: https://reviewboard.mozilla.org/r/141662/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/141662/

Comment 2

[Xidorn Quan [:xidorn] UTC+10]

Attachment: Bug 1366735 part 2 - Use the new struct in stylo.

Review commit: https://reviewboard.mozilla.org/r/141664/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/141664/

Comment 3

[Xidorn Quan [:xidorn] UTC+10]

Attachment: Bug 1366735 part 3 - Remove SetCounterStyleDirty.

When the counter style in the style struct changes, CalcDifference
would return ReconstructFrame, which should cause corresponding use
node to be reconstructed. That means a use node with retired counter
style should always be destroyed in the next flush, so it makes no
sense to reset them anymore.

However, we would still need to mark counter lists dirty because
otherwise their content may not get update when there are changes to
counter styles which are still valid.

Review commit: https://reviewboard.mozilla.org/r/141666/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/141666/

Comment 4

[Xidorn Quan [:xidorn] UTC+10]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=5add5d0cfbaf792595f8969e8ffe3c40e2302a80

Comment 5

[Cameron McCormack (:heycam)]

Comment on attachment 8870212 [details]
Bug 1366735 part 1 - Change counter functions to use struct rather than nsCSSValue::Array.

https://reviewboard.mozilla.org/r/141662/#review145388

r=me, since I accidentally reviewed this before realising it wasn't addressed to me...

::: layout/style/nsRuleNode.cpp:8922
(Diff revision 1)
> +              CounterStyleManager()->BuildCounterStyle(style.GetAtomValue());
> +          } else if (style.GetUnit() == eCSSUnit_Symbols) {
> +            counterFunc->mCounterStyle =
> +              new AnonymousCounterStyle(style.GetArrayValue());
> +          } else {
> +            MOZ_ASSERT_UNREACHABLE("Unknow counter style");

Unknown

Comment 6

[Cameron McCormack (:heycam)]

Comment on attachment 8870213 [details]
Bug 1366735 part 2 - Use the new struct in stylo.

https://reviewboard.mozilla.org/r/141664/#review145400

::: layout/style/nsStyleStruct.h:3065
(Diff revision 1)
> +    // One and only one of mCounterStyle and mCounterStyleName must be
> +    // non-null at any time.
>      mozilla::CounterStylePtr mCounterStyle;
> +    nsCOMPtr<nsIAtom> mCounterStyleName;

At this point I wonder whether this should be a class, encapsulating that requirement, rather than a struct.

::: servo/components/style/gecko_bindings/sugar/ns_com_ptr.rs:19
(Diff revision 1)
>          self.mRawPtr
>      }
> +
> +    /// Set this pointer from an addrefed raw pointer.
> +    #[inline]
> +    pub unsafe fn set_raw<U>(&mut self, ptr: *mut T) {

I think the name set_raw doesn't make it obvious that it's assigning something without AddRefing.

For RefPtr, we have RefPtr::from_addrefed, so we do

  let refptr = RefPtr::from_addrefed(raw_ptr);
  some_gecko_thing.mSomething.set_move(refptr);

Should we follow the same pattern?  (Well, it is pretty wordy, but I think we should be consistent.)

Comment 7

[Xidorn Quan [:xidorn] UTC+10]

Comment on attachment 8870213 [details]
Bug 1366735 part 2 - Use the new struct in stylo.

https://reviewboard.mozilla.org/r/141664/#review145400

> At this point I wonder whether this should be a class, encapsulating that requirement, rather than a struct.

Maybe... but it seems to me the code is pretty short, and this pattern would only be used in these two places, so encapsulating that may not save us any line of code.

I was actually considering merging `nsCOMPtr<nsIAtom>` into `CounterStylePtr`, which is doable and would save us a word in memory for the structs. But I think we can do that later. If we eventually want to do something like that, we probably don't want to encapsulate it for now.

> I think the name set_raw doesn't make it obvious that it's assigning something without AddRefing.
> 
> For RefPtr, we have RefPtr::from_addrefed, so we do
> 
>   let refptr = RefPtr::from_addrefed(raw_ptr);
>   some_gecko_thing.mSomething.set_move(refptr);
> 
> Should we follow the same pattern?  (Well, it is pretty wordy, but I think we should be consistent.)

Probably `set_from_addrefed`?

Comment 8

[Xidorn Quan [:xidorn] UTC+10]

Not sure why MozReview cancels r? heycam for part 2...

Comment 9

[Cameron McCormack (:heycam)]

(In reply to Xidorn Quan [:xidorn] UTC+10 from comment #7)
> Probably `set_from_addrefed`?

OK.  (Maybe we should have that for RefPtr too then.  Unless there's some advantage to the pattern of x.move(RefPtr::from_addrefed(...)).)

Comment 10

[Xidorn Quan [:xidorn] UTC+10]

(In reply to Cameron McCormack (:heycam) from comment #9)
> (In reply to Xidorn Quan [:xidorn] UTC+10 from comment #7)
> > Probably `set_from_addrefed`?
> 
> OK.  (Maybe we should have that for RefPtr too then.  Unless there's some
> advantage to the pattern of x.move(RefPtr::from_addrefed(...)).)

Maybe. In long term, I think we want something like AddRefed<T> similar to our already_AddRefed in Gecko, and make things return that (e.g. "impl From<Atom> for AddRefed<nsIAtom>" and verse vice, and make binding functions use that as well), so we can have less unsafe code, and stronger type-safety.

Comment 11

[Xidorn Quan [:xidorn] UTC+10]

Comment on attachment 8870212 [details]
Bug 1366735 part 1 - Change counter functions to use struct rather than nsCSSValue::Array.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/141662/diff/1-2/

Comment 12

[Xidorn Quan [:xidorn] UTC+10]

Comment on attachment 8870213 [details]
Bug 1366735 part 2 - Use the new struct in stylo.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/141664/diff/1-2/

Comment 13

[Xidorn Quan [:xidorn] UTC+10]

Comment on attachment 8870214 [details]
Bug 1366735 part 3 - Remove SetCounterStyleDirty.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/141666/diff/1-2/

Comment 14

[Cameron McCormack (:heycam)]

Comment on attachment 8870213 [details]
Bug 1366735 part 2 - Use the new struct in stylo.

https://reviewboard.mozilla.org/r/141664/#review145736

Comment 15

[Cameron McCormack (:heycam)]

Comment on attachment 8870214 [details]
Bug 1366735 part 3 - Remove SetCounterStyleDirty.

https://reviewboard.mozilla.org/r/141666/#review145830

Can you please mention in a comment in nsStyleContent::CalcDifference that we rely on returning ReconstructFrame so that we don't have counter use nodes with references to stale counter styles.

Comment 16

[Xidorn Quan [:xidorn] UTC+10]

Servo side: servo/servo#17016

Comment 17

[Xidorn Quan [:xidorn] UTC+10]

Comment on attachment 8870212 [details]
Bug 1366735 part 1 - Change counter functions to use struct rather than nsCSSValue::Array.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/141662/diff/2-3/

Comment 18

[Xidorn Quan [:xidorn] UTC+10]

Comment on attachment 8870213 [details]
Bug 1366735 part 2 - Use the new struct in stylo.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/141664/diff/2-3/

Comment 19

[Xidorn Quan [:xidorn] UTC+10]

Comment on attachment 8870214 [details]
Bug 1366735 part 3 - Remove SetCounterStyleDirty.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/141666/diff/2-3/

Comment 20

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/787d3a0c5dd1
part 1 - Change counter functions to use struct rather than nsCSSValue::Array. r=heycam
https://hg.mozilla.org/integration/autoland/rev/3da40efe9851
part 2 - Use the new struct in stylo. r=heycam
https://hg.mozilla.org/integration/autoland/rev/ed34125549db
part 3 - Remove SetCounterStyleDirty. r=heycam

Comment 21

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/mozilla-central/rev/787d3a0c5dd1
https://hg.mozilla.org/mozilla-central/rev/3da40efe9851
https://hg.mozilla.org/mozilla-central/rev/ed34125549db