Bug 1431255 will land without enabling on Fennec, even with code migrated to videocontrols.js. This can be done by getting UAWidgetsChild.jsm to run in Fennec.

I am actually thinking about moving all the functionality of UAWidgetsChild.jsm to dom/, and to C++. That will save me the trouble figuring out places to put the frame script in each of the browsers. Is that achievable? Is that what we want? Is dom/ the right place to put this? (This is a question for :bholley too but he blocks needinfo for now.)

I, for one, couldn't find the C++ equivalent to Services.scriptloader.loadSubScript().

Wouldn't moving it to toolkit/ and then loading it in toolkit/modules/ActorManagerParent.jsm instead of nsBrowserGlue.js be enough to have it running on Fennec ?

Untested, but it seems to me that it should work, as toolkit modules are loaded on Fennec.

Thanks for the help! I didn't realize we have that setup in toolkit. With that, I guess we will still need a fix for bug 1483657?

This is the working patch. Other than moving the file, I would need to remove |group: "browsers",| from the definition. Apparently, we don't do |ActorManagerChild.attach(this, "browsers");| on Fennec even though its <browser>s has messagemanagergroup="browsers". https://searchfox.org/mozilla-central/rev/71ef4447db179639be9eff4471f32a95423962d7/browser/base/content/tab-content.js#20 I am still not sure if we want to land this.

Actually, I've just seen that the casting button is gone. It would need to be fixed.

