Add src parameter to AMO API pings

VERIFIED FIXED in mozilla2.0b8

Status

()

Toolkit
Add-ons Manager
VERIFIED FIXED
7 years ago
7 years ago

People

(Reporter: fligtar, Assigned: Margaret)

Tracking

Trunk
mozilla2.0b8
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite -
in-litmus -

Firefox Tracking Flags

(blocking2.0 final+, blocking1.9.2 -, status1.9.2 .17-fixed, blocking1.9.1 -, status1.9.1 .19-fixed)

Details

Attachments

(2 attachments, 1 obsolete attachment)

(Reporter)

Description

7 years ago
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
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.
blocking2.0: --- → final+
Whiteboard: [good first bug]
(Assignee)

Comment 2

7 years ago
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?
Assignee: nobody → margaret.leibovic
Attachment #486216 - Flags: review?(dtownsend)
(Assignee)

Updated

7 years ago
Whiteboard: [good first bug] → [good first bug][needs review dtownsend]
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.
Attachment #486216 - Flags: review?(dtownsend) → review+
(Assignee)

Comment 4

7 years ago
Created attachment 486767 [details] [diff] [review]
patch v2

Updated to only change API urls.
Attachment #486216 - Attachment is obsolete: true
(Assignee)

Updated

7 years ago
Whiteboard: [good first bug][needs review dtownsend] → [can land]
(Assignee)

Comment 5

7 years ago
http://hg.mozilla.org/mozilla-central/rev/5ade2f730922
Status: NEW → RESOLVED
Last Resolved: 7 years ago
Resolution: --- → FIXED
Whiteboard: [can land]

Comment 6

7 years ago
what should this additional parameter should be for Thunderbird or SeaMonkey? is it just ?src=thunderbird and ?src=seamonkey there?
(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
(Reporter)

Comment 8

7 years ago
(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.

Updated

7 years ago
Blocks: 610065

Updated

7 years ago
Blocks: 610066
(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?
(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.
(Reporter)

Comment 11

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

Comment 12

7 years ago
(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?
(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.
(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?
Target Milestone: --- → mozilla2.0b8
(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?
blocking1.9.1: --- → ?
blocking1.9.2: --- → ?
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?
Status: RESOLVED → VERIFIED
Flags: in-testsuite?
Flags: in-litmus-
Not t the moment, I don't think it is worth much effort right now either as it is hardcoded in the preference.
(In reply to comment #15)
> Branch drivers, would a patch for this be taken?

Yes, but it's not blocking.
blocking1.9.1: ? → -
blocking1.9.2: ? → -
Created attachment 517459 [details] [diff] [review]
branch patch

This patch is applicable to the branches
Attachment #517459 - Flags: review?(robert.bugzilla)
Attachment #517459 - Flags: review?(robert.bugzilla) → review+
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.
Attachment #517459 - Flags: approval1.9.2.16?
Attachment #517459 - Flags: approval1.9.1.18?
Comment on attachment 517459 [details] [diff] [review]
branch patch

Approved for 1.9.2.16 and 1.9.1.18, a=dveditz
Attachment #517459 - Flags: approval1.9.2.16?
Attachment #517459 - Flags: approval1.9.2.16+
Attachment #517459 - Flags: approval1.9.1.18?
Attachment #517459 - Flags: approval1.9.1.18+
Landed:

http://hg.mozilla.org/releases/mozilla-1.9.1/rev/9c8182d5a5ec
http://hg.mozilla.org/releases/mozilla-1.9.2/rev/12b279eed95f
status1.9.1: --- → .19-fixed
status1.9.2: --- → .17-fixed
We can see it in the prefs, I don't think it's worth a test for this
Flags: in-testsuite? → in-testsuite-
You need to log in before you can comment on or make changes to this bug.