1386076 - Let WebExtensions to look up the application update channel

Status: RESOLVED WORKSFORME

(WebExtensions :: General, defect, P5)

Description

[Carlos [nohamelin]]

I would like to know, from a WebExtension addon, if the Firefox build where it is run is from: Release, Beta, Nightly, ESR, etc. Currently, it's exposed to legacy add-ons via the UpdateUtils module (resource://gre/modules/UpdateUtils.jsm).

My use case to request it is to build proper links to web resources related to Firefox, as these are can be dependent of the build. A specific example is link to  the sub-folder in archive.mozilla.org containing language packs compatible with the application, where e.g. for release, it is something as 

https://archive.mozilla.org/pub/firefox/releases/54.0/win32/xpi/

while for Developer Edition builds is:

https://archive.mozilla.org/pub/devedition/releases/55.0b1/win32/xpi/
--------------------------------^^^^^^^^^^


(It seems that app version number can't be read by WE addons neither, but that would be other bug request).

Comment 1

[Andrew Swan [:aswan]]

This seems like a reasonable enough thing to add to getBrowserInfo()
https://developer.mozilla.org/en-US/Add-ons/WebExtensions/API/runtime/getBrowserInfo

Comment 2

[Tomislav Jovanovic :zombie]

Agree with Andrew, patches welcome.

Comment 3

[Carlos [nohamelin]]

Ok, I can give a try with a patch.

Comment 4

[Carlos [nohamelin]]

Attachment: update-channel.patch

No considering testing, is it all I need to touch? Some things:
* I don't know if I should go with the updateUtils module to fetch the channel, or directly with AppConstants.MOZ_UPDATE_CHANNEL, as I did here. The former looks as a safer choice?
* I don't think the parnertship bits of the channel need be exposed too, as the UpdateUtils js module can do.
* Is there an recommended order for the parameters of getBrowserinfo?

And how the testing should be done here? the tests for getBrowserinfo use a mockAppInfo object, but it doesn't look suitable for the channel...

Comment 6

[Luca Greco [:rpl] [:luca] [:lgreco]]

(In reply to Carlos [nohamelin] from comment #4)
> Created attachment 8895015 [details] [diff] [review]
> update-channel.patch
> 
> No considering testing, is it all I need to touch? Some things:

Hi Carlos, it looks that you are definitely on the right track,
thanks a lot for working on this! 

> * I don't know if I should go with the updateUtils module to fetch the
> channel, or directly with AppConstants.MOZ_UPDATE_CHANNEL, as I did here.
> The former looks as a safer choice?

I took a brief look at what would be different by using "AppConstants.MOZ_UPDATE_CHANNEL vs. the UpdateUtils.jsm":

- the AppConstants.MOZ_UPDATE_CHANNEL is assigned a value during the build and it is not going to change,
  on the contrary it looks that the UpdateUtils.jsm can be customized using a preference 
  (http://searchfox.org/mozilla-central/rev/e5b13e6224dbe3182050cf442608c4cb6a8c5c55/toolkit/modules/UpdateUtils.jsm#34-35), 
  my guess is that it is customizable to "allow the user to optionally choose to change the update channel" 
  and/or to "make easier to test code that is affected by the updateChannel value"    

(I'm also going needinfo :mossop and :aswan about this, to double-check our findings)

> * I don't think the parnertship bits of the channel need be exposed too, as
> the UpdateUtils js module can do.

I tend to agree (maybe the partnership bits could be part of another property added in a follow up, e.g. if the partnership bits can be useful info for any partnership bundled extension).

> * Is there an recommended order for the parameters of getBrowserinfo?

Do you mean the properties of the value resolved by the API method, am I right? 
I would say "alphabetically ordered" (even if they are not currently alphabetically ordered). 

> And how the testing should be done here? the tests for getBrowserinfo use a
> mockAppInfo object, but it doesn't look suitable for the channel...

Looking at the current test test_ext_runtime_getBrowserInfo.js (http://searchfox.org/mozilla-central/rev/e5b13e6224dbe3182050cf442608c4cb6a8c5c55/toolkit/components/extensions/test/xpcshell/test_ext_runtime_getBrowserInfo.js):

If we opt for the AppConstants.MOZ_UPDATE_CHANNEL, in the test case we should use the AppConstants.MOZ_UPDATE_CHANNEL value to assert the updateChannel value returned by the runtime.getBrowserInfo API method, because using a static value will turn the test into a "permanent failure" as soon as it reaches the merge day (e.g. MOZ_UPDATE_CHANNEL will be initially "nightly" when the test lands on m-c, but it will become "beta" as soon as we reach the beta merge day, and it will change again once it reaches release).  

Otherwise, if we are going to use UpdateUtils.updateChannel as the source of this property value, it seems that we can change it in the test by customizing the preference (by setting it when the test starts and resetting it once it has completed, e.g. like we do in other tests like this one: http://searchfox.org/mozilla-central/rev/e5b13e6224dbe3182050cf442608c4cb6a8c5c55/toolkit/components/extensions/test/xpcshell/test_ext_background_private_browsing.js#34-37).

----

Dave (or Andrew), which is the best source to use to get the value that we should return as the updateChannel property in the runtime.getBrowserInfo WE API method result? AppConstants.MOZ_UPDATE_CHANNEL or the UpdateUtils.updateChannel?
(I tend to think that we should use AppConstants.MOZ_UPDATE_CHANNEL because it is statically assigned during the build and it cannot be changed from the preferences).

Also, what do you think about the partnership bits? should they be included in the information returned by the `runtime.getBrowserInfo` API method?

Comment 7

[Dave Townsend [:mossop]]

Deferring to Matt here

Comment 8

[Carlos [nohamelin]]

(In reply to Luca Greco [:rpl] from comment #6)
>   on the contrary it looks that the UpdateUtils.jsm can be customized using
> a preference 

Note that it use the default branch, so user values are ignored by the module.

Comment 9

[Molly Howell (she/her) (no longer active)]

It sounds like what we need here is information which is guaranteed to reflect the application that's currently running, and the way to get that is MOZ_UPDATE_CHANNEL. The app.update.channel pref can theoretically be changed (although only by editing the defaults, not from about:config) so that it's different from the running browser. And UpdateUtils itself should probably be avoided by consumers not directly related to update. So I would agree that using AppConstants.MOZ_UPDATE_CHANNEL is slightly preferable here.

I also agree about the partnership stuff; it wouldn't be hard to add down the line and I don't see the immediate need.

Comment 10

[Mike Hommey [:glandium]]

MOZ_UPDATE_CHANNEL or app.update.channel are actually the wrong thing to check for. They are only relevant for the update service. Local builds don't have those set to a useful value, nor do most Linux distro builds.

Comment 11

[Carlos [nohamelin]]

(In reply to Luca Greco [:rpl] (PTO until 08 Oct) from comment #6)
> If we opt for the AppConstants.MOZ_UPDATE_CHANNEL, in the test case we
> should use the AppConstants.MOZ_UPDATE_CHANNEL value to assert the
> updateChannel value returned by the runtime.getBrowserInfo API method,
> because using a static value will turn the test into a "permanent failure"
> as soon as it reaches the merge day (e.g. MOZ_UPDATE_CHANNEL will be
> initially "nightly" when the test lands on m-c, but it will become "beta" as
> soon as we reach the beta merge day, and it will change again once it
> reaches release).  

What is the recommended place to do the import of the AppConstants module for it? I saw others test files using directly AppConstants but I don't know from where it is got: I don't understand yet how the test files are structured.


(In reply to Mike Hommey [:glandium] from comment #10)
> MOZ_UPDATE_CHANNEL or app.update.channel are actually the wrong thing to
> check for. They are only relevant for the update service. Local builds don't
> have those set to a useful value, nor do most Linux distro builds.
Yeah, MOZ_UPDATE_CHANNEL have the value "default" for both. I don't know from where we could get more specific info in these cases.

Comment 12

[Mike Hommey [:glandium]]

The version number as displayed in the about firefox dialog is the reliable way to know the "channel".

Comment 13

[Mike Hommey [:glandium]]

And that version is MOZ_APP_VERSION_DISPLAY, and is available in AppConstants too.

Comment 14

[Carlos [nohamelin]]

(In reply to Mike Hommey [:glandium] from comment #12)
> The version number as displayed in the about firefox dialog is the reliable
> way to know the "channel".

I still would not help us to know locally, for example, if a build given by a Linux distro was built from release or esr: both would have, for example, "59.0" and "default" for MOZ_APP_VERSION_DISPLAY and MOZ_UPDATE_CHANNEL.

Comment 15

[Mike Hommey [:glandium]]

Is there a reason not to add "esr" in MOZ_APP_VERSION_DISPLAY?

Comment 16

[Jared Hirsch [:jhirsch] (he/him) (Needinfo please)]

> Is there a reason not to add "esr" in MOZ_APP_VERSION_DISPLAY?

It would be useful for Firefox Screenshots to be able to locally detect if a given Firefox is ESR.

Do you know if there's a bug to track adding the 'esr' string to MOZ_APP_VERSION_DISPLAY?

Comment 17

[Mike Hommey [:glandium]]

I don't think there is. If there was, I'd expect it would on Sylvestre's radar.

Comment 18

[Sylvestre Ledru [:Sylvestre]]

Good idea, I created bug 1432737 for this.

Comment 19

[Firefox Bug Husbandry Bot]

Bulk move of bugs per https://bugzilla.mozilla.org/show_bug.cgi?id=1483958

Comment 20

[Caitlin Neiman [:caitmuenster]]

Hey Carlos! Did bug 1432737 give you what you needed? Just want to see if this is still blocked on missing information.

Comment 21

[Carlos [nohamelin]]

(In reply to Caitlin Neiman [:caitmuenster] from comment #20)
> Hey Carlos! Did bug 1432737 give you what you needed? Just want to see if
> this is still blocked on missing information.

Yeah, so it seems.

Comment 22

[Carlos [nohamelin]]

Ups, wrong needinfo removal... the other doesn't seem anymore necessary anyway.

Comment 23

[Caitlin Neiman [:caitmuenster]]

Ok, great! I'm going to go ahead and close this bug out. :) If it turns out that something is indeed missing, please do leave a comment and we'll re-open this.