1006186 - Casting apps should be able to specify supported types themselves

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: Screencasting, defect)

Description

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

Attachment: unregister_acceptable_types.patch

Comment 1

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

Attachment: unregister_acceptable_types.patch

Comment 2

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

Comment on attachment 8417708 [details] [diff] [review]
unregister_acceptable_types.patch

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

::: mobile/android/chrome/content/CastingApps.js
@@ +1,4 @@
>  /* This Source Code Form is subject to the terms of the Mozilla Public
>   * License, v. 2.0. If a copy of the MPL was not distributed with this file,
>   * You can obtain one at http://mozilla.org/MPL/2.0/. */
> +//"use strict";

this is left over cruft

Comment 3

[Mark Finkle (:mfinkle) (use needinfo?)]

Comment on attachment 8417708 [details] [diff] [review]
unregister_acceptable_types.patch

Can you split the unregisterTarget part out of the types part? I'd r+ an unregisterTarget (with no types stuff) quickly, but the types stuff will take more thought.

Comment 4

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

Attachment: acceptable_types.patch

Comment 5

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

Attachment: acceptable_types.patch

Comment 6

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

Attachment: acceptable_types.patch

Comment 7

[Mark Finkle (:mfinkle) (use needinfo?)]

Comment on attachment 8442980 [details] [diff] [review]
acceptable_types.patch


>diff --git a/mobile/android/chrome/content/CastingApps.js b/mobile/android/chrome/content/CastingApps.js

>     return new RokuApp(aService);
>+  },
>+  types: ['video/mp4'],
>+  exts: ['mp4']
>+};

I am not a fan of doing this as two separate arrays. I would be happier if we found a way to combine the two together, maybe like this:

  supports: [{ type: "video/mp4", extension: "mp4" }]

BUT I see a problem where a single type might have two different extensions ("image/jpeg" -> "jpg" and "jpeg") and the supports way is a bit more verbose. Therefore, I am going to let this drop and only request:

* exts -> extensions
* use " not '

>+
>+function allowableExtension(aURI, exts) {
>+  if (aURI && aURI instanceof Ci.nsIURL) {
>+    for (let x in exts) {
>+      if (aURI.fileExtension == exts[x]) return true;
>+    }
>   }
>-};
>+  return false;
>+}
>+
>+function allowableMimeType(aType, types) {
>+  for (let x in types) {
>+    if (aType == types[x]) return true;
>+  }
>+  return false;
>+}
>+      

Can these be moved into the CastingApp object as member methods? If not, at least move them above the Target objects or between the Targets and CastingApps.

