602367 - Add src parameter to AMO API pings

Status: VERIFIED FIXED

(Toolkit :: Add-ons Manager, defect)

Description

[Justin Scott [:fligtar]]

Please add a src=firefox parameter to all API requests that Firefox makes.

Note: this is separate from the other "firefox" in the URL, as a non-firefox source may request firefox add-ons and would still have firefox as the app part of the URL.

A sample new URL would be:
https://services.addons.mozilla.org/en-US/firefox/api/1.5/search/guid:inspector%40mozilla.org?src=firefox

Comment 1

[Dave Townsend [:mossop]]

So we can correctly track which accesses of the API are from Firefox we should do this for Firefox 4, we could also optionally backport it to older branches, it is I think just a pref change.

Comment 2

[:Margaret Leibovic]

Attachment: patch

I also noticed an AMO API call for the about:home snippets (http://mxr.mozilla.org/mozilla-central/source/browser/base/content/aboutHome.js#64). Should I change that as well?

Comment 3

[Dave Townsend [:mossop]]

Comment on attachment 486216 [details] [diff] [review]
patch

>diff --git a/browser/app/profile/firefox.js b/browser/app/profile/firefox.js
>--- a/browser/app/profile/firefox.js
>+++ b/browser/app/profile/firefox.js
>@@ -54,34 +54,34 @@ pref("browser.chromeURL","chrome://brows
> pref("browser.hiddenWindowChromeURL", "chrome://browser/content/hiddenWindow.xul");
> 
> // Enables some extra Extension System Logging (can reduce performance)
> pref("extensions.logging.enabled", false);
> 
> // Preferences for AMO integration
> pref("extensions.getAddons.cache.enabled", true);
> pref("extensions.getAddons.maxResults", 15);
>-pref("extensions.getAddons.get.url", "https://services.addons.mozilla.org/%LOCALE%/%APP%/api/%API_VERSION%/search/guid:%IDS%");
>-pref("extensions.getAddons.search.browseURL", "https://addons.mozilla.org/%LOCALE%/%APP%/search?q=%TERMS%");
>-pref("extensions.getAddons.search.url", "https://services.addons.mozilla.org/%LOCALE%/%APP%/api/%API_VERSION%/search/%TERMS%/all/%MAX_RESULTS%/%OS%/%VERSION%");
>-pref("extensions.webservice.discoverURL", "https://services.addons.mozilla.org/%LOCALE%/%APP%/discovery/%VERSION%/%OS%");
>+pref("extensions.getAddons.get.url", "https://services.addons.mozilla.org/%LOCALE%/%APP%/api/%API_VERSION%/search/guid:%IDS%?src=firefox");
>+pref("extensions.getAddons.search.browseURL", "https://addons.mozilla.org/%LOCALE%/%APP%/search?q=%TERMS%&src=firefox");
>+pref("extensions.getAddons.search.url", "https://services.addons.mozilla.org/%LOCALE%/%APP%/api/%API_VERSION%/search/%TERMS%/all/%MAX_RESULTS%/%OS%/%VERSION%?src=firefox");
>+pref("extensions.webservice.discoverURL", "https://services.addons.mozilla.org/%LOCALE%/%APP%/discovery/%VERSION%/%OS%?src=firefox");

We really only care about the API calls, which is extensions.getAddons.get.url and extensions.getAddons.search.url. Leave the rest of the URLs alone, otherwise this is fine.

Comment 4

[:Margaret Leibovic]

Attachment: patch v2

Updated to only change API urls.

Comment 5

[:Margaret Leibovic]

http://hg.mozilla.org/mozilla-central/rev/5ade2f730922

Comment 6

[Robert Kaiser]

what should this additional parameter should be for Thunderbird or SeaMonkey? is it just ?src=thunderbird and ?src=seamonkey there?

Comment 7

[Dave Townsend [:mossop]]

(In reply to comment #6)
> what should this additional parameter should be for Thunderbird or SeaMonkey?
> is it just ?src=thunderbird and ?src=seamonkey there?

I would guess so, but up to the AMO team to say what they are expecting

Comment 8

[Justin Scott [:fligtar]]

(In reply to comment #6)
> what should this additional parameter should be for Thunderbird or SeaMonkey?
> is it just ?src=thunderbird and ?src=seamonkey there?

Sure.

Comment 9

[Henrik Skupin [:whimboo][⌚️UTC+1]]

(In reply to comment #1)
> So we can correctly track which accesses of the API are from Firefox we should
> do this for Firefox 4, we could also optionally backport it to older branches,
> it is I think just a pref change.

Any plans for backporting the fix?

(In reply to comment #3)
> We really only care about the API calls, which is extensions.getAddons.get.url
> and extensions.getAddons.search.url. Leave the rest of the URLs alone,
> otherwise this is fine.

Where do we exactly use those URLs in the ui?

Comment 10

[Dave Townsend [:mossop]]

(In reply to comment #9)
> (In reply to comment #1)
> > So we can correctly track which accesses of the API are from Firefox we should
> > do this for Firefox 4, we could also optionally backport it to older branches,
> > it is I think just a pref change.
> 
> Any plans for backporting the fix?

I think we should be able to do this, Margaret can you prepare a patch for branch? The prefs to change will differ slightly as there is the recommended query there.

> (In reply to comment #3)
> > We really only care about the API calls, which is extensions.getAddons.get.url
> > and extensions.getAddons.search.url. Leave the rest of the URLs alone,
> > otherwise this is fine.
> 
> Where do we exactly use those URLs in the ui?

One is the url used for pulling metadata for add-ons and the other is for pulling search results.

Comment 11

[Justin Scott [:fligtar]]

We probably don't need to backport this -- I mainly wanted it for the guid search / metadata update which is new in Firefox 4.

Comment 12

[:Margaret Leibovic]

(In reply to comment #11)
> We probably don't need to backport this -- I mainly wanted it for the guid
> search / metadata update which is new in Firefox 4.

So does that mean I don't need to prepare a patch for branch?

Comment 13

[Dave Townsend [:mossop]]

(In reply to comment #12)
> (In reply to comment #11)
> > We probably don't need to backport this -- I mainly wanted it for the guid
> > search / metadata update which is new in Firefox 4.
> 
> So does that mean I don't need to prepare a patch for branch?

If fligtar doesn't care then I don't care so leave it.

Comment 14

[Henrik Skupin [:whimboo][⌚️UTC+1]]

(In reply to comment #3)
> >+pref("extensions.webservice.discoverURL", "https://services.addons.mozilla.org/%LOCALE%/%APP%/discovery/%VERSION%/%OS%?src=firefox");

Why aren't we using '?src=%APP%' here?

Also what about the backports you wanted to have Dave? Shall we set the blocking 1.9.2 and 1.9.1 flags?

Comment 15

[Dave Townsend [:mossop]]

(In reply to comment #14)
> (In reply to comment #3)
> > >+pref("extensions.webservice.discoverURL", "https://services.addons.mozilla.org/%LOCALE%/%APP%/discovery/%VERSION%/%OS%?src=firefox");
> 
> Why aren't we using '?src=%APP%' here?

It's in the Firefox prefs so I don't think there is much to be gained from changing that now. 

> Also what about the backports you wanted to have Dave? Shall we set the
> blocking 1.9.2 and 1.9.1 flags?

Branch drivers, would a patch for this be taken?

Comment 16

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Ok, so marking as verified fixed with Mozilla/5.0 (Macintosh; Intel Mac OS X 10.6; rv:2.0b12pre) Gecko/20110209 Firefox/4.0b12pre ID:20110209030359

A search for 'mozmill' sends the following request:
https://services.addons.mozilla.org/en-US/firefox/api/1.5/search/mozmill/all/30/Darwin/4.0b12pre?src=firefox

Do we have an automated test we could use to check for the application name?

Comment 17

[Dave Townsend [:mossop]]

Not t the moment, I don't think it is worth much effort right now either as it is hardcoded in the preference.

Comment 18

[Daniel Veditz [:dveditz]]

(In reply to comment #15)
> Branch drivers, would a patch for this be taken?

Yes, but it's not blocking.

Comment 19

[Dave Townsend [:mossop]]

Attachment: branch patch

This patch is applicable to the branches

Comment 20

[Dave Townsend [:mossop]]

Comment on attachment 517459 [details] [diff] [review]
branch patch

Looking for approval to land this on the branches. It's a straightforward pref change so no risk.

Comment 21

[Daniel Veditz [:dveditz]]

Comment on attachment 517459 [details] [diff] [review]
branch patch

Approved for 1.9.2.16 and 1.9.1.18, a=dveditz

Comment 22

[Dave Townsend [:mossop]]

Landed:

http://hg.mozilla.org/releases/mozilla-1.9.1/rev/9c8182d5a5ec
http://hg.mozilla.org/releases/mozilla-1.9.2/rev/12b279eed95f

Comment 23

[Dave Townsend [:mossop]]

We can see it in the prefs, I don't think it's worth a test for this