Closed
Bug 1294823
Opened 8 years ago
Closed 8 years ago
Don't use a fullscreen view for HLS
Categories
(Firefox for Android Graveyard :: Audio/Video, defect, P2)
Tracking
(fennec50+, firefox50 verified, firefox51 verified)
VERIFIED
FIXED
Firefox 51
People
(Reporter: snorp, Assigned: snorp)
References
Details
Attachments
(3 files, 2 obsolete files)
182.14 KB,
image/png
|
Details | |
686.94 KB,
image/png
|
Details | |
17.09 KB,
patch
|
sebastian
:
review+
ritu
:
approval-mozilla-beta+
|
Details | Diff | Splinter Review |
It's jarring.
Assignee | ||
Updated•8 years ago
|
Assignee: nobody → snorp
tracking-fennec: --- → ?
Assignee | ||
Comment 1•8 years ago
|
||
Attachment #8780708 -
Flags: review?(s.kaspari)
Comment 2•8 years ago
|
||
Comment on attachment 8780708 [details] [diff] [review]
Don't use fullscreen mode for the HLS video view on Android
Review of attachment 8780708 [details] [diff] [review]:
-----------------------------------------------------------------
::: mobile/android/base/java/org/mozilla/gecko/BrowserApp.java
@@ +965,5 @@
> if (hideFirstrunPager(TelemetryContract.Method.BACK)) {
> return;
> }
>
> + if (mVideoLayout.haveVideo()) {
Is it really haveVideo and not hasVideo? I'm confused.
@@ +4122,5 @@
> Telemetry.sendUIEvent(TelemetryContract.Event.LAUNCH, method, "restricted");
> }
> }
> +
> + static class VideoFrameLayout extends FrameLayout {
Can we move this to a separate file in org.mozilla.gecko.widget? The line count of BrowserApp is too damn high.
@@ +4183,5 @@
> + }
> +
> + @Override
> + public boolean onTouchEvent(MotionEvent event) {
> + mVideo.onTouchEvent(event);
* Doesn't the video view already get the touch events?
* Why don't we return the boolean from mVideo.onTouchEvent()?
@@ +4190,5 @@
> +
> + @Override
> + public boolean onTrackballEvent(MotionEvent event) {
> + mVideo.onTrackballEvent(event);
> + return true;
* return mVideo.onTrackballEvent(..) ?
Attachment #8780708 -
Flags: review?(s.kaspari) → review+
Assignee | ||
Comment 3•8 years ago
|
||
(In reply to Sebastian Kaspari (:sebastian) from comment #2)
> Comment on attachment 8780708 [details] [diff] [review]
> Don't use fullscreen mode for the HLS video view on Android
>
> Review of attachment 8780708 [details] [diff] [review]:
> -----------------------------------------------------------------
>
> ::: mobile/android/base/java/org/mozilla/gecko/BrowserApp.java
> @@ +965,5 @@
> > if (hideFirstrunPager(TelemetryContract.Method.BACK)) {
> > return;
> > }
> >
> > + if (mVideoLayout.haveVideo()) {
>
> Is it really haveVideo and not hasVideo? I'm confused.
Could be either. I guess hasVideo() seems better.
>
> @@ +4122,5 @@
> > Telemetry.sendUIEvent(TelemetryContract.Event.LAUNCH, method, "restricted");
> > }
> > }
> > +
> > + static class VideoFrameLayout extends FrameLayout {
>
> Can we move this to a separate file in org.mozilla.gecko.widget? The line
> count of BrowserApp is too damn high.
Yup, fair enough.
>
> @@ +4183,5 @@
> > + }
> > +
> > + @Override
> > + public boolean onTouchEvent(MotionEvent event) {
> > + mVideo.onTouchEvent(event);
>
> * Doesn't the video view already get the touch events?
> * Why don't we return the boolean from mVideo.onTouchEvent()?
It does, but we want touches in the containing FrameLayout to be eaten and not pass through to the underlying LayerView. That's why we always return true since that means the event has been handled.
Comment 4•8 years ago
|
||
Comment 5•8 years ago
|
||
Comment 6•8 years ago
|
||
Tried it out on http://www.newshub.co.nz/business/powerball-winners-cash-in-tickets-2016071211#axzz4EA1uumyJ
I noticed a few things
- the overlay feels somehow not very accurate, spanning across the whole width of the screen (see attachment)
- it shows an error message for the video if it's not playing indicating to the user that they can't view the video (see attachment)
I'm currently worried this experience is actually worse than the full screen version.
Thoughts?
Assignee | ||
Comment 7•8 years ago
|
||
(In reply to Barbara Bermes [:barbara] from comment #6)
> Tried it out on
> http://www.newshub.co.nz/business/powerball-winners-cash-in-tickets-
> 2016071211#axzz4EA1uumyJ
>
> I noticed a few things
> - the overlay feels somehow not very accurate, spanning across the whole
> width of the screen (see attachment)
That's by design here -- we aren't trying to position it on top of the real video.
> - it shows an error message for the video if it's not playing indicating to
> the user that they can't view the video (see attachment)
Yeah, that has always been the case, but we can probably fix that. I'll file separately.
>
> I'm currently worried this experience is actually worse than the full screen
> version.
>
> Thoughts?
Why do you think it's worse? The fullscreen one feels like you've been kicked into a totally different app unexpectedly to me. At least with this one you can tell you're still in Firefox, and you can see the web page coming through the overlay.
Assignee | ||
Updated•8 years ago
|
tracking-fennec: ? → 50+
Assignee | ||
Comment 8•8 years ago
|
||
Attachment #8783688 -
Flags: review+
Assignee | ||
Updated•8 years ago
|
Attachment #8780708 -
Attachment is obsolete: true
Pushed by jwillcox@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/b6e4605e1217
Don't use fullscreen mode for the HLS video view on Android r=sebastian
Comment 10•8 years ago
|
||
Backout by jwillcox@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/a924e27a4666
Backout because I accidentally pushed the wrong patch r=me
Assignee | ||
Comment 11•8 years ago
|
||
Attachment #8785465 -
Flags: review?(s.kaspari)
Assignee | ||
Updated•8 years ago
|
Attachment #8783688 -
Attachment is obsolete: true
Comment 12•8 years ago
|
||
Comment on attachment 8785465 [details] [diff] [review]
Don't use fullscreen mode for the HLS video view on Android
Review of attachment 8785465 [details] [diff] [review]:
-----------------------------------------------------------------
::: mobile/android/base/java/org/mozilla/gecko/media/VideoPlayer.java
@@ +1,1 @@
> +package org.mozilla.gecko.media;
nit: License header.
@@ +25,5 @@
> +
> +public class VideoPlayer extends FrameLayout {
> + private VideoView mVideo;
> + private FullScreenMediaController mController;
> + private FullScreenListener mFullScreenListener;
nit: For new Java code we stopped using the 'm' prefix.
@@ +180,5 @@
> + // http://androidxref.com/6.0.1_r10/xref/frameworks/base/core/java/android/widget/MediaController.java#239
> + //
> + // The media buttons are in a horizontal linear layout which is itself packed into
> + // a vertical layout. The vertical layout is the only child of the FrameLayout which
> + // MediaController inherits from.
Isn't this a bit fragile? I guess this will need some testing on various Android versions and devices?
@@ +182,5 @@
> + // The media buttons are in a horizontal linear layout which is itself packed into
> + // a vertical layout. The vertical layout is the only child of the FrameLayout which
> + // MediaController inherits from.
> + LinearLayout child = (LinearLayout)getChildAt(0);
> + LinearLayout buttons = (LinearLayout)child.getChildAt(0);
nit: space after (LinearLayout) - Did you run checkstyle yet?
Attachment #8785465 -
Flags: review?(s.kaspari) → review+
Assignee | ||
Comment 13•8 years ago
|
||
(In reply to Sebastian Kaspari (:sebastian) from comment #12)
> Comment on attachment 8785465 [details] [diff] [review]
> Don't use fullscreen mode for the HLS video view on Android
>
> Review of attachment 8785465 [details] [diff] [review]:
> -----------------------------------------------------------------
>
> ::: mobile/android/base/java/org/mozilla/gecko/media/VideoPlayer.java
> @@ +1,1 @@
> > +package org.mozilla.gecko.media;
>
> nit: License header.
>
> @@ +25,5 @@
> > +
> > +public class VideoPlayer extends FrameLayout {
> > + private VideoView mVideo;
> > + private FullScreenMediaController mController;
> > + private FullScreenListener mFullScreenListener;
>
> nit: For new Java code we stopped using the 'm' prefix.
>
> @@ +180,5 @@
> > + // http://androidxref.com/6.0.1_r10/xref/frameworks/base/core/java/android/widget/MediaController.java#239
> > + //
> > + // The media buttons are in a horizontal linear layout which is itself packed into
> > + // a vertical layout. The vertical layout is the only child of the FrameLayout which
> > + // MediaController inherits from.
>
> Isn't this a bit fragile? I guess this will need some testing on various
> Android versions and devices?
Maybe, but nothing has really ever changed there since it was written: http://androidxref.com/7.0.0_r1/xref/frameworks/base/core/res/res/layout/media_controller.xml
>
> @@ +182,5 @@
> > + // The media buttons are in a horizontal linear layout which is itself packed into
> > + // a vertical layout. The vertical layout is the only child of the FrameLayout which
> > + // MediaController inherits from.
> > + LinearLayout child = (LinearLayout)getChildAt(0);
> > + LinearLayout buttons = (LinearLayout)child.getChildAt(0);
>
> nit: space after (LinearLayout) - Did you run checkstyle yet?
I didn't run checkstyle, I'll definitely do that.
Comment 14•8 years ago
|
||
Pushed by jwillcox@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/3f617fcd2cd2
Don't use fullscreen mode for the HLS video view on Android r=sebastian
Updated•8 years ago
|
Priority: -- → P2
Comment 15•8 years ago
|
||
bugherder |
Status: NEW → RESOLVED
Closed: 8 years ago
status-firefox51:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 51
Updated•8 years ago
|
QA Contact: mihai.ninu
Assignee | ||
Comment 16•8 years ago
|
||
Comment on attachment 8785465 [details] [diff] [review]
Don't use fullscreen mode for the HLS video view on Android
Approval Request Comment
[Feature/regressing bug #]: 1286133
[User impact if declined]: Bad HLS experience
[Describe test coverage new/current, TreeHerder]: Aurora, Nightly
[Risks and why]: Low
[String/UUID change made/needed]: None
Attachment #8785465 -
Flags: approval-mozilla-beta?
status-firefox50:
--- → affected
Flags: qe-verify+
Comment on attachment 8785465 [details] [diff] [review]
Don't use fullscreen mode for the HLS video view on Android
HLS video is currently planned for Fx50, let's uplift this to Beta.
Attachment #8785465 -
Flags: approval-mozilla-beta? → approval-mozilla-beta+
This patch didn't apply cleanly to beta, could we get a rebased patch?
Flags: needinfo?(snorp)
Assignee | ||
Comment 19•8 years ago
|
||
Flags: needinfo?(snorp)
Comment 20•8 years ago
|
||
Verified as fixed on both Aurora (51.0a2 - 2016-10-06) and Beta 50.0b4 builds, on a Samsung Galaxy S6 EDGE (android 6.0) and on a Nexus 7 (Android 5.1)
Updated•6 years ago
|
Status: RESOLVED → VERIFIED
Flags: qe-verify+
Updated•4 years ago
|
Product: Firefox for Android → Firefox for Android Graveyard
You need to log in
before you can comment on or make changes to this bug.
Description
•