Closed Bug 209506 Opened 22 years ago Closed 22 years ago

[cookies] remove contractid's for nsICookie & nsIPermission

Categories

(Core :: Networking: Cookies, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED

People

(Reporter: dwitte, Assigned: dwitte)

References

Details

Attachments

(1 file, 1 obsolete file)

Currently, we declare factory constructors for nsICookie and nsIPermission (along with contractids) in the cookie modulefactory. This allows these objects to be instantiated by the world, which is wrong - these objects should only be created by the backend, and the world should be able to QI to them. (There exist no xpcom setters on these interfaces). A side-effect of this is that a default ctor is required on the impls, which is irritating. If I understand correctly, we should be able to remove the factory constructors and contractids without hurting things. Patch coming up.
Hardware: DEC → All
Attached patch v1 patch (obsolete) — Splinter Review
what this does: 1) removes factory ctors & contractids for nsICookie/nsIPermission 2) adds private default ctors, to prevent nsCOMPtr warnings about the missing ctor 3) cleans up nsIPermission a bit (remove crufty nsSupportsWeakReference as we've already done for nsICookie, and reduce class size by using PRUint16's) 4) fix an OOM condition in nsCookie's ctor. the last two are unrelated, but while i'm in there... ;)
Comment on attachment 125729 [details] [diff] [review] v1 patch darin/bz, would be great if you could do the honors ;) also mvl, can you take a look at this? it touches your turf so would be nice if you could ok it.
Attachment #125729 - Flags: superreview?(bzbarsky)
Attachment #125729 - Flags: review?(darin)
this patch saves 1.2k of codesize on linux btw, which is nice for such a simple change.
Comment on attachment 125729 [details] [diff] [review] v1 patch // {28F16D80-157B-11d5-A542-0010A401EB10} #define NS_PERMISSION_CID \ { 0x28f16d80, 0x157b, 0x11d5, { 0xa5, 0x42, 0x0, 0x10, 0xa4, 0x1, 0xeb, 0x10 } } now that you no longer register nsPermission as a module, do you still need the CID declaration? (nsCookie probably has one too somewhere)
Comment on attachment 125729 [details] [diff] [review] v1 patch Assigning PRUint32 into a PRUint16 just seems like a bad idea.... Other than that, looks ok...
Attachment #125729 - Flags: superreview?(bzbarsky) → superreview-
bz: well, the acceptable set of values for each is {0,1,2}... they are flags on nsIPermissionManager. i think it is safe to assume undefined behavior if someone passes in a value that is so drastically out-of-range. that said, i agree with you in general... maybe we can convert to PRUint16 earlier (i.e., closer to the interface).
Comment on attachment 125729 [details] [diff] [review] v1 patch The permissions parts look ok to me. I am wondering if removing the possibility to create an nsICookie breaks the frozenness of it. (Not that is was anything like usefull...) on PRUint32: remember that the objects are short-lived, and are bloated with nsCSTring already, so don't go through too much pain :)
Attachment #125729 - Flags: review?(darin)
Attached patch v1.1Splinter Review
same as before, but puts back the PRUint32's in nsPermission.h, and also removes CID's for nsCookie/nsCookie2 and nsPermission. (the PRUint16 thing was a bit hackish, but it's not worth it to go change the callers too - like mvl said, these are short-lived and they're already bloated anyway. so I just reverted back to PRUint32).
Attachment #125729 - Attachment is obsolete: true
Attachment #125793 - Flags: superreview?(bzbarsky)
Attachment #125793 - Flags: review?(darin)
Attachment #125793 - Flags: superreview?(bzbarsky) → superreview+
Comment on attachment 125793 [details] [diff] [review] v1.1 r=darin
Attachment #125793 - Flags: review?(darin) → review+
i'll check this in with s/private/protected/ on the default ctors, per sicking's suggestion on irc. sicking will hopefully be landing a patch to nsDerivedSafe soon, that will remove the need for these hackish ctors - so i'll leave this bug open for now, and remove them when it lands.
Depends on: 209667
default ctors removed.
Status: NEW → RESOLVED
Closed: 22 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: