1494515 - Some static atom cleanups

Status: RESOLVED FIXED

(Core :: XPCOM, enhancement)

Description

[Nicholas Nethercote [inactive]]

These cleanups are possible due to previous changes, esp. bug 1482782 which combined all the static atom sources into nsGkAtoms.

Comment 1

[Nicholas Nethercote [inactive]]

Attachment: Rework the comments about static atoms

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
read.

Comment 2

[Nicholas Nethercote [inactive]]

Attachment: Clean up static atom registration

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.

Comment 3

[Nicholas Nethercote [inactive]]

Attachment: Remove an unused argument from nsStaticAtom's constructor

Presumably the commit that introduced `aStringOffset` could have removed this,
but overlooked it.

Comment 4

[Nathan Froyd [:froydnj]]

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.

Comment 5

[Nicholas Nethercote [inactive]]

> 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.

Comment 6

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/aab722576793
Rework the comments about static atoms. r=froydnj
https://hg.mozilla.org/integration/mozilla-inbound/rev/1a9268216cd7
Clean up static atom registration. r=froydnj
https://hg.mozilla.org/integration/mozilla-inbound/rev/cb1bd4097d6b
Remove an unused argument from nsStaticAtom's constructor. r=froydnj

Comment 7

[Cristina Coroiu [:ccoroiu]]

https://hg.mozilla.org/mozilla-central/rev/aab722576793
https://hg.mozilla.org/mozilla-central/rev/1a9268216cd7
https://hg.mozilla.org/mozilla-central/rev/cb1bd4097d6b