411197 - Share more code in the Privacy preference pane

Status: RESOLVED FIXED

(Camino Graveyard :: Preferences, defect)

Description

[Stuart Morgan]

Now that everything in the Privacy pane is all Cocoa-y, there's a lot of opportunity to share code among the three very similar sheets.

Comment 1

[Stuart Morgan]

Attachment: sharing and cleanup

This shares more of the initialization, filter, and sort logic. The actual filter and sort implementations wouldn't have really benefited from merging, so I left them alone.

Since I was already moving a fair amount of stuff around, it seemed like a good opportunity to do a much-need organization/grouping of methods, so I did that as well--and some style cleanup for good measure, since by then I was touching pretty much everything anyway.

Comment 2

[Stuart Morgan]

Attachment: new nib

Corresponding nib, since I needed to change the filter field actions.

I'm not addicted to revving this nib. I can quit anytime I want!

Comment 3

[Mark Mentovai]

Comment on attachment 295876 [details] [diff] [review]
sharing and cleanup

Cleanup diffs are so gnarly, but this looks good.

>+- (IBAction)launchKeychainAccess:(id)sender

>+  FSRef fsRef;
>+  CFURLRef urlRef;
>+  OSErr err = ::LSGetApplicationForInfo('APPL', 'kcmr', NULL, kLSRolesAll, &fsRef, &urlRef);

We don't need fsRef, you can pass NULL there.  Maybe wrap this line while you're changing it.

>+    CFRelease(fileSystemURL);

Should probably follow the ::pattern used for other calls in this function.

This function leaks, but it's not obvious.  urlRef is actually owned by the callee and needs to be released.

>+  CookieDateFormatter* cookieDateFormatter =
>+    [[CookieDateFormatter alloc] initWithDateFormat:@"%b %d, %Y"
>+                               allowNaturalLanguage:NO];

One of us is going to step on the other of us (411516).  I'll let you go first, mine's easier to merge in after your change than the other way around.

>+  // This is an artifact of something weird in the nib; if the table is
>+  // rebuilt (or the nib is otherwise fixed) this can be removed).

Specifically what?

>++ (int)indexForPolicy:(int)policy

policyForIndex uses switch, why doesn't this?

Comment 4

[Stuart Morgan]

Landed on trunk and MOZILLA_1_8_BRANCH, with all the changes suggested (except for the switch, which the compiler won't let me do). I also added a note that the weird table issue may have been 10.2-only, and that we should re-test sometime.