1409079 - stylo: re-enable web components and update tests.

Status: RESOLVED FIXED

(Core :: CSS Parsing and Computation, enhancement, P3)

Description

[Emilio Cobos Álvarez (:emilio)]

They're surprisingly close to working to most extent.

Comment 1

[Emilio Cobos Álvarez (:emilio)]

Attachment: Bug 1409079: Re-enable web components in stylo.

I can probably simplify some of the checks to only look at nsContentUtils, but
that can be a followup, I think. Also we should remove the `stylo` annotation
from test_interfaces.js

I'll work on both tomorrow if this patch gets accepted.

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

Comment 2

[Emilio Cobos Álvarez (:emilio)]

Tentatively marking the dependency on bug 1404789 since I haven't tested this patch without it.

Comment 3

[Cameron McCormack (:heycam)]

Comment on attachment 8919005 [details]
Bug 1409079: Re-enable web components in stylo.

https://reviewboard.mozilla.org/r/189894/#review195220

You'll need a DOM peer's review on the test_interfaces.js change, I guess.

::: commit-message-340e9:3
(Diff revision 1)
> +I can probably simplify some of the checks to only look at nsContentUtils, but
> +that can be a followup, I think. Also we should remove the `stylo` annotation
> +from test_interfaces.js
> +
> +I'll work on both tomorrow if this patch gets accepted.

Some of this doesn't really belong in a commit message.

Comment 4

[Emilio Cobos Álvarez (:emilio)]

Boris, note that the rest of the patch is already reviewed, you can just look at the interfaces.js changes if you want (feel free to review the whole patch of course).

Comment 5

[Emilio Cobos Álvarez (:emilio)]

Actually, Olli is on my timezone, and I promised to cleanup the IsWebComponentsEnabled checks...

Comment 6

[Emilio Cobos Álvarez (:emilio)]

Attachment: Bug 1409079: Simplify WebComponents enabled checks.

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

Comment 7

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 8919005 [details]
Bug 1409079: Re-enable web components in stylo.

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

Comment 8

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

Comment on attachment 8919005 [details]
Bug 1409079: Re-enable web components in stylo.

https://reviewboard.mozilla.org/r/189894/#review195460

Comment 9

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

Comment on attachment 8919220 [details]
Bug 1409079: Simplify WebComponents enabled checks.

https://reviewboard.mozilla.org/r/190138/#review195462

::: dom/webidl/Element.webidl:242
(Diff revision 1)
>  
>  // http://w3c.github.io/webcomponents/spec/shadow/#extensions-to-element-interface
>  partial interface Element {
> -  [Throws,Func="nsDocument::IsWebComponentsEnabled"]
> +  [Throws,Pref="dom.webcomponents.enabled"]
>    ShadowRoot createShadowRoot();
> -  [Func="nsDocument::IsWebComponentsEnabled"]
> +  [Pref="dom.webcomponents.enabled"]

I'd prefer Func=nsContentUtils::IsWebComponentsEnabled since that is faster.
Same with all the .webidl

Comment 10

[Boris Zbarsky [:bzbarsky]]

> I'd prefer Func=nsContentUtils::IsWebComponentsEnabled since that is faster.

Faster along what dimension?  I'd think it's slower to check (extra function call compared to just a boolean check), but a bit faster to set up (fewer AddBoolVarCache calls).

Comment 11

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

I'd assume nsContentUtils::IsWebComponentsEnabled gets inlined.

Comment 12

[Boris Zbarsky [:bzbarsky]]

> I'd assume nsContentUtils::IsWebComponentsEnabled gets inlined.

Not easily, because it's an indirect call via the function pointer stored in Prefable.  Some sort of PGO/LTO might be able to do it, maybe.

Comment 13

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

ok, I learnt something new :)
Feel free to use Pref.

Comment 14

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 8919005 [details]
Bug 1409079: Re-enable web components in stylo.

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

Comment 15

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 8919220 [details]
Bug 1409079: Simplify WebComponents enabled checks.

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

Comment 16

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/a840d960ab75
Re-enable web components in stylo. r=heycam,smaug
https://hg.mozilla.org/integration/autoland/rev/da7f10ba4344
Simplify WebComponents enabled checks. r=smaug

Comment 17

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/02dcc553e820
followup: Skip test also in release and add shell null-check. r=me

Comment 18

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/mozilla-central/rev/a840d960ab75
https://hg.mozilla.org/mozilla-central/rev/da7f10ba4344
https://hg.mozilla.org/mozilla-central/rev/02dcc553e820