Now that bug 217286 has landed, I need to get off my butt on this one. At the very least, need an Allowed Sites tab with the ability to add sites. Next, should be able to directly blacklist sites as well (using similar UI to Allowed Sites). Finally, the user should be able to white/blacklist from the Manage Cookies tab. There's actually more complex stuff that I'm thinking about for the cookie management (tree view, explorer view), but this is a good start, I'll file a separate bug for more complex/more usable Manage Cookies behaviour.

I really want this in 1.6a

see bug 33467, esp comments 35 onward. that should give you something to start from...

based somewhat on mvl's patch in bug 33467, except it doesn't use a dialog for adding sites (dialogs suck!) and actually lets you whitelist sites you've visited from the Manage Sites window. It would be interesting to filter the sites shown in Manage Sites to not show sites on the whitelist, so that people managing cookies don't have to keep filtering them, but I want to think about that more

Comment on attachment 133584 [details] [diff] [review] patch minor note, mvl mentioned that the whitelist button on the Manage Cookies tab is a little big and not completely consistent with the checkbox to blacklist. Really, I think the whole UI for that tab is rather useless, and this is a provisional entry before I rework the tab as a whole, but that won't be before the trunk freeze (but hopefully soon after it reopen) we could drop the button in the meantime if its that big of an issue

this is probably a better idea for now, since the other button was mostly provisional on a wider rewrite

Comment on attachment 133591 [details] [diff] [review] alternate patch sans button on Manage Cookies tab >Index: mozilla/extensions/wallet/cookieviewer/resources/content/CookieViewer.js >=================================================================== >+function setCookiePermissions(action) { >+ if (action == "block") >+ perm = false; >+ else >+ perm = true; perm = action != "block"; // or just |== "allow";| ? >+ var site = document.getElementById('cookie-site'); >+ // trim any leading space and scheme nit: indentation of comment r=me, but that doesn't count for much. please also get r=neil and sr=jag/alecf/someone ;)

Comment on attachment 133591 [details] [diff] [review] alternate patch sans button on Manage Cookies tab neil, if you could take a look at this too, per dwitte

Comment on attachment 133591 [details] [diff] [review] alternate patch sans button on Manage Cookies tab >+ var site = document.getElementById('cookie-site'); >+ buttonEnabling(site); >+ site.focus(); This is wrong for several reasons: 1. At startup you know the site will be a blank field so you can just set disabled="true" attributes on the buttons 2. The cookie-site isn't in the active tab when the dialog opens 3. Don't focus textboxes in an onload handler >+function setCookiePermissions(action) { action should be one of nsIPermissionManager.ALLOW_ACTION or nsIPermissionManager.DENY_ACTION >+ var site = document.getElementById('cookie-site'); >+ // trim any leading space and scheme >+ site.value = site.value.replace(/^\s*([-\w]*:\/+)?/, ""); Use a local variable for this, please. Also make the replacement string http:// to save adding it later. Or better still, fix .add to take a string instead of a uri :-P >+ if (site.value) { This should be unnecessary, buttonEnabling should enable based on the trimmed value.

new patch coming in ~3 hours (still at work)

-> retargeting I might actually get the whole rewrite done and obsolete this patch completely

okay, so all four review comments addressed also removed the duplicate buttonEnabling call that dwitte spotted (oninput fires when site.value = ""; gets handled, so we don't need to call it again)

Comment on attachment 133996 [details] [diff] [review] better patch, addressing Neil's comments + site.value = ""; + site.focus(); Would you mind switching these around, I'm not convinced that oninput fires when you change the value unless the field already has focus. >+ textfield.value = textfield.value.replace(/^\s*([-\w]*:\/+)?/, ""); Sorry, but this is no good. Please make this a variable (I'd prefer a local, recomputing it in setCookiePermissions with a replacement string of "http://").

/me gets it now, or so he thinks

Comment on attachment 134045 [details] [diff] [review] patch I even fixed the odd indenting this time!

Comment on attachment 134045 [details] [diff] [review] patch Almost... >+ var url = site.value.replace(/^\s*([-\w]*:\/+)?/, "http://");; Fix that ;; while you're there. >+ var url = textfield.value.replace(/^\s*([-\w]*:\/+)?/, ""); >+ var block = document.getElementById("btnBlock"); >+ var allow = document.getElementById("btnAllow"); >+ block.disabled = !url.value; >+ allow.disabled = !url.value; .value is wrong. Also, call it "site" instead?

nothing like uploading the wrong diff...

Comment on attachment 134066 [details] [diff] [review] this should be it sr=alecf

Comment on attachment 134066 [details] [diff] [review] this should be it sr=alecf

fixed on trunk.

mconner: did this patch add the same functions to image manager/blocker?

V/fixed, Mac OS X, Mozilla 1.7RC2. I finally had some time to play with this feature. very nice. adding w/ the block and allow buttons behaves correctly. I did not test session yet. The buttons also over-write existing entries. Some inline-editing would probably be the next step, I'll dupe check before I file more RFE's.

yes, image manager picked this up since the source file is the same. Obviously the session button is hidden there :)