Hey Fallen, just a heads up that this binding removal is coming to a head. I'm not 100% sure what comm-central needs to do in order to use the new Custom Element - if that's not in the cards, at the very least, you might want to fork browser.xml.

(In reply to Mike Conley (:mconley) (:⚙️) from comment #28)

Hey Fallen, just a heads up that this binding removal is coming to a head. I'm not 100% sure what comm-central needs to do in order to use the new Custom Element - if that's not in the cards, at the very least, you might want to fork browser.xml.

In theory this should "just work" for TB since it will be loaded via customElements.js. In practice, it would be worth testing with the patches here applied to confirm nothing out of the ordinary happens. Also if anything extends the 'browser' binding that will need updating - although I see one in suite/ but none for TB): https://searchfox.org/comm-central/search?q=xml%23browser&path=.

I've done a handful of try pushes during development, but here's a full one with -p all -u all: https://treeherder.mozilla.org/#/jobs?repo=try&revision=a9d373214dc9820dab892bd7e89f3b52f1330bf0

I've also seen some semi-unexpected talos results from previous pushes, so I've done two separate ones with the same base to see how stable the results are:

1. https://treeherder.mozilla.org/perf.html#/compare?originalProject=try&originalRevision=5ad6871711b65ed37f7f6e84f400c8efb1712293&newProject=try&newRevision=0389eb8b80a51a7a7f636b55ba0e3b28617fd215&framework=1&showOnlyImportant=1
2. https://treeherder.mozilla.org/perf.html#/compare?originalProject=try&originalRevision=5ad6871711b65ed37f7f6e84f400c8efb1712293&newProject=try&newRevision=2c861abb7cfdb6b1b89cc7bef55bfa3f86f9c65d&framework=1&showOnlyImportant=1

