Share more code in the Privacy preference pane

RESOLVED FIXED in Camino1.6

Status

RESOLVED FIXED
11 years ago
11 years ago

People

(Reporter: stuart.morgan+bugzilla, Assigned: stuart.morgan+bugzilla)

Tracking

({fixed1.8.1.12})

unspecified
Camino1.6
x86
Mac OS X
fixed1.8.1.12

Details

Attachments

(2 attachments)

(Assignee)

Description

11 years ago
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.
(Assignee)

Comment 1

11 years ago
Created attachment 295876 [details] [diff] [review]
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.
Attachment #295876 - Flags: superreview?(mark)
(Assignee)

Comment 2

11 years ago
Created attachment 295877 [details]
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

11 years ago
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?
Attachment #295876 - Flags: superreview?(mark) → superreview+
(Assignee)

Comment 4

11 years ago
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.
Status: NEW → RESOLVED
Last Resolved: 11 years ago
Keywords: fixed1.8.1.12
Resolution: --- → FIXED
Target Milestone: --- → Camino1.6
You need to log in before you can comment on or make changes to this bug.