As a user, I am not required to explicitly download supported content decryption modules, resulting in the automatic download of said CDMs in the background so that I can decrypt and play back content at the time of my choosing with minimal to zero wait time.

CC'ing catlee because he was working with Adobe on the releng/AUS side.

Stephen was looking into this, please unassign if i misunderstood.

catlee, or anyone else who knows: can you confirm that this plugin will be published/advertised through the same GMP update list as OpenH264 in bug 1009816?

The other open questions: This is presumably implemented as a GMP as well? Consequently, is it also expected to be installed into the user profile and registered via mozIGeckoMediaPluginService.addPluginDirectory()?

Yes, this is a GMP which will be delivered like OpenH264. I can't answer the question about whether this will be in the same GMP update as OpenH264, although that's what I expected from previous conversations.

(In reply to Georg Fritzsche [:gfritzsche] from comment #4) > The other open questions: > This is presumably implemented as a GMP as well? Yes. > Consequently, is it also expected to be installed into the user profile and > registered via mozIGeckoMediaPluginService.addPluginDirectory()? Yes. (In reply to Georg Fritzsche [:gfritzsche] from comment #3) > catlee, or anyone else who knows: can you confirm that this plugin will be > published/advertised through the same GMP update list as OpenH264 in bug > 1009816? Yes. There's a URL in the same XML file that you parse for OpenH264, i.e: https://aus4-dev.allizom.org/update/3/GMP/%VERSION%/%BUILD_ID%/%BUILD_TARGET%/%LOCALE%/%CHANNEL%/%OS_VERSION%/%DISTRIBUTION%/%DISTRIBUTION_VERSION%/update.xml For example: https://aus4-dev.allizom.org/update/3/GMP/36.0a1/20120222174716/WINNT_x86-msvc/en-US/nightly/default/default/default/update.xml (this was added in bug 1098451, which is where I copied those links from)

Posting my work in progress to pick up any feedback along the way, if there is any. This patch simply renames OpenH264Provider to GMPProvider as we will be using it for EME (and presumably future GMPs) as well.

This patch makes GMPInstallManager aware of the EME plugin. Several assumptions were made in this patch: - The EME plugin is referenced in the same xml file as the OpenH264 plugin (this appears to be confirmed by comment 6). - We will support a pref to disable auto update for EME just like for OpenH264. - We want to support installs of the EME plugin even if the install of the OpenH264 failed or was disabled, and vice-versa. - The way we currently reject Promises in this patch when we encounter install failure does not prevent us from installing additional plugins. Would be great to have someone confirm who has more experience with Promises. - The installation of plugins that are neither OpenH264 or EME should currently be rejected. My next patch will update GMPProvider.jsm and make it aware of EME. I ran the following tests to try and guarantee that we haven’t broken the download/install of OpenH264 plugins. If there are other tests that I could run, please let me know: ./mach test toolkit/mozapps/extensions/test/browser/browser_openH264.js ./mach test toolkit/mozapps/extensions/test/xpcshell/test_openh264.js ./mach test toolkit/modules/tests/xpcshell/test_GMPInstallManager.js

(In reply to Stephen Pohl [:spohl] from comment #8) > Created attachment 8538608 [details] [diff] [review] > 2. GMPInstallManager (wip) > > This patch makes GMPInstallManager aware of the EME plugin. Several > assumptions were made in this patch: > - The EME plugin is referenced in the same xml file as the OpenH264 plugin > (this appears to be confirmed by comment 6). Correct! > - We will support a pref to disable auto update for EME just like for > OpenH264. We should not download any CDMs or call addPluginDir if media.eme.enabled is false. This is important, as some people won't want any proprietary code to be installed on their PCs. We should not download Adobe's CDM or call addPluginDir for Adobe's CDM if media.eme.adobe-access.enabled is false. My intention was to have a separate pref for each EME CDM that we have, so that the user can turn them off individually in the addon manager UI. Gecko already checks this pref inside the EME JS APIs implementation. Having a separate pref to prevent updates of all CDMs is probably a good idea too. I don't think that (at this stage) that pref needs to be CDM vendor specific. > - We want to support installs of the EME plugin even if the install of the > OpenH264 failed or was disabled, and vice-versa. Yep. > - The installation of plugins that are neither OpenH264 or EME should > currently be rejected. Yes!

(In reply to Chris Pearce (:cpearce) from comment #9) > (In reply to Stephen Pohl [:spohl] from comment #8) > > - We will support a pref to disable auto update for EME just like for > > OpenH264. > > We should not download any CDMs or call addPluginDir if media.eme.enabled is > false. This is important, as some people won't want any proprietary code to > be installed on their PCs. Good, this is what the patch is currently doing. > We should not download Adobe's CDM or call addPluginDir for Adobe's CDM if > media.eme.adobe-access.enabled is false. My intention was to have a separate > pref for each EME CDM that we have, so that the user can turn them off > individually in the addon manager UI. Gecko already checks this pref inside > the EME JS APIs implementation. In the XML that you reference in comment 6 we only have Adobe's CDM. Once we have several different ones, how can Adobe's CDM be differentiated from others? The id is currently set to "eme", which seems very generic. Do we need to parse the URL? Will we change the id? > Having a separate pref to prevent updates of all CDMs is probably a good > idea too. I don't think that (at this stage) that pref needs to be CDM > vendor specific. Could media.eme.enabled be used for this, or does it need to be separate? If separate, how do the two differ?

(In reply to Stephen Pohl [:spohl] from comment #8) > I ran the following tests to try and guarantee that we haven’t broken the > download/install of OpenH264 plugins. If there are other tests that I could > run, please let me know: > ./mach test toolkit/mozapps/extensions/test/browser/browser_openH264.js > ./mach test toolkit/mozapps/extensions/test/xpcshell/test_openh264.js > ./mach test toolkit/modules/tests/xpcshell/test_GMPInstallManager.js These are the relevant ones. I will take a closer look at the patch tomorrow. (In reply to Stephen Pohl [:spohl] from comment #10) > In the XML that you reference in comment 6 we only have Adobe's CDM. Once we > have several different ones, how can Adobe's CDM be differentiated from > others? The id is currently set to "eme", which seems very generic. Do we > need to parse the URL? Will we change the id? We should really change the id, e.g. eme-adobe or the like.

The question for catlee was answered already.

(In reply to Georg Fritzsche [:gfritzsche] from comment #11) > (In reply to Stephen Pohl [:spohl] from comment #10) > > In the XML that you reference in comment 6 we only have Adobe's CDM. Once we > > have several different ones, how can Adobe's CDM be differentiated from > > others? The id is currently set to "eme", which seems very generic. Do we > > need to parse the URL? Will we change the id? > > We should really change the id, e.g. eme-adobe or the like. I think changing the id now is a good way to future-proof this.

(In reply to Chris Pearce (:cpearce) from comment #13) > (In reply to Georg Fritzsche [:gfritzsche] from comment #11) > > (In reply to Stephen Pohl [:spohl] from comment #10) > > > In the XML that you reference in comment 6 we only have Adobe's CDM. Once we > > > have several different ones, how can Adobe's CDM be differentiated from > > > others? The id is currently set to "eme", which seems very generic. Do we > > > need to parse the URL? Will we change the id? > > > > We should really change the id, e.g. eme-adobe or the like. > > I think changing the id now is a good way to future-proof this. sure, that's simple enough. we just picked 'eme' as a placeholder for lack of a better name at the time. can we make that change right away?

(In reply to Chris AtLee [:catlee] from comment #14) > (In reply to Chris Pearce (:cpearce) from comment #13) > > I think changing the id now is a good way to future-proof this. > > sure, that's simple enough. we just picked 'eme' as a placeholder for lack > of a better name at the time. > can we make that change right away? I know of no reason on the Gecko or Adobe side for us not to change immediately.

Note that there is Fennec work on-going in bug 1110271 that touches a bit on the code involved here.

Comment on attachment 8538607 [details] [diff] [review] 1. Rename OpenH264Provider to GMPProvider (wip) Review of attachment 8538607 [details] [diff] [review]: ----------------------------------------------------------------- We also have to restructure the prefs for GMPProvider now - they are currently all in the "media.gmp-openh264." branch. ::: toolkit/mozapps/extensions/test/browser/browser_openH264.js @@ +5,5 @@ > "use strict"; > > Cu.import("resource://gre/modules/Promise.jsm"); > let {AddonTestUtils} = Cu.import("resource://testing-common/AddonManagerTesting.jsm", {}); > +let OpenH264Scope = Cu.import("resource://gre/modules/addons/GMPProvider.jsm"); |let GMPScope| or something similar.

Comment on attachment 8538607 [details] [diff] [review] 1. Rename OpenH264Provider to GMPProvider (wip) Review of attachment 8538607 [details] [diff] [review]: ----------------------------------------------------------------- ::: toolkit/mozapps/extensions/test/xpcshell/test_openh264.js @@ +191,5 @@ > addPluginDirectory: path => addedPaths.push(path), > removePluginDirectory: path => removedPaths.push(path), > }; > > + let OpenH264Scope = Cu.import("resource://gre/modules/addons/GMPProvider.jsm"); let GMPScope @@ +246,5 @@ > Assert.deepEqual(removedPaths, []); > }); > > add_task(function* test_periodicUpdate() { > + let OpenH264Scope = Cu.import("resource://gre/modules/addons/GMPProvider.jsm"); let GMPScope

Comment on attachment 8538608 [details] [diff] [review] 2. GMPInstallManager (wip) Review of attachment 8538608 [details] [diff] [review]: ----------------------------------------------------------------- ::: toolkit/modules/GMPInstallManager.jsm @@ +456,5 @@ > log.info("Found " + gmpAddons.length + " addons advertised."); > let addonsToInstall = gmpAddons.filter(gmpAddon => { > log.info("Found addon: " + gmpAddon.toString()); > + let addonUpdateEnabled; > + if (gmpAddon.isOpenH264) { This doesnt scale so well anymore when we add more plugins. Can we just have an array of supported ids and base our checks on the ids directly? @@ +490,5 @@ > deferred.reject(); > }); > }); > + // All addons were installed successfully. > + deferred.resolve({status: "addons-install"}); Two issues here: * you are not waiting for the async install operations in the forEach() * you also reject the promise on the first failure, but dont cancel further installations I think we should decide whether we want to skip installations of subsequent plugins if one fails (i don't think so?). If we dont, we need to return installation status per plugin once all installs are done. I think we should just refactor this function to be task-based, i.e. simpleCheckAndInstall: Task.async(function*() { ... ... and then we can e.g. start to do |let paths = yield this.installAddon()| on each gmp addon. @@ +729,5 @@ > + /** > + * EME has special handling. > + * @return true if the plugin is the EME plugin > + */ > + get isEME() { I dont think this scales when we add more plugins. Cant we just directly check for the id where needed and remove these individual boolean properties?

(In reply to Chris Pearce (:cpearce) from comment #13) > (In reply to Georg Fritzsche [:gfritzsche] from comment #11) > > (In reply to Stephen Pohl [:spohl] from comment #10) > > > In the XML that you reference in comment 6 we only have Adobe's CDM. Once we > > > have several different ones, how can Adobe's CDM be differentiated from > > > others? The id is currently set to "eme", which seems very generic. Do we > > > need to parse the URL? Will we change the id? > > > > We should really change the id, e.g. eme-adobe or the like. > > I think changing the id now is a good way to future-proof this. I made this change on aus4-dev (the only place we publish eme updates right now). Eg: https://aus4-dev.allizom.org/update/3/GMP/36.0a1/20120222174716/WINNT_x86-msvc/en-US/nightly/default/default/default/update.xml

Thanks, Georg! Addressed feedback. (In reply to Georg Fritzsche [:gfritzsche] from comment #17) > We also have to restructure the prefs for GMPProvider now - they are > currently all in the "media.gmp-openh264." branch. Patch 3 (which makes the necessary changes to GMPProvider) will be taking care of this.

(In reply to Georg Fritzsche [:gfritzsche] from comment #19) > Comment on attachment 8538608 [details] [diff] [review] > 2. GMPInstallManager (wip) > > Review of attachment 8538608 [details] [diff] [review]: > ----------------------------------------------------------------- > > ::: toolkit/modules/GMPInstallManager.jsm > @@ +456,5 @@ > > log.info("Found " + gmpAddons.length + " addons advertised."); > > let addonsToInstall = gmpAddons.filter(gmpAddon => { > > log.info("Found addon: " + gmpAddon.toString()); > > + let addonUpdateEnabled; > > + if (gmpAddon.isOpenH264) { > > This doesnt scale so well anymore when we add more plugins. > Can we just have an array of supported ids and base our checks on the ids > directly? Agreed. This seemed okay for two or fewer plugins, but I didn't realize that we're planning to have several more. I'll go ahead and make this more scalable. > @@ +490,5 @@ > > deferred.reject(); > > }); > > }); > > + // All addons were installed successfully. > > + deferred.resolve({status: "addons-install"}); > > Two issues here: > * you are not waiting for the async install operations in the forEach() Once you had time to read the points below, do you still think there's a need to wait for async install operations here? > * you also reject the promise on the first failure, but dont cancel further > installations Comment 9 (second to last reply) seemed to imply that this is the expected behavior, i.e. that although an installation might fail, further plugins should still be installed. > I think we should decide whether we want to skip installations of subsequent > plugins if one fails (i don't think so?). > If we dont, we need to return installation status per plugin once all > installs are done. I would immediately agree with this if there wasn't [1]. Since we don't seem to care about installation status, should we change this anyway? It seems like we might want to do this once we start caring about the installation status? Currently, I figured it might be enough to just generally reject the promise if one or more installs failed. Otherwise (i.e. if all installs succeeded), we'd resolve it. [1] http://mxr.mozilla.org/mozilla-central/source/browser/base/content/browser.js#1353

Setting n-i to keep the question at the end of comment 10 on the radar. It would help me if we could list all the necessary prefs and their expected behaviors. Does the following seem right? media.eme.enabled: disable downloads/installs/updates for all EME CDMs regardless of origin. media.eme.adobe-access.enabled: disable download/install/update of Adobe EME CDM. Having renamed the id for Adobe's CDM in our XML, would it make sense to rename media.eme.adobe-access.enabled to media.eme-adobe.enabled or media.eme.adobe.enabled?

(In reply to Stephen Pohl [:spohl] from comment #22) > > Two issues here: > > * you are not waiting for the async install operations in the forEach() > > Once you had time to read the points below, do you still think there's a > need to wait for async install operations here? > > > * you also reject the promise on the first failure, but dont cancel further > > installations > > Comment 9 (second to last reply) seemed to imply that this is the expected > behavior, i.e. that although an installation might fail, further plugins > should still be installed. As it is right now, its definitely broken: you immediately resolve the promise with status:"addons-install" (as you don't wait on the installations). Then you reject the same promise, potentially multiple times, for each failed install. Each promise should be resolved or rejected once. While we don't have the browser.js code using things right now, we do at least return a promise that resolves after all installs for tests. We will probably need status information returned for the tests too. Although if we can assure test-coverage of install status in another way we could consider dropping the status info - its probably the easiest way here though.

(In reply to Stephen Pohl [:spohl] from comment #23) > media.eme.enabled: disable downloads/installs/updates for all EME CDMs > regardless of origin. > media.eme.adobe-access.enabled: disable download/install/update of Adobe EME > CDM. ... if the prefs are set to 'false', of course. If media.eme.enabled is 'true', individual CDMs could be disabled by their respective pref, for example by setting media.eme.adobe-access.enabled to 'false'.

(In reply to Georg Fritzsche [:gfritzsche] from comment #24) > While we don't have the browser.js code using things right now, we do at > least return a promise ... "we should at least return a promise"

(In reply to Stephen Pohl [:spohl] from comment #23) > Having renamed the id for Adobe's CDM in our XML, would it make sense to > rename media.eme.adobe-access.enabled to media.eme-adobe.enabled or > media.eme.adobe.enabled? If possible, lets please try to keep a common scheme for GMP prefs here, so we can just key off prefs by the GMP id instead of having hard-coded mappings for each of them around our code-base. media.eme-adobe.enabled would be in the same scheme as our existing OpenH264 prefs.

(In reply to Georg Fritzsche [:gfritzsche] from comment #24) > As it is right now, its definitely broken: you immediately resolve the > promise with status:"addons-install" (as you don't wait on the > installations). Ah, yes. Being new to how promises work, an oversight like this is exactly what I was concerned about. Thanks for catching! > Then you reject the same promise, potentially multiple > times, for each failed install. > Each promise should be resolved or rejected once. My understanding was that only the first resolve or reject would matter and any subsequent ones would simply be ignored. The intention was to reject if a failure occurred and resolve the promise should it not have been rejected by the time all installs completed. This obviously didn't work, since I didn't wait for installs to complete before resolving... :-/ > While we don't have the browser.js code using things right now, we do at > least return a promise that resolves after all installs for tests. > We will probably need status information returned for the tests too. > Although if we can assure test-coverage of install status in another way we > could consider dropping the status info - its probably the easiest way here > though. Just double-checking: are you saying that the refactoring (suggested in comment 19) would be the right thing to do then?

(In reply to Georg Fritzsche [:gfritzsche] from comment #27) > media.eme-adobe.enabled would be in the same scheme as our existing OpenH264 > prefs. Agreed, I'd prefer this too. Chris, are you okay with me renaming media.eme.adobe-access.enabled to media.eme-adobe.enabled and updating its use in MediaKeySystemAccess.cpp (I think this is the only place where it's used at the moment)?

(In reply to Stephen Pohl [:spohl] from comment #28) > > Then you reject the same promise, potentially multiple > > times, for each failed install. > > Each promise should be resolved or rejected once. > > My understanding was that only the first resolve or reject would matter and > any subsequent ones would simply be ignored. That is true for Promise.jsm, but i think it makes for confusing code (and should at least be documented). > > While we don't have the browser.js code using things right now, we do at > > least return a promise that resolves after all installs for tests. > > We will probably need status information returned for the tests too. > > Although if we can assure test-coverage of install status in another way we > > could consider dropping the status info - its probably the easiest way here > > though. > > Just double-checking: are you saying that the refactoring (suggested in > comment 19) would be the right thing to do then? Yes, i think that's the easiest approach (lots of .then() nesting is not very readable). If you see better/cleaner options, that's great too.

(In reply to Stephen Pohl [:spohl] from comment #23) > Setting n-i to keep the question at the end of comment 10 on the radar. It > would help me if we could list all the necessary prefs and their expected > behaviors. Does the following seem right? > > media.eme.enabled: disable downloads/installs/updates for all EME CDMs > regardless of origin. > media.eme.adobe-access.enabled: disable download/install/update of Adobe EME > CDM. > Yes, that's the behaviour we need. > Having renamed the id for Adobe's CDM in our XML, would it make sense to > rename media.eme.adobe-access.enabled to media.eme-adobe.enabled or > media.eme.adobe.enabled? I'm fine with making this change; it makes the pref shorter which is nice.

(In reply to Chris Pearce (:cpearce) from comment #31) > (In reply to Stephen Pohl [:spohl] from comment #23) > > Setting n-i to keep the question at the end of comment 10 on the radar. It > > would help me if we could list all the necessary prefs and their expected > > behaviors. Does the following seem right? > > > > media.eme.enabled: disable downloads/installs/updates for all EME CDMs > > regardless of origin. > > media.eme.adobe-access.enabled: disable download/install/update of Adobe EME > > CDM. > > > > Yes, that's the behaviour we need. Chris, do you know if we should also support an 'autoupdate' pref similar to OpenH264? This came up in bug 1117765 comment 2. If yes, should it be a global media.eme.autoupdate pref and/or one for each vendor?

The purpose of the autoupdate pref is to support the addon manager UI "Automatic updates" pane. Assuming that these will be exposed in the addons manager with UI similar to OpenH264, then they should use matching autoupdate prefs.

First go at refactoring simpleCheckAndInstall as described in comment 19. I'm not sure how to best report the results though. In some scenarios, we used to simply return an object with a |status| property (for example "nothing-new-to-install"). For the success/failure of individual plugin installs however, I believe we'd want more granular information about each individual install. Right now I'm using an array with objects that have a |name| and |status| property. My concern is that the caller will have to deal with both single objects as well as an array of objects as possible return values. Shoving all the results into one object's |status| property would eliminate this, but parsing the results would be more cumbersome. Georg, do you have any good ideas here?

This changes the current use of the media.eme.adobe-access.enabled pref to media.eme-adobe.enabled, as discussed in comment 31.

(In reply to Stephen Pohl [:spohl] from comment #35) > Created attachment 8546417 [details] [diff] [review] > 3. Rename Adobe Access pref > > This changes the current use of the media.eme.adobe-access.enabled pref to > media.eme-adobe.enabled, as discussed in comment 31. We'll probably want to check for media.eme.enabled as well here, but haven't verified yet.

Comment on attachment 8546417 [details] [diff] [review] 3. Rename Adobe Access pref Review of attachment 8546417 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/media/eme/MediaKeySystemAccess.cpp @@ +107,5 @@ > } > > #ifdef XP_WIN > if (aKeySystem.EqualsLiteral("com.adobe.access") && > + Preferences::GetBool("media.eme-adobe.enabled", false) && I think this should be media.eme.adobe.enabled, since that's the form the other eme pref, and other media prefs uses.

(In reply to Chris Pearce (:cpearce) from comment #37) > Comment on attachment 8546417 [details] [diff] [review] > 3. Rename Adobe Access pref > > Review of attachment 8546417 [details] [diff] [review]: > ----------------------------------------------------------------- > > ::: dom/media/eme/MediaKeySystemAccess.cpp > @@ +107,5 @@ > > } > > > > #ifdef XP_WIN > > if (aKeySystem.EqualsLiteral("com.adobe.access") && > > + Preferences::GetBool("media.eme-adobe.enabled", false) && > > I think this should be media.eme.adobe.enabled, since that's the form the > other eme pref, and other media prefs uses. I assumed we wanted media.eme-adobe.enabled (per comment 27). It does make it easier to key off of it, especially now that this is the id used in the xml (comment 20).

Comment on attachment 8546414 [details] [diff] [review] 2. GMPInstallManager (wip) Review of attachment 8546414 [details] [diff] [review]: ----------------------------------------------------------------- I would just return a status object, say e.g.: { status: xxx, // general status results: [...], // optional or default to empty, containing individual results } Lets not worry too much about the detailed format - right now the results are just interesting for testing, we can still modify this later if needed. (Just had a quick look over the rest, let me know if there is more things to consider) ::: toolkit/modules/GMPInstallManager.jsm @@ +485,1 @@ > if (!addonsToInstall.length) { Empty line before this please. @@ +488,5 @@ > } > + > + let installResults = []; > + let failureEncountered = false; > + for (let i = 0; i < addonsToInstall.length; i++) { for (let addon of addonsToInstall) @@ +491,5 @@ > + let failureEncountered = false; > + for (let i = 0; i < addonsToInstall.length; i++) { > + let threwException = false; > + let result = > + yield this.installAddon(addonsToInstall[i]).catch(function() { |yield someRejectingPromise| throws and the result.type check seems redundant. Lets just use try-catch here instead. @@ +508,5 @@ > + return Promise.reject(installResults); > + } > + return Promise.resolve(installResults); > + } catch(e) { > + log.error("Could not check for addons"); log.error("Could not check for addons", e); @@ +509,5 @@ > + } > + return Promise.resolve(installResults); > + } catch(e) { > + log.error("Could not check for addons"); > + return Promise.reject({status: "exception"}); I don't think its relevant information to callers that we had an exception. Either just reject() or use something more descriptive.

I'm posting this to pick up any feedback that there may be already. Here are my observations and questions so far: Questions: 1. We used to be able to disable the OpenH264Provider via the |media.gmp-gmpopenh264.provider.enabled| pref. As it stands, my patch no longer supports disabling the GMPProvider via a pref. Should this be added back in? I don't mind either way, I just didn't understand the reason why this addon provider could be disabled via a pref. 2. Should there be two different prefs to control logging in GMPInstallManager and GMPProvider respectively? Currently, we're using two separate prefs but they could easily be consolidated. 3. We need to get the official license text, plugin name, plugin description and homepage URL for the Adobe EME CDM (see TODO's in patch). Who could assist with this, or where could I get these from? 4. The files currently delivered for eme-adobe[1] are named "access.dll", "access.info", "access.voucher.bin" and "license.txt". However, GMPParent.cpp is actually looking for a file called "adobe.info" to register the plugin (the string "adobe" is obtained by getting the substring starting at the fifth character of the gmp-id, i.e. "eme-adobe" becomes "adobe"). Should we special case "eme-adobe" in GMPParent.cpp? Or should we ask Adobe to rename their files? FYI: If we wanted to do the same thing as OpenH264, we would need to rename the gmp-id to something like eme-emeadobe and ask Adobe to rename the files to "emeadobe.{extension}". Observations/TODOs: A. Currently, when installing eme-adobe, we install the files in a relative directory of eme-adobe/0.1/access_gmp_win_x86_alpha_7007.1/ instead of just eme-adobe/0.1/. This structure prevents GMPParent.cpp from finding the .info file. Will need to investigate why we install in this incorrect directory. B. For eme-adobe, also observe the |media.eme.enabled| to enable/disable it when appropriate. C. If answer to question 2 above was yes, add default pref for |media.gmp-provider.log| D. Add official license text, plugin name, plugin description and homepage URL for the Adobe EME CDM. E. Patch still needs thorough testing. F. Need to write tests. G. A bug should be filed to uninstall old GMP directories. Currently, we unregister the path to the GMP, but we fail to actually wipe the directory from disk. Not sure if enabling/disabling a GMP should also install it to/wipe it from disk, or if we only want to wipe outdated GMPs. [1] http://cdmdownload.adobe.com/firefox/win/x86/access_gmp_win_x86_alpha_7007.1

Comment on attachment 8549378 [details] [diff] [review] 4. Refactor GMPProvider Review of attachment 8549378 [details] [diff] [review]: ----------------------------------------------------------------- ::: toolkit/modules/GMPInstallManager.jsm @@ +650,5 @@ > gmpResults = GMPAddon.parseGMPAddonsNode(updatesChildElement); > } > } > this._deferred.resolve(gmpResults); > + this._updateLastCheck(); This accidentally slipped into this patch. I'll move this into patch 2, the refactor of GMPInstallManager.

I'm not sure if i will get to it this week (kind of work week), but i'll check on monday at the latest.

(In reply to Stephen Pohl [:spohl] from comment #40) > Observations/TODOs: > A. Currently, when installing eme-adobe, we install the files in a relative > directory of eme-adobe/0.1/access_gmp_win_x86_alpha_7007.1/ instead of just > eme-adobe/0.1/. This structure prevents GMPParent.cpp from finding the .info > file. Will need to investigate why we install in this incorrect directory. This turned out to be due to the way the zip file is structured. For OpenH264, all files are located in the root of the zip folder, i.e. openh264-macosx64-v1.1-Firefox34.zip/{filename}. The Adobe EME zip file places the files in a subdirectory, i.e. access_gmp_win_x86_alpha_7007.1.zip/access_gmp_win_x86_alpha_7007.1/{filename}. Is this something that we can ask Adobe to change? Or do we need to support this type of zip structure?

We should ask Adobe to change. ekr would that be you?

FYI: I've uploaded an eme-adobe zip that properly installs and registers[1]. This zip has all the files in the root of the zip. Also, all the files are renamed from access.{extension} to adobe.{extension} (see question 4 in comment 40 for why this is necessary). To test with this zip, you can create a local xml file with the following content: <?xml version="1.0" encoding="UTF-8"?> <updates> <addons> <addon id="eme-adobe" URL="http://people.mozilla.org/~spohl/access_gmp_win_x86_alpha_7007.1.zip" hashFunction="sha512" hashValue="25d7851d359a41944bc618cdcce447eb455f0fab5f28d22148da082d18dca5201af9810cfb4881cc73d8202c31968696b220bc8d031cca22ea3746e85c524bc1" size="3102188" version="0.1"/> </addons> </updates> Then, simply set media.gmp-manager.url.override to your local file. This will skip the SSL cert check. After restarting Firefox, eme-adobe should get installed after about a minute. [1] https://people.mozilla.org/~spohl/access_gmp_win_x86_alpha_7007.1.zip

No, it would be cpearce or henri.

(In reply to Stephen Pohl [:spohl] from comment #40) > Created attachment 8549378 [details] [diff] [review] > 4. The files currently delivered for eme-adobe[1] are named "access.dll", > "access.info", "access.voucher.bin" and "license.txt". However, > GMPParent.cpp is actually looking for a file called "adobe.info" to register > the plugin (the string "adobe" is obtained by getting the substring starting > at the fifth character of the gmp-id, i.e. "eme-adobe" becomes "adobe"). > Should we special case "eme-adobe" in GMPParent.cpp? Or should we ask Adobe > to rename their files? I will ask the Adobe engineers to change the layout of their zip file to include the .dll and .info files in the root of the zip, and re-publish the zip. > Observations/TODOs: > A. Currently, when installing eme-adobe, we install the files in a relative > directory of eme-adobe/0.1/access_gmp_win_x86_alpha_7007.1/ instead of just > eme-adobe/0.1/. This structure prevents GMPParent.cpp from finding the .info > file. Will need to investigate why we install in this incorrect directory. I'd assume the install to go into gmp-adobe/$version/. It doesn't really matter what directory the GMP goes into, provided it fits the pattern: $addon-id/$version/$addon-id.dll $addon-id/$version/$addon-id.info and provided $addon-id fits the pattern gmp-$someString. That means we need to get the addon-id in the AUS XML file changed to gmp-adobe, and ask adobe to change their .dll and .info to gmp-adobe.{dll,info}. spohl: Are you happy with using the addon-id "gmp-adobe" make sense to you? Does something else make more sense? Once we're clear on that, I will email Adobe and ask them to publish a new ZIP. Once Adobe has a new zip published, I'll ask releng to update the zip file. Adobe are developing against Nightly 2015-01-08 while we make some changes to the GMP binary interface, so unfortunately you won't be able to actually *use* the GMP you download until they both our implementations align. :( > D. Add official license text, plugin name, plugin description and homepage > URL for the Adobe EME CDM. I will ask Adobe to provide these too. It may take them a while to put together a home page, but hopefully they can at least give us a placeholder URL. > G. A bug should be filed to uninstall old GMP directories. Currently, we > unregister the path to the GMP, but we fail to actually wipe the directory > from disk. Not sure if enabling/disabling a GMP should also install it > to/wipe it from disk, or if we only want to wipe outdated GMPs. I think we should wipe outdated GMPs. If they become un-outdated (somehow) we'll end up just downloading them again, right? (In reply to Benjamin Smedberg [:bsmedberg] from comment #44) > We should ask Adobe to change. ekr would that be you? That would be me.

I'll take care of it.

s/I'll ask releng to update the zip file/I'll ask releng to update the XML file on AUS/

(In reply to Chris Pearce (:cpearce) from comment #47) > (In reply to Stephen Pohl [:spohl] from comment #40) > > Observations/TODOs: > > A. Currently, when installing eme-adobe, we install the files in a relative > > directory of eme-adobe/0.1/access_gmp_win_x86_alpha_7007.1/ instead of just > > eme-adobe/0.1/. This structure prevents GMPParent.cpp from finding the .info > > file. Will need to investigate why we install in this incorrect directory. > > I'd assume the install to go into gmp-adobe/$version/. > > It doesn't really matter what directory the GMP goes into, provided it fits > the pattern: > $addon-id/$version/$addon-id.dll > $addon-id/$version/$addon-id.info > > and provided $addon-id fits the pattern gmp-$someString. > > That means we need to get the addon-id in the AUS XML file changed to > gmp-adobe, and ask adobe to change their .dll and .info to > gmp-adobe.{dll,info}. > > spohl: Are you happy with using the addon-id "gmp-adobe" make sense to you? > Does something else make more sense? Once we're clear on that, I will email > Adobe and ask them to publish a new ZIP. We discussed this on IRC and we settled on gmp-eme-adobe. This allows us to easily identify whether or not we should respect the media.eme.enabled pref before downloading an EME GMP by looking for a substring of "gmp-eme-", starting at index 0 of the GMP id. (Note: we can't look at the .info file in the GMP because we would have to download it before deciding whether or not to download it...) > Once Adobe has a new zip published, I'll ask releng to update the zip file. > Adobe are developing against Nightly 2015-01-08 while we make some changes > to the GMP binary interface, so unfortunately you won't be able to actually > *use* the GMP you download until they both our implementations align. :( > > > D. Add official license text, plugin name, plugin description and homepage > > URL for the Adobe EME CDM. > > I will ask Adobe to provide these too. It may take them a while to put > together a home page, but hopefully they can at least give us a placeholder > URL. I'm planning to land the download of Adobe's EME CDM pref'd off (via media.gmp-eme-adobe.enabled), with reasonable placeholders until we get the official text. Currently, I have: Name: Adobe EME CDM provided by Adobe Systems Incorporated Description: Play back protected web video. License text: (The current contents of license.txt in the EME GMP downloaded from Adobe.) Homepage URL: https://www.adobe.com/ > > G. A bug should be filed to uninstall old GMP directories. Currently, we > > unregister the path to the GMP, but we fail to actually wipe the directory > > from disk. Not sure if enabling/disabling a GMP should also install it > > to/wipe it from disk, or if we only want to wipe outdated GMPs. > > I think we should wipe outdated GMPs. If they become un-outdated (somehow) > we'll end up just downloading them again, right? Agreed. I'd prefer to handle this in a separate bug though unless you disagree. We don't currently do this for OpenH264 either.

Minor cleanup, addressed feedback and changed the GMP id for Adobe EME to gmp-eme-adobe.

Renamed the Adobe Access pref to the new id: gmp-eme-adobe.

Leaving this at f? to get feedback on the questions in comment 40 before requesting a formal review. This patch adds observers on media.eme.enabled when the GMP is an EME plugin. I've also added the license text currently available in the downloadable zip to our license file. I've completely dropped the change mentioned in comment 41 since I no longer believe that it would have been correct.

Comment on attachment 8550669 [details] [diff] [review] 4. Refactor GMPProvider Review of attachment 8550669 [details] [diff] [review]: ----------------------------------------------------------------- ::: toolkit/mozapps/extensions/internal/GMPProvider.jsm @@ +128,5 @@ > + * @param aVal The value to set. > + */ > + set: function(aKey, aPlugin, aVal) { > + let log = > + Log.repository.getLoggerWithMessagePrefix("Toolkit.GMPProvider", fix indent @@ +171,5 @@ > + */ > +function GMPWrapper(aPlugin) { > + this._plugin = aPlugin; > + this._log = > + Log.repository.getLoggerWithMessagePrefix("Toolkit.GMPProvider", fix indent

Silly question, but: If I could also get some feedback on how we typically check in JavaScript whether or not an object is in an array, I'd appreciate it. I think there are currently three different ways in these patches: 1. if (arr.indexOf(obj) >= 0) 2. arr.find(function(obj) { return desiredObj == obj } 3. let found = false; for (let aObj of arr) { if (aObj == desiredObj) { found = true; break; } }

(In reply to Stephen Pohl [:spohl] from comment #50) > Description: Play back protected web video. Can we, please, avoid the loaded word "protected"? I suggest: "Play DRM video and audio"?

(In reply to Stephen Pohl [:spohl] from comment #55) > Silly question, but: If I could also get some feedback on how we typically > check in JavaScript whether or not an object is in an array, I'd appreciate > it. .indexOf() please.

(In reply to Stephen Pohl [:spohl] from comment #50) > > I think we should wipe outdated GMPs. If they become un-outdated (somehow) > > we'll end up just downloading them again, right? > > Agreed. I'd prefer to handle this in a separate bug though unless you > disagree. We don't currently do this for OpenH264 either. Did you file this yet?

Comment on attachment 8539345 [details] [diff] [review] 1. Rename OpenH264Provider to GMPProvider Review of attachment 8539345 [details] [diff] [review]: ----------------------------------------------------------------- ::: toolkit/mozapps/extensions/test/browser/browser.ini @@ +44,5 @@ > [browser_installssl.js] > [browser_newaddon.js] > skip-if = e10s > [browser_openH264.js] > +# GMPProvider.jsm is not shipped on Android Bug 1110271 landed in the mean-time, you'll have to rebase.

Comment on attachment 8539345 [details] [diff] [review] 1. Rename OpenH264Provider to GMPProvider Review of attachment 8539345 [details] [diff] [review]: ----------------------------------------------------------------- The commit message has r=bsmedberg?

Comment on attachment 8550667 [details] [diff] [review] 2. Refactor GMPInstallManager Review of attachment 8550667 [details] [diff] [review]: ----------------------------------------------------------------- I think we are missing test-updates for this in test_GMPInstallManager.js. ::: toolkit/modules/GMPInstallManager.jsm @@ +110,2 @@ > */ > + set: function(key, addon, val) { What is this change needed for? @@ +453,5 @@ > log.info("Last check was: " + secondsSinceLast + > " seconds ago, minimum seconds: " + secondsBetweenChecks); > if (secondsBetweenChecks > secondsSinceLast) { > log.info("Will not check for updates."); > return Promise.resolve({status: "too-frequent-no-check"}); |return {status: ...};| @@ +465,3 @@ > log.info("Found addon: " + gmpAddon.toString()); > + > + let addonUpdateEnabled = true; Default to |false| and you could drop a conditional branch below? @@ +465,5 @@ > log.info("Found addon: " + gmpAddon.toString()); > + > + let addonUpdateEnabled = true; > + if (gmpAddon.id.indexOf("gmp-eme-") == 0) { > + addonUpdateEnabled = this._isAddonUpdateEnabled(EME_ID); This seems confusing as there is no specific addon with the id "eme" - can't we just use a separate pref instead of overloading _isAddonUpdateEnabled? @@ +467,5 @@ > + let addonUpdateEnabled = true; > + if (gmpAddon.id.indexOf("gmp-eme-") == 0) { > + addonUpdateEnabled = this._isAddonUpdateEnabled(EME_ID); > + if (!addonUpdateEnabled) { > + log.info("Auto-update is off for all EME plugins, skipping check."); |return false;| and lose one level of nesting below. @@ +479,5 @@ > + knownAddon = true; > + break; > + } > + } > + if (knownAddon) { if (GMP_ADDONS.indexOf(gmpAddon.id) != -1) ... @@ +493,5 @@ > + "', skipping check."); > + } > + } > + > + return addonUpdateEnabled && gmpAddon.isValid && !gmpAddon.isInstalled; How about early-returning on isValid and isInstalled and skipping the other work? @@ +500,3 @@ > if (!addonsToInstall.length) { > log.info("No new addons to install, returning"); > + return Promise.resolve({status: "nothing-new-to-install"}); Just |return {status: ...};| @@ +504,5 @@ > + > + let installResults = []; > + let failureEncountered = false; > + for (let addon of addonsToInstall) { > + let result = "Installation of " + addon.id; I don't think the string adds much value here. For the test change we'll probably want to do something like: installResults.push({ id: addon.id, result: /*success or failure value*/, }); @@ +519,5 @@ > + return Promise.reject({status: "failed", > + results: installResults}); > + } > + return Promise.resolve({status: "succeeded", > + results: installResults}); Just |return {status: ...};|

Comment on attachment 8550668 [details] [diff] [review] 3. Rename Adobe Access pref Review of attachment 8550668 [details] [diff] [review]: ----------------------------------------------------------------- While this is trivial, i'm not a peer and don't know how review policy is for dom, media, etc. cpearce, i see you in hg history?

(In reply to Stephen Pohl [:spohl] from comment #40) > Created attachment 8549378 [details] [diff] [review] > 4. Refactor GMPProvider > > I'm posting this to pick up any feedback that there may be already. Here are > my observations and questions so far: > > Questions: > 1. We used to be able to disable the OpenH264Provider via the > |media.gmp-gmpopenh264.provider.enabled| pref. As it stands, my patch no > longer supports disabling the GMPProvider via a pref. Should this be added > back in? I don't mind either way, I just didn't understand the reason why > this addon provider could be disabled via a pref. This is in toolkit and not all applications using toolkit want this provider. > 2. Should there be two different prefs to control logging in > GMPInstallManager and GMPProvider respectively? Currently, we're using two > separate prefs but they could easily be consolidated. I'd be happy to see them controlled by one. > 3. We need to get the official license text, plugin name, plugin description > and homepage URL for the Adobe EME CDM (see TODO's in patch). Who could > assist with this, or where could I get these from? Thats a question for the media people :) > B. For eme-adobe, also observe the |media.eme.enabled| to enable/disable it > when appropriate. Yes please. > C. If answer to question 2 above was yes, add default pref for > |media.gmp-provider.log| We don't really need default prefs. We should just default to a reasonable value if the pref does not exist. > E. Patch still needs thorough testing. > F. Need to write tests. Yes please :) > G. A bug should be filed to uninstall old GMP directories. Currently, we > unregister the path to the GMP, but we fail to actually wipe the directory > from disk. Not sure if enabling/disabling a GMP should also install it > to/wipe it from disk, or if we only want to wipe outdated GMPs. I don't think enable/disable should remove an up-to-date GMP.

Comment on attachment 8550669 [details] [diff] [review] 4. Refactor GMPProvider Review of attachment 8550669 [details] [diff] [review]: ----------------------------------------------------------------- See also the addressed questions above. ::: toolkit/locales/en-US/chrome/mozapps/extensions/extensions.properties @@ +56,5 @@ > notification.installError=There was an error installing %1$S. > notification.installError.retry=Try again > notification.installError.retry.tooltip=Try downloading and installing this add-on again > +#LOCALIZATION NOTE (notification.gmpPending) GMP plugin will be automatically installed later. > +notification.gmpPending=Will be installed shortly. I think we should format the name here and below ("%1$S will be automatically ..."). ::: toolkit/mozapps/extensions/content/extensions.js @@ +52,5 @@ > const XMLURI_PARSE_ERROR = "http://www.mozilla.org/newlayout/xml/parsererror.xml" > > const VIEW_DEFAULT = "addons://discover/"; > > +const GMP_ADDON_IDS = ["gmp-gmpopenh264", "gmp-eme-adobe"]; I don't like that we have to update the IDs every time we add some GMP. We should be able to just use |this.mAddon.isGMPlugin| to decide whether we need to show this. ::: toolkit/mozapps/extensions/content/extensions.xml @@ +1189,5 @@ > </method> > > <method name="_updateState"> > <body><![CDATA[ > + let GMP_ADDON_IDS = ["gmp-gmpopenh264", "gmp-eme-adobe"]; I don't like that we have to update the IDs every time we add some GMP. We should be able to just use |this.mAddon.isGMPlugin| to decide whether we need to show this. ::: toolkit/mozapps/extensions/internal/GMPProvider.jsm @@ -36,5 @@ > -const OPENH264_PREF_ENABLED = "enabled"; > -const OPENH264_PREF_VERSION = "version"; > -const OPENH264_PREF_LASTUPDATE = "lastUpdate"; > -const OPENH264_PREF_AUTOUPDATE = "autoupdate"; > -const OPENH264_PREF_PROVIDERENABLED = "provider.enabled"; Don't drop this. @@ +48,5 @@ > +// Note regarding the value of |fullDescription| below: This is part of an awful > +// hack to include the licenses for GMP plugins without having bug 624602 fixed > +// yet, and intentionally ignores localisation. > +const GMP_PLUGINS = [ > + { id: "gmp-gmpopenh264", Nit: id on new line. @@ +72,5 @@ > let gLogger; > let gLogDumping = false; > let gLogAppenderDump = null; > > +// Note: disabling logging currently requires a restart to take effect. Side-note: This is not too hard to change, see e.g. configureLogging in Experiments.jsm. @@ +168,5 @@ > +/** > + * The GMPWrapper provides the info for the various GMP plugins to public > + * callers through the API. > + */ > +function GMPWrapper(aPlugin) { Nit: s/aPlugin/aPluginInfo/ or something like that? @@ +175,5 @@ > + Log.repository.getLoggerWithMessagePrefix("Toolkit.GMPProvider", > + "GMPWrapper" + "::" + > + this._plugin.id + "::"); > + if (this._plugin.id.indexOf("gmp-eme-") == 0) { > + this._isEME = true; We could have an (optional) isEme flag on GMP_PLUGINS above instead. @@ +494,5 @@ > > shutdown: function() { > this._log.trace("shutdown()"); > + for (let id in this._plugins) { > + this._plugins[id].wrapper.shutdown(); Notice how this returned OpenH264Wrapper._updateTask before this change. You will have to wait for the updateTasks of the wrappers. @@ +496,5 @@ > this._log.trace("shutdown()"); > + for (let id in this._plugins) { > + this._plugins[id].wrapper.shutdown(); > + } > + this._plugins.forEach(id => this._plugins[id].wrapper.shutdown()); Lets only shut them down once. @@ +514,5 @@ > } > }, > > getAddonsByTypes: function(aTypes, aCallback) { > + if (aTypes && aTypes.indexOf("plugin") < 0) { Don't drop .isEnabled @@ +535,4 @@ > }, > > + buildPluginList: function() { > + let list = {}; I'd prefer this to be a proper Map(): https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Map @@ -383,4 @@ > }, > > - get isEnabled() { > - return prefs.get(OPENH264_PREF_PROVIDERENABLED, false); Don't drop this. @@ +544,5 @@ > + fullDescription: aPlugin.fullDescription, > + homepageURL: aPlugin.homepageURL, > + optionsURL: aPlugin.optionsURL, > + wrapper: null > + } Missing semicolon. @@ +550,5 @@ > + if (wrapper.isInstalled) { > + let gmpPath = OS.Path.join(OS.Constants.Path.profileDir, plugin.id, > + GMPPrefs.get(KEY_PLUGIN_VERSION, plugin.id, > + null)); > + wrapper.gmpPath = gmpPath; Why are we setting this from outside of the wrapper? We can just build the path dynamically in |get gmpPath()|. @@ +552,5 @@ > + GMPPrefs.get(KEY_PLUGIN_VERSION, plugin.id, > + null)); > + wrapper.gmpPath = gmpPath; > + } > + plugin.wrapper = wrapper; This looks cyclic.

Comment on attachment 8550667 [details] [diff] [review] 2. Refactor GMPInstallManager Review of attachment 8550667 [details] [diff] [review]: ----------------------------------------------------------------- ::: toolkit/modules/GMPInstallManager.jsm @@ +516,5 @@ > + installResults.push(result); > + } > + if (failureEncountered) { > + return Promise.reject({status: "failed", > + results: installResults}); Yoric says that from a Task, throwing is better for rejection. So |throw {status: ...};|.

(In reply to Georg Fritzsche [:gfritzsche] [slow to respond until Jan 19] from comment #64) > > + plugin.wrapper = wrapper; > > This looks cyclic. But apparently we dont really mind that in JS (as long as its localized cycles that dont block the GC of course).

(In reply to Georg Fritzsche [:gfritzsche] [slow to respond until Jan 19] from comment #58) > (In reply to Stephen Pohl [:spohl] from comment #50) > > > I think we should wipe outdated GMPs. If they become un-outdated (somehow) > > > we'll end up just downloading them again, right? > > > > Agreed. I'd prefer to handle this in a separate bug though unless you > > disagree. We don't currently do this for OpenH264 either. > > Did you file this yet? Filed as bug 1123785.

Updated for current trunk, updated reviewer in commit message. Carrying over r+.

Updated commit message. Carrying over r+.

Addressed feedback. > ::: toolkit/modules/GMPInstallManager.jsm > @@ +110,2 @@ > > */ > > + set: function(key, addon, val) { > > What is this change needed for? I thought the original structure was confusing, since GMPPrefs.get() took |addon| as second and |defaultValue| as third argument. After updating the tests, I see now that the original structure had the benefit of making |addon| optional. I went ahead and reverted this change.

Addressed feedback. Setting this to f? instead of r? for now due to an issue in updating test_openh264.js. For some reason, setting GMPScope.gmpService to MockGMPService[1] does not seem to have any effect for the calls to gmpService in GMPWrapper.onPrefEnabledChanged and GMPWrapper.onPrefVersionChanged. However, it still seems to work for the calls to gmpService in GMPProvider.startup. I'm saying this because the startup-related tests[2] pass, but tests related to a version pref change fail[3]. So, for some reason we're able to add the plugin directory to the |addedPaths| array during startup but we're unable to add them to either |addedPaths| or |removedPaths| on pref changes. Georg, do you have any idea what might be going on here? (In reply to Georg Fritzsche [:gfritzsche] from comment #64) > ::: toolkit/mozapps/extensions/internal/GMPProvider.jsm > @@ +494,5 @@ > > > > shutdown: function() { > > this._log.trace("shutdown()"); > > + for (let id in this._plugins) { > > + this._plugins[id].wrapper.shutdown(); > > Notice how this returned OpenH264Wrapper._updateTask before this change. > You will have to wait for the updateTasks of the wrappers. I'm now returning |this._updateTasks| from GMPWrapper.shutdown(). Is this sufficient? [1] http://mxr.mozilla.org/mozilla-central/source/toolkit/mozapps/extensions/test/xpcshell/test_openh264.js#196 [2] http://mxr.mozilla.org/mozilla-central/source/toolkit/mozapps/extensions/test/xpcshell/test_openh264.js#203 [3] http://mxr.mozilla.org/mozilla-central/source/toolkit/mozapps/extensions/test/xpcshell/test_openh264.js#210

A change to GMPInstallManager accidentally slipped into the patch for GMPProvider.

I think I'll go ahead and split out the updates to the existing tests into a separate patch. Right now, they're scattered across multiple patches. This should make it easier to follow. Aside from this, there will be two more patches: 1. New tests for Adobe EME, based on browser_openH264.js and test_openh264.js, with the main difference that we'll also test the global pref "media.eme.enabled". 2. A patch that prefs Adobe EME off by default and completely hides it in the addon manager.

(In reply to Stephen Pohl [:spohl] from comment #74) > 1. New tests for Adobe EME, based on browser_openH264.js and > test_openh264.js, with the main difference that we'll also test the global > pref "media.eme.enabled". Or possibly morph them into browser_gmpProvider/test_gmpProvider?

Comment on attachment 8553732 [details] [diff] [review] 2. Refactor GMPInstallManager Review of attachment 8553732 [details] [diff] [review]: ----------------------------------------------------------------- r+ for this patch except browser_openH264.js & test_openH264.js (which will be broken out to a different patch. ::: toolkit/modules/GMPInstallManager.jsm @@ +473,5 @@ > + log.info("Addon invalid or already installed."); > + return false; > + } > + > + if (gmpAddon.id.indexOf("gmp-eme-") == 0 && !this._isEMEEnabled) { Please comment on why that "gmp-eme-" check works.

(In reply to Georg Fritzsche [:gfritzsche] from comment #76) > Comment on attachment 8553732 [details] [diff] [review] > 2. Refactor GMPInstallManager > > Review of attachment 8553732 [details] [diff] [review]: > ----------------------------------------------------------------- > > r+ for this patch except browser_openH264.js & test_openH264.js (which will > be broken out to a different patch. > > ::: toolkit/modules/GMPInstallManager.jsm > @@ +473,5 @@ > > + log.info("Addon invalid or already installed."); > > + return false; > > + } > > + > > + if (gmpAddon.id.indexOf("gmp-eme-") == 0 && !this._isEMEEnabled) { > > Please comment on why that "gmp-eme-" check works. Also, now that we have consolidated the logging implementations, we should not configure the logger twice (especially not add two dump appenders etc.). Let's just remove the parts here: > let parentLogger = Log.repository.getLogger(PARENT_LOGGER_ID); > parentLogger.level = gLogEnabled ? Log.Level.Debug : Log.Level.Fatal; > let appender = new Log.DumpAppender(); > parentLogger.addAppender(appender); That means you can also remove the "gLogEnabled" lazy getter.

Comment on attachment 8553733 [details] [diff] [review] 4. Refactor GMPProvider Review of attachment 8553733 [details] [diff] [review]: ----------------------------------------------------------------- Note: I didn't check the tests as they will be broken out into a new patch. Apart from the patches and the comments below i think we are nearly good here. ::: toolkit/mozapps/extensions/internal/GMPProvider.jsm @@ +85,2 @@ > > + let logDumping = GMPPrefs.get(KEY_LOG_ENABLED, false); "enabled" is different than "dump". By default things just go to the browser console (AFAIR), adding the dump appender means you get logging to the terminal etc. @@ +113,5 @@ > + * Obtains the specified preference in relation to the specified plugin. > + * @param aKey The preference key value to use. > + * @param aPlugin The plugin to scope the preference to. > + * @param aDefaultValue The default value if no preference exists. > + * @return The obtained preference value, or the defaultVlaue if none exists. Nit: Typo with defaultVlaue @@ +127,5 @@ > + */ > + set: function(aKey, aVal, aPlugin) { > + let log = > + Log.repository.getLoggerWithMessagePrefix("Toolkit.GMPProvider", > + "GMPPrefs.set" + "::"); The way its used here you end up with a message like: > Toolkit.GMPProvider GMPPrefs.set::Setting pref: I think you want this: getLoggerWithMessagePrefix("Toolkit.GMPProvider","GMPPrefs::set ") @@ +346,3 @@ > } catch (e) { > + this._log.error("findUpdates() - updateTask for " + this._plugin.id + > + " threw: " + e); While we are changing this, Log.jsm can do this now: this._log.error("foo threw", e); @@ +447,5 @@ > this._log = Log.repository.getLoggerWithMessagePrefix("Toolkit.GMPProvider", > "GMPProvider" + "::"); > + let telemetry = {}; > + if (!this._plugins) { > + this.buildPluginList(); This is where the provider is initialized. this._plugins is expected to be null here - just skip the null check and build the plugin list. That also means that you can remove the other null-check/buildPluginList() below. @@ +453,5 @@ > > + Services.obs.addObserver(this, AddonManager.OPTIONS_NOTIFICATION_DISPLAYED, > + false); > + Preferences.observe(GMPPrefs.get(KEY_LOG_ENABLED), configureLogging); > + Preferences.observe(GMPPrefs.get(KEY_LOGGING_LEVEL), configureLogging); You only need to observe() KEY_LOG_BASE, you will get notified for changes in that pref branch. @@ +458,2 @@ > > + for (let [id, plugin] of this._plugins.entries()) { Just: for (let [id,plugins] of this._plugins) { @@ +483,5 @@ > } > > + try { > + let greBinDir = Services.dirsvc.get(NS_GRE_BIN_DIR, > + Ci.nsILocalFile); You are skipping the EME_PREF_ENABLED check for clearkey here now? @@ +503,5 @@ > Services.obs.removeObserver(this, AddonManager.OPTIONS_NOTIFICATION_DISPLAYED); > + Preferences.ignore(GMPPrefs.get(KEY_LOG_ENABLED), configureLogging); > + Preferences.ignore(GMPPrefs.get(KEY_LOGGING_LEVEL), configureLogging); > + for (let plugin of this._plugins.values()) { > + plugin.wrapper.shutdown(); wrapper.shutdown() returning its updateTask is a first step, but now we need to return something from GMPProvider.shutdown() that allows the addon manager to wait on all of them. E.g. some chained promise or some task that waits on all of them. @@ +515,5 @@ > + } > + > + if (!this._plugins) { > + this.buildPluginList(); > + } This can be removed. @@ +534,5 @@ > } > > + if (!this._plugins) { > + this.buildPluginList(); > + } This can be removed. @@ +539,5 @@ > + > + let results = []; > + for (let id of this._plugins.keys()) { > + this.getAddonByID(id, function(aAddon) { > + results.push(aAddon); I think we should simplify this. E.g.: let results = [p.wrapper for ([id, p] of this._plugins)];

Updated for current trunk, carrying over r+.

Comment on attachment 8553733 [details] [diff] [review] 4. Refactor GMPProvider Review of attachment 8553733 [details] [diff] [review]: ----------------------------------------------------------------- Drive-by semicolon nits that showed up in my editor while I was looking over this code. ::: toolkit/mozapps/extensions/internal/GMPProvider.jsm @@ +35,5 @@ > +/** > + * Keys which can be used via GMPPrefs. > + */ > +const KEY_PROVIDER_ENABLED = "media.gmp-provider.enabled"; > +const KEY_LOG_BASE = "media.gmp" Nit: missing semicolon. @@ +331,5 @@ > try { > let installManager = new GMPInstallManager(); > + let gmpAddons = yield installManager.checkForAddons(); > + let update = gmpAddons.find(function(aAddon) { > + return aAddon.id === this._plugin.id Nit: missing semicolon @@ +437,5 @@ > }, > }; > > let GMPProvider = { > + get name() { "GMPProvider" }, Nit: missing semicolon @@ +486,5 @@ > + let greBinDir = Services.dirsvc.get(NS_GRE_BIN_DIR, > + Ci.nsILocalFile); > + let clearkeyPath = OS.Path.join(greBinDir.path, > + CLEARKEY_PLUGIN_ID, > + CLEARKEY_VERSION) Nit: missing semicolon

Comment on attachment 8553733 [details] [diff] [review] 4. Refactor GMPProvider Review of attachment 8553733 [details] [diff] [review]: ----------------------------------------------------------------- ::: toolkit/mozapps/extensions/internal/GMPProvider.jsm @@ +115,5 @@ > + * @param aPlugin The plugin to scope the preference to. > + * @param aDefaultValue The default value if no preference exists. > + * @return The obtained preference value, or the defaultVlaue if none exists. > + */ > + get: function(aKey, aPlugin, aDefaultValue) { It looks like you're trying to overload the `get` method but that doesn't work in JS AFAIK.

(In reply to Matthew N. [:MattN] from comment #80) > Comment on attachment 8553733 [details] [diff] [review] > Drive-by semicolon nits that showed up in my editor while I was looking over > this code. Thanks! (In reply to Matthew N. [:MattN] from comment #81) > Comment on attachment 8553733 [details] [diff] [review] > > + get: function(aKey, aPlugin, aDefaultValue) { > > It looks like you're trying to overload the `get` method but that doesn't > work in JS AFAIK. You're right, of course. I caught this for `set` but completely forgot that I also tried to overload `get`. Thanks for catching!

Addressed review feedback and changed GMPPrefs.get to take |defaultValue| as second and |addon| as third argument to match GMPPrefs.set and GMPProvider's GMPPrefs.get. Carrying over r+.

Addressed feedback and split out tests.

Work in progress. Updated existing tests due to the refactor of GMPInstallManager and GMPProvider and added tests for gmp-eme-adobe to browser_gmpProvider.js.

Comment on attachment 8555029 [details] [diff] [review] 4. Refactor GMPProvider Review of attachment 8555029 [details] [diff] [review]: ----------------------------------------------------------------- ::: toolkit/mozapps/extensions/internal/GMPProvider.jsm @@ +174,5 @@ > + } > + > + > +} > +GMPWrapper.prototype = { Nit: Empty line before this. @@ +499,5 @@ > + for (let plugin of this._plugins.values()) { > + try { > + let updateTask = plugin.wrapper.shutdown(); > + if (updateTask) { > + yield updateTask; You don't need to null-check. |yield null| is fine and we don't need to guard on it. @@ +509,5 @@ > > + this._plugins = null; > + > + if (!shutdownSucceeded) { > + throw { status: "shutdown failed" }; We weren't returning a defined value here before. Lets just |throw new Error("Shutdown failed")| @@ +514,2 @@ > } > + return { status: "shutdown succeeded" }; I don't think we need to return a specific value here, the promise resolving implies success.

We want to disable Adobe EME by default until we're ready for primetime. Also, we don't want to display it in the addon manager when disabled.

Comment on attachment 8555469 [details] [diff] [review] 6. Disable Adobe EME by default and hide from addon manager Review of attachment 8555469 [details] [diff] [review]: ----------------------------------------------------------------- ::: toolkit/mozapps/extensions/internal/GMPProvider.jsm @@ +565,5 @@ > }; > + > + // Don't show Adobe EME in addon manager when disabled. > + if (!plugin.isEME || (GMPPrefs.get(KEY_EME_ENABLED, false) && > + GMPPrefs.get(KEY_PLUGIN_ENABLED, false, "gmp-eme-adobe"))) { I don't think we should overload the enabled pref that way. If you need to make code changes, you could as well 1) just remove gmp-eme-adobe or 2) add a hidden pref to disable specific GMPs 2) means anyone could just remove the disable pref from all.js etc. when we decide to turn it on for a channel. Opinions?

This should cover what we discussed on irc.

Comment on attachment 8556014 [details] [diff] [review] 6. Disable Adobe EME by default and hide from addon manager Review of attachment 8556014 [details] [diff] [review]: ----------------------------------------------------------------- I think you also need to turn auto-update off for gmp-eme-adobe by default or add another check against the pref in GMPInstallManager.simpleCheckAndInstall(). Otherwise it will get auto-installed but hidden.

Yes, of course. :-/

Missed comma.

Comment on attachment 8556060 [details] [diff] [review] 6. Disable Adobe EME by default and hide from addon manager Review of attachment 8556060 [details] [diff] [review]: ----------------------------------------------------------------- ::: toolkit/mozapps/extensions/internal/GMPProvider.jsm @@ +562,5 @@ > isEME: aPlugin.isEME > }; > + > + // Only show GMPs in addon manager that aren't hidden. > + if (!GMPPrefs.get(KEY_PLUGIN_HIDDEN, false, plugin.id)) { Lets move the check up before |let plugin =| and skip retrieving the strings from the bundle etc.

Addressed feedback. Carrying over r+.

Comment on attachment 8555029 [details] [diff] [review] 4. Refactor GMPProvider Review of attachment 8555029 [details] [diff] [review]: ----------------------------------------------------------------- ::: toolkit/mozapps/extensions/internal/GMPProvider.jsm @@ +149,5 @@ > + * @param aPlugin The plugin to scope the preference to. > + * @return A preference key scoped to the specified plugin. > + */ > + getPrefKey: function(aKey, aPlugin) { > + return aKey.replace("{0}", aPlugin || ""); Nit: extra space after `return` @@ +172,5 @@ > + Preferences.observe(GMPPrefs.getPrefKey(KEY_EME_ENABLED, this._plugin.id), > + this.onPrefEnabledChanged, this); > + } > + > + Nit: extra lines @@ +174,5 @@ > + } > + > + > +} > +GMPWrapper.prototype = { > Nit: Empty line before this. Actually, it's common to have the constructor immediately above the prototype object without a new line in between

(In reply to Matthew N. [:MattN] from comment #95) > @@ +174,5 @@ > > + } > > + > > + > > +} > > +GMPWrapper.prototype = { > > > Nit: Empty line before this. > > Actually, it's common to have the constructor immediately above the > prototype object without a new line in between Is it? At least this file has new lines separating everything, i thought we value consistency.

(In reply to Georg Fritzsche [:gfritzsche] from comment #96) > (In reply to Matthew N. [:MattN] from comment #95) > > @@ +174,5 @@ > > > + } > > > + > > > + > > > +} > > > +GMPWrapper.prototype = { > > > > > Nit: Empty line before this. > > > > Actually, it's common to have the constructor immediately above the > > prototype object without a new line in between > > Is it? At least this file has new lines separating everything, i thought we > value consistency. If this file doesn't do that then I guess we should be consistent and put a new line.

Minor update to get logging working again. Carrying over r+.

Addressed feedback. Also, the last iteration of patches inadvertently broke logging. Although we were setting up the logger in GMPProvider with "Toolkit.GMPProvider" we tried to get the logger as "GMPInstallManager" in GMPInstallManager. Also, we no longer respected the media.gmp.log pref. Georg, I'm setting this back to r? to make sure that what I'm doing in configureLogging() looks sane to you. My testing showed that all logging-related prefs are working correctly now, both from GMPProvider.jsm as well as GMPInstallManager.jsm. Flipping prefs also works without restarting the browser.

This updates all existing tests to work with and test Adobe EME. I've tried to change the tests so that they're able to pick up any future GMPs without further changes. AFAICT, the only thing left to do here is to add tests for the media.eme.enabled pref.

Comment on attachment 8556260 [details] [diff] [review] 4. Refactor GMPProvider Review of attachment 8556260 [details] [diff] [review]: ----------------------------------------------------------------- ::: toolkit/mozapps/extensions/internal/GMPProvider.jsm @@ +35,5 @@ > + */ > +const KEY_PROVIDER_ENABLED = "media.gmp-provider.enabled"; > +const KEY_PROVIDER_LASTCHECK = "media.gmp-manager.lastCheck"; > +const KEY_LOG_BASE = "media.gmp"; > +const KEY_LOG_ENABLED = KEY_LOG_BASE + ".log"; KEY_LOG_BASE should be "media.gmp.log.". I don't think we need a "log enabled" pref. @@ +36,5 @@ > +const KEY_PROVIDER_ENABLED = "media.gmp-provider.enabled"; > +const KEY_PROVIDER_LASTCHECK = "media.gmp-manager.lastCheck"; > +const KEY_LOG_BASE = "media.gmp"; > +const KEY_LOG_ENABLED = KEY_LOG_BASE + ".log"; > +const KEY_LOGGING_LEVEL = KEY_LOG_BASE + ".logging.level"; KEY_LOG_BASE + "level". @@ +37,5 @@ > +const KEY_PROVIDER_LASTCHECK = "media.gmp-manager.lastCheck"; > +const KEY_LOG_BASE = "media.gmp"; > +const KEY_LOG_ENABLED = KEY_LOG_BASE + ".log"; > +const KEY_LOGGING_LEVEL = KEY_LOG_BASE + ".logging.level"; > +const KEY_LOGGING_DUMP = KEY_LOG_BASE + ".logging.dump"; KEY_LOG_BASE + "dump". @@ +79,3 @@ > > function configureLogging() { > + if (!GMPPrefs.get(KEY_LOG_ENABLED, false)) { Lets just always enable the log. We should always log warnings & errors to the browser console. @@ +105,1 @@ > if (logDumping != gLogDumping) { We could just use (logDumping != !!gLogAppenderDump).

This drops the "media.gmp.log" pref from all.js. Carrying over r+.

Addressed feedback. Thanks Georg, this makes it a lot neater! I've confirmed that logging continues to work as expected.

Updated for latest patch "4. Refactor GMPProvider". Carrying over r+.

This adds tests for the "media.eme.enabled" pref. It also adds a test to ensure that hidden GMPs are properly hidden in the addon manager.

Comment on attachment 8557143 [details] [diff] [review] 4. Refactor GMPProvider Review of attachment 8557143 [details] [diff] [review]: ----------------------------------------------------------------- Thanks, that looks good to me.

Comment on attachment 8557305 [details] [diff] [review] 6. Tests Review of attachment 8557305 [details] [diff] [review]: ----------------------------------------------------------------- I think test_GMPInstallManager.js needs an update to cover installation and update of N GMPs both for checkForAddons() and simpleCheckAndInstall(). A test of the new failure paths (where possible) would also be good. Side-note: an ignore-whitespace patch would have been helpful with all the indentation changes. ::: toolkit/modules/tests/xpcshell/test_GMPInstallManager.js @@ +14,5 @@ > > do_get_profile(); > > function run_test() {Cu.import("resource://gre/modules/Preferences.jsm") > + Preferences.set("media.gmp.log", true); This needs to set |media.gmp.log.level| to Log.Level.Trace or .Info ::: toolkit/mozapps/extensions/test/browser/browser_gmpProvider.js @@ +31,4 @@ > > let gInstalledAddonId = ""; > let gInstallDeferred = null; > +let prefs = Services.prefs; gPrefs ::: toolkit/mozapps/extensions/test/xpcshell/test_gmpProvider.js @@ +11,5 @@ > > +let gMockAddons = []; > +let gAddonIds = []; > +let gMockEmeAddons = []; > +let gEmeAddonIds = [] Given the uses below i would just use 2 Maps (id -> mockAddon) instead of the 4 arrays here. @@ +32,3 @@ > > let gInstalledAddonId = ""; > +let prefs = Services.prefs; gPrefs @@ +78,4 @@ > > + let mockAddon = null; > + for (let tempMockAddon of gMockAddons) { > + if (tempMockAddon.id === addon.id) { * just use .find()? * or better, just build a Map above of (id -> mockAddon) @@ +123,5 @@ > const TEST_DATE = new Date(2013, 0, 1, 12); > const TEST_VERSION = "1.2.3.4"; > const TEST_TIME_SEC = Math.round(TEST_DATE.getTime() / 1000); > > + let addons = yield promiseAddonsByIDs(gAddonIds); This could be replaced by promiseAddonsByIDs([...gMockAddons.keys()]) @@ +129,4 @@ > > + for (let addon of addons) { > + let mockAddon = null; > + for (let tempMockAddon of gMockAddons) { Map please. @@ +175,4 @@ > }); > > add_task(function* test_enable() { > + let addons = yield promiseAddonsByIDs(gAddonIds); [...gMockAddons.keys()] @@ +190,5 @@ > + } > +}); > + > +add_task(function* test_globalEmeDisabled() { > + let addons = yield promiseAddonsByIDs(gEmeAddonIds); Same here, [...gEmeAddonIds.keys()] etc.

Updated for current trunk. Carrying over r+.

Comment on attachment 8557143 [details] [diff] [review] 4. Refactor GMPProvider Review of attachment 8557143 [details] [diff] [review]: ----------------------------------------------------------------- ::: toolkit/mozapps/extensions/content/EMEAdobe-license.txt @@ +1,1 @@ > +Adobe Access GMP Plugin for Firefox Note: Adobe are changing their DRM systems branding to "Primetime", rather than "Access". I assume this text file won't be landing anyway since we're going to point to the EULA on the server hosted by them, I just want to call this out so it doesn't fall through the cracks.

Ben, are you able to update the xml file to the new id for Adobe EME (see comment 50 for background)? The new id is "gmp-eme-adobe" and I was looking at this xml file (not sure if there are any others at the moment): https://aus4-dev.allizom.org/update/3/GMP/36.0a2/20120222174716/WINNT_x86-msvc/en-US/nightly/default/default/default/update.xml If you'd prefer that I file a bug for this, just let me know. Thanks!

(In reply to Stephen Pohl [:spohl] from comment #113) > Ben, are you able to update the xml file to the new id for Adobe EME (see > comment 50 for background)? The new id is "gmp-eme-adobe" and I was looking > at this xml file (not sure if there are any others at the moment): > https://aus4-dev.allizom.org/update/3/GMP/36.0a2/20120222174716/WINNT_x86- > msvc/en-US/nightly/default/default/default/update.xml > > If you'd prefer that I file a bug for this, just let me know. Thanks! Done

(In reply to Ben Hearsum [:bhearsum] from comment #114) > Done Awesome, thanks! :-)

Addressed feedback. (In reply to Georg Fritzsche [:gfritzsche] from comment #110) > Comment on attachment 8557305 [details] [diff] [review] > 6. Tests > > Review of attachment 8557305 [details] [diff] [review]: > ----------------------------------------------------------------- > > I think test_GMPInstallManager.js needs an update to cover installation and > update of N GMPs both for checkForAddons() AFAICT, checkForAddons already covers installations and updates for N GMPs (see |test_checkForAddons_multipleAddonNoUpdatesSomeInvalid| and |test_checkForAddons_updatesWithAddons|). > and simpleCheckAndInstall(). > A test of the new failure paths (where possible) would also be good. Once I started looking into this, I realized that all three simpleCheckAndInstall() tests should have been failing after my changes to GMPInstallManager.jsm, but they didn't. They reported neither pass nor fail and the test harness simply assumed that they passed! I've since revisited each test in this file and they're now passing as expected. One of the main culprits was the use of both add_test() and add_task(), which does not seem to go well in xpcshell tests. In some places, we would also call run_next_test() before the current test actually finished and other silly things. Since we've actually removed instances where simpleCheckAndInstall() used to return rejected promises, I don't believe we need additional tests for simpleCheckAndInstall(). AFAICT, there are no new failure paths here. Did I miss something? > Side-note: an ignore-whitespace patch would have been helpful with all the > indentation changes. Sorry, I had no idea I could do this in mercurial. I'll see if the bugzilla interdiff between the previous and current "patch 6" is usable. If not, I'll post a separate interdiff patch that should make reviewing the differences easier. > ::: toolkit/mozapps/extensions/test/xpcshell/test_gmpProvider.js > @@ +123,5 @@ > > const TEST_DATE = new Date(2013, 0, 1, 12); > > const TEST_VERSION = "1.2.3.4"; > > const TEST_TIME_SEC = Math.round(TEST_DATE.getTime() / 1000); > > > > + let addons = yield promiseAddonsByIDs(gAddonIds); > > This could be replaced by promiseAddonsByIDs([...gMockAddons.keys()]) Excuse my ignorance, but what is this notation? ^^^

Comment on attachment 8563880 [details] [diff] [review] 6. Tests Review of attachment 8563880 [details] [diff] [review]: ----------------------------------------------------------------- ::: toolkit/modules/tests/xpcshell/test_GMPInstallManager.js @@ +48,5 @@ > GMPPrefs.set(GMPPrefs.KEY_ADDON_AUTOUPDATE, true, addon2); > run_next_test(); > }); > > + removed this extra new line locally.

Minor improvement to the format of one of our log statements. Carrying over r+.

(In reply to Chris Pearce (:cpearce) from comment #112) > Comment on attachment 8557143 [details] [diff] [review] > 4. Refactor GMPProvider > > Review of attachment 8557143 [details] [diff] [review]: > ----------------------------------------------------------------- > > ::: toolkit/mozapps/extensions/content/EMEAdobe-license.txt > @@ +1,1 @@ > > +Adobe Access GMP Plugin for Firefox > > Note: Adobe are changing their DRM systems branding to "Primetime", rather > than "Access". I assume this text file won't be landing anyway since we're > going to point to the EULA on the server hosted by them, I just want to call > this out so it doesn't fall through the cracks. Thanks, Chris. I've gone ahead and revised this patch a bit. Georg, would you mind having one more look at this? I'll be posting an interdiff (between this patch and the previous version that you've r+'d) shortly.

Interdiff between attachment 8557143 [details] [diff] [review] and attachment 8563887 [details] [diff] [review] to facilitate review.

I tried kicking off a try run but the trees are closed at the moment. Will try again tomorrow morning.

As discussed on IRC. All 100 tests pass.

Updated for current trunk. See comment 119 and comment 120 for details on this review request.

Comment on attachment 8563889 [details] [diff] [review] Interdiff for patch 4 Review of attachment 8563889 [details] [diff] [review]: ----------------------------------------------------------------- This looks fine, except the href URL is empty. I'm not sure what our requirements here are now and where it should point, please clear this up before landing. ::: toolkit/mozapps/extensions/internal/GMPProvider.jsm @@ +59,5 @@ > { > id: "gmp-eme-adobe", > name: "eme-adobe_name", > description: "eme-adobe_description", > + fullDescription: "<xhtml:a href=\"\" target=\"_blank\">License information</xhtml:a>.", comment 112 says "we're going to point to the EULA on the server hosted by them". This just has an empty href.

Comment on attachment 8563889 [details] [diff] [review] Interdiff for patch 4 Lets r+ the actual patch instead...

Thanks, Georg! (In reply to Georg Fritzsche [:gfritzsche] from comment #124) > This looks fine, except the href URL is empty. > I'm not sure what our requirements here are now and where it should point, > please clear this up before landing. I'll be updating this with the proper URL in bug 1129721.

Comment on attachment 8564235 [details] [diff] [review] 6. Tests Review of attachment 8564235 [details] [diff] [review]: ----------------------------------------------------------------- Thanks, this looks good now. As discussed, please file a follow-up for discussion on the N-plugins test coverage for simpleCheckAndInstall() and the other paths. ::: toolkit/modules/tests/xpcshell/test_GMPInstallManager.js @@ +430,5 @@ > let data = "e~=0.5772156649"; > let zipFile = createNewZipFile(zipFileName, data); > let hashFunc = "sha256"; > let expectedDigest = yield GMPDownloader.computeHash(hashFunc, zipFile); > + do_print("expectedDigest: " + expectedDigest); Left-over debugging? @@ +438,2 @@ > } > + do_print("fileSize: " + fileSize); Left-over debugging? ::: toolkit/mozapps/extensions/test/browser/browser_gmpProvider.js @@ +99,5 @@ > + gPrefs.clearUserPref(gGetKey(GMPScope.KEY_PLUGIN_ENABLED, addon.id)); > + gPrefs.clearUserPref(gGetKey(GMPScope.KEY_PLUGIN_LAST_UPDATE, addon.id)); > + gPrefs.clearUserPref(gGetKey(GMPScope.KEY_PLUGIN_AUTOUPDATE, addon.id)); > + gPrefs.clearUserPref(gGetKey(GMPScope.KEY_PLUGIN_VERSION, addon.id)); > + gPrefs.clearUserPref(gGetKey(GMPScope.KEY_PLUGIN_HIDDEN, addon.id)); Functions don't get "g" prefixed.

This drops an observer which was never used. Carrying over r+.

Updated for current trunk. Carrying over r+.

Georg, a try run[1] of all the patches revealed two issues in browser_gmpProvider.js: 1. We were leaking two windows with URL about:addons 2. The testPreferencesButton test would fail on some platforms. The first issue was fairly straightforward to fix. Setting GMPScope.GMPInstallManager to MockGMPInstallManager in testPreferencesButton seemingly made us leak two windows. Resetting it to the actual/real GMPInstallManager at the end of the test fixed this. For issue number 2 I've had to close and reopen the window manager in the test. As discussed over IRC, the minimum for testPreferencesButton to pass is to close and reopen the window manager the first four times that the inner for loop is executed, i.e. the four times that we set the enabled pref to false (twice for OpenH264, twice for Adobe EME). Anything less than that will make us fail with either 1 or 2 failures. Unfortunately, I'm not sure why this is or why we're not failing on all platforms. With this additional patch, try is now green[2]. If you're fine with these additional changes, I'll fold them into patch 6 before landing. [1] https://treeherder.mozilla.org/#/jobs?repo=try&revision=df7ad6b35b45 [2] https://treeherder.mozilla.org/#/jobs?repo=try&revision=d3ee53a3d52c

Comment on attachment 8565296 [details] [diff] [review] Additional test changes Review of attachment 8565296 [details] [diff] [review]: ----------------------------------------------------------------- ::: toolkit/mozapps/extensions/test/browser/browser_gmpProvider.js @@ +91,5 @@ > > gManagerWindow = yield open_manager(); > gCategoryUtilities = new CategoryUtilities(gManagerWindow); > > + registerCleanupFunction(function() { Nit: Unnecessary change. @@ +306,5 @@ > JSON.stringify(preferences) + "\n"); > for (let addon of gMockAddons) { > + yield close_manager(gManagerWindow); > + gManagerWindow = yield open_manager(); > + gCategoryUtilities = new CategoryUtilities(gManagerWindow); This part looks wrong: * we lose a little bit of test-coverage and * we may have uncovered an underlying issue here However, we don't have time to investigate this issue fully right now and we should unblock this bug. Given that the worst-case here should be some stale AddonManger UI state and that we should have proper manual QA of this path anyway, we can move the investigation to a follow-up and land this one for now. @@ +330,5 @@ > > add_task(function* testUpdateButton() { > gPrefs.clearUserPref(GMPScope.KEY_PROVIDER_LASTCHECK); > > + let realInstallManager = GMPScope.GMPInstallManager; Nit: originalInstallManager

Thanks, Georg! (In reply to Georg Fritzsche [:gfritzsche] from comment #131) > Comment on attachment 8565296 [details] [diff] [review] > Additional test changes > > Review of attachment 8565296 [details] [diff] [review]: > ----------------------------------------------------------------- > > ::: toolkit/mozapps/extensions/test/browser/browser_gmpProvider.js > @@ +91,5 @@ > > > > gManagerWindow = yield open_manager(); > > gCategoryUtilities = new CategoryUtilities(gManagerWindow); > > > > + registerCleanupFunction(function() { > > Nit: Unnecessary change. We can't use |yield| in an arrow function, but you correctly pointed out on IRC that this should have been changed to a generator function in that case. > > @@ +306,5 @@ > > JSON.stringify(preferences) + "\n"); > > for (let addon of gMockAddons) { > > + yield close_manager(gManagerWindow); > > + gManagerWindow = yield open_manager(); > > + gCategoryUtilities = new CategoryUtilities(gManagerWindow); > > This part looks wrong: > * we lose a little bit of test-coverage and > * we may have uncovered an underlying issue here > > However, we don't have time to investigate this issue fully right now and we > should unblock this bug. > Given that the worst-case here should be some stale AddonManger UI state and > that we should have proper manual QA of this path anyway, we can move the > investigation to a follow-up and land this one for now. Agreed, I'll file followup bugs shortly.

Trees are currently closed. Will land when they reopen.

Now with Task.async() for the cleanup function. Carrying over r+.

https://hg.mozilla.org/integration/mozilla-inbound/rev/bd746b89c47c https://hg.mozilla.org/integration/mozilla-inbound/rev/800c27e0e316 https://hg.mozilla.org/integration/mozilla-inbound/rev/23191f01ed55 https://hg.mozilla.org/integration/mozilla-inbound/rev/7a87c365a1d1 https://hg.mozilla.org/integration/mozilla-inbound/rev/929f592663d1 https://hg.mozilla.org/integration/mozilla-inbound/rev/883b84c33c4b

(In reply to Georg Fritzsche [:gfritzsche] from comment #127) > Comment on attachment 8564235 [details] [diff] [review] > Thanks, this looks good now. > As discussed, please file a follow-up for discussion on the N-plugins test > coverage for simpleCheckAndInstall() and the other paths. Filed bug 1133843. (In reply to Georg Fritzsche [:gfritzsche] from comment #131) > Comment on attachment 8565296 [details] [diff] [review] > @@ +306,5 @@ > > JSON.stringify(preferences) + "\n"); > > for (let addon of gMockAddons) { > > + yield close_manager(gManagerWindow); > > + gManagerWindow = yield open_manager(); > > + gCategoryUtilities = new CategoryUtilities(gManagerWindow); > > This part looks wrong: > * we lose a little bit of test-coverage and > * we may have uncovered an underlying issue here > > However, we don't have time to investigate this issue fully right now and we > should unblock this bug. > Given that the worst-case here should be some stale AddonManger UI state and > that we should have proper manual QA of this path anyway, we can move the > investigation to a follow-up and land this one for now. Filed bug 1133782.

Kamil, are you the right person to ask for QA verification of this bug? If not, do you know who might be? I'll be gathering documentation on what to test in the meantime. Thanks!

https://hg.mozilla.org/mozilla-central/rev/bd746b89c47c https://hg.mozilla.org/mozilla-central/rev/800c27e0e316 https://hg.mozilla.org/mozilla-central/rev/23191f01ed55 https://hg.mozilla.org/mozilla-central/rev/7a87c365a1d1 https://hg.mozilla.org/mozilla-central/rev/929f592663d1 https://hg.mozilla.org/mozilla-central/rev/883b84c33c4b

Stephen, I believe Syd Polk is currently the QA contact for EME.

I will be working with any automation around this effort. I know nothing about it now but need to get up to speed. What do you need specifically for this bug?

Comment on attachment 8554636 [details] [diff] [review] 1. Rename OpenH264Provider to GMPProvider Approval Request Comment [Feature/regressing bug #]: Adobe EME [User impact if declined]: Adobe EME will not be downloadable in Firefox 37. [Describe test coverage new/current, TreeHerder]: Has been on m-c for a few days. Uplifting will give us greater test exposure. [Risks and why]: There is risk to the download of OpenH264, as this is a refactor of the download code for all GMPs. Uplifting will help us detect and fix any bugs faster. [String/UUID change made/needed]: none in this patch.

Comment on attachment 8563881 [details] [diff] [review] 2. Refactor GMPInstallManager Approval Request Comment [Feature/regressing bug #]: Adobe EME [User impact if declined]: Adobe EME will not be downloadable in Firefox 37. [Describe test coverage new/current, TreeHerder]: Has been on m-c for a few days. Uplifting will give us greater test exposure. [Risks and why]: There is risk to the download of OpenH264, as this is a refactor of the download code for all GMPs. Uplifting will help us detect and fix any bugs faster. [String/UUID change made/needed]: none in this patch.

Approval Request Comment [Feature/regressing bug #]: Adobe EME [User impact if declined]: Adobe EME will not be downloadable in Firefox 37. [Describe test coverage new/current, TreeHerder]: Has been on m-c for a few days. Uplifting will give us greater test exposure. [Risks and why]: There is risk to the download of OpenH264, as this is a refactor of the download code for all GMPs. Uplifting will help us detect and fix any bugs faster. [String/UUID change made/needed]: none in this patch.

Approval Request Comment [Feature/regressing bug #]: Adobe EME [User impact if declined]: Adobe EME will not be downloadable in Firefox 37. [Describe test coverage new/current, TreeHerder]: Has been on m-c for a few days. Uplifting will give us greater test exposure. [Risks and why]: There is risk to the download of OpenH264, as this is a refactor of the download code for all GMPs. Uplifting will help us detect and fix any bugs faster. [String/UUID change made/needed]: We added strings (approved by Adobe) for the entries in the addons manager. These were: Name: "Primetime Content Decryption Module provided by Adobe Systems, Incorporated" Description: "Play back protected web video."

Approval Request Comment [Feature/regressing bug #]: Adobe EME [User impact if declined]: Adobe EME will not be downloadable in Firefox 37. [Describe test coverage new/current, TreeHerder]: Has been on m-c for a few days. Uplifting will give us greater test exposure. [Risks and why]: There is risk to the download of OpenH264, as this is a refactor of the download code for all GMPs. Uplifting will help us detect and fix any bugs faster. [String/UUID change made/needed]: none in this patch.

Approval Request Comment [Feature/regressing bug #]: Adobe EME [User impact if declined]: Adobe EME will not be downloadable in Firefox 37. [Describe test coverage new/current, TreeHerder]: Has been on m-c for a few days. Uplifting will give us greater test exposure. [Risks and why]: There is risk to the download of OpenH264, as this is a refactor of the download code for all GMPs. Uplifting will help us detect and fix any bugs faster. [String/UUID change made/needed]: none in this patch.

(In reply to Syd Polk :sydpolk from comment #140) > I will be working with any automation around this effort. I know nothing > about it now but need to get up to speed. What do you need specifically for > this bug? The OpenH264 test plan may be a good start to add Adobe EME specific tests. I've listed all the links I could find at the bottom of this comment. The way to enable logging has changed slightly. To enable logging, set "media.gmp.log.level" to 0 and "media.gmp.log.dump" to true. Comment 131 also calls for some additional, manual QA of the addons manager UI. Specifically, enabling/disabling and installing/uninstalling via prefs should work as expected and the UI in the addons manager should update correctly. The prefs are: media.{0}.enabled media.{0}.version where {0} is the GMP id, such as "gmp-gmpopenh264" or "gmp-eme-adobe". https://wiki.mozilla.org/QA/OpenH264/Test_Plan https://wiki.mozilla.org/OpenH264_plugin/Test_Plan https://moztrap.mozilla.org/manage/cases/?

(In reply to Stephen Pohl [:spohl] from comment #146) > [Describe test coverage new/current, TreeHerder]: Has been on m-c for a few > days. Uplifting will give us greater test exposure. > [Risks and why]: There is risk to the download of OpenH264, as this is a > refactor of the download code for all GMPs. Uplifting will help us detect > and fix any bugs faster. Also, we would like to test Adobe's CDM on Beta 37. By uplifting this refactoring of the GMP downloader now, we can find our GMP downloader bugs that are independent of Adobe's CDM package and server configuration.

Comment on attachment 8566941 [details] [diff] [review] Patch 4 for aurora The strings need to be hard coded for 37 so as not to impact l10n. cpeterson and jst previously confirmed that we can test with English only. Aurora-

(In reply to Lawrence Mandel [:lmandel] (use needinfo) from comment #149) > Comment on attachment 8566941 [details] [diff] [review] > Patch 4 for aurora > > The strings need to be hard coded for 37 so as not to impact l10n. cpeterson > and jst previously confirmed that we can test with English only. Aurora- Meant to add, please hard code the strings for this patch and resubmit for approval.

Folded all the patches into one and removed all string changes. Approval Request Comment [Feature/regressing bug #]: Adobe EME [User impact if declined]: Adobe EME will not be downloadable in Firefox 37. [Describe test coverage new/current, TreeHerder]: Has been on m-c for a few days. Uplifting will give us greater test exposure. [Risks and why]: There is risk to the download of OpenH264, as this is a refactor of the download code for all GMPs. Uplifting will help us detect and fix any bugs faster. Also, we would like to test Adobe's CDM on Beta 37. By uplifting this refactoring of the GMP downloader now, we can find our GMP downloader bugs that are independent of Adobe's CDM package and server configuration. [String/UUID change made/needed]: none

Comment on attachment 8567345 [details] [diff] [review] Patch for aurora As previously discussed, we're going to take some EME related changes in 37. Thanks to Steven for hardcoding the strings and consolidating the patches. Aurora+ Stephen - How can we verify that this change doesn't affect the OpenH264 download?

We exercise the download and integration in the addons manager for all GMPs (including OpenH264) in the following tests, which are run by default on our branches: toolkit/mozapps/extensions/test/browser/browser_gmpProvider.js toolkit/mozapps/extensions/test/xpcshell/test_gmpProvider.js toolkit/modules/tests/xpcshell/test_GMPInstallManager.js This should cover the majority of our scenarios. I don't expect any fallout from these, as they've run on trunk for several days now. However, there may be edge-cases that we will only find out about through our users on aurora/beta.

(In reply to Stephen Pohl [:spohl] from comment #153) > I don't expect any fallout from these, as they've run on trunk for several > days now. I should clarify that I don't expect more fallout than what's captured in bug 1135242. We seem to be running into test failures when we merge to beta. I'm still investigating why this might be.

Syd, will you verify the landings here on 37 and 38? Or is someone else up for this?

This refresh also includes the patch from bug 1135242, which is tests-only (see bug 1135242 comment 10). I'm unable to set the approval flag to a+ for aurora, but this patch should be treated as such.

I'll verify it.