(In reply to Brian Grinstead [:bgrins] from comment #30)

I've also seen some semi-unexpected talos results from previous pushes, so I've done two separate ones with the same base to see how stable the results are:

1. https://treeherder.mozilla.org/perf.html#/compare?originalProject=try&originalRevision=5ad6871711b65ed37f7f6e84f400c8efb1712293&newProject=try&newRevision=0389eb8b80a51a7a7f636b55ba0e3b28617fd215&framework=1&showOnlyImportant=1
2. https://treeherder.mozilla.org/perf.html#/compare?originalProject=try&originalRevision=5ad6871711b65ed37f7f6e84f400c8efb1712293&newProject=try&newRevision=2c861abb7cfdb6b1b89cc7bef55bfa3f86f9c65d&framework=1&showOnlyImportant=1

Hm, seeing a few new Windows7-32 regressions on those. Trying two ideas to see if they will help:

1. Don't use the delayConnectedCallback: https://treeherder.mozilla.org/perf.html#/compare?originalProject=try&originalRevision=5ad6871711b65ed37f7f6e84f400c8efb1712293&newProject=try&newRevision=8b32fe71a8aa72c1397ccdd863d963b1a779e6b5&framework=1&showOnlyImportant=1
2. Don't load non-browser Custom Elements for chrome HTML docs: https://treeherder.mozilla.org/perf.html#/compare?originalProject=try&originalRevision=5ad6871711b65ed37f7f6e84f400c8efb1712293&newProject=try&newRevision=621a3059d9ede6fa32ed503987a4fca889847da5&framework=1&showOnlyImportant=1

The results in Comment 31 are surprising. I wouldn't expect either of these - and especially not (2) - to make things worse.

Trying a new base push to see if there's variation on these win7-32 builds even without any patches applied:

Base vs base: https://treeherder.mozilla.org/perf.html#/compare?originalProject=try&originalRevision=5ad6871711b65ed37f7f6e84f400c8efb1712293&newProject=try&newRevision=f2e9fedb224724380a09cddb840b3c7f543a6643&framework=1&showOnlyImportant=1

And with the patches applied compared to the new base:

1. No changes https://treeherder.mozilla.org/perf.html#/compare?originalProject=try&originalRevision=f2e9fedb224724380a09cddb840b3c7f543a6643&newProject=try&newRevision=0389eb8b80a51a7a7f636b55ba0e3b28617fd215&framework=1&showOnlyImportant=1
2. No chnges https://treeherder.mozilla.org/perf.html#/compare?originalProject=try&originalRevision=f2e9fedb224724380a09cddb840b3c7f543a6643&newProject=try&newRevision=2c861abb7cfdb6b1b89cc7bef55bfa3f86f9c65d&framework=1&showOnlyImportant=1
3. Don't use the delayConnectedCallback: https://treeherder.mozilla.org/perf.html#/compare?originalProject=try&originalRevision=f2e9fedb224724380a09cddb840b3c7f543a6643&newProject=try&newRevision=8b32fe71a8aa72c1397ccdd863d963b1a779e6b5&framework=1&showOnlyImportant=1
4. Don't load non-browser Custom Elements for chrome HTML docs: https://treeherder.mozilla.org/perf.html#/compare?originalProject=try&originalRevision=f2e9fedb224724380a09cddb840b3c7f543a6643&newProject=try&newRevision=621a3059d9ede6fa32ed503987a4fca889847da5&framework=1&showOnlyImportant=1

(In reply to Mike Conley (:mconley) (:⚙️) from comment #28)

Hey Fallen, just a heads up that this binding removal is coming to a head. I'm not 100% sure what comm-central needs to do in order to use the new Custom Element - if that's not in the cards, at the very least, you might want to fork browser.xml.

Thanks Mike! We should be fine, Arshad is doing our de-xbl work and given we're using a few other custom elements they should be loaded. I'm cc'ing him though so he is aware.

(In reply to Brian Grinstead [:bgrins] from comment #32)

The results in Comment 31 are surprising. I wouldn't expect either of these - and especially not (2) - to make things worse.

Trying a new base push to see if there's variation on these win7-32 builds even without any patches applied:

Base vs base: https://treeherder.mozilla.org/perf.html#/compare?originalProject=try&originalRevision=5ad6871711b65ed37f7f6e84f400c8efb1712293&newProject=try&newRevision=f2e9fedb224724380a09cddb840b3c7f543a6643&framework=1&showOnlyImportant=1

So the new base is significantly better in tp5o_scroll and tresize on win7-32 pgo, and significantly worse in tscrollx pgo windows10-64. This was pushed with the same try syntax with the same rev, so this is quite a moving target.

Hm, so I did two more pushes this morning with a new m-c tip and get the following results:

• https://treeherder.mozilla.org/perf.html#/compare?originalProject=try&originalRevision=231091724dbf5fec5712e83bf39f9da5c8b91483&newProject=try&newRevision=d1545452d4c2d2b24ecc1b02f52c800fc34ce1bc&framework=1&showOnlyImportant=1
• https://treeherder.mozilla.org/perf.html#/compare?originalProject=try&originalRevision=231091724dbf5fec5712e83bf39f9da5c8b91483&newProject=try&newRevision=72f489d20f06d089eb968d76db29929c7511f851&framework=1&showOnlyImportant=1

Basically no change, except for a tsvg_static opt e10s stylo windows7-32 improvement which I'll chalk up to noise given Comment 34. Mike, I obviously don't want to land something with a regression, but I'm having trouble determining if there's an improvement, regression, or no change as a result of this patch. What do you think we should do?

Note: I'm pushing these with ./mach try fuzzy -q '!qr /-talos-e10s' --rebuild 6 --no-artifact

If Talos is being inconsistent, then I suggest we not worry about it, tbh.

Darn, a change to browser_browser_languages_subdialog.js landed in Bug 1486507 yesterday that fails with the patch queue applied:

• https://treeherder.mozilla.org/#/jobs?repo=try&revision=d14f6ce7f9183a361c6ac9be530988aab1089be0&selectedJob=220655302
• https://hg.mozilla.org/mozilla-central/rev/a94a05a0a11d

(In reply to Brian Grinstead [:bgrins] from comment #38)

Darn, a change to browser_browser_languages_subdialog.js landed in Bug 1486507 yesterday that fails with the patch queue applied:

• https://treeherder.mozilla.org/#/jobs?repo=try&revision=d14f6ce7f9183a361c6ac9be530988aab1089be0&selectedJob=220655302
• https://hg.mozilla.org/mozilla-central/rev/a94a05a0a11d

As best I can tell, this is catching an issue where by removing the DOM node during window unload, we are preventing the "cancel" telemetry from firing when the tab is removed (due to this._frame.contentWindow being null when trying to call this._frame.contentWindow.close()).

Probably calling just destroy instead of removing the element is the right thing to do anyway (closer than the current XBL behavior), although when doing that I see a leak on browser/components/extensions/test/browser/browser_ext_windows_create_url.js in linux32 debug: https://treeherder.mozilla.org/#/jobs?repo=try&revision=2b78973d5bb32817bb6cb273ab9018bf889353ef&selectedJob=220690293. So going to look into that now.

(In reply to Brian Grinstead [:bgrins] from comment #39)

Probably calling just destroy instead of removing the element is the right thing to do anyway (closer than the current XBL behavior), although when doing that I see a leak on browser/components/extensions/test/browser/browser_ext_windows_create_url.js in linux32 debug: https://treeherder.mozilla.org/#/jobs?repo=try&revision=2b78973d5bb32817bb6cb273ab9018bf889353ef&selectedJob=220690293. So going to look into that now.

So when just calling destroy we were getting past the !aBrowser.parentNode condition and doing the aBrowser.isSyntheticDocument getter (https://searchfox.org/mozilla-central/rev/5053031ba7621fa8f63f42de4c204ab3561e4e59/browser/base/content/browser-fullZoom.js#349). This would throw due to contentDocument being null, and presumably the leak is related to that (perhaps aCallback not being called). I confirmed locally that changing that condition to !aBrowser.mInitialized makes the leak go away. Did a try push to confirm that, and now going to see why we don't currently leak with the XBL version (which presumably still has a parent node as well).

(In reply to Brian Grinstead [:bgrins] from comment #40)

(In reply to Brian Grinstead [:bgrins] from comment #39)

Probably calling just destroy instead of removing the element is the right thing to do anyway (closer than the current XBL behavior), although when doing that I see a leak on browser/components/extensions/test/browser/browser_ext_windows_create_url.js in linux32 debug: https://treeherder.mozilla.org/#/jobs?repo=try&revision=2b78973d5bb32817bb6cb273ab9018bf889353ef&selectedJob=220690293. So going to look into that now.

So when just calling destroy we were getting past the !aBrowser.parentNode condition and doing the aBrowser.isSyntheticDocument getter (https://searchfox.org/mozilla-central/rev/5053031ba7621fa8f63f42de4c204ab3561e4e59/browser/base/content/browser-fullZoom.js#349). This would throw due to contentDocument being null, and presumably the leak is related to that (perhaps aCallback not being called). I confirmed locally that changing that condition to !aBrowser.mInitialized makes the leak go away. Did a try push to confirm that, and now going to see why we don't currently leak with the XBL version (which presumably still has a parent node as well).

Ohh, so the .parentNode lookup seems just wrong for both CE and XBL, but the thing is with XBL isSyntheticDocument becomes undefined once it's destructed (so it never calls the property at https://searchfox.org/mozilla-central/rev/c3ebaf6de2d481c262c04bb9657eaf76bf47e2ac/toolkit/content/widgets/browser.xml#630 which would cause the error).

I think the right fix here is to check for mInitialized rather than checking the parent node (if there's no parent node then we won't have mInitialized set anyway).

https://hg.mozilla.org/mozilla-central/rev/4e943c6de64e
https://hg.mozilla.org/mozilla-central/rev/bd38d0bde8a3
https://hg.mozilla.org/mozilla-central/rev/b342c8e8306e

(In reply to Brian Grinstead [:bgrins] from comment #2)

We may be able to
remove the clickthrough attribute altogether by changing
NodeAllowsClickThrough to return false in the case of browser/tree elements
and true for a scrollbar.

Did this change end up being made? As far as I can tell, the patches that landed removed the clickthrough attribute without replacement, which means that web content in background windows now responds to mouse events.

(In reply to Markus Stange [:mstange] from comment #44)

(In reply to Brian Grinstead [:bgrins] from comment #2)

We may be able to
remove the clickthrough attribute altogether by changing
NodeAllowsClickThrough to return false in the case of browser/tree elements
and true for a scrollbar.

Did this change end up being made? As far as I can tell, the patches that landed removed the clickthrough attribute without replacement, which means that web content in background windows now responds to mouse events.

+ni for this...

(In reply to Markus Stange [:mstange] from comment #44)

(In reply to Brian Grinstead [:bgrins] from comment #2)

We may be able to
remove the clickthrough attribute altogether by changing
NodeAllowsClickThrough to return false in the case of browser/tree elements
and true for a scrollbar.

Did this change end up being made? As far as I can tell, the patches that landed removed the clickthrough attribute without replacement, which means that web content in background windows now responds to mouse events.

Looks like I missed this, thanks for the reminder! To be sure: the STR here is that when you have two windows and you click on something in the unfocused window we shouldn't process that as a click on the web content, but only focus the window?

That's right. And hovering things shouldn't apply :hover etc. As you said, making NodeAllowsClickThrough return false for browsers (either by adding the clickthrough attribute to the <browser> elements that we create, or through a new check in that function) should take care of this.