Closed
Bug 602367
Opened 14 years ago
Closed 14 years ago
Add src parameter to AMO API pings
Categories
(Toolkit :: Add-ons Manager, defect)
Toolkit
Add-ons Manager
Tracking
()
VERIFIED
FIXED
mozilla2.0b8
People
(Reporter: fligtar, Assigned: Margaret)
References
Details
Attachments
(2 files, 1 obsolete file)
1.61 KB,
patch
|
Details | Diff | Splinter Review | |
1.81 KB,
patch
|
robert.strong.bugs
:
review+
dveditz
:
approval1.9.2.17+
dveditz
:
approval1.9.1.19+
|
Details | Diff | Splinter Review |
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•14 years ago
|
||
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•14 years ago
|
||
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•14 years ago
|
Whiteboard: [good first bug] → [good first bug][needs review dtownsend]
Comment 3•14 years ago
|
||
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•14 years ago
|
||
Updated to only change API urls.
Attachment #486216 -
Attachment is obsolete: true
Assignee | ||
Updated•14 years ago
|
Whiteboard: [good first bug][needs review dtownsend] → [can land]
Assignee | ||
Comment 5•14 years ago
|
||
http://hg.mozilla.org/mozilla-central/rev/5ade2f730922
Status: NEW → RESOLVED
Closed: 14 years ago
Resolution: --- → FIXED
Whiteboard: [can land]
Comment 6•14 years ago
|
||
what should this additional parameter should be for Thunderbird or SeaMonkey? is it just ?src=thunderbird and ?src=seamonkey there?
Comment 7•14 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? I would guess so, but up to the AMO team to say what they are expecting
Reporter | ||
Comment 8•14 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.
Comment 9•14 years ago
|
||
(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•14 years ago
|
||
(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•14 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•14 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?
Comment 13•14 years ago
|
||
(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•13 years ago
|
||
(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
Comment 15•13 years ago
|
||
(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: --- → ?
Comment 16•13 years ago
|
||
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-
Comment 17•13 years ago
|
||
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•13 years ago
|
||
(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: ? → -
Comment 19•13 years ago
|
||
This patch is applicable to the branches
Attachment #517459 -
Flags: review?(robert.bugzilla)
Updated•13 years ago
|
Attachment #517459 -
Flags: review?(robert.bugzilla) → review+
Comment 20•13 years ago
|
||
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 21•13 years ago
|
||
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+
Comment 22•13 years ago
|
||
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
Comment 23•13 years ago
|
||
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.
Description
•