Closed Bug 1400459 Opened 7 years ago Closed 7 years ago

Devirtualize nsIAtom

Categories

(Core :: XPCOM, enhancement)

Product:
Core
Component:
XPCOM
Type:
enhancement
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED

People

(Reporter: n.nethercote, Assigned: n.nethercote)

References

Details

Attachments

(4 files, 1 obsolete file)

(part 1) - Remove return value from nsIAtom::ToUTF8String()
(deleted), patch
froydnj
: review+
Details | Diff | Splinter Review
(part 2a) - XXX: convert nsCOMPtr<nsIAtom> to RefPtr<nsIAtom>
(deleted), patch
froydnj
: review+
Details | Diff | Splinter Review
(part 2) - Devirtualize nsIAtom
(deleted), patch
froydnj
: review+
Details | Diff | Splinter Review
Bug 1400459 (part 2) - Devirtualize nsIAtom. .
(deleted), text/x-review-board-request
heycam
: review+
n.nethercote
: review+
Details
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) (deleted) — — 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
Attached patch (part 2) - Devirtualize nsIAtom (deleted) — — Splinter Review
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: 7 years ago
Resolution: --- → FIXED
Blocks: 1257126
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: