Closed
Bug 406026
Opened 17 years ago
Closed 13 years ago
Consider allowing the blocklist url to direct to a particular point in the blocklist
Categories
(Toolkit :: Add-ons Manager, enhancement)
Toolkit
Add-ons Manager
Tracking
()
VERIFIED
FIXED
mozilla7
People
(Reporter: mossop, Assigned: peregrino)
References
Details
Attachments
(1 file, 5 obsolete files)
30.86 KB,
patch
|
Details | Diff | Splinter Review |
The blocklist page is likely to get long as we blocklist more and more. It would be helpful to be able to link to a particular section of the page or even a unique page for each extension/plugin that has been plocklisted.
Updated•16 years ago
|
Product: Firefox → Toolkit
Reporter | ||
Updated•13 years ago
|
Severity: normal → enhancement
Comment 2•13 years ago
|
||
I'm going to push for the blocklist page to move to AMO in the near future so that it will be updated dynamically with the blocklist itself. I think the easiest way to handle this is to just append the blocked item's GUID to the URL and AMO can only display blocks relevant to that item. Sure, there might be multiple blocks for a particular item, but it's not so bad to see them all in those rare cases and this being automated is a lot easier. I'd like to see this in Firefox 5 as it's a pretty minor change and I suspect the blocklist is going to get pretty large in the coming months.
Comment 3•13 years ago
|
||
(In reply to comment #2) > I think the easiest way to handle this is to just append the blocked item's > GUID to the URL and AMO can only display blocks relevant to that item. Sure, > there might be multiple blocks for a particular item, but it's not so bad to > see them all in those rare cases and this being automated is a lot easier. That wouldn't work for plugins - they don't have a GUID.
Comment 4•13 years ago
|
||
(In reply to comment #3) > (In reply to comment #2) > > I think the easiest way to handle this is to just append the blocked item's > > GUID to the URL and AMO can only display blocks relevant to that item. Sure, > > there might be multiple blocks for a particular item, but it's not so bad to > > see them all in those rare cases and this being automated is a lot easier. > > That wouldn't work for plugins - they don't have a GUID. Indeed. Including URLs with every blocklist item will significantly increase the daily download size of the file. So I'd like to find an automatic way to do this if possible. Mossop, any ideas? If we were to include a unique block id that was included in each item, could that be used instead?
I agree (and suggested it in a dupe, bug 627539). Add unique ids to entries that are appended to the query string (comma delineated?). That way if multiple are being blocked at once we can show all relevant data. If there is no query string, by default the page shows everything/what's there currently. We could even include ?type="plugin|extension|dll" automagically (because the xml tags are different for plugins and extension blocklist items I think) and use that to make the kitchen sink info page smaller / more relevant if there is no id specified.
Reporter | ||
Comment 6•13 years ago
|
||
Yeah, including a unique block ID in the blocklist is probably the way to go then just use that as the url fragment or something
Comment 7•13 years ago
|
||
Okay, <emItem id="item_1@domain"> already has an id in it, so I guess we'd need to make something like blockID="12" for emItem, pluginItem, and gfxBlacklistEntry. Can we agree on a format soon, as we'll be making some changes to server-side blocklist in an upcoming release and it'd be good to roll this in with that.
Comment 8•13 years ago
|
||
Dave, this seems like a minor change that has a big win as we're going to start blocklisting *a lot* more stuff in the coming months. Moving the blocklist page to AMO is scheduled for a milestone that codefreezes this week, so we can get the server side portion up quickly. Do you think this could make 4?
Comment 9•13 years ago
|
||
be nice if we could start pointing at individual blocklist items instead of a section of the blocklist, too, but I am thinking that's a separate bug, isn't it?
Comment 10•13 years ago
|
||
(In reply to comment #9) > be nice if we could start pointing at individual blocklist items instead of a > section of the blocklist, too, but I am thinking that's a separate bug, isn't > it? Nope, that's what this bug is about. We're not using anchors on the page; it will be part of the URL.
Comment 11•13 years ago
|
||
Must stop reading titles. Thanks for the clarification.
Reporter | ||
Comment 12•13 years ago
|
||
I'm not terribly optimistic about this making 4.0 final at this point however I believe that it would be possible to take this in a maintenance release shortly after.
Reporter | ||
Comment 13•13 years ago
|
||
Just considered one issue that might make this more difficult to fix before Firefox 5. The current blocklist UI (https://bug455906.bugzilla.mozilla.org/attachment.cgi?id=345404) displays a list of newly blocked items and then a single link for more info, so that one link would have to support displaying multiple things somehow.
Comment 14•13 years ago
|
||
If there are multiple items, we can omit the id from the URL and it will show the full page with all of them and they can view more details for each individually from there if desired.
Reporter | ||
Comment 15•13 years ago
|
||
(In reply to comment #7) > Okay, <emItem id="item_1@domain"> already has an id in it, so I guess we'd need > to make something like blockID="12" for emItem, pluginItem, and > gfxBlacklistEntry. > > Can we agree on a format soon, as we'll be making some changes to server-side > blocklist in an upcoming release and it'd be good to roll this in with that. This seems like a fine suggestion to me. If you wanted more resolution you could put it on the versionRange inside there, this would allow different block IDs for different blocked versions of the same extension but that is probably going to be rare.
Comment 16•13 years ago
|
||
Okay, great. So to summarize changes for this bug: * we'll add a unique blockID to all the block items (add-ons, plugins, gfx) * the blocklist URL will be changed to AMO and I'll post that once it's live * if there is a single blockID, it should be appended to the blocklist URL to view information on only that block * if there is no blockID or multiple blockIDs, it should be ommitted from the URL to show all blocks
Comment 17•13 years ago
|
||
Is this this proper format for blockID? <emItem id="guid" blockID="12"> <pluginItem blockID="13"> <gfxBlacklistEntry blockID="14">
Reporter | ||
Comment 18•13 years ago
|
||
(In reply to comment #17) > Is this this proper format for blockID? > > <emItem id="guid" blockID="12"> > <pluginItem blockID="13"> > <gfxBlacklistEntry blockID="14"> Yes that is fine
Comment 19•13 years ago
|
||
Items have a blockID with https://github.com/jbalogh/zamboni/commit/0726516.
Comment 20•13 years ago
|
||
Mossop, it seems like this could be a good Fx5 candidate, though not sure if you'd have time to do it before next week. We expect the server-side changes to be live next week or the week after.
Reporter | ||
Comment 21•13 years ago
|
||
Definitely but I'm getting pretty swamped already. Maybe Mehdi might be interested in tackling it?
Whiteboard: [wanted fx5]
Comment 22•13 years ago
|
||
(In reply to comment #21) > Definitely but I'm getting pretty swamped already. Maybe Mehdi might be > interested in tackling it? Sounds good to me :) Jeff, Justin: is there a staging version of the new blocklist?
Assignee: nobody → mars.martian+bugmail
Comment 23•13 years ago
|
||
Staging is https://addons.allizom.org/z/blocklist/3/%7Bec8030f7-c20a-464f-9b0e-13a3a9e97384%7D/4.0/. The HTML version on AMO is staged at https://addons.allizom.org/z/en-US/firefox/blocked/ and https://addons.allizom.org/z/en-US/firefox/blocked/%blockID%. The final URL will be https://addons.mozilla.org/%LOCALE%/%APP%/blocked/%blockID%. %blockID% will come from blocklist.xml. There's a bug right now where blockID is not showing up everywhere, but the fix is on its way to staging. Do we want blocklist.xml to include links to the AMO URLs or is that something that belongs in Firefox?
Reporter | ||
Comment 24•13 years ago
|
||
(In reply to comment #23) > Do we want blocklist.xml to include links to the AMO URLs or is that something > that belongs in Firefox? We should keep that in Firefox I think. Mehdi, as a guide what I was expecting this code to look like included adding a couple of methods to nsIBlocklistService.idl called getBlocklistURLForAddon and getBlocklistURLForPlugin roughly matching getAddonBlocklistState and getPluginBlocklistState but returning the specific URL for the passed in item.
Comment 25•13 years ago
|
||
This implements the feature and seems to work. Dave mentioned that he wanted testing for this and I'll be working on that separately. Some bits of this patch that might be a point of contention: - This makes the blocklist dialog non-modal. With the dialog modal, clicking on a link would create a new Firefox window that, once closed, would return the user to a UI where mouse clicks were not taken into effect. It didn't seem like a problem with this patch and while it is a separate matter, it can be easily fixed here and I think having the dialog be non-modal does not tarnish its effectiveness. - Variables and properties are named |blockID| which kind of deviates from our camelcase style guide. I did this because it's named the same way in blocklist.xml and I wanted to remain consistent.
Attachment #525556 -
Flags: feedback?(dtownsend)
Reporter | ||
Comment 26•13 years ago
|
||
Comment on attachment 525556 [details] [diff] [review] Patch for functionality. Couple of things to clear up here, where are the changes for the add-ons manager UI? >diff --git a/toolkit/mozapps/extensions/nsBlocklistService.js b/toolkit/mozapps/extensions/nsBlocklistService.js > var ww = Cc["@mozilla.org/embedcomp/window-watcher;1"]. > getService(Ci.nsIWindowWatcher); > ww.openWindow(null, URI_BLOCKLIST_DIALOG, "", >- "chrome,centerscreen,dialog,modal,titlebar", args); >+ "chrome,centerscreen,dialog,titlebar", args); This is going to break things because the code immediately after is going to execute before the user has made their choices in the window. >diff --git a/xpcom/system/nsIBlocklistService.idl b/xpcom/system/nsIBlocklistService.idl Could I persuade you to move this to toolkit/mozapps/extensions while you're here? It was only put in xpcom/system due to a build system issue that is no longer a problem. >+ /** >+ * Determine the blocklist web page of an add-on. >+ * @param id >+ * The ID of the blocked add-on. >+ * @returns The URL of the description page. >+ */ >+ AString getAddonBlocklistURL(in AString id); I'd like this to match the signature for getAddonBlocklistState and also take a version parameter. Unused for now but might be useful in the future.
Attachment #525556 -
Flags: feedback?(dtownsend) → feedback-
Comment 28•13 years ago
|
||
(In reply to comment #27) > Will this make Firefox 6? Haven't heard any status updates in a while, but just wanted to note that the server-side portions of this are all live in production now. The URL to use is in comment #23
Reporter | ||
Comment 29•13 years ago
|
||
(In reply to comment #28) > (In reply to comment #27) > > Will this make Firefox 6? > > Haven't heard any status updates in a while, but just wanted to note that the > server-side portions of this are all live in production now. The URL to use is > in comment #23 Mehdi was making noises about finishing this patch off but otherwise we'll get this done for Firefox 6.
Comment 30•13 years ago
|
||
(In reply to comment #29) > Mehdi was making noises about finishing this patch off but otherwise we'll > get this done for Firefox 6. There are a few things left to do: - make the dialog non-modal and move code that runs after to an event listener - address nits in comment 26 - change link that shows up in Addons Manager when an addon is disabled - make a test I should have some time this coming Wednesday to finish all of these.
Assignee | ||
Comment 31•13 years ago
|
||
Taking this from mmm, this is the current status of the patch: * Changed the getAddonBlocklistURL signature * Moved the code after opening the new window to an async callback I will leave moving the nsIBlocklistService.idl to toolkit/mozapps/extensions until is finished so that viewing the diff is easier (otherwise, all the lines will be new...) And as mmm says, this is still missing: - change link that shows up in Addons Manager when an addon is disabled - make a test
Assignee: mars.martian+bugmail → colmeiro
Attachment #525556 -
Attachment is obsolete: true
Attachment #533483 -
Flags: feedback?(dtownsend)
Reporter | ||
Comment 32•13 years ago
|
||
Comment on attachment 533483 [details] [diff] [review] WIP patch Review of attachment 533483 [details] [diff] [review]: ----------------------------------------------------------------- Looking good so far ::: toolkit/mozapps/extensions/nsBlocklistService.js @@ +427,5 @@ > + this._loadBlocklist(); > + > + let blItem = this._addonEntries[id]; > + if (!blItem || !blItem.blockID) > + return ""; Let's return null here, makes more sense as its the case where we don't have a blocklist URI. Same for the plugin case @@ +767,5 @@ > result[id].push(new BlocklistItemData(null)); > + > + let blockID = blocklistElement.getAttribute("blockID"); > + if (blockID) > + result[id].blockID = blockID; No need for the if. getAttribute will return null if there is no attribute so just assign it directly as below. @@ +894,5 @@ > + } > + > + if (!matchFailed) > + return !blockEntry.blockID ? > + "" : this._createBlocklistURL(blockEntry.blockID); Use braces around a multi-line statement @@ +1014,5 @@ > + /* > + Some tests run without UI, so the async code listens to a message > + that can be sent programatically > + */ > + blocklistWindow.addEventListener("unload",function(event){ nit: Put a space before the opening brace and after the comma @@ +1015,5 @@ > + Some tests run without UI, so the async code listens to a message > + that can be sent programatically > + */ > + blocklistWindow.addEventListener("unload",function(event){ > + if(event.target.location == URI_BLOCKLIST_DIALOG) { You don't need to check this, you added the event to the window your interested in so it's guaranteed to be coming from there. @@ +1024,2 @@ > > + Services.obs.addObserver({ observe: function(aSubject,aTopic,aData){ You can actually just pass the plain function here. nit: Spaces after commas and before the opening brace @@ +1033,5 @@ > + addonList[i].item.userDisabled = true; > + } > + if (args.restart) > + restartApp(); > + }},"addon-blocklist-closed",false) All this block is indented two spaces too far. So this isn't exactly what I was suggesting you do but it turns out when I was thinking more about it last night that there were problems with even that, so what you've got here is probably about as good as it gets. I'd suggest however creating a function that the unload event handler calls or the observer calls so we don't have to dispatch the observer notification during normal use, only during tests. ::: xpcom/system/nsIBlocklistService.idl @@ +117,5 @@ > + > + /** > + * Determine the blocklist web page of an add-on. > + * @param id > + * The ID of the blocked add-on. Indent the argument name and description so it lines up with the returns description below.
Attachment #533483 -
Flags: feedback?(dtownsend) → feedback+
Assignee | ||
Comment 33•13 years ago
|
||
Updated patch, only tests missing
Attachment #533483 -
Attachment is obsolete: true
Attachment #534125 -
Flags: review?(dtownsend)
Reporter | ||
Comment 34•13 years ago
|
||
Comment on attachment 534125 [details] [diff] [review] WIP patch Waiting on the latest patch here.
Attachment #534125 -
Flags: review?(dtownsend)
Assignee | ||
Comment 35•13 years ago
|
||
Final patch that passes all the tests
Attachment #534125 -
Attachment is obsolete: true
Attachment #534593 -
Flags: review?(dtownsend)
Reporter | ||
Comment 36•13 years ago
|
||
Comment on attachment 534593 [details] [diff] [review] Pproposed Patch Review of attachment 534593 [details] [diff] [review]: ----------------------------------------------------------------- There's a small problem that could cause a leak so let's get that fixed before we land this. ::: toolkit/mozapps/extensions/nsBlocklistService.js @@ +893,5 @@ > + } > + > + if (!matchFailed) { > + if(!blockEntry.blockID) { > + return null Nit: misses a semicolon and no need for braces around single statement if/else parts like this @@ +1022,5 @@ > + /* > + Some tests run without UI, so the async code listens to a message > + that can be sent programatically > + */ > + let blocklistCbk = function(){ Nit: space before the opening brace. Let's call the function applyBlocklistChanges. This function needs to remove the observer added for "addon-blocklist-closed" or we'll leak one for every time the UI is shown. Simplest way is just to add applyBlocklistChanges as the observer listener and have it remove itself. @@ +1025,5 @@ > + */ > + let blocklistCbk = function(){ > + for (let i = 0; i < addonList.length; i++) { > + if (!addonList[i].disable) > + continue; Keep the space after this line @@ +1030,5 @@ > + if (addonList[i].item instanceof Ci.nsIPluginTag) > + addonList[i].item.disabled = true; > + else > + addonList[i].item.softDisabled = true; > + } And another space here @@ +1047,5 @@ > + > + blocklistWindow.addEventListener("unload", function(event) { > + if(event.target.location == URI_BLOCKLIST_DIALOG) { > + blocklistCbk(); > + blocklistWindow.removeEventListener("unload",arguments.callee); Nit: Spaces after commas ::: toolkit/mozapps/extensions/test/xpcshell/test_bug514327_2.js @@ +69,5 @@ > do_throw("Plugin tag not found"); > > + //run the code after the blocklist is closed > + Services.obs.notifyObservers(null, "addon-blocklist-closed", null); > + do_timeout(0, function(){ Use do_execute_soon instead
Attachment #534593 -
Flags: review?(dtownsend) → review-
Assignee | ||
Comment 37•13 years ago
|
||
Addressed modifications told in last comment
Attachment #534593 -
Attachment is obsolete: true
Attachment #534617 -
Flags: review?(dtownsend)
Reporter | ||
Comment 38•13 years ago
|
||
Comment on attachment 534617 [details] [diff] [review] Pproposed Patch Review of attachment 534617 [details] [diff] [review]: ----------------------------------------------------------------- Looking good, two things to tweak. ::: toolkit/mozapps/extensions/nsBlocklistService.js @@ +1035,5 @@ > + if (args.restart) > + restartApp(); > + > + Services.obs.notifyObservers(self, "blocklist-updated", ""); > + Services.obs.removeObserver(this,"addon-blocklist-closed"); Spaces after commas, and you need to use arguments.callee rather than this to refer to the executing function @@ +1040,3 @@ > } > > + Services.obs.addObserver(applyBlocklistChanges,"addon-blocklist-closed",false) Put a space after each comma
Attachment #534617 -
Flags: review?(dtownsend) → review+
Assignee | ||
Comment 39•13 years ago
|
||
Final patch passing all tests on tryserver
Attachment #534617 -
Attachment is obsolete: true
Reporter | ||
Comment 40•13 years ago
|
||
Landed: http://hg.mozilla.org/mozilla-central/rev/e07897c7b558
Status: NEW → RESOLVED
Closed: 13 years ago
Flags: in-testsuite+
Flags: in-litmus-
Resolution: --- → FIXED
Whiteboard: [wanted fx6]
Target Milestone: --- → mozilla7
Updated•13 years ago
|
Attachment #535187 -
Attachment is patch: true
Attachment #535187 -
Attachment mime type: text/x-patch → text/plain
Comment 41•13 years ago
|
||
Verified fixed with Mozilla/5.0 (Macintosh; Intel Mac OS X 10.6; rv:6.0a2) Gecko/20110529 Firefox/6.0a2 ID:20110529042002 I have used the steps from our existent Litmus test and simply added the blockID item to the custom blocklist: https://litmus.mozilla.org/show_test.cgi?id=10853 Dave, so this time we don't need a manual test? I could simply update the above mentioned test. But if that's not necessary I'm ok with.
Status: RESOLVED → VERIFIED
Reporter | ||
Comment 42•13 years ago
|
||
(In reply to comment #41) > Verified fixed with Mozilla/5.0 (Macintosh; Intel Mac OS X 10.6; rv:6.0a2) > Gecko/20110529 Firefox/6.0a2 ID:20110529042002 > > I have used the steps from our existent Litmus test and simply added the > blockID item to the custom blocklist: > https://litmus.mozilla.org/show_test.cgi?id=10853 > > Dave, so this time we don't need a manual test? I could simply update the > above mentioned test. But if that's not necessary I'm ok with. I think we're fine on automated testing for the url so I'd just leave the test as-is
You need to log in
before you can comment on or make changes to this bug.
Description
•