1431255 - Provide a way to create a 'UA' shadow root / slots that are not accessible to the page

Status: RESOLVED FIXED

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

Description

[Brian Grinstead [:bgrins]]

In order to remove XBL bindings in-content, I'd like to be able to create a shadow root and move content into it without the page seeing it.

For example, the <marquee> element could create a UA shadow root and then put the light dom inside of a wrapper element that moves around. The in-content bindings I suspect this would be useful for are:

1) <marquee>
2) XML pretty printing
3) <videocontrols>
4) plugin problem ui

I believe that (1) and (2) could be implemented just with this feature, while (3) and (4) will require some kind of js environment in addition to this.

Comment 1

[Brian Grinstead [:bgrins]]

Here's an example of how this looks in chromium: https://cs.chromium.org/chromium/src/third_party/WebKit/Source/core/html/HTMLMarqueeElement.cpp?rcl=545ae8c4766e100e92936a13018d3106cb8cbc4a&l=57-79

Comment 2

[Tim Nguyen :ntim]

I wonder if we can modify the stuff used by the DevTools highlighter to support injecting elsewhere than the canvas frame:
 https://dxr.mozilla.org/mozilla-central/source/dom/webidl/AnonymousContent.webidl

Comment 3

[Tim Nguyen :ntim]

> 2) XML pretty printing

For XML pretty printing, a possibility is to support XML files similarly to how JSON files are supported by the devtools JSON viewer. Would be interesting to sync this on the DevTools' roadmap.

Comment 4

[Boris Zbarsky [:bzbarsky]]

The JSON viewer has a bunch of problems compared to the XML viewer, starting with the fact that the DOM it exposes is not the right DOM...

Comment 5

[Brian Grinstead [:bgrins]]

FWIW I think we can turn the XML pretty printing into HTML with no JS by modifying the existing XSLT to render out appropriate <details> / <summary> tags. Then we need a way to place to put that DOM - I was thinking this could be in a UA shadow root (attached to the root element), although I believe we also discussed somehow using the existing native anon content / CanvasFrame similar to how the devtools highlighters work.

Comment 6

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

