More nsIAtom work happening in this bug: rename Atom as nsAtom, expose it in nsIAtom.h, and replace nsHtml5Atom with it.

Henri: MozReview apparently doesn't allow multiple reviews, but you might like to review the last patch ("Remove nsHtml5Atom").

Comment on attachment 8910634 [details] Bug 1401873 - Remove nsHtml5Atom. . https://reviewboard.mozilla.org/r/182070/#review187456

(In reply to Nicholas Nethercote [:njn] from comment #6) > Henri: MozReview apparently doesn't allow multiple reviews, but you might > like to review the last patch ("Remove nsHtml5Atom"). I think it's supposed to allow multiple reviews, but probably autocomplete had put "henri" instead of "hsivonen" as the second requestee.

Comment on attachment 8910631 [details] Bug 1401873 - Expose nsAtom in nsIAtom.h. . https://reviewboard.mozilla.org/r/182064/#review187568 My only concern is the below. I did think from the commit message that `nsAtom` was about to get smaller, which I was excited about, though. :) ::: xpcom/ds/nsAtomTable.cpp:211 (Diff revision 1) > typedef mozilla::TrueType HasThreadSafeRefCnt; > > MozExternalRefCountType DynamicAddRef(); > MozExternalRefCountType DynamicRelease(); > > + bool HasRefs() { return mRefCnt > 0; } This method doesn't feel like a good candidate for being made a public method; it's used as part of implementation details of the internal atom implementation. I guess this doesn't matter much as of this patch, but I see in later patches that `nsAtom` is made a more public data structure, and I think it would be a good idea to not expose this method there.

Comment on attachment 8910632 [details] Bug 1401873 - Remove nsHtml5Atom. . https://reviewboard.mozilla.org/r/182066/#review187572 I kind of like keeping the factory functions and making the factory functions private...but I guess you still have to declare `friend` for those, and then the friends have access to the constructors as well, so it's not clear that situation would be much better. With factory functions, at least, you'd ensure that the `dont_AddRef` logic was at least in one place, rather than in several. ::: xpcom/ds/nsAtomTable.cpp:115 (Diff revision 1) > + // nsAtom constructors are private because they must be constructed in very > + // restricted ways. The following functions are those responsible. "The following functions are those responsible" sounds a bit cut off to me. Maybe "The following functions are the only functions permitted to construct nsAtoms at this time."? And, for good measure, "Please use caution if you feel the need to add functions to this list."?

Comment on attachment 8910630 [details] Bug 1401873 - Rename Atom as nsAtom. . https://reviewboard.mozilla.org/r/182062/#review187638

Comment on attachment 8910633 [details] Bug 1401873 - Expose nsAtom in nsIAtom.h. . https://reviewboard.mozilla.org/r/182068/#review187640 r=me with the concerns below addressed. ::: xpcom/ds/nsIAtom.h:105 (Diff revision 1) > + ~nsAtom(); > + > + NS_DECL_NSIATOM > + NS_IMETHOD QueryInterface(REFNSIID aIID, void** aInstancePtr) final; > + typedef mozilla::TrueType HasThreadSafeRefCnt; > + > + MozExternalRefCountType DynamicAddRef(); > + MozExternalRefCountType DynamicRelease(); Given that this class is now receiving wider exposure, can we privatize the destructor and the `Dynamic*` refcounting functions? The privatization of `HasRefs` was discussed in a previous patch, and addressing it here, there, or in a separate patch would be a good thing too.

Comment on attachment 8910634 [details] Bug 1401873 - Remove nsHtml5Atom. . https://reviewboard.mozilla.org/r/182070/#review187642 ::: parser/html/nsHtml5AtomTable.cpp:11 (Diff revision 1) > #include "nsThreadUtils.h" > > +nsAtom* > +NewHtml5Atom(const nsAString& aStr) > +{ > + return new nsAtom(nsAtom::AtomKind::HTML5Atom, aStr, 0); Might want to make a note here or in the `friend` declaration in `nsAtom` that `HTML5Atom` kinds don't use reference counting and therefore we don't need to return an `already_AddRefed<nsAtom>` here. Or do return an `already_AddRefed` here and `.take()` it in the `nsHtml5AtomEntry` constructor? ::: xpcom/ds/nsAtomTable.cpp:191 (Diff revision 1) > NS_IMPL_QUERY_INTERFACE(nsAtom, nsIAtom); > > NS_IMETHODIMP > nsAtom::ToUTF8String(nsACString& aBuf) > { > + MOZ_ASSERT(!IsHTML5Atom(), "Attempt to AddRef an HTML5 atom"); Surely the assertion message here is incorrect. :) ::: xpcom/ds/nsAtomTable.cpp:199 (Diff revision 1) > } > > NS_IMETHODIMP_(size_t) > nsAtom::SizeOfIncludingThis(MallocSizeOf aMallocSizeOf) > { > + MOZ_ASSERT(!IsHTML5Atom(), "Attempt to AddRef an HTML5 atom"); ...and here.

> Given that this class is now receiving wider exposure, can we privatize the > destructor and the `Dynamic*` refcounting functions? The destructor is used in AtomTableClearEntry() and GCAtomTableLocked(). If I make the destructor private, these will have to be friends. Do you want that? Perhaps I should create an nsAtomFriend class, and make AtomTableClearEntry, GCAtomTableLocked(), and RegisterStaticAtoms() all static functions within it.

> I kind of like keeping the factory functions and making the factory > functions private...but I guess you still have to declare `friend` for > those, and then the friends have access to the constructors as well, so it's > not clear that situation would be much better. With factory functions, at > least, you'd ensure that the `dont_AddRef` logic was at least in one place, > rather than in several. It seemed weird to have private constructors *and* private factory methods, which is why I got rid of the factory methods. Having to duplicate the dont_AddRef calls is a downside, but I judged that preferable...

BTW, it might not be clear, but my plan is for nsIAtom to eventually be merged into nsAtom, via bug 1400459 and bug 1400460.

froydnj: I made everything private as you requested, partly by introducing a new nsAtomFriend class. I also combined parts 2, 3 and 4 into a single patch because they were highly related. The factory methods are still gone, though.

Comment on attachment 8910632 [details] Bug 1401873 - Remove nsHtml5Atom. . https://reviewboard.mozilla.org/r/182066/#review187850

(In reply to Nicholas Nethercote [:njn] from comment #19) > froydnj: I made everything private as you requested, partly by introducing a > new nsAtomFriend class. I also combined parts 2, 3 and 4 into a single patch > because they were highly related. The factory methods are still gone, though. WFM. Thanks.

https://hg.mozilla.org/integration/mozilla-inbound/rev/6602b0670805a8303607ea45bebcbb15a59ee222 Bug 1401873 - Rename Atom as nsAtom. r=froydnj. https://hg.mozilla.org/integration/mozilla-inbound/rev/9c24fa59b0073d866fe44e6c969c5481d78a6793 Bug 1401873 - Expose nsAtom in nsIAtom.h. r=froydnj. https://hg.mozilla.org/integration/mozilla-inbound/rev/8203dbc82127f9e19408d08ab4d3f1ddd64be40c Bug 1401873 - Remove nsHtml5Atom. r=froydnj,hsivonen.

https://hg.mozilla.org/mozilla-central/rev/6602b0670805 https://hg.mozilla.org/mozilla-central/rev/9c24fa59b007 https://hg.mozilla.org/mozilla-central/rev/8203dbc82127