Closed Bug 406026 Opened 13 years ago Closed 9 years ago

Consider allowing the blocklist url to direct to a particular point in the blocklist

Categories

(Toolkit :: Add-ons Manager, enhancement)

enhancement
Not set

Tracking

()

VERIFIED FIXED
mozilla7

People

(Reporter: mossop, Assigned: peregrino)

References

Details

Attachments

(1 file, 5 obsolete files)

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.
Product: Firefox → Toolkit
Duplicate of this bug: 627539
Severity: normal → enhancement
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.
Blocks: 629818
(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.
(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.
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
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.
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?
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?
(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.
Must stop reading titles. Thanks for the clarification.
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.
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.
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.
(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.
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
Is this this proper format for blockID?

<emItem id="guid" blockID="12">
<pluginItem blockID="13">
<gfxBlacklistEntry blockID="14">
(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
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.
Definitely but I'm getting pretty swamped already. Maybe Mehdi might be interested in tackling it?
Whiteboard: [wanted fx5]
(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
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?
(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.
Attached patch Patch for functionality. (obsolete) — Splinter Review
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)
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-
Will this make Firefox 6?
Whiteboard: [wanted fx5] → [wanted fx6]
(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
(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.
(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.
Attached patch WIP patch (obsolete) — Splinter Review
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)
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+
Attached patch WIP patch (obsolete) — Splinter Review
Updated patch, only tests missing
Attachment #533483 - Attachment is obsolete: true
Attachment #534125 - Flags: review?(dtownsend)
Comment on attachment 534125 [details] [diff] [review]
WIP patch

Waiting on the latest patch here.
Attachment #534125 - Flags: review?(dtownsend)
Attached patch Pproposed Patch (obsolete) — Splinter Review
Final patch that passes all the tests
Attachment #534125 - Attachment is obsolete: true
Attachment #534593 - Flags: review?(dtownsend)
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-
Attached patch Pproposed Patch (obsolete) — Splinter Review
Addressed modifications told in last comment
Attachment #534593 - Attachment is obsolete: true
Attachment #534617 - Flags: review?(dtownsend)
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+
Attached patch Final PatchSplinter Review
Final patch passing all tests on tryserver
Attachment #534617 - Attachment is obsolete: true
Landed: http://hg.mozilla.org/mozilla-central/rev/e07897c7b558
Status: NEW → RESOLVED
Closed: 9 years ago
Flags: in-testsuite+
Flags: in-litmus-
Resolution: --- → FIXED
Whiteboard: [wanted fx6]
Target Milestone: --- → mozilla7
Attachment #535187 - Attachment is patch: true
Attachment #535187 - Attachment mime type: text/x-patch → text/plain
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
(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
Blocks: 673880
You need to log in before you can comment on or make changes to this bug.