1009909 - Firefox desktop: Integrate the openh264 media plugin in the add-ons manager

Status: VERIFIED FIXED

(Toolkit :: Add-ons Manager, defect)

Description

[Benjamin Smedberg]

Implement the UX which exposes openh264 to the user as specified in bug 1007694. The user must be able to disable the plugin, and if automatic updates are disabled, must be able to trigger a check for updates.

Comment 2

[Brian R. Bondy [:bbondy]]

I'll need some way to know if a plugin has been uninstalled from the UI. Please let me know this info once it is determined.  This is so that bug 1009816 doesn't re-install the plugin.

Comment 3

[Dão Gottwald [:dao]]

(In reply to Brian R. Bondy [:bbondy] from comment #2)
> I'll need some way to know if a plugin has been uninstalled from the UI.

Don't the existing add-on manager APIs (AddonManager.jsm?) provide this for plugins? Or is openh264 not a normal plugin? If so, how is it different?

Comment 4

[Brian R. Bondy [:bbondy]]

(In reply to Dão Gottwald [:dao] from comment #3)
> (In reply to Brian R. Bondy [:bbondy] from comment #2)
> > I'll need some way to know if a plugin has been uninstalled from the UI.
> 
> Don't the existing add-on manager APIs (AddonManager.jsm?) provide this for
> plugins? 

I don't think so, but I'm not sure.

> Or is openh264 not a normal plugin? If so, how is it different?

It is not a normal plugin, it is a Gecko Media plugin. 
https://wiki.mozilla.org/GeckoMediaPlugins

Comment 5

[Dão Gottwald [:dao]]

I don't think this bug is actionable without the add-on manager core gaining support for a new "Gecko Media plugin" add-on type. I think Blair wanted to add this in bug 950933, but I'm not sure.

Comment 6

[Benjamin Smedberg]

Dão, this is the bug for implementing the addon manager core, but specifically for OpenH264 because that is time-sensitive and must hit FF33. Because these addons are supposed to be part of the plugins pane and not a separate pane, I believe this involves changes to the existing PluginProvider and not a new provider.

Bug 950933 is for the future EME media plugins, which don't have a UI spec yet.

Comment 7

[Dão Gottwald [:dao]]

(In reply to Benjamin Smedberg  [:bsmedberg] from comment #6)
> Dão, this is the bug for implementing the addon manager core, but
> specifically for OpenH264 because that is time-sensitive and must hit FF33.

Blair would probably the best owner for this then. (Adjusting the summary, since it's misleading.)

> Because these addons are supposed to be part of the plugins pane and not a
> separate pane, I believe this involves changes to the existing
> PluginProvider and not a new provider.

Maybe? I don't know, but my assumption is that a separate provider for a new add-on type needs to be added. I also expect that listing multiple add-on types in one pane is doable.

> Bug 950933 is for the future EME media plugins, which don't have a UI spec
> yet.

So would the EME media plugin not be a Gecko Media plugin? Whether some particular plugins need special treatment in the UI seems like a separate question to me. Blair did mention media decoder plugins in bug 950933 comment 0.

Comment 8

[Georg Fritzsche [:gfritzsche]]

I'm taking this over for now for the PluginProvider integration and will lean on Dao if i get stuck with the UI.

Comment 9

[Georg Fritzsche [:gfritzsche]]

Open questions, going with a minimal, OpenH264-only implementation for this bug:
* How to get the OpenH264 version? I only found mozIGeckoMediaPluginService, which doesn't help me.
* While we don't have OpenH264 downloaded and installed (or it was user-removed etc.), do we still show the entry?
* How can we tell when it is installed? Any pref or interface to check?
* How can we tell when the plugin got installed mid-session? Blocked by bug 1009816?
* How do we initiate an update check? Blocked by bug 1009816?

Comment 10

[Georg Fritzsche [:gfritzsche]]

I think there is no open question for Blair right now.

Comment 11

[Benjamin Smedberg]

> * While we don't have OpenH264 downloaded and installed (or it was
> user-removed etc.), do we still show the entry?

Yes.

> * How do we initiate an update check? Blocked by bug 1009816?

Yes, or a followup. There needs to be an API for initiating an update check.

Comment 12

[(Currently slow to respond) Philipp Sackl [:phlsa] (Firefox UX) please use needinfo]

(In reply to Benjamin Smedberg  [:bsmedberg] from comment #11)
> > * While we don't have OpenH264 downloaded and installed (or it was
> > user-removed etc.), do we still show the entry?
> 
> Yes.

Will it be possible to uninstall the plugin altogether (as opposed to disabling it)?
If so, why would we still show the entry in that case?

If it is still downloading, we could show the entry with a progress bar instead of the buttons on the right.

Comment 13

[Benjamin Smedberg]

That's really a UI question for you, although if we did support some option which hid it, we'd also need to have a way to get it back, which I don't think is worth it. I think the only options should be enable/disable.

Comment 14

[(Currently slow to respond) Philipp Sackl [:phlsa] (Firefox UX) please use needinfo]

(In reply to Benjamin Smedberg  [:bsmedberg] from comment #13)
> That's really a UI question for you, although if we did support some option
> which hid it, we'd also need to have a way to get it back, which I don't
> think is worth it. I think the only options should be enable/disable.

If there are no pressing reasons to do otherwise, we should only allow disabling through the UI, not a complete deinstallation.

Comment 15

[(Currently slow to respond) Philipp Sackl [:phlsa] (Firefox UX) please use needinfo]

For the not-yet-installed-case:

We can replace the plugin description (the line below the name of the plugin) in the list with something like »Will be installed shortly« in italics.

The »More«-link should be there already.

Comment 16

[Georg Fritzsche [:gfritzsche]]

Thanks, remaining open questions for the ni?s:
* How to get the OpenH264 version? I only found mozIGeckoMediaPluginService, which doesn't help me.
* How can we tell when the plugin is installed? Any pref or interface to check?
* How can we tell when the plugin got installed mid-session? Blocked by bug 1009816?

Comment 17

[Brian R. Bondy [:bbondy]]

(In reply to Georg Fritzsche [:gfritzsche] from comment #16)
> * How can we tell when the plugin got installed mid-session? Blocked by bug
> 1009816?

This shouldn't be blocked in any way by bug 1009816. 
Bug 1009816 will ensure that the addon update offered by AUS gets installed into:

Windows:
'Path' entry for child entries of registry entry 'Software\\MozillaPlugins' 

OS X: 
~/Library/Internet Plugins/
 
Linux/Unix/Other:
/usr/lib/mozilla/plugins/ 

In the implementation I may store a pref that says what the last downloaded version was, but I don't think this should be the source of truth for GMP Plugin versioning or installed detection.  Things can be installed manually and possibly by other packaging.

If we want some kind of registry of what's registered and eligible for loading, then that needs to be documented in
https://wiki.mozilla.org/GeckoMediaPlugins and implemented in another bug, and also respected by only loading those things by the plugin loading code.

I'll also need a way to know if it was ever uninstalled by the way from this bug. (So I don't re-install it on the next update version).

----

Here are my local notes for manually installing by the way:

about:config
Set media.peerconnection.video.h264_enabled to true

Clone https://github.com/cisco/openh264
$ make plugin

Follow the instructions for missing dependencies from the build output and build again.

Copy the built libgmpopenh264.so/dylib and gmpopenh264.info into
~/Library/Internet Plug-Ins/gmp-gmpopenh264

Visit http://mozilla.github.io/webrtc-landing/pc_test_h264.html and press "Start!”

Use Activity Monitor to see if it starts:
Nightly Plugin Process
Disable other plug-ins like Flash so they don’t show up as a child process.

If you see it, try moving the directory away from /Users/brianbondy/Library/Internet Plug-Ins/ and then restart Firefox and try again.
Nightly Plugin Process should not start this time.

Comment 18

[Brian R. Bondy [:bbondy]]

> * How can we tell when the plugin got installed mid-session? Blocked by bug 1009816?

I don't know how the plugin loading code works, but my code will ensure it's at the places specified above. I'm not sure if that means it's eligible or not to be used mid-session without a browser restart.  This may be another bug we need to post if we want it and don't currently have support for it.

Comment 19

[Brian R. Bondy [:bbondy]]

If you want by the way, I can add the pref of what version was installed by AUS, and document that anyone who installs manually has to also do that. But then we'd need to post another bug to not load the plugin if it's not specified in the pref.

Comment 20

[Blair McBride [:Unfocused] (UNAVAILABLE)]

(In reply to Benjamin Smedberg  [:bsmedberg] from comment #6)
> Dão, this is the bug for implementing the addon manager core, but
> specifically for OpenH264 because that is time-sensitive and must hit FF33.
> Because these addons are supposed to be part of the plugins pane and not a
> separate pane, I believe this involves changes to the existing
> PluginProvider and not a new provider.

Wish someone would have asked someone who was familiar with the Add-ons Manager code, to clear up these assumptions.

It doesn't need to be in the PluginsProvider. But, nor does it need to be in a new provider. In the UI, categories are defined by add-on types - which can come from any number of providers.

I'd originally wanted this done in XPIProvider - packaged as XPI add-ons of either type=plugin or type=media-decoder (which can still easily be shown in the plugins category in the UI). XPIProvider already has battle-tested code for handling installs/updates/etc - PluginProvider doesn't.

Also, if these are going in the /plugins/ directory, does that imply plugin-host is scanning that directory for these? Why? It's awfully inefficient, considering the Add-ons Manager needs to know about these anyway, and can just tell plugin-host about them.

Comment 21

[Blair McBride [:Unfocused] (UNAVAILABLE)]

Talked a bit with Georg on IRC about all this. Given this is essentially a rush-job needing a quick hack to meet a deadline, he's going to go ahead with what he started, implementing it in PluginProvider. Then, when we have time on our side, we'll rip all of this out and replace it with a more thorough and future-proof implementation. Not the ideal situation, but...

Comment 22

[Benjamin Smedberg]

The OpenH264 plugin from Cisco won't be packaged as an XPI. So for now, let's keep to the original plan and focus on the thing at hand.

I'm happy for it to be a separate OpenH264Provider as long as it produces the correct UI.

Comment 23

[Georg Fritzsche [:gfritzsche]]

Attachment: wip.patch

Comment 24

[Georg Fritzsche [:gfritzsche]]

Attachment: WIP

WIP checkpoint, basic item present.

Next:
* need to figure out how we want to provide "is installed" and version info
* enabling preferences
* possibly move the provider to a separate JSM

Comment 25

[Georg Fritzsche [:gfritzsche]]

Attachment: WIP

This is relatively complete, but still only using faked data. Irving, do you mind taking a quick look whether you agree on the path taken?

Next:
* need to figure out how we want to provide "is installed" and version info
* wait for decisions on bug 1032814

Comment 26

[:Irving Reid (No longer working on Firefox)]

Comment on attachment 8450178 [details] [diff] [review]
WIP

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

looks OK to me so far.

::: toolkit/mozapps/extensions/internal/OpenH264Provider.jsm
@@ +28,5 @@
> +/**
> + * The OpenH264Wrapper provides the info for the OpenH264 GMP plugin to public callers through the API.
> + */
> +function OpenH264Wrapper(aId, aName, aDescription, aVersion) {
> +  this.__defineGetter__("id", function() aId);

I believe we're moving away from __defineGetter__ to Object.defineProperty(), though I don't feel strongly enough about it to r-

@@ +109,5 @@
> +      aListener.onUpdateFinished(this);
> +  },
> +
> +  get pluginLibraries() { return []; },
> +  get pluginMimeTypes() { return []; },

Is this plugin ever triggered by a MIME type, or only by specific content tags?

@@ +146,5 @@
> +    let description = pluginsBundle.GetStringFromName("openH264_description");
> +    return new OpenH264Wrapper(OPENH264_PLUGIN_ID,
> +                               OPENH264_PLUGIN_NAME,
> +                               description,
> +                               "1.9.3.4");

I'm not sure if this implementation will break things, but other providers remember and cache their add-on objects so that they usually return the same object in response to a request for the same add-on (XPIProvider, for example, returns a different instance only if the add-on is updated)

::: toolkit/mozapps/extensions/jar.mn
@@ +28,5 @@
>    content/mozapps/extensions/newaddon.xul                       (content/newaddon.xul)
>    content/mozapps/extensions/newaddon.js                        (content/newaddon.js)
>    content/mozapps/extensions/setting.xml                        (content/setting.xml)
>    content/mozapps/extensions/pluginPrefs.xul                    (content/pluginPrefs.xul)
> +  content/mozapps/extensions/openH264Prefs.xul                  (content/openH264Prefs.xul)

this file isn't included in the WIP patch?

Comment 27

[Georg Fritzsche [:gfritzsche]]

(In reply to :Irving Reid from comment #26)
> > +  get pluginLibraries() { return []; },
> > +  get pluginMimeTypes() { return []; },
> 
> Is this plugin ever triggered by a MIME type, or only by specific content
> tags?

This is to handle code in about:plugins (toolkit/content/plugins.html), which expects addons of type plugins to have those properties.
I'm thinking we can special-case this later when we have more GeckoMediaPlugins and just stubbing it out now.

> @@ +146,5 @@
> > +    let description = pluginsBundle.GetStringFromName("openH264_description");
> > +    return new OpenH264Wrapper(OPENH264_PLUGIN_ID,
> > +                               OPENH264_PLUGIN_NAME,
> > +                               description,
> > +                               "1.9.3.4");
> 
> I'm not sure if this implementation will break things, but other providers
> remember and cache their add-on objects so that they usually return the same
> object in response to a request for the same add-on (XPIProvider, for
> example, returns a different instance only if the add-on is updated)

PluginProvider does it that way, so it works fine.

Comment 28

[Georg Fritzsche [:gfritzsche]]

Attachment: WIP, v3

This now uses these prefs:
* media.openh264.path - observed, if not set, openh264 is not installed, otherwise the plugin directory
* media.openh264.version

Comment 29

[Georg Fritzsche [:gfritzsche]]

Attachment: List view, pending install

Comment 30

[Georg Fritzsche [:gfritzsche]]

Attachment: Detail view, pending install

Comment 31

[Georg Fritzsche [:gfritzsche]]

Attachment: List view, installed

Comment 32

[Georg Fritzsche [:gfritzsche]]

Attachment: Detail view, installed

Comment 33

[Georg Fritzsche [:gfritzsche]]

(In reply to Georg Fritzsche [:gfritzsche] from comment #29)
> Created attachment 8451021 [details]
> List view, pending install

Blair, is there a way to make the description italic for the "Will be installed shortly." case?
I could do openh264-specific styling, but i'd rather avoid that if possible.

Comment 34

[Blair McBride [:Unfocused] (UNAVAILABLE)]

(In reply to Georg Fritzsche [:gfritzsche] [away July 10-14] from comment #27)
> > I'm not sure if this implementation will break things, but other providers
> > remember and cache their add-on objects so that they usually return the same
> > object in response to a request for the same add-on (XPIProvider, for
> > example, returns a different instance only if the add-on is updated)
> 
> PluginProvider does it that way, so it works fine.

Yea, it's fine. It's just an optimization in other providers. Think we have a bug to do the same in PluginProvider too, but it's not a high priority.

Comment 35

[Blair McBride [:Unfocused] (UNAVAILABLE)]

(In reply to Georg Fritzsche [:gfritzsche] [away July 10-14] from comment #33)
> (In reply to Georg Fritzsche [:gfritzsche] from comment #29)
> > Created attachment 8451021 [details]
> > List view, pending install
> 
> Blair, is there a way to make the description italic for the "Will be
> installed shortly." case?
> I could do openh264-specific styling, but i'd rather avoid that if possible.

No, there isn't. 

Rather than use the description, I'd recommend using a notification, and add a string in extensions.properties under notifications.whatever and detail.notifications.whatever

Comment 36

[Georg Fritzsche [:gfritzsche]]

Attachment: List view, pending install, warning notification

Comment 37

[Georg Fritzsche [:gfritzsche]]

Attachment: List view, pending install, info notification

Comment 38

[Georg Fritzsche [:gfritzsche]]

Attachment: Detail view, pending install, warning notification

Comment 39

[Georg Fritzsche [:gfritzsche]]

Attachment: Detail view, pending install, info notification

Comment 40

[Georg Fritzsche [:gfritzsche]]

There are still traces of the info notification style, but re-activating it looks a little broken.

I wonder if the warning notification style is ok, otherwise i'll have to rework the info one (and i have no idea about the styling).

Comment 41

[(Currently slow to respond) Philipp Sackl [:phlsa] (Firefox UX) please use needinfo]

Given the low visibility of this item, the warning style is absolutely good enough.

Comment 42

[Georg Fritzsche [:gfritzsche]]

Attachment: AddonManager integration, v4

This is complete except:
* registering path with the GMP service (pending bug 1032814)
* manually triggering update (pending bug 1035225)

I would like to get review excepting these two parts (marked as TODO in the patch), so that things can get picked up if necessary while i'm away for MozFest EA.

https://tbpl.mozilla.org/?tree=Try&rev=af8db470cc9e

Comment 43

[Blair McBride [:Unfocused] (UNAVAILABLE)]

Comment on attachment 8451777 [details] [diff] [review]
AddonManager integration, v4

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

::: toolkit/mozapps/extensions/internal/OpenH264Provider.jsm
@@ +18,5 @@
> +const URI_EXTENSION_STRINGS    = "chrome://mozapps/locale/extensions/extensions.properties";
> +const STRING_TYPE_NAME         = "type.%ID%.name";
> +
> +const OPENH264_PLUGIN_ID       = "gmp:openh264";
> +const OPENH264_PLUGIN_NAME     = "OpenH264";

Has it been confirmed that this is the string that will be used in all locales? Often there's an exception for product names, where the name does have a localised version - ja is a common example locale that comes to mind.

@@ +20,5 @@
> +
> +const OPENH264_PLUGIN_ID       = "gmp:openh264";
> +const OPENH264_PLUGIN_NAME     = "OpenH264";
> +const OPENH264_PREF_BRANCH     = "media.openh264.";
> +const OPENH264_PREF_ENABLED    = "enabled";

Given this is toolkit, we need a way to disable this provider, not just the decoder. IMO, it'd be fine if all it did was make getAddonByID/getAddonsByTypes not return anything.

@@ +34,5 @@
> +
> +/**
> + * The OpenH264Wrapper provides the info for the OpenH264 GMP plugin to public callers through the API.
> + */
> +function OpenH264Wrapper(aId, aName, aDescription) {

Refactoring this out of PluginProvider makes it simpler to ensure we only construct one of these - so we should do that.

In which case, it may be easier to just make a plain JS object, with read-only getters being normal properties, and freeze it via Object.freeze()

@@ +75,5 @@
> +  Object.defineProperty(this, "operationsRequiringRestart", { get: () => AddonManager.OP_NEEDS_RESTART_NONE });
> +
> +  Object.defineProperty(this, "permissions", { get: () => {
> +    let permissions = AddonManager.PERM_CAN_DISABLE |
> +                      AddonManager.PERM_CAN_ENABLE |

.permissions should only include PERM_CAN_ENABLE if you can enable *from the current state*. ie, if it's not already enabled. And visa versa for PERM_CAN_DISABLE.

@@ +83,5 @@
> +}
> +
> +OpenH264Wrapper.prototype = {
> +  optionsType: AddonManager.OPTIONS_TYPE_INLINE,
> +  optionsURL: "chrome://mozapps/content/extensions/openH264Prefs.xul",

If you don't freeze the object, there should be getters like the rest of them. 

Also, this URL should be a const.

@@ +86,5 @@
> +  optionsType: AddonManager.OPTIONS_TYPE_INLINE,
> +  optionsURL: "chrome://mozapps/content/extensions/openH264Prefs.xul",
> +
> +  get updateDate() {
> +    return this.installDate;

installDate here will only be correct for the first install. We only actually use updateDate anywhere, so my stance has been to not implement installDate unless we can give it a useful value.

@@ +102,5 @@
> +    return true;
> +  },
> +
> +  get foreignInstall() {
> +    return true;

This shouldn't be counted as a foreign install.

@@ +110,5 @@
> +    return true;
> +  },
> +
> +  findUpdates: function(aListener, aReason, aAppVersion, aPlatformVersion) {
> +    // TODO: Hook up to openh264 update for AddonManager.UPDATE_WHEN_USER_REQUESTED (pending bug 1035225)

If you're planning on doing this, then you won't need the inline prefs  - you just need to expose a applyBackgroundUpdates property. That'll make the UI for it consistent with every other add-on.

@@ +123,5 @@
> +
> +  get pluginMimeTypes() { return []; },
> +  get pluginLibraries() {
> +    let path = prefs.get(OPENH264_PREF_PATH, null);
> +    return path && path.length ? [path] : [];

This should be the leafName of the file. In an array.

@@ +127,5 @@
> +    return path && path.length ? [path] : [];
> +  },
> +  get pluginFullpath() {
> +    let path = prefs.get(OPENH264_PREF_PATH, null);
> +    return path && path.length ? path : "";

This should be an array.

@@ +131,5 @@
> +    return path && path.length ? path : "";
> +  },
> +
> +  get isInstalled() {
> +    return prefs.get(OPENH264_PREF_PATH, "").length > 0;

Hm, what about platforms where we don't have a compatible library to install? The UI will be misleading people in that case.

::: toolkit/mozapps/extensions/test/browser/browser_openH264.js
@@ +20,5 @@
> +let gIsEnUsLocale;
> +
> +function getInstallItem() {
> +  let doc = gManagerWindow.document;
> +  let view = doc.getElementById("view-port").selectedPanel;

Unused?

::: toolkit/mozapps/extensions/test/xpcshell/test_duplicateplugins.js
@@ +116,5 @@
>  
>  // Test that the plugins were coalesced and all appear in the returned list
>  function run_test_1() {
>    AddonManager.getAddonsByTypes(["plugin"], function(aAddons) {
> +    do_check_eq(aAddons.length, 6);

It's non-obvious what's happening here - since the test is meant to be solely returning what the mock PluginHost above knows about. I'd rather this test just disabled the OpenH264 provider. Of, failing that, just filtered out the gmp:openh264 item here.

::: toolkit/mozapps/extensions/test/xpcshell/test_pluginchange.js
@@ +94,5 @@
>      do_check_eq(addons[0].name, "Flash");
>      do_check_false(addons[0].userDisabled);
>      do_check_eq(addons[1].name, "Java");
>      do_check_false(addons[1].userDisabled);
> +    do_check_eq(addons[2].name, "OpenH264");

Ditto with test_pduplicateplugins.js - would rather we disabled the OpenH264 provider during this test.

Comment 44

[Georg Fritzsche [:gfritzsche]]

Attachment: AddonManager integration, v5

Comment 45

[Georg Fritzsche [:gfritzsche]]

(In reply to Blair McBride [:Unfocused] from comment #43)
> ::: toolkit/mozapps/extensions/internal/OpenH264Provider.jsm
> @@ +18,5 @@
> > +const URI_EXTENSION_STRINGS    = "chrome://mozapps/locale/extensions/extensions.properties";
> > +const STRING_TYPE_NAME         = "type.%ID%.name";
> > +
> > +const OPENH264_PLUGIN_ID       = "gmp:openh264";
> > +const OPENH264_PLUGIN_NAME     = "OpenH264";
> 
> Has it been confirmed that this is the string that will be used in all
> locales? Often there's an exception for product names, where the name does
> have a localised version - ja is a common example locale that comes to mind.

Made localizable.

> @@ +110,5 @@
> > +    return true;
> > +  },
> > +
> > +  findUpdates: function(aListener, aReason, aAppVersion, aPlatformVersion) {
> > +    // TODO: Hook up to openh264 update for AddonManager.UPDATE_WHEN_USER_REQUESTED (pending bug 1035225)
> 
> If you're planning on doing this, then you won't need the inline prefs  -
> you just need to expose a applyBackgroundUpdates property. That'll make the
> UI for it consistent with every other add-on.

Done, but now we don't get the preferences button anymore. Is that intentional? Shouldn't the button behavior be similar to OPTION_TYPE_INLINE?

> @@ +131,5 @@
> > +    return path && path.length ? path : "";
> > +  },
> > +
> > +  get isInstalled() {
> > +    return prefs.get(OPENH264_PREF_PATH, "").length > 0;
> 
> Hm, what about platforms where we don't have a compatible library to
> install? The UI will be misleading people in that case.

We target all our supported desktop platforms.

Comment 46

[Georg Fritzsche [:gfritzsche]]

Attachment: AddonManager integration, v5

Comment 47

[Blair McBride [:Unfocused] (UNAVAILABLE)]

(In reply to Georg Fritzsche [:gfritzsche] [away July 10-14] from comment #45)
> (In reply to Blair McBride [:Unfocused] from comment #43)
> Done, but now we don't get the preferences button anymore. Is that
> intentional? Shouldn't the button behavior be similar to OPTION_TYPE_INLINE?

It's intentional (all add-ons work like this). Bug 625465 will surface the details pane more.

> > @@ +131,5 @@
> > > +    return path && path.length ? path : "";
> > > +  },
> > > +
> > > +  get isInstalled() {
> > > +    return prefs.get(OPENH264_PREF_PATH, "").length > 0;
> > 
> > Hm, what about platforms where we don't have a compatible library to
> > install? The UI will be misleading people in that case.
> 
> We target all our supported desktop platforms.


People run Firefox on more than just the platforms Mozilla officially supports. We should make a reasonable effort to not break things in such cases.

Comment 48

[Blair McBride [:Unfocused] (UNAVAILABLE)]

Comment on attachment 8452351 [details] [diff] [review]
AddonManager integration, v5

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

::: toolkit/mozapps/extensions/internal/OpenH264Provider.jsm
@@ +66,5 @@
> +  get pendingOperations() { return AddonManager.PENDING_NONE; },
> +
> +  get operationsRequiringRestart() { return AddonManager.OP_NEEDS_RESTART_NONE },
> +
> +  get permissions() {

Missing any tests for many of these properties.

@@ +68,5 @@
> +  get operationsRequiringRestart() { return AddonManager.OP_NEEDS_RESTART_NONE },
> +
> +  get permissions() {
> +    let permissions = AddonManager.PERM_CAN_UPGRADE;
> +    if (!this.appDisabled) {

appDisabled is hardcoded to be false, so this check is unneeded.

@@ +112,5 @@
> +    return prefs.get(OPENH264_PREF_AUTOUPDATE, true) ?
> +      AddonManager.AUTOUPDATE_ENABLE : AddonManager.AUTOUPDATE_DISABLE;
> +  },
> +
> +  set applyBackgroundUpdates(aVal) {

Especially need some test coverage for this property.

@@ +192,5 @@
> +                               description);
> +  },
> +
> +  getAddonByID: function(aId, aCallback) {
> +    if (prefs.get(OPENH264_PREF_PROVIDERENABLED, true) && aId == OPENH264_PLUGIN_ID) {

OPENH264_PREF_PROVIDERENABLED needs to default to false, so it's disabled by default with apps opting in to it.

Also, these two functions would be cleaner if that pref was memoized into a property of the provider.

::: toolkit/mozapps/extensions/jar.mn
@@ +31,5 @@
>    content/mozapps/extensions/pluginPrefs.xul                    (content/pluginPrefs.xul)
>    content/mozapps/xpinstall/xpinstallConfirm.xul                (content/xpinstallConfirm.xul)
>    content/mozapps/xpinstall/xpinstallConfirm.js                 (content/xpinstallConfirm.js)
>    content/mozapps/xpinstall/xpinstallConfirm.css                (content/xpinstallConfirm.css)
> +  content/mozapps/xpinstall/xpinstallItem.xml                   (content/xpinstallItem.xml)
\ No newline at end of file

Unneeded & irrelevant change.

::: toolkit/mozapps/extensions/test/xpcshell/test_openh264.js
@@ +60,5 @@
> +
> +  Services.prefs.setBoolPref(OPENH264_PREF_ENABLED, false);
> +  Services.prefs.setCharPref(OPENH264_PREF_LASTUPDATE, "" + TEST_DATE.getTime());
> +  Services.prefs.setCharPref(OPENH264_PREF_VERSION, TEST_VERSION);
> +  Services.prefs.setCharPref(OPENH264_PREF_PATH, "some/path");

Tests that these prefs are updated? Or, in other words, that changes are persisted across restarts.

@@ +80,5 @@
> +
> +  let mimetypes = addon.pluginMimeTypes;
> +  Assert.ok(mimetypes);
> +  Assert.equal(mimetypes.length, 0);
> +  let libraries = addon.pluginLibraries;

Test for the contents of this array?

@@ +83,5 @@
> +  Assert.equal(mimetypes.length, 0);
> +  let libraries = addon.pluginLibraries;
> +  Assert.ok(libraries);
> +  Assert.equal(libraries.length, 1);
> +  Assert.ok(addon.pluginFullpath.length > 0);

Ditto.

Comment 49

[Benjamin Smedberg]

The UX design for this in bug 1007694 and specifically https://bugzilla.mozilla.org/show_bug.cgi?id=1007694#c17 specify that we show the Preferences button for this plugin.

Unfocused if you disagree with that UX decision, please work it out with Philipp, but for now that's the UI we need to implement/review.

Comment 50

[Blair McBride [:Unfocused] (UNAVAILABLE)]

Ugh, ok. In which case, a blank options file will do it - while still allowing us to be consistent in both the UI and the API for the update setting.

Comment 51

[Georg Fritzsche [:gfritzsche]]

(In reply to Blair McBride [:Unfocused] from comment #48)
> @@ +68,5 @@
> > +  get operationsRequiringRestart() { return AddonManager.OP_NEEDS_RESTART_NONE },
> > +
> > +  get permissions() {
> > +    let permissions = AddonManager.PERM_CAN_UPGRADE;
> > +    if (!this.appDisabled) {
> 
> appDisabled is hardcoded to be false, so this check is unneeded.

I would rather hardcode this only in one place.

> ::: toolkit/mozapps/extensions/test/xpcshell/test_openh264.js
> @@ +60,5 @@
> > +
> > +  Services.prefs.setBoolPref(OPENH264_PREF_ENABLED, false);
> > +  Services.prefs.setCharPref(OPENH264_PREF_LASTUPDATE, "" + TEST_DATE.getTime());
> > +  Services.prefs.setCharPref(OPENH264_PREF_VERSION, TEST_VERSION);
> > +  Services.prefs.setCharPref(OPENH264_PREF_PATH, "some/path");
> 
> Tests that these prefs are updated? Or, in other words, that changes are
> persisted across restarts.

Those prefs are not handled from the code in this patch, so i don't see a need to test the pref persistance here.

Comment 52

[Georg Fritzsche [:gfritzsche]]

Attachment: AddonManager integration, v6

Comment 53

[Georg Fritzsche [:gfritzsche]]

https://tbpl.mozilla.org/?tree=Try&rev=c6ee3ba74ad5

Comment 54

[Blair McBride [:Unfocused] (UNAVAILABLE)]

Comment on attachment 8456514 [details] [diff] [review]
AddonManager integration, v6

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

\o/

Comment 55

[David Benham]

Not sure if it is _name and/or _description that dictate how it is displayed in the add-ons manager, but to be compliant with the OpenH264 binary license (http://www.openh264.org/BINARY_LICENSE.txt), it needs appear as 
 
  "OpenH264 Video Codec"
  "by Cisco Systems, Inc."

Comment 56

[Georg Fritzsche [:gfritzsche]]

Thanks for the heads-up David. _name & _description are both shown and we should keep the name short and user-friendly.

Given this section:
> 3. Third party software, in the location where end users can control the use of the Cisco-provided binary, must display the following text:
>  "OpenH264 Video Codec provided by Cisco Systems, Inc."
... i assume we should be fine with changing the description to something like:
> Play back web video and use video chats.
> OpenH264 Video Codec provided by Cisco Systems, Inc.

Philipp, what do you think?

Comment 57

[Benjamin Smedberg]

This is Cisco software and we should just display the name exactly as they have specified it.

Bug 103928 covers displaying the license information, so let's not let it delay landing this bug.

Comment 58

[Georg Fritzsche [:gfritzsche]]

Switched to specified name and it seems ok: http://imgur.com/38WF4kC

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

Comment 59

[Ryan VanderMeulen [:RyanVM]]

Backed out for xpcshell failures.
https://hg.mozilla.org/integration/fx-team/rev/9a6b96d6559d

https://tbpl.mozilla.org/php/getParsedLog.php?id=43937920&tree=Fx-Team

Comment 60

[Georg Fritzsche [:gfritzsche]]

Trivial test oversight fixed: https://hg.mozilla.org/integration/fx-team/rev/889755c1797b

Comment 61

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/889755c1797b

Comment 62

[Marco Mucci [:MarcoM]]

Hi Florin, can a QA contact be assigned for verification of this bug.

Comment 63

[Ryan VanderMeulen [:RyanVM]]

Follow-up to only run on Firefox (and un-break c-c):
https://hg.mozilla.org/mozilla-central/rev/8f739f0633c6

Comment 64

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

Is OpenH264Provider.jsm needed on Android, where we already have h264 support? I see it's loaded at startup.

Comment 65

[Georg Fritzsche [:gfritzsche]]

I'm not sure how Fennec works here, the addon manager parts here are desktop specific.

Is H264 already working with WebRTC there?

Comment 66

[Eric Rescorla (:ekr)]

(In reply to Mark Finkle (:mfinkle) from comment #64)
> Is OpenH264Provider.jsm needed on Android, where we already have h264
> support? I see it's loaded at startup.

We have H.264 for WebRTC on Android? That's news to me.

Comment 67

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

(In reply to Eric Rescorla (:ekr) from comment #66)
> (In reply to Mark Finkle (:mfinkle) from comment #64)
> > Is OpenH264Provider.jsm needed on Android, where we already have h264
> > support? I see it's loaded at startup.
> 
> We have H.264 for WebRTC on Android? That's news to me.

I assume by "for WebRTC" you mean something different than the ability to play videos using h264?

Comment 68

[Eric Rescorla (:ekr)]

(In reply to Mark Finkle (:mfinkle) from comment #67)
> (In reply to Eric Rescorla (:ekr) from comment #66)
> > (In reply to Mark Finkle (:mfinkle) from comment #64)
> > > Is OpenH264Provider.jsm needed on Android, where we already have h264
> > > support? I see it's loaded at startup.
> > 
> > We have H.264 for WebRTC on Android? That's news to me.
> 
> I assume by "for WebRTC" you mean something different than the ability to
> play videos using h264?

http://www.webrtc.org/

Comment 69

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

(In reply to Eric Rescorla (:ekr) from comment #68)
> (In reply to Mark Finkle (:mfinkle) from comment #67)
> > (In reply to Eric Rescorla (:ekr) from comment #66)
> > > (In reply to Mark Finkle (:mfinkle) from comment #64)
> > > > Is OpenH264Provider.jsm needed on Android, where we already have h264
> > > > support? I see it's loaded at startup.
> > > 
> > > We have H.264 for WebRTC on Android? That's news to me.
> > 
> > I assume by "for WebRTC" you mean something different than the ability to
> > play videos using h264?
> 
> http://www.webrtc.org/

Yeah, I know what WebRTC is. Why do we need OpenH264Provider for it? Does the OpenH264 plugin work on Android? If not, then I'll file bugs to remove any OpenH264 code from being executed and loaded until it is ready to be used on Android.

We do not want unused cruft shipped (taking up space in the APK), loaded (slowing down startup) and using memory.

Comment 70

[Eric Rescorla (:ekr)]

(In reply to Mark Finkle (:mfinkle) from comment #69)
> (In reply to Eric Rescorla (:ekr) from comment #68)
> > (In reply to Mark Finkle (:mfinkle) from comment #67)
> > > (In reply to Eric Rescorla (:ekr) from comment #66)
> > > > (In reply to Mark Finkle (:mfinkle) from comment #64)
> > > > > Is OpenH264Provider.jsm needed on Android, where we already have h264
> > > > > support? I see it's loaded at startup.
> > > > 
> > > > We have H.264 for WebRTC on Android? That's news to me.
> > > 
> > > I assume by "for WebRTC" you mean something different than the ability to
> > > play videos using h264?
> > 
> > http://www.webrtc.org/
> 
> Yeah, I know what WebRTC is. 

Then I don't understand the question you were asking.


> Why do we need OpenH264Provider for it?

Because we want H.264 to work with WebRTC and making it work
portably with the hardware is hard.


> Does
> the OpenH264 plugin work on Android?

I assume not.


> If not, then I'll file bugs to remove
> any OpenH264 code from being executed and loaded until it is ready to be
> used on Android.
> 
> We do not want unused cruft shipped (taking up space in the APK), loaded
> (slowing down startup) and using memory.

OK.

Comment 71

[Marco Mucci [:MarcoM]]

Hi Alexandra, following up to see if this bug can be verified by the end of the iteration on Monday August 4.

Comment 72

[Ada [:adalucinet]]

Verified as fixed with latest Aurora (buildID: 20140731004002) and Nightly 34.0a1 (Build ID: 20140731030206) builds on Ubuntu 13.04 64bit, Mac OS X 10.9.4 and Windows 7 x64.