1425356 - [meta] Remove XUL templates

Status: RESOLVED FIXED

(Core :: XUL, enhancement)

Description

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

Per discussions, we don't think we need XUL templating in Firefox.

The only usecase I can find is actually the mac window menu, which uses a ton of RDF-ridden code to do something that should be possible with about 15 lines of JS.

I'm probably wrong.

But even if I am, there's clearly not a lot of stuff here so we should just give it a shot.

Comment 1

[Boris Zbarsky [:bzbarsky]]

I see 4 uses in Thunderbird and a bunch in Seamonkey.  We should at least file bugs on them to come up with an alternative.... better yet if we can suggest one.

Comment 2

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

(In reply to Boris Zbarsky [:bz] (no decent commit message means r-) from comment #1)
> I see 4 uses in Thunderbird and a bunch in Seamonkey.  We should at least
> file bugs on them to come up with an alternative.... better yet if we can
> suggest one.

Filed bug 1425962 and bug 1425963 for this.

Comment 3

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

Attachment: Bug 1425356 - remove local-build-only 'buster' XSLT qa tool,

This tool has basically been untouched since hg@1, except for codebase-wise changes, and as far as I can tell has
never had UI in Firefox (only in comm-central). It's the only remaining consumer of XUL templates, and I'm assuming it
can just be removed.

Review commit: https://reviewboard.mozilla.org/r/208746/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/208746/

Comment 4

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

Attachment: Bug 1425356 - remove some tests that only make sense when XUL templates are supported,

Review commit: https://reviewboard.mozilla.org/r/208748/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/208748/

Comment 5

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

Attachment: Bug 1425356 - stop testing treeview RDF-template persistence because we don't use it anywhere,

Review commit: https://reviewboard.mozilla.org/r/208750/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/208750/

Comment 6

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

Attachment: Bug 1425356 - remove XUL template support,

Review commit: https://reviewboard.mozilla.org/r/208752/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/208752/

Comment 7

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

Attachment: Bug 1425356 - remove various dead XUL sort attributes,

- sort-statics-last is no longer meaningful now non-static (ie template) items don't exist
- we don't use the 'resource' attribute anywhere in XUL.
- sortResource/sortResource2 are unused.
- we don't have meaningful consumers for sortSeparators

Review commit: https://reviewboard.mozilla.org/r/208754/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/208754/

Comment 8

[Florian Quèze [:florian]]

Comment on attachment 8938016 [details]
Bug 1425356 - remove local-build-only 'buster' XSLT qa tool,

https://reviewboard.mozilla.org/r/208746/#review214470

Please also cleanup xslt-qa references at https://searchfox.org/mozilla-central/rev/ff462a7f6b4dd3c3fdc53c9bc0b328f42a5b3d2b/browser/base/content/test/static/browser_all_files_referenced.js#179

Comment 9

[Florian Quèze [:florian]]

Comment on attachment 8938020 [details]
Bug 1425356 - remove various dead XUL sort attributes,

https://reviewboard.mozilla.org/r/208754/#review214472

r=me for the browser/ part, but please ask a dom reviewer to look at the dom/ changes.

Comment 10

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

Comment on attachment 8938016 [details]
Bug 1425356 - remove local-build-only 'buster' XSLT qa tool,

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/208746/diff/1-2/

Comment 11

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

Comment on attachment 8938017 [details]
Bug 1425356 - remove some tests that only make sense when XUL templates are supported,

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/208748/diff/1-2/

Comment 12

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

Comment on attachment 8938018 [details]
Bug 1425356 - stop testing treeview RDF-template persistence because we don't use it anywhere,

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/208750/diff/1-2/

Comment 13

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

Comment on attachment 8938019 [details]
Bug 1425356 - remove XUL template support,

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/208752/diff/1-2/

Comment 14

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

Comment on attachment 8938020 [details]
Bug 1425356 - remove various dead XUL sort attributes,

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/208754/diff/1-2/

Comment 15

[Florian Quèze [:florian]]

Comment on attachment 8938016 [details]
Bug 1425356 - remove local-build-only 'buster' XSLT qa tool,

https://reviewboard.mozilla.org/r/208746/#review214480

The browser_all_files_referenced.js change looks good. It seems we can now remove that old communicator.css file, but that's unrelated to what you are doing here, so it's for a follow-up :-).

Comment 16

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

https://treeherder.mozilla.org/#/jobs?repo=try&revision=188dacce1eb8

First push here was broken because I missed a reference in a layout/ moz.build file. Dunno why it builds fine locally and fails on try, but hey-ho.

Comment 17

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8938019 [details]
Bug 1425356 - remove XUL template support,

https://reviewboard.mozilla.org/r/208752/#review214566

So much code removal.  :)

Is this going to break tbird nightlies seriously enough that we should give them a bit of time to deal before we land this?

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

