Should be able to blacklist extensions centrally, in update.mozilla.org

RESOLVED FIXED in mozilla1.8.1alpha1

Status

()

defect
P2
normal
RESOLVED FIXED
15 years ago
11 years ago

People

(Reporter: brendan, Assigned: rstrong)

Tracking

({fixed1.8.1})

unspecified
mozilla1.8.1alpha1
Points:
---
Dependency tree / graph
Bug Flags:
blocking1.8b5 -
blocking-firefox2 +

Firefox Tracking Flags

(Not tracked)

Details

()

Attachments

(2 attachments, 4 obsolete attachments)

Reporter

Description

15 years ago
Should we ship one that has a bad security hole, or even if one we don't host
becomes widespread and then compromised.  This is like the Windows registry's
ActiveX blacklist, but web-wide, not per-machine (might want per-machine too for
a layered administrative approach).

/be

Comment 1

15 years ago
Bug 250854 is related ("XPInstall Permission Manager: Blacklist feature").

Prog.

Comment 2

14 years ago
This seems like a really good one to try to get for 1.1. Any chance it's gonna
happen?
Flags: blocking-aviary1.1?
Whiteboard: [asaP1]

Updated

14 years ago
Assignee: bugs → benjamin
Flags: blocking-aviary1.1? → blocking-aviary1.1+

Updated

14 years ago
Priority: -- → P2
Target Milestone: --- → Firefox1.1

Updated

14 years ago
Blocks: 290759
Benjamin, is this realistic at this point with everything else that's happening?
Reporter

Comment 4

14 years ago
Prioritize what else is happening, and then let's schedule -- we don't have a
realistic schedule right now, so I would much rather we all sort out what must,
should, and would be nice to have for 1.1.  This is somewhere between should and
must in my book, so don't starve it for unknown "everthing elses".

/be

Comment 5

14 years ago
This needs coordination with the extension update service, but after that gets
sorted out this is pretty simple to implement.

Comment 6

14 years ago
That would be bug 290759.  Tell us the format you want.  I guess it will be
something similar to 
http://lxr.mozilla.org/update1.0/source/update/VersionCheck.php#200

Updated

14 years ago
Flags: blocking1.8b4?

Updated

14 years ago
Flags: blocking1.8b4? → blocking1.8b4+

Comment 7

14 years ago
shaver, you were going to take a look at this?  any reasonable chance of this
getting in to 1.5? e.g. it will need to land before we lockdown for 1.8b4 within
the next week or so.

/cb
Whiteboard: [asaP1] → [asaP1] [at risk] [needs owner]
I was going to look at 290759, which is the client side of this work.
(I could have sworn I made this comment the other day...)

This is indeed the client-side work.  I'm waiting for Ben's EM patch in 296566
to land, and then I'll be able to better scope and schedule the work.  I have
hopes that it will land today!

Updated

14 years ago
Assignee: benjamin → shaver

Comment 10

14 years ago
-> robstrong per phone discussion
Assignee: shaver → rob_strong
Per the phone conversation I stated I would take on the EM portion of this bug
and not the other aspects (e.g. plugins, UM0 which I believe may be another bug,
service for retrieving or reading the blacklist, etc.). Shaver also promised a
sample file to work from that would be attached to this bug.

Updated

14 years ago
Whiteboard: [asaP1] [at risk] [needs owner] → [asaP1] [ETA ?]
Per the phone conversation I will take on the EM portion of this bug - I don't
have the time available to take on or drive the other aspects like disabling
plugins, server side work for the blacklist, etc. Time is running short and
without the base functionality of having the blacklist data to work from I may
not have the time available to finish the EM portion. A simplified description
of the EM portion includes:
ui for user notification that an item has been blacklisted
disabling of blacklisted extensions

What it does not include:
ui for and the disabling of plugins
server side work for the blacklist
periodically retrieving the blacklist or whatever is decided upon.
format of the blacklist

-> shaver
Assignee: rob_strong → shaver

Comment 13

