|Submitter||Diff||Changes||Open Issues||Last Updated|
|Error loading review requests:|
59 bytes, text/x-review-board-request
|Details | Review|
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 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.
Attachment #8878020 - Flags: review?(bobbyholley) → review?(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|
Attachment #8878020 - Flags: review?(wchen) → review+
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.)
Pushed by firstname.lastname@example.org: https://hg.mozilla.org/integration/autoland/rev/0e13adff6e7e Disable Web Components in Servo-styled documents. r=wchen
Assignee: nobody → cam
Status: NEW → RESOLVED
Last Resolved: 9 months ago
status-firefox56: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla56
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.
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.
You need to log in before you can comment on or make changes to this bug.