Closed Bug 1400459 Opened 3 years ago Closed 3 years ago

Devirtualize nsIAtom

Categories

(Core :: XPCOM, enhancement)

enhancement
Not set
normal

Tracking

()

RESOLVED FIXED

People

(Reporter: njn, Assigned: njn)

References

(Blocks 1 open bug)

Details

Attachments

(4 files, 1 obsolete file)

I want to devirtualize nsIAtom, which means:

- Making it no longer inherit from nsISupports.

- Get rid of nsHtml5Atom.

- Convert nsCOMPtr<nsIAtom> to RefPtr<nsIAtom> everywhere.

- Convert nsCOMArray<nsIAtom> to nsTArray<RefPtr<nsIAtom>>.

- Similarly, convert hash tables that contain nsIAtoms appropriately.

This will make things faster, because all the nsIAtom operations will be non-virtual.

That's enough for this bug. We can then rename nsIAtom as nsAtom in a follow-up bug, which will be a large but mechanical change.
Blocks: 1400460
Depends on: 1401873
It's infallible.
Attachment #8911679 - Flags: review?(nfroyd)
I did this patch with a global search-and-replace, and I'm uploading it
separately from the subsequent patch, which was hand-written, for ease of
reviewing. I will merge this patch with the subsequent patch before landing.
Attachment #8911680 - Flags: review?(nfroyd)
Attached patch (part 2) - Devirtualize nsIAtom (obsolete) — Splinter Review
This patch merges nsAtom into nsIAtom. For the moment, both names can be used
interchangeable due to a typedef. The patch also devirtualizes nsIAtom, by
removing its inheritance from nsISupports, removing NS_DECL_NSIATOM, and
dropping the use of NS_IMETHOD_. It also removes nsIAtom's IIDs.

These changes trigger knock-on changes throughout the codebase, changing the
types of lots of things as follows.

- nsCOMPtr<nsIAtom> --> RefPtr<nsIAtom>

- nsCOMArray<nsIAtom> --> nsTArray<RefPtr<nsIAtom>>
  - Count() --> Length()
  - ObjectAt() --> ElementAt()
  - AppendObject() --> AppendElement()
  - RemoveObjectAt() --> RemoveElementAt()

- ns*Hashtable<nsISupportsHashKey, ...> -->
  ns*Hashtable<nsRefPtrHashKey<nsIAtom>, ...>

- nsInterfaceHashtable<T, nsIAtom> --> nsRefPtrHashtable<T, nsIAtom>
  - This requires adding a Get() method to nsRefPtrHashtable that it lacks but
    nsInterfaceHashtable has.

- nsCOMPtr<nsIMutableArray> --> nsTArray<RefPtr<nsIAtom>>
  - nsArrayBase::Create() --> nsTArray()
  - GetLength() --> Length()
  - do_QueryElementAt() --> operator[]

The patch also has some changes to Rust code that manipulates nsIAtom.
Attachment #8911681 - Flags: review?(nfroyd)
Depends on: 1402772
Attachment #8911687 - Flags: review?(nfroyd)
Attachment #8911681 - Attachment is obsolete: true
Attachment #8911681 - Flags: review?(nfroyd)
Comment on attachment 8911679 [details] [diff] [review]
(part 1) - Remove return value from nsIAtom::ToUTF8String()

Review of attachment 8911679 [details] [diff] [review]:
-----------------------------------------------------------------

::: xpcom/ds/nsIAtom.h
@@ +21,5 @@
>  {
>  public:
>    NS_DECLARE_STATIC_IID_ACCESSOR(NS_IATOM_IID)
>  
> +  NS_IMETHOD_(void) ToUTF8String(nsACString& aString) = 0;

Maybe we should move this down next to the ToString() method, so it's obvious that both of them are infallible?
Attachment #8911679 - Flags: review?(nfroyd) → review+
Attachment #8911680 - Flags: review?(nfroyd) → review+
Attachment #8911687 - Flags: review?(nfroyd) → review+
https://hg.mozilla.org/integration/mozilla-inbound/rev/1a063bcbef8a7bd9e6564c5b2c29264163066664
Bug 1400459 (part 1) - Remove return value from nsIAtom::ToUTF8String(). r=froydnj.
Keywords: leave-open
Comment on attachment 8912519 [details]
Bug 1400459 (part 2) - Devirtualize nsIAtom. .

https://reviewboard.mozilla.org/r/183828/#review189044

Carrying over r=froydnj.
Attachment #8912519 - Flags: review+
Attachment #8912519 - Flags: review?(nfroyd) → review?(cam)
Comment on attachment 8912519 [details]
Bug 1400459 (part 2) - Devirtualize nsIAtom. .

https://reviewboard.mozilla.org/r/183828/#review189060
Attachment #8912519 - Flags: review?(cam) → review+
Pushed by cmccormack@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/1ede5092b369
(part 2) - Devirtualize nsIAtom. r=heycam.
This all landed.
Keywords: leave-open
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Blocks: 1257126
You need to log in before you can comment on or make changes to this bug.