1564731 - Include distribution id in the api_url for html discopane

Status: VERIFIED FIXED

(Toolkit :: Add-ons Manager, enhancement, P1)

Description

[Hector Zhao [:hectorz]]

Beijing office has been altering the discopane url to show recommended extensions tailored for Fx China repack1. Since we need to cover existing profiles, this is done with an extension. And recently, it took me some time to figure out necessary changes after bug 1546248 introduced the new html discopane.

Fx China repack can be uniquely identified by its distribution id "MozillaOnline". I'd like to propose that the distribution id be included in "extensions.getAddons.discovery.api_url" like several other update urls2 and then treat "distribution=MozillaOnline" just like "edition=china" in the addons-server.

Thanks!

Comment 1

[Stuart Colville [:muffinresearch]]

:jimm Hi - would you be able to get someone from your team to figure out the options for handling this in the client?

As mentioned on slack last week, if it's possible to avoid aliasing API params I'd prefer it, but if doing that facilitates getting a solution then let's discuss it. However first it would be good to figure out what's possible and what the timescale looks like.

Comment 2

[Jim Mathies [:jimm]]

(In reply to Stuart Colville [:scolville] [:muffinresearch] from comment #1)

:jimm Hi - would you be able to get someone from your team to figure out the
options for handling this in the client?

As mentioned on slack last week, if it's possible to avoid aliasing API
params I'd prefer it, but if doing that facilitates getting a solution then
let's discuss it. However first it would be good to figure out what's
possible and what the timescale looks like.

We'll chat about this at our standup tomorrow.

Comment 3

[Rob Wu [:robwu]]

Is there any reason why you cannot just add the distribution in the extensions.getAddons.discovery.api_url preference?

Comment 4

[Hector Zhao [:hectorz]]

(In reply to Rob Wu [:robwu] from comment #3)

Is there any reason why you cannot just add the distribution in the extensions.getAddons.discovery.api_url preference?

Hi Rob,

I'm not sure I understand your suggestion.

We've been appending edition=china to extensions.webservice.discoverURL with our extension for a while, and with our most recent update, extensions.getAddons.discovery.api_url is covered as well.

The problem with this approach is, if our extension is disabled for any reason, the pref will either:

• be reset if set on default branch; or
• become outdated if set on user branch, e.g. it stays .../v4/...&edition=china even if the default is updated to .../v5/....

That's why I'd like to find a solution that works regardless of our extension. The distribution id is defined as MozillaOnline in every Fx China repacks ever shipped, and it's readily available in the urlFormatter. Also, using its value like https://services.addons.mozilla.org/api/v4/discovery/?edition=%DISTRIBUTION%&lang=%LOCALE% works for me if there're concerns about introducing another param.

Comment 5

[Rob Wu [:robwu]]

(In reply to Hector Zhao [:hectorz] from comment #4)

(In reply to Rob Wu [:robwu] from comment #3)

Is there any reason why you cannot just add the distribution in the extensions.getAddons.discovery.api_url preference?

Hi Rob,

I'm not sure I understand your suggestion.

We've been appending edition=china to extensions.webservice.discoverURL with our extension for a while, and with our most recent update, extensions.getAddons.discovery.api_url is covered as well.

This was my suggestion, and since you used it before, and are using it now, I think that there is no need to include yet another option/pref.

The problem with this approach is, if our extension is disabled for any reason, the pref will either:

• be reset if set on default branch; or
• become outdated if set on user branch, e.g. it stays .../v4/...&edition=china even if the default is updated to .../v5/....

This was apparently not a problem before. Why is it a problem now?

That's why I'd like to find a solution that works regardless of our extension. The distribution id is defined as MozillaOnline in every Fx China repacks ever shipped, and it's readily available in the urlFormatter. Also, using its value like https://services.addons.mozilla.org/api/v4/discovery/?edition=%DISTRIBUTION%&lang=%LOCALE% works for me if there're concerns about introducing another param.

This proposed change is small and looks reasonable. I don't object to it, though such a parameter would only be useful if AMO supports the flag.

Comment 6

[Hector Zhao [:hectorz]]

(In reply to Rob Wu [:robwu] from comment #5)

This was my suggestion, and since you used it before, and are using it now, I think that there is no need to include yet another option/pref.

Ah, sorry I didn't explain the current situation more clearly in the original comment.

This was apparently not a problem before. Why is it a problem now?

We were surprised by the introduction of html about:addons in late 68 beta cycle, and it brings the once hypothetical problems back to my attention. I think my proposal of this upstream solution should work better than keep patching things up on my side.

That's why I'd like to find a solution that works regardless of our extension. The distribution id is defined as MozillaOnline in every Fx China repacks ever shipped, and it's readily available in the urlFormatter. Also, using its value like https://services.addons.mozilla.org/api/v4/discovery/?edition=%DISTRIBUTION%&lang=%LOCALE% works for me if there're concerns about introducing another param.

This proposed change is small and looks reasonable. I don't object to it, though such a parameter would only be useful if AMO supports the flag.

I surely hope supporting this on AMO would similarly be a small enough change.

Stuart, would checking edition=MozillaOnline instead of distribution=MozillaOnline further reduce the effort?

Comment 7

[Stuart Colville [:muffinresearch]]

(In reply to Hector Zhao [:hectorz] from comment #6)

(In reply to Rob Wu [:robwu] from comment #5)

This was my suggestion, and since you used it before, and are using it now, I think that there is no need to include yet another option/pref.

Ah, sorry I didn't explain the current situation more clearly in the original comment.

This was apparently not a problem before. Why is it a problem now?

We were surprised by the introduction of html about:addons in late 68 beta cycle, and it brings the once hypothetical problems back to my attention. I think my proposal of this upstream solution should work better than keep patching things up on my side.

That's why I'd like to find a solution that works regardless of our extension. The distribution id is defined as MozillaOnline in every Fx China repacks ever shipped, and it's readily available in the urlFormatter. Also, using its value like https://services.addons.mozilla.org/api/v4/discovery/?edition=%DISTRIBUTION%&lang=%LOCALE% works for me if there're concerns about introducing another param.

This proposed change is small and looks reasonable. I don't object to it, though such a parameter would only be useful if AMO supports the flag.

I surely hope supporting this on AMO would similarly be a small enough change.

Stuart, would checking edition=MozillaOnline instead of distribution=MozillaOnline further reduce the effort?

Certainly, we can accommodate serving the same response for edition=china or edition=MozillaOnline assuming we're ok making the changes on the Firefox side to handle this too. We'd just need to make sure the AMO patch lands ahead of the Firefox changes.

If that goes ahead just needs-info me again here and I'll file a bug to update the API.

Comment 8

[Shane Caraveo (:mixedpuppy)]

It's been a long time, but I seem to recall we had an ability to set prefs in a distribution.ini file, but from what i see it may not be possible to override the defaults here.

• does AMO allow for any distribution id? What happens if the distro id is for foobar?

Mike will probably have some better insight on this.

Comment 9

[Mike Kaply [:mkaply]]

does AMO allow for any distribution id? What happens if the distro id is for foobar?

This was definitely a unique MozillaOnline problem. I don't forsee using for other distros.

I'm confused though. I thought we were doing this via distribution.ini

extensions.webservice.discoverURL="https://discovery.addons.mozilla.org/%LOCALE%/firefox/discovery/pane/%VERSION%/%OS%/%COMPATIBILITY_MODE%?edition=china"

Why can't we just do this by setting a new pref in distribution.ini?

Comment 10

[Rob Wu [:robwu]]

ni? see comment 9

Comment 11

[Hector Zhao [:hectorz]]

(In reply to Mike Kaply [:mkaply] from comment #9)

does AMO allow for any distribution id? What happens if the distro id is for foobar?

Any unknown value should simply be ignored, exactly how it works now: 1 2

This was definitely a unique MozillaOnline problem. I don't forsee using for other distros.

Yes, it's MozillaOnline only, and I'm not suggesting we should do anything for other distros.

It's just I think it's better to make minimal server change to include "MozillaOnline" in the existing special treatment of "china" and adding edition=%DISTRIBUTION% in Fx than adding an special treatment like

if (distributionId === "MozillaOnline") {
  url.searchParams.append("edition", "china");
}

in Fx to avoid making any change to AMO.

I'm confused though. I thought we were doing this via distribution.ini

extensions.webservice.discoverURL="https://discovery.addons.mozilla.org/%LOCALE%/firefox/discovery/pane/%VERSION%/%OS%/%COMPATIBILITY_MODE%?edition=china"

Yes, we were. But I think it doesn't mean we cannot make any improvement.

Why can't we just do this by setting a new pref in distribution.ini?

A few reasons:

1. Setting the pref in distribution.ini only works for new profiles, which means I have to cover existing installations with extension anyway, on the contrary, my proposal here should work for everyone who keep their Fx updated;
2. Setting the pref in distribution.ini also has the same problem as setting the pref on user branch with extension, as I've described in comment 4: "it stays .../v4/...&edition=china even if the default is updated to .../v5/...".
3. Should the above mentioned situation happen, existence of the outdated value in distribution.ini would make it harder for any extension override, since the pref value on default branch would be overridden by distribution.ini soon after the Fx startup.

More generally, this is probably about me not having to endlessly keeping all these overrides in our extension (which needs to be compatible with the lastest ESR, which at this moment is Fx 60+) while hopefully only incurring marginal maintaining effort on global teams (see comment 5 and comment 7).

Comment 12

[Hector Zhao [:hectorz]]

(In reply to Hector Zhao [:hectorz] from comment #11)

Why can't we just do this by setting a new pref in distribution.ini?

A few reasons:

1. Setting the pref in distribution.ini only works for new profiles, which means I have to cover existing installations with extension anyway, on the contrary, my proposal here should work for everyone who keep their Fx updated;
2. Setting the pref in distribution.ini also has the same problem as setting the pref on user branch with extension, as I've described in comment 4: "it stays .../v4/...&edition=china even if the default is updated to .../v5/...".
3. Should the above mentioned situation happen, existence of the outdated value in distribution.ini would make it harder for any extension override, since the pref value on default branch would be overridden by distribution.ini soon after the Fx startup.

Maybe I should mention this explicitly since not everybody is familar with how distribution works: the file distribution.ini is included in Fx installers and won't be updated with regular Fx update.

Comment 13

[Rob Wu [:robwu]]

I filed a bug for the AMO side of the change, at: https://github.com/mozilla/addons-server/issues/11937

Please confirm that the parameter is spelled and capitalized correctly, i.e. that Services.urlFormatter.formatURL("%DISTRIBUTION%") does indeed return "MozillaOnline".

Comment 14

[Hector Zhao [:hectorz]]

(In reply to Rob Wu [:robwu] from comment #13)

I filed a bug for the AMO side of the change, at: https://github.com/mozilla/addons-server/issues/11937

Thank you!

Please confirm that the parameter is spelled and capitalized correctly, i.e. that Services.urlFormatter.formatURL("%DISTRIBUTION%") does indeed return "MozillaOnline".

Yes, I confirm the value of %DISTRIBUTION% in China repack is MozillaOnline

Comment 15

[Shane Caraveo (:mixedpuppy)]

mkaply: our inability to update distribution data is really a PITA.

Comment 16

[Mike Kaply [:mkaply]]

mkaply: our inability to update distribution data is really a PITA.

I didn't architect it :).

Comment 17

[Hector Zhao [:hectorz]]

Attachment: Bug 1564731 - Pass distribution id as edition in the discopane api_url. r?robwu

Comment 18

[Hector Zhao [:hectorz]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=a660a6d487c0f63d13e74e5655d2902654075f6a

Comment 19

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/2ad2bbb5bfd2
Pass distribution id as edition in the discopane api_url. r=robwu,rpl

Comment 20

[Stefan Hindli [:stefan_hindli]]

https://hg.mozilla.org/mozilla-central/rev/2ad2bbb5bfd2

Comment 21

[Yanfang Liu]

Verified as fixed on the latest Firefox Nightly 70.0a1 (2019-08-08).