759747 - Fullscreen Flash video does not appear on ICS

Status: VERIFIED FIXED

(Firefox for Android Graveyard :: General, defect, P1)

Description

[James Willcox (:snorp) ([e-mail]) (he/him)]

On sites like YouTube, only the player controls and other non-video content appears in fullscreen mode.

Comment 1

[Johnathan Nightingale [:johnath]]

Provisionally plussing, but we need product's call on whether this is a blocker.

Comment 2

[Karen Rudnitski [:kar]]

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]]

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) ([e-mail]) (he/him)]

Attachment: Fix up fullscreen Flash handling on Android 4.0+

Comment 5

[Brad Lassey [:blassey] (use needinfo?)]

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?)]

Attachment: patch with nits addressed

Comment 7

[James Willcox (:snorp) ([e-mail]) (he/him)]

Attachment: Fix up fullscreen Flash handling on Android 4.0+

Comment 8

[Brad Lassey [:blassey] (use needinfo?)]

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) ([e-mail]) (he/him)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/1d1aabb06e7c

Comment 10

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/1d1aabb06e7c

Comment 11

[Aaron Train [:aaronmt]]

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 12

[Brad Lassey [:blassey] (use needinfo?)]

https://hg.mozilla.org/releases/mozilla-aurora/rev/c9aa071bf2fd
https://hg.mozilla.org/releases/mozilla-beta/rev/b0f32cd82c97
https://hg.mozilla.org/releases/mozilla-beta/rev/33043ace32f0

Comment 15

[Naoki Hirata :nhirata (please use needinfo instead of cc)]

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.