1445079 - Remove nsHTMLTags::sTagAtomTable[]

Status: RESOLVED FIXED

(Core :: DOM: HTML Parser, enhancement)

Description

[Nicholas Nethercote [inactive]]

nsHTMLTags::sTagAtomTable[] is completely unnecessary; we just end up with a pile of duplicate atom pointers.

Comment 1

[Nicholas Nethercote [inactive]]

Attachment: Bug 1445079 - Rename nsHTMLTags::sTagUnicodeTable[] as sTagNames[].

It's just an array of names, not a hash table.

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

Comment 2

[Nicholas Nethercote [inactive]]

Attachment: Bug 1445079 - Remove nsHTMLTags::sTagAtomTable[].

The atoms in nsHTMLTags are a subset of nsGkAtoms, which means that
sTagAtomTable[] currently ends up holding duplicate pointers to the same static
atoms.

This patch removes sTagAtomTable[]. The only place that used sTagAtomTable[]
was nsHTMLTags::AddRefTable(). It now instead calls NS_GetStaticAtom() to get
the static atoms registered by nsGkAtoms.

The patch also moves some checking of sTagNames from RegisterAtoms() (which is
removed) to AddRefTable().

All this reduces the number of duplicate static atoms from 148 to 12.

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

Comment 3

[Nathan Froyd [:froydnj]]

Comment on attachment 8958267 [details]
Bug 1445079 - Rename nsHTMLTags::sTagUnicodeTable[] as sTagNames[].

https://reviewboard.mozilla.org/r/227196/#review233160

Comment 4

[Nathan Froyd [:froydnj]]

Comment on attachment 8958268 [details]
Bug 1445079 - Remove nsHTMLTags::sTagAtomTable[].

https://reviewboard.mozilla.org/r/227198/#review233244

This seems like a win, even if the debug checking is a little more complicated.

::: parser/htmlparser/nsHTMLTags.cpp:48
(Diff revision 1)
> +#ifdef DEBUG
> +    uint32_t maxTagNameLength = 0;
> +#endif

Making this a `DebugOnly<uint32_t>` would enable getting rid of the preprocessor conditional here and at the `MOZ_ASSERT` outside of the loop.  You'd still want the conditional inside the loop, though, because you wouldn't want to do the string stuff...so not sure this change would be worth it.

Comment 5

[Nicholas Nethercote [inactive]]

> Making this a `DebugOnly<uint32_t>` would enable getting rid of the
> preprocessor conditional here and at the `MOZ_ASSERT` outside of the loop. 
> You'd still want the conditional inside the loop, though, because you
> wouldn't want to do the string stuff...so not sure this change would be
> worth it.

I tried DebugOnly<> but it didn't play nicely with std::max(). 

I will move all the checking stuff into a second loop. The loop header will be repeated, but I think that's worth it to have a single DEBUG block for all the checking.

Comment 6

[Nicholas Nethercote [inactive]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/f82612511ab33500b0e4c7f161bc49b7a7618c60
Bug 1445079 - Rename nsHTMLTags::sTagUnicodeTable[] as sTagNames[]. r=froydnj

https://hg.mozilla.org/integration/mozilla-inbound/rev/cffdea92ed42a8fce6dff6e1fbba9a7e8b25ea91
Bug 1445079 - Remove nsHTMLTags::sTagAtomTable[]. r=froydnj

Comment 7

[Arthur Iakab [arthur_iakab]]

https://hg.mozilla.org/mozilla-central/rev/f82612511ab3
https://hg.mozilla.org/mozilla-central/rev/cffdea92ed42