Remove unused fullscreen front end code

RESOLVED FIXED in Firefox 23

Status

RESOLVED FIXED
6 years ago
4 years ago

People

(Reporter: jimm, Assigned: bbondy)

Tracking

Trunk
Firefox 23
x86_64
Windows 8.1
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: feature=work)

Attachments

(1 attachment)

(Reporter)

Description

6 years ago
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.
(Reporter)

Comment 1

6 years ago
Also, if we do implement something for this, we should add context menu tests for video elements.
Summary: Use the dom's built-in video fullscreen functionality → Fix fullscreen video which currently isn't working

Updated

6 years ago
Blocks: 841214

Updated

6 years ago
Blocks: 842639
No longer blocks: 841214
Whiteboard: [metro-mvp?] → feature=work
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.
(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.
Depends on: 704229
(Reporter)

Updated

6 years ago
Component: General → Browser
(Assignee)

Comment 4

6 years ago
Performing work in bug 850458
Status: NEW → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → DUPLICATE
Duplicate of bug: 850458
(Assignee)

Comment 5

6 years ago
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
Status: RESOLVED → REOPENED
Resolution: DUPLICATE → ---
(Assignee)

Comment 6

6 years ago
Created attachment 730951 [details] [diff] [review]
Patch v1.

I don't think it's in use so I think we should remove it.
Assignee: nobody → netzen
Attachment #730951 - Flags: review?(mbrubeck)
Attachment #730951 - Flags: review?(mbrubeck) → review+
(Assignee)

Updated

6 years ago
Summary: Fix fullscreen video which currently isn't working → Remove unused fullscreen front end code
(Assignee)

Comment 8

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

Comment 9

6 years ago
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
(Assignee)

Comment 10

6 years ago
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.
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.
(Assignee)

Comment 12

6 years ago
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.
(Assignee)

Comment 13

6 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/0cfcf56f7b4e
Target Milestone: Firefox 22 → Firefox 23
https://hg.mozilla.org/mozilla-central/rev/0cfcf56f7b4e
Status: REOPENED → RESOLVED
Last Resolved: 6 years ago6 years ago
Resolution: --- → FIXED
Depends on: 864733
OS: Windows 8 Metro → Windows 8.1
You need to log in before you can comment on or make changes to this bug.