1046644 - Honor OpenH264 autoupdate pref for the initial GMP install check

Status: VERIFIED FIXED

(Firefox :: General, defect)

Description

[Georg Fritzsche [:gfritzsche]]

(Nils Ohlmeier [:drno] from bug 1044268, comment 1)
> 1) The request for a user pref which allows certain users or entities to opt
> for not even downloading the binary code of the plugin to their machines.

We do have an option to turn off auto updates in the detail view of the OpenH264 plugin entry.
It looks like we don't honor the backing pref (media.gmp-gmpopenh264.autoupdate) for the initial download check though.

Comment 1

[Georg Fritzsche [:gfritzsche]]

Brian, can you take this?

Comment 2

[Brian R. Bondy [:bbondy]]

Yep I can take this but I don't know of any pref that currently exists for this. You just want me to add a new one right? And it would be one pref per addon ID? (media.{full-id}.autoupdate)

Comment 3

[Georg Fritzsche [:gfritzsche]]

We do have the above-mentioned media.gmp-gmpopenh264.autoupdate pref - unset means "default", i.e. true, here.
The addon manager options for the OpenH264 plugin set this pref.

I assume we'll keep the pattern when we start supporting more GMP plugins, but it's hard to say for sure now.

Comment 4

[Brian R. Bondy [:bbondy]]

Attachment: Patch rev1.

Comment 5

[Georg Fritzsche [:gfritzsche]]

Comment on attachment 8465911 [details] [diff] [review]
Patch rev1.

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

Can you add a test that no update/install happens for simpleCheckAndInstall() with the autoupdate pref off?
r=me with that.

Comment 6

[Brian R. Bondy [:bbondy]]

Ya I didn't because I don't currently have a test at all for simpleCheckAndInstall. I'll do this before landing though. Thanks for the review!

Comment 7

[Brian R. Bondy [:bbondy]]

Attachment: Patch rev2

Carrying forward r+.

Changes for rev2:
- Added promise return for simpleCheckAndInstall
- Added 3 tests for simpleCheckAndInstall for the 3 cases which are not errors but that don't install addons.

Manual tests performed: 
- Ran xpcshell locally
- Verified addons off via addon manager leads to a log message that indicates that and no addon installed.
- Verified addons default via addon manager leads to a log message that indicates success and that addons manager reflects that too.

Comment 8

[Brian R. Bondy [:bbondy]]

Running through try now, will land once that passes.

Comment 9

[Brian R. Bondy [:bbondy]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/350c38f30020

Comment 10

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/350c38f30020

Comment 11

[Randell Jesup [:jesup] (needinfo me)]

Comment on attachment 8466455 [details] [diff] [review]
Patch rev2

Approval Request Comment
[Feature/regressing bug #]: OpenH264

[User impact if declined]: Users will be unable to stop the browser from downloading a non-free plugin.  This is an issue of principle for some of our users, and might be a bandwidth issue for a few on very limited/shared or expensive connections.

[Describe test coverage new/current, TBPL]: Tested by QA.  In central for a week or so.  Includes several tests.

[Risks and why]: I believe the risk is low, but I'm not the expert on the frontend code.  From what I can see it's straightforward.

[String/UUID change made/needed]: none

Comment 12

[Randell Jesup [:jesup] (needinfo me)]

https://hg.mozilla.org/releases/mozilla-aurora/rev/cf771b63d6d7

Comment 13

[Bogdan Maris, Desktop QA]

Checked that automatic updates options work as expected before openh264 plugin is installed and after the plugin is installed. Tested on Windows 7 64bit, Ubuntu 14.04 32bit and Mac OS X 10.9.4 using latest Nightly and Aurora. Logs can be seen bellow: 

Nightly:
Windows 7 64bit
Before install: https://pastebin.mozilla.org/6038066

After install: https://pastebin.mozilla.org/6038200

Ubuntu 14.04 32bit
Before install: https://pastebin.mozilla.org/6038327

After install: https://pastebin.mozilla.org/6038451

Mac OS X 10.9.4
Before install: https://pastebin.mozilla.org/6038084

After install: https://pastebin.mozilla.org/6038523

Aurora:
Windows 7 64bit
Before install: https://pastebin.mozilla.org/6048899

After install: https://pastebin.mozilla.org/6048995

Ubuntu 14.04 32bit
Before install: https://pastebin.mozilla.org/6049252

After install: https://pastebin.mozilla.org/6049805

Mac OS X 10.9.4
Before install: https://pastebin.mozilla.org/6048160

After install: https://pastebin.mozilla.org/6048473

Comment 14

[Richard Z.]

The whole update code is still broken. With Firefox 33 it will manipulate several pref values and mark them as "user set" in about:config.

Those are media.gmp-gmpopenh264.lastUpdate, media.gmp-gmpopenh264.version, media.gmp-manager.lastCheck

I think it is broken to abuse user preferences to store persistent variables.

Comment 15

[Georg Fritzsche [:gfritzsche]]

There is nothing broken with that. The preferences are used for storing state all over the place, "user set" just means "non-default value".