Remove XULDocument::mRefMap and its corresponding complexity (and maybe devirtualize nsIDocument::GetElementByID)

RESOLVED FIXED in Firefox 59

Status

()

enhancement
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: Gijs, Assigned: bzbarsky)

Tracking

({dev-doc-needed})

Trunk
mozilla59
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox59 fixed)

Details

Attachments

(5 attachments)

(In reply to Boris Zbarsky [:bz] from bug 1425356 comment #17)
> Was the "ref" attribute on XUL elements only used for this template stuff? 
> If so, can we remove XULDocument::mRefMap and its corresponding complexity
> as a followup (and devirtualize nsIDocument::GetElementByID!)?  It looks to
> me per
> https://searchfox.org/mozilla-central/
> search?q=%20ref%3D%22&case=false&regexp=false&path=xul like the "ref"
> attribute is only used for template bits...

I don't know anything about mRefMap and GetElementByID particularly, but based on the searchfox query, I think the premise that "ref" is only used for templates is correct.
Note: The devirtualization already happened in bug 1426494, but we can still remove some complexity here.
'ref' used to be used for templates, but those are gone as of bug 1425356 being
fixed.

MozReview-Commit-ID: GerfZrckypp
Attachment #8939989 - Flags: review?(bugs)
Assignee: nobody → bzbarsky
Status: NEW → ASSIGNED
MozReview-Commit-ID: He9RTzZMmSr
Attachment #8939992 - Flags: review?(gijskruitbosch+bugs)
The WebIDL for this was already removed in bug 1425356.

MozReview-Commit-ID: HWxPe3a0Pmi
Attachment #8939993 - Flags: review?(bugs)
Comment on attachment 8939992 [details] [diff] [review]
part 4.  Remove uses of 'ref' in XUL trees

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

LGTM, thanks for doing this!
Attachment #8939992 - Flags: review?(gijskruitbosch+bugs) → review+
Attachment #8939993 - Flags: review?(bugs) → review+
Attachment #8939991 - Flags: review?(bugs) → review+
Comment on attachment 8939990 [details] [diff] [review]
part 2.  Remove the now-nearly-unused XULDocument::GetElementsForID method


>-        // This will clear the array if there are no elements.
>-        GetElementsForID(id, elements);
>-        if (!elements.Count()) {
>+        auto* entry = mIdentifierMap.GetEntry(id);
Could you not use auto here. I have no idea what entry is here.

>+        if (!entry) {
>+            continue;
>+        }
>+
>+        // We want to hold strong refs to the elements while applying
>+        // persistent attributes, just in case.
>+        elements.Clear();
>+        elements.SetCapacity(entry->GetIdElements().Length());
>+        for (auto* element : entry->GetIdElements()) {
I wouldn't mind the actual type here either. auto* doesn't really help readability.
Attachment #8939990 - Flags: review?(bugs) → review+
> Could you not use auto here. I have no idea what entry is here.

I guess mainly it doesn't matter too much what the exact entry type is, and I make it clear that we're holding a pointer to the entry.  But sure, I can list the explicit type...
Comment on attachment 8939989 [details] [diff] [review]
part 1.  Remove XULDocument's mRefMap member, since there should be no XUL elements with 'ref' attributes anymore

(MDN should be updated to hint that all this stuff related to templates is gone.)
Attachment #8939989 - Flags: review?(bugs) → review+
> (MDN should be updated to hint that all this stuff related to templates is gone.)

Good point.  I added dev-doc-needed on bug 1425356.
I guess the "ref no longer acts like id" bit should get documented too.
Keywords: dev-doc-needed
Pushed by bzbarsky@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/501c70abf837
part 1.  Remove XULDocument's mRefMap member, since there should be no XUL elements with 'ref' attributes anymore.  r=smaug
https://hg.mozilla.org/integration/mozilla-inbound/rev/85e218bf000c
part 2.  Remove the now-nearly-unused XULDocument::GetElementsForID method.  r=smaug
https://hg.mozilla.org/integration/mozilla-inbound/rev/cee585723368
part 3.  Remove the special-casing of the 'ref' attribute in CanBroadcast.  r=smaug
https://hg.mozilla.org/integration/mozilla-inbound/rev/37fdd4a04f4e
part 4.  Remove uses of 'ref' in XUL trees.  r=gijs
https://hg.mozilla.org/integration/mozilla-inbound/rev/96efa1b6f4d5
part 5.  Remove the 'ref' getter/setter on XULElement.  r=smaug
Pushed by bzbarsky@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/c713ef0ecf09
part 1.  Remove XULDocument's mRefMap member, since there should be no XUL elements with 'ref' attributes anymore.  r=smaug
https://hg.mozilla.org/integration/mozilla-inbound/rev/07e377e19c7d
part 2.  Remove the now-nearly-unused XULDocument::GetElementsForID method.  r=smaug
https://hg.mozilla.org/integration/mozilla-inbound/rev/c9fad2f64448
part 3.  Remove the special-casing of the 'ref' attribute in CanBroadcast.  r=smaug
https://hg.mozilla.org/integration/mozilla-inbound/rev/cb168f6a6bfd
part 4.  Remove uses of 'ref' in XUL trees.  r=gijs
https://hg.mozilla.org/integration/mozilla-inbound/rev/f5ed4ddcc512
part 5.  Remove the 'ref' getter/setter on XULElement.  r=smaug
Flags: needinfo?(bzbarsky)
You need to log in before you can comment on or make changes to this bug.