To allow us to switch from XBL to custom elements it needs to be enabled in chrome regardless of the preference.

We should probably first look at the performance issues Custom Element currently have. And if (CustomElementRegistry::IsCustomElementEnabled(cx, ${obj})) { in the generated code... that is way too slow, unfortunately.

Would you be able to rebase this and get an updated talos compare link now that Custom Elements are on by default in nightly?

Or alternatively: Andrew, do you know if will we be shipping Custom Elements without a pref to disable it? In that case we wouldn't need special handling for the browser chrome.

Perf numbers should show up here after the try runs are complete: https://treeherder.mozilla.org/perf.html#/comparechooser?newProject=try&newRevision=ed118f245173b584c8dd1bbb5eb547c33cd89049

(In reply to Brian Grinstead [:bgrins] from comment #4) > Or alternatively: Andrew, do you know if will we be shipping Custom Elements > without a pref to disable it? In that case we wouldn't need special handling > for the browser chrome. Olli would know.

I wouldn't be ready to do that. We will need to be able to disable Custom Element and Shadow DOM.

(In reply to Dave Townsend [:mossop] from comment #6) > Perf numbers should show up here after the try runs are complete: > https://treeherder.mozilla.org/perf.html#/ > comparechooser?newProject=try&newRevision=ed118f245173b584c8dd1bbb5eb547c33cd > 89049 This shows a lot of regressions against the last two days of m-c: https://treeherder.mozilla.org/perf.html#/compare?originalProject=mozilla-central&newProject=try&newRevision=ed118f245173b584c8dd1bbb5eb547c33cd89049&framework=1&selectedTimeRange=172800&showOnlyImportant=1. I seemed to remember that range compare being noisy so I did two other pushes to get a better idea of what's going on: 1) A try push with your patch applied and a base on try: https://treeherder.mozilla.org/perf.html#/compare?originalProject=try&originalRevision=92d98650ff918222f1a2ff7046cc0ac39fe1c271&newProject=try&newRevision=b68ab0d3347b1b9cce8aa1618e94347568625d21&framework=1&showOnlyImportant=1. 2) A try push with your patch but also just fall back to `nsContentUtils::IsCustomElementsEnabled()` in both calls to `CustomElementRegistry::IsCustomElementEnabled` instead of checking the document principle (i.e. https://hg.mozilla.org/try/rev/5b0cb8218dfd84957883b33efc6064fa141e4f98): https://treeherder.mozilla.org/perf.html#/compare?originalProject=try&originalRevision=92d98650ff918222f1a2ff7046cc0ac39fe1c271&newProject=try&newRevision=451a58d86b1f918701fe1b3402bb52f1fbde6145&framework=1&showOnlyImportant=1 For (1) I'm seeing a similar set of regressions as with your push. For (2) I'm not seeing much regression (and oddly a displaylist_mutate improvement, which must be noise). I'm not sure if there's much to optimize in the `CustomElementRegistry::IsCustomElementEnabled` functions in the patch. Another option may be to use 'are we in the parent process' as the condition to always have them on instead of being based on the document principle.

(In reply to Brian Grinstead [:bgrins] from comment #9) > Another option may be to use 'are we in the parent process' as the condition > to always have them on instead of being based on the document principle. Mossop and smaug: any opinion about using this approach instead? It wouldn't work for the 'remote xul' case but it seems like it'd be more performant and allow forward progress on the browser chrome migration. Right now I'm not sure how we'd load the CE definitions for remote XUL anyway - that's something we'd have to think about with the approach in https://bugzilla.mozilla.org/show_bug.cgi?id=1411707#c11.

It is still possible to load web pages in the parent process, I'd rather not expose Custom elements in such pages. Some combination of process check and principal check might work. It would slow down DOM operations in browser chrome though. if (CustomElementRegistry::IsCustomElementEnabled() || (parentProcess && CustomElementRegistry::IsCustomElementEnabledForCx(cx)))

I would like to use custom elements on resource://payments/paymentRequest.xhtml which I load in a remote browser so neither the process check not principal check will work for me btw. I was planning to just file a follow-up bug once this bug was resolved but it seemed like it may be useful to consider this use while we're re-considering what condition(s) to use so we don't need to change it significantly later. We're currently using a polyfill until CE is ready since we aren't going to ride the trains for a while.

As smaug said you can still see webpages in the main process so we'd need something more here.

(In reply to Dave Townsend [:mossop] from comment #13) > As smaug said you can still see webpages in the main process so we'd need > something more here. Just want to make sure I understand in what situation(s) this is happening: 1) In what cases are we shipping non-e10s mode by default? 2) Are there still cases where we load content webpages in the main process even when in e10s mode?

(In reply to Brian Grinstead [:bgrins] from comment #14) > (In reply to Dave Townsend [:mossop] from comment #13) > > As smaug said you can still see webpages in the main process so we'd need > > something more here. > > Just want to make sure I understand in what situation(s) this is happening: > > 1) In what cases are we shipping non-e10s mode by default? Looks like there are only cases controlled by prefs left: https://dxr.mozilla.org/mozilla-central/source/toolkit/xre/nsAppRunner.cpp#5074 > 2) Are there still cases where we load content webpages in the main process > even when in e10s mode? I'm not seeing any obvious place where the sidebar loads things into a remote process so that is one. Another potential is system-addons loading pages into panels etc. and not going to the trouble to make that remote, maybe that is less of a concern though.

(In reply to Olli Pettay [:smaug] (unusual timezone [JST] for a week) from comment #11) > It is still possible to load web pages in the parent process, I'd rather not > expose Custom elements in such pages. > > > Some combination of process check and principal check might work. It would > slow down DOM operations in browser chrome though. > > if (CustomElementRegistry::IsCustomElementEnabled() || (parentProcess && > CustomElementRegistry::IsCustomElementEnabledForCx(cx))) I tried something like this by modifying the patch in the bug and the perf is better - only a few medium confidence regressions: https://treeherder.mozilla.org/perf.html#/compare?originalProject=try&originalRevision=4443515a9229f7f940c585aecf0d5590306a6035&newProject=try&newRevision=523a839bea6ce2f674a84071dd2ab74194619fb9&framework=1&showOnlyImportant=1 / https://hg.mozilla.org/try/rev/863c558b19d1c98e7e425948850d82b4e3e045ca#l1.30.

I have a new comparison with the version I just sent up to mozreview that shows no regression: https://treeherder.mozilla.org/perf.html#/compare?originalProject=try&originalRevision=4443515a9229f7f940c585aecf0d5590306a6035&newProject=try&newRevision=d6d216f9115c0a77859abb1c76e9368e7b15ac85&framework=1&showOnlyImportant=1.

Hmm… with this patch (local build and your try build) I get "TypeError: Illegal constructor" whenever I try to construct a custom element so I'm not sure the patch is sufficient. Does it work for you?

I have the same issue. It works with HTMLElement, but extending XULElement errors with `Illegal constructor`.

(In reply to Zibi Braniecki [:gandalf][:zibi] from comment #20) > I have the same issue. It works with HTMLElement, but extending XULElement > errors with `Illegal constructor`. Weird… I was testing with HTMLElement.

(In reply to Matthew N. [:MattN] (PM if requests are blocking you) from comment #19) > Hmm… with this patch (local build and your try build) I get "TypeError: > Illegal constructor" whenever I try to construct a custom element so I'm not > sure the patch is sufficient. Does it work for you? I see that as well with HTMLElement, but even without this patch applied. I'm guessing the work in Bug 1404420 didn't add support for HTML custom elements in XUL documents (which we don't currently need at the moment for XBL work). For instance, if I put this at the bottom of browser.js it throws Illegal Constructor, but if I extend XULElement instead it works. ``` class MyComponent extends HTMLElement { connectedCallback() { this.innerHTML = "test"; } } customElements.define('my-component', MyComponent); document.documentElement.appendChild(document.createElement("my-component")); ```

Ok, the case I want to work is an HTML element in an XHTML document btw.

(In reply to Matthew N. [:MattN] (PM if requests are blocking you) from comment #23) > Ok, the case I want to work is an HTML element in an XHTML document btw. Can you file a separate bug for that, I can look into it.

(In reply to Dave Townsend [:mossop] from comment #24) > (In reply to Matthew N. [:MattN] (PM if requests are blocking you) from > comment #23) > > Ok, the case I want to work is an HTML element in an XHTML document btw. > > Can you file a separate bug for that, I can look into it. Bug 1446557

Comment on attachment 8961490 [details] Bug 1421070: Always enable custom elements in chrome. Olli Pettay [:smaug] (only webcomponents and event handling reviews, please) has approved the revision. https://phabricator.services.mozilla.com/D791

Pushed by dtownsend@mozilla.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/794ee6857d83 Always enable custom elements in chrome. r=smaug

Backed out changeset 794ee6857d83 (bug 1421070) for 15 failures in toolkit/components/payments/test/mochitest/test_ObservedPropertiesMixin.html Push with failures: https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&revision=b0f1f640001c9215115ff4fbea8d7fa9844f287e&filter-resultStatus=testfailed&filter-resultStatus=busted&filter-resultStatus=exception&filter-resultStatus=runnable&filter-resultStatus=pending&filter-resultStatus=running&filter-resultStatus=success&selectedJob=169739153 Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=169739153&repo=mozilla-inbound Backout: https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&revision=ce8f1316d545c72f1b91635d030212289cfdf42a&filter-resultStatus=testfailed&filter-resultStatus=busted&filter-resultStatus=exception&filter-resultStatus=runnable&filter-resultStatus=pending&filter-resultStatus=running&filter-resultStatus=success

(In reply to Eliza Balazs [:ebalazs_] from comment #29) > Backed out changeset 794ee6857d83 (bug 1421070) for 15 failures in > toolkit/components/payments/test/mochitest/test_ObservedPropertiesMixin.html > Failure log: > https://treeherder.mozilla.org/logviewer.html#?job_id=169739153&repo=mozilla- > inbound These tests were supposed to be running with custom elements disabled (using the shim) due to a leak mentioned in bug 1413418 comment 8. I don't see the leak failure on this push but these tests also don't need to run with e10s off (which is why it's bypassing the dom.webcomponents.customelements.enabled pref after this bug. For now I think it's fine to `skip-if = !e10s` for this directory and I will deal with fixing these tests to work without the shim.

(In reply to Matthew N. [:MattN] (PM if requests are blocking you) from comment #30) > For now I think it's fine to `skip-if = !e10s` for this directory and I will > deal with fixing these tests to work without the shim. To clarify, the reason why that's fine is because it's not relevant to the tests as they aren't doing cross-process stuff and the PaymentRequest API was never implemented for non-e10s anyways so we'll never use this code in production with non-e10s.

Pushed by dtownsend@mozilla.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/72815b4da101 Always enable custom elements in chrome. r=smaug, rs=MattN