Closed Bug 1362302 Opened 3 years ago Closed 3 years ago

Use nsIAtom for counter style name

Categories

(Core :: CSS Parsing and Computation, enhancement)

53 Branch
enhancement
Not set

Tracking

()

RESOLVED FIXED
mozilla55
Tracking Status
firefox55 --- fixed

People

(Reporter: xidorn, Assigned: xidorn)

References

Details

(Whiteboard: [stylo])

Attachments

(2 files)

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
Whiteboard: [stylo]
Assignee: nobody → xidorn+moz
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?
Attachment #8864803 - Flags: review?(dbaron) → review+
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
Attachment #8864824 - Flags: review?(manishearth) → review+
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.
(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 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/
Attachment #8864803 - Flags: review+ → review?(dbaron)
Attachment #8864803 - Flags: review?(dbaron)
Servo side: servo/servo#16750
Comment on attachment 8864803 [details]
Bug 1362302 part 1 - Use nsIAtom for counter style names.

https://reviewboard.mozilla.org/r/136482/#review139796
Attachment #8864803 - Flags: review?(dbaron) → review+
Pushed by xquan@mozilla.com:
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
https://hg.mozilla.org/mozilla-central/rev/9a8a5130ea32
https://hg.mozilla.org/mozilla-central/rev/69a52aed9d22
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
You need to log in before you can comment on or make changes to this bug.