(In reply to Tim Guan-tin Chien [:timdream] (please needinfo) from comment #7) > Apparently, we don't do |ActorManagerChild.attach(this, "browsers");| on > Fennec even though its <browser>s has messagemanagergroup="browsers". > > https://searchfox.org/mozilla-central/rev/ > 71ef4447db179639be9eff4471f32a95423962d7/browser/base/content/tab-content. > js#20 Sounds like a bug to me. Kris, is there any reason we don't do ActorManagerChild.attach(this, "browsers") in toolkit instead of browser ?

(In reply to Tim Nguyen :ntim from comment #9) > (In reply to Tim Guan-tin Chien [:timdream] (please needinfo) from comment > #7) > > Apparently, we don't do |ActorManagerChild.attach(this, "browsers");| on > > Fennec even though its <browser>s has messagemanagergroup="browsers". > > > > https://searchfox.org/mozilla-central/rev/ > > 71ef4447db179639be9eff4471f32a95423962d7/browser/base/content/tab-content. > > js#20 > > > Sounds like a bug to me. Kris, is there any reason we don't do > ActorManagerChild.attach(this, "browsers") in toolkit instead of browser ? Because we don't define any actors with that group anywhere outside of browser/, and we don't load any toolkit frame scripts from which it would be appropriate to load those actors.

(In reply to Kris Maglione [:kmag] from comment #10) > Because we don't define any actors with that group anywhere outside of > browser/, and we don't load any toolkit frame scripts from which it would be > appropriate to load those actors. Actually, this is false. https://searchfox.org/mozilla-central/rev/71ef4447db179639be9eff4471f32a95423962d7/toolkit/modules/ActorManagerParent.jsm#181 ManifestMessagesChild.jsm in dom/ is defined here in toolkit. That's why I thought it would work in the first place. Perhaps we should move it.

(In reply to Tim Guan-tin Chien [:timdream] (please needinfo) from comment #11) > (In reply to Kris Maglione [:kmag] from comment #10) > > Because we don't define any actors with that group anywhere outside of > > browser/, and we don't load any toolkit frame scripts from which it would be > > appropriate to load those actors. > > Actually, this is false. > > https://searchfox.org/mozilla-central/rev/ > 71ef4447db179639be9eff4471f32a95423962d7/toolkit/modules/ActorManagerParent. > jsm#181 > > ManifestMessagesChild.jsm in dom/ is defined here in toolkit. That's why I > thought it would work in the first place. Fair enough, but that also isn't used by Fennec.

This moves UAWidgetsChild.jsm from browser to toolkit so that Fennec could pick it up.

When the chrome script receives a DOM event, Event.target is no longer the NAC-containing <video> element. This patch allow the CastingApps.js to find the right element. Depends on D5085

Let's go with what :ntim suggests (Thanks!) since the reply over bug 1483657 is that I could do whatever sees fit. For simplicity, I'll try to enable the same UAWidgetsChild.jsm there.

Comment on attachment 9006708 [details] Bug 1483656 - Part III, Enable UA Widget on Fennec and Reftest by moving UAWidgetsChild.jsm to toolkit Jared Wein [:jaws] (please needinfo? me) has approved the revision.

Comment on attachment 9006709 [details] Bug 1483656 - Part II, Allow CastingApps to send its events to the right place Jared Wein [:jaws] (please needinfo? me) has approved the revision.

Another tap does not make video controls disappear. The patches here are not complete.

and bug 1486278 still happens with UA Widget.

Rebased and added one more patch to fix the behavior https://treeherder.mozilla.org/#/jobs?repo=try&revision=ac4c44994bde7d827a5b729a8c37a3dba5573677

On Android debug builds we failed on either one of these assertions. https://searchfox.org/mozilla-central/rev/0b8ed772d24605d7cb44c1af6d59e4ca023bd5f5/layout/generic/nsVideoFrame.cpp#187-188 https://treeherder.mozilla.org/#/jobs?repo=try&author=timdream@gmail.com&fromchange=250d821f0ff11a844de80313e930cf2dd0c454ad&selectedJob=200536001 I am building a local debug build to find out (had been stuck because of bug 998926). I am able to reproduce the crash but I couldn't get the assertion to print on either try or locally.

Looks like the assertion failure happens at MOZ_ASSERT(1 >= mContent->GetShadowRoot()->GetChildCount());

And unsurprising, the extra DOM was an extra <div id="videocontrols">. Which means the UA Widgets is being initialized twice. I don't think simple printf() shows up in the logcat, so I spent some time figured out I should use NS_WARNING(). console.log() does work in JS though. Anyway, the reason why it initialized twice was because UAWidgetsChild.jsm received UAWidgetBindToTree twice. An #ifdef was there to do things differently on Android, so I am going to tweak this to see if I can reproduce the bug on desktop. https://searchfox.org/mozilla-central/rev/0b8ed772d24605d7cb44c1af6d59e4ca023bd5f5/dom/html/HTMLMediaElement.cpp#4556-4563

(In reply to Tim Guan-tin Chien [:timdream] (please needinfo) from comment #24) > An #ifdef was there to do things differently on Android, so I am going to > tweak this to see if I can reproduce the bug on desktop. OK. I can't reproduce that on Desktop. :'( (In reply to Tim Guan-tin Chien [:timdream] (please needinfo) from comment #24) > And unsurprising, the extra DOM was an extra <div id="videocontrols">. Which > means the UA Widgets is being initialized twice. The other problem here is that the weak map should supposedly guard against the two events, given that we would save the initialized widget in it. I could potentially produce a fix with that without figuring out why BindToTree is called twice.

(In reply to Tim Guan-tin Chien [:timdream] (please needinfo) from comment #24) > I don't think simple printf() shows up in the logcat It doesn't, but printf_stderr does.

(In reply to Tim Guan-tin Chien [:timdream] (please needinfo) from comment #25) > The other problem here is that the weak map should supposedly guard against > the two events, given that we would save the initialized widget in it. I > could potentially produce a fix with that without figuring out why > BindToTree is called twice. HTMLMediaElement::BindToTree wasn't being called twice, yet we handled it twice from different UAWidgetsChild instances. I verify that right now and figuring out why it is initialized twice if so. From the console.trace() it looks like browser-content.js is loaded again when createIframe() is called on the test? Is creating an in-proc <iframe mozbrowser> really cause that? That said, that couldn't explain why Reftest fails when I include the patch of bug 1483657 in try. I couldn't reproduce Reftest failures locally though. This is troubsome...

Comment on attachment 9010745 [details] Bug 1483656 - Part I, Correct references in TouchUtils r=jaws Jared Wein [:jaws] (please needinfo? me) has approved the revision.

Try with all the dependencies and patches https://treeherder.mozilla.org/#/jobs?repo=try&revision=ca8e9dbc0c7bf65ea3e3cc5760af3b87421b40ba&selectedJob=202933415

This revert reftest changes in bug 1431255 Part VIII (c42039f3ffe7) so that we could test UA Widget in these tests. Depends on D5085

One more Try after rebase: https://treeherder.mozilla.org/#/jobs?repo=try&revision=8f2eed0527cec0c9d1da02e69ab6dfa98a332a4c

Pushed by tchien@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/a632efc6cac4 Part I, Correct references in TouchUtils r=jaws https://hg.mozilla.org/integration/autoland/rev/ce8a60ec0ee9 Part II, Allow CastingApps to send its events to the right place r=jaws https://hg.mozilla.org/integration/autoland/rev/1111ed41e9d1 Part III, Enable UA Widget on Fennec and Reftest by moving UAWidgetsChild.jsm to toolkit r=jaws https://hg.mozilla.org/integration/autoland/rev/8427d9f7e96a Part VI, Undo reftest.list changes r=jaws

https://hg.mozilla.org/mozilla-central/rev/a632efc6cac4 https://hg.mozilla.org/mozilla-central/rev/ce8a60ec0ee9 https://hg.mozilla.org/mozilla-central/rev/1111ed41e9d1 https://hg.mozilla.org/mozilla-central/rev/8427d9f7e96a