::: dom/xul/nsXULElement.h:333
(Diff revision 2)
>  
>  #define XUL_ELEMENT_FLAG_BIT(n_) NODE_FLAG_BIT(ELEMENT_TYPE_SPECIFIC_BITS_OFFSET + (n_))
>  
>  // XUL element specific bits
>  enum {
> -  XUL_ELEMENT_TEMPLATE_GENERATED =        XUL_ELEMENT_FLAG_BIT(0),
> +  XUL_ELEMENT_UNUSED =                    XUL_ELEMENT_FLAG_BIT(0),

It would be better to just drop this bit and renumber the other two bits.  Worth checking whether this effectively frees up a node bit, later on!

::: dom/xul/nsXULElement.h:338
(Diff revision 2)
> -  XUL_ELEMENT_TEMPLATE_GENERATED =        XUL_ELEMENT_FLAG_BIT(0),
> +  XUL_ELEMENT_UNUSED =                    XUL_ELEMENT_FLAG_BIT(0),
>    XUL_ELEMENT_HAS_CONTENTMENU_LISTENER =  XUL_ELEMENT_FLAG_BIT(1),
>    XUL_ELEMENT_HAS_POPUP_LISTENER =        XUL_ELEMENT_FLAG_BIT(2)
>  };
>  
>  ASSERT_NODE_FLAGS_SPACE(ELEMENT_TYPE_SPECIFIC_BITS_OFFSET + 3);

And this would become "+ 2" instead of "+ 3".

Comment 18

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8938020 [details]
Bug 1425356 - remove various dead XUL sort attributes,

https://reviewboard.mozilla.org/r/208754/#review214570

Comment 19

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

Comment on attachment 8938019 [details]
Bug 1425356 - remove XUL template support,

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/208752/diff/2-3/

Comment 20

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

Comment on attachment 8938020 [details]
Bug 1425356 - remove various dead XUL sort attributes,

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/208754/diff/2-3/

Comment 21

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

Comment on attachment 8938019 [details]
Bug 1425356 - remove XUL template support,

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/208752/diff/3-4/

Comment 22

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

Comment on attachment 8938020 [details]
Bug 1425356 - remove various dead XUL sort attributes,

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/208754/diff/3-4/

Comment 23

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

Apologies for the many updates here. Third time lucky on the trypush, I hope...

Note that I've now updated test_interfaces.html and the package manifests (oops - I grepped those previously but in hindsight didn't use the right search string - also, seems android is fine with missing installer files, but not desktop...), and fixed the review nits from comment #17.

Comment 24

[Axel Hecht [:Pike]]

Comment on attachment 8938016 [details]
Bug 1425356 - remove local-build-only 'buster' XSLT qa tool,

https://reviewboard.mozilla.org/r/208746/#review214784

Comment 25

[Blake Kaplan (:mrbkap) (inactive)]

Comment on attachment 8938017 [details]
Bug 1425356 - remove some tests that only make sense when XUL templates are supported,

https://reviewboard.mozilla.org/r/208748/#review215512

Comment 26

[Blake Kaplan (:mrbkap) (inactive)]

Comment on attachment 8938018 [details]
Bug 1425356 - stop testing treeview RDF-template persistence because we don't use it anywhere,

https://reviewboard.mozilla.org/r/208750/#review215514

Comment 27

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

Comment on attachment 8938016 [details]
Bug 1425356 - remove local-build-only 'buster' XSLT qa tool,

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/208746/diff/2-3/

Comment 28

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

Comment on attachment 8938017 [details]
Bug 1425356 - remove some tests that only make sense when XUL templates are supported,

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/208748/diff/2-3/

Comment 29

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

Comment on attachment 8938018 [details]
Bug 1425356 - stop testing treeview RDF-template persistence because we don't use it anywhere,

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/208750/diff/2-3/

Comment 30

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

Comment on attachment 8938019 [details]
Bug 1425356 - remove XUL template support,

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/208752/diff/4-5/

Comment 31

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

Comment on attachment 8938020 [details]
Bug 1425356 - remove various dead XUL sort attributes,

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/208754/diff/4-5/

Comment 32

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/0c89956f724e
remove local-build-only 'buster' XSLT qa tool, r=florian,Pike
https://hg.mozilla.org/integration/autoland/rev/4b4a36a57d94
remove some tests that only make sense when XUL templates are supported, r=mrbkap
https://hg.mozilla.org/integration/autoland/rev/490e100ac2e5
stop testing treeview RDF-template persistence because we don't use it anywhere, r=mrbkap
https://hg.mozilla.org/integration/autoland/rev/ca9153588760
remove XUL template support, r=bz
https://hg.mozilla.org/integration/autoland/rev/886d3489da20
remove various dead XUL sort attributes, r=bz,florian

Comment 34

[Noemi Erli[:noemi_erli]]

https://hg.mozilla.org/mozilla-central/rev/0c89956f724e
https://hg.mozilla.org/mozilla-central/rev/4b4a36a57d94
https://hg.mozilla.org/mozilla-central/rev/490e100ac2e5
https://hg.mozilla.org/mozilla-central/rev/ca9153588760
https://hg.mozilla.org/mozilla-central/rev/886d3489da20