(In reply to Tim Nguyen :ntim from comment #2)
> I wonder if we can modify the stuff used by the DevTools highlighter to
> support injecting elsewhere than the canvas frame:
>  https://dxr.mozilla.org/mozilla-central/source/dom/webidl/AnonymousContent.
> webidl

I'd be interested in considering this for the highlighter use case.  I am tracking various new pathways for highlighters over in bug 1411122.

Comment 7

[Brian Grinstead [:bgrins]]

Attachment: datetime-input-prototype.patch

This patch isn't trying to tackle the UA shadow root which we'd still need for elements that embed user's content inside them like <marquee>. But it's something I wanted to get feedback on that might help for other in-content bindings where we need to run JS alongside the content (unlike <marquee>).

Till and Bobby: I had a few questions about this but especially about security - please see the code comment inside the DateInput class in tab-content.js. This would be an alternate approach to what we've discussed before about using self-hosted JS.

Comment 8

[Bobby Holley (:bholley)]

Interesting idea!

For those just joining us: Brian is trying to figure out how to sanely eliminate the xbl-in-content we have right now for various widgets. The patch here is a possible approach to the scripting environment when script is needed. Basically, when the frame constructor creates the relevant NAC, it dispatches an event that is intercepted by a frame script, which then goes and pokes at the DOM over Xrays. This is nice and simple, and eliminates the need for the XBL scope. The question is whether it's secure/sane.

I think it's worth breaking down the various facets of our existing XBL scope design for comparison.

First, the XBL scope machinery is all in C++. This is necessary because it's used for XBL script, which lives in a weird declarative xml format, gets parsed by a pile of ugly C++ code, and then needs to be injected somewhere. In the new world, this is a design constraint that we can drop. However, the hardcoded C++ implementation also allows the DOM binding machinery to reflect NAC into the XBL scope rather than the content scope. This is useful because it gives us strong guarantees around content interfacing with NAC (more on that later). It also allows us to do hardcoded IsXBLScope() checks to expose various interfaces and functionality. The downside of doing it in C++ is complexity.

Second, the XBL scope machinery spins up a separate global for each content global. This is because we employ the principle of least privilege, and run XBL code with ExpandedPrincipal([ContentPrincipal]), rather than with the system principal. If we were willing to use the system principal, we could have just stuck it all in the privileged junk scope. But we wanted EPs, which meant at least xblScope-per-origin, and origin keying is a hassle in C++, so we did xblscope-per-global. The downside of xblscope-per-global is memory usage.

Brian's patch goes the other way on both counts. I think keeping things mostly out of C++ is a nice goal, so let's see if we can take his approach and eliminate design choice #1. That said, I do think we probably want to preserve the defense-in-depth of ExpandedPrincipal([ContentPrincipal]), so I think we should maintain design choice #2. To do that, we can adjust Brian's patch pretty easily to do the work in a Sandbox instead. We can keep an origin-keyed map of sandboxes in the frame script, and in the more-e10s future, we should rarely have more than a couple of these in a given process.

So now we get to the bit about the NAC reflectors. The current setup is all a bit hokey. Layout-generated NAC is security-sensitive, because shuffling it around can violate layout assumptions and cause crashes. As a result, we have all sorts of checks to avoid exposing NAC to content. Additionally, for less security-sensitive reasons, we want to prevent web content from touching the implementation details of host widgets. So we piggy-back on the aforementioned protections and stick all the host widgets under a NAC ancestor. But this means that we end up needing to expose NAC to script so that the widget can manipulate it. And it's very hard to keep script in a given global from touching objects in that global. So we solve this by hoisting all NAC reflectors into the XBL scope, which is the only place they should be touched.

Really, it seems like we should add a way to distinguish the bonafide NAC from the widget objects. Then, we could put the bonafide NAC in the unprivileged junk scope (where content can't touch it, but chrome stuff like devtools can), and the host widget objects in the regular content scope (where they have weak protections, but that's probably ok).

So I think my proposed setup looks like this:
(1) Use Brian's event dispatching setup for the frame constructor to trigger async widget installation from frame script.
(2) Run the widget code in a script-created nsEP sandbox. Any necessary privileged APIs are injected into the sandbox via ExportHelpers.
(3) Create a shadow root on the host NAC, and pass that to the sandbox as the root for widget content. This can probably just be an ordinary shadow root, since the host NAC element isn't script-accessible.
(4) Reflectors go in the unprivileged junk scope if (IsInNativeAnonymousSubtree() && !IsInShadowTree()). We'd need to make sure to unset the shadow bit for the NAC-within-shadow case to distinguish it from the shadow-within-NAC case.

Thoughts?

Comment 9

[Boris Zbarsky [:bzbarsky]]

So by "widgets" you mean things like videcontrols and marquee guts, right?

For the marquee case, we want the shadow root on the element itself, not on NAC, I'd think.

I'd sort of like us to think about whether we want to have such a think as layout-generated NAC at all, or whether we should just have some concept of "host shadow".

Finally, in your proposal, since we have an nsEP sandbox anyway, why not put the reflectors for the shadow-in-NAC case in there instead of the page global and keep our existing protections?

Comment 10

[Brian Grinstead [:bgrins]]

(In reply to Boris Zbarsky [:bz] (no decent commit message means r-) from comment #9)
> So by "widgets" you mean things like videcontrols and marquee guts, right?

I think I'd say "widgets" are the in-content bindings that need to have JS running to function:
- Plugin problem UI: pluginProblem, replacement
- Video Controls: videocontrols, touchcontrols, nocontrols, suppressChangeEvent
- Date/time inputs: date-input, time-input, datetime-input-base

And the in-content bindings that shouldn't need JS:
- Scrollbars: scrollbar, scrollbar-base, thumb
- XML Pretty printing: prettyprint
- Marquee: 5 total bindings
- Input field key handling: inputFields and textAreas platform bindings

And then how the anon content interacts with the host element I think is separate from whether we need JS running:
- For marquee we need to attach a shadow root to the element itself since we preserve / wrap the content
- Input field handling has no content
- All the others get rendered 'inside' the host element they are attached to and on top of any of the element's children

Comment 11

[Bobby Holley (:bholley)]

So, we discussed this quite a bit on IRC, and came up with a reasonable plan.

We determined that NAC isn't going away (at the very least before/after), and that scripted widgets aren't going away either (video controls). We decided that we might as well build a system that would be usable for the gray-area widgets like input/date, which could realistically be implemented either way.

So the proposed design is as-follows:
* We create/manage (in C++) a per-origin HostShadowScope, similar to the XBL scope but without being attached to a particular global, and thus per-origin. This scope would have nsEP(Origin).
* We introduce the concept of a host shadow root. This may be as simple as creating shadow roots for elements that are not in the spec's whitelist for shadow hosts.
* In BindToTree for affected elements, we create the shadow root and install the various elements inside them. Elements in host shadow trees should have their reflectors live in the HostShadowScope, similar to how we currently stick NAC reflectors into the XBL scope. We can probably do this via an explicit element creation API.
* After the shadow root is set up, BindToTree uses a script runner to dispatch a sync event to the frame script. We dispatch a corresponding event in UnbindFromTree.
* If the frame script recognizes the element as one needing JS widget code, it injects the appropriate code into the HostShadowScope, and invokes any needed constructors.
* The frame script provides the minimal set of APIs to the HostShadowScope via ExportHelpers. In particular, it may need to provide an element creation API that creates elements whose reflectors live in the HostShadowScope.
* Bonafide NAC reflectors go into the unprivileged junk scope.

Does this seem like a good plan to everyone?

Comment 12

[Bobby Holley (:bholley)]

NI for thoughts on comment 11.

Comment 13

[Brian Grinstead [:bgrins]]

One note: we'd need host shadow roots to always be enabled regardless of prefs, since we wouldn't want to break the browser chrome or HTML content if someone flipped the dom.webcomponents.shadowdom.enabled pref. I mention this because perf concerns have been raised when attempting to do the same thing with Custom Elements in Bug 1421070.

Comment 14

[Emilio Cobos Álvarez (:emilio)]

I don't have enough knowledge about how our XBL-in-content story is and its security properties, nor context about why it is the way it is, so I don't think I can add anything really useful to most of the discussion from comment 11 (always happy to learn about this stuff though :P).

I'll just mention that "Host Shadow Root" looks like a really confusing name choice (why Host? that's its own thing in the Shadow DOM spec, and it's definitely not a Shadow Root).

(In reply to Brian Grinstead [:bgrins] from comment #13)
> One note: we'd need host shadow roots to always be enabled regardless of
> prefs, since we wouldn't want to break the browser chrome or HTML content if
> someone flipped the dom.webcomponents.shadowdom.enabled pref. I mention this
> because perf concerns have been raised when attempting to do the same thing
> with Custom Elements in Bug 1421070.

Perf shouldn't be too bad now I think. We should fix bug 1441136 for a couple places in layout and do profiling to see if there's something obviously wrong, but modulo that I don't think the same concern that applies to custom-elements (which need to pay extra price for tons of common operations in the DOM) applies to Shadow DOM. Olli can confirm that though.

Comment 15

[Bobby Holley (:bholley)]

(In reply to Emilio Cobos Álvarez [:emilio] from comment #14)
> I don't have enough knowledge about how our XBL-in-content story is and its
> security properties, nor context about why it is the way it is, so I don't
> think I can add anything really useful to most of the discussion from
> comment 11 (always happy to learn about this stuff though :P).
> 
> I'll just mention that "Host Shadow Root" looks like a really confusing name
> choice (why Host? that's its own thing in the Shadow DOM spec, and it's
> definitely not a Shadow Root).

Host meaning "host environment". I agree it's confusing, maybe UAShadowRoot instead?

Comment 16

[Emilio Cobos Álvarez (:emilio)]

(In reply to Bobby Holley (:bholley) from comment #15)
> Host meaning "host environment". I agree it's confusing, maybe UAShadowRoot
> instead?

That sounds way less confusing to me :)

Comment 17

[Boris Zbarsky [:bzbarsky]]

Comment 11 sounds good to me.  The key part is that we are not injecting any page-accesible APIs, and thus it does not matter if page script runs (off an earlier scriptrunner) before our injection happens.

We _do_ need to think a bit about what happens if said script removes the element from the DOM or moves it around; I don't recall whether scriptrunners from that could run before our "older" scriptrunner, but I would think they can.

Comment 18

[Brian Grinstead [:bgrins]]

I believe mobile touchControls are the only case where we load more XUL and XBL bindings inside the anon content of an in-content binding. Since I don't think we'll want to figure out how to run chrome Custom Elements inside of this native anon content in the content document, removing the touchControls binding (and the in-content xul.css injection) should help. The plan is to remove that binding and instead use a restyled version of the desktop videocontrols binding (which has only HTML in its anon content) in Bug 1444489.

Comment 19

[Emilio Cobos Álvarez (:emilio)]

Whatever solution we take here, can we make sure that they don't have the same footguns as XBL has regarding the scope chain? See bug 1446342.

Comment 20

[Tim Guan-tin Chien [:timdream] (please needinfo)]

Question: for this proposal, do we care if the created NAC is still <xul:datetimebox> and not <div class="datetimebox">? After all, this is the only XUL element in the whole document tree...

Comment 21

[Brian Grinstead [:bgrins]]

(In reply to Tim Guan-tin Chien [:timdream] (please needinfo) from comment #20)
> Question: for this proposal, do we care if the created NAC is still
> <xul:datetimebox> and not <div class="datetimebox">? After all, this is the
> only XUL element in the whole document tree...

I'd like to stop using XUL NAC in-content (tracking this in Bug 1446829), but I don't think it would have an effect on the plan here either way.

Comment 22

[Tim Guan-tin Chien [:timdream] (please needinfo)]

Talked to bholley today and would like to work implement what he had proposed on comment 11.

Finding of the day:

0. comment 13 is true; will need to find a fix for it; possibly by checking if the passed object is a NAC? See (3) below.
1. Create a shadow root is surprisingly straightforward, simply copy the necessary lines of code from Element::AttachShadow() to nsXULElement::BindToTree() and we can create it inside <datetimebox>.
2. Brian's patch in comment 7 crashes on debug build from lots of layout assertions. Since the sync event to frame script will happen at somewhat the same place, the shadow root approach will fail too whenever the script tries to mutate the DOM in its constructor. This is easy to workaround with a setTimeout() but I am sure this is not the real fix. Need to investigate how we run XBL constructors to get away from this.
3. nsDocument::IsShadowDOMEnabled(JSContext* aCx, JSObject* aObject) returns false when the JSObject is the NAC element.

Also, a note to <marquee>:

4. The reason why <marquee> XBL binding didn't bind to a NAC inside is that the binding implements the element itself. We don't implement HTMLMarqueeElement as described in [1]. I assume, asides from implementing UA shadow root here, I would need to write a small C++ wrapper, implementing HTMLMarqueeElement and hook it up to whatever JS we loaded inside the UA shadow root?

[1] https://html.spec.whatwg.org/multipage/obsolete.html#the-marquee-element

That keeps <datetimebox> and <videocontrols> the easier ones to prototype the feature, compared to <marquee>.

PS inputFields and textAreas bindings are not in the scope of this bug even though they bound to <html:input> and <html:textarea>. They should be dealt with in bug 1419091.

Comment 23

[Tim Guan-tin Chien [:timdream] (please needinfo)]

(In reply to Tim Guan-tin Chien [:timdream] (please needinfo) from comment #22)
> I would need to write a small C++ wrapper, implementing
> HTMLMarqueeElement and hook it up to whatever JS we loaded inside the UA
> shadow root?

JS-implemented WebIDL APIs can't be used here because of bug 1450827.

Comment 24

[ExE Boss]

When attaching a `HostShadowRoot`, it should be created by a different [ChromeOnly] method to avoid priviledge escalations bugs (similar to Blink’s `DidAddUserAgentShadowRoot` callback).

Comment 25

[ExE Boss]

Actually, as comment #15 suggests, we should probably call it `UAShadowRoot` instead of `HostShadowRoot`.

Comment 26

[Brian Grinstead [:bgrins]]

(In reply to Tim Guan-tin Chien [:timdream] (please needinfo) from comment #22)
> Also, a note to <marquee>:
> 
> 4. The reason why <marquee> XBL binding didn't bind to a NAC inside is that
> the binding implements the element itself. We don't implement
> HTMLMarqueeElement as described in [1]. I assume, asides from implementing
> UA shadow root here, I would need to write a small C++ wrapper, implementing
> HTMLMarqueeElement and hook it up to whatever JS we loaded inside the UA
> shadow root?
> 
> [1] https://html.spec.whatwg.org/multipage/obsolete.html#the-marquee-element

This will be fixed in Bug 1425874, which is almost done but needs a push over the finish line. Agree that <datetimebox> and <videocontrols> are better for prototyping regardless though. In particular, I'd like if we didn't need any JS at all for <marquee> tag (instead slotting in the user content into a host that uses CSS animations to move around).

Comment 27

[Emilio Cobos Álvarez (:emilio)]

(In reply to Tim Guan-tin Chien [:timdream] (please needinfo) from comment #22)
> Talked to bholley today and would like to work implement what he had
> proposed on comment 11.
> 
> Finding of the day:
> 
> 0. comment 13 is true; will need to find a fix for it; possibly by checking
> if the passed object is a NAC? See (3) below.
> 1. Create a shadow root is surprisingly straightforward, simply copy the
> necessary lines of code from Element::AttachShadow() to
> nsXULElement::BindToTree() and we can create it inside <datetimebox>.
> 2. Brian's patch in comment 7 crashes on debug build from lots of layout
> assertions. Since the sync event to frame script will happen at somewhat the
> same place, the shadow root approach will fail too whenever the script tries
> to mutate the DOM in its constructor. This is easy to workaround with a
> setTimeout() but I am sure this is not the real fix. Need to investigate how
> we run XBL constructors to get away from this.
> 3. nsDocument::IsShadowDOMEnabled(JSContext* aCx, JSObject* aObject) returns
> false when the JSObject is the NAC element.

Why would you add the host to the NAC element?

The general impression I had is that we'd add the ShadowRoot to the actual content element, but just not expose it. That allows us to avoid NAC altogether (as in, nsIAnonymousContentCreator, I guess the content in that ShadowRoot is still kind-of NAC). Attaching ShadowRoots frame construction looks a bit like a recipe for disaster.

Comment 28

[ExE Boss]

(In reply to Brian Grinstead [:bgrins] from comment #26)
> (In reply to Tim Guan-tin Chien [:timdream] (please needinfo) from comment
> #22)
> > Also, a note to <marquee>:
> > 
> > 4. The reason why <marquee> XBL binding didn’t bind to a NAC inside is that
> > the binding implements the element itself. We don’t implement
> > HTMLMarqueeElement as described in [1]. I assume, asides from implementing
> > UA shadow root here, I would need to write a small C++ wrapper, implementing
> > HTMLMarqueeElement and hook it up to whatever JS we loaded inside the UA
> > shadow root?
> > 
> > [1] https://html.spec.whatwg.org/multipage/obsolete.html#the-marquee-element
> 
> This will be fixed in Bug 1425874, which is almost done but needs a push
> over the finish line. Agree that <datetimebox> and <videocontrols> are
> better for prototyping regardless though. In particular, I’d like if we
> didn’t need any JS at all for <marquee> tag (instead slotting in the user
> content into a host that uses CSS animations to move around).

You mean do something similar to what Chrome does?
https://cs.chromium.org/chromium/src/third_party/WebKit/Source/core/html/HTMLMarqueeElement.cpp?rcl=545ae8c4766e100e92936a13018d3106cb8cbc4a&l=57-79

Although, I would personally prefer if the <marquee> shadow tree used a UA stylesheet instead of the inline <style> element that Chrome uses.

Comment 29

[Emilio Cobos Álvarez (:emilio)]

(In reply to Emilio Cobos Álvarez [:emilio] (Away-ish from 27/05 to 09/06) from comment #27)
> Attaching ShadowRoots frame construction
> looks a bit like a recipe for disaster.

Err, this should read "Attaching ShadowRoots *during* frame construction.

Comment 30

[Tim Guan-tin Chien [:timdream] (please needinfo)]

Emilio convinced me that get rid of NAC at the same time should be the better approach. See the discussion on IRC. Though note the complication on <videocontrols>.

https://mozilla.logbot.info/developers/20180424#c14657216

Comment 31

[Emilio Cobos Álvarez (:emilio)]

(In reply to Tim Guan-tin Chien [:timdream] (please needinfo) from comment #30)
> Emilio convinced me that get rid of NAC at the same time should be the
> better approach. See the discussion on IRC. Though note the complication on
> <videocontrols>.
> 
> https://mozilla.logbot.info/developers/20180424#c14657216

Oh, just note that when I said Element::GetShadowRoot I meant the WebIDL Element.shadowRoot attribute (which is actually GetShadowRootByMode in native code), not the actual C++'s Element::GetShadowRoot. But I guess it should mostly behave like a closed shadow root in that regard...

Olli may know more about whether this UA-only thing would need special-casing vs. just a closed shadow root during event handling or what not... And then there's all the script security stuff that Bobby and Boris know about. But those need to happen regardless of what do we attach the ShadowRoot to.

Comment 32

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

FWIW, I don't yet trust that closed shadow DOM nodes don't leak to light DOM JS. I've been filing spec bugs about such issues recently. 

Firing a DOM event when binding a "widget-element" first time to document may prove to be too slow, or
almost certainly is.
(our xbl implemented elements are too slow too, but we shouldn't compare to them, but to natively implemented elements)


The end result should be to implement all elements in C++. It is unclear to me why we'd want any temporary setup where there is still some JS.

Comment 33

[Brian Grinstead [:bgrins]]

(In reply to Olli Pettay [:smaug] from comment #32)
> FWIW, I don't yet trust that closed shadow DOM nodes don't leak to light DOM
> JS. I've been filing spec bugs about such issues recently. 

AIUI the plan in Comment 11 doesn't rely on using a closed shadow root.

> Firing a DOM event when binding a "widget-element" first time to document
> may prove to be too slow, or
> almost certainly is.
> (our xbl implemented elements are too slow too, but we shouldn't compare to
> them, but to natively implemented elements)

Why shouldn't we compare to in-content XBL elements? The motivation for making this change isn't to improve performance for these particular tags but to remove platform support for in-content XBL, which should allow us to improve performance / security across the board.

> The end result should be to implement all elements in C++. It is unclear to
> me why we'd want any temporary setup where there is still some JS.

Migrating the existing bindings to use this mechanism is much less work than implementing these UIs in C++ (esp videocontrols and the datetime picker / calendar which are thousands of lines of JS). Further, I don't see this as a temporary setup - it should be easier to build/maintain/test these UIs with DOM/JS (while supporting accessibility, styles, etc). I suspect that's why in-content XBL was added in the first place although I don't know all the history there.

The in-content bindings I was thinking could drop JS entirely (although some still require a UA shadow root) are: marquee, scrollbars, xml pretty print, input field key handling, and maybe the plugin problem UI depending on what we want to do with UX there.

Comment 34

[Brian Grinstead [:bgrins]]

(In reply to Brian Grinstead [:bgrins] from comment #33)
> The in-content bindings I was thinking could drop JS entirely (although some
> still require a UA shadow root) are: marquee, scrollbars, xml pretty print,
> input field key handling, and maybe the plugin problem UI depending on what
> we want to do with UX there.

Forgot to mention <resizer> which also doesn't need JS and is being converted to NAC in Bug 1450017

Comment 35

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

xbl for marquee was added because a quick hack was needed. xbl for date/time was added because creating NAC manually is super painful (need to deal with reflow etc), and I think same applies for videocontrol.
Once shadow DOM is ready, creating NAC-like structures should be quite a bit easier, and it can be done in C++.
I would imagine several of our form control implementations to move to use such setup.
(range, number etc.)

Why in-content XBL would be slower than shadow DOM + some random chrome JS?
(in general web components do slow down DOM operations a bit)

And we need to keep performance in mind all the time. I would guess if one decided to modify
Speedometer just a bit to test type=date, and not type=text and type=checkbox what it does now, our score would drop significantly comparing to other browsers.

Comment 36

[Brian Grinstead [:bgrins]]

(In reply to Olli Pettay [:smaug] from comment #35)
> xbl for marquee was added because a quick hack was needed. xbl for date/time
> was added because creating NAC manually is super painful (need to deal with
> reflow etc), and I think same applies for videocontrol.
> Once shadow DOM is ready, creating NAC-like structures should be quite a bit
> easier, and it can be done in C++.
> I would imagine several of our form control implementations to move to use
> such setup.
> (range, number etc.)

Yeah I expect the current consumers of NAC could (should?) migrate to use this eventually. I'm not sure if there are cases where continuing to use NAC will be more appropriate (maybe if we need fine-grained control over layout?).

> Why in-content XBL would be slower than shadow DOM + some random chrome JS?
> (in general web components do slow down DOM operations a bit)
> 
> And we need to keep performance in mind all the time. I would guess if one
> decided to modify
> Speedometer just a bit to test type=date, and not type=text and
> type=checkbox what it does now, our score would drop significantly comparing
> to other browsers.

I just mean that the main goal for this proposal is to stop using in-content XBL so that we can remove the platform support.  I'd prefer to not widen the scope to also include improving performance for these few elements, since it will delay achieving the main goal. Of course making them faster would also be great - but it seems like a separate project to me.

Comment 37

[Bobby Holley (:bholley)]

(In reply to Tim Guan-tin Chien [:timdream] (please needinfo) from comment #22)
> 4. The reason why <marquee> XBL binding didn't bind to a NAC inside is that
> the binding implements the element itself. We don't implement
> HTMLMarqueeElement as described in [1]. I assume, asides from implementing
> UA shadow root here, I would need to write a small C++ wrapper, implementing
> HTMLMarqueeElement and hook it up to whatever JS we loaded inside the UA
> shadow root?

That's right. We'd need a C++ class for the element, which we would attach a shadow root to.


(In reply to ExE Boss from comment #24)
> When attaching a `HostShadowRoot`, it should be created by a different
> [ChromeOnly] method to avoid priviledge escalations bugs (similar to Blink’s
> `DidAddUserAgentShadowRoot` callback).

It doesn't need to be [ChromeOnly] because it doesn't need to be a webIDL method at all, it can/should be done in C++ from BindToTree.

(In reply to Tim Guan-tin Chien [:timdream] (please needinfo) from comment #30)
> Emilio convinced me that get rid of NAC at the same time should be the
> better approach. See the discussion on IRC.

Emilio is exactly right here, we should get rid of the NAC. I thought I said this during our call but maybe I was unclear.

(In reply to Brian Grinstead [:bgrins] from comment #33)
> (In reply to Olli Pettay [:smaug] from comment #32)
> > FWIW, I don't yet trust that closed shadow DOM nodes don't leak to light DOM
> > JS. I've been filing spec bugs about such issues recently. 
> 
> AIUI the plan in Comment 11 doesn't rely on using a closed shadow root.

It sort of does. The idea is that we'd be attaching shadow roots to elements that normally can't have shadow roots, and using the same capability-based mechanism to prevent them from leaking to content. However, we're also going to have two additional guarantees:
* The JS runs in a separate global with an nsExpandedPrincipal.
* The DOM reflectors for the shadow content live in that global, rather than the regular global.

Those are the guarantees we currently offer for in-content XBL, and they seem to be sufficient.

(In reply to Olli Pettay [:smaug] from comment #35)
> xbl for marquee was added because a quick hack was needed. xbl for date/time
> was added because creating NAC manually is super painful (need to deal with
> reflow etc), and I think same applies for videocontrol.
> Once shadow DOM is ready, creating NAC-like structures should be quite a bit
> easier, and it can be done in C++.

The current plan is to create the structures in C++, from BindToTree. The question is where the scripting bits live. It's totally reasonable to implement simple logic in C++, but we do have some widgets with very complex logic, which is what this setup is designed to support. We can still eventually move that logic into C++ if we decide to, but I don't want to block XBL removal on rewriting all these widgets.

> And we need to keep performance in mind all the time. I would guess if one
> decided to modify
> Speedometer just a bit to test type=date, and not type=text and
> type=checkbox what it does now, our score would drop significantly comparing
> to other browsers.

Sure, but that feels like a pretty synthetic problem. Yes, Speedometer tests form controls. But we primarily _care_ about Speedometer because it tests frameworks. Fundamentally, I don't think that date pickers are a likely user-perceptible performance bottleneck.

Comment 38

[Bobby Holley (:bholley)]

(In reply to Brian Grinstead [:bgrins] from comment #36)
> I'm not sure if there are cases where continuing to use NAC
> will be more appropriate (maybe if we need fine-grained control over
> layout?).

We will still need NAC for ::before/::after, at minimum. Many/most of the other stuff can migrate to the BindToTree setup we're implementing here.

Comment 39

[Tim Guan-tin Chien [:timdream] (please needinfo)]

(In reply to Brian Grinstead [:bgrins] from comment #33)
> (In reply to Olli Pettay [:smaug] from comment #32)
> > FWIW, I don't yet trust that closed shadow DOM nodes don't leak to light DOM
> > JS. I've been filing spec bugs about such issues recently. 
> 
> AIUI the plan in Comment 11 doesn't rely on using a closed shadow root.

I am a bit confused here. So if I replace the NAC <xul:datetimebox> with the shadow root, and put the gut of date/time controls inside it, we would depend on the closed UA shadow root to hide it from the web content, isn't?

What are the exact spec and feature dependencies for the UA shadow root here to be shippable?

===

Removing <xul:datetimebox> also means the work here will be more involved, particularly around nsIDateTimeInputArea implementation.

https://searchfox.org/mozilla-central/search?q=nsIDateTimeInputArea&case=false&regexp=false&path=

I assume we could have JS listen to events directly on the host element in system event group, similar to what we do in video controls. I vaguely remember there were many follow-ups involving this change, so the tail is going to be long here...

Comment 40

[Brian Grinstead [:bgrins]]

(In reply to Tim Guan-tin Chien [:timdream] (please needinfo) from comment #39)
> Removing <xul:datetimebox> also means the work here will be more involved,
> particularly around nsIDateTimeInputArea implementation.
> 
> https://searchfox.org/mozilla-central/
> search?q=nsIDateTimeInputArea&case=false&regexp=false&path=
> 
> I assume we could have JS listen to events directly on the host element in
> system event group, similar to what we do in video controls. I vaguely
> remember there were many follow-ups involving this change, so the tail is
> going to be long here...

Yeah nsIDateTimeInputArea is an extra complication for date/time picker. We don't have a clear way to get rid of [implements] - ideally we could remove the dependence on XPCOM (I seem to remember Bobby had an idea on how to do that after the initial prototype.. it may have been similar to what you are referring to with listening to system events).

Comment 41

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

I guess you mean <input type=date> would become a shadow host, and it would have a closed shadow root, right?

Not sure what you mean with "What are the exact spec and feature dependencies".
We don't have Shadow DOM quite yet implemented. It is ongoing work. (two major things to fix are some sequential focus handling issues and touch.target handling in shadow DOM)
And there are spec issues like https://github.com/w3c/touch-events/issues/92 to be fixed so that closed shadow DOM doesn't get exposed to the outside world.

Comment 42

[Tim Guan-tin Chien [:timdream] (please needinfo)]

(In reply to Olli Pettay [:smaug] from comment #41)
> I guess you mean <input type=date> would become a shadow host, and it would
> have a closed shadow root, right?

Yes. Without a NAC <xul:datetimebox> we will depend on the setup you'd described.

> And there are spec issues like https://github.com/w3c/touch-events/issues/92
> to be fixed so that closed shadow DOM doesn't get exposed to the outside
> world.

Thanks for the pointer.

Comment 43

[Tim Guan-tin Chien [:timdream] (please needinfo)]

Attachment: shadowroot-bind-to-tree.patch

This WIP on top of brian's patch will create a shadow root inside <input>.

The next thing to figure out is to run the widget JS from there.

Comment 44

[Tim Guan-tin Chien [:timdream] (please needinfo)]

Attachment: shadowroot-bind-to-tree-sandbox.patch

I've finally been able to pick up from where I've left.

This is a slight modification from the patch earlier:

1) Create a ShadowRoot from HTMLInputElement::BindToTree() and dispatch a DateTimeElementInserted event.
2) React to that event in tab-content.js by creating a Sandbox with the global set to the shadow root.
3) Run the widget code in it so it could create elements on its own in there.

Here is something I would need more inputs:

- The sandbox currently has the principal == input.nodePrincipal. I've not created the HostShadowScope as described in comment 11. The said principal should also have access to system group events, and a few IsChromeOrXBL() methods/properties defined in HTML{Video|Input}Element.webidl. Perhaps IsChromeOrXBL() needs to be modified such that it could recognize the sandbox/principal?
- comment 11 also mentioned reflectors which I am not sure if I need to do anything explicitly. There will be no NAC involved in this setup; if there isn't any blocker preventing this, the closed shadow root will be used in place of <xul:datetimebox> or <xul|videocontrol>. In this case, do I need to do anything with the elements created by the widget JS code?

Am I going in the right direction here?

Comment 45

[Brian Grinstead [:bgrins]]

Can you post up a rolled-up and rebased version of the patch for testing?

Comment 46

[Tim Guan-tin Chien [:timdream] (please needinfo)]

Attachment: datetime-input-prototype.patch

I was working on top of 9055d9d89a4. I can rebase to the latest central later.

Comment 47

[Tim Guan-tin Chien [:timdream] (please needinfo)]

Attachment: datetime-input-prototype.patch

Comment 48

[Tim Guan-tin Chien [:timdream] (please needinfo)]

Attachment: shadowroot-bind-to-tree-sandbox.patch

Rebased... hg was confused a little bit a while ago.

Comment 49

[Tim Guan-tin Chien [:timdream] (please needinfo)]

(In reply to Tim Guan-tin Chien [:timdream] (please needinfo) from comment #30)
> Emilio convinced me that get rid of NAC at the same time should be the
> better approach. See the discussion on IRC. Though note the complication on
> <videocontrols>.
> 
> https://mozilla.logbot.info/developers/20180424#c14657216

Slightly change of direction; once I figured out sandbox I don't see Shadow DOM is a necessity to the encapsulation setup? I mean, we already have <xul:videocontrols> and <xul:datetimebox> and they are and their DOM within not accessible to the content already. Having to put them into a Shadow DOM feels like an extra work.

That leaves us the plugin problem UI which is not contained in a NAC (and is attached directly to <html:object> whatnot). I think I can normalize it by adding a <xul:pluginproblem> NAC so to align the three use cases. Maybe they can all move to be inside a UA shadow DOM in the future.

Does that make sense?

Comment 50

[Emilio Cobos Álvarez (:emilio)]

Sure, if you use NAC then Shadow DOM looks somewhat redundant... But I thought the idea was binding a ShadowRoot to the <input> / <video> element? That allows to get rid of NAC.

Also, it's not generally safe to dispatch JS events from frame construction, I'd prefer to avoid it if possible...

Comment 51

[Emilio Cobos Álvarez (:emilio)]

Oh, that JS event you're _removing_, and it comes from another patch... nvm.

Comment 52

[Bobby Holley (:bholley)]

For the principal, you should be using an ExpandedPrincipal. You can do this from js by passing nodePrincipal as a one-element array.

Emilio is correct - the reason to switch to Shadow DOM is that we can delegate content creation to JS rather than hard coding it in c++ with NAC. Once you do that, you'll need to do something to put the content reflectors into the UA Shadow root scope, similar to what we currently do to put nac reflectors in the ContentXBLScope.

Comment 53

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

We want content creation to happen in C++, but implement the widget-y like elements using UA shadow root.
That would simplify many parts of Gecko quite a bit.

Comment 54

[Bobby Holley (:bholley)]

(In reply to Olli Pettay [:smaug] from comment #53)
> We want content creation to happen in C++, but implement the widget-y like
> elements using UA shadow root.
> That would simplify many parts of Gecko quite a bit.

Yes, sorry. Got that mixed up in my hurried reply.

Comment 55

[Tim Guan-tin Chien [:timdream] (please needinfo)]

The NAC route didn't work. The <style> tag append into it cannot handle @import. I am not sure if that could be a blocker to bug 1437956 but I have stopped exploring the route here.

I've been trying to figure out how to set up the scripting environment correctly; It is proven that to be difficult to get any of the features we want to migrate to into a usable proof-of-concept.

- For datetimebox we do not have a solution to removing nsIDateTimeInputArea interface.
- For videocontrols, it takes some trickery to layout the div replacing videocontrols in shadow root correctly.
- I have not figure out how I can trigger pluginProblem UI easily in reasonable development iterations.

I've also realized a plain Cu.Sandbox is not usable for our purpose here, because it cannot set the scope chain (and probably all the other things as well).

So indeed I would need to set up the scripting environment in C++. I will do that in this bug while awaiting the issues above gets resolved.

Comment 56

[Emilio Cobos Álvarez (:emilio)]

Self-reminder that we should have the following rules in UA shadow roots to make them not observable:

  slot { all: inherit; display: contents; }

Otherwise we may hit issues like: https://bugs.chromium.org/p/chromium/issues/detail?id=850872

Comment 57

[Emilio Cobos Álvarez (:emilio)]

Well I guess you could still have display: inherit and observe it...

Comment 58

[Boris Zbarsky [:bzbarsky]]

Yeah, I think when we make up UA shadow trees we have to make sure inheritance ignores them...

Comment 59

[Tim Guan-tin Chien [:timdream] (please needinfo)]

Attachment: Bug 1431255 - Part II, Create a Shadow Root in HTMLMediaElement when enabled, skipping <xul:videocontrols>

This prevents XBL binding from being attached, and create the Shadow Root to host controls to be
created by the script.

A few changes in layout is needed for the child frames to be created and for other places
that touches the video controls to find it correctly.

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

Comment 60

[Tim Guan-tin Chien [:timdream] (please needinfo)]

Attachment: Bug 1431255 - Part III, Create per-origin sandboxes from XPCJSRuntime and load UA widgets scripts

This patch create the basic structure on how the widget script can be loaded and be pointed to
the Shadow Root.

The patch right now short circuit the original per-window XBL sandbox to the frame script,
to be replaced with per-origin sandbox.

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

Comment 61

[Tim Guan-tin Chien [:timdream] (please needinfo)]

Attachment: Bug 1431255 - Part IV, Load videocontrols.js, migrated from videoControls binding

The full videocontrols.xml needs to be migrated.
The change here simply shows that the script works and the layout is sized correctly.

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

Comment 62

[Tim Guan-tin Chien [:timdream] (please needinfo)]

I've decided to use video controls as the first use case of this feature, because of the to-be-removed nsI* interface in <datetimebox>. I've managed to fix the layout bit thanks for the help from :dholbert.

(In reply to Emilio Cobos Álvarez [:emilio] from comment #56)
>   slot { all: inherit; display: contents; }

We will only encounter this case in marquee and pluginProblems. The work on the marquee would not happen on this bug though because the plan is to move the animation controls to C++.

Comment 63

[Tim Guan-tin Chien [:timdream] (please needinfo)]

Comment on attachment 8988356 [details]
Bug 1431255 - Part II, Create a Shadow Root in HTMLMediaElement when enabled, skipping <xul:videocontrols>

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

Comment 64

[Tim Guan-tin Chien [:timdream] (please needinfo)]

Comment on attachment 8988357 [details]
Bug 1431255 - Part III, Create per-origin sandboxes from XPCJSRuntime and load UA widgets scripts

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

Comment 65

[Tim Guan-tin Chien [:timdream] (please needinfo)]

Comment on attachment 8988358 [details]
Bug 1431255 - Part IV, Load videocontrols.js, migrated from videoControls binding

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

Comment 66

[Tim Guan-tin Chien [:timdream] (please needinfo)]

Comment on attachment 8988357 [details]
Bug 1431255 - Part III, Create per-origin sandboxes from XPCJSRuntime and load UA widgets scripts

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

Comment 67

[Tim Guan-tin Chien [:timdream] (please needinfo)]

Comment on attachment 8988358 [details]
Bug 1431255 - Part IV, Load videocontrols.js, migrated from videoControls binding

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

Comment 68

[Tim Guan-tin Chien [:timdream] (please needinfo)]

Comment on attachment 8988357 [details]
Bug 1431255 - Part III, Create per-origin sandboxes from XPCJSRuntime and load UA widgets scripts

Hi Bobby, might if you could look at the patch? So I ended up keyed the sandboxes to CString and reuse CStringHasher. I am not sure other hashers will give us the same hash with different principal objects. Let me know if this has any undesired effect.

Comment 69

[Tim Guan-tin Chien [:timdream] (please needinfo)]

With these patches, there is now working video controls coming from the Shadow DOM. There are still some odd behaviors. I will need to copy the entire test suite and make sure they don't break. Oh, and make it aware the "controls" attribute.

The next bit, unrelated to video controls, is to put the DOM reflector into another scope.

Comment 70

[Bobby Holley (:bholley)]

(In reply to Tim Guan-tin Chien [:timdream] (please needinfo) from comment #68)
> Hi Bobby, might if you could look at the patch? So I ended up keyed the
> sandboxes to CString and reuse CStringHasher. I am not sure other hashers
> will give us the same hash with different principal objects. Let me know if
> this has any undesired effect.

They will if you create a custom hasher that does ::Equals. The invariant for principals is that a.equals(b) if and only if a.origin == b.origin.

I think you should use nsCOMPtr<nsIPrincipal> as the hash key, and define a custom policy that does ::Equals. That will avoid doing a full stringification (the GetOrigin call) every time we want to do a lookup, including the associated heap overhead. It will also avoid the memory hazard your current patch has with insertion. ;-)

Comment 71

[Bobby Holley (:bholley)]

Comment on attachment 8988357 [details]
Bug 1431255 - Part III, Create per-origin sandboxes from XPCJSRuntime and load UA widgets scripts

https://reviewboard.mozilla.org/r/253612/#review260762

::: browser/base/content/tab-content.js:502
(Diff revision 3)
>      return;
>    removeEventListener("MozAfterPaint", onFirstNonBlankPaint);
>    sendAsyncMessage("Browser:FirstNonBlankPaint");
>  });
> +
> +class PageWidgets {

Maybe UAWidget? Happy to discuss the naming further, I just want it to make sense and be consistent everywhere.

::: browser/base/content/tab-content.js:513
(Diff revision 3)
> +  }
> +
> +  handleEvent(aEvent) {
> +    switch (aEvent.type) {
> +      case "PageWidgetBindToTree":
> +        this.setupSandbox(aEvent.target);

This is about tearing down the widget, not the sandbox, right? Naming should reflect that.

::: js/xpconnect/idl/xpccomponents.idl:184
(Diff revision 3)
>      /*
> +     * Get the sandbox for running JS-implemented UA widgets (video controls etc.),
> +     * hosted inside UA-created Shadow DOM.
> +     */
> +    [implicit_jscontext]
> +    jsval getUASandbox(in Element hostElement);

Let's call this UAWidgetScope, to match terminology elsewhere. Naming of other functions should change.

I also think the input argument should be an nsIPrincipal.

::: js/xpconnect/src/XPCComponents.cpp:2176
(Diff revision 3)
> +    JSObject* wrapper = hostElement->AsContent()->GetWrapper();
> +    if (!wrapper) {
> +      return NS_ERROR_FAILURE;
> +    }
> +
> +    JS::RootedObject contentScope(cx, wrapper);
> +    JSAutoRealm ar(cx, contentScope);

I don't think we want any of this.

::: js/xpconnect/src/XPCComponents.cpp:2188
(Diff revision 3)
> +
> +    RootedObject scope(cx, 
> +                       XPCJSRuntime::Get()->GetUASandbox(hostElement->NodePrincipal()));
> +    NS_ENSURE_TRUE(scope, NS_ERROR_OUT_OF_MEMORY); // See bug 858642.
> +
> +    scope = js::UncheckedUnwrap(scope);

Not sure why we need the unwrap call. I think we want to call ExposeObjectToActiveJS, and then JS_WrapObject, then rval.set(ObjectValue(obj)).

::: js/xpconnect/src/XPCJSRuntime.cpp:3078
(Diff revision 3)
> +
> +    // Create a new sandbox
> +    JSAutoRequest ar(cx);
> +
> +    SandboxOptions options;
> +    //options.sandboxName.AssignLiteral("UA sandbox Compartment");

Let's call this "UA Widget Scope"

::: js/xpconnect/src/XPCJSRuntime.cpp:3081
(Diff revision 3)
> +
> +    SandboxOptions options;
> +    //options.sandboxName.AssignLiteral("UA sandbox Compartment");
> +    options.wantXrays = false;
> +    options.wantComponents = false;
> +    options.isContentXBLScope = true;

This should be false.

::: js/xpconnect/src/XPCJSRuntime.cpp:3097
(Diff revision 3)
> +                              static_cast<nsIExpandedPrincipal*>(ep),
> +                              options);
> +    NS_ENSURE_SUCCESS(rv, nullptr);
> +
> +    auto sandbox = &v.toObject();
> +    MOZ_ALWAYS_TRUE(mUASandboxMap.putNew(origin.get(), sandbox));

This is a memory hazard, because |origin| is a raw pointer to the buffer of the stack-allocated string, will go away at the end of this function.

As mentioned previously, we should key the hash map based on nsIPrincipal instead.

::: js/xpconnect/src/XPCJSRuntime.cpp:3099
(Diff revision 3)
> +    NS_ENSURE_SUCCESS(rv, nullptr);
> +
> +    auto sandbox = &v.toObject();
> +    MOZ_ALWAYS_TRUE(mUASandboxMap.putNew(origin.get(), sandbox));
> +
> +    MOZ_ASSERT(xpc::IsInContentXBLScope(js::UncheckedUnwrap(sandbox)));

This should be removed.

Comment 72

[Tim Guan-tin Chien [:timdream] (please needinfo)]

Attachment: Bug 1431255 - Part V, Set the reflectors of the UA Widget DOM to UA Widget Scope

This is not working yet. Something is still missing from the setup.

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

Comment 73

[Tim Guan-tin Chien [:timdream] (please needinfo)]

Comment on attachment 8988356 [details]
Bug 1431255 - Part II, Create a Shadow Root in HTMLMediaElement when enabled, skipping <xul:videocontrols>

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

Comment 74

[Tim Guan-tin Chien [:timdream] (please needinfo)]

Comment on attachment 8988357 [details]
Bug 1431255 - Part III, Create per-origin sandboxes from XPCJSRuntime and load UA widgets scripts

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

Comment 75

[Tim Guan-tin Chien [:timdream] (please needinfo)]

Comment on attachment 8988358 [details]
Bug 1431255 - Part IV, Load videocontrols.js, migrated from videoControls binding

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

Comment 76

[Tim Guan-tin Chien [:timdream] (please needinfo)]

Comment on attachment 8989602 [details]
Bug 1431255 - Part V, Set the reflectors of the UA Widget DOM to UA Widget Scope

This does what's discussed, but I did not observe the effect of restricting the DOM access to the content script. I suspect some other flag of the node is also checked? I couldn't find it yet.

About ReparentWrappersInSubtree(): it looks like it updates the wrappers of nodes to XBL scope, when a usual node is inserted into XBL anonymous tree. I can see that the button element created in videocontrols.xml (with createElement("button")) is exposed to the content if I comment out that part. I can also see that with the way the nodes are inserted from videocontrols.js, a "ReparentWrappersInUAWidgetSubtree()" method in that place can intercept it. Does that mean we no longer need special element insertion APIs to make this work? Let me know if you have thoughts.

One more thing: I unfortunately have to litter a lot of function w/ the name "GetUAWidgetScope" in various places, just to get to implementation and pick up the principal etc. I made a flowchart on that so hopefully, it is more clear. I will upload it here.

Comment 77

[Tim Guan-tin Chien [:timdream] (please needinfo)]

Attachment: flowchart on relationships b/t different GetUAWidgetScope() methods

Made with 

https://mermaidjs.github.io/mermaid-live-editor/#/edit/eyJjb2RlIjoiZ3JhcGggVERcbkooXCJQYWdlV2lkZ2V0LnNldHVwV2lkZ2V0KGFFbGVtZW50KVwiKVxuRihcIkZpbmRBc3NvY2lhdGVkR2xvYmFsKClcIilcblIoXCJSZXBhcmVudFdyYXBwZXJzSW5VQVdpZGdldFN1YnRyZWUobnNJQ29udGVudCogYVJvb3QpXCIpXG5cbkFbXCJuc1hQQ0NvbXBvbmVudHNfVXRpbHM6OkdldFVBV2lkZ2V0U2NvcGUobW96aWxsYTo6ZG9tOjpFbGVtZW50KiBob3N0RWxlbWVudCxcbkpTQ29udGV4dCogY3gsXG5NdXRhYmxlSGFuZGxlVmFsdWUgcnZhbClcIl1cbkJbXCJ4cGM6OkdldFVBV2lkZ2V0U2NvcGUoSlNDb250ZXh0KiBjeCwgSlNPYmplY3QqIGNvbnRlbnRTY29wZUFyZylcIl1cbkNbXCJYUENXcmFwcGVkTmF0aXZlU2NvcGU6OkdldFVBV2lkZ2V0U2NvcGUoSlNDb250ZXh0KiBjeClcIl1cbkRbXCJYUENKU1J1bnRpbWU6OkdldFVBV2lkZ2V0U2NvcGUoSlNDb250ZXh0KiBjeCwgbnNJUHJpbmNpcGFsKiBwcmluY2lwYWwpXCJdXG5cblMoXCJ4cGM6OkNyZWF0ZVNhbmRib3hPYmplY3QoKVwiKVxuXG5KIC0tPiBBXG5GIC0tPiBCXG5SIC0tPiBCXG5cbkEgLS0-IERcbkIgLS0-IENcbkMgLS0-IERcblxuRCAtLT4gUyIsIm1lcm1haWQiOnsidGhlbWUiOiJkZWZhdWx0In19

Comment 78

[Tim Guan-tin Chien [:timdream] (please needinfo)]

Attachment: Bug 1431255 - Part I, Add dom.ua_widget.enabled pref and enable for Desktop Nightly only

This would allow us to control this feature independently of Shadow DOM.
The feature is only turned on when Shadow DOM is turned on.

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

Comment 79

[Tim Guan-tin Chien [:timdream] (please needinfo)]

Attachment: Bug 1431255 - Part VI, Redirect target in ContextMenu.jsm to HTMLMediaElement

ALlow context menu makes the right decision so it shows the media element
related menu items.

This is not the right fix; we'll need something else that identifies UA widget
shadow DOM boundary.

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

Comment 80

[Tim Guan-tin Chien [:timdream] (please needinfo)]

Attachment: Bug 1431255 - Part VIII, Adjust videocontrols tests to test on UA Widget

Also, duplicate the unchanged tests to xbl dir, ensuring the legacy XBL
videocontrols continue to work.

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

Comment 81

[Tim Guan-tin Chien [:timdream] (please needinfo)]

Comment on attachment 8988356 [details]
Bug 1431255 - Part II, Create a Shadow Root in HTMLMediaElement when enabled, skipping <xul:videocontrols>

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

Comment 82

[Tim Guan-tin Chien [:timdream] (please needinfo)]

Comment on attachment 8988357 [details]
Bug 1431255 - Part III, Create per-origin sandboxes from XPCJSRuntime and load UA widgets scripts

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/253612/diff/4-5/

Comment 83

[Tim Guan-tin Chien [:timdream] (please needinfo)]

Comment on attachment 8988358 [details]
Bug 1431255 - Part IV, Load videocontrols.js, migrated from videoControls binding

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/253614/diff/4-5/

Comment 84

[Tim Guan-tin Chien [:timdream] (please needinfo)]

Comment on attachment 8989602 [details]
Bug 1431255 - Part V, Set the reflectors of the UA Widget DOM to UA Widget Scope

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

Comment 85

[Tim Guan-tin Chien [:timdream] (please needinfo)]

Comment on attachment 8989602 [details]
Bug 1431255 - Part V, Set the reflectors of the UA Widget DOM to UA Widget Scope

Patch updated for a better way to get to the principal.

I've moved the tests over and realized I should handle the case when the element is running on chrome pages. Will update patch again when that's fixed.

Comment 86

[Bobby Holley (:bholley)]

Comment on attachment 8989602 [details]
Bug 1431255 - Part V, Set the reflectors of the UA Widget DOM to UA Widget Scope

https://reviewboard.mozilla.org/r/254628/#review262306

(In reply to Tim Guan-tin Chien [:timdream] (please needinfo) from comment #76)
> Comment on attachment 8989602 [details]
> Bug 1431255 - Set the reflectors of the UA Widget DOM to UA Widget Scope
> 
> This does what's discussed, but I did not observe the effect of restricting
> the DOM access to the content script. I suspect some other flag of the node
> is also checked? I couldn't find it yet.

Not sure, you'll need to debug. But per comments I think we should avoid using an entire nmode flag for this if we can.

> 
> About ReparentWrappersInSubtree(): it looks like it updates the wrappers of
> nodes to XBL scope, when a usual node is inserted into XBL anonymous tree. I
> can see that the button element created in videocontrols.xml (with
> createElement("button")) is exposed to the content if I comment out that
> part. I can also see that with the way the nodes are inserted from
> videocontrols.js, a "ReparentWrappersInUAWidgetSubtree()" method in that
> place can intercept it. Does that mean we no longer need special element
> insertion APIs to make this work? Let me know if you have thoughts.

I think the result of our discussion in SF was that we'd forbid adoption to/from UA widget scopes (since we control all the code). So I think we should delete this code.


> 
> One more thing: I unfortunately have to litter a lot of function w/ the name
> "GetUAWidgetScope" in various places, just to get to implementation and pick
> up the principal etc. I made a flowchart on that so hopefully, it is more
> clear. I will upload it here.

::: dom/base/nsINode.h:138
(Diff revision 2)
> +  // Whether the node is in a shadow tree implementing UA widget
> +  NODE_IS_IN_UA_WIDGET_SHADOW_TREE =      NODE_FLAG_BIT(9),

Node bits are very valuable. Do we really need this? My previous suggestion was to check if we're in a shadow tree, and if so, check whether the host element is one that has a UA Widget.

::: dom/bindings/BindingDeclarations.h:524
(Diff revision 2)
>  GetWrapperCache(const SmartPtr<T>& aObject)
>  {
>    return GetWrapperCache(aObject.get());
>  }
>  
> +enum class PrivilegedScope {

I'd call this ReflectionScope, and change None to Content.

::: js/xpconnect/src/nsXPConnect.cpp:1190
(Diff revision 2)
>      // compat and not security for remote XUL, we just always claim to be XBL.
>      //
>      // Note that, for performance, we don't check AllowXULXBLForPrincipal here,
>      // and instead rely on the fact that AllowContentXBLScope() only returns false in
>      // remote XUL situations.
> -    return AccessCheck::isChrome(c) || IsContentXBLCompartment(c) || !AllowContentXBLScope(realm);
> +    return AccessCheck::isChrome(c) || IsContentXBLCompartment(c) || !AllowContentXBLScope(realm) || IsUAWidgetCompartment(c);

Do we actually need this?

Comment 87

[Tim Guan-tin Chien [:timdream] (please needinfo)]

Comment on attachment 8989602 [details]
Bug 1431255 - Part V, Set the reflectors of the UA Widget DOM to UA Widget Scope

https://reviewboard.mozilla.org/r/254628/#review262306

> Do we actually need this?

Yes we do; without it videocontrols.js will not have access to IsChromeOrXBL methods like

https://searchfox.org/mozilla-central/source/dom/webidl/HTMLVideoElement.webidl#55

Comment 88

[Bobby Holley (:bholley)]

(In reply to Tim Guan-tin Chien [:timdream] (please needinfo) from comment #87)
> Comment on attachment 8989602 [details]
> Bug 1431255 - Set the reflectors of the UA Widget DOM to UA Widget Scope
> 
> https://reviewboard.mozilla.org/r/254628/#review262306
> 
> > Do we actually need this?
> 
> Yes we do; without it videocontrols.js will not have access to IsChromeOrXBL
> methods like
> 
> https://searchfox.org/mozilla-central/source/dom/webidl/HTMLVideoElement.
> webidl#55

In that case, let's add a new IsChromeOrXBLOrUAWidget, so that we only expose the things we actually need. A lot of the stuff currently exposed as IsChromeOrXBL isn't relevant to UAWidget.

Comment 89

[Tim Guan-tin Chien [:timdream] (please needinfo)]

Attachment: Bug 1431255 - (To be removed) Put stylesheet inline

Unlike XBL, apparently stylesheet no longer blocks the constructor.
An event listener is added to work with that. This also removes FOUC.

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

Comment 90

[Tim Guan-tin Chien [:timdream] (please needinfo)]

Attachment: Bug 1431255 - Part IX, Remove fullscreenchange workaround for bug 718107

Also, acknowledge the control bar hidden behavior caused by the bug.

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

Comment 91

[Tim Guan-tin Chien [:timdream] (please needinfo)]

Attachment: Bug 1431255 - Part X, Remove XBL workaround in bug 462114

This is documented throughly in bug 448909 comment 79.
It is no longer needed for our current setup.

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

Comment 92

[Tim Guan-tin Chien [:timdream] (please needinfo)]

Attachment: Bug 1431255 - Part XI, Remove bogus comments in videocontrols.js

Also, unremoved <xul:slider> properties

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

Comment 93

[Tim Guan-tin Chien [:timdream] (please needinfo)]

Comment on attachment 8990242 [details]
Bug 1431255 - Part I, Add dom.ua_widget.enabled pref and enable for Desktop Nightly only

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

Comment 94

[Tim Guan-tin Chien [:timdream] (please needinfo)]

Comment on attachment 8988356 [details]
Bug 1431255 - Part II, Create a Shadow Root in HTMLMediaElement when enabled, skipping <xul:videocontrols>

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/253610/diff/4-5/

Comment 95

[Tim Guan-tin Chien [:timdream] (please needinfo)]

Comment on attachment 8988357 [details]
Bug 1431255 - Part III, Create per-origin sandboxes from XPCJSRuntime and load UA widgets scripts

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/253612/diff/5-6/

Comment 96

[Tim Guan-tin Chien [:timdream] (please needinfo)]

Comment on attachment 8988358 [details]
Bug 1431255 - Part IV, Load videocontrols.js, migrated from videoControls binding

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/253614/diff/5-6/

Comment 97

[Tim Guan-tin Chien [:timdream] (please needinfo)]

Comment on attachment 8989602 [details]
Bug 1431255 - Part V, Set the reflectors of the UA Widget DOM to UA Widget Scope

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

Comment 98

[Tim Guan-tin Chien [:timdream] (please needinfo)]

Comment on attachment 8990243 [details]
Bug 1431255 - Part VI, Redirect target in ContextMenu.jsm to HTMLMediaElement

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

Comment 99

[Tim Guan-tin Chien [:timdream] (please needinfo)]

Comment on attachment 8990244 [details]
Bug 1431255 - Part VIII, Adjust videocontrols tests to test on UA Widget

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

Comment 100

[Tim Guan-tin Chien [:timdream] (please needinfo)]

Comment on attachment 8990242 [details]
Bug 1431255 - Part I, Add dom.ua_widget.enabled pref and enable for Desktop Nightly only

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

Comment 101

[Tim Guan-tin Chien [:timdream] (please needinfo)]

Comment on attachment 8988356 [details]
Bug 1431255 - Part II, Create a Shadow Root in HTMLMediaElement when enabled, skipping <xul:videocontrols>

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/253610/diff/5-6/

Comment 102

[Tim Guan-tin Chien [:timdream] (please needinfo)]

Comment on attachment 8988357 [details]
Bug 1431255 - Part III, Create per-origin sandboxes from XPCJSRuntime and load UA widgets scripts

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/253612/diff/6-7/

Comment 103

[Tim Guan-tin Chien [:timdream] (please needinfo)]

Comment on attachment 8988358 [details]
Bug 1431255 - Part IV, Load videocontrols.js, migrated from videoControls binding

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/253614/diff/6-7/

Comment 104

[Tim Guan-tin Chien [:timdream] (please needinfo)]

Comment on attachment 8989602 [details]
Bug 1431255 - Part V, Set the reflectors of the UA Widget DOM to UA Widget Scope

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

Comment 105

[Tim Guan-tin Chien [:timdream] (please needinfo)]

Comment on attachment 8990243 [details]
Bug 1431255 - Part VI, Redirect target in ContextMenu.jsm to HTMLMediaElement

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

Comment 106

[Tim Guan-tin Chien [:timdream] (please needinfo)]

Comment on attachment 8990244 [details]
Bug 1431255 - Part VIII, Adjust videocontrols tests to test on UA Widget

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

Comment 107

[Tim Guan-tin Chien [:timdream] (please needinfo)]

Comment on attachment 8990871 [details]
Bug 1431255 - (To be removed) Put stylesheet inline

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

Comment 108

[Tim Guan-tin Chien [:timdream] (please needinfo)]

Comment on attachment 8990872 [details]
Bug 1431255 - Part IX, Remove fullscreenchange workaround for bug 718107

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

Comment 109

[Tim Guan-tin Chien [:timdream] (please needinfo)]

Comment on attachment 8990873 [details]
Bug 1431255 - Part X, Remove XBL workaround in bug 462114

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

Comment 110

[Tim Guan-tin Chien [:timdream] (please needinfo)]

Comment on attachment 8990874 [details]
Bug 1431255 - Part XI, Remove bogus comments in videocontrols.js

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

Comment 111

[Tim Guan-tin Chien [:timdream] (please needinfo)]

Alright. The reason why my patch broke is that I didn't initialize CompartmentPrivate::isUAWidgetCompartment. Strangely the compiler did not yell at me.

The patch should work now.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=008482bb92ccdd2cdbb12d01f610107098869979

There are a few things:

1. I am not sure where I should move the PageWidgets class. Ideally, it should be in a frame script that we would load in both Fennec and Desktop, and even Reftest. Currently, it is disabled on Fennec and Reftest.

2. The feature is disabled too on a few DevTools inspector tests. They assert # of children (anon and non-anon) under the <video> node, which certainly has changed with this patch. It would be better if this is addressed altogether when we decide how to present UA Widget in DevTools.

3. I am still not sure about ReparentWrappersInSubtree(); I know the plan is to create separate DOM APIs for inserting these elements, but I have not yet thought of good interfaces for the needed use cases. We'll need something accepts a DOM tree imported from XML parser in HTML mode [1] because of localization, and the good old createElement() and createTextNode() [2].

[1] https://hg.mozilla.org/try/rev/e3048945368aa8dd8cc8716ba6a4928770a9e9ab#l5.1163
[2] https://hg.mozilla.org/try/rev/e3048945368aa8dd8cc8716ba6a4928770a9e9ab#l5.630

I also feel that implementing APIs for [1] and [2], while separate, it's internal working would not be far from the ReparentWrappersInSubtree() method we have right now.

Are there any specific reasons why we don't want to reuse ReparentWrappersInSubtree() like [3]?

[3] https://hg.mozilla.org/try/rev/fa50606f942dd3b5f3ff2bea5b832774c4d6b496

Bobby, let me know if you have any insight on (3). I will start asking for reviews once we made a final decision on that. Thanks!

Comment 112

[Bobby Holley (:bholley)]

(In reply to Tim Guan-tin Chien [:timdream] (please needinfo) from comment #111)
> Alright. The reason why my patch broke is that I didn't initialize
> CompartmentPrivate::isUAWidgetCompartment. Strangely the compiler did not
> yell at me.

Initialize it at construction time or set it to true when creating a UA widget compartment? The compiler would only be able to yell at you about the former, and leaving members uninitialized is allowed in C++ (though I think we finally landed some static analysis to forbid it).

> 
> The patch should work now.
> 
> https://treeherder.mozilla.org/#/
> jobs?repo=try&revision=008482bb92ccdd2cdbb12d01f610107098869979
> 
> There are a few things:
> 
> 1. I am not sure where I should move the PageWidgets class. Ideally, it
> should be in a frame script that we would load in both Fennec and Desktop,
> and even Reftest. Currently, it is disabled on Fennec and Reftest.

In terms of "where in the tree", I'm not the right person to say. But I do think that we should minimize the logic that needs to be parsed and loaded for every page, and push as much as possible into the per-widget logic (which can be loaded on-demand in the cases that widget is actually used).

> 
> 2. The feature is disabled too on a few DevTools inspector tests. They
> assert # of children (anon and non-anon) under the <video> node, which
> certainly has changed with this patch. It would be better if this is
> addressed altogether when we decide how to present UA Widget in DevTools.

Yeah, that seems fine to punt on for now.
 
> 3. I am still not sure about ReparentWrappersInSubtree(); I know the plan is
> to create separate DOM APIs for inserting these elements, but I have not yet
> thought of good interfaces for the needed use cases.

The idea we discussed in SF was that we'd just have something like createElementForUAWidget, createTextNodeForUAWidget, and importNodeForUAWidget, expose them only in UAWidget scopes, and un-expose the non-ForShadowRoot methods in UAWidget scopes. These methods would then find a way to ensure that the resulting node was associated with the shadow root before being exposed to JS, so that the reflector ends up in the right place. The easiest (but kinda hacky) way to do this with the current setup would be to just append them as a child to the shadow root before invoking ToJS, and then remove them again. That's probably fine, though there's some chance it could trigger extra layout flushes in corner cases. Certainly fine for starting though.

> We'll need something
> accepts a DOM tree imported from XML parser in HTML mode [1] because of
> localization, and the good old createElement() and createTextNode() [2].
> 
> [1]
> https://hg.mozilla.org/try/rev/e3048945368aa8dd8cc8716ba6a4928770a9e9ab#l5.
> 1163
> [2]
> https://hg.mozilla.org/try/rev/e3048945368aa8dd8cc8716ba6a4928770a9e9ab#l5.
> 630
> 
> I also feel that implementing APIs for [1] and [2], while separate, it's
> internal working would not be far from the ReparentWrappersInSubtree()
> method we have right now.

The internals would be entirely different, see below.

> 
> Are there any specific reasons why we don't want to reuse
> ReparentWrappersInSubtree() like [3]?

ReparentWrappersInSubtree relies on a whole pile of black magic to move a reflector from one JS global to another (the TransplantObject stuff). That stuff is quite touchy, and we may even be able to get rid of it at some point, so I don't want to add a new dependency here, when we can instead just expose APIs to create the reflector in the right place to start.

Comment 113

[Tim Guan-tin Chien [:timdream] (please needinfo)]

(In reply to Bobby Holley (:bholley) from comment #112)
> (In reply to Tim Guan-tin Chien [:timdream] (please needinfo) from comment
> #111)
> > Alright. The reason why my patch broke is that I didn't initialize
> > CompartmentPrivate::isUAWidgetCompartment. Strangely the compiler did not
> > yell at me.
> 
> Initialize it at construction time or set it to true when creating a UA
> widget compartment? The compiler would only be able to yell at you about the
> former, and leaving members uninitialized is allowed in C++ (though I think
> we finally landed some static analysis to forbid it).
> 

I mean, https://hg.mozilla.org/try/rev/34357122d42f777c11566bfd19edf2ad5e26ddf5#l14.12

--- a/js/xpconnect/src/XPCJSRuntime.cpp
+++ b/js/xpconnect/src/XPCJSRuntime.cpp
@@ -172,16 +172,17 @@ public:
 namespace xpc {
 
 CompartmentPrivate::CompartmentPrivate(JS::Compartment* c)
     : wantXrays(false)
     , allowWaivers(true)
     , isWebExtensionContentScript(false)
     , allowCPOWs(false)
     , isContentXBLCompartment(false)
+    , isUAWidgetCompartment(false)
     , isAddonCompartment(false)
     , universalXPConnectEnabled(false)
     , forcePermissiveCOWs(false)
     , wasNuked(false)
     , mWrappedJSMap(JSObject2WrappedJSMap::newMap(XPC_JS_MAP_LENGTH))
 {
     MOZ_COUNT_CTOR(xpc::CompartmentPrivate);
     mozilla::PodArrayZero(wrapperDenialWarnings);

and the compiler didn't error and it returns |true| when I access it elsewhere.

> ReparentWrappersInSubtree relies on a whole pile of black magic to move a reflector from one JS global to another

OK, I will not do that.

Comment 114

[Bobby Holley (:bholley)]

(In reply to Tim Guan-tin Chien [:timdream] (please needinfo) from comment #113)
> and the compiler didn't error and it returns |true| when I access it
> elsewhere.

Yeah, that's allowed per C++. We're very close to getting static analysis to check for this in bug 525063, but looks like it hasn't landed yet.

Comment 115

[Tim Guan-tin Chien [:timdream] (please needinfo)]

Attachment: Bug 1431255 - Part VII, Allow Shadow DOM in SVG <use> subtree creation

This check prevents us from recurring into frame construction when the video
element is bound into a SVG <use> subtree.

Specifically, this allow UA Widget to pass in dom/svg/crashtests/1282985-1.svg.

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

Comment 116

[Tim Guan-tin Chien [:timdream] (please needinfo)]

Attachment: Bug 1431255 - Part VII, Trap mouse/touch/pointer events in audio/video element for UI Widgets

This is akin to whats done in bug 1327097

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

Comment 117

[Tim Guan-tin Chien [:timdream] (please needinfo)]

Comment on attachment 8988358 [details]
Bug 1431255 - Part IV, Load videocontrols.js, migrated from videoControls binding

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/253614/diff/7-8/

Comment 118

[Tim Guan-tin Chien [:timdream] (please needinfo)]

Comment on attachment 8989602 [details]
Bug 1431255 - Part V, Set the reflectors of the UA Widget DOM to UA Widget Scope

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/254628/diff/4-5/

Comment 119

[Tim Guan-tin Chien [:timdream] (please needinfo)]

Comment on attachment 8990243 [details]
Bug 1431255 - Part VI, Redirect target in ContextMenu.jsm to HTMLMediaElement

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

Comment 120

[Tim Guan-tin Chien [:timdream] (please needinfo)]

Comment on attachment 8990244 [details]
Bug 1431255 - Part VIII, Adjust videocontrols tests to test on UA Widget

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

Comment 121

[Tim Guan-tin Chien [:timdream] (please needinfo)]

Comment on attachment 8990871 [details]
Bug 1431255 - (To be removed) Put stylesheet inline

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

Comment 122

[Tim Guan-tin Chien [:timdream] (please needinfo)]

Comment on attachment 8990872 [details]
Bug 1431255 - Part IX, Remove fullscreenchange workaround for bug 718107

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

Comment 123

[Tim Guan-tin Chien [:timdream] (please needinfo)]

Comment on attachment 8990873 [details]
Bug 1431255 - Part X, Remove XBL workaround in bug 462114

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

Comment 124

[Tim Guan-tin Chien [:timdream] (please needinfo)]

Comment on attachment 8990874 [details]
Bug 1431255 - Part XI, Remove bogus comments in videocontrols.js

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

Comment 125

[Tim Guan-tin Chien [:timdream] (please needinfo)]

It turned out the APIs can be a lot simpler than I thought. Given that the goal here is to append DOM element before its reflector is exposed to JS (and given that the DOM element will be eventually appended anyway), I simply created

ShadowRoot.importNodeAndAppendChildAt(parentNode, node, deep)
ShadowRoot.createElementAndAppendChildAt(parentNode, localName)

for our two use cases in videocontrols.

I'll start asking for reviews after verifying the try results.

Comment 126

[Tim Guan-tin Chien [:timdream] (please needinfo)]

Comment on attachment 8990242 [details]
Bug 1431255 - Part I, Add dom.ua_widget.enabled pref and enable for Desktop Nightly only

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

Comment 127

[Tim Guan-tin Chien [:timdream] (please needinfo)]

Comment on attachment 8988356 [details]
Bug 1431255 - Part II, Create a Shadow Root in HTMLMediaElement when enabled, skipping <xul:videocontrols>

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/253610/diff/6-7/

Comment 128

[Tim Guan-tin Chien [:timdream] (please needinfo)]

Comment on attachment 8988357 [details]
Bug 1431255 - Part III, Create per-origin sandboxes from XPCJSRuntime and load UA widgets scripts

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/253612/diff/7-8/

Comment 129

[Tim Guan-tin Chien [:timdream] (please needinfo)]

Comment on attachment 8988358 [details]
Bug 1431255 - Part IV, Load videocontrols.js, migrated from videoControls binding

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/253614/diff/8-9/

Comment 130

[Tim Guan-tin Chien [:timdream] (please needinfo)]

Comment on attachment 8989602 [details]
Bug 1431255 - Part V, Set the reflectors of the UA Widget DOM to UA Widget Scope

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/254628/diff/5-6/

Comment 131

[Tim Guan-tin Chien [:timdream] (please needinfo)]

Comment on attachment 8990243 [details]
Bug 1431255 - Part VI, Redirect target in ContextMenu.jsm to HTMLMediaElement

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/255284/diff/4-5/

Comment 132

[Tim Guan-tin Chien [:timdream] (please needinfo)]

Comment on attachment 8991513 [details]
Bug 1431255 - Part VII, Allow Shadow DOM in SVG <use> subtree creation

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

Comment 133

[Tim Guan-tin Chien [:timdream] (please needinfo)]

Comment on attachment 8991514 [details]
Bug 1431255 - Part VII, Trap mouse/touch/pointer events in audio/video element for UI Widgets

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

Comment 134

[Tim Guan-tin Chien [:timdream] (please needinfo)]

Comment on attachment 8990244 [details]
Bug 1431255 - Part VIII, Adjust videocontrols tests to test on UA Widget

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/255286/diff/4-5/

Comment 135

[Tim Guan-tin Chien [:timdream] (please needinfo)]

Comment on attachment 8990871 [details]
Bug 1431255 - (To be removed) Put stylesheet inline

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

Comment 136

[Tim Guan-tin Chien [:timdream] (please needinfo)]

Comment on attachment 8990872 [details]
Bug 1431255 - Part IX, Remove fullscreenchange workaround for bug 718107

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

Comment 137

[Tim Guan-tin Chien [:timdream] (please needinfo)]

Comment on attachment 8990873 [details]
Bug 1431255 - Part X, Remove XBL workaround in bug 462114

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

Comment 138

[Tim Guan-tin Chien [:timdream] (please needinfo)]

Comment on attachment 8990874 [details]
Bug 1431255 - Part XI, Remove bogus comments in videocontrols.js

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

Comment 139

[Tim Guan-tin Chien [:timdream] (please needinfo)]

I am still working on identifying the cause of new intermittent oranges showing up on Try, so holding off front-end related patches for now.

Comment 140

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

Comment on attachment 8990242 [details]
Bug 1431255 - Part I, Add dom.ua_widget.enabled pref and enable for Desktop Nightly only

https://reviewboard.mozilla.org/r/255282/#review263442

::: dom/base/nsContentUtils.h:3108
(Diff revision 4)
>    CreateJSValueFromSequenceOfObject(JSContext* aCx,
>                                      const mozilla::dom::Sequence<JSObject*>& aTransfer,
>                                      JS::MutableHandle<JS::Value> aValue);
>  
>    static bool
> +  IsUAWidgetEnabled() { return sIsShadowDOMEnabled && sIsUAWidgetEnabled; }

It isn't clear from the name what UAWidget means.
At least add some comment

Comment 141

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

Have you done any performance testing? Is the new setup faster or slower than the existing one?

Comment 142

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

Comment on attachment 8988356 [details]
Bug 1431255 - Part II, Create a Shadow Root in HTMLMediaElement when enabled, skipping <xul:videocontrols>

https://reviewboard.mozilla.org/r/253610/#review263444

::: dom/html/HTMLMediaElement.cpp:4533
(Diff revision 7)
> +    AsyncEventDispatcher* dispatcher =
> +      new AsyncEventDispatcher(this,
> +                               NS_LITERAL_STRING("UAWidgetBindToTree"),
> +                               CanBubble::eYes,
> +                               ChromeOnlyDispatch::eYes);
> +    dispatcher->PostDOMEvent();

This means that UAWidgetBindToTree is possibly handled way after the next refresh driver tick and user may see some unexpected UI, assume the even is used for initializing the UI.


Can the size of the controls affect to the size of the video element? If so, async event handling would be problematic also in that case.

::: layout/base/nsCSSFrameConstructor.cpp:5518
(Diff revision 7)
>    // never create frames for comments or PIs
>    if (aContent->IsComment() || aContent->IsProcessingInstruction()) {
>      return false;
>    }
>  
> +  // never create frames inside video that is not in the Shadow DOM, i.e. not

This looks very hackish. I'm not a layout peer, but from maintenance point of view this looks rather worrisome.

Comment 143

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

Comment on attachment 8991514 [details]
Bug 1431255 - Part VII, Trap mouse/touch/pointer events in audio/video element for UI Widgets

https://reviewboard.mozilla.org/r/256420/#review263448

Comment 144

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 8988356 [details]
Bug 1431255 - Part II, Create a Shadow Root in HTMLMediaElement when enabled, skipping <xul:videocontrols>

https://reviewboard.mozilla.org/r/253610/#review263648

::: layout/base/nsCSSFrameConstructor.cpp:5522
(Diff revision 7)
>  
> +  // never create frames inside video that is not in the Shadow DOM, i.e. not
> +  // video controls. Anonymous contents (the poster image, caption div,
> +  // and the old XBL binding video controls) don't go through here.
> +  if (aParentFrame && aParentFrame->IsHTMLVideoFrame() &&
> +      (!aContent->IsElement() || !aContent->AsElement()->IsInShadowTree())) {

This is the wrong place to do this, the right place to do this would be the same we have for suppressing `<select>` or `<details>` children, for example (see `ShouldSuppressFrameInSelect`, etc.).

But I don't think we want to add more special cases lightly. Why do we need it? If we don't want to create frames for the poster, etc, we should just avoid adding them in `nsVideoFrame::CreateAnonymousContent`.

Comment 145

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 8991513 [details]
Bug 1431255 - Part VII, Allow Shadow DOM in SVG <use> subtree creation

https://reviewboard.mozilla.org/r/256418/#review263650

::: commit-message-ab1f2:4
(Diff revision 2)
> +Bug 1431255 - Part VII, Allow Shadow DOM in SVG <use> subtree creation
> +
> +This check prevents us from recurring into frame construction when the video
> +element is bound into a SVG <use> subtree.

Which check? How does the failure look like?

I don't  think this is right off-hand, why is svg use subtrees special so we don't need to destroy frames for it, yet the assert below passes?

Comment 146

[Tim Guan-tin Chien [:timdream] (please needinfo)]

Attachment: Backtrace running dom/svg/crashtests/1282985-1.svg (Part VII)

(In reply to Emilio Cobos Álvarez (:emilio) from comment #145)

Please refer to the backtrace of the test case. When nsSVGUseFrame is being processed, it will clone the subtree <use> is referenced as anonymous content from SVGUseElement::CreateAnonymousContent(). The test case is the only test case where <video> is being cloned into the SVG <use> subtree.

The setup here needs to call Element::AttachShadowWithoutNameChecks() from HTMLMediaElement::BindToTree(), which caused the "recurring into frame construction" failure.

SVG <use> will be the only place <video> is being placed into anonymous content, therefore I concluded IsAnonymousContentInSVGUseSubtree() is sound. I've also constructed a test myself proving that the controls of two <video> elements (the original one + the one in <use> subtree) both works just like the XBL one.

Let me know if you have any alternative fix in mind.

Comment 147

[Tim Guan-tin Chien [:timdream] (please needinfo)]

(In reply to Emilio Cobos Álvarez (:emilio) from comment #144)
> But I don't think we want to add more special cases lightly. Why do we need
> it? If we don't want to create frames for the poster, etc, we should just
> avoid adding them in `nsVideoFrame::CreateAnonymousContent`.

What I tried to do is to have the frame from UA Widget <div id="videocontrols"> element, attached by us in UA Shadow DOM, being accepted, without accepting frames created by DOM elements added by the web content (think of <video><span>hello</span></video>).

As the comment stated (not clear enough, perhaps?), this is not related to adding poster/caption frames, which will come from NAC that we will create with or without UA Shadow Root.

We will skip the creation of <xul:videocontrols> NAC when UA Widget is enabled, thus skipping the frame creation and XBL binding (see changes in nsVideoFrame.cpp).

Noted that nsVideoFrame was marked as IsLeaf before (and ignores all the children except for NACs), which I had to turn off.

Comment 148

[Emilio Cobos Álvarez (:emilio)]

(In reply to Tim Guan-tin Chien [:timdream] (please needinfo) from comment #147)
> (In reply to Emilio Cobos Álvarez (:emilio) from comment #144)
> > But I don't think we want to add more special cases lightly. Why do we need
> > it? If we don't want to create frames for the poster, etc, we should just
> > avoid adding them in `nsVideoFrame::CreateAnonymousContent`.
> 
> What I tried to do is to have the frame from UA Widget <div
> id="videocontrols"> element, attached by us in UA Shadow DOM, being
> accepted, without accepting frames created by DOM elements added by the web
> content (think of <video><span>hello</span></video>).

Ok, I see, so that's because you turned <video> into a non-Leaf frame. Shouldn't that happen automatically though, given the child nodes are not slotted anywhere?

Comment 149

[Emilio Cobos Álvarez (:emilio)]

(In reply to Tim Guan-tin Chien [:timdream] (please needinfo) from comment #146)
> Created attachment 8991925 [details]
> Backtrace running dom/svg/crashtests/1282985-1.svg (Part VII)
> 
> (In reply to Emilio Cobos Álvarez (:emilio) from comment #145)
> 
> Please refer to the backtrace of the test case. When nsSVGUseFrame is being
> processed, it will clone the subtree <use> is referenced as anonymous
> content from SVGUseElement::CreateAnonymousContent(). The test case is the
> only test case where <video> is being cloned into the SVG <use> subtree.
> 
> The setup here needs to call Element::AttachShadowWithoutNameChecks() from
> HTMLMediaElement::BindToTree(), which caused the "recurring into frame
> construction" failure.

I guess the content in the <svg:use> subtree can't dynamically change, can it? If it can, that fix wouldn't be generally sound...

<svg:use> is really annoying, tbh. We should probably just fix bug 1450250...

Comment 150

[Tim Guan-tin Chien [:timdream] (please needinfo)]

(In reply to Olli Pettay [:smaug] (vacation Jul 15->) from comment #142)
> This means that UAWidgetBindToTree is possibly handled way after the next
> refresh driver tick and user may see some unexpected UI, assume the even is
> used for initializing the UI.
> 
> Can the size of the controls affect to the size of the video element? If so,
> async event handling would be problematic also in that case.

Unfortunately yes; for <audio controls>, the element will grow to the dimension of the control bars. Also, if you look at Part X, you will realize the video controls could not render on the tick of the event, because unlike XBL, the stylesheet load does not block (the "controls" is blank before the stylesheet is applied, I checked).

So what's the problem if we couldn't size the <audio> element in that tick? I didn't know that's a requirement. If that's the case, maybe for <audio controls> we will need to hard code a few numbers in nsVideoFrame? I don't understand layout enough to suggest the best approach here.

Comment 151

[Tim Guan-tin Chien [:timdream] (please needinfo)]

(In reply to Emilio Cobos Álvarez (:emilio) from comment #149)
> I guess the content in the <svg:use> subtree can't dynamically change, can
> it? If it can, that fix wouldn't be generally sound...
> 
> <svg:use> is really annoying, tbh. We should probably just fix bug 1450250...

The <use> tree only changes if the tree it referred changes. Does that means the patch is acceptable?

(In reply to Emilio Cobos Álvarez (:emilio) from comment #148)
> > What I tried to do is to have the frame from UA Widget <div
> > id="videocontrols"> element, attached by us in UA Shadow DOM, being
> > accepted, without accepting frames created by DOM elements added by the web
> > content (think of <video><span>hello</span></video>).
> 
> Ok, I see, so that's because you turned <video> into a non-Leaf frame.
> Shouldn't that happen automatically though, given the child nodes are not
> slotted anywhere?

It didn't, I can try again (hold tight; recompiling)

Comment 152

[Emilio Cobos Álvarez (:emilio)]

(In reply to Tim Guan-tin Chien [:timdream] (please needinfo) from comment #151)
> (In reply to Emilio Cobos Álvarez (:emilio) from comment #149)
> > I guess the content in the <svg:use> subtree can't dynamically change, can
> > it? If it can, that fix wouldn't be generally sound...
> > 
> > <svg:use> is really annoying, tbh. We should probably just fix bug 1450250...
> 
> The <use> tree only changes if the tree it referred changes. Does that means
> the patch is acceptable?
> 
> (In reply to Emilio Cobos Álvarez (:emilio) from comment #148)
> > > What I tried to do is to have the frame from UA Widget <div
> > > id="videocontrols"> element, attached by us in UA Shadow DOM, being
> > > accepted, without accepting frames created by DOM elements added by the web
> > > content (think of <video><span>hello</span></video>).
> > 
> > Ok, I see, so that's because you turned <video> into a non-Leaf frame.
> > Shouldn't that happen automatically though, given the child nodes are not
> > slotted anywhere?
> 
> It didn't, I can try again (hold tight; recompiling)

I guess if you want to make this work with both NAC and this mechanism, you may need to use the dynamic IsLeaf stuff, and override nsIFrame::IsLeafDynamic.

Comment 153

[Tim Guan-tin Chien [:timdream] (please needinfo)]

(In reply to Emilio Cobos Álvarez (:emilio) from comment #152)
> > It didn't, I can try again (hold tight; recompiling)
> 
> I guess if you want to make this work with both NAC and this mechanism, you
> may need to use the dynamic IsLeaf stuff, and override
> nsIFrame::IsLeafDynamic.

Cool, I'll do that. You somehow managed to read my unsubmitted comment on the need to work when pref'd off. :)

Comment 154

[Bobby Holley (:bholley)]

Comment on attachment 8988357 [details]
Bug 1431255 - Part III, Create per-origin sandboxes from XPCJSRuntime and load UA widgets scripts

https://reviewboard.mozilla.org/r/253612/#review263708

r=me with those things fixed and those additional quick look-overs by the frame script and GC people. Nice work!

::: browser/base/content/tab-content.js:532
(Diff revision 8)
>      return;
>    removeEventListener("MozAfterPaint", onFirstNonBlankPaint);
>    sendAsyncMessage("Browser:FirstNonBlankPaint");
>  });
> +
> +class UAWidgets {

Please also get review on the tab-content.js changes for somebody famililar with that stuff (kmag, mconley, Mossop, Gijs, etc).

::: browser/base/content/tab-content.js:553
(Diff revision 8)
> +    }
> +  }
> +
> +  setupWidget(aElement) {
> +    let shadowRoot = aElement.openOrClosedShadowRoot;
> +    let sandbox = Cu.getUAWidgetScope(aElement);

Seems like we should move this below the switch, so that we avoid creating the sandbox if we're not going to do anything anyway.

::: js/xpconnect/idl/xpccomponents.idl:184
(Diff revision 8)
>      /*
> +     * Get the sandbox for running JS-implemented UA widgets (video controls etc.),
> +     * hosted inside UA-created Shadow DOM.
> +     */
> +    [implicit_jscontext]
> +    jsval getUAWidgetScope(in Element hostElement);

As mentioned in comment 71, the input here should be an nsIPrincipal.

::: js/xpconnect/src/XPCJSRuntime.cpp:3075
(Diff revision 8)
> +
> +    SandboxOptions options;
> +    options.sandboxName.AssignLiteral("UA Widget Scope");
> +    options.wantXrays = false;
> +    options.wantComponents = false;
> +    options.isContentXBLScope = true; // XXX to be replaced

I understand this goes away in Part V, but is there any actual need for it now? If not, please go ahead and remove it (along with the other "to be replaced" line below).

::: js/xpconnect/src/xpcprivate.h:602
(Diff revision 8)
>  
>      static const char* const mStrings[XPCJSContext::IDX_TOTAL_COUNT];
>      jsid mStrIDs[XPCJSContext::IDX_TOTAL_COUNT];
>      JS::Value mStrJSVals[XPCJSContext::IDX_TOTAL_COUNT];
>  
> +    struct Hasher {

This looks right to me, but please run this block (and the callsite for sweep) by sfink or jonco or mccr8 to double-check.

Comment 155

[Bobby Holley (:bholley)]

Comment on attachment 8989602 [details]
Bug 1431255 - Part V, Set the reflectors of the UA Widget DOM to UA Widget Scope

https://reviewboard.mozilla.org/r/254628/#review263714

r=me with those things fixed.

::: commit-message-4ae95:10
(Diff revision 6)
> +would check its containing shadow and its UA Widget bit.
> +
> +To prevent JS access of the DOM element before it is in the
> +UA Widget Shadom DOM tree, various DOM methods are set to inaccessible to
> +UA Widget script. It would need to use the two special methods in ShadowRoot
> +instead to insert the DOM directly into.

into the shadow tree.

::: dom/base/ShadowRoot.h:175
(Diff revision 6)
> +  nsINode* ImportNodeAndAppendChildAt(nsINode& aParentNode,
> +                                      nsINode& aNode,
> +                                      bool aDeep, mozilla::ErrorResult& rv);
> +
> +  nsINode* CreateElementAndAppendChildAt(nsINode& aParentNode,
> +                                         const nsAString& aTagName,
> +                                         mozilla::ErrorResult& rv);
> +

Please add comments above these methods explaining why we need them.

::: dom/bindings/BindingDeclarations.h:526
(Diff revision 6)
> +  XBLScope,
> +  UAWidgetScope

Given that this is an enum class, usage throughout the code will be prefixed by ReflectionScope, so I don't think we need the word "Scope" again. So let's make this ReflectionScope::XBL and ReflectionScope::UAWidget.

::: dom/bindings/BindingDeclarations.h:555
(Diff revision 6)
>  
>    // We don't want to make this an nsCOMPtr because of performance reasons, but
>    // it's safe because ParentObject is a stack class.
>    nsISupports* const MOZ_NON_OWNING_REF mObject;
>    nsWrapperCache* const mWrapperCache;
> -  bool mUseXBLScope;
> +  ReflectionScope mUseReflectionScope;

This should be called mReflectionScope, not mUseReflectionScope.

::: dom/bindings/BindingUtils.h:1408
(Diff revision 6)
>    return aObject.mObject;
>  }
>  
>  template <typename T>
> -inline bool
> -GetUseXBLScope(T* aParentObject)
> +inline mozilla::dom::ReflectionScope
> +GetUseReflectionScope(T* aParentObject)

This should be called GetReflectionScope, same below.

::: dom/bindings/BindingUtils.h:1701
(Diff revision 6)
> +
> +    case ReflectionScope::Content:
> +      return obj;
> +  }
> +
> +  return nullptr;

Make this MOZ_CRASH("Unknown ReflectionScope variant") instead of returning nullptr.

::: dom/webidl/Document.webidl:66
(Diff revision 6)
>    [Pure]
>    HTMLCollection getElementsByClassName(DOMString classNames);
>    [Pure]
>    Element? getElementById(DOMString elementId);
>  
> -  [CEReactions, NewObject, Throws]
> +  [CEReactions, NewObject, Throws, Func="IsNotUAWidget"]

Please add a comment here explaining why we're restricting in UA Widget scopes.

::: dom/webidl/Node.webidl:65
(Diff revision 6)
>    [CEReactions, SetterThrows, Pure]
>             attribute DOMString? nodeValue;
>    [CEReactions, SetterThrows, GetterCanOOM,
>     SetterNeedsSubjectPrincipal=NonSystem, Pure]
>             attribute DOMString? textContent;
> -  [CEReactions, Throws]
> +  [CEReactions, Throws, Func="IsNotUAWidget"]

Comment here as well.

::: dom/webidl/ShadowRoot.webidl:36
(Diff revision 6)
>    HTMLCollection getElementsByClassName(DOMString classNames);
>    [CEReactions, SetterThrows, TreatNullAs=EmptyString]
>    attribute DOMString innerHTML;
> +
> +  // Special UA Widget Shadow DOM only methods, ensuring elements have their
> +  // reflectors in the UA Widget scope.

I'd add a sentence saying:

"When JS invokes importNode or createElement, the binding code needs to create a reflector, and so invoking those methods directly on the content document would cause the reflector to be created in the content scope, at which point it would be difficult to move into the UA Widget scope. As such, these methods allow UA widget code to simultaneously create nodes _and_ associate them with the UA widget tree, so that the reflectors get created in the right scope."

::: js/xpconnect/src/XPCComponents.cpp:2169
(Diff revision 6)
>  NS_IMETHODIMP
>  nsXPCComponents_Utils::GetUAWidgetScope(mozilla::dom::Element* hostElement,
>                                          JSContext* cx,
>                                          MutableHandleValue rval)
>  {
>      rval.set(UndefinedValue());
>  
> -    JSObject* scope = XPCJSRuntime::Get()->GetUAWidgetScope(cx, hostElement->NodePrincipal());
> -    NS_ENSURE_TRUE(scope, NS_ERROR_OUT_OF_MEMORY); // See bug 858642.
> +    JSObject* wrapper = hostElement->AsContent()->GetWrapper();
> +    if (!wrapper) {
> +        return NS_ERROR_UNEXPECTED;
> +    }
> +    JSObject* scope = xpc::GetUAWidgetScope(cx, wrapper);

So rather than doing it this way, I think we should make GetUAWidgetScope take an nsIPrincipal per my previous comment. xpc::GetUAWidgetScope should take an nsIPrincipal, and you can add an JSObject overload (which gets the compartment principal and then delegate to the main function) for usage by the binding code.

::: js/xpconnect/src/XPCWrappedNativeScope.cpp:317
(Diff revision 6)
> +    RootedObject scope(cx, XPCJSRuntime::Get()->GetUAWidgetScope(cx, principal));
> +    NS_ENSURE_TRUE(scope, nullptr); // See bug 858642.
> +
> +    scope = js::UncheckedUnwrap(scope);
> +    JS::ExposeObjectToActiveJS(scope);
> +    return scope;
> +}

So this part whould be split into a separate function from the above, and the Cu method would call it directly.

::: js/xpconnect/src/xpcpublic.h:677
(Diff revision 6)
>   */
>  bool IsChromeOrXBL(JSContext* cx, JSObject* /* unused */);
>  
>  /**
> - * Same as IsChromeOrXBL but can be used in worker threads as well.
> + * A test for whether WebIDL methods should NOT be exposed to UA Widget,
> + * so DOM elements won't be accessible to web content.

I'd say "This is used to prevent UA widget code from directly creating and adopting nodes via the content document, since they should use the special create-and-insert apis instead."

::: toolkit/content/tests/widgets/mochitest.ini:27
(Diff revision 6)
>  skip-if = (verify && debug)
> +[test_ua_widget.html]

This causes the skip-if annotation to apply to the new test, rather than the old test it was associated with. Please fix that.

::: toolkit/content/tests/widgets/test_ua_widget.html:4
(Diff revision 6)
> +<!DOCTYPE HTML>
> +<html>
> +<head>
> +  <title>Video controls test</title>

Title needs adjustment.

::: toolkit/content/tests/widgets/test_ua_widget.html:87
(Diff revision 6)
> +ok(window.spanElementFromUAWidget instanceof HTMLSpanElement, "<span> exposed");
> +ok(window.divElementFromUAWidget instanceof HTMLDivElement, "<div> exposed");
> +
> +try {
> +  window.spanElementFromUAWidget.textContent;
> +  ok(false, "Should throws.");

nit: throw (singular).

::: toolkit/content/tests/widgets/test_ua_widget.html:89
(Diff revision 6)
> +
> +try {
> +  window.spanElementFromUAWidget.textContent;
> +  ok(false, "Should throws.");
> +} catch (err) {
> +  ok(err instanceof Error, "Permission denied to access <span>");

I'd make this ok(/denied/.test(err)), and then include the error text in the explanation string, to be sure that the right kind of exception is being thrown.

::: toolkit/content/tests/widgets/test_ua_widget.html:96
(Diff revision 6)
> +
> +try {
> +  window.divElementFromUAWidget.textContent;
> +  ok(false, "Should throws.");
> +} catch (err) {
> +  ok(err instanceof Error, "Permission denied to access <div>");

Same here.

Comment 156

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

(In reply to Tim Guan-tin Chien [:timdream] (please needinfo) from comment #150)
> So what's the problem if we couldn't size the <audio> element in that tick?
Layout information needs to be accurate when getComputedStyle() or such is called.

Comment 157

[Tim Guan-tin Chien [:timdream] (please needinfo)]

Hi :dholbert,

Could you help understand what's the best approach for fixing the problem in comment 150? Maybe we do need to hardcode control bar height in nsVideoFrame.cpp? Thanks.

Comment 158

[Emilio Cobos Álvarez (:emilio)]

(In reply to Tim Guan-tin Chien [:timdream] (please needinfo) from comment #157)
> Hi :dholbert,
> 
> Could you help understand what's the best approach for fixing the problem in
> comment 150? Maybe we do need to hardcode control bar height in
> nsVideoFrame.cpp? Thanks.

I think I'm adding something similar to what you need for <svg:use> in bug 1450250.

You basically need code to ensure that by the time you get to FlushPendingNotifications all the setup is correct. Calling into Javascript for that is not ideal, though if it's trusted it may not be too bad.

In any case, it may be worth checking whether that's something we can get away with... There are definitely elements (like intrinsically sized <img>, for example) which may not accurately represent the final layout information if you query it too quickly.

The <input type="date"> and such controls have a similar setup compared to what you're introducing here, right? Are they prone to the same kind of bug?

Comment 159

[Emilio Cobos Álvarez (:emilio)]

That being said, AsyncEventDispatcher::RunDOMEventWhenSafe instead of PostDOMEvent may give you what you want?

Not 100% sure of the times we have an scriptblocker on the stack but I don't think we dispatch a lot of user-visible events from there. Only sync mutation events I think, which we're trying to get rid of anyway.

Comment 160

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

I would suggest creating the Shadow DOM content in C++.

Comment 161

[Tim Guan-tin Chien [:timdream] (please needinfo)]

To illustrate the problem, this is the test case I am using:

<script>
  var n = 5;

  function loop(el) {
    window.requestAnimationFrame(() => {
      console.log('raf', el.clientHeight, el.clientWidth);
      if (n) {
        n--;
        loop(el);
      }
    });
  }

  window.onload = function() {
    let el = new Audio();
    el.src = "audio.ogg";
    el.controls = true;
    console.log('sync', el.clientHeight, el.clientWidth);
    document.body.appendChild(el);
    loop(el);
  }
</script>

I can see the dimensions tagged with "raf" will be 0,0 sometimes, while XBL binding setup ensures they always print out the final size.

(In reply to Emilio Cobos Álvarez (:emilio) from comment #159)
> That being said, AsyncEventDispatcher::RunDOMEventWhenSafe instead of
> PostDOMEvent may give you what you want?

I don't see changing that fix this problem so I am not sure about the desired result of this change.

(In reply to Emilio Cobos Álvarez (:emilio) from comment #158)
> The <input type="date"> and such controls have a similar setup compared to
> what you're introducing here, right? Are they prone to the same kind of bug?

This is sadly correct. For <audio controls> I can get away with hardcode the dimensions, but for <input type="date"> the width would depend on the text run, so we do need the actual markup.

(In reply to Olli Pettay [:smaug] (vacation Jul 15->) from comment #160)
> I would suggest creating the Shadow DOM content in C++.

A lot of things would have to move from JS to achieve the right result. The markup (1) for sure, but also the stylesheet (2), and the initial classNames/attributes that toggle the styles (3). Isolating (3) would not be trivial, and may not be possible.

What's out options here?

Comment 162

[Tim Guan-tin Chien [:timdream] (please needinfo)]

Emilio said over IRC saying bug 1450250 will replace Part VII.

Comment 163

[Tim Guan-tin Chien [:timdream] (please needinfo)]

(It seems that I've managed to update the MozReview patches without updating the Bugzilla component. Fun.)

I've updated the patch to include all the test fixes needed.

The one and only showstopper right how is comment 142, where :smaug asks that the element must have the right bounding rect at the time of the insertion. This is illustrated in comment 161.

As discussed, to achieve that we would need to run the script and its stylesheet at the time of BindToTree. One way to do it would be moving all the sandbox and subscript loading bits back to C++ (currently in tab-content.js), plus loading the stylesheet in sync (nsDOMWindowUtils::LoadSheetInShadowRoot()?). Perhaps the part where I monitor the "controls" property with MutationObserver in videocontrols.js would have to change too, since it did not switch implementation synchronously.

VideoControls also calls into getComputedStyle() at initialization. I don't know if that will cause any issue in this unsafe timing.

So, I don't know if we want to run JS in BindToTree() synchronously as an acceptable trade-off. I'll try to do that and probe for additional issues for now, but I would *love* to get a better understand of the sentiments. Please let me know if we would like to go for another approach.

Comment 164

[Boris Zbarsky [:bzbarsky]]

We do NOT want to run JS in BindToTree.  Running it off a scriptrunner that BindToTree posts might be ok.

Comment 165

[Emilio Cobos Álvarez (:emilio)]

Olli pointed out that there may be the chance of another scriptrunner before in the queue which runs page scripts and could observe the intermediate state...

You might want to generalize the mechanism I'm adding for <use> in https://phabricator.services.mozilla.com/D2154 to work with <video> as well, that should make it work properly.

Comment 166

[Tim Guan-tin Chien [:timdream] (please needinfo)]

OK. I don't think I tested the case correctly in comment 161, because I didn't exclude the effect of stylesheet loading. With that, I can confirm that |RunDOMEventWhenSafe()| fixes the problem by running the script before appendChild() returns.

I will put that in the patch as a test case.

A slightly different test that involves the MutationObserver in videocontrols.js would be

    let el = new Audio();
    el.src = "audio.wav";
    document.body.appendChild(el);
    is(el.clientHeight, 0, "Height of audio element without controls");
    el.controls = true;
    is(el.clientHeight, 40, "Height of audio element with controls");

Which means I would need to replace that with another AsyncEventDispatcher notifying attribute change.

Oh, and I would need to load the stylesheet synchronously, so nsDOMWindowUtils::LoadSheetInShadowRoot() that is...?

(In reply to Emilio Cobos Álvarez (:emilio) from comment #165)
> You might want to generalize the mechanism I'm adding for <use> in
> https://phabricator.services.mozilla.com/D2154 to work with <video> as well,
> that should make it work properly.

I have not yet look at it. Will do.

Comment 167

[Jared Wein [:jaws] (please needinfo? me)]

Comment on attachment 8990243 [details]
Bug 1431255 - Part VI, Redirect target in ContextMenu.jsm to HTMLMediaElement

https://reviewboard.mozilla.org/r/255284/#review264796

::: commit-message-c9024:3
(Diff revision 6)
> +Bug 1431255 - Part VI, Redirect target in ContextMenu.jsm to HTMLMediaElement
> +
> +ALlow context menu makes the right decision so it shows the media element

This sounds awkward. Does this sound better?

"Set the referenced node to that of the containing media element so related menu items will be shown in the context menu."

Comment 168

[Tim Guan-tin Chien [:timdream] (please needinfo)]

Attachment: Bug 1431255 - Part XII, Don't set hidden explicitly on animating elements

For elements with an animationProp, always set its hidden state from the
startFade() method, so opposite-direction animation can be correctly
cancelled.

Additionally, considers elements with "fadeout" class as hidden,
so other state can be toggled correctly.

This patches fixes intermittent failures on slower platforms, and perma-orange
on Windows.

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

Comment 169

[Tim Guan-tin Chien [:timdream] (please needinfo)]

Comment on attachment 8990242 [details]
Bug 1431255 - Part I, Add dom.ua_widget.enabled pref and enable for Desktop Nightly only

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/255282/diff/5-6/

Comment 170

[Tim Guan-tin Chien [:timdream] (please needinfo)]

Comment on attachment 8988356 [details]
Bug 1431255 - Part II, Create a Shadow Root in HTMLMediaElement when enabled, skipping <xul:videocontrols>

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/253610/diff/8-9/

Comment 171

[Tim Guan-tin Chien [:timdream] (please needinfo)]

Comment on attachment 8988357 [details]
Bug 1431255 - Part III, Create per-origin sandboxes from XPCJSRuntime and load UA widgets scripts

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/253612/diff/9-10/

Comment 172

[Tim Guan-tin Chien [:timdream] (please needinfo)]

Comment on attachment 8988358 [details]
Bug 1431255 - Part IV, Load videocontrols.js, migrated from videoControls binding

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/253614/diff/10-11/

Comment 173

[Tim Guan-tin Chien [:timdream] (please needinfo)]

Comment on attachment 8989602 [details]
Bug 1431255 - Part V, Set the reflectors of the UA Widget DOM to UA Widget Scope

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/254628/diff/7-8/

Comment 174

[Tim Guan-tin Chien [:timdream] (please needinfo)]

Comment on attachment 8990243 [details]
Bug 1431255 - Part VI, Redirect target in ContextMenu.jsm to HTMLMediaElement

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/255284/diff/6-7/

Comment 175

[Tim Guan-tin Chien [:timdream] (please needinfo)]

Comment on attachment 8991514 [details]
Bug 1431255 - Part VII, Trap mouse/touch/pointer events in audio/video element for UI Widgets

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

Comment 176

[Tim Guan-tin Chien [:timdream] (please needinfo)]

Comment on attachment 8990244 [details]
Bug 1431255 - Part VIII, Adjust videocontrols tests to test on UA Widget

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/255286/diff/6-7/

Comment 177

[Tim Guan-tin Chien [:timdream] (please needinfo)]

Comment on attachment 8990872 [details]
Bug 1431255 - Part IX, Remove fullscreenchange workaround for bug 718107

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/255876/diff/5-6/

Comment 178

[Tim Guan-tin Chien [:timdream] (please needinfo)]

Comment on attachment 8990873 [details]
Bug 1431255 - Part X, Remove XBL workaround in bug 462114

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/255878/diff/5-6/

Comment 179

[Tim Guan-tin Chien [:timdream] (please needinfo)]

Comment on attachment 8990874 [details]
Bug 1431255 - Part XI, Remove bogus comments in videocontrols.js

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/255880/diff/5-6/

Comment 180

[Tim Guan-tin Chien [:timdream] (please needinfo)]

Comment on attachment 8990871 [details]
Bug 1431255 - (To be removed) Put stylesheet inline

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/255874/diff/5-6/

Comment 181

[Tim Guan-tin Chien [:timdream] (please needinfo)]

Just rebased the patch and did a Try run yesterday.

Two problems remaining before I can start the next review round:

1) The videocontrols.css style sheet would have to load synchronously like what we do for <resource> in XBL, or the dimension of the element will not be correct when, say |appendChild(video)| returned to the content script. The tests are added in Part IV with videocontrols.js.

2) Switch from PostDOMEvent() to RunDOMEventWhenSafe() breaks standalone video documents, where the <video> element is appended here in C++.

https://searchfox.org/mozilla-central/rev/3fdc491e118c5cdfbaf6e2d52f3466d2b27ad1de/dom/html/VideoDocument.cpp#135

I don't know why it breaks given that I am sure event listener in tab-content.js is set before BindToTree().

:emilio, could you give me some pointers on these two issues? Thanks.

Comment 182

[Emilio Cobos Álvarez (:emilio)]

For (1) the easiest could be to do a sync load and ensure that the XUL prototype cache has it cached... Though that'd be somewhat hacky...

Note that Loader::LoadStyleLink always passes `false` for `aSyncLoad` to `CreateSheet`. Also, I think it doesn't actually do the load if it doesn't manage to find it...

Something like https://gist.github.com/emilio/ec55c2753fa4d3784df952c3785cdf1d probably does the trick, though it's certainly not great... I'd prefer to come up with a better pattern. Boris knows the loader as well, we could probably talk about how these sheets should work.

For (2) I don't really know, you could try having a script-blocker earlier in CreateSyntheticVideoDocument, but short of debugging it myself I cannot really guess why, sorry :)

Comment 183

[Emilio Cobos Álvarez (:emilio)]

(In reply to Emilio Cobos Álvarez (:emilio) from comment #182)
> Note that Loader::LoadStyleLink always passes `false` for `aSyncLoad` to
> `CreateSheet`. Also, I think it doesn't actually do the load if it doesn't
> manage to find it...

I mean CreateSheet(aSyncLoad = true) doesn't load the link itself. Not sure really about why, there's probably a lot of callers to audit, but making it load the link might be cleaner, then passing aSyncLoad = true if info.mContent->IsInUAWidget(), or something.

Comment 184

[Tim Guan-tin Chien [:timdream] (please needinfo)]

(In reply to Emilio Cobos Álvarez (:emilio) from comment #183)
> I mean CreateSheet(aSyncLoad = true) doesn't load the link itself. Not sure
> really about why, there's probably a lot of callers to audit, but making it
> load the link might be cleaner, then passing aSyncLoad = true if
> info.mContent->IsInUAWidget(), or something.

Yeah, I dug into Loader::LoadStyleLink() and SheetLoadData::mSyncLoad would need to be switched to true explicitly.

I'll ask Boris to review this approach instead.

Comment 185

[Tim Guan-tin Chien [:timdream] (please needinfo)]

Note to self: run patches on Talos; bug 1284877 added an <html:video> in browser.xul.

https://searchfox.org/mozilla-central/rev/f0c15db995198a1013e1c5f5b5bea54ef83f1049/browser/base/content/popup-notifications.inc#32

Comment 186

[Tim Guan-tin Chien [:timdream] (please needinfo)]

Attachment: Bug 1431255 - Part XIII, Make <link> in UA Widget load stylesheets synchronously

Set SheetLoadData::mSyncLoad to true when the link element is in a UA Widget.

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

Comment 187

[Tim Guan-tin Chien [:timdream] (please needinfo)]

Attachment: Bug 1431255 - Part XIV, Move the creation of video element in synthetic media document

This patch moves the call into CreateSyntheticVideoDocument() from
VideoDocument::StartDocumentLoad() to VideoDocument::SetScriptGlobalObject()
because there isn't a inner window at the time of document load,
thus the UAWidgetBindToTree event will not be dispatched
(see EventDispatcher::Dispatch()).

This is modeled after where ImageDocument::CreateSyntheticDocument() is called.
Although it is unclear to me why it was being done differently in bug 448603.

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

Comment 188

[Tim Guan-tin Chien [:timdream] (please needinfo)]

Comment on attachment 8988356 [details]
Bug 1431255 - Part II, Create a Shadow Root in HTMLMediaElement when enabled, skipping <xul:videocontrols>

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/253610/diff/9-10/

Comment 189

[Tim Guan-tin Chien [:timdream] (please needinfo)]

Comment on attachment 8988357 [details]
Bug 1431255 - Part III, Create per-origin sandboxes from XPCJSRuntime and load UA widgets scripts

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/253612/diff/10-11/

Comment 190

[Tim Guan-tin Chien [:timdream] (please needinfo)]

Comment on attachment 8988358 [details]
Bug 1431255 - Part IV, Load videocontrols.js, migrated from videoControls binding

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/253614/diff/11-12/

Comment 191

[Tim Guan-tin Chien [:timdream] (please needinfo)]

Comment on attachment 8989602 [details]
Bug 1431255 - Part V, Set the reflectors of the UA Widget DOM to UA Widget Scope

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/254628/diff/8-9/

Comment 192

[Tim Guan-tin Chien [:timdream] (please needinfo)]

Comment on attachment 8990243 [details]
Bug 1431255 - Part VI, Redirect target in ContextMenu.jsm to HTMLMediaElement

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/255284/diff/7-8/

Comment 193

[Tim Guan-tin Chien [:timdream] (please needinfo)]

Comment on attachment 8991514 [details]
Bug 1431255 - Part VII, Trap mouse/touch/pointer events in audio/video element for UI Widgets

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/256420/diff/4-5/

Comment 194

[Tim Guan-tin Chien [:timdream] (please needinfo)]

Comment on attachment 8990244 [details]
Bug 1431255 - Part VIII, Adjust videocontrols tests to test on UA Widget

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/255286/diff/7-8/

Comment 195

[Tim Guan-tin Chien [:timdream] (please needinfo)]

Comment on attachment 8990872 [details]
Bug 1431255 - Part IX, Remove fullscreenchange workaround for bug 718107

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/255876/diff/6-7/

Comment 196

[Tim Guan-tin Chien [:timdream] (please needinfo)]

Comment on attachment 8990873 [details]
Bug 1431255 - Part X, Remove XBL workaround in bug 462114

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/255878/diff/6-7/

Comment 197

[Tim Guan-tin Chien [:timdream] (please needinfo)]

Comment on attachment 8990874 [details]
Bug 1431255 - Part XI, Remove bogus comments in videocontrols.js

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/255880/diff/6-7/

Comment 198

[Tim Guan-tin Chien [:timdream] (please needinfo)]

Comment on attachment 8998014 [details]
Bug 1431255 - Part XII, Don't set hidden explicitly on animating elements

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

Comment 199

[Tim Guan-tin Chien [:timdream] (please needinfo)]

Try looks good! Finally.

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

Talos is still on-going.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=9e2195206228cc2e9c1e3219631674091904ee33

Comment 200

[Tim Guan-tin Chien [:timdream] (please needinfo)]

Comment on attachment 8988356 [details]
Bug 1431255 - Part II, Create a Shadow Root in HTMLMediaElement when enabled, skipping <xul:videocontrols>

Tests of video element dimensions are added in Part IV in test_audiocontrols_dimensions.html.

Comment 201

[Tim Guan-tin Chien [:timdream] (please needinfo)]

Regressed on Talos :'(

https://treeherder.mozilla.org/perf.html#/compare?originalProject=mozilla-central&originalRevision=37b33c4f58b9&newProject=try&newRevision=9e2195206228cc2e9c1e3219631674091904ee33&framework=1&showOnlyImportant=1

Comment 202

[Steve Fink [:sfink] [:s:]]

Comment on attachment 8988357 [details]
Bug 1431255 - Part III, Create per-origin sandboxes from XPCJSRuntime and load UA widgets scripts

https://reviewboard.mozilla.org/r/253612/#review269112

Looks good to me.

::: js/xpconnect/src/xpcprivate.h:606
(Diff revision 11)
>  
> +    struct Hasher {
> +        typedef RefPtr<mozilla::BasePrincipal> Key;
> +        typedef Key Lookup;
> +        static uint32_t hash(const Lookup& l) {
> +            return l->GetOriginNoSuffixHash();

Why create a new GetOriginNoSuffixHash for just this one case, instead of GetOriginNoSuffix + hash()?

It seems like if you're going to add something to BasePrincipal, then it ought to be an entire BasePrincipal::NoSuffixHasher (along with a BasePrincipal::Hasher that uses the whole origin). But that feels like scope creep for this bug.

::: js/xpconnect/src/xpcprivate.h:622
(Diff revision 11)
> +    };
> +
> +    typedef JS::GCHashMap<RefPtr<mozilla::BasePrincipal>,
> +                          JS::Heap<JSObject*>,
> +                          Hasher,
> +                          js::SystemAllocPolicy,

I thought Gecko generally crashed on OOM instead of propagating errors. I'm fine either way, but do you prefer SystemAllocPolicy or InfallibleAllocPolicy?

https://searchfox.org/mozilla-central/source/memory/mozalloc/mozalloc.h#202

Comment 203

[Brian Grinstead [:bgrins]]

(In reply to Tim Guan-tin Chien [:timdream] (please needinfo) from comment #201)
> Regressed on Talos :'(
> 
> https://treeherder.mozilla.org/perf.html#/compare?originalProject=mozilla-
> central&originalRevision=37b33c4f58b9&newProject=try&newRevision=9e2195206228
> cc2e9c1e3219631674091904ee33&framework=1&showOnlyImportant=1

By the way - I see the base is an m-c rev (with one run on each test). What I usually do to limit noise is to run: `./mach try -b o -p linux64,macosx64,win64 -u none -t all --rebuild-talos 6` once with no patches applied and then once with the patches applied and then compare those revisions. Also, make sure both are non-artifact builds in this case, since comparing across non-artifact and artifact also causes noise.

Comment 204

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

Comment on attachment 8988356 [details]
Bug 1431255 - Part II, Create a Shadow Root in HTMLMediaElement when enabled, skipping <xul:videocontrols>

https://reviewboard.mozilla.org/r/253610/#review269110

r+, but this needs a layout peer review too

::: dom/media/webvtt/vtt.jsm:1021
(Diff revision 10)
> +        // controls is a NAC; The control bar is in a XBL binding.
> -      controlBar = controls.ownerDocument.getAnonymousElementByAttribute(
> +        controlBar = controls.ownerDocument.getAnonymousElementByAttribute(
> -        controls, "anonid", "controlBar");
> +          controls, "anonid", "controlBar");
> +      } else {
> +        // controls is a <div> inside Shadow DOM.
> +        controlBar = controls.querySelector("[anonid=controlBar]");

could you please use faster APIs than querySelector. Like, use id in the shadow content and use shadowroot.getElementById()?

::: layout/generic/nsVideoFrame.cpp:182
(Diff revision 10)
> +{
> +  if (mVideoControls) {
> +    return mVideoControls;
> +  }
> +  if (mContent->GetShadowRoot()) {
> +    return mContent->GetShadowRoot()->GetFirstChild();

this needs a comment. Why firstchild?

::: layout/generic/nsVideoFrame.cpp:385
(Diff revision 10)
>                    borderPadding.left, borderPadding.top, 0, childStatus);
>        MOZ_ASSERT(childStatus.IsFullyComplete(),
>                   "We gave our child unconstrained available block-size, "
>                   "so it should be complete!");
>  
> -      if (child->GetContent() == mVideoControls && isBSizeShrinkWrapping) {
> +      if (child->GetContent() == videoControlsDiv && isBSizeShrinkWrapping) {

all these reflow changes need a layout peer review. It isn't clear to me whether manually dealing with shadow dom this way in reflow is fine.

::: layout/generic/nsVideoFrame.cpp:444
(Diff revision 10)
> + *
> + * When there isn't a ShadowRoot, nsVideoFrame has to be a leaf so web content
> + * element can be ignored.
> + */
> +bool
> +nsVideoFrame::IsLeafDynamic() const

This needs a layout peer review

Comment 205

[Tim Guan-tin Chien [:timdream] (please needinfo)]

(In reply to Brian Grinstead [:bgrins] from comment #203)
> (In reply to Tim Guan-tin Chien [:timdream] (please needinfo) from comment
> #201)
> > Regressed on Talos :'(
> > 
> > https://treeherder.mozilla.org/perf.html#/compare?originalProject=mozilla-
> > central&originalRevision=37b33c4f58b9&newProject=try&newRevision=9e2195206228
> > cc2e9c1e3219631674091904ee33&framework=1&showOnlyImportant=1

I assume this is because of the cost to invoke JS even for <video> without controls. This can kind of being confirmed by the micro benchmark smaug asked me to do (see my next comment). I was hoping to move the logic of selecting controls into videocontrols.js. I added a changeset that will selectively create shadow root and dispatch JS in BindToTree() instead. Let's see the new talos results.

> By the way - I see the base is an m-c rev (with one run on each test). What
> I usually do to limit noise is to run: `./mach try -b o -p
> linux64,macosx64,win64 -u none -t all --rebuild-talos 6` once with no
> patches applied and then once with the patches applied and then compare
> those revisions. Also, make sure both are non-artifact builds in this case,
> since comparing across non-artifact and artifact also causes noise.

Sure, here is the talos run with base changeset running on Try also:

https://treeherder.mozilla.org/perf.html#/compare?originalProject=try&originalRevision=b08485861c30f5859bfc3ed3ae2b36990bce230b&newProject=try&newRevision=557123e84827e3be193d1560351f280820e11a72&framework=1&showOnlyImportant=1

Comment 206

[Tim Guan-tin Chien [:timdream] (please needinfo)]

Attachment: benchmark.html

This is the benchmark I do.

===========

With UA Widget:

appendChild(<video>): 11
appendChild(<video controls>): 10475
appendChild(<video controls>); v.clientWidth: 9878

appendChild(<video>): 11
appendChild(<video controls>): 7098
appendChild(<video controls>); v.clientWidth: 5986

appendChild(<video>): 7
appendChild(<video controls>): 6262
appendChild(<video controls>); v.clientWidth: 6128

============

appendChild(<video>): 5
appendChild(<video controls>): 5
appendChild(<video controls>); v.clientWidth: 8866

appendChild(<video>): 5
appendChild(<video controls>): 6
appendChild(<video controls>); v.clientWidth: 9003

appendChild(<video>): 5
appendChild(<video controls>): 5
appendChild(<video controls>); v.clientWidth: 8943

=============

Clearly, because we'd moved the time to run JS from layout to DOM, |appendChild(<video controls>)| now takes some time. Yet it looks like it's more performant when we do need controls ...

Comment 207

[Tim Guan-tin Chien [:timdream] (please needinfo)]

(In reply to Tim Guan-tin Chien [:timdream] (please needinfo) from comment #206)
> ============
> 
> appendChild(<video>): 5
> appendChild(<video controls>): 5
> appendChild(<video controls>); v.clientWidth: 8866
> 
> appendChild(<video>): 5
> appendChild(<video controls>): 6
> appendChild(<video controls>); v.clientWidth: 9003
> 
> appendChild(<video>): 5
> appendChild(<video controls>): 5
> appendChild(<video controls>); v.clientWidth: 8943

These obviously are numbers with the XUL binding.

Comment 208

[Tim Guan-tin Chien [:timdream] (please needinfo)]

(In reply to Tim Guan-tin Chien [:timdream] (please needinfo) from comment #205)

Fixed logic errors:

https://treeherder.mozilla.org/perf.html#/compare?originalProject=try&originalRevision=b08485861c30f5859bfc3ed3ae2b36990bce230b&newProject=try&newRevision=296ba29e285536228f626f1c8b3220e939ba8075&framework=1&showOnlyImportant=1

Comment 209

[Brian Grinstead [:bgrins]]

(In reply to Tim Guan-tin Chien [:timdream] (please needinfo) from comment #185)
> Note to self: run patches on Talos; bug 1284877 added an <html:video> in
> browser.xul.
> 
> https://searchfox.org/mozilla-central/rev/
> f0c15db995198a1013e1c5f5b5bea54ef83f1049/browser/base/content/popup-
> notifications.inc#32

If the html:video in browser.xul doesn't have controls (and it appears it doesn't if we are talking about https://searchfox.org/mozilla-central/rev/aff5d4ad5d7fb2919d267cbc23b1d87ae3cf0110/browser/base/content/popup-notifications.inc#32) then it shouldn't be affected by the change, at least based on the numbers in Comment 206, right?

Comment 210

[Tim Guan-tin Chien [:timdream] (please needinfo)]

(In reply to Brian Grinstead [:bgrins] from comment #209)
> If the html:video in browser.xul doesn't have controls (and it appears it
> doesn't if we are talking about
> https://searchfox.org/mozilla-central/rev/
> aff5d4ad5d7fb2919d267cbc23b1d87ae3cf0110/browser/base/content/popup-
> notifications.inc#32) then it shouldn't be affected by the change, at least
> based on the numbers in Comment 206, right?

It would affect the patch here but not the WIP I on Try which only lazily attaches shadow root (I hope).

Comment 211

[Tim Guan-tin Chien [:timdream] (please needinfo)]

I tried to pref-off the feature and run Talos again, and this is the results:

https://treeherder.mozilla.org/perf.html#/compare?originalProject=try&originalRevision=b08485861c30f5859bfc3ed3ae2b36990bce230b&newProject=try&newRevision=3677654d184a&framework=1&showOnlyImportant=1

I would need to profile these seemly unrelated tests to tell what slow them down (other than the <video> element itself...)

Comment 212

[Tim Guan-tin Chien [:timdream] (please needinfo)]

Hum, I don't think I can make sense of the profiler result. Comparing two rasterflood_gradient pgo e10s stylo linux64 runs on the disabled patch and the based changeset on central shows a difference, not on the binding code but native calls in Performance::NowUnclamped(). This is not even what the test is testing for.

https://perf-html.io/from-url/https%3A%2F%2Fqueue.taskcluster.net%2Fv1%2Ftask%2FRClCH--gS2aZE5xx-jqY7g%2Fruns%2F0%2Fartifacts%2Fpublic%2Ftest_info%2Fprofile_rasterflood_gradient.zip/flame-graph/?file=profile_rasterflood_gradient%2Fbenchmarks_pagecycle_2%2Fcycle_0.profile&globalTrackOrder=0-1-2-3&hiddenGlobalTracks=1-2&localTrackOrderByPid=25514-0~&thread=4&v=3

https://perf-html.io/from-url/https%3A%2F%2Fqueue.taskcluster.net%2Fv1%2Ftask%2FVlQgbAFQRMqhvqom08iFEw%2Fruns%2F0%2Fartifacts%2Fpublic%2Ftest_info%2Fprofile_rasterflood_gradient.zip/flame-graph/?file=profile_rasterflood_gradient%2Fbenchmarks_pagecycle_2%2Fcycle_0.profile&globalTrackOrder=0-1-2-3&hiddenGlobalTracks=1-2&localTrackOrderByPid=1004-0~&thread=4&v=3

https://treeherder.mozilla.org/#/jobs?repo=try&revision=68d6c978f7754282c2d8e9de430fad056c8ea0dc
https://hg.mozilla.org/try/rev/d63beeb779ccbd1fb6716b67ddbc8568bfaa3fef

Comment 213

[Brian Grinstead [:bgrins]]

(In reply to Tim Guan-tin Chien [:timdream] (please needinfo) from comment #212)
> Hum, I don't think I can make sense of the profiler result. Comparing two
> rasterflood_gradient pgo e10s stylo linux64 runs on the disabled patch and
> the based changeset on central shows a difference, not on the binding code
> but native calls in Performance::NowUnclamped(). This is not even what the
> test is testing for.

That could well be noise - I think I've seen that test show up on a lot on unrelated work. Would be worth doing another compare and see if it still shows up.

Comment 214

[Bobby Holley (:bholley)]

Yeah, the rasterflood change looks like noise.

First, if you ever have an interesting talos result, the first thing to do is to retrigger it to at least 10 runs for both new and baseline. 5 isn't really enough.

Second, click the graph link on Talos Compare. In this case, you'll see that there's a bimodal distribution, and the new run just happened to get more samples in the higher bucket.

I've gone ahead and done some retriggers so that you can double-check, but I think you're fine.

Comment 215

[Tim Guan-tin Chien [:timdream] (please needinfo)]

OK, that's a relief. I am retriggering this test case too to better filtering out the noise.

(In reply to Tim Guan-tin Chien [:timdream] (please needinfo) from comment #208)
> https://treeherder.mozilla.org/perf.html#/
> compare?originalProject=try&originalRevision=b08485861c30f5859bfc3ed3ae2b3699
> 0bce230b&newProject=try&newRevision=296ba29e285536228f626f1c8b3220e939ba8075&
> framework=1&showOnlyImportant=1

Comment 216

[Tim Guan-tin Chien [:timdream] (please needinfo)]

Comment on attachment 8988356 [details]
Bug 1431255 - Part II, Create a Shadow Root in HTMLMediaElement when enabled, skipping <xul:videocontrols>

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/253610/diff/10-11/

Comment 217

[Tim Guan-tin Chien [:timdream] (please needinfo)]

Comment on attachment 8988357 [details]
Bug 1431255 - Part III, Create per-origin sandboxes from XPCJSRuntime and load UA widgets scripts

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/253612/diff/11-12/

Comment 218

[Tim Guan-tin Chien [:timdream] (please needinfo)]

Comment on attachment 8988358 [details]
Bug 1431255 - Part IV, Load videocontrols.js, migrated from videoControls binding

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/253614/diff/12-13/

Comment 219

[Tim Guan-tin Chien [:timdream] (please needinfo)]

Comment on attachment 8989602 [details]
Bug 1431255 - Part V, Set the reflectors of the UA Widget DOM to UA Widget Scope

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/254628/diff/9-10/

Comment 220

[Tim Guan-tin Chien [:timdream] (please needinfo)]

Comment on attachment 8990243 [details]
Bug 1431255 - Part VI, Redirect target in ContextMenu.jsm to HTMLMediaElement

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/255284/diff/8-9/

Comment 221

[Tim Guan-tin Chien [:timdream] (please needinfo)]

Comment on attachment 8991514 [details]
Bug 1431255 - Part VII, Trap mouse/touch/pointer events in audio/video element for UI Widgets

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/256420/diff/5-6/

Comment 222

[Tim Guan-tin Chien [:timdream] (please needinfo)]

Comment on attachment 8990244 [details]
Bug 1431255 - Part VIII, Adjust videocontrols tests to test on UA Widget

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/255286/diff/8-9/

Comment 223

[Tim Guan-tin Chien [:timdream] (please needinfo)]

Comment on attachment 8990872 [details]
Bug 1431255 - Part IX, Remove fullscreenchange workaround for bug 718107

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/255876/diff/7-8/

Comment 224

[Tim Guan-tin Chien [:timdream] (please needinfo)]

Comment on attachment 8990873 [details]
Bug 1431255 - Part X, Remove XBL workaround in bug 462114

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/255878/diff/7-8/

Comment 225

[Tim Guan-tin Chien [:timdream] (please needinfo)]

Comment on attachment 8990874 [details]
Bug 1431255 - Part XI, Remove bogus comments in videocontrols.js

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/255880/diff/7-8/

Comment 226

[Tim Guan-tin Chien [:timdream] (please needinfo)]

Comment on attachment 8998014 [details]
Bug 1431255 - Part XII, Don't set hidden explicitly on animating elements

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

Comment 227

[Tim Guan-tin Chien [:timdream] (please needinfo)]

Comment on attachment 8998899 [details]
Bug 1431255 - Part XIII, Make <link> in UA Widget load stylesheets synchronously

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

Comment 228

[Tim Guan-tin Chien [:timdream] (please needinfo)]

Comment on attachment 8998900 [details]
Bug 1431255 - Part XIV, Move the creation of video element in synthetic media document

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

Comment 229

[Tim Guan-tin Chien [:timdream] (please needinfo)]

Comment on attachment 8988356 [details]
Bug 1431255 - Part II, Create a Shadow Root in HTMLMediaElement when enabled, skipping <xul:videocontrols>

:smaug, could you do a quick re-review? I made a change that creates the Shadow Root and calls into JS lazily (i.e. only if the site sets the controls attribute). This has reduced our performance regressions by a lot (I am making sure other numbers on Talos are just noise right now).

Comment 230

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

Comment on attachment 8988356 [details]
Bug 1431255 - Part II, Create a Shadow Root in HTMLMediaElement when enabled, skipping <xul:videocontrols>

https://reviewboard.mozilla.org/r/253610/#review269396

::: dom/html/HTMLMediaElement.cpp:4511
(Diff revisions 10 - 11)
>        if (mDecoder) {
>          mDecoder->SetLooping(!!aValue);
>        }
> -    } else if (aName == nsGkAtoms::controls) {
> +    } else if (nsContentUtils::IsUAWidgetEnabled() &&
> +               aName == nsGkAtoms::controls &&
> +               GetParentNode()) {

What is GetParentNode() check about?
I assume you mean something like IsInComposedDoc()

::: dom/html/HTMLMediaElement.cpp:4949
(Diff revisions 10 - 11)
> +
> +  // Add a closed shadow root to host video controls
> +  RefPtr<ShadowRoot> shadowRoot =
> +    AttachShadowWithoutNameChecks(ShadowRootMode::Closed);
> +  shadowRoot->SetIsComposedDocParticipant(IsInComposedDoc());
> +  SetShadowRoot(shadowRoot);

Why you need SetShadowRoot call? AttachShadowWithoutNameChecks does that already.
Same with SetIsComposedDocParticipant

Comment 231

[Daniel Holbert [:dholbert]]

Comment on attachment 8988356 [details]
Bug 1431255 - Part II, Create a Shadow Root in HTMLMediaElement when enabled, skipping <xul:videocontrols>

https://reviewboard.mozilla.org/r/253610/#review269388

I basically just looked at the changes within /layout -- I assume that's fine. :)

r=me with nits addressed.

::: dom/html/HTMLMediaElement.cpp:4578
(Diff revision 11)
> +                               ChromeOnlyDispatch::eYes);
> +    dispatcher->RunDOMEventWhenSafe();
> +#else
> +    // We don't want to construct Shadow Root and call into JS,
> +    // if the website never asks for native video controls.
> +    // If controls attribute is set later, Shadow Root and controls are 

nit: EOL whitespace

::: layout/generic/nsVideoFrame.cpp:401
(Diff revision 11)
> +      MOZ_ASSERT(false,
> +                 "Extra child frame found in nsVideoFrame. "
> +                 "Please remove whitespace around the videocontrols container element.");

Two nits:
 (1) Use MOZ_ASSERT_UNREACHABLE here, for a more declarative / human-readable MOZ_ASSERT(false,...).

 (2) The message is perhaps a little too presumptive that potential assertion-failures here must've been caused by whitespace. Maybe get rid of the last line here, or soften it a bit (E.g. phrase as "possibly from stray whitespace [etc]")

::: layout/generic/nsVideoFrame.cpp:436
(Diff revision 11)
> + * nsVideoFrame should be a non-leaf frame when UA Widget is enabled,
> + * so the videocontrols frame inserted in the Shadow DOM can be picked up.

I think you meant s/videocontrols frame/videocontrols element/, right?

(This distinction matters because the point of IsLeaf / IsLeafDynamic is to let us know whether to *construct frames*, I think. So: once we have a "videocontrols frame", this function isn't relevant anymore.)

::: layout/generic/nsVideoFrame.cpp:438
(Diff revision 11)
> + * Other elements from the web content is ignored because there isn't a
> + * <slot/>.

s/is ignored/won't generate child frames/

Maybe elaborate slightly after "There isn't a <slot>" -- e.g. add "...in our videocontrols webcomponent implementation" or something like that (whatever would be accurate here).

Comment 232

[Tim Guan-tin Chien [:timdream] (please needinfo)]

Comment on attachment 8988356 [details]
Bug 1431255 - Part II, Create a Shadow Root in HTMLMediaElement when enabled, skipping <xul:videocontrols>

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/253610/diff/11-12/

Comment 233

[Tim Guan-tin Chien [:timdream] (please needinfo)]

Comment on attachment 8988357 [details]
Bug 1431255 - Part III, Create per-origin sandboxes from XPCJSRuntime and load UA widgets scripts

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/253612/diff/12-13/

Comment 234

[Tim Guan-tin Chien [:timdream] (please needinfo)]

Comment on attachment 8988358 [details]
Bug 1431255 - Part IV, Load videocontrols.js, migrated from videoControls binding

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/253614/diff/13-14/

Comment 235

[Tim Guan-tin Chien [:timdream] (please needinfo)]

Comment on attachment 8989602 [details]
Bug 1431255 - Part V, Set the reflectors of the UA Widget DOM to UA Widget Scope

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/254628/diff/10-11/

Comment 236

[Tim Guan-tin Chien [:timdream] (please needinfo)]

Comment on attachment 8990243 [details]
Bug 1431255 - Part VI, Redirect target in ContextMenu.jsm to HTMLMediaElement

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/255284/diff/9-10/

Comment 237

[Tim Guan-tin Chien [:timdream] (please needinfo)]

Comment on attachment 8991514 [details]
Bug 1431255 - Part VII, Trap mouse/touch/pointer events in audio/video element for UI Widgets

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/256420/diff/6-7/

Comment 238

[Tim Guan-tin Chien [:timdream] (please needinfo)]

Comment on attachment 8990244 [details]
Bug 1431255 - Part VIII, Adjust videocontrols tests to test on UA Widget

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/255286/diff/9-10/

Comment 239

[Tim Guan-tin Chien [:timdream] (please needinfo)]

Comment on attachment 8990872 [details]
Bug 1431255 - Part IX, Remove fullscreenchange workaround for bug 718107

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/255876/diff/8-9/

Comment 240

[Tim Guan-tin Chien [:timdream] (please needinfo)]

Comment on attachment 8990873 [details]
Bug 1431255 - Part X, Remove XBL workaround in bug 462114

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/255878/diff/8-9/

Comment 241

[Tim Guan-tin Chien [:timdream] (please needinfo)]

Comment on attachment 8990874 [details]
Bug 1431255 - Part XI, Remove bogus comments in videocontrols.js

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/255880/diff/8-9/

Comment 242

[Tim Guan-tin Chien [:timdream] (please needinfo)]

Comment on attachment 8998014 [details]
Bug 1431255 - Part XII, Don't set hidden explicitly on animating elements

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

Comment 243

[Tim Guan-tin Chien [:timdream] (please needinfo)]

Comment on attachment 8998899 [details]
Bug 1431255 - Part XIII, Make <link> in UA Widget load stylesheets synchronously

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

Comment 244

[Tim Guan-tin Chien [:timdream] (please needinfo)]

Comment on attachment 8998900 [details]
Bug 1431255 - Part XIV, Move the creation of video element in synthetic media document

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

Comment 245

[ExE Boss]

Comment on attachment 8988356 [details]
Bug 1431255 - Part II, Create a Shadow Root in HTMLMediaElement when enabled, skipping <xul:videocontrols>

https://reviewboard.mozilla.org/r/253610/#review269450

I have two grammar related nitpicks:

::: layout/generic/nsVideoFrame.cpp:437
(Diff revisions 11 - 12)
>  }
>  
>  /**
>   * nsVideoFrame should be a non-leaf frame when UA Widget is enabled,
> - * so the videocontrols frame inserted in the Shadow DOM can be picked up.
> - * Other elements from the web content is ignored because there isn't a
> + * so the videocontrols container element inserted under the Shadow Root can be
> + * picked up. Other elements from the web content is ignored because there isn't

s/is ignored/are ignored/

::: layout/generic/nsVideoFrame.cpp:441
(Diff revisions 11 - 12)
> - * so the videocontrols frame inserted in the Shadow DOM can be picked up.
> - * Other elements from the web content is ignored because there isn't a
> - * <slot/>.
> + * so the videocontrols container element inserted under the Shadow Root can be
> + * picked up. Other elements from the web content is ignored because there isn't
> + * a <slot/> in our videocontrol Shadow DOM.
>   *
> - * When there isn't a ShadowRoot, nsVideoFrame has to be a leaf so web content
> - * element can be ignored.
> + * When the UA Widget is disabled, i.e. the videocontrols is bound as anonymous
> + * contents with XBL, nsVideoFrame has to be a leaf so web content element can

s/contents/content/

Comment 246

[ExE Boss]

(In reply to ExE Boss from comment #245)
> Comment on attachment 8988356 [details]
> Bug 1431255 - Part II, Create a Shadow Root in HTMLMediaElement when
> enabled, skipping <xul:videocontrols>
> 
> https://reviewboard.mozilla.org/r/253610/#review269450
> 
> I have two grammar related nitpicks:
> 
> ::: layout/generic/nsVideoFrame.cpp:437
> (Diff revisions 11 - 12)
> >  }
> >  
> >  /**
> >   * nsVideoFrame should be a non-leaf frame when UA Widget is enabled,
> > - * so the videocontrols frame inserted in the Shadow DOM can be picked up.
> > - * Other elements from the web content is ignored because there isn't a
> > + * so the videocontrols container element inserted under the Shadow Root can be
> > + * picked up. Other elements from the web content is ignored because there isn't
> 
> s/is ignored/are ignored/

Also, s/the web content/web content/.

Comment 247

[Daniel Holbert [:dholbert]]

Comment on attachment 8988356 [details]
Bug 1431255 - Part II, Create a Shadow Root in HTMLMediaElement when enabled, skipping <xul:videocontrols>

https://reviewboard.mozilla.org/r/253610/#review269450

> s/is ignored/are ignored/

(per bottom of https://bugzilla.mozilla.org/show_bug.cgi?id=1431255#c231 , I think the wording should change here anyway; "ignored" seems a bit too vague about what we're getting at here [generating vs. not-generating child frames])

Comment 248

[Jared Wein [:jaws] (please needinfo? me)]

Comment on attachment 8988357 [details]
Bug 1431255 - Part III, Create per-origin sandboxes from XPCJSRuntime and load UA widgets scripts

https://reviewboard.mozilla.org/r/253612/#review269464

Comment 249

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 8988357 [details]
Bug 1431255 - Part III, Create per-origin sandboxes from XPCJSRuntime and load UA widgets scripts

https://reviewboard.mozilla.org/r/253612/#review269466

::: browser/base/content/tab-content.js:532
(Diff revision 13)
>  addMessageListener("DOM:WebManifest:hasManifestLink", ManifestMessages);
>  addMessageListener("DOM:ManifestObtainer:Obtain", ManifestMessages);
>  addMessageListener("DOM:Manifest:FireAppInstalledEvent", ManifestMessages);
>  addMessageListener("DOM:WebManifest:fetchIcon", ManifestMessages);
> +
> +class UAWidgets {

Sorry for the unsolicited feedback - we might want to shift this over to using the Actor model that kmag just landed on inbound in bug 1472491 to avoid the memory / processing overhead of adding this to the frame script.

Comment 250

[Jared Wein [:jaws] (please needinfo? me)]

Comment on attachment 8988358 [details]
Bug 1431255 - Part IV, Load videocontrols.js, migrated from videoControls binding

https://reviewboard.mozilla.org/r/253614/#review269468

::: toolkit/content/widgets/videocontrols.js:1885
(Diff revision 14)
> -        this.castingButton = document.getAnonymousElementByAttribute(binding, "anonid", "castingButton");
> -        this.closedCaptionButton = document.getAnonymousElementByAttribute(binding, "anonid", "closedCaptionButton");
> -        this.textTrackList = document.getAnonymousElementByAttribute(binding, "anonid", "textTrackList");
> +        this.controlsSpacer     = this.shadowRoot.getElementById("controlsSpacer");
> +        this.clickToPlay        = this.shadowRoot.getElementById("clickToPlay");
> +        this.fullscreenButton   = this.shadowRoot.getElementById("fullscreenButton");

nit, can you update these to only have one space before the equal sign? The alignment here hasn't scaled well.

::: toolkit/themes/shared/media/videocontrols.css:8
(Diff revision 14)
>  video > xul|videocontrols,
> -audio > xul|videocontrols {
> +audio > xul|videocontrols,

Can we drop these two selectors or do you have a follow-up to do so?

Comment 251

[Jared Wein [:jaws] (please needinfo? me)]

Comment on attachment 8990244 [details]
Bug 1431255 - Part VIII, Adjust videocontrols tests to test on UA Widget

https://reviewboard.mozilla.org/r/255286/#review269474

Comment 252

[Jared Wein [:jaws] (please needinfo? me)]

Comment on attachment 8990872 [details]
Bug 1431255 - Part IX, Remove fullscreenchange workaround for bug 718107

https://reviewboard.mozilla.org/r/255876/#review269482

Comment 253

[Jared Wein [:jaws] (please needinfo? me)]

Comment on attachment 8990873 [details]
Bug 1431255 - Part X, Remove XBL workaround in bug 462114

https://reviewboard.mozilla.org/r/255878/#review269484

Thank you for cleaning these workarounds up!

Comment 254

[Jared Wein [:jaws] (please needinfo? me)]

Comment on attachment 8990874 [details]
Bug 1431255 - Part XI, Remove bogus comments in videocontrols.js

https://reviewboard.mozilla.org/r/255880/#review269486

Comment 255

[Jared Wein [:jaws] (please needinfo? me)]

Comment on attachment 8998014 [details]
Bug 1431255 - Part XII, Don't set hidden explicitly on animating elements

https://reviewboard.mozilla.org/r/257662/#review269488

::: commit-message-0a7c2:10
(Diff revision 4)
> +cancelled.
> +
> +Additionally, considers elements with "fadeout" class as hidden,
> +so other states can be toggled correctly.
> +
> +This patch fixes intermittent failures on slower platforms, and perma-orange

Which perma-orange does this fix? Can we re-enable some test on Windows?

Comment 256

[Tim Guan-tin Chien [:timdream] (please needinfo)]

(In reply to Mike Conley (:mconley) (:⚙️) from comment #249)
> > +class UAWidgets {
> 
> Sorry for the unsolicited feedback - we might want to shift this over to
> using the Actor model that kmag just landed on inbound in bug 1472491 to
> avoid the memory / processing overhead of adding this to the frame script.

Oh I didn't realize it is in the inbound already. I'll rebase over that in this bug or in a follow up.

(In reply to Jared Wein [:jaws] (please needinfo? me) from comment #250)
> ::: toolkit/themes/shared/media/videocontrols.css:8
> (Diff revision 14)
> >  video > xul|videocontrols,
> > -audio > xul|videocontrols {
> > +audio > xul|videocontrols,
> 
> Can we drop these two selectors or do you have a follow-up to do so?

They are still needed by the XUL binding. We'll remove them once the XUL binding is removed.

(In reply to Jared Wein [:jaws] (please needinfo? me) from comment #255)
> > +This patch fixes intermittent failures on slower platforms, and perma-orange
> 
> Which perma-orange does this fix? Can we re-enable some test on Windows?

I didn't disable any tests, just that without this patch the UA Widget won't pass on Try.

Comment 257

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8998899 [details]
Bug 1431255 - Part XIII, Make <link> in UA Widget load stylesheets synchronously

https://reviewboard.mozilla.org/r/261656/#review269492

r-, but I think with the comments below addressed this will be good to go.  I just want to make sure I look at the result.

I have reviews turned off, but please just needinfo me when the patch is updated.

::: layout/style/Loader.cpp:1961
(Diff revision 3)
>    nsINode* context = aInfo.mContent;
>    if (!context) {
>      context = mDocument;
>    }
>  
> +  bool syncLoad = aInfo.mContent && aInfo.mContent->IsInUAWidget();

I don't think I'm OK with doing this for all URLs.  If someone puts a `<link>` in a UA widget with an http:// URL, we do _not_ want to be syncloading that.

The XBL resource loader restricts the sync behavior to IsChromeURI() URIs and I believe we should as well.

::: layout/style/Loader.cpp:2038
(Diff revision 3)
>                                            principal,
>                                            context);
>    NS_ADDREF(data);
>  
> +  if (syncLoad) {
> +    data->mSyncLoad = true;

I would much prefer it if we added an aSyncLoad argument to the relevant SheetLoadData constructor.

In the process of doing that, we would run into an interesting question: what should happen for `<style>` inside a UA widget?  Should `@import` from inside it be syncloaded?  With this patch as-is they would not be.  I think I'm OK with that, but it might end up being a bit of a footgun.

::: layout/style/Loader.cpp:2057
(Diff revision 3)
>                                                       data->mLoaderPrincipal,
>                                                       data->mSheet->GetCORSMode(),
>                                                       data->mSheet->GetReferrerPolicy());
>      mSheets->mPendingDatas.Put(&key, data);
>  
>      data->mMustNotify = true;

Setting mMustNotify to true in the syncload case is a bit odd.

Of course deferring the load like this in the syncload case is a bit odd.  Should we be skipping this whole block in the syncload case?  Or not doing a syncload if the conditions of this block hold?

::: layout/style/Loader.cpp:2067
(Diff revision 3)
>    rv = LoadSheet(data, state, false);
>    if (NS_FAILED(rv)) {
>      return Err(rv);
>    }
>  
>    data->mMustNotify = true;

Again, that's wrong in the syncload case.  For one thing, in the syncload case "data" might be a dangling pointer by now, as far as I can tell.  We should really come up with a better ownership model for this code... :(  Maybe file a followup on that?

Anyway, we should skip this set if we're doing a syncload.  And probably assert that we never syncload when aObserver is non-null.  I assume these loads are never coming from the parser, right?

::: layout/style/SheetLoadData.h:128
(Diff revision 3)
>  
>    // Number of sheets we @import-ed that are still loading
>    uint32_t mPendingChildren;
>  
> -  // mSyncLoad is true when the load needs to be synchronous -- right
> -  // now only for LoadSheetSync and children of sync loads.
> +  // mSyncLoad is true when the load needs to be synchronous.
> +  // For LoadSheetSync, <link> in UA Widgets, and children of sync loads.

This comment should probably reflect that only some `<link>` in UA widgets are syncloaded, per comments above.

Comment 258

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8998900 [details]
Bug 1431255 - Part XIV, Move the creation of video element in synthetic media document

https://reviewboard.mozilla.org/r/261658/#review269494

This looks just like bug 1261536 (well, with a bit less cleanup and the update to use InitialSetupHasBeenDone() in the condition instead of testing for the root element).  That patch got backed out for causing intermittent orange in the videocontrols tests.

I would much prefer that we just reland this in bug 1261536, before landing the rest of the patches in this bug, to reduce risk...

r=me I guess, since the patch in bug 1261536 already has r=me.

Comment 259

[Tim Guan-tin Chien [:timdream] (please needinfo)]

Comment on attachment 8988356 [details]
Bug 1431255 - Part II, Create a Shadow Root in HTMLMediaElement when enabled, skipping <xul:videocontrols>

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/253610/diff/12-13/

Comment 260

[Tim Guan-tin Chien [:timdream] (please needinfo)]

Comment on attachment 8988357 [details]
Bug 1431255 - Part III, Create per-origin sandboxes from XPCJSRuntime and load UA widgets scripts

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/253612/diff/13-14/

Comment 261

[Tim Guan-tin Chien [:timdream] (please needinfo)]

Comment on attachment 8988358 [details]
Bug 1431255 - Part IV, Load videocontrols.js, migrated from videoControls binding

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/253614/diff/14-15/

Comment 262

[Tim Guan-tin Chien [:timdream] (please needinfo)]

Comment on attachment 8989602 [details]
Bug 1431255 - Part V, Set the reflectors of the UA Widget DOM to UA Widget Scope

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/254628/diff/11-12/

Comment 263

[Tim Guan-tin Chien [:timdream] (please needinfo)]

Comment on attachment 8990243 [details]
Bug 1431255 - Part VI, Redirect target in ContextMenu.jsm to HTMLMediaElement

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/255284/diff/10-11/

Comment 264

[Tim Guan-tin Chien [:timdream] (please needinfo)]

Comment on attachment 8991514 [details]
Bug 1431255 - Part VII, Trap mouse/touch/pointer events in audio/video element for UI Widgets

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/256420/diff/7-8/

Comment 265

[Tim Guan-tin Chien [:timdream] (please needinfo)]

Comment on attachment 8990244 [details]
Bug 1431255 - Part VIII, Adjust videocontrols tests to test on UA Widget

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/255286/diff/10-11/

Comment 266

[Tim Guan-tin Chien [:timdream] (please needinfo)]

Comment on attachment 8990872 [details]
Bug 1431255 - Part IX, Remove fullscreenchange workaround for bug 718107

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/255876/diff/9-10/

Comment 267

[Tim Guan-tin Chien [:timdream] (please needinfo)]

Comment on attachment 8990873 [details]
Bug 1431255 - Part X, Remove XBL workaround in bug 462114

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/255878/diff/9-10/

Comment 268

[Tim Guan-tin Chien [:timdream] (please needinfo)]

Comment on attachment 8990874 [details]
Bug 1431255 - Part XI, Remove bogus comments in videocontrols.js

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/255880/diff/9-10/

Comment 269

[Tim Guan-tin Chien [:timdream] (please needinfo)]

Comment on attachment 8998014 [details]
Bug 1431255 - Part XII, Don't set hidden explicitly on animating elements

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/257662/diff/4-5/

Comment 270

[Tim Guan-tin Chien [:timdream] (please needinfo)]

Comment on attachment 8998899 [details]
Bug 1431255 - Part XIII, Make <link> in UA Widget load stylesheets synchronously

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

Comment 271

[Tim Guan-tin Chien [:timdream] (please needinfo)]

(In reply to Boris Zbarsky [:bzbarsky, bz on IRC] (vacation Aug 16-27) from comment #257)
> I have reviews turned off, but please just needinfo me when the patch is
> updated.

Review comment addressed. Thanks!

Comment 272

[Tim Guan-tin Chien [:timdream] (please needinfo)]

Comment on attachment 8990242 [details]
Bug 1431255 - Part I, Add dom.ua_widget.enabled pref and enable for Desktop Nightly only

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/255282/diff/6-7/

Comment 273

[Tim Guan-tin Chien [:timdream] (please needinfo)]

Comment on attachment 8988356 [details]
Bug 1431255 - Part II, Create a Shadow Root in HTMLMediaElement when enabled, skipping <xul:videocontrols>

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/253610/diff/13-14/

Comment 274

[Tim Guan-tin Chien [:timdream] (please needinfo)]

Comment on attachment 8988357 [details]
Bug 1431255 - Part III, Create per-origin sandboxes from XPCJSRuntime and load UA widgets scripts

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/253612/diff/14-15/

Comment 275

[Tim Guan-tin Chien [:timdream] (please needinfo)]

Comment on attachment 8988358 [details]
Bug 1431255 - Part IV, Load videocontrols.js, migrated from videoControls binding

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/253614/diff/15-16/

Comment 276

[Tim Guan-tin Chien [:timdream] (please needinfo)]

Comment on attachment 8989602 [details]
Bug 1431255 - Part V, Set the reflectors of the UA Widget DOM to UA Widget Scope

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/254628/diff/12-13/

Comment 277

[Tim Guan-tin Chien [:timdream] (please needinfo)]

Comment on attachment 8990243 [details]
Bug 1431255 - Part VI, Redirect target in ContextMenu.jsm to HTMLMediaElement

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/255284/diff/11-12/

Comment 278

[Tim Guan-tin Chien [:timdream] (please needinfo)]

Comment on attachment 8991514 [details]
Bug 1431255 - Part VII, Trap mouse/touch/pointer events in audio/video element for UI Widgets

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/256420/diff/8-9/

Comment 279

[Tim Guan-tin Chien [:timdream] (please needinfo)]

Comment on attachment 8990244 [details]
Bug 1431255 - Part VIII, Adjust videocontrols tests to test on UA Widget

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/255286/diff/11-12/

Comment 280

[Tim Guan-tin Chien [:timdream] (please needinfo)]

Comment on attachment 8990872 [details]
Bug 1431255 - Part IX, Remove fullscreenchange workaround for bug 718107

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/255876/diff/10-11/

Comment 281

[Tim Guan-tin Chien [:timdream] (please needinfo)]

Comment on attachment 8990873 [details]
Bug 1431255 - Part X, Remove XBL workaround in bug 462114

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/255878/diff/10-11/

Comment 282

[Tim Guan-tin Chien [:timdream] (please needinfo)]

Comment on attachment 8990874 [details]
Bug 1431255 - Part XI, Remove bogus comments in videocontrols.js

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/255880/diff/10-11/

Comment 283

[Tim Guan-tin Chien [:timdream] (please needinfo)]

Comment on attachment 8998014 [details]
Bug 1431255 - Part XII, Don't set hidden explicitly on animating elements

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/257662/diff/5-6/

Comment 284

[Tim Guan-tin Chien [:timdream] (please needinfo)]

Comment on attachment 8998899 [details]
Bug 1431255 - Part XIII, Make <link> in UA Widget load stylesheets synchronously

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/261656/diff/4-5/

Comment 285

[Tim Guan-tin Chien [:timdream] (please needinfo)]

Comment on attachment 8988356 [details]
Bug 1431255 - Part II, Create a Shadow Root in HTMLMediaElement when enabled, skipping <xul:videocontrols>

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/253610/diff/14-15/

Comment 286

[Tim Guan-tin Chien [:timdream] (please needinfo)]

Comment on attachment 8988357 [details]
Bug 1431255 - Part III, Create per-origin sandboxes from XPCJSRuntime and load UA widgets scripts

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/253612/diff/15-16/

Comment 287

[Tim Guan-tin Chien [:timdream] (please needinfo)]

Comment on attachment 8988358 [details]
Bug 1431255 - Part IV, Load videocontrols.js, migrated from videoControls binding

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/253614/diff/16-17/

Comment 288

[Tim Guan-tin Chien [:timdream] (please needinfo)]

Comment on attachment 8989602 [details]
Bug 1431255 - Part V, Set the reflectors of the UA Widget DOM to UA Widget Scope

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/254628/diff/13-14/

Comment 289

[Tim Guan-tin Chien [:timdream] (please needinfo)]

Comment on attachment 8990243 [details]
Bug 1431255 - Part VI, Redirect target in ContextMenu.jsm to HTMLMediaElement

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/255284/diff/12-13/

Comment 290

[Tim Guan-tin Chien [:timdream] (please needinfo)]

Comment on attachment 8991514 [details]
Bug 1431255 - Part VII, Trap mouse/touch/pointer events in audio/video element for UI Widgets

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/256420/diff/9-10/

Comment 291

[Tim Guan-tin Chien [:timdream] (please needinfo)]

Comment on attachment 8990244 [details]
Bug 1431255 - Part VIII, Adjust videocontrols tests to test on UA Widget

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/255286/diff/12-13/

Comment 292

[Tim Guan-tin Chien [:timdream] (please needinfo)]

Comment on attachment 8990872 [details]
Bug 1431255 - Part IX, Remove fullscreenchange workaround for bug 718107

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/255876/diff/11-12/

Comment 293

[Tim Guan-tin Chien [:timdream] (please needinfo)]

Comment on attachment 8990873 [details]
Bug 1431255 - Part X, Remove XBL workaround in bug 462114

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/255878/diff/11-12/

Comment 294

[Tim Guan-tin Chien [:timdream] (please needinfo)]

Comment on attachment 8990874 [details]
Bug 1431255 - Part XI, Remove bogus comments in videocontrols.js

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/255880/diff/11-12/

Comment 295

[Tim Guan-tin Chien [:timdream] (please needinfo)]

Comment on attachment 8998014 [details]
Bug 1431255 - Part XII, Don't set hidden explicitly on animating elements

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/257662/diff/6-7/

Comment 296

[Tim Guan-tin Chien [:timdream] (please needinfo)]

Comment on attachment 8998899 [details]
Bug 1431255 - Part XIII, Make <link> in UA Widget load stylesheets synchronously

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/261656/diff/5-6/

Comment 297

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8998899 [details]
Bug 1431255 - Part XIII, Make <link> in UA Widget load stylesheets synchronously

https://reviewboard.mozilla.org/r/261656/#review269530

Thank you!

::: layout/style/Loader.cpp:1967
(Diff revisions 3 - 6)
>    }
>  
> -  bool syncLoad = aInfo.mContent && aInfo.mContent->IsInUAWidget();
> +  bool syncLoad = aInfo.mContent &&
> +                  aInfo.mContent->IsInUAWidget() &&
> +                  IsChromeURI(aInfo.mURI);
>    LOG(("  Link sync load: '%s'", syncLoad ? "true" : "false"));

Please do add an a diagnostic assert that if syncLoad is true aObserver is null.

Comment 298

[Tim Guan-tin Chien [:timdream] (please needinfo)]

Latest patch on Talos: there isn't any performance change nor noise.

https://treeherder.mozilla.org/perf.html#/compare?originalProject=try&originalRevision=96e4c7ebbb0b&newProject=try&newRevision=d8365239c096&framework=1&showOnlyImportant=1

Comment 299

[Tim Guan-tin Chien [:timdream] (please needinfo)]

Comment on attachment 8990242 [details]
Bug 1431255 - Part I, Add dom.ua_widget.enabled pref and enable for Desktop Nightly only

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/255282/diff/7-8/

Comment 300

[Tim Guan-tin Chien [:timdream] (please needinfo)]

Comment on attachment 8988356 [details]
Bug 1431255 - Part II, Create a Shadow Root in HTMLMediaElement when enabled, skipping <xul:videocontrols>

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/253610/diff/15-16/

Comment 301

[Tim Guan-tin Chien [:timdream] (please needinfo)]

Comment on attachment 8988357 [details]
Bug 1431255 - Part III, Create per-origin sandboxes from XPCJSRuntime and load UA widgets scripts

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/253612/diff/16-17/

Comment 302

[Tim Guan-tin Chien [:timdream] (please needinfo)]

Comment on attachment 8988358 [details]
Bug 1431255 - Part IV, Load videocontrols.js, migrated from videoControls binding

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/253614/diff/17-18/

Comment 303

[Tim Guan-tin Chien [:timdream] (please needinfo)]

Comment on attachment 8989602 [details]
Bug 1431255 - Part V, Set the reflectors of the UA Widget DOM to UA Widget Scope

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/254628/diff/14-15/

Comment 304

[Tim Guan-tin Chien [:timdream] (please needinfo)]

Comment on attachment 8990243 [details]
Bug 1431255 - Part VI, Redirect target in ContextMenu.jsm to HTMLMediaElement

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/255284/diff/13-14/

Comment 305

[Tim Guan-tin Chien [:timdream] (please needinfo)]

Comment on attachment 8991514 [details]
Bug 1431255 - Part VII, Trap mouse/touch/pointer events in audio/video element for UI Widgets

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/256420/diff/10-11/

Comment 306

[Tim Guan-tin Chien [:timdream] (please needinfo)]

Comment on attachment 8990244 [details]
Bug 1431255 - Part VIII, Adjust videocontrols tests to test on UA Widget

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/255286/diff/13-14/

Comment 307

[Tim Guan-tin Chien [:timdream] (please needinfo)]

Comment on attachment 8990872 [details]
Bug 1431255 - Part IX, Remove fullscreenchange workaround for bug 718107

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/255876/diff/12-13/

Comment 308

[Tim Guan-tin Chien [:timdream] (please needinfo)]

Comment on attachment 8990873 [details]
Bug 1431255 - Part X, Remove XBL workaround in bug 462114

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/255878/diff/12-13/

Comment 309

[Tim Guan-tin Chien [:timdream] (please needinfo)]

Comment on attachment 8990874 [details]
Bug 1431255 - Part XI, Remove bogus comments in videocontrols.js

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/255880/diff/12-13/

Comment 310

[Tim Guan-tin Chien [:timdream] (please needinfo)]

Comment on attachment 8998014 [details]
Bug 1431255 - Part XII, Don't set hidden explicitly on animating elements

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/257662/diff/7-8/

Comment 311

[Tim Guan-tin Chien [:timdream] (please needinfo)]

Comment on attachment 8998899 [details]
Bug 1431255 - Part XIII, Make <link> in UA Widget load stylesheets synchronously

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/261656/diff/6-7/

Comment 312

[Tim Guan-tin Chien [:timdream] (please needinfo)]

Comment on attachment 8990242 [details]
Bug 1431255 - Part I, Add dom.ua_widget.enabled pref and enable for Desktop Nightly only

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/255282/diff/8-9/

Comment 313

[Tim Guan-tin Chien [:timdream] (please needinfo)]

Comment on attachment 8988356 [details]
Bug 1431255 - Part II, Create a Shadow Root in HTMLMediaElement when enabled, skipping <xul:videocontrols>

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/253610/diff/16-17/

Comment 314

[Tim Guan-tin Chien [:timdream] (please needinfo)]

Comment on attachment 8988357 [details]
Bug 1431255 - Part III, Create per-origin sandboxes from XPCJSRuntime and load UA widgets scripts

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/253612/diff/17-18/

Comment 315

[Tim Guan-tin Chien [:timdream] (please needinfo)]

Comment on attachment 8988358 [details]
Bug 1431255 - Part IV, Load videocontrols.js, migrated from videoControls binding

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/253614/diff/18-19/

Comment 316

[Tim Guan-tin Chien [:timdream] (please needinfo)]

Comment on attachment 8989602 [details]
Bug 1431255 - Part V, Set the reflectors of the UA Widget DOM to UA Widget Scope

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/254628/diff/15-16/

Comment 317

[Tim Guan-tin Chien [:timdream] (please needinfo)]

Comment on attachment 8990243 [details]
Bug 1431255 - Part VI, Redirect target in ContextMenu.jsm to HTMLMediaElement

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/255284/diff/14-15/

Comment 318

[Tim Guan-tin Chien [:timdream] (please needinfo)]

Comment on attachment 8991514 [details]
Bug 1431255 - Part VII, Trap mouse/touch/pointer events in audio/video element for UI Widgets

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/256420/diff/11-12/

Comment 319

[Tim Guan-tin Chien [:timdream] (please needinfo)]

Comment on attachment 8990244 [details]
Bug 1431255 - Part VIII, Adjust videocontrols tests to test on UA Widget

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/255286/diff/14-15/

Comment 320

[Tim Guan-tin Chien [:timdream] (please needinfo)]

Comment on attachment 8990872 [details]
Bug 1431255 - Part IX, Remove fullscreenchange workaround for bug 718107

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/255876/diff/13-14/

Comment 321

[Tim Guan-tin Chien [:timdream] (please needinfo)]

Comment on attachment 8990873 [details]
Bug 1431255 - Part X, Remove XBL workaround in bug 462114

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/255878/diff/13-14/

Comment 322

[Tim Guan-tin Chien [:timdream] (please needinfo)]

Comment on attachment 8990874 [details]
Bug 1431255 - Part XI, Remove bogus comments in videocontrols.js

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/255880/diff/13-14/

Comment 323

[Tim Guan-tin Chien [:timdream] (please needinfo)]

Comment on attachment 8998014 [details]
Bug 1431255 - Part XII, Don't set hidden explicitly on animating elements

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/257662/diff/8-9/

Comment 324

[Tim Guan-tin Chien [:timdream] (please needinfo)]

Comment on attachment 8998899 [details]
Bug 1431255 - Part XIII, Make <link> in UA Widget load stylesheets synchronously

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/261656/diff/7-8/

Comment 325

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/d82268ca1463
Part I, Add dom.ua_widget.enabled pref and enable for Desktop Nightly only r=smaug
https://hg.mozilla.org/integration/autoland/rev/cfecf5444baf
Part II, Create a Shadow Root in HTMLMediaElement when enabled, skipping <xul:videocontrols> r=dholbert,smaug
https://hg.mozilla.org/integration/autoland/rev/a0200438265b
Part III, Create per-origin sandboxes from XPCJSRuntime and load UA widgets scripts r=bholley,jaws,sfink
https://hg.mozilla.org/integration/autoland/rev/4ddca5eb06c2
Part IV, Load videocontrols.js, migrated from videoControls binding r=jaws
https://hg.mozilla.org/integration/autoland/rev/74adb43ed112
Part V, Set the reflectors of the UA Widget DOM to UA Widget Scope r=bholley
https://hg.mozilla.org/integration/autoland/rev/0974d8fc7f03
Part VI, Redirect target in ContextMenu.jsm to HTMLMediaElement r=jaws
https://hg.mozilla.org/integration/autoland/rev/06335a11cf2a
Part VII, Trap mouse/touch/pointer events in audio/video element for UI Widgets r=smaug
https://hg.mozilla.org/integration/autoland/rev/b12fb0dc3b49
Part VIII, Adjust videocontrols tests to test on UA Widget r=jaws
https://hg.mozilla.org/integration/autoland/rev/203715029040
Part IX, Remove fullscreenchange workaround for bug 718107 r=jaws
https://hg.mozilla.org/integration/autoland/rev/e907d31a7160
Part X, Remove XBL workaround in bug 462114 r=jaws
https://hg.mozilla.org/integration/autoland/rev/eadb58678b56
Part XI, Remove bogus comments in videocontrols.js r=jaws
https://hg.mozilla.org/integration/autoland/rev/c42039f3ffe7
Part XII, Don't set hidden explicitly on animating elements r=jaws
https://hg.mozilla.org/integration/autoland/rev/dca187f7c72c
Part XIII, Make <link> in UA Widget load stylesheets synchronously r=bzbarsky

Comment 326

[Eliza Balazs [:ebalazs_]]

https://hg.mozilla.org/mozilla-central/rev/d82268ca1463
https://hg.mozilla.org/mozilla-central/rev/cfecf5444baf
https://hg.mozilla.org/mozilla-central/rev/a0200438265b
https://hg.mozilla.org/mozilla-central/rev/4ddca5eb06c2
https://hg.mozilla.org/mozilla-central/rev/74adb43ed112
https://hg.mozilla.org/mozilla-central/rev/0974d8fc7f03
https://hg.mozilla.org/mozilla-central/rev/06335a11cf2a
https://hg.mozilla.org/mozilla-central/rev/b12fb0dc3b49
https://hg.mozilla.org/mozilla-central/rev/203715029040
https://hg.mozilla.org/mozilla-central/rev/e907d31a7160
https://hg.mozilla.org/mozilla-central/rev/eadb58678b56
https://hg.mozilla.org/mozilla-central/rev/c42039f3ffe7
https://hg.mozilla.org/mozilla-central/rev/dca187f7c72c

Comment 327

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/fd0d6079d0d2
disable ua widgets, since they break tests when shadow DOM is enabled, r=bustage, CLOSED TREE

Comment 328

[Tiberius Oros[:tiberius_oros]]

https://hg.mozilla.org/mozilla-central/rev/fd0d6079d0d2

Comment 329

[Tim Guan-tin Chien [:timdream] (please needinfo)]

Let's re-enable the feature in another bug.