stylo: re-enable web components and update tests.

RESOLVED FIXED in Firefox 58

Status

()

enhancement
P3
normal
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: emilio, Assigned: emilio)

Tracking

unspecified
mozilla58
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox57 wontfix, firefox58 fixed)

Details

Attachments

(2 attachments)

They're surprisingly close to working to most extent.
Depends on: stylo-shadow
Blocks: 1409086
Blocks: stylo-shadow
No longer depends on: stylo-shadow
Tentatively marking the dependency on bug 1404789 since I haven't tested this patch without it.
Depends on: 1404789
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.
Attachment #8919005 - Flags: review?(cam) → review+
Attachment #8919005 - Flags: review?(bzbarsky)
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).
No longer depends on: 1404789
No longer blocks: 1409086
Actually, Olli is on my timezone, and I promised to cleanup the IsWebComponentsEnabled checks...
Priority: -- → P3
Comment on attachment 8919005 [details]
Bug 1409079: Re-enable web components in stylo.

https://reviewboard.mozilla.org/r/189894/#review195460
Attachment #8919005 - Flags: review?(bugs) → review+
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
Attachment #8919220 - Flags: review?(bugs) → review+
> 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).
I'd assume nsContentUtils::IsWebComponentsEnabled gets inlined.
> 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.
ok, I learnt something new :)
Feel free to use Pref.
Pushed by ecoal95@gmail.com:
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
Pushed by ecoal95@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/02dcc553e820
followup: Skip test also in release and add shell null-check. r=me
Depends on: 1414999
Depends on: 1415013
You need to log in before you can comment on or make changes to this bug.