Closed Bug 209506 Opened 21 years ago Closed 21 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: 21 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: