Kindle Fire: YouTube videos do not open in unavailable YouTube App

VERIFIED FIXED

Status

Fennec Graveyard
General
VERIFIED FIXED
6 years ago
4 years ago

People

(Reporter: akeybl, Assigned: blassey)

Tracking

(Depends on: 4 bugs, {verified-beta})

Firefox 9
verified-beta
Dependency tree / graph

Details

Attachments

(1 attachment, 3 obsolete attachments)

(Reporter)

Description

6 years ago
When trying to play a YouTube video, on the Kindle Fire I get a dialog that this action will open an external app, but when I hit "OK" no app is launched. There is no YouTube app available on the Kindle Fire.
Created attachment 578976 [details] [diff] [review]
patch to ship a simple youtube player
Assignee: nobody → blassey.bugs
Attachment #578976 - Flags: review?(mark.finkle)
Attachment #578976 - Flags: review?(doug.turner)
Created attachment 578978 [details] [diff] [review]
patch to ship a simple youtube player
Attachment #578976 - Attachment is obsolete: true
Attachment #578976 - Flags: review?(mark.finkle)
Attachment #578976 - Flags: review?(doug.turner)
Attachment #578978 - Flags: review?(mark.finkle)
Attachment #578978 - Flags: review?(doug.turner)
pushed to try https://tbpl.mozilla.org/?tree=Try&rev=7feacaa09970

Comment 4

6 years ago
Try run for 7feacaa09970 is complete.
Detailed breakdown of the results available here:
    https://tbpl.mozilla.org/?tree=Try&rev=7feacaa09970
Results (out of 1 total builds):
    failure: 1
Builds available at http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/blassey@mozilla.com-7feacaa09970

Comment 5

6 years ago
Try run for b5fd4bb0ee8c is complete.
Detailed breakdown of the results available here:
    https://tbpl.mozilla.org/?tree=Try&rev=b5fd4bb0ee8c
Results (out of 2 total builds):
    failure: 2
Builds available at http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/blassey@mozilla.com-b5fd4bb0ee8c
(Reporter)

Comment 6

