Closed Bug 1494515 Opened Last year Closed Last year

Some static atom cleanups


(Core :: XPCOM, enhancement)

Not set



Tracking Status
firefox64 --- fixed


(Reporter: njn, Assigned: njn)



(3 files)

These cleanups are possible due to previous changes, esp. bug 1482782 which combined all the static atom sources into nsGkAtoms.
nsGkAtoms.h has a big comment explaining how static atom definitions get
expanded by macros. This comment was very useful when there were multiple
sources of static atoms and their definitions used macros a lot. But bug
1482782 combined all the static atom sources into nsGkAtoms and removed a lot
of macro use. So now the comment is now something of a hindrance, duplicating
quite a bit of the code (and not entirely accurately).

This commit removes the big comment, and moves the still-useful parts inline
with the code. This makes things much easier to follow.

This commit also reformats some of the remaining macros so they are easier to
Attachment #9012483 - Flags: review?(nfroyd)
Assignee: nobody → n.nethercote
Static atom registration is a bit of a mess. NS_InitAtomTable() calls
nsGkAtoms::RegisterStaticAtoms(), which calls NS_RegisterStaticAtoms(); i.e. we
go from nsAtomTable.cpp to nsGkAtoms.cpp and back.

(And NS_RegisterStaticAtoms() is declared in a .cpp file, not a .h file!)

This commit makes NS_InitAtomTable() a friend of nsGkAtoms, so NS_InitAtomTable
can see nsGkAtoms's atoms array directly, thus removing the need for
NS_RegisterAtomTable() and nsGkAtoms::RegisterStaticAtoms().

This commit also removes an out-of-date part of a comment from XPCOMInit.cpp.
Attachment #9012484 - Flags: review?(nfroyd)
Presumably the commit that introduced `aStringOffset` could have removed this,
but overlooked it.
Attachment #9012485 - Flags: review?(nfroyd)
Attachment #9012483 - Flags: review?(nfroyd) → review+
Attachment #9012484 - Flags: review?(nfroyd) → review+
Comment on attachment 9012485 [details] [diff] [review]
Remove an unused argument from nsStaticAtom's constructor

Review of attachment 9012485 [details] [diff] [review]:

::: xpcom/ds/nsAtom.h
@@ -107,5 @@
>    typedef mozilla::TrueType HasThreadSafeRefCnt;
>  protected:
>    // Used by nsStaticAtom.
> -  constexpr nsAtom(const char16_t* aStr, uint32_t aLength, uint32_t aHash)

You would think one of our ever-zealous compiler warning settings would have complained about this.
Attachment #9012485 - Flags: review?(nfroyd) → review+
> You would think one of our ever-zealous compiler warning settings would have complained about this.

That requires -Wunused-parameter, which we currently don't enable. See bug 1232547 for details.
Pushed by
Rework the comments about static atoms. r=froydnj
Clean up static atom registration. r=froydnj
Remove an unused argument from nsStaticAtom's constructor. r=froydnj
You need to log in before you can comment on or make changes to this bug.