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

Status: RESOLVED FIXED

(Camino Graveyard :: Preferences, defect, P3)

Description

[Wevah (Nate Weaver)]

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

[Jasper]

Attachment: 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

[Jasper]

wrong bug, that was bound to happen some day ;)

Comment 3

[Wevah (Nate Weaver)]

Assigning to olivier.

Comment 4

[Wevah (Nate Weaver)]

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

Marking attached image obsolete since this is the wrong bug for it. ;)

Comment 5

[olivier]

Attachment: force a refilter when changing the value of a cookie permission

Comment 6

[Simon Fraser [no longer active]]

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.

Comment 7

[olivier]

(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

[Simon Fraser [no longer active]]

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.

Comment 9

[olivier]

Attachment: force refilter after modification of a value (with in order iteration)

Comment 10

[Wevah (Nate Weaver)]

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

Comment 11

[Simon Fraser [no longer active]]

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.

Comment 12

[olivier]

Attachment: patchfile reflecting last comments

Comment 13

[olivier]

Attachment: patchfile reflecting last comments

Comment 14

[Josh Aas]

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.

Comment 15

[olivier]

Attachment: patch reflecting last comments

Comment 16

[Mike Pinkerton (not reading bugmail)]

+      if ([[NSString stringWithUTF8Stringhost.get()]
rangeOfStringinFilterString].location == NSNotFound)

this doesn't compile. Where is |-rangeOfStringinFilterString| defined?

Comment 17

[Mike Pinkerton (not reading bugmail)]

oops, figured it out, the patch had some small errors. corrected and landed.

Comment 18

[Smokey Ardisson (offline for a while; not following bugs - do not email)]

Comment on attachment 188808 [details] [diff] [review]
patchfile reflecting last comments

Clearing requests on obsolete patches on a fixed bug; apologies for the
bugspam.