6 years ago
(In reply to Brad Lassey [:blassey] from comment #2)
> Created attachment 578978 [details] [diff] [review] [diff] [details] [review]
> patch to ship a simple youtube player

In my email I implied that this youtube player was not the same as presenting a webm video. Is that true? What video are we sourcing here?
(In reply to Alex Keybl [:akeybl] from comment #6)
> (In reply to Brad Lassey [:blassey] from comment #2)
> > Created attachment 578978 [details] [diff] [review] [diff] [details] [review] [diff] [details] [review]
> > patch to ship a simple youtube player
> 
> In my email I implied that this youtube player was not the same as
> presenting a webm video. Is that true? What video are we sourcing here?

this patch is using the h264 stream.

Comment 8

6 years ago
Comment on attachment 578978 [details] [diff] [review]
patch to ship a simple youtube player

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

::: embedding/android/AndroidManifest.xml.in
@@ +125,5 @@
>                  <category android:name="android.intent.category.DEFAULT" />
>              </intent-filter>
>          </activity>
> +	<activity android:name="org.mozilla.gecko.YouTubePlayer"
> +                  android:label="YouTubePlayer"

localized string.  Can we not call this YouTube specifically?  Maybe we can reuse this activity for different players.  Also, it gets out out of worrying about Trademarks.

::: embedding/android/GeckoAppShell.java
@@ +813,5 @@
> +                String[] handlers = getHandlersForURL(aUriSpec, aAction);
> +                if (handlers.length == 0) {
> +                    intent = new Intent(Intent.ACTION_MAIN);
> +                    intent.setClassName(GeckoApp.mAppContext.getPackageName(),
> +                                        "org.mozilla.gecko.YouTubePlayer");

Again, the name of this class should probably be VideoPlayer or something more general.

::: embedding/android/YouTubePlayer.java
@@ +1,1 @@
> +package org.mozilla.gecko;

missing license header.

@@ +40,5 @@
> +    try {
> +      String info_uri = "http://www.youtube.com/get_video_info?&video_id=" + id;
> +      URL url = new URL(info_uri);
> +      URLConnection urlConnection = url.openConnection();
> +      InputStream in = new BufferedInputStream(urlConnection.getInputStream());

This is all blocking.  Although, I didn't see a real problem, I am not sure we should be doing this on the main thread.  It could be that I am on a very fast network and didn't see any hang or something.

@@ +45,5 @@
> +      try {
> +        int i;
> +        StringBuffer sb = new StringBuffer();
> +        while (-1 != (i = in.read()))
> +          sb.append((char)i);

Is this the best way to convert a InputStream to a StringBuffer?  Do you know the max size of the response and preallocate a byte array?

@@ +52,5 @@
> +        for (i = 0; i < data.length; i++) {
> +          String[] key_val = data[i].split("=");
> +	  if (key_val.length == 2)
> +	    map.put(key_val[0], URLDecoder.decode(key_val[1]));
> +        }

I really don't know what the response of this get is.  I kinda wish it was just a json blob.  If we had that, most of this parsing goes away.  Is there such a thing?  http://code.google.com/intl/da-DK/apis/youtube/2.0/developers_guide_json.html
Attachment #578978 - Flags: review?(doug.turner) → review-
(Reporter)

Comment 9

6 years ago
Here's Tony's feedback from the try build:

"Tested against brad's 12/4 try server build + youtube plugin.

- launching youtube video prompts "Launch appliction - This link needs to be opened with an application".  
** it would be nice if we can offer the Android sharing menu with a youtube player option to choose from.

- video playback in youtube player is not centered.  video is upper left justified.  exists on any orientation.

- find a youtube video where the screen image is maximizes the viewport.   Video playback does not display in full screen.

- switch video from landscape to portrait orientation during playback.   Not only does the video recalibrate viewport, but at times will leave black boxes

- Youtube player video (from Silk) will hide the system menu at the bottom.   Loading it from fennec does not have the minimized system control menu.

- Back button on youtube player doesnt take one click to return back to Fennec.  If you make touch actions within the video player controls, those will be recorded part of the history.  (eg. click play, pause, then play - 3 touch actions.  Now if you hit back, you'll have to triple click it before returning to fennec)   The more users click the controls, the less they will know how to get back to fennec due to larger click history."
(Reporter)

Comment 10

6 years ago
(In reply to Alex Keybl [:akeybl] from comment #9)
> Here's Tony's feedback from the try build:
> 
> "Tested against brad's 12/4 try server build + youtube plugin.
> 
> - launching youtube video prompts "Launch appliction - This link needs to be
> opened with an application".  
> ** it would be nice if we can offer the Android sharing menu with a youtube
> player option to choose from.

We're not considering this a blocker.

> - video playback in youtube player is not centered.  video is upper left
> justified.  exists on any orientation.

This is fixed in the latest patch.

> - find a youtube video where the screen image is maximizes the viewport.  
> Video playback does not display in full screen.

Can you clarify Tony?

> - switch video from landscape to portrait orientation during playback.   Not
> only does the video recalibrate viewport, but at times will leave black boxes

We're not considering this a blocker.

> - Youtube player video (from Silk) will hide the system menu at the bottom. 
> Loading it from fennec does not have the minimized system control menu.

We're not considering this a blocker.

> - Back button on youtube player doesnt take one click to return back to
> Fennec.  If you make touch actions within the video player controls, those
> will be recorded part of the history.  (eg. click play, pause, then play - 3
> touch actions.  Now if you hit back, you'll have to triple click it before
> returning to fennec)   The more users click the controls, the less they will
> know how to get back to fennec due to larger click history."

We're going to try to get this fixed before landing - hopefully today before going to build for beta.
Comment on attachment 578978 [details] [diff] [review]
patch to ship a simple youtube player


>diff --git a/embedding/android/AndroidManifest.xml.in b/embedding/android/AndroidManifest.xml.in

>+	<activity android:name="org.mozilla.gecko.YouTubePlayer"
>+                  android:label="YouTubePlayer"

Doesn't look like we need a "label" and lets use "org.mozilla.gecko.VideoPlayer" here. Therefore we won't have any strings to localize

In fact, I like using "VideoPlayer" and variants in all places we are currently using "YouTubePlayer", just to keep thing general.

Your indenting in this file is a bit off too

>diff --git a/embedding/android/GeckoAppShell.java b/embedding/android/GeckoAppShell.java

>+                    intent.setClassName(GeckoApp.mAppContext.getPackageName(),
>+                                        "org.mozilla.gecko.YouTubePlayer");

org.mozilla.gecko.VideoPlayer

>diff --git a/embedding/android/Makefile.in b/embedding/android/Makefile.in

>+  YouTubePlayer.java \

VideoPlayer.java

>+  res/layout/youtubeplayer.xml \

videoplayer.xml

>diff --git a/embedding/android/YouTubePlayer.java b/embedding/android/YouTubePlayer.java

Indenting in here is 2 spaces, but we use 4 for Java files

>+public class YouTubePlayer extends Activity

VideoPlayer

>+{
>+  /** Called when the activity is first created. */
>+  @Override
>+  public void onCreate(Bundle savedInstanceState)
>+  {
>+    super.onCreate(savedInstanceState);
>+    setContentView(R.layout.youtubeplayer);
>+    mVideoView = (VideoView) findViewById(R.id.VideoView);
>+    MediaController mediaController = new MediaController(this);
>+    mediaController.setAnchorView(mVideoView);
>+    // Set video link (mp4 format )
>+    Intent intent = getIntent();
>+    Uri data = intent.getData();
>+    String ssp = data.getSchemeSpecificPart();
>+    String id = ssp.substring(0, ssp.indexOf('?'));
>+    String spec = getSpecFromVideoID(id);

Do you want to verify that spec is not "" (failure case)

>+  String getSpecFromVideoID(String id) {

Can you add an example of what the URL returns? So people know what the format of the text is we are parsing here

>+        for (i = 0; i < data.length; i++) {
>+          String[] key_val = data[i].split("=");
>+	  if (key_val.length == 2)

TAB snuck in here

>+	    if (key_val.length == 2) {
>+	      Log.i("YouTubePlayer", key_val[0] + ": " + URLDecoder.decode(key_val[1]));

VideoPlayer

>+	  if (mime != null && mime.startsWith("video/mp4") && uri != null) {
>+	    spec = uri;
>+	  }
>+	}

TAB here too

>+    } catch (Exception e) {
>+      Log.e("YouTubePlayer", "exception", e);

VideoPlayer

I agree with Doug about moving to a simpler parsing format, like JSON - but I don't think we should hold up landing and testing the feature on that

Let's file a followup to investigate JSON or other formats.

>diff --git a/embedding/android/resources/layout/youtubeplayer.xml b/embedding/android/resources/layout/youtubeplayer.xml

>+<LinearLayout xmlns:android="http://schemas.android.com/apk/res/android"
>+    android:orientation="vertical"
>+    android:layout_width="fill_parent"
>+    android:layout_height="fill_parent"
>+    >
>+<VideoView android:layout_height="fill_parent"
>+android:layout_width="fill_parent" android:id="@+id/VideoView"></VideoView>

VideoView needs some indenting love

r+ with the nits fixed
Attachment #578978 - Flags: review?(mark.finkle) → review+
(In reply to Alex Keybl [:akeybl] from comment #10)
> > - find a youtube video where the screen image is maximizes the viewport.  
> > Video playback does not display in full screen.
> 
> Can you clarify Tony?
> >

yeah, poorly worded.  basically, video does not stretch entirely across the device display, when redirected from fennec.

It does stretch correctly if you redirect from silk.

Tony
> - Back button on youtube player doesnt take one click to return back to
> Fennec.  If you make touch actions within the video player controls, those
> will be recorded part of the history.  (eg. click play, pause, then play - 3
> touch actions.  Now if you hit back, you'll have to triple click it before
> returning to fennec)   The more users click the controls, the less they will
> know how to get back to fennec due to larger click history."

I don't see what you describe here. If the controls are up, hitting back dismisses them. Hitting back when the controls aren't up brings you back to fennec.
Created attachment 579586 [details] [diff] [review]
patch
Attachment #578978 - Attachment is obsolete: true
Attachment #579586 - Flags: review?(doug.turner)
Created attachment 579589 [details] [diff] [review]
patch

Doug suggested using android.net.Uri to do the parsing, this patch does that
Attachment #579586 - Attachment is obsolete: true
Attachment #579586 - Flags: review?(doug.turner)
Attachment #579589 - Flags: review?(doug.turner)
Comment on attachment 579589 [details] [diff] [review]
patch

>@@ -34,11 +34,11 @@
>     <uses-feature android:name="android.hardware.camera.autofocus" android:required="false"/>
>  
>     <application android:label="@MOZ_APP_DISPLAYNAME@"
>-		 android:icon="@drawable/icon"
>+                 android:icon="@drawable/icon"
> #if MOZILLA_OFFICIAL
>-		 android:debuggable="false">
>+                 android:debuggable="false">
> #else
>-		 android:debuggable="true">
>+                 android:debuggable="true">
> #endif
> 
>         <activity android:name="App"
>@@ -113,7 +113,7 @@
>           <intent-filter>
>             <action android:name="org.mozilla.gecko.reportCrash" />
>           </intent-filter>
>-	</activity>
>+        </activity>
> #endif
> 
>         <activity android:name="LauncherShortcuts"
Note, I removed the tabs on these lines, hense the ws changes

Updated

6 years ago
Attachment #579589 - Flags: review?(doug.turner) → review+
(Assignee)

Updated

6 years ago
Attachment #579589 - Flags: approval-mozilla-beta?
Attachment #579589 - Flags: approval-mozilla-aurora?
(In reply to Brad Lassey [:blassey] from comment #13)
> > - Back button on youtube player doesnt take one click to return back to
> > Fennec.  If you make touch actions within the video player controls, those
> > will be recorded part of the history.  (eg. click play, pause, then play - 3
> > touch actions.  Now if you hit back, you'll have to triple click it before
> > returning to fennec)   The more users click the controls, the less they will
> > know how to get back to fennec due to larger click history."
> 
> I don't see what you describe here. If the controls are up, hitting back
> dismisses them. Hitting back when the controls aren't up brings you back to
> fennec.

hmm, i rebooted the device and i can't reproduce the multiple backs anymore.   back dismisses the onscreen controls, and then skips back to fennec as you described.  send me a build with the video centering fix, and i'll be happy to validate it.
(Reporter)

Comment 18

6 years ago
Comment on attachment 579589 [details] [diff] [review]
patch

[Triage Comment]
Should only affect behavior on Kindle Fire and other devices without the YouTube system app, and may pave the way for submitting to the Amazon Appstore. Let's land on aurora and beta.
Attachment #579589 - Flags: approval-mozilla-beta?
Attachment #579589 - Flags: approval-mozilla-beta+
Attachment #579589 - Flags: approval-mozilla-aurora?
Attachment #579589 - Flags: approval-mozilla-aurora+
pushed https://hg.mozilla.org/mozilla-central/rev/658fad825c36, https://hg.mozilla.org/releases/mozilla-aurora/rev/ca986aa710c7 and https://hg.mozilla.org/releases/mozilla-beta/rev/2bbfe14f8b8d
Status: NEW → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
Verified fix on beta 5, Kindle Fire.  youtube player launches when play button is clicked. 

Mozilla/5.0 (Android; Linux armv7l; rv:9.0 Gecko/2011011206 Firefox/9.0 Fennec/9.0
Keywords: verified-beta
(Reporter)

Updated

6 years ago
Depends on: 708283

Updated

6 years ago
Depends on: 708532

Updated

6 years ago
Depends on: 708534

Updated

6 years ago
Depends on: 708535

Updated

6 years ago
Depends on: 708536

Comment 21

6 years ago
Build id : Mozilla/5.0 (Android; Linux armv7l; rv:9.0 Gecko/2011011206 Firefox/9.0 Fennec/9.0
Devices: Samsung Galasy SII and Acer Iconia (2.3.4 and 3.2)

- youtube app plays the video after the play button is pressed. No pop up or notification appears.

Updated

6 years ago
Status: RESOLVED → VERIFIED
This caused a regression in that it creates a second application shortcut in the Android launcher; which when invoked causes an NPE: bug 708574

Updated

6 years ago
Depends on: 708574
(Assignee)

Updated

5 years ago
status-firefox10: --- → fixed
status-firefox11: --- → fixed
status-firefox9: --- → fixed
(Reporter)

Updated

5 years ago
Blocks: 690791
You need to log in before you can comment on or make changes to this bug.