Last Comment Bug 705572 - Kindle Fire: YouTube videos do not open in unavailable YouTube App
: Kindle Fire: YouTube videos do not open in unavailable YouTube App
Status: VERIFIED FIXED
: verified-beta
Product: Fennec Graveyard
Classification: Graveyard
Component: General (show other bugs)
: Firefox 9
: All All
: -- normal (vote)
: ---
Assigned To: Brad Lassey [:blassey] (use needinfo?)
:
Mentors:
Depends on: 708532 708534 708535 708536 708283 708574
Blocks: 690791
  Show dependency treegraph
 
Reported: 2011-11-27 15:20 PST by Alex Keybl [:akeybl]
Modified: 2013-02-27 13:15 PST (History)
7 users (show)
See Also:
QA Whiteboard:
Iteration: ---
Points: ---


Attachments
patch to ship a simple youtube player (6.15 KB, patch)
2011-12-04 21:16 PST, Brad Lassey [:blassey] (use needinfo?)
no flags Details | Diff | Review
patch to ship a simple youtube player (6.22 KB, patch)
2011-12-04 21:21 PST, Brad Lassey [:blassey] (use needinfo?)
dougt: review-
mark.finkle: review+
Details | Diff | Review
patch (9.66 KB, patch)
2011-12-06 20:43 PST, Brad Lassey [:blassey] (use needinfo?)
no flags Details | Diff | Review
patch (8.90 KB, patch)
2011-12-06 21:32 PST, Brad Lassey [:blassey] (use needinfo?)
dougt: review+
akeybl: approval‑mozilla‑aurora+
akeybl: approval‑mozilla‑beta+
Details | Diff | Review

Description Alex Keybl [:akeybl] 2011-11-27 15:20:10 PST
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.
Comment 1 Brad Lassey [:blassey] (use needinfo?) 2011-12-04 21:16:42 PST
Created attachment 578976 [details] [diff] [review]
patch to ship a simple youtube player
Comment 2 Brad Lassey [:blassey] (use needinfo?) 2011-12-04 21:21:45 PST
Created attachment 578978 [details] [diff] [review]
patch to ship a simple youtube player
Comment 3 Brad Lassey [:blassey] (use needinfo?) 2011-12-05 10:45:17 PST
pushed to try https://tbpl.mozilla.org/?tree=Try&rev=7feacaa09970
Comment 4 Mozilla RelEng Bot 2011-12-05 10:50:29 PST
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 Mozilla RelEng Bot 2011-12-05 11:10:26 PST
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
Comment 6 Alex Keybl [:akeybl] 2011-12-05 12:50:44 PST
(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?
Comment 7 Brad Lassey [:blassey] (use needinfo?) 2011-12-05 13:09:25 PST
(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 Doug Turner (:dougt) 2011-12-06 11:22:07 PST
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
Comment 9 Alex Keybl [:akeybl] 2011-12-06 13:06:11 PST
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."
Comment 10 Alex Keybl [:akeybl] 2011-12-06 13:56:19 PST
(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 11 Mark Finkle (:mfinkle) (use needinfo?) 2011-12-06 14:06:54 PST
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
Comment 12 Tony Chung [:tchung] 2011-12-06 14:08:47 PST
(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
Comment 13 Brad Lassey [:blassey] (use needinfo?) 2011-12-06 20:40:53 PST
> - 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.
Comment 14 Brad Lassey [:blassey] (use needinfo?) 2011-12-06 20:43:14 PST
Created attachment 579586 [details] [diff] [review]
patch
Comment 15 Brad Lassey [:blassey] (use needinfo?) 2011-12-06 21:32:48 PST
Created attachment 579589 [details] [diff] [review]
patch

Doug suggested using android.net.Uri to do the parsing, this patch does that
Comment 16 Brad Lassey [:blassey] (use needinfo?) 2011-12-06 21:35:01 PST
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
Comment 17 Tony Chung [:tchung] 2011-12-06 21:40:41 PST
(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.
Comment 18 Alex Keybl [:akeybl] 2011-12-06 21:41:24 PST
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.
Comment 20 Tony Chung [:tchung] 2011-12-07 09:28:55 PST
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
Comment 21 Ioana Chiorean 2011-12-08 06:43:02 PST
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.
Comment 22 Aaron Train [:aaronmt] 2011-12-08 07:21:25 PST
This caused a regression in that it creates a second application shortcut in the Android launcher; which when invoked causes an NPE: bug 708574

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