Closed Bug 1046644 Opened 5 years ago Closed 5 years ago

Honor OpenH264 autoupdate pref for the initial GMP install check

Categories

(Firefox :: General, defect)

defect
Not set
Points:
3

Tracking

()

VERIFIED FIXED
Firefox 34
Iteration:
34.2
Tracking Status
firefox33 --- verified
firefox34 --- verified

People

(Reporter: gfritzsche, Assigned: bbondy)

References

(Blocks 1 open bug)

Details

Attachments

(1 file, 1 obsolete file)

(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.
Flags: firefox-backlog+
Brian, can you take this?
Flags: needinfo?(netzen)
Whiteboard: [openh264-uplift]
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)
Flags: needinfo?(netzen)
Assignee: nobody → netzen
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.
Attached patch Patch rev1. (obsolete) — Splinter Review
Attachment #8465911 - Flags: review?(georg.fritzsche)
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.
Attachment #8465911 - Flags: review?(georg.fritzsche) → review+
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!
Attached patch Patch rev2Splinter Review
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.
Attachment #8466455 - Flags: review+
Running through try now, will land once that passes.
Flags: in-testsuite+
Attachment #8465911 - Attachment is obsolete: true
https://hg.mozilla.org/mozilla-central/rev/350c38f30020
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Iteration: --- → 34.2
QA Whiteboard: [qa?]
QA Whiteboard: [qa?] → [qa+]
QA Contact: drno
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
Attachment #8466455 - Flags: approval-mozilla-aurora?
Attachment #8466455 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
QA Contact: drno → florin.mezei
QA Contact: florin.mezei → alexandra.lucinet
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
Status: RESOLVED → VERIFIED
QA Whiteboard: [qa+] → [qa!]
Blocks: 1057990
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.
There is nothing broken with that. The preferences are used for storing state all over the place, "user set" just means "non-default value".
You need to log in before you can comment on or make changes to this bug.