1039226 - Trigger explicit OpenH264 updates from OpenH264Provider

Status: VERIFIED FIXED

(Toolkit :: Add-ons Manager, defect)

Description

[Georg Fritzsche [:gfritzsche]]

As a follow-up to bug 1009909, we need to trigger an update of OpenH264 explicitly from the OpenH264Provider via the API from bug 1009909.

Comment 1

[Brian R. Bondy [:bbondy]]

Note that I'm doing a check for update at most daily on Firefox startup on delayed Firefox startup.

Comment 2

[Georg Fritzsche [:gfritzsche]]

Attachment: WIP

Comment 3

[Georg Fritzsche [:gfritzsche]]

This should be roughly what we want.
I currently hit issues with the test mocking on browser_openH264.js, line 215:
> TypeError: OpenH264Scope.OpenH264Wrapper is undefined

Comment 4

[Georg Fritzsche [:gfritzsche]]

Attachment: Triggering updates, test coverage

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

Comment 5

[Blair McBride [:Unfocused] (UNAVAILABLE)]

Comment on attachment 8459672 [details] [diff] [review]
Triggering updates, test coverage

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

::: toolkit/mozapps/extensions/internal/OpenH264Provider.jsm
@@ +16,5 @@
>  Cu.import("resource://gre/modules/Preferences.jsm");
>  Cu.import("resource://gre/modules/osfile.jsm");
>  Cu.import("resource://gre/modules/Log.jsm");
> +Cu.import("resource://gre/modules/Task.jsm");
> +Cu.import("resource://gre/modules/GMPInstallManager.jsm");

Should lazy-load this.

@@ +171,5 @@
>        aListener.onNoCompatibilityUpdateAvailable(this);
>      if ("onNoUpdateAvailable" in aListener)
>        aListener.onNoUpdateAvailable(this);
>      if ("onUpdateFinished" in aListener)
>        aListener.onUpdateFinished(this);

I had assumed this was temporary - but then implementing AddonInstall for this is overkill.

Can replace all those listener calls with just:
  AddonManagerPrivate.callNoUpdateListeners(this, aListener);

@@ +173,5 @@
>        aListener.onNoUpdateAvailable(this);
>      if ("onUpdateFinished" in aListener)
>        aListener.onUpdateFinished(this);
> +
> +    if (aReason !== AddonManager.UPDATE_WHEN_USER_REQUESTED ||

Should let through UPDATE_WHEN_PERIODIC_UPDATE too - some people leave their browsers open for a Very Long Time, so the automatic check in browser.js doesn't help much there (this was mentioned somewhere in bug 1009816).

Comment 6

[Blair McBride [:Unfocused] (UNAVAILABLE)]

(In reply to Blair McBride [:Unfocused] from comment #5)
> Should let through UPDATE_WHEN_PERIODIC_UPDATE too - some people leave their
> browsers open for a Very Long Time, so the automatic check in browser.js
> doesn't help much there (this was mentioned somewhere in bug 1009816).

Forgot to mention - the background update should only go ahead if the on-startup check hasn't happened in the past day.

And you *DO* need to check the state of applyBackgroundUpdates when doing background update checks. AddonManager.shouldAutoUpdate() is a useful helper function here. AddonManager.jsm normally does this for you, but only if you use onUpdateAvailable and implement AddonInstall.

Comment 7

[Georg Fritzsche [:gfritzsche]]

(In reply to Blair McBride [:Unfocused] from comment #6)
> (In reply to Blair McBride [:Unfocused] from comment #5)
> > Should let through UPDATE_WHEN_PERIODIC_UPDATE too - some people leave their
> > browsers open for a Very Long Time, so the automatic check in browser.js
> > doesn't help much there (this was mentioned somewhere in bug 1009816).
> 
> Forgot to mention - the background update should only go ahead if the
> on-startup check hasn't happened in the past day.
> 
> And you *DO* need to check the state of applyBackgroundUpdates when doing
> background update checks. AddonManager.shouldAutoUpdate() is a useful helper
> function here. AddonManager.jsm normally does this for you, but only if you
> use onUpdateAvailable and implement AddonInstall.

That is not the scope of this bug though, i'll file a follow-up.

Comment 8

[Georg Fritzsche [:gfritzsche]]

Attachment: Triggering updates, test coverage

Comment 9

[Georg Fritzsche [:gfritzsche]]

https://hg.mozilla.org/integration/fx-team/rev/609690a7e2a6

Comment 10

[Georg Fritzsche [:gfritzsche]]

I think the easiest testing steps here are setting media.gmp-gmpopenh264.lastUpdate to something <1 day ago on a new profile before the automatic update/install happens.
It's an int pref, seconds since unix epoch - e.g. just set it to the result of |Date.now()/1000 - 60|.

Then trigger updates manually from either:
* the context menu on openh264 in the list view of the Addons Manager plugin category
* the detail view of openh264 - you need to turn automatic updates off to get the update link

Comment 11

[Ed Morley [:emorley]]

Backed out for exceptions in browser_openH264.js:
https://tbpl.mozilla.org/php/getParsedLog.php?id=44438505&tree=Fx-Team

remote:   https://hg.mozilla.org/integration/fx-team/rev/162e125d88e4

Comment 12

[Maire Reavy [:mreavy]]

Hi Georg -- Do we still need to land this or did this get landed in another bug (or was it overtaken by events)?

Comment 13

[Georg Fritzsche [:gfritzsche]]

Thanks for the reminder Maire, this dropped off my radar over the recent busy schedule.

Fixed test and landed: https://hg.mozilla.org/integration/mozilla-inbound/rev/3c052ee8d76a

Comment 14

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/3c052ee8d76a

Comment 15

[Georg Fritzsche [:gfritzsche]]

Comment on attachment 8460326 [details] [diff] [review]
Triggering updates, test coverage

Approval Request Comment
[Feature/regressing bug #]: OpenH264 integration.
[User impact if declined]: Non-working manual triggering of update checks for the plugin.
[Describe test coverage new/current, TBPL]: Automated testing, manual spot-checking, up on m-c.
[Risks and why]: Low-risk, contained to the "find updates" actions in the addon manager UX.
[String/UUID change made/needed]: None.

Comment 16

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

https://hg.mozilla.org/releases/mozilla-aurora/rev/9dde3907962c

Comment 17

[Ada [:adalucinet]]

Verified as fixed with latest Aurora 33.0a2 and Nightly 34.0a1 builds on Windows 7 x64, Mac OS X 10.9.4 and Ubuntu 13.04 64bit (e.g.: https://pastebin.mozilla.org/5804582 - logs after openh264 is installed and Find Updates option is selected).