>-    if (!this.getVideo(video, 0, 0)) {
>+    let supportedExtensions = SimpleServiceDiscovery.getSupportedExtensions();
>+    let supportedMimeTypes = SimpleServiceDiscovery.getSupportedMimeTypes();
>+
>+    if (!this.getVideo(video, 0, 0, supportedMimeTypes, supportedExtensions)) {

You use this pattern in a few places (getting the arrays and passing them to getVideo. Why not just get the arrays in getVideo?

>+  getVideo: function(aElement, aX, aY, types, exts) {

Drop the new params and just get the arrays here. I assume you want to pass them to _getVideo for performance reasons? Maybe that's over-optimization and we should just let _getVideo pull the arrays from SimpleServiceDiscovery?

>-  prompt: function(aCallback) {
>+  prompt: function(aCallback, test) {

* rename: test -> aFilterFunc

>+    function testFunc(service) {
>+      return allowableExtension(video.sourceURI, service.exts) || allowableMimeType(video.type, service.types);
>+    }

rename: testFunc -> filterFunc

>diff --git a/mobile/android/modules/SimpleServiceDiscovery.jsm b/mobile/android/modules/SimpleServiceDiscovery.jsm

>+  _supportedExtensions: new Array(),
>+  _supportedMimeTypes: new Array(),

for arrays, we use:

  _supportedExtensions: [],
  _supportedMimeTypes: [],


>   _searchSocket: null,
>   _searchInterval: 0,
>   _searchTimestamp: 0,
>@@ -219,6 +221,14 @@ var SimpleServiceDiscovery = {
>     }
>   },
> 
>+  getSupportedExtensions: function() {
>+    return this._supportedExtensions;
>+  },
>+
>+  getSupportedMimeTypes: function() {
>+    return this._supportedMimeTypes;
>+  },
>+
>   registerTarget: function registerTarget(aTarget) {
>     // We must have "target" and "factory" defined
>     if (!("target" in aTarget) || !("factory" in aTarget)) {
>@@ -229,6 +239,8 @@ var SimpleServiceDiscovery = {
>     // Only add if we don't already know about this target
>     if (!this._targets.has(aTarget.target)) {
>       this._targets.set(aTarget.target, aTarget);
>+      this._supportedMimeTypes = this._supportedMimeTypes.concat(aTarget.types);
>+      this._supportedExtensions = this._supportedExtensions.concat(aTarget.exts);

This will add duplicates to the data members. I wonder if we should just not try to cache and build the arrays in the getters

>     // Only remove if we know about this target
>     if (this._targets.has(aTarget.target)) {
>       this._targets.delete(aTarget.target);
>+      this._targets.forEach(function(target) {
>+	      this._supportedMimeTypes = this._supportedMimeTypes.concat(target.types);
>+	      this._supportedExtensions = this._supportedExtensions.concat(target.exts);

TABs in the indents and again, this will add dupes. I suggest just using the forEach loop to create an array in the getters

Comment 8

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

Attachment: acceptable_types.patch

Comment 9

[Mark Finkle (:mfinkle) (use needinfo?)]

Comment on attachment 8458654 [details] [diff] [review]
acceptable_types.patch

>diff --git a/mobile/android/chrome/content/CastingApps.js b/mobile/android/chrome/content/CastingApps.js

> var mediaPlayerTarget = {
>@@ -172,9 +177,12 @@ var CastingApps = {
>     return Services.io.newURI(aURL, aOriginCharset, aBaseURI);
>   },

As you pointed out, we need to add some types and extensions to mediaPlayerTarget too


>-  getVideo: function(aElement, aX, aY) {
>+  getVideo: function(aElement, aX, aY, types, exts) {
>+    let exts = SimpleServiceDiscovery.getSupportedExtensions();
>+    let types = SimpleServiceDiscovery.getSupportedMimeTypes();

We can remove the unused args passed to getVideo

>-  _getVideo: function(aElement) {
>+  _getVideo: function(aElement, types, exts) {

We should switch to aTypes and aExtensions to match style

>     if (!(aElement instanceof HTMLVideoElement)) {
>       return null;
>     }
> 
>-    // Given the hardware support for H264, let's only look for 'mp4' sources
>-    function allowableExtension(aURI) {
>-      if (aURI && aURI instanceof Ci.nsIURL) {
>-        return (aURI.fileExtension == "mp4");
>-      }
>-      return false;
>-    }
> 
>     // Grab the poster attribute from the <video>
>     let posterURL = aElement.poster;
>@@ -228,8 +229,8 @@ var CastingApps = {
>     if (sourceURL) {
>       // Use the file extension to guess the mime type
>       let sourceURI = this.makeURI(sourceURL, null, this.makeURI(aElement.baseURI));
>-      if (allowableExtension(sourceURI)) {
>-        return { element: aElement, source: sourceURI.spec, poster: posterURL };
>+      if (this.allowableExtension(sourceURI, exts)) {
>+        return { element: aElement, source: sourceURI.spec, poster: posterURL, sourceURI: sourceURI};
>       }
>     }
> 
>@@ -241,8 +242,8 @@ var CastingApps = {
> 
>       // Using the type attribute is our ideal way to guess the mime type. Otherwise,
>       // fallback to using the file extension to guess the mime type
>-      if (sourceNode.type == "video/mp4" || allowableExtension(sourceURI)) {
>-        return { element: aElement, source: sourceURI.spec, poster: posterURL };
>+      if (this.allowableMimeType(sourceNode.type, types) || this.allowableExtension(sourceURI, exts)) {
>+        return { element: aElement, source: sourceURI.spec, poster: posterURL, sourceURI: sourceURI, type: sourceNode.type };
>       }
>     }
> 
>@@ -361,21 +362,25 @@ var CastingApps = {
>     }
>   },
> 
>-  prompt: function(aCallback) {
>+  prompt: function(aCallback, aFilterFunc) {
>     let items = [];
>+    let filteredServices = [];
>     SimpleServiceDiscovery.services.forEach(function(aService) {
>       let item = {
>         label: aService.friendlyName,
>         selected: false
>       };
>-      items.push(item);
>+      if (!aFilterFunc || aFilterFunc(aService)) {
>+        filteredServices.push(aService);
>+        items.push(item);
>+      }
>     });
> 
>     let prompt = new Prompt({
>       title: Strings.browser.GetStringFromName("casting.prompt")
>     }).setSingleChoiceItems(items).show(function(data) {
>       let selected = data.button;
>-      let service = selected == -1 ? null : SimpleServiceDiscovery.services[selected];
>+      let service = selected == -1 ? null : filteredServices[selected];
>       if (aCallback)
>         aCallback(service);
>     });
>@@ -394,6 +399,10 @@ var CastingApps = {
>       return;
>     }
> 
>+    function testFunc(service) {
>+      return this.allowableExtension(video.sourceURI, service.extensions) || this.allowableMimeType(video.type, service.types);
>+    }
>+
>     this.prompt(function(aService) {
>       if (!aService)
>         return;
>@@ -438,7 +447,7 @@ var CastingApps = {
>           }.bind(this), this);
>         }.bind(this));
>       }.bind(this));
>-    }.bind(this));
>+    }.bind(this), testFunc.bind(this));
>   },
> 
>   closeExternal: function() {
>@@ -487,5 +496,21 @@ var CastingApps = {
>     if (status == "completed") {
>       this.closeExternal();
>     }
>+  },
>+
>+  allowableExtension: function(aURI, exts) {
>+    if (aURI && aURI instanceof Ci.nsIURL) {
>+      for (let x in exts) {
>+        if (aURI.fileExtension == exts[x]) return true;
>+      }
>+    }
>+    return false;
>+  },
>+
>+  allowableMimeType: function(aType, types) {
>+    for (let x in types) {
>+      if (aType == types[x]) return true;
>+    }
>+    return false;
>   }
> };
>diff --git a/mobile/android/modules/SimpleServiceDiscovery.jsm b/mobile/android/modules/SimpleServiceDiscovery.jsm
>--- a/mobile/android/modules/SimpleServiceDiscovery.jsm
>+++ b/mobile/android/modules/SimpleServiceDiscovery.jsm
>@@ -1,4 +1,4 @@
>-// -*- indent-tabs-mode: nil; js-indent-level: 2 -*-
>+// -*- Mode: js; tab-width: 2; indent-tabs-mode: nil; js2-basic-offset: 2; js2-skip-preprocessor-directives: t; -*-
> /* This Source Code Form is subject to the terms of the Mozilla Public
>  * License, v. 2.0. If a copy of the MPL was not distributed with this file,
>  * You can obtain one at http://mozilla.org/MPL/2.0/. */
>@@ -51,6 +51,8 @@ var SimpleServiceDiscovery = {
> 
>   _targets: new Map(),
>   _services: new Map(),
>+  _supportedExtensions: [],
>+  _supportedMimeTypes: [],

Based on the code in the getters, we don't need the member arrays anymore

>+  getSupportedExtensions: function() {

>+    this._targets.forEach(function(target) {
>+        exts = this._supportedExtensions.concat(target.extensions);
>+      }, this);

Although, we will change this to:

          exts = exts.concat(target.extensions);

>+  getSupportedMimeTypes: function() {

Same pattern here

>   registerTarget: function registerTarget(aTarget) {

>+      this._supportedMimeTypes = this._supportedMimeTypes.concat(aTarget.types);
>+      this._supportedExtensions = this._supportedExtensions.concat(aTarget.extensions);

Not needed

>       this._targets.delete(aTarget.target);
>+      this._targets.forEach(function(target) {
>+        this._supportedMimeTypes = this._supportedMimeTypes.concat(target.types);
>+        this._supportedExtensions = this._supportedExtensions.concat(target.exts);
>+      }, this);

Not needed

r+, and I can make the changes and land it if that's OK

Comment 10

[Mark Finkle (:mfinkle) (use needinfo?)]

https://hg.mozilla.org/integration/fx-team/rev/e66b5696189e

Comment 11

[Mark Finkle (:mfinkle) (use needinfo?)]

Just in case, for tests:
https://hg.mozilla.org/integration/fx-team/rev/14fe614bac3b

Comment 12

[Mark Finkle (:mfinkle) (use needinfo?)]

Fixup the discovery tests since we support webm now:
https://hg.mozilla.org/integration/fx-team/rev/fbedb66e0ebf

Comment 13

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/e66b5696189e
https://hg.mozilla.org/mozilla-central/rev/14fe614bac3b
https://hg.mozilla.org/mozilla-central/rev/fbedb66e0ebf