1483656 - Enable UA Widget on Fennec and Reftest

Status: RESOLVED FIXED

(Core :: XBL, defect, P5)

Description

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

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.

Comment 1

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

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.)

Comment 2

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

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

Comment 3

[Tim Nguyen :ntim]

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 ?

Comment 4

[Tim Nguyen :ntim]

Attachment: Move UAWidgetsChild.jsm to toolkit so it works on Fennec

Comment 5

[Tim Nguyen :ntim]

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

Comment 6

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

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?

Comment 7

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

Attachment: Working patch per ntim's idea

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.

Comment 8

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

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

Comment 9

[Tim Nguyen :ntim]

(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 ?

Comment 10

[Kris Maglione [:kmag]]

(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.

Comment 11

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

(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.

Comment 12

[Kris Maglione [:kmag]]

(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.

Comment 13

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

Attachment: Bug 1483656 - Part III, Enable UA Widget on Fennec and Reftest by moving UAWidgetsChild.jsm to toolkit

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

Comment 14

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

Attachment: Bug 1483656 - Part II, Allow CastingApps to send its events to the right place

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

Comment 15

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

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 16

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

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 17

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

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.

Comment 18

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

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

Comment 19

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

and bug 1486278 still happens with UA Widget.

Comment 20

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

Rebased and added one more patch to fix the behavior

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

Comment 21

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

Attachment: Bug 1483656 - Part I, Correct references in TouchUtils r=jaws

Comment 22

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

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=[e-mail]&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.

Comment 23

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

Looks like the assertion failure happens at

MOZ_ASSERT(1 >= mContent->GetShadowRoot()->GetChildCount());

Comment 24

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

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

Comment 25

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

(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.

Comment 26

[Kris Maglione [:kmag]]

(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.

Comment 27

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

(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 28

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

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.

Comment 30

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

Try with all the dependencies and patches

https://treeherder.mozilla.org/#/jobs?repo=try&revision=ca8e9dbc0c7bf65ea3e3cc5760af3b87421b40ba&selectedJob=202933415

Comment 31

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

Attachment: Bug 1483656 - Part VI, Undo reftest.list changes r=jaws

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

Depends on D5085

Comment 32

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

One more Try after rebase:

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

Comment 33

[Pulsebot]

Pushed by [e-mail]:
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

Comment 34

[Cosmin Sabou [:CosminS]]

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