Filtered cookie exceptions list becomes unfiltered if an Allow/Deny value is changed.

RESOLVED FIXED in Camino0.9

Status

P3
normal
RESOLVED FIXED
14 years ago
13 years ago

People

(Reporter: moz, Assigned: olivier)

Tracking

unspecified
Camino0.9
PowerPC
Mac OS X

Details

Attachments

(1 attachment, 5 obsolete attachments)

(Reporter)

Description

14 years ago
If the cookie exceptions list (as per Bug 263336) is filtered and then the
"Allow/Deny" state of one of the items is changed, the list becomes unfiltered,
though the filter text field does not clear. The list should remain filtered
until the the field is explicitly cleared or the sheet is dismissed.

Comment 1

14 years ago
Created attachment 175349 [details]
image to show 10.2 bug

YUCK thank you apple for this "wonderfull" bug.

K so look at the image and you will see the infamous "top doesn't draw
correctly" issue Mike rightfully pointed out.

Comment 2

14 years ago
wrong bug, that was bound to happen some day ;)
(Reporter)

Comment 3

14 years ago
Assigning to olivier.
Assignee: pinkerton → olivier
(Reporter)

Comment 4

14 years ago
Comment on attachment 175349 [details]
image to show 10.2 bug

Marking attached image obsolete since this is the wrong bug for it. ;)
Attachment #175349 - Attachment is obsolete: true
(Assignee)

Comment 5

14 years ago
Created attachment 176912 [details] [diff] [review]
force a refilter when changing the value of a cookie permission
Attachment #176912 - Flags: review?

Comment 6

14 years ago
Comment on attachment 176912 [details] [diff] [review]
force a refilter when changing the value of a cookie permission

>Index: PreferencePanes/Privacy/PrivacyPane.mm
>===================================================================
>RCS file: /cvsroot/mozilla/camino/PreferencePanes/Privacy/PrivacyPane.mm,v
>retrieving revision 1.31

>-        [self populatePermissionCache];
>+        //[self populatePermissionCache]; no need to do it here since filtering the list will call this method

Might as well just remove the line.

>+- (void) filterCookiesPermissionsWithString: (NSString*) inFilterString
>+{
>+  // the user wants to filter down the list of cookies. Reinitialize the list of permission in case
>+  // they deleted or replaced a letter.
>+  [self populatePermissionCache];
>+  
>+  if ([inFilterString length]) {
>+    nsCAutoString host;
>+    for (int row = mCachedPermissions->Count() - 1; row >= 0; row--) {
>+      mCachedPermissions->ObjectAt(row)->GetHost(host);
>+      if ([[NSString stringWithUTF8String: host.get()] rangeOfString: inFilterString].location == NSNotFound)
>+        // remove from cookie permissions list
>+        mCachedPermissions->RemoveObjectAt(row);
>+    }
>+  }
>+}
>+
>+- (void) filterCookiesWithString: (NSString*) inFilterString
>+{
>+  // reinitialize the list of cookies in case user deleted a letter or replaced a letter
>+  [self populateCookieCache];
>+  
>+  if ([inFilterString length]) {
>+    nsCAutoString host;
>+    for (int row = mCachedCookies->Count() - 1; row >= 0; row--) {
>+      // only search on the host
>+      mCachedCookies->ObjectAt(row)->GetHost(host);
>+      if ([[NSString stringWithUTF8String: host.get()] rangeOfString: inFilterString].location == NSNotFound)
>+        mCachedCookies->RemoveObjectAt(row);
>+    }
>+  }
>+}

I'm not a big fan of this backwards enumeration removing items; it's prone to
errors iof someone changes the logic. I'd much prefer that we have a static
array of all items, then copy into a 'filtered' array if there is a search
string.
(Assignee)

Comment 7

