1373155 - stylo: disable Web Components in Servo-styled documents

Status: RESOLVED FIXED

(Core :: CSS Parsing and Computation, defect)

Description

[Cameron McCormack (:heycam)]

Seems easier to just force Web Components to be disabled in Servo-styled documents than to fix the crashes and get valid (but incorrect) styles.

Comment 1

[Cameron McCormack (:heycam)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=4714376f5a5deb6db4e66b4ec014c53f6c03d3f0

Comment 2

[Cameron McCormack (:heycam)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=30a108c98dae2b8d8f1b56bfee05a53ee176ec88

Comment 3

[Cameron McCormack (:heycam)]

Attachment: Bug 1373155 - Disable Web Components in Servo-styled documents.

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

Comment 4

[Bobby Holley (:bholley)]

Comment on attachment 8878020 [details]
Bug 1373155 - Disable Web Components in Servo-styled documents.

Overholt said this patch was generally ok but needs some tweaks, and asked that I delegate review to wchen.

Comment 5

[William Chen [:wchen]]

Comment on attachment 8878020 [details]
Bug 1373155 - Disable Web Components in Servo-styled documents.

https://reviewboard.mozilla.org/r/149434/#review154154

The checks look good to me. For the shadow DOM tests, why are we now failing instead of skipping? The tests don't look very meaninful when stylo is enabled since we disable shadow DOM.

::: dom/base/nsDocument.cpp:6058
(Diff revision 1)
> -  // Check for the webcomponents permission. See Bug 1181555.
>    JSAutoCompartment ac(aCx, obj);
>    JS::Rooted<JSObject*> global(aCx, JS_GetGlobalForObject(aCx, obj));
>    nsCOMPtr<nsPIDOMWindowInner> window =
>      do_QueryInterface(nsJSUtils::GetStaticScriptGlobal(global));
> +  nsIDocument* doc = window ? window->GetExtantDoc() : nullptr;

Lets move this line to just before we use |doc|

Comment 6

[Cameron McCormack (:heycam)]

Comment on attachment 8878020 [details]
Bug 1373155 - Disable Web Components in Servo-styled documents.

https://reviewboard.mozilla.org/r/149434/#review154154

I thought it might be good to be notified when these tests do start passing, once we add shadow DOM support to stylo later down the track.  (It's easy to forget to re-enable skipped tests.)

Comment 7

[Cameron McCormack (:heycam)]

Comment on attachment 8878020 [details]
Bug 1373155 - Disable Web Components in Servo-styled documents.

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

Comment 8

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/0e13adff6e7e
Disable Web Components in Servo-styled documents. r=wchen

Comment 9

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

https://hg.mozilla.org/mozilla-central/rev/0e13adff6e7e

Comment 10

[Andrew Overholt [:overholt]]

My super minor nit here was just that we are referring to just (AFAICT with a quick skim) Shadow DOM as "Web Components" and I'd rather we were explicit about Shadow DOM vs. Custom Elements.

Comment 11

[Cameron McCormack (:heycam)]

Thanks for pointing that out.  If there are specific renamings of those nsDocument/nsContentUtils methods or the pref that you'd like, please file a bug and I can take a look.