Closed Bug 1483656 Opened Last year Closed Last year

Enable UA Widget on Fennec and Reftest

Categories

(Core :: XBL, defect, P5)

defect

Tracking

()

RESOLVED FIXED
mozilla64
Tracking Status
firefox63 --- wontfix
firefox64 --- fixed

People

(Reporter: timdream, Assigned: timdream)

References

(Depends on 1 open bug)

Details

Attachments

(4 files, 2 obsolete files)

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.)
Flags: needinfo?(bugs)
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 ?
Flags: needinfo?(timdream)
Assignee: timdream → ntim.bugs
Untested, but it seems to me that it should work, as toolkit modules are loaded on Fennec.
Assignee: ntim.bugs → timdream
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?
Flags: needinfo?(timdream)
Attached patch Working patch per ntim's idea (obsolete) — Splinter Review
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.
Attachment #9002426 - Attachment is obsolete: true
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 ?
Flags: needinfo?(kmaglione+bmo)
(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.
Flags: needinfo?(kmaglione+bmo)
(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.
Flags: needinfo?(kmaglione+bmo)
(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.
Flags: needinfo?(kmaglione+bmo)
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.
Flags: needinfo?(bugs)
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.
Attachment #9006708 - Flags: review+
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.
Attachment #9006709 - Flags: review+
Another tap does not make video controls disappear. The patches here are not complete.
and bug 1486278 still happens with UA Widget.
Attachment #9002915 - Attachment is obsolete: true
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.
Attachment #9010745 - Flags: review+
Duplicate of this bug: 1483657
Summary: Enable UA Widget on Fennec → Enable UA Widget on Fennec and Reftest
Attachment #9006708 - Attachment description: Bug 1483656 - Part I, Enable UA Widget on Fennec → Bug 1483656 - Part III, Enable UA Widget on Fennec and Reftest
This revert reftest changes in bug 1431255 Part VIII (c42039f3ffe7)
so that we could test UA Widget in these tests.

Depends on D5085
Attachment #9006708 - Attachment description: Bug 1483656 - Part III, Enable UA Widget on Fennec and Reftest → Bug 1483656 - Part III, Enable UA Widget on Fennec and Reftest by moving UAWidgetsChild.jsm to toolkit
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
Depends on: 1499372
Depends on: 1518664
You need to log in before you can comment on or make changes to this bug.