14 years ago
(In reply to comment #6)

> >+        //[self populatePermissionCache]; no need to do it here since
filtering the list will call this method
> Might as well just remove the line.
will do

> I'm not a big fan of this backwards enumeration removing items; it's prone to
> errors iof someone changes the logic. I'd much prefer that we have a static
> array of all items, then copy into a 'filtered' array if there is a search
> string.

That was the code of the original patch (I just moved it to a separate method).
Since this code is executed at every keystroke, we probably want to keep it as
efficient as possible. Maybe adding a warning in the comments to point out the
reverse enumeration?

Comment 8

14 years ago
Copying into a new array isn't necessarily more work, and I doubt would have a
perceptible impact on performance. I just think it's more maintainable in future.
(Assignee)

Comment 9

14 years ago
Created attachment 178166 [details] [diff] [review]
force refilter after modification of a value (with in order iteration)
Attachment #176912 - Attachment is obsolete: true
Attachment #178166 - Flags: review?
(Assignee)

Updated

14 years ago
Attachment #178166 - Attachment description: force refilter after modification of a value → force refilter after modification of a value (with in order iteration)
(Assignee)

Updated

14 years ago
Attachment #178166 - Flags: review? → review?(sfraser_bugs)
(Assignee)

Updated

14 years ago
Attachment #176912 - Flags: review?
(Reporter)

Comment 10

14 years ago
Status on reviewing this patch? If no one else is going to, I may take a look at it.

Comment 11

13 years ago
Comment on attachment 178166 [details] [diff] [review]
force refilter after modification of a value (with in order iteration)

>Index: PreferencePanes/Privacy/PrivacyPane.mm
>===================================================================

>+- (void) filterCookiesPermissionsWithString: (NSString*) inFilterString
>+{
>+  // the user wants to filter down the list of cookies. Reinitialize the list of permission in case
>+  // they deleted or replaced a letter.
>+  [self populatePermissionCache];
>+  
>+  if ([inFilterString length]) {
>+    nsCAutoString host;

This can go inside the loop.

>+    NSMutableArray *indexToRemove = [NSMutableArray array];
>+    for (int row = 0; row < mCachedPermissions->Count(); row++) {
>+      mCachedPermissions->ObjectAt(row)->GetHost(host);
>+      if ([[NSString stringWithUTF8String: host.get()] rangeOfString: inFilterString].location == NSNotFound)
>+        // add to our buffer
>+        [indexToRemove addObject: [NSNumber numberWithInt: row]];
>+    }
>+    
>+    //remvoe the items at the saved indexes

"remove"

>+    //
>+    NSEnumerator *theEnum = [indexToRemove objectEnumerator];
>+    NSNumber *currentItem;
>+    while (currentItem = [theEnum nextObject])
>+        mCachedPermissions->RemoveObjectAt([currentItem intValue]);

Does removing objects mess up subsequent indices?

>+- (void) filterCookiesWithString: (NSString*) inFilterString
>+{
>+  // reinitialize the list of cookies in case user deleted a letter or replaced a letter
>+  [self populateCookieCache];
>+  
>+  if ([inFilterString length]) {
>+    nsCAutoString host;

Again, move inside.

>+    NSMutableArray *indexToRemove = [NSMutableArray array];
>+    for (int row = 0; row < mCachedCookies->Count(); row++) {
>+      // only search on the host
>+      mCachedCookies->ObjectAt(row)->GetHost(host);
>+      if ([[NSString stringWithUTF8String: host.get()] rangeOfString: inFilterString].location == NSNotFound)
>+        [indexToRemove addObject: [NSNumber numberWithInt: row]];
>+    }
>+    
>+    //remvoe the items at the saved indexes
>+    //
>+    NSEnumerator *theEnum = [indexToRemove objectEnumerator];
>+    NSNumber *currentItem;
>+    while (currentItem = [theEnum nextObject])
>+      mCachedCookies->RemoveObjectAt([currentItem intValue]);

Same question here.
Attachment #178166 - Flags: review?(sfraser_bugs) → review-
(Assignee)

Comment 12

13 years ago
Created attachment 188808 [details] [diff] [review]
patchfile reflecting last comments
Attachment #188808 - Flags: review?(sfraser_bugs)
(Assignee)

Comment 13

13 years ago
Created attachment 188809 [details] [diff] [review]
patchfile reflecting last comments
Attachment #178166 - Attachment is obsolete: true
Attachment #188808 - Attachment is obsolete: true
Attachment #188809 - Flags: review?(sfraser_bugs)

Updated

13 years ago
Attachment #188809 - Flags: review?(sfraser_bugs) → review+

Updated

13 years ago
Priority: -- → P3
Target Milestone: --- → Camino0.9

Comment 14

13 years ago
Comment on attachment 188809 [details] [diff] [review]
patchfile reflecting last comments

>-        [self populatePermissionCache];
>+        //[self populatePermissionCache]; no need to do it here since filtering the list will call this method
>+        //re-filter

Just get rid of those lines.

>+    //remove the items at the saved indexes
>+    //

No need for the second empty comment line.


>+      if ([[NSString stringWithUTF8String: host.get()] rangeOfString: inFilterString].location == NSNotFound)
>+        [indexToRemove addObject: [NSNumber numberWithInt: row]];

Please don't put spaces between colons and arguments. This happens a lot in
your patch and needs to be fixed.
Attachment #188809 - Flags: review-
(Assignee)

Comment 15

13 years ago
Created attachment 190563 [details] [diff] [review]
patch reflecting last comments
Attachment #188809 - Attachment is obsolete: true
Attachment #190563 - Flags: review?
+      if ([[NSString stringWithUTF8Stringhost.get()]
rangeOfStringinFilterString].location == NSNotFound)

this doesn't compile. Where is |-rangeOfStringinFilterString| defined?
oops, figured it out, the patch had some small errors. corrected and landed.
Status: NEW → RESOLVED
Last Resolved: 13 years ago
Resolution: --- → FIXED
Comment on attachment 188808 [details] [diff] [review]
patchfile reflecting last comments

Clearing requests on obsolete patches on a fixed bug; apologies for the
bugspam.
Attachment #188808 - Flags: review?(sfraser_bugs)
You need to log in before you can comment on or make changes to this bug.