Last Comment Bug 379326 - Cocoa wrapper for nsIPermissionManager
: Cocoa wrapper for nsIPermissionManager
Status: RESOLVED FIXED
: fixed1.8.1.5
Product: Camino Graveyard
Classification: Graveyard
Component: General (show other bugs)
: unspecified
: x86 Mac OS X
: -- normal (vote)
: ---
Assigned To: Stuart Morgan
:
:
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2007-04-30 21:03 PDT by Stuart Morgan
Modified: 2007-06-08 19:45 PDT (History)
1 user (show)
See Also:
QA Whiteboard:
Iteration: ---
Points: ---


Attachments
wrapper (13.93 KB, patch)
2007-04-30 21:03 PDT, Stuart Morgan
no flags Details | Diff | Splinter Review
v2 (14.64 KB, patch)
2007-04-30 21:19 PDT, Stuart Morgan
jaas: review+
mikepinkerton: superreview+
Details | Diff | Splinter Review
Trunk project patch (5.00 KB, patch)
2007-06-08 14:38 PDT, Smokey Ardisson (offline for a while; not following bugs - do not email)
no flags Details | Diff | Splinter Review
Branch project patch (5.00 KB, patch)
2007-06-08 14:43 PDT, Smokey Ardisson (offline for a while; not following bugs - do not email)
no flags Details | Diff | Splinter Review

Description Stuart Morgan 2007-04-30 21:03:44 PDT
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.
Comment 1 Stuart Morgan 2007-04-30 21:19:05 PDT
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.
Comment 2 Stuart Morgan 2007-05-11 14:42:12 PDT
Comment on attachment 263330 [details] [diff] [review]
v2

Josh, since no-one is biting, would you be willing to take a look at this?
Comment 3 Josh Aas 2007-05-14 21:07:58 PDT
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.
Comment 4 Stuart Morgan 2007-05-14 21:22:54 PDT
(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".
Comment 5 Mike Pinkerton (not reading bugmail) 2007-06-06 10:54:16 PDT
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.
Comment 6 Smokey Ardisson (offline for a while; not following bugs - do not email) 2007-06-08 14:38:59 PDT
Created attachment 267753 [details] [diff] [review]
Trunk project patch
Comment 7 Smokey Ardisson (offline for a while; not following bugs - do not email) 2007-06-08 14:43:29 PDT
Created attachment 267754 [details] [diff] [review]
Branch project patch
Comment 8 Stuart Morgan 2007-06-08 19:45:59 PDT
Landed on trunk and MOZILLA_1_8_BRANCH with some additional commenting.

Prefpane patches will follow before long in new bugs.

Note You need to log in before you can comment on or make changes to this bug.