Last Comment Bug 602367 - Add src parameter to AMO API pings
: Add src parameter to AMO API pings
Status: VERIFIED FIXED
:
Product: Toolkit
Classification: Components
Component: Add-ons Manager (show other bugs)
: Trunk
: All All
: -- normal (vote)
: mozilla2.0b8
Assigned To: :Margaret Leibovic
:
: Andy McKay [:andym]
Mentors:
Depends on:
Blocks: 610065 610066
  Show dependency treegraph
 
Reported: 2010-10-06 16:18 PDT by Justin Scott [:fligtar]
Modified: 2011-03-30 10:25 PDT (History)
6 users (show)
dtownsend: in‑testsuite-
hskupin: in‑litmus-
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
final+
-
.17-fixed
-
.19-fixed


Attachments
patch (5.92 KB, patch)
2010-10-26 16:19 PDT, :Margaret Leibovic
dtownsend: review+
Details | Diff | Splinter Review
patch v2 (1.61 KB, patch)
2010-10-28 15:51 PDT, :Margaret Leibovic
no flags Details | Diff | Splinter Review
branch patch (1.81 KB, patch)
2011-03-07 10:52 PST, Dave Townsend [:mossop]
robert.strong.bugs: review+
dveditz: approval1.9.2.17+
dveditz: approval1.9.1.19+
Details | Diff | Splinter Review

Description Justin Scott [:fligtar] 2010-10-06 16:18:12 PDT
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] 2010-10-06 16:32:46 PDT
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 2010-10-26 16:19:16 PDT
Created attachment 486216 [details] [diff] [review]
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] 2010-10-27 13:29:47 PDT
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 2010-10-28 15:51:08 PDT
Created attachment 486767 [details] [diff] [review]
patch v2

Updated to only change API urls.
Comment 5 :Margaret Leibovic 2010-11-05 15:47:48 PDT
http://hg.mozilla.org/mozilla-central/rev/5ade2f730922
Comment 6 Robert Kaiser 2010-11-05 15:53:07 PDT
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] 2010-11-05 15:57:06 PDT
(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] 2010-11-05 16:11:23 PDT
(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) 2010-11-12 00:39:42 PST
(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] 2010-11-12 08:29:13 PST
(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] 2010-11-12 10:48:26 PST
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 2010-11-15 09:52:07 PST
(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] 2010-11-15 11:35:24 PST
(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) 2011-02-10 14:24:11 PST
(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] 2011-02-10 14:41:38 PST
(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) 2011-02-10 17:19:38 PST
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] 2011-02-15 13:16:33 PST
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] 2011-02-18 10:19:22 PST
(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] 2011-03-07 10:52:50 PST
Created attachment 517459 [details] [diff] [review]
branch patch

This patch is applicable to the branches
Comment 20 Dave Townsend [:mossop] 2011-03-07 12:24:11 PST
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] 2011-03-09 11:11:47 PST
Comment on attachment 517459 [details] [diff] [review]
branch patch

Approved for 1.9.2.16 and 1.9.1.18, a=dveditz
Comment 23 Dave Townsend [:mossop] 2011-03-30 10:25:50 PDT
We can see it in the prefs, I don't think it's worth a test for this

Note You need to log in before you can comment on or make changes to this bug.