Closed
Bug 430120
Opened 17 years ago
Closed 17 years ago
Update blocklist URL to include same info as update URL
Categories
(Toolkit :: Add-ons Manager, defect)
Tracking
()
VERIFIED
FIXED
mozilla1.9
People
(Reporter: alex, Assigned: mossop)
References
Details
(Keywords: verified1.8.1.15)
Attachments
(2 files, 2 obsolete files)
20.40 KB,
patch
|
mossop
:
review+
beltzner
:
approval1.9+
|
Details | Diff | Splinter Review |
11.59 KB,
patch
|
robert.strong.bugs
:
review+
dveditz
:
approval1.8.1.15+
|
Details | Diff | Splinter Review |
Currently the extensions.blocklist.url is
https://addons.mozilla.org/blocklist/1/%APP_ID%/%APP_VERSION%/
and the app.update.url is
https://aus2.mozilla.org/update/3/%PRODUCT%/%VERSION%/%BUILD_ID%/%BUILD_TARGET%/%LOCALE%/%CHANNEL%/%OS_VERSION%/%DISTRIBUTION%/%DISTRIBUTION_VERSION%/update.xml
We need extensions.blocklist.url to contain all of the same information, which means we need to add the following to it:
%PRODUCT%
%VERSION%
%BUILD_ID%
%BUILD_TARGET%
%LOCALE%
%CHANNEL%
%OS_VERSION%
%DISTRIBUTION%
%DISTRIBUTION_VERSION%
This will require updates to the Firefox 3 pref, and modifications to the web service that supports the blocklist URL.
Comment 1•17 years ago
|
||
we've been wanting to move to this ping instead of the AUS ping for a while now to be able to keep track of what our usage looks like, to watch out for weird anomalies, etc -- now that the earlier bug has been fixed & this looks like it's giving us reliable feedback, would like to move to blocklist pings across the board, but need this other locale, channel, OS, etc info. would be very very very good to get this into fx3. (and, actually, into any updates of fx2 we do.)
Assignee | ||
Comment 2•17 years ago
|
||
I can work on the necessary changes to the prefs and blocklist service.
Assignee: nobody → dtownsend
Comment 3•17 years ago
|
||
Should this be blocking 1.9 or fx3?
Comment 4•17 years ago
|
||
BTW - I am all for this. I think we have other uses for this information aside from just metrics. For example -- business logic for specific platforms, locales, etc. that extends past the original scope of the blocklist XML.
Updated•17 years ago
|
Flags: blocking-firefox3+
Assignee | ||
Comment 5•17 years ago
|
||
This is an initial patch that allows the blocklist service to replace the same parameters that the update service does. It includes a unit test to verify all the parameters are correctly replaced.
This patch can be safely checked in now and then once the blocklist service is updated a simple pref change will let us switch to the new url format.
Attachment #316986 -
Flags: review?(robert.bugzilla)
Comment 6•17 years ago
|
||
Does it matter that these are both 404s:
const XMLURI_BLOCKLIST = "http://www.mozilla.org/2006/addons-blocklist";
const XMLURI_PARSE_ERROR = "http://www.mozilla.org/newlayout/xml/parsererror.xml"
Assignee | ||
Comment 7•17 years ago
|
||
(In reply to comment #6)
> Does it matter that these are both 404s:
> const XMLURI_BLOCKLIST =
> "http://www.mozilla.org/2006/addons-blocklist";
> const XMLURI_PARSE_ERROR =
> "http://www.mozilla.org/newlayout/xml/parsererror.xml"
No these are just namespaces used in the xml, they don't have to exist anywhere. Constants should perhaps be XMLNS_... to signify then.
Comment 8•17 years ago
|
||
Dave, are these supposed to have var= added to them (only see &):
+ gPrefs.setCharPref(PREF_BLOCKLIST_URL, "http://localhost:4444/2?" +
+ "%APP_ID%&%APP_VERSION%&%PRODUCT%&%VERSION%&%BUILD_ID%&" +
+ "%BUILD_TARGET%&%LOCALE%&%CHANNEL%&" +
+ "%OS_VERSION%&%PLATFORM_VERSION%&%DISTRIBUTION%&%DISTRIBUTION_VERSION%");
Are they missing query string var names?
Comment 9•17 years ago
|
||
BTW - tracking service changes in bug 430278.
Assignee | ||
Comment 10•17 years ago
|
||
(In reply to comment #8)
> Dave, are these supposed to have var= added to them (only see &):
> + gPrefs.setCharPref(PREF_BLOCKLIST_URL, "http://localhost:4444/2?" +
> + "%APP_ID%&%APP_VERSION%&%PRODUCT%&%VERSION%&%BUILD_ID%&"
> +
> + "%BUILD_TARGET%&%LOCALE%&%CHANNEL%&" +
> +
> "%OS_VERSION%&%PLATFORM_VERSION%&%DISTRIBUTION%&%DISTRIBUTION_VERSION%");
>
> Are they missing query string var names?
I don't see any need for them for the unit test. So long as it is replacing the parameters the surrounding text isn't all that relevant. I would have done it as part of the path rather then the query string but the js http server makes that harder to catch.
Comment 11•17 years ago
|
||
Okay, just looked weird at first for the URL pref. :)
![]() |
||
Comment 12•17 years ago
|
||
Comment on attachment 316986 [details] [diff] [review]
patch rev 1
>diff --git a/toolkit/mozapps/extensions/src/nsBlocklistService.js b/toolkit/mozapps/extensions/src/nsBlocklistService.js
>--- a/toolkit/mozapps/extensions/src/nsBlocklistService.js
>+++ b/toolkit/mozapps/extensions/src/nsBlocklistService.js
>...
>@@ -196,16 +203,87 @@ function matchesOSABI(blocklistElement)
> if (choices.length > 0 && choices.indexOf(gApp.XPCOMABI) < 0)
> return false;
> }
>
> return true;
> }
>
> /**
>+ * Gets the current value of the locale. It's possible for this preference to
>+ * be localized, so we have to do a little extra work here. Similar code
>+ * exists in nsHttpHandler.cpp when building the UA string.
>+ */
>+function getLocale() {
>+ try {
>+ // Get the default branch
>+ var prefs = Components.classes["@mozilla.org/preferences-service;1"]
>+ .getService(Components.interfaces.nsIPrefService);
>+ var defaultPrefs = prefs.getDefaultBranch(null);
>+ return defaultPrefs.getCharPref(PREF_GENERAL_USERAGENT_LOCALE);
>+ } catch (e) {}
>+
>+ return gPref.getCharPref(PREF_GENERAL_USERAGENT_LOCALE);
>+}
>+
>+/**
>+ * Read the update channel from defaults only. We do this to ensure that
>+ * the channel is tightly coupled with the application and does not apply
>+ * to other instances of the application that may use the same profile.
>+ */
nit: s/instances/installations/
>@@ -215,16 +293,54 @@ function Blocklist() {
> gVersionChecker = Cc["@mozilla.org/xpcom/version-comparator;1"].
> getService(Ci.nsIVersionComparator);
> gConsole = Cc["@mozilla.org/consoleservice;1"].
> getService(Ci.nsIConsoleService);
>
> gOS = Cc["@mozilla.org/observer-service;1"].
> getService(Ci.nsIObserverService);
> gOS.addObserver(this, "xpcom-shutdown", false);
>+
>+ // Not all builds have a known ABI
>+ try {
>+ gABI = gApp.XPCOMABI;
>+ }
>+ catch (e) {
Is there a reason not to do what the EM code does here (e.g. UNKNOWN_XPCOM_ABI)?
>+ LOG("UpdateService", "XPCOM ABI unknown: updates are not possible.");
>+ }
Copy / paste from "UpdateService". Also note that the LOG function only takes one param.
>+
>+ var osVersion;
>+ var sysInfo = Components.classes["@mozilla.org/system-info;1"]
>+ .getService(Components.interfaces.nsIPropertyBag2);
>+ try {
>+ osVersion = sysInfo.getProperty("name") + " " + sysInfo.getProperty("version");
>+ }
>+ catch (e) {
>+ LOG("UpdateService", "OS Version unknown: updates are not possible.");
>+ }
same here
Attachment #316986 -
Flags: review?(robert.bugzilla) → review-
Comment 13•17 years ago
|
||
would be very good to get this into fx2.0.0.next also so that we can have coherent tracking.
Flags: blocking1.8.1.15?
Updated•17 years ago
|
Whiteboard: [needs new patch]
Assignee | ||
Comment 14•17 years ago
|
||
Fixes to my sloppy copy and pasting.
I've included the unknownABI bit but would like confirmation from morgamic or Alex that that is an acceptable thing to pass.
I've also included the config changes for review though those can't land till the server side is done (bug 430278). A couple of the apps seem to have an incorrect detailsURL so I've fixed those up too.
Attachment #316986 -
Attachment is obsolete: true
Attachment #317229 -
Flags: review?(robert.bugzilla)
Assignee | ||
Updated•17 years ago
|
Whiteboard: [needs new patch] → [has patch][need review robstrong]
Updated•17 years ago
|
Flags: wanted1.8.1.x+
Flags: blocking1.8.1.15?
Flags: blocking1.8.1.15+
![]() |
||
Comment 15•17 years ago
|
||
Comment on attachment 317229 [details] [diff] [review]
patch rev 2
iirc my main concern with the previous patch was that if the ABI was unknown it looked like it would bail and not download the blocklist. You could always just omit it if you prefer. Either way r=me
Attachment #317229 -
Flags: review?(robert.bugzilla) → review+
![]() |
||
Updated•17 years ago
|
Whiteboard: [has patch][need review robstrong] → [has patch]
Assignee | ||
Comment 16•17 years ago
|
||
This is just a minor tweak to bring the blocklist url in line with what it should be.
Seeking approval to land, this changes the url used to request the blocklist but does not affect the actual functionality of the blocklist. There is better testing that the blocklist service is actually attempting to retrieve included.
Attachment #317229 -
Attachment is obsolete: true
Attachment #317361 -
Flags: review+
Attachment #317361 -
Flags: approval1.9?
Comment 17•17 years ago
|
||
Comment on attachment 317361 [details] [diff] [review]
patch rev 3
a1.9=beltzner
Attachment #317361 -
Flags: approval1.9? → approval1.9+
Updated•17 years ago
|
Keywords: checkin-needed
Whiteboard: [has patch] → [has approval]
Updated•17 years ago
|
Keywords: checkin-needed
Whiteboard: [has approval] → [has approval][needs bug 430278 to land]
Updated•17 years ago
|
Whiteboard: [has approval][needs bug 430278 to land] → [has patch][has approval][needs bug 430278 to land]
Assignee | ||
Comment 18•17 years ago
|
||
Checking in browser/app/profile/firefox.js;
/cvsroot/mozilla/browser/app/profile/firefox.js,v <-- firefox.js
new revision: 1.326; previous revision: 1.325
done
Checking in calendar/sunbird/app/profile/sunbird.js;
/cvsroot/mozilla/calendar/sunbird/app/profile/sunbird.js,v <-- sunbird.js
new revision: 1.54; previous revision: 1.53
done
Checking in mail/app/profile/all-thunderbird.js;
/cvsroot/mozilla/mail/app/profile/all-thunderbird.js,v <-- all-thunderbird.js
new revision: 1.113; previous revision: 1.112
done
Checking in suite/browser/browser-prefs.js;
/cvsroot/mozilla/suite/browser/browser-prefs.js,v <-- browser-prefs.js
new revision: 1.91; previous revision: 1.90
done
Checking in toolkit/mozapps/extensions/src/nsBlocklistService.js;
/cvsroot/mozilla/toolkit/mozapps/extensions/src/nsBlocklistService.js,v <-- nsBlocklistService.js
new revision: 1.12; previous revision: 1.11
done
RCS file: /cvsroot/mozilla/toolkit/mozapps/extensions/test/unit/test_bug430120.js,v
done
Checking in toolkit/mozapps/extensions/test/unit/test_bug430120.js;
/cvsroot/mozilla/toolkit/mozapps/extensions/test/unit/test_bug430120.js,v <-- test_bug430120.js
initial revision: 1.1
done
Status: NEW → RESOLVED
Closed: 17 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Whiteboard: [has patch][has approval][needs bug 430278 to land]
Target Milestone: --- → Firefox 3
Assignee | ||
Comment 19•17 years ago
|
||
On the branch the blocklist is all still bundled in with the EM. This is still basically the same patch though.
Attachment #322920 -
Flags: review?(robert.bugzilla)
![]() |
||
Comment 20•17 years ago
|
||
Comment on attachment 322920 [details] [diff] [review]
branch patch
r=me Thanks!
Attachment #322920 -
Flags: review?(robert.bugzilla) → review+
Assignee | ||
Comment 21•17 years ago
|
||
Comment on attachment 322920 [details] [diff] [review]
branch patch
Seeking approval to land this on the branch.
Attachment #322920 -
Flags: approval1.8.1.15?
Comment 22•17 years ago
|
||
Comment on attachment 322920 [details] [diff] [review]
branch patch
Approved for 1.8.1.15, a=dveditz for release-drivers
Attachment #322920 -
Flags: approval1.8.1.15? → approval1.8.1.15+
Assignee | ||
Comment 23•17 years ago
|
||
Landed on branch
Checking in browser/app/profile/firefox.js;
/cvsroot/mozilla/browser/app/profile/firefox.js,v <-- firefox.js
new revision: 1.71.2.84; previous revision: 1.71.2.83
done
Checking in calendar/sunbird/app/profile/sunbird.js;
/cvsroot/mozilla/calendar/sunbird/app/profile/sunbird.js,v <-- sunbird.js
new revision: 1.17.2.28; previous revision: 1.17.2.27
done
Checking in mail/app/profile/all-thunderbird.js;
/cvsroot/mozilla/mail/app/profile/all-thunderbird.js,v <-- all-thunderbird.js
new revision: 1.44.2.47; previous revision: 1.44.2.46
done
Checking in toolkit/mozapps/extensions/src/nsExtensionManager.js.in;
/cvsroot/mozilla/toolkit/mozapps/extensions/src/nsExtensionManager.js.in,v <-- nsExtensionManager.js.in
new revision: 1.144.2.66; previous revision: 1.144.2.65
done
Keywords: fixed1.8.1.15
Comment 24•17 years ago
|
||
Mozilla/5.0 (Macintosh; U; Intel Mac OS X; en-US; rv:1.8.1.15pre) Gecko/20080610 BonEcho/2.0.0.15pre
Branch looks good. Verifying.
Keywords: fixed1.8.1.15 → verified1.8.1.15
Comment 25•17 years ago
|
||
BTW this is the new extensions.update.url for 1.8.1.15 nightly:
https://addons.mozilla.org/update/VersionCheck.php?reqVersion=%REQ_VERSION%&id=%ITEM_ID%&version=%ITEM_VERSION%&maxAppVersion=%ITEM_MAXAPPVERSION%&status=%ITEM_STATUS%&appID=%APP_ID%&appVersion=%APP_VERSION%&appOS=%APP_OS%&appABI=%APP_ABI%
Updated•17 years ago
|
Product: Firefox → Toolkit
Comment 26•16 years ago
|
||
Verified fix on Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.1.2) Gecko/20090729 Firefox/3.5.2
app.update.url = https://aus2.mozilla.org/update/3/%PRODUCT%/%VERSION%/%BUILD_ID%/%BUILD_TARGET%/%LOCALE%/%CHANNEL%/%OS_VERSION%/%DISTRIBUTION%/%DISTRIBUTION_VERSION%/update.xml
extensions.blocklist.url = https://addons.mozilla.org/blocklist/3/%APP_ID%/%APP_VERSION%/%PRODUCT%/%BUILD_ID%/%BUILD_TARGET%/%LOCALE%/%CHANNEL%/%OS_VERSION%/%DISTRIBUTION%/%DISTRIBUTION_VERSION%/
Updated•16 years ago
|
Status: RESOLVED → VERIFIED
You need to log in
before you can comment on or make changes to this bug.
Description
•