842130 - Remove unused fullscreen front end code

Status: RESOLVED FIXED

(Firefox for Metro Graveyard :: Browser, defect)

Description

[Jim Mathies [:jimm]]

We currently have fennec code (which doesn't appear to be working at the moment) that supports loading videos into xul pages in fresh tabs. The user accesses this from the context menu on the video.

Our current built-in video playback overlay has a full screen option on desktop, which opens a new xul window and prompts the user for full screen permissions. I'm guessing this is using the new full dom api.

We should investigate using this in metro. We would have to solve the top level window creation problem, and update the html5 video overlay somehow to include a full screen button.

Comment 1

[Jim Mathies [:jimm]]

Also, if we do implement something for this, we should add context menu tests for video elements.

Comment 2

[Chris Pearce [:cpearce (Not reading bugmail)]]

The videocontrols in desktop Firefox do not create a new XUL window, they use the DOM fullscreen API to call mozRequestFullScreen() on the <video> element. The permission dialog code lives in desktop Firefox's Chrome.

Before DOM fullscreen was enabled in desktop Firefox we opened a new XUL Window and made that sizemode=fullscreen and style the video to be width=100%, maybe that's the code "(which doesn't appear to be working at the moment)" you're referring to?

I think we should just replace the out of date video controls in Metrofox with the touch controls in B2G/Android. Then we don't have old fennec controls in Metrofox, and we can reuse the code and reduce the amount of testing required.

Comment 3

[Matt Brubeck (:mbrubeck)]

(In reply to Chris Pearce (:cpearce) from comment #2)
> Before DOM fullscreen was enabled in desktop Firefox we opened a new XUL
> Window and made that sizemode=fullscreen and style the video to be
> width=100%, maybe that's the code "(which doesn't appear to be working at
> the moment)" you're referring to?

Basically -- we are using the old XUL Fennec code (bug 590715) which is different from the old desktop code; instead of a new XUL window it overlays a <browser> above the content in the existing window.  But same basic idea.

> I think we should just replace the out of date video controls in Metrofox
> with the touch controls in B2G/Android. Then we don't have old fennec
> controls in Metrofox, and we can reuse the code and reduce the amount of
> testing required.

I believe we are already using the same video controls as Android and B2G:
http://mxr.mozilla.org/mozilla-central/source/browser/metro/theme/content.css#303
http://mxr.mozilla.org/mozilla-central/source/b2g/chrome/content/content.css#75
http://mxr.mozilla.org/mozilla-central/source/mobile/android/themes/core/content.css#291

Both Android and Metro have the fullscreen button hidden, with a comment mentioning bug 704229:
http://mxr.mozilla.org/mozilla-central/source/browser/metro/theme/touchcontrols.css#55
http://mxr.mozilla.org/mozilla-central/source/mobile/android/themes/core/touchcontrols.css#55

We should follow the work in that bug, which gets rid of some custom theming that is currently duplicated in Android and Metro.  However, that seems to be orthogonal to the problems in our fullscreen implementation.

Comment 4

[Brian R. Bondy [:bbondy]]

Performing work in bug 850458

Comment 5

[Brian R. Bondy [:bbondy]]

Re-opening for this:

(In reply to Jim Mathies [:jimm] from comment #14)
> We still have all the xul fennec fullscreen code sitting in the repo.
> Deciding what to do with that was bug 842130. If we're going to dupe over,
> we'll need to figure out what we plan to do with all of this code here.
> 
> http://mxr.mozilla.org/mozilla-central/source/browser/metro/base/content/
> video.js
> http://mxr.mozilla.org/mozilla-central/source/browser/metro/modules/video.jsm
> http://mxr.mozilla.org/mozilla-central/source/browser/metro/base/content/
> pages/fullscreen-video.xhtml

Comment 6

[Brian R. Bondy [:bbondy]]

Attachment: Patch v1.

I don't think it's in use so I think we should remove it.

Comment 7

[Brian R. Bondy [:bbondy]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/9eb6532ccfc3

Comment 8

[Brian R. Bondy [:bbondy]]

Backout for potential bustage on some slaves (but not others :S) 
https://hg.mozilla.org/integration/mozilla-inbound/rev/85eedb253f25

Comment 9

[Brian R. Bondy [:bbondy]]

Philor mentioned he'd check into it for Bug 850458 and this bug.
I think the problem is that the push needed a clobber on the build machines.
I think the clobber was needed because of the removal of the jsm module.
If that's all that was needed then the 2 pushes that need to go back in are:
https://hg.mozilla.org/integration/mozilla-inbound/rev/9eb6532ccfc3
https://hg.mozilla.org/integration/mozilla-inbound/rev/b420dcf17910

Comment 10

[Brian R. Bondy [:bbondy]]

OK I see what's happened....
There are no more modules in metro/modules.

So it doesn't create the objdir directory, then the last line in package-manifest.in causes an error if that dir doesn't exist:
@BINPATH@/metro/module

Ally will be adding a new module there so I'll just wait for her patch to land before I re-land this.

So I think the machines it was working on were the machines that were not clobbers.

Comment 11

[Phil Ringnalda (:philor)]

Sorry, forgot to report my negative results. None of the builds while you were in admitted to being clobbers, but there's some path through the clobberer (maybe it's "clobber set for job A, slave does job B but notices it has an objdir for job A so it clobbers it before doing job B, then maybe days later does job A and has already clobbered", not sure) where the build doesn't say it was a clobber, but it was. So I can neither confirm nor deny that the busted ones were clobbers.

Comment 12

[Brian R. Bondy [:bbondy]]

Thanks philor, no problem I think the problem is just a missing output directory when there are no modules.  Ally is landing something soon that adds a module so I'll just wait for that, test it on try and then re-land.

Comment 13

[Brian R. Bondy [:bbondy]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/0cfcf56f7b4e

Comment 14

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/0cfcf56f7b4e