Closed Bug 1009909 Opened 6 years ago Closed 6 years ago

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

Categories

(Toolkit :: Add-ons Manager, defect)

defect
Not set
Points:
5

Tracking

()

VERIFIED FIXED
mozilla33
Iteration:
34.1

People

(Reporter: benjamin, Assigned: gfritzsche)

References

(Blocks 2 open bugs)

Details

Attachments

(5 files, 11 obsolete files)

996.26 KB, image/png
Details
956.15 KB, image/png
Details
1.02 MB, image/png
Details
1.01 MB, image/png
Details
34.82 KB, patch
Unfocused
: review+
Details | Diff | Splinter Review
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.
Flags: firefox-backlog+
Summary: Firefox desktop: openh264 UX → Firefox desktop: implement openh264 UX
Blocks: OpenH264
Duplicate of this bug: 985256
Whiteboard: blocked on UX, not yet estimable → p=5
Points: --- → 5
QA Whiteboard: [qa?]
Whiteboard: p=5
QA Whiteboard: [qa?] → [qa+]
Assignee: nobody → dao
Status: NEW → ASSIGNED
Iteration: --- → 33.2
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.
(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?
Component: General → Add-ons Manager
Product: Firefox → Toolkit
(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
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.
Depends on: 950933
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.
No longer depends on: 950933
(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.
Flags: needinfo?(bmcbride)
OS: Linux → All
Hardware: x86_64 → All
Summary: Firefox desktop: implement openh264 UX → Firefox desktop: Integrate the openh264 media plugin in the add-ons manager
I'm taking this over for now for the PluginProvider integration and will lean on Dao if i get stuck with the UI.
Assignee: dao → georg.fritzsche
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?
Flags: needinfo?(netzen)
Flags: needinfo?(joshmoz)
Flags: needinfo?(ekr)
I think there is no open question for Blair right now.
Flags: needinfo?(bmcbride)
> * 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.
(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.
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.
(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.
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.
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?
(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.
Flags: needinfo?(netzen)
> * 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.
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.
(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.
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...
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.
Attached patch wip.patch (obsolete) — Splinter Review
Attached patch WIP (obsolete) — Splinter Review
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
Attachment #8449497 - Attachment is obsolete: true
Attached patch WIP (obsolete) — Splinter Review
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
Attachment #8449499 - Attachment is obsolete: true
Attachment #8450178 - Flags: feedback?(irving)
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?
Attachment #8450178 - Flags: feedback?(irving) → feedback+
Flags: needinfo?(joshmoz)
Flags: needinfo?(ekr)
(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.
Attached patch WIP, v3 (obsolete) — Splinter Review
This now uses these prefs:
* media.openh264.path - observed, if not set, openh264 is not installed, otherwise the plugin directory
* media.openh264.version
Attachment #8450178 - Attachment is obsolete: true
Attached image List view, pending install (obsolete) —
Attached image Detail view, pending install (obsolete) —
Attached image List view, installed
Attached image Detail view, installed
(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.
Flags: needinfo?(bmcbride)
(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.
(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
Flags: needinfo?(bmcbride)
Attachment #8451021 - Attachment is obsolete: true
Attachment #8451022 - Attachment is obsolete: true
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).
Flags: needinfo?(philipp)
Given the low visibility of this item, the warning style is absolutely good enough.
Flags: needinfo?(philipp)
Attachment #8451569 - Attachment is obsolete: true
Attachment #8451567 - Attachment is obsolete: true
Attached patch AddonManager integration, v4 (obsolete) — Splinter Review
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
Attachment #8451020 - Attachment is obsolete: true
Attachment #8451777 - Flags: review?(bmcbride)
Iteration: 33.2 → 33.3
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.
Attachment #8451777 - Flags: review?(bmcbride) → review-
Attached patch AddonManager integration, v5 (obsolete) — Splinter Review
Attachment #8451777 - Attachment is obsolete: true
Attachment #8452344 - Flags: review?(bmcbride)
(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.
Attached patch AddonManager integration, v5 (obsolete) — Splinter Review
Attachment #8452344 - Attachment is obsolete: true
Attachment #8452344 - Flags: review?(bmcbride)
Attachment #8452351 - Flags: review?(bmcbride)
(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 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.
Attachment #8452351 - Flags: review?(bmcbride) → review-
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.
Flags: needinfo?(bmcbride)
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.
Flags: needinfo?(bmcbride)
(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.
Depends on: 1032814
Attachment #8452351 - Attachment is obsolete: true
Attachment #8456514 - Flags: review?(bmcbride)
Comment on attachment 8456514 [details] [diff] [review]
AddonManager integration, v6

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

\o/
Attachment #8456514 - Flags: review?(bmcbride) → review+
Depends on: 1039028
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."
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?
Flags: needinfo?(philipp)
Blocks: 1039226
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.
Switched to specified name and it seems ok: http://imgur.com/38WF4kC

https://hg.mozilla.org/integration/fx-team/rev/46acc7f0704b
Flags: needinfo?(philipp)
Blocks: 1039028
No longer depends on: 1039028
Blocks: 1040048
https://hg.mozilla.org/mozilla-central/rev/889755c1797b
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla33
Hi Florin, can a QA contact be assigned for verification of this bug.
Flags: needinfo?(florin.mezei)
Follow-up to only run on Firefox (and un-break c-c):
https://hg.mozilla.org/mozilla-central/rev/8f739f0633c6
Flags: needinfo?(florin.mezei)
QA Contact: alexandra.lucinet
Iteration: 33.3 → 34.1
Is OpenH264Provider.jsm needed on Android, where we already have h264 support? I see it's loaded at startup.
I'm not sure how Fennec works here, the addon manager parts here are desktop specific.

Is H264 already working with WebRTC there?
(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.
(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?
(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/
(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.
(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.
Hi Alexandra, following up to see if this bug can be verified by the end of the iteration on Monday August 4.
Flags: needinfo?(alexandra.lucinet)
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.
Status: RESOLVED → VERIFIED
QA Whiteboard: [qa+] → [qa!]
Flags: needinfo?(alexandra.lucinet)
Flags: in-testsuite+
Blocks: 1086668
You need to log in before you can comment on or make changes to this bug.