Cocoa wrapper for nsIPermissionManager

RESOLVED FIXED

Status

Camino Graveyard
General
RESOLVED FIXED
10 years ago
10 years ago

People

(Reporter: Stuart Morgan, Assigned: Stuart Morgan)

Tracking

({fixed1.8.1.5})

Details

Attachments

(3 attachments, 1 obsolete attachment)

(Assignee)

Description

10 years ago
Created attachment 263329 [details] [diff] [review]
wrapper

Since dealing with nsIPermissionManager makes several of our pref panes much nastier than they need to be, I wrapped in and nsIPermission in a Cocoa API; now that 1.5 is basically done it seems like a good time to start pushing it through review.

I have follow-up patches rewriting Privacy and Web Features on this API, but I'm breaking it up into self-contained chunks for ease of reviewing.
Attachment #263329 - Flags: review?
(Assignee)

Comment 1

10 years ago
Created attachment 263330 [details] [diff] [review]
v2

Helps when I remember to finish commenting the header.

Also, this will of course need a project patch for the new files.
Attachment #263329 - Attachment is obsolete: true
Attachment #263330 - Flags: review?
Attachment #263329 - Flags: review?
(Assignee)

Comment 2

10 years ago
Comment on attachment 263330 [details] [diff] [review]
v2

Josh, since no-one is biting, would you be willing to take a look at this?
Attachment #263330 - Flags: review? → review?(joshmoz)

Comment 3

10 years ago
Comment on attachment 263330 [details] [diff] [review]
v2

extern const int CHPermissionUnknown;

Why is this extern? It is only used in the impl itself. Perhaps you're planning to use it in the pref pane impls in the future?

+  if (!mManager)
+    return;
+  mManager->Remove(nsDependentCString([host UTF8String]), [type UTF8String]);

Why don't you just reverse the !mManager test and do the Remove call instead of an early return?

Same thing for removeAllPermissions.

Same thing at the end of setPolicy:forURI:type:

Just fix that stuff on checkin.
Attachment #263330 - Flags: review?(joshmoz) → review+
(Assignee)

Comment 4

10 years ago
(In reply to comment #3)
> Why is this extern? It is only used in the impl itself. Perhaps you're planning
> to use it in the pref pane impls in the future?

It's just a generally useful constant for dealing with permissions, and I'd like this API to be general.

> Why don't you just reverse the !mManager test and do the Remove call instead of
> an early return?

Will do. I was just in a groove of "do sanity checking, then continue".
(Assignee)

Updated

10 years ago
Attachment #263330 - Flags: superreview?(mikepinkerton)
Comment on attachment 263330 [details] [diff] [review]
v2

sr=pink

+@interface CHPermission : NSObject {
+ @private
+  NSString* mHost;
+  NSString* mType;

i assume these are strong refs?

I'd like to see at least some function-level and perhaps some inline comments about these functions. Yeah, they're straightforward, but documentation about pre/post conditions and any hidden assumptions never hurts.
Attachment #263330 - Flags: superreview?(mikepinkerton) → superreview+
Created attachment 267753 [details] [diff] [review]
Trunk project patch
Created attachment 267754 [details] [diff] [review]
Branch project patch
(Assignee)

Comment 8

10 years ago
Landed on trunk and MOZILLA_1_8_BRANCH with some additional commenting.

Prefpane patches will follow before long in new bugs.
Status: NEW → RESOLVED
Last Resolved: 10 years ago
Keywords: fixed1.8.1.5
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.