Status

()

enhancement
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: njn, Assigned: njn)

Tracking

(Blocks 1 bug)

Trunk
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(4 attachments, 1 obsolete attachment)

Assignee

Description

2 years ago
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.
Assignee

Updated

2 years ago
Blocks: 1400460
Assignee

Updated

2 years ago
Depends on: 1401873
Assignee

Comment 1

2 years ago
It's infallible.
Attachment #8911679 - Flags: review?(nfroyd)
Assignee

Comment 2

2 years ago
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)
Assignee

Comment 3

2 years ago
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)
Assignee

Updated

2 years ago
Depends on: 1402772
Assignee

Comment 4

2 years ago
Attachment #8911687 - Flags: review?(nfroyd)
Assignee

Updated

2 years ago
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+
Assignee

Comment 6

2 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/1a063bcbef8a7bd9e6564c5b2c29264163066664
Bug 1400459 (part 1) - Remove return value from nsIAtom::ToUTF8String(). r=froydnj.
Assignee

Updated

2 years ago
Keywords: leave-open
Comment hidden (mozreview-request)
Assignee

Comment 9

2 years ago
mozreview-review
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 10

2 years ago
mozreview-review
Comment on attachment 8912519 [details]
Bug 1400459 (part 2) - Devirtualize nsIAtom. .

https://reviewboard.mozilla.org/r/183828/#review189060
Attachment #8912519 - Flags: review?(cam) → review+

Comment 11

2 years ago
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
Assignee

Updated

2 years ago
Status: NEW → RESOLVED
Last Resolved: 2 years ago
Resolution: --- → FIXED
Assignee

Updated

2 years ago
Blocks: 1257126
You need to log in before you can comment on or make changes to this bug.