Last Comment Bug 759747 - Fullscreen Flash video does not appear on ICS
: Fullscreen Flash video does not appear on ICS
Status: VERIFIED FIXED
l10n
: productwanted
Product: Firefox for Android
Classification: Client Software
Component: General (show other bugs)
: Trunk
: ARM Android
: P1 normal (vote)
: Firefox 16
Assigned To: James Willcox (:snorp) (jwillcox@mozilla.com)
:
Mentors:
: 764635 764657 (view as bug list)
Depends on:
Blocks: 727421 761773
  Show dependency treegraph
 
Reported: 2012-05-30 07:30 PDT by James Willcox (:snorp) (jwillcox@mozilla.com)
Modified: 2012-06-28 11:51 PDT (History)
9 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
verified
fixed
verified
soft
14+


Attachments
Fix up fullscreen Flash handling on Android 4.0+ (18.64 KB, patch)
2012-06-06 12:11 PDT, James Willcox (:snorp) (jwillcox@mozilla.com)
blassey.bugs: review-
Details | Diff | Review
patch with nits addressed (11.79 KB, patch)
2012-06-06 19:12 PDT, Brad Lassey [:blassey] (use needinfo?)
no flags Details | Diff | Review
Fix up fullscreen Flash handling on Android 4.0+ (18.98 KB, patch)
2012-06-06 19:37 PDT, James Willcox (:snorp) (jwillcox@mozilla.com)
blassey.bugs: review+
Details | Diff | Review

Description James Willcox (:snorp) (jwillcox@mozilla.com) 2012-05-30 07:30:50 PDT
On sites like YouTube, only the player controls and other non-video content appears in fullscreen mode.
Comment 1 Johnathan Nightingale [:johnath] 2012-05-31 11:59:14 PDT
Provisionally plussing, but we need product's call on whether this is a blocker.
Comment 2 Karen Rudnitski [:kar] 2012-06-04 08:56:20 PDT
Although this would be good to have in our first release, we don't yet have a fix properly identified and I'm not sure there's enough time to 1) identify a fix, 2) write the fix & implement it, and 3) test it appropriately for launch. 

Feedback in the reviews on the topic have requested support, but so far no one has truly complained about the lack of (at least yet).

Therefore to mitigate, can we please add a string that layers over the box stating that full screen is not currently supported for the operating system (this should obviously only appear for those on ICS!). This will mitigate the issue of users just not knowing why it's not working.

We need to get this issue fixed for the next release, though. We'll add this as a key discussion in the release stakeholders meeting June 5 to ensure everyone has been looped in.
Comment 3 Axel Hecht [:Pike] 2012-06-04 09:18:48 PDT
I'll allow it, for:

- well separated UI element
- the users life sucks at that point anyway, and English string is better than failure
Comment 4 James Willcox (:snorp) (jwillcox@mozilla.com) 2012-06-06 12:11:59 PDT
Created attachment 630660 [details] [diff] [review]
Fix up fullscreen Flash handling on Android 4.0+
Comment 5 Brad Lassey [:blassey] (use needinfo?) 2012-06-06 19:09:56 PDT
Comment on attachment 630660 [details] [diff] [review]
Fix up fullscreen Flash handling on Android 4.0+

Review of attachment 630660 [details] [diff] [review]:
-----------------------------------------------------------------

Great work. The postDelayed()'s make me worry about race conditions they may be band-aiding.

::: mobile/android/base/GeckoApp.java
@@ +1579,5 @@
> +        mMainHandler.postDelayed(new Runnable() { 
> +            public void run() {
> +                mLayerController.getView().setVisibility(View.VISIBLE);
> +            }
> +        }, 500);

why post delayed? Changed this to just post() locally and it works fine.

@@ +3134,5 @@
> +            mMainHandler.postDelayed(new Runnable() { 
> +                public void run() {
> +                    mLayerController.getView().setVisibility(View.INVISIBLE);
> +                }
> +            }, 500);

