Closed Bug 842130 Opened 11 years ago Closed 11 years ago
Remove unused fullscreen front end code
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.
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
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
Performing work in bug 850458
Status: NEW → RESOLVED
Closed: 11 years ago
Resolution: --- → DUPLICATE
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 → ---
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+
Target Milestone: --- → Firefox 22
Summary: Fix fullscreen video which currently isn't working → Remove unused fullscreen front end code
Backout for potential bustage on some slaves (but not others :S) https://hg.mozilla.org/integration/mozilla-inbound/rev/85eedb253f25
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
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.
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.
Target Milestone: Firefox 22 → Firefox 23
Status: REOPENED → RESOLVED
Closed: 11 years ago → 11 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.