Closed
Bug 209506
Opened 22 years ago
Closed 22 years ago
[cookies] remove contractid's for nsICookie & nsIPermission
Categories
(Core :: Networking: Cookies, defect)
Core
Networking: Cookies
Tracking
()
RESOLVED
FIXED
People
(Reporter: dwitte, Assigned: dwitte)
References
Details
Attachments
(1 file, 1 obsolete file)
14.66 KB,
patch
|
darin.moz
:
review+
bzbarsky
:
superreview+
|
Details | Diff | Splinter Review |
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.
Assignee | ||
Updated•22 years ago
|
Hardware: DEC → All
Assignee | ||
Comment 1•22 years ago
|
||
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... ;)
Assignee | ||
Comment 2•22 years ago
|
||
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)
Assignee | ||
Comment 3•22 years ago
|
||
this patch saves 1.2k of codesize on linux btw, which is nice for such a simple
change.
Comment 4•22 years ago
|
||
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 5•22 years ago
|
||
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-
Comment 6•22 years ago
|
||
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 7•22 years ago
|
||
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 :)
Assignee | ||
Updated•22 years ago
|
Attachment #125729 -
Flags: review?(darin)
Assignee | ||
Comment 8•22 years ago
|
||
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
Assignee | ||
Updated•22 years ago
|
Attachment #125793 -
Flags: superreview?(bzbarsky)
Attachment #125793 -
Flags: review?(darin)
![]() |
||
Updated•22 years ago
|
Attachment #125793 -
Flags: superreview?(bzbarsky) → superreview+
Comment 9•22 years ago
|
||
Comment on attachment 125793 [details] [diff] [review]
v1.1
r=darin
Attachment #125793 -
Flags: review?(darin) → review+
Assignee | ||
Comment 10•22 years ago
|
||
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
Assignee | ||
Comment 11•22 years ago
|
||
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.
Description
•