again I'm questioning the postDelayed(). This is working for me locally:
            super.addView(view, index);
            mMainHandler.post(new Runnable() { 
                public void run() {
                    mLayerController.getView().setVisibility(View.INVISIBLE);
                }
            });

@@ +3145,5 @@
> +            if (event.isSystem()) {
> +                return super.onKeyDown(keyCode, event);
> +            }
> +            mFullScreenPluginView.onKeyDown(keyCode, event);
> +            return true;

should be:
return mFullScreenPluginView.onKeyDown(keyCode, event);

no?

@@ +3154,5 @@
> +            if (event.isSystem()) {
> +                return super.onKeyUp(keyCode, event);
> +            }
> +            mFullScreenPluginView.onKeyUp(keyCode, event);
> +            return true;

same as above

@@ +3159,5 @@
> +        }
> +
> +        @Override
> +        public boolean onTouchEvent(MotionEvent event) {
> +            return true;

if we don't pass these to the plugin, I think we should return false instead of true.

@@ +3165,5 @@
> +
> +        @Override
> +        public boolean onTrackballEvent(MotionEvent event) {
> +            mFullScreenPluginView.onTrackballEvent(event);
> +            return true;

same as above
Comment 6 Brad Lassey [:blassey] (use needinfo?) 2012-06-06 19:12:47 PDT
Created attachment 630809 [details] [diff] [review]
patch with nits addressed
Comment 7 James Willcox (:snorp) (jwillcox@mozilla.com) 2012-06-06 19:37:00 PDT
Created attachment 630813 [details] [diff] [review]
Fix up fullscreen Flash handling on Android 4.0+
Comment 8 Brad Lassey [:blassey] (use needinfo?) 2012-06-06 19:42:44 PDT
Comment on attachment 630813 [details] [diff] [review]
Fix up fullscreen Flash handling on Android 4.0+

Review of attachment 630813 [details] [diff] [review]:
-----------------------------------------------------------------

::: mobile/android/base/GeckoApp.java
@@ +3131,5 @@
> +             * it for some reason so we need to hide that. Hiding the LayerView causes
> +             * its surface to be destroyed, which causes a pause composition
> +             * event to be sent to Gecko. We synchronously wait for that to be
> +             * processed. Simultaneously, however, Flash is waiting on a mutex so
> +             * the delay below is an attempt to avoid a deadlock.

comment needs to be reworked
Comment 9 James Willcox (:snorp) (jwillcox@mozilla.com) 2012-06-06 20:09:28 PDT
https://hg.mozilla.org/integration/mozilla-inbound/rev/1d1aabb06e7c
Comment 10 Ed Morley [:emorley] 2012-06-07 05:44:53 PDT
https://hg.mozilla.org/mozilla-central/rev/1d1aabb06e7c
Comment 11 Aaron Train [:aaronmt] 2012-06-07 06:45:24 PDT
Issue resolved on a glance tour through a latest inbound; (06/07); YouTube full-screen is working for me on my Galaxy Nexus (4.0.4), and Galaxy Note (4.0.3). Marking status-firefox16 as fixed.

This still a softy?
Comment 13 Kevin Brosnan [:kbrosnan] 2012-06-13 19:31:13 PDT
*** Bug 764635 has been marked as a duplicate of this bug. ***
Comment 14 Naoki Hirata :nhirata (please use needinfo instead of cc) 2012-06-14 15:15:31 PDT
*** Bug 764657 has been marked as a duplicate of this bug. ***
Comment 15 Naoki Hirata :nhirata (please use needinfo instead of cc) 2012-06-15 09:21:34 PDT
Full screen works... non full screen doesn't show video right always on ics ?

Need to investigate more.  Marking verified for full screen flash video for 14 b7 candidate 3.

Note You need to log in before you can comment on or make changes to this bug.