Closed Bug 222553 Opened 17 years ago Closed 17 years ago

Rework cookieviewer to support whitelisting

Categories

(Core :: Networking: Cookies, defect)

defect
Not set

Tracking

()

VERIFIED FIXED
mozilla1.6beta

People

(Reporter: mconnor, Assigned: mconnor)

References

Details

(Whiteboard: checkwin, checklinux)

Attachments

(1 file, 5 obsolete files)

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
Blocks: 216743
Target Milestone: --- → mozilla1.6alpha
see bug 33467, esp comments 35 onward. that should give you something to start
from...
Attached patch patch (obsolete) — Splinter Review
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
Attachment #133584 - Flags: review?(dwitte)
this is probably a better idea for now, since the other button was mostly
provisional on a wider rewrite
Attachment #133584 - Attachment is obsolete: true
Attachment #133584 - Flags: review?(dwitte)
Attachment #133591 - Flags: review?(dwitte)
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 ;)
Attachment #133591 - Flags: review?(dwitte) → review+
Attachment #133591 - Flags: superreview?(jag)
Attachment #133591 - Flags: superreview?(alecf)
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
Attachment #133591 - Flags: review+ → review?(neil.parkwaycc.co.uk)
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.
Attachment #133591 - Flags: review?(neil.parkwaycc.co.uk) → review-
Attachment #133591 - Flags: superreview?(jag)
Attachment #133591 - Flags: superreview?(alecf)
new patch coming in ~3 hours (still at work)
Status: NEW → ASSIGNED
-> retargeting

I might actually get the whole rewrite done and obsolete this patch completely
Target Milestone: mozilla1.6alpha → mozilla1.6beta
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)
Attachment #133591 - Attachment is obsolete: true
Attachment #133996 - Flags: review?(neil.parkwaycc.co.uk)
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://").
Attachment #133996 - Flags: review?(neil.parkwaycc.co.uk) → review-
Attached patch patch (obsolete) — Splinter Review
/me gets it now, or so he thinks
Attachment #133996 - Attachment is obsolete: true
Comment on attachment 134045 [details] [diff] [review]
patch

I even fixed the odd indenting this time!
Attachment #134045 - Flags: review?(neil.parkwaycc.co.uk)
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?
Attachment #134045 - Flags: review?(neil.parkwaycc.co.uk) → review-
Attached patch v0.5 - can I beat IanN? (obsolete) — Splinter Review
Attachment #134045 - Attachment is obsolete: true
Attachment #134065 - Flags: review?(neil.parkwaycc.co.uk)
Attachment #134065 - Flags: review?(neil.parkwaycc.co.uk)
nothing like uploading the wrong diff...
Attachment #134065 - Attachment is obsolete: true
Attachment #134066 - Flags: review?(neil.parkwaycc.co.uk)
Attachment #134066 - Flags: review?(neil.parkwaycc.co.uk) → review+
Attachment #134066 - Flags: superreview?(alecf)
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
Attachment #134066 - Flags: superreview?(alecf) → superreview+
fixed on trunk.
Status: ASSIGNED → RESOLVED
Closed: 17 years ago
Resolution: --- → FIXED
Blocks: 100573
Blocks: 33467
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.
Status: RESOLVED → VERIFIED
Whiteboard: checkwin, checklinux
yes, image manager picked this up since the source file is the same.  Obviously
the session button is hidden there :)
You need to log in before you can comment on or make changes to this bug.