1421070 - Enable custom elements in chrome XUL documents by default

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, task, P2)

Description

[Dave Townsend [:mossop]]

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

Comment 1

[Dave Townsend [:mossop]]

Attachment: Bug 1421070: Always enable custom elements in chrome.

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

Comment 2

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

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.

Comment 3

[Brian Grinstead [:bgrins]]

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

Comment 4

[Brian Grinstead [:bgrins]]

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.

Comment 5

[Dave Townsend [:mossop]]

Comment on attachment 8932269 [details]
Bug 1421070: Always enable custom elements in chrome.

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

Comment 6

[Dave Townsend [:mossop]]

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

Comment 7

[Andrew Overholt [:overholt]]

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

Comment 8

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

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

Comment 9

[Brian Grinstead [:bgrins]]

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

Comment 10

[Brian Grinstead [:bgrins]]

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

Comment 11

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

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

Comment 12

[Matthew N. [:MattN]]

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.

Comment 13

[Dave Townsend [:mossop]]

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

Comment 14

[Brian Grinstead [:bgrins]]

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

Comment 15

[Dave Townsend [:mossop]]

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

Comment 16

[Brian Grinstead [:bgrins]]

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

Comment 17

[Brian Grinstead [:bgrins]]

Attachment: Bug 1421070: Always enable custom elements in chrome.

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

Comment 18

[Brian Grinstead [:bgrins]]

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.

Comment 19

[Matthew N. [:MattN]]

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?

Comment 20

[Zibi Braniecki [:zbraniecki][:gandalf]]

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

Comment 21

[Matthew N. [:MattN]]

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

Comment 22

[Brian Grinstead [:bgrins]]

(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"));
```

Comment 23

[Matthew N. [:MattN]]

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

Comment 24

[Dave Townsend [:mossop]]

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

Comment 25

[Matthew N. [:MattN]]

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

[Phabricator Automation]

Attachment: Bug 1421070: Always enable custom elements in chrome.

Comment 27

[Phabricator Automation]

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

Comment 28

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/794ee6857d83
Always enable custom elements in chrome. r=smaug

Comment 29

[Eliza Balazs [:ebalazs_]]

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

Comment 30

[Matthew N. [:MattN]]

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

Comment 31

[Matthew N. [:MattN]]

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

Comment 32

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/72815b4da101
Always enable custom elements in chrome. r=smaug, rs=MattN

Comment 33

[Bogdan Tara[:bogdan_tara | bogdant]]

https://hg.mozilla.org/mozilla-central/rev/72815b4da101