Closed Bug 577494 Opened 11 years ago Closed 11 years ago

Fullscreen in flash video does not hide dock, menubar

Categories

(Core :: Plug-ins, defect)

x86
macOS
defect
Not set
major

Tracking

()

VERIFIED FIXED
mozilla2.0b4
Tracking Status
blocking2.0 --- betaN+

People

(Reporter: dean.wyatte, Assigned: BenWa)

References

()

Details

(Keywords: regression)

Attachments

(1 file, 3 obsolete files)

User-Agent:       Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.6; en-US; rv:2.0b1) Gecko/20100630 Firefox/4.0b1
Build Identifier: Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.6; en-US; rv:2.0b1) Gecko/20100630 Firefox/4.0b1

When using fullscreen mode in a flash video (e.g., youtube.com, hulu.com), the dock and menu bar are not properly hidden. Maybe leaving the menubar exposed was intentional, but leaving the dock exposed makes it impossible to access most flash video controls like play/pause/seek.

Reproducible: Always

Steps to Reproduce:
1. Go to youtube.com
2. Click any video
3. When video plays, enter fullscreen in the flash video by clicking the appropriate flash button (four arrows, second from right)
Actual Results:  
Fullscreen mode is entered, but the dock and menubar are left exposed.

Expected Results:  
Previous versions of Firefox hid the dock and menubar.
Does this happen with Firefox in a new profile? http://support.mozilla.com/en-US/kb/Managing+profiles
Version: unspecified → Trunk
The bug still happens under a new profile. Here is some output from the Terminal when attempting to use fullscreen on youtube. 

setzer:~ dwyatte$ 2010-07-08 13:03:15.526 plugin-container[89640:903] Warning once: This application, or a library it uses, is using NSQuickDrawView, which has been deprecated. Apps should cease use of QuickDraw and move to Quartz.
Thu Jul  8 13:03:15 setzer.local plugin-container[89640] <Error>: kCGErrorIllegalArgument: CGSOrderWindowList
Thu Jul  8 13:03:15 setzer.local plugin-container[89640] <Error>: kCGErrorFailure: Set a breakpoint @ CGErrorBreakpoint() to catch errors as they are logged.
This is likely due to the new out-of-process handling of plugins in 4.0 on OSX.
Status: UNCONFIRMED → NEW
Ever confirmed: true
blocking2.0: --- → ?
Blocks: 567265
Component: General → Plug-ins
Product: Firefox → Core
QA Contact: general → plugins
(In reply to comment #3)
> This is likely due to the new out-of-process handling of plugins in 4.0 on OSX.

Well, Firefox 3.6.8 doesn't show this behavior, so I don't think OOPP are related here. Do we know when this has been regressed? Would be great if someone could check for a regression range.
(In reply to comment #4)
> Well, Firefox 3.6.8 doesn't show this behavior, so I don't think OOPP are
> related here.

OOPP wasn't introduced in OSX until Firefox 4.x
What makes you think that? The Lorentz branch has been landed on 1.9.2 for Firefox 3.6.4. See the release notes:

http://www.mozilla.com/en-US/firefox/3.6.4/releasenotes/
But not on Mac.
My apologies! That's true. Would someone has time to run a regression check?
We don't need a regression check since this is a caused by OOPP. 

I think that Flash calls CGSOrderWindowList to hide the dock and this now fails because it's not invoked from the main process.
Update using 4.0b2:

Upon entering fullscreen, it is possible to hide the dock by clicking on the flash object. However, upon exiting fullscreen, the Dock is not unhidden -- the only way to get the Dock back is to click anywhere on the screen
blocking2.0: ? → betaN+
Assignee: nobody → b56girard
Blocks: 561462
Attached patch Interposing System Calls v1 (obsolete) — Splinter Review
This patch is based on Chrome's implementation of interposing system calls. We catch the system call and forward an appropriate message to the browser process. 

The file PluginInterposeOSX.mm is a modified version of chrome's plugin_interpose_util_mac.mm thus I licensed it as BSD. I need a legal review for importing Chrome code.

This patch also fixes bug 561462.
Attachment #462177 - Flags: review?(joshmoz)
Comment on attachment 462177 [details] [diff] [review]
Interposing System Calls v1

+__attribute__((visibility("default")))

Is it really necessary to mess with symbol visibility anywhere in this patch?

+@implementation NSWindow (ChromePluginInterposing)

This code is not going to be regularly sync'd directly from the chromium tree. We should change the names to not reference Chrome. Only the license needs to reference Chrome, afaik.

+#if defined(OS_MACOSX)

Probably better to use the "XP_" versions of these macros in new code.
Attachment #462177 - Flags: review?(joshmoz)
Attached patch Interposing System Calls v2 (obsolete) — Splinter Review
Thanks for the comments, they are all applied here.

The use of XP_* OS_* macros is very inconsistent in PluginModule* PluginInstance* for all operating systems unfortunately so I am never sure which to use.
Attachment #462177 - Attachment is obsolete: true
Attachment #463416 - Flags: review?(joshmoz)
I replaced 'ChromePluginInterposing' to just 'PluginInterposing'.

Also Chrome is interposing the calls to Hide/Show cursor. We will need to interpose these as well. I will open a follow up bug once this land.
Comment on attachment 463416 [details] [diff] [review]
Interposing System Calls v2

+@interface NSWindow (PluginInterposing)
+- (void)chromePlugin_orderOut:(id)sender;
+- (void)chromePlugin_orderFront:(id)sender;
+- (void)chromePlugin_makeKeyAndOrderFront:(id)sender;
+- (void)chromePlugin_setWindowNumber:(NSInteger)num;
+@end

Still a bunch of references to Chrome.
Attachment #463416 - Flags: review?(joshmoz) → review-
Attached patch Interposing System Calls v3 (obsolete) — Splinter Review
Opps, vim defaulted to case sensitive search.
Fixed.
Attachment #463416 - Attachment is obsolete: true
Attachment #463587 - Flags: review?(joshmoz)
Attachment #463587 - Flags: review?(joshmoz) → review+
Can I get a legal review for importing chrome code?
The file dom/plugins/PluginInterposeOSX.mm in this patch is based on Chrome's chrome/plugin/plugin_interpose_util_mac.mm file with some of our modifications:
http://src.chromium.org/viewvc/chrome/trunk/src/chrome/plugin/plugin_interpose_util_mac.mm?view=markup&pathrev=45105

I replaced the header pointing to the LICENSE file with the contents of the LICENSE file.
AIUI, this is all BSD and fine for us to import. The r+ indicates, to me at least, that you've done the right thing w.r.t including the license.
We agreed on IRC we should modify about:license#chromium. I will post a patch tomorrow.
This is good to land.
Attachment #463587 - Attachment is obsolete: true
Keywords: checkin-needed
Pushed to mozilla-central:
http://hg.mozilla.org/mozilla-central/rev/4bc4033139c8
Status: NEW → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla2.0b4
Verified fixed with Mozilla/5.0 (Macintosh; Intel Mac OS X 10.6; rv:2.0b5pre) Gecko/20100823 Minefield/4.0b5pre
Status: RESOLVED → VERIFIED
Blocks: 914002
Points: --- → 8
Flags: firefox-backlog+
Iteration: --- → 33.3
Iteration: 33.3 → ---
Points: 8 → ---
Flags: firefox-backlog+
You need to log in before you can comment on or make changes to this bug.