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

Status: RESOLVED FIXED

(Core :: XUL, enhancement)

Description

[:Gijs (out for now; he/him)]

(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.

Comment 1

[Boris Zbarsky [:bzbarsky]]

Note: The devirtualization already happened in bug 1426494, but we can still remove some complexity here.

Comment 2

[Boris Zbarsky [:bzbarsky]]

Attachment: part 1. Remove XULDocument's mRefMap member, since there should be no XUL elements with 'ref' attributes anymore

'ref' used to be used for templates, but those are gone as of bug 1425356 being
fixed.

MozReview-Commit-ID: GerfZrckypp

Comment 3

[Boris Zbarsky [:bzbarsky]]

Attachment: part 2. Remove the now-nearly-unused XULDocument::GetElementsForID method

MozReview-Commit-ID: Gv23I8gLhem

Comment 4

[Boris Zbarsky [:bzbarsky]]

Attachment: part 3. Remove the special-casing of the 'ref' attribute in CanBroadcast

MozReview-Commit-ID: K8Zkedorr7U

Comment 5

[Boris Zbarsky [:bzbarsky]]

Attachment: part 4. Remove uses of 'ref' in XUL trees

MozReview-Commit-ID: He9RTzZMmSr

Comment 6

[Boris Zbarsky [:bzbarsky]]

Attachment: part 5. Remove the 'ref' getter/setter on XULElement

The WebIDL for this was already removed in bug 1425356.

MozReview-Commit-ID: HWxPe3a0Pmi

Comment 7

[:Gijs (out for now; he/him)]

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!

Comment 8

[Olli Pettay [:smaug][[e-mail]]]

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.

Comment 9

[Boris Zbarsky [:bzbarsky]]

> 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 10

[Olli Pettay [:smaug][[e-mail]]]

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.)

Comment 11

[Boris Zbarsky [:bzbarsky]]

> (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.

Comment 12

[Boris Zbarsky [:bzbarsky]]

I guess the "ref no longer acts like id" bit should get documented too.

Comment 13

[Pulsebot]

Pushed by [e-mail]:
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

Comment 14

[Cristina Coroiu [:ccoroiu]]

Backed out 5 changesets (bug 1426525) for bustage at build/src/layout/xul/tree/nsTreeContentView.cpp r=backout on a CLOSED TREE 

Failure push: https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&revision=96efa1b6f4d5266a3c5cabc24ac46ca701986b8a&filter-classifiedState=unclassified&filter-resultStatus=testfailed&filter-resultStatus=busted&filter-resultStatus=exception&selectedJob=154423969

Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=154423969&repo=mozilla-inbound&lineNumber=16959

Backout: https://hg.mozilla.org/integration/mozilla-inbound/rev/44d2697ba0a3456e3016e356abb4663b6aff1a9b

Comment 15

[Pulsebot]

Pushed by [e-mail]:
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

Comment 16

[Andreea Pavel [:apavel]]

https://hg.mozilla.org/mozilla-central/rev/c713ef0ecf09
https://hg.mozilla.org/mozilla-central/rev/07e377e19c7d
https://hg.mozilla.org/mozilla-central/rev/c9fad2f64448
https://hg.mozilla.org/mozilla-central/rev/cb168f6a6bfd
https://hg.mozilla.org/mozilla-central/rev/f5ed4ddcc512