14 years ago
Per drivers discussion, this is not going to make 1.5, but we will keep it on
the burner as something we'll implement in a dot release if need be to handle
any security or integrity issues introduced by Extensions or plugins.
Flags: blocking1.8b4-
Flags: blocking1.8b4+
Flags: blocking-aviary2.0?
Whiteboard: [asaP1] [ETA ?]

Updated

14 years ago
Flags: blocking-aviary1.5+
Fx2 if not earlier.  (see comment 13)
Flags: blocking-aviary2? → blocking-aviary2+
Blocks: 318338
Assignee: shaver → robert.bugzilla
Target Milestone: Firefox1.5 → Firefox 2 alpha2
Since we are adding restart capability when blacklisted items are found during the background check it would be a good thing to have session restore capability... adding deps on bug 328154
Depends on: 328154
Posted patch patch (obsolete) — Splinter Review
Benjamin, this adds some of what will be needed to support toolkit version knowledge in the EM... I can remove it if you think it is premature.

Darin, this affects app update as well and I'd appreciate your review on the code in general as well.
Attachment #213121 - Flags: superreview?(darin)
Attachment #213121 - Flags: review?(benjamin)
Posted patch patch (fixed typo) (obsolete) — Splinter Review
Had a typo in the last patch.

Also, the strings were provided by beltzner. If there are string changes I'd prefer to take care of them in bug 308916 instead of this bug though comments would be welcome.
Attachment #213134 - Flags: superreview?(darin)
Attachment #213134 - Flags: review?(benjamin)
Attachment #213121 - Attachment is obsolete: true
Attachment #213121 - Flags: superreview?(darin)
Attachment #213121 - Flags: review?(benjamin)

Comment 19

13 years ago
Comment on attachment 213134 [details] [diff] [review]
patch (fixed typo)

