1362302 - Use nsIAtom for counter style name

Status: RESOLVED FIXED

(Core :: CSS Parsing and Computation, enhancement)

Description

[Xidorn Quan [:xidorn] UTC+10]

We currently use string for counter style names. nsIAtom can be used instead so that:
1. no need to rehash the string when rebuild counter style table
2. no need to convert string between Servo form and Gecko form when crossing FFI

Comment 1

[Xidorn Quan [:xidorn] UTC+10]

Attachment: Bug 1362302 part 1 - Use nsIAtom for counter style names.

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

Comment 2

[Xidorn Quan [:xidorn] UTC+10]

Attachment: Bug 1362302 part 2 - Fix stylo issue after changing counter style names to nsIAtom.

Only counter() and counters() functions are affected so far, since
list-style-type hasn't been hooked with counter-style yet.

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

Comment 3

[Xidorn Quan [:xidorn] UTC+10]

Comment on attachment 8864803 [details]
Bug 1362302 part 1 - Use nsIAtom for counter style names.

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

Comment 4

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 8864803 [details]
Bug 1362302 part 1 - Use nsIAtom for counter style names.

https://reviewboard.mozilla.org/r/136482/#review139758

r=dbaron with these comments addressed (definitely the first, maybe the second)

(Please ignore the two blank ones -- in order to delete them I need to actually scroll to the right place in the massive MozReview UI... "Edit Review" only lets me make them blank and not delete them.)

::: layout/style/CounterStyleManager.cpp:1416
(Diff revision 2)
>  /* virtual */ CounterStyle*
>  CustomCounterStyle::GetFallback()
>  {
>    if (!mFallback) {
>      const nsCSSValue& value = mRule->GetDesc(eCSSCounterDesc_Fallback);
> -    if (value.UnitHasStringValue()) {
> +    if (value.GetUnit() != eCSSUnit_Null) {

It's safer to check for the actual type you want rather than != Null.

::: layout/style/nsCSSParser.cpp:4951
(Diff revision 2)
>  }
>  
>  bool
>  CSSParserImpl::ParseCounterStyleNameValue(nsCSSValue& aValue)
>  {
> -  nsString name;
> +  if (nsCOMPtr<nsIAtom> name = ParseCounterStyleName(false)) {



::: layout/style/nsCSSParser.cpp:4952
(Diff revision 2)
>  
>  bool
>  CSSParserImpl::ParseCounterStyleNameValue(nsCSSValue& aValue)
>  {
> -  nsString name;
> -  if (ParseCounterStyleName(name, false)) {
> +  if (nsCOMPtr<nsIAtom> name = ParseCounterStyleName(false)) {
> +    aValue.SetAtomIdentValue(name.forget());



::: layout/style/nsCSSRuleProcessor.cpp:896
(Diff revision 2)
> -  nsDataHashtable<nsStringHashKey, nsCSSCounterStyleRule*> mCounterStyleRuleTable;
> +  // The hashtable doesn't need to hold a strong reference to the name
> +  // atom, because nsCSSCounterStyleRule always does. If the name changes
> +  // we need to discard this table and rebuild it anyway.
> +  nsDataHashtable<nsPtrHashKey<nsIAtom>,
> +                  nsCSSCounterStyleRule*> mCounterStyleRuleTable;

Maybe it's safer to just reference-count anyway?

Comment 5

[Manish Goregaokar [:manishearth]]

Comment on attachment 8864824 [details]
Bug 1362302 part 2 - Fix stylo issue after changing counter style names to nsIAtom.

https://reviewboard.mozilla.org/r/136512/#review139768

Comment 6

[Xidorn Quan [:xidorn] UTC+10]

Comment on attachment 8864803 [details]
Bug 1362302 part 1 - Use nsIAtom for counter style names.

https://reviewboard.mozilla.org/r/136482/#review139758

> It's safer to check for the actual type you want rather than != Null.

I actually intentionally use this approach instead of checking the actual type, because `GetAtomValue` has an assertion, so it would assert when something is changed to make it easier to locate the root cause. Probably I should add another assertion to just assert the exact unit I want here to make it clearer?

> Maybe it's safer to just reference-count anyway?

The key is never dereferenced, so I don't think it's worth that... We can probably add an assertion somewhere else (probably in `CounterStyleManager::BuildCounterStyle`) to assert that the rule we get is still using the name we query.

Comment 7

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

(In reply to Xidorn Quan [:xidorn] UTC+10 from comment #6)
> Comment on attachment 8864803 [details]
> Bug 1362302 part 1 - Use nsIAtom for counter style names.
> 
> https://reviewboard.mozilla.org/r/136482/#review139758
> 
> > It's safer to check for the actual type you want rather than != Null.
> 
> I actually intentionally use this approach instead of checking the actual
> type, because `GetAtomValue` has an assertion, so it would assert when
> something is changed to make it easier to locate the root cause. Probably I
> should add another assertion to just assert the exact unit I want here to
> make it clearer?

It's safer the other way, though, since only a single pattern of corrupt memory would lead to being treated as an atom, as opposed to all but one pattern.  It's also safer against future changes.  You can add your own assertion, though.

Comment 8

[Xidorn Quan [:xidorn] UTC+10]

Comment on attachment 8864803 [details]
Bug 1362302 part 1 - Use nsIAtom for counter style names.

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

Comment 9

[Xidorn Quan [:xidorn] UTC+10]

Comment on attachment 8864824 [details]
Bug 1362302 part 2 - Fix stylo issue after changing counter style names to nsIAtom.

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

Comment 10

[Xidorn Quan [:xidorn] UTC+10]

Comment on attachment 8864803 [details]
Bug 1362302 part 1 - Use nsIAtom for counter style names.

What about this?

https://reviewboard.mozilla.org/r/136482/diff/2-3/

Comment 11

[Xidorn Quan [:xidorn] UTC+10]

Comment on attachment 8864803 [details]
Bug 1362302 part 1 - Use nsIAtom for counter style names.

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

Comment 12

[Xidorn Quan [:xidorn] UTC+10]

Comment on attachment 8864824 [details]
Bug 1362302 part 2 - Fix stylo issue after changing counter style names to nsIAtom.

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

Comment 13

[Xidorn Quan [:xidorn] UTC+10]

Servo side: servo/servo#16750

Comment 14

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 8864803 [details]
Bug 1362302 part 1 - Use nsIAtom for counter style names.

https://reviewboard.mozilla.org/r/136482/#review139796

Comment 15

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/9a8a5130ea32
part 1 - Use nsIAtom for counter style names. r=dbaron
https://hg.mozilla.org/integration/autoland/rev/69a52aed9d22
part 2 - Fix stylo issue after changing counter style names to nsIAtom. r=manishearth

Comment 16

[Iris Hsiao [:ihsiao]]

https://hg.mozilla.org/mozilla-central/rev/9a8a5130ea32
https://hg.mozilla.org/mozilla-central/rev/69a52aed9d22