>   removeFile: function(file) {
>+    if (!file)
>+      return;

Why is this necessary? I understand being defensive, but I don't see how its good to end up with null here.

>+  /**
>+   * The blocklist XML file looks something like this:
>+   *
>+   * <blocklist xmlns="http://www.mozilla.org/2006/blocklist">
>+   *   <emItems>

What is the "emItems" for? Do you expect to extend this format in the future to return other information? Do we handle other items gracefully?

>+  _loadBlocklistFromFile: function(file) {

>+      var parser = Components.classes["@mozilla.org/xmlextras/domparser;1"]
>+                            .createInstance(Components.interfaces.nsIDOMParser);

nit: whitespace lineup

>+      var doc = parser.parseFromStream(fileStream, "UTF-8", fileStream.available(), "text/xml");
>+      
>+      for (var i = 0; i < doc.documentElement.childNodes.length; ++i) {
>+        var emItemsElement = doc.documentElement.childNodes.item(i);
>+        if (emItemsElement.nodeType == Components.interfaces.nsIDOMNode.ELEMENT_NODE ||
>+            emItemsElement.localName == "emItems")
>+          break;
>+      }

Why don't we test the namespace also (? What happens here if we don't find an <emItems> element? Seems to me we need some additional defense against malformed input. Why not put this code below inside the loop, with a helper func if you think that would make it more readable.

I'm vaguely worried about the fact that the EM service is popping up UI for blocked installs in various places; in an ideal world that would be handled by callbacks/observers from the client code, but I understand that's out of scope for 1.8.

>+   * @param   toolkitVersion
>+   *          The version of the toolkit we are checking for compatibility
>+   *          against. If this parameter is undefined, the version of the running
>+   *          application's toolkit is used.

>-  isCompatible: function (datasource, source, version) {
>+  isCompatible: function (datasource, source, appVersion, toolkitVersion) {

What are the situations where somebody would want/need to pass an explicit toolkitVersion instead of using gApp.platformVersion? Is this here to deal with what extensions would become incompatible in update scenarios? I don't see any usage of toolkitVersion later in the function, is that going to be added later as part of the other bug?

>+  _rdfGet_detailsURL: function(item, property) {
>+    var id = stripPrefix(item.Value, PREFIX_ITEM_URI);
>+    if (this.getItemProperty(id, "blocklisted") == "false")
>+      return EM_L("none");

nit: "none"? seems to me it would be better to return NS_RDF_NO_VALUE or an empty string.

>Index: browser/app/profile/firefox.js

>+// Blocklist preferences
>+pref("extensions.blocklist.enabled", true);
>+pref("extensions.blocklist.interval", 86400);
>+pref("extensions.blocklist.url", "https://addons.mozilla.org/blocklist/firefox.xml");
>+pref("extensions.blocklist.detailsURL", "http://www.mozilla.com/blocklist/");

If these prefs are missing does the EM recover gracefully?

>+  var sbs = Components.classes["@mozilla.org/intl/stringbundle;1"]
>+                      .getService(Components.interfaces.nsIStringBundleService);
>+  var strings = sbs.createBundle(URI_BLOCKLIST_PROPERTIES);

Is there any particular reason you're using sbs directly instead of using <stringbundle> in the XUL? I'm not up on the decision to use one or the other, I just thought that the XUL version was preferred for some reason (it would block onload, perhaps, until it was loaded?).

>Index: toolkit/mozapps/extensions/content/blocklist.css
>===================================================================
>RCS file: toolkit/mozapps/extensions/content/blocklist.css
>diff -N toolkit/mozapps/extensions/content/blocklist.css
>--- /dev/null	1 Jan 1970 00:00:00 -0000
>+++ toolkit/mozapps/extensions/content/blocklist.css	25 Feb 2006 00:42:55 -0000
>@@ -0,0 +1,5 @@
>+blocklistItem {
>+  -moz-binding: url("chrome://mozapps/content/extensions/blocklist.xml#blocklistItem");
>+  -moz-box-orient: vertical;
>+}

nit: in general I'd prefer not to invent new XUL element names. How about <richlistitem class="blocklistItem">?
Attachment #213134 - Flags: review?(benjamin) → review-
(In reply to comment #19)
> (From update of attachment 213134 [details] [diff] [review] [edit])
> >   removeFile: function(file) {
> >+    if (!file)
> >+      return;
> Why is this necessary? I understand being defensive, but I don't see how its
> good to end up with null here.
That snuck in from other tests I'm doing for EM failures... I'll remove it.

> >+  /**
> >+   * The blocklist XML file looks something like this:
> >+   *
> >+   * <blocklist xmlns="http://www.mozilla.org/2006/blocklist">
> >+   *   <emItems>
> 
> What is the "emItems" for? Do you expect to extend this format in the future to
> return other information? Do we handle other items gracefully?
When we add plugins this same file can be used (e.g. I dislike the idea of downloading a second file for plugins and potentially other items in the future).
Yes... with plugins being the only one planned.
Yes, other items are handled gracefully.

> >+  _loadBlocklistFromFile: function(file) {
> 
> >+      var parser = Components.classes["@mozilla.org/xmlextras/domparser;1"]
> >+                            .createInstance(Components.interfaces.nsIDOMParser);
> 
> nit: whitespace lineup
Will do.

> >+      var doc = parser.parseFromStream(fileStream, "UTF-8", fileStream.available(), "text/xml");
> >+      
> >+      for (var i = 0; i < doc.documentElement.childNodes.length; ++i) {
> >+        var emItemsElement = doc.documentElement.childNodes.item(i);
> >+        if (emItemsElement.nodeType == Components.interfaces.nsIDOMNode.ELEMENT_NODE ||
> >+            emItemsElement.localName == "emItems")
> >+          break;
> >+      }
> 
> Why don't we test the namespace also (? What happens here if we don't find an
> <emItems> element? Seems to me we need some additional defense against
> malformed input. Why not put this code below inside the loop, with a helper
> func if you think that would make it more readable.
I'll review this again this evening to make sure it does the right thing.

> I'm vaguely worried about the fact that the EM service is popping up UI for
> blocked installs in various places; in an ideal world that would be handled by
> callbacks/observers from the client code, but I understand that's out of scope
> for 1.8.
It is a bit of a PITA :/
I could move the blocked install notification into extensions.js but that would also require canceling installs when the ui is closed instead of just completing the install. I believe that notification would be a bit tricky unless we just didn't care about the edgecases where the client code didn't have an observer (e.g. options panel, other chrome apps, etc.).

> >+   * @param   toolkitVersion
> >+   *          The version of the toolkit we are checking for compatibility
> >+   *          against. If this parameter is undefined, the version of the running
> >+   *          application's toolkit is used.
> 
> >-  isCompatible: function (datasource, source, version) {
> >+  isCompatible: function (datasource, source, appVersion, toolkitVersion) {
> 
> What are the situations where somebody would want/need to pass an explicit
> toolkitVersion instead of using gApp.platformVersion? Is this here to deal with
> what extensions would become incompatible in update scenarios? I don't see any
> usage of toolkitVersion later in the function, is that going to be added later
> as part of the other bug?
Yes on all counts... this is the "adds some of what will be needed to support toolkit version knowledge in the EM... I can remove it if you think it is premature." as noted in comment #17

> >+  _rdfGet_detailsURL: function(item, property) {
> >+    var id = stripPrefix(item.Value, PREFIX_ITEM_URI);
> >+    if (this.getItemProperty(id, "blocklisted") == "false")
> >+      return EM_L("none");
> 
> nit: "none"? seems to me it would be better to return NS_RDF_NO_VALUE or an
> empty string.
I used "none" to be consistent with how this was accomplished in bug 296566 for update notification... I've been working on cleaning up the ui and I believe I have a better approach.

> >Index: browser/app/profile/firefox.js
> 
> >+// Blocklist preferences
> >+pref("extensions.blocklist.enabled", true);
> >+pref("extensions.blocklist.interval", 86400);
> >+pref("extensions.blocklist.url", "https://addons.mozilla.org/blocklist/firefox.xml");
> >+pref("extensions.blocklist.detailsURL", "http://www.mozilla.com/blocklist/");
> 
> If these prefs are missing does the EM recover gracefully?
I believe so but I'll double check before resubmitting.

> >+  var sbs = Components.classes["@mozilla.org/intl/stringbundle;1"]
> >+                      .getService(Components.interfaces.nsIStringBundleService);
> >+  var strings = sbs.createBundle(URI_BLOCKLIST_PROPERTIES);
> 
> Is there any particular reason you're using sbs directly instead of using
> <stringbundle> in the XUL? I'm not up on the decision to use one or the other,
> I just thought that the XUL version was preferred for some reason (it would
> block onload, perhaps, until it was loaded?).
No and I'll fix this.

> >Index: toolkit/mozapps/extensions/content/blocklist.css
> >===================================================================
> >RCS file: toolkit/mozapps/extensions/content/blocklist.css
> >diff -N toolkit/mozapps/extensions/content/blocklist.css
> >--- /dev/null	1 Jan 1970 00:00:00 -0000
> >+++ toolkit/mozapps/extensions/content/blocklist.css	25 Feb 2006 00:42:55 -0000
> >@@ -0,0 +1,5 @@
> >+blocklistItem {
> >+  -moz-binding: url("chrome://mozapps/content/extensions/blocklist.xml#blocklistItem");
> >+  -moz-box-orient: vertical;
> >+}
> 
> nit: in general I'd prefer not to invent new XUL element names. How about
> <richlistitem class="blocklistItem">?
Not a problem

Updated

13 years ago
Attachment #213134 - Flags: superreview?(darin)
Benjamin, sorry for the delay and I should be done with this over the weekend. I talked over a couple of changes that beltzner would like and I am looking at if / how I can utilize the existing list.xul / list.js instead of creating blocklist.xul, etc. - needing to show this from a service is a bit of a PITA but there is no other way to do so without creating cases where we wouldn't show the dialog.

Comment 23

13 years ago
Do we have something for "More Info" to redirect to?
It is in the works and will be hosted on mozilla.com. The xml file will be hosted on AMO.
QA Contact: bugs → extension.manager
Posted patch patch (obsolete) — Splinter Review
Attachment #213134 - Attachment is obsolete: true
Posted patch patch (obsolete) — Splinter Review
Attachment #213968 - Attachment is obsolete: true
Comment on attachment 214204 [details] [diff] [review]
patch

Benjamin, I am not going to add the changes to app update in this bug since there are other more drastic changes needed to it in Bug 324121. Otherwise, this should address your review comments.

(In reply to comment #19)
> What is the "emItems" for? Do you expect to extend this format in the future to
> return other information? Do we handle other items gracefully?
When we add plugin blocklisting the format will almost certainly be different hence the use of emItems which handle EM managed items.

> >+      var doc = parser.parseFromStream(fileStream, "UTF-8", fileStream.available(), "text/xml");
> >+      
> >+      for (var i = 0; i < doc.documentElement.childNodes.length; ++i) {
> >+        var emItemsElement = doc.documentElement.childNodes.item(i);
> >+        if (emItemsElement.nodeType == Components.interfaces.nsIDOMNode.ELEMENT_NODE ||
> >+            emItemsElement.localName == "emItems")
> >+          break;
> >+      }
> 
> Why don't we test the namespace also (? What happens here if we don't find an
> <emItems> element? Seems to me we need some additional defense against
> malformed input. Why not put this code below inside the loop, with a helper
> func if you think that would make it more readable.
The only value I see in testing for the namespace is possibly for versioning and I added a check.
If we don't find an <emItems> element the blocklist will be empty and no items will be blocklisted.
I added a helper and though I could see adding a second it wouldn't provide much value.

> I'm vaguely worried about the fact that the EM service is popping up UI for
> blocked installs in various places; in an ideal world that would be handled by
> callbacks/observers from the client code, but I understand that's out of scope
> for 1.8.
I changed this slightly so we pass null for notifications and attempt to use the appropriate manager when blocking an install.

> What are the situations where somebody would want/need to pass an explicit
> toolkitVersion instead of using gApp.platformVersion? Is this here to deal with
> what extensions would become incompatible in update scenarios? I don't see any
> usage of toolkitVersion later in the function, is that going to be added later
> as part of the other bug?
I added it so the interface wouldn't need to be changed for when the EM supports a targetApplication for toolkit@mozilla.org in bug 299716. Just to verify, is that still the name we want?

> 
> >+  _rdfGet_detailsURL: function(item, property) {
> >+    var id = stripPrefix(item.Value, PREFIX_ITEM_URI);
> >+    if (this.getItemProperty(id, "blocklisted") == "false")
> >+      return EM_L("none");
> 
> nit: "none"? seems to me it would be better to return NS_RDF_NO_VALUE or an
> empty string.
Fixed for the blocklist. I am planning on changing availableUpdateURL so it doesn't return "none" in bug 329045.

> If these prefs are missing does the EM recover gracefully?
Yes.
If blocklist url is not present it bails.
If blocklist enabled is not present it defaults to true
If blocklist interval is not present it defaults to 86400
If blocklist details url is not present the ui doesn't display the url

> nit: in general I'd prefer not to invent new XUL element names. How about
> <richlistitem class="blocklistItem">?
I modified lists.xul, etc. to support blocklist notification, added an info icon (I planned on doing this for 2.0 and needed it for this), and fixed a leak in list.js.
Attachment #214204 - Flags: review?(benjamin)
Comment on attachment 214204 [details] [diff] [review]
patch

Darin, with Benjamin on paternity leave I'm hoping you would review this in his stead?
Attachment #214204 - Flags: review?(benjamin) → review?(darin)

Comment 29

13 years ago
Comment on attachment 214204 [details] [diff] [review]
patch

>Index: toolkit/mozapps/extensions/public/nsIExtensionManager.idl
...
>+  /**
>+   * Checks for changes to the blocklist, application disables / enables items
>+   * that have been added / removed from the blocklist, and if there are
>+   * additions displays a list of the items added.
>+   */
>+  void checkForBlocklistChanges();

Just reading this comment, leaves me with several questions:

1) Is the checking asynchronous or synchronous?
2) If the checking is asynchronous, then what happens if I call this
method twice in quick succession?
3) How does someone using this API observe the changes to the blocklist?


>Index: toolkit/mozapps/extensions/src/nsExtensionManager.js.in

>+const XMLURI_BLOCKLIST                = "http://www.mozilla.org/2006/blocklist";

Perhaps the namespace URL should include the word "extensions" or "addons":

  "http://www.mozilla.org/2006/addons-blocklist"


>+var Blocklist = {
>+  /**
>+   * Extension ID -> array of Version Ranges
>+   * Each value in the version range array is a JS Object that has the
>+   * following properties:
>+   *   "minVersion"  The minimum version in a version range (default = 0).
>+   *   "maxVersion"  The maximum version in a version range (default = *).
>+   *   "targetApps"  Application ID -> array of Version Ranges
>+   *                 (default = current application ID)
>+   *                 Each value in the version range array is a JS Object that
>+   *                 has the following properties:
>+   *   "minVersion"  The minimum version in a version range (default = 0).
>+   *   "maxVersion"  The maximum version in a version range (default = *).
>+   */
>+  entries: null,

This comment block seems out-of-date.


>+  notify: function() {
>+    var defaults = gPref.QueryInterface(Components.interfaces.nsIPrefService)
>+                        .getDefaultBranch(null);
>+    try {
>+      var dsURI = defaults.getCharPref(PREF_BLOCKLIST_URL);
>+      if (defaults.getBoolPref(PREF_BLOCKLIST_ENABLED) == false)
>+        return;

So, you do not allow users to easily override the default settings
for these prefs?  Why?


>+    request.channel.loadFlags |= Components.interfaces.nsIRequest.LOAD_BYPASS_CACHE;

Hrm... are we sure we don't want to support caching the blocklist?
Maybe the blocklist will grow large over time.  Maybe caching will
help reduce server load.  How often do we check for updates?  If we
only check once per day, then the blocklist can be cached for a day.
That could have a huge impact on performance.  Just food for thought
I guess.  Since we control the server hosting the blocklist, we could
choose to tune the cache options server-side.  But, this code change
prevents that altogether.


>+    request.onerror = function(event) { self.onXMLError(event); };
>+    request.onload  = function(event) { self.onXMLLoad(event);  };
>+    request.send(null);

By the way, if |send| throws an exception, then we end up leaking
the request object and the scope of those anonymous functions.  It
is therefore better (albeit awkward) to set onerror and onload after
calling send.


>+    var blocklistFile = getFile(KEY_PROFILEDIR, [FILE_BLOCKLIST]);
>+    if (blocklistFile.exists())
>+      blocklistFile.remove(false);

If the blocklistFile is busy (windows), then this will throw.  Do
things return to an okay state in that case?


>+    var em = Components.classes["@mozilla.org/extensions/manager;1"]
>+                       .getService(Components.interfaces.nsIExtensionManager)
>+                       .QueryInterface(Components.interfaces.nsIExtensionManager2);

Why not pass nsIExtensionManager2 directly to getService?
Also, for the 1.8 branch, we've been adopting the policy
of naming interfaces that are exclusively for the 1.8 branch:
nsIExtensionManager_MOZILLA_1_8_BRANCH

It's damn ugly, but it is done for good reason:  To avoid
conflicting with a future nsIExtensionManager2 interface
that might be introduced later on the trunk.


>+      var doc = parser.parseFromStream(fileStream, "UTF-8", fileStream.available(), "text/xml");

file.fileSize is probably better than fileStream.available() since
the available() method need not report the entire length of the
data stream.  (In this case it does, but best to avoid such
dependencies IMO)


>+      var itemNodes = this._getItemNodes(doc.documentElement);
>+      for (var i = 0; i < itemNodes.length; ++i) {
>+        var blocklistElement = itemNodes.item(i);
>+        if (blocklistElement.nodeType != Components.interfaces.nsIDOMNode.ELEMENT_NODE ||
>+            blocklistElement.localName != "emItem")
>+          continue;

since you're inside a loop, it might be good to create a temp
variable outside the loop body for nsIDOMNode.  that way you
don't pay for looking it up over and over.


>+        for (var x = 0; x < blocklistElement.childNodes.length; ++x) {
>+          var versionRangeElement = blocklistElement.childNodes.item(x);

you might want a local var for blocklistElement.childNodes as well
for similar reasons.


>+            // appDisabled is determined by an item being compatible,
>+            // satisfying its dependencies, and not being blocklisted

So, a user cannot override the blocklist?  That seems bad to me.


>+  checkForBlocklistChanges: function() {
...
>+    // show the blocklist notification window if there are new blocklist items.
>+    if (items.length > 0)
>+      showBlocklistMessage(items, false);
>+  },

Ah, so the user will be prompted by this function, and it just
consults the offline copy of the blocklist.  OK.  This should
be documented in the interface IMO.


>+    if (!responseXML || responseXML.documentElement.namespaceURI == XMLURI_PARSE_ERROR ||
>+        (request.status != 200 && request.status != 0)) {

This same test is repeated elsewhere.  Perhaps we should move it
into a subroutine.


Sorry, I am not the best person to review UI changes.  I suggest
getting UI review on screenshots of the feature, and then I
suggest getting a xul hacker like beng to review your xul+js+css
UI code changes.


r=darin on the extension manager portion of this patch
Attachment #214204 - Flags: review?(darin) → review+
Comment on attachment 213956 [details]
screenshot of blocklist dialogs

Mike, I went ahead and made the changes we discussed when you were at the office.
Attachment #213956 - Flags: ui-review?(beltzner)
Comment on attachment 213956 [details]
screenshot of blocklist dialogs

thanks, Rob. I'm pretty sure I'll eventually think of a better way to do the "More information" bit, but I can't think of it at this particular moment, and it seems like the sort of thing that's easy to do in a follow-on bug.

(Click here for _more information_? Find out _what is considered dangerous_? This is driving me _nuts_?)
Attachment #213956 - Flags: ui-review?(beltzner) → ui-review+
Comment on attachment 214204 [details] [diff] [review]
patch

Scott, the files left to be reviewed are extensions.js, extensions.xml, extensions.xul, list.js, list.xul, and extensions.css. The ui is currently for all practical purposes overloading an element for status info which is why some of the css is a bit funky. I'll be taking care of that in bug 329045.
Attachment #214204 - Flags: superreview?(mscott)

Comment 33

13 years ago
Comment on attachment 214204 [details] [diff] [review]
patch

you left me the easy part.

1) Please add the new blocklist prefs to all-thunderbird.js as well.

2) Instead of calling setAttribute to set the class attribute on infoIcon, you can use .className. You can also get rid of the switch statement, simplifying things:

if (iconClass in params)  document.getElementById("infoIcon").className = 'spaced ' + params["iconClass"];

that line may need tweaked to account for the default "alert-icon" case using a ?: syntax.

3) This:

+    message = document.getElementById("moreInfoBox")
+    message.hidden = false;

Can just be  document.getElementById("moreInfoBox").hidden = false;

4) message.setAttribute("value"
can you just do message.value = ...
here?
Attachment #214204 - Flags: superreview?(mscott) → superreview+
This addresses the review comments... carrying over reviews

I'll be working with morgamic to improve the caching scheme.
I'm thinking of adding code to manage the XMLHttpRequests especially for extension update check where it is more of an issue.
It recovers gracefully if the file can't be removed and it throws.
Attachment #214204 - Attachment is obsolete: true
Attachment #214469 - Flags: review+
Checked in on trunk. Please file new bugs for any regressions and assign them to me. In case there is any confusion... the bug for creating / hosting the blocklist file on AMO is bug 290759 and has not been completed yet.
Status: NEW → RESOLVED
Last Resolved: 13 years ago
Resolution: --- → FIXED
Target Milestone: Firefox 2 alpha2 → Firefox 2 alpha1
Comment on attachment 214469 [details] [diff] [review]
patch for checkin

a=ben@mozilla.org for the 1.8.1 branch
Attachment #214469 - Flags: approval-branch-1.8.1+
Fixed on MOZILLA_1_8_BRANCH
Keywords: fixed1.8.1

Comment 38

13 years ago
Could we get an example extension (blacklisted on a.m.o) to do l10n work?
*** Bug 339949 has been marked as a duplicate of this bug. ***
Product: Firefox → Toolkit
You need to log in before you can comment on or make changes to this bug.