Closed
Bug 162114
Opened 22 years ago
Closed 22 years ago
Freeze nsIProperties
Categories
(Core :: XPCOM, defect)
Tracking
()
VERIFIED
FIXED
People
(Reporter: chak, Assigned: dougt)
References
()
Details
Attachments
(1 file, 7 obsolete files)
10.50 KB,
patch
|
ccarlen
:
review+
alecf
:
superreview+
|
Details | Diff | Splinter Review |
nsIAtom
nsIAtomService
nsIProperties
Comment 1•22 years ago
|
||
*** Bug 162113 has been marked as a duplicate of this bug. ***
void nsIProperties::undefine ( in string prop )
Undefines a property.
Returns:
NS_ERROR_FAILURE if a property with that name doesn't already exist.
Remove "already"
the other two things you are trying to freeze don't properly register in unstable.
before you freeze them, could you please find out why they don't register
correctly and perhaps endeavor to fix the problem?
Not Found
The requested URL /mozilla/build/latest/mozilla/xpcom/dox/class_nsIAtom.html was
not found on this server.
Apache/1.3.22 Server at unstable.elemental.com Port 80
Not Found
The requested URL
/mozilla/build/latest/mozilla/xpcom/dox/class_nsIAtomService.html was not found
on this server.
Apache/1.3.22 Server at unstable.elemental.com Port 80
59 [scriptable, uuid(e5d0d92b-ea45-4622-ab48-302baf2094ee)]
60 interface nsIAtomService : nsISupports {
This is an interesting piece of "documentation":
62 /**
63 * Version of NS_NewAtom that doesn't require linking against the
64 * XPCOM library. See nsIAtom.idl.
65 */
66 nsIAtom getAtom(in wstring value);
So is this:
68 /**
69 * Version of NS_NewPermanentAtom that doesn't require linking against
70 * the XPCOM library. See nsIAtom.idl.
71 */
72 nsIAtom getPermanentAtom(in wstring value);
73 };
40 interface nsIAtom : nsISupports
41 {
what does this mean: (esp stringbuf)
42 /**
43 * Translate the unicode string into the stringbuf.
44 */
45 [noscript] AString toString();
normally scriptable creatures begin with lowercase letters (someone from
embedding recently tried to correct an interface which broke this rule.
unfortunately the interface was already frozen.):
47 /**
48 * Return a pointer to a zero terminated unicode string.
49 */
50 void GetUnicode([shared, retval] out wstring aResult);
If atoms are supposed to be unique then shouldn't it be "Find _the_ atom" ...
(occurs three times)?
77 /**
78 * Find an atom that matches the given ISO-Latin1 C string. The
79 * C string is translated into its unicode equivalent.
80 */
81 extern NS_COM nsIAtom* NS_NewAtom(const char* isolatin1);
Are you freezing this function:
112 /**
113 * Return a count of the total number of atoms currently
114 * alive in the system.
115 */
If so could the comment be changed to "Return the total number of atoms"...
(this next bit is a suggestion, i'm not quite sure why you used the wording you
did:) "created by XPCOM in this session"
116 extern NS_COM nsrefcnt NS_GetNumberOfAtoms(void);
Assignee | ||
Comment 3•22 years ago
|
||
This effectively freezes the interface as is. My only concern is that there is
no way to enumerate the properties. Maybe we should add an nsISimpleEnumerator
attribute?
Assignee | ||
Comment 4•22 years ago
|
||
This effectively freezes the interface as is. My only concern is that there is
no way to enumerate the properties. Maybe we should add an nsISimpleEnumerator
attribute?
Assignee | ||
Comment 5•22 years ago
|
||
This effectively freezes the interface as is. My only concern is that there is
no way to enumerate the properties. Maybe we should add an nsISimpleEnumerator
attribute?
Assignee | ||
Comment 6•22 years ago
|
||
Comment on attachment 94935 [details] [diff] [review]
Freezes nsIProperties v.1
bugzilla fluff
Attachment #94935 -
Attachment is obsolete: true
Assignee | ||
Comment 7•22 years ago
|
||
Comment on attachment 94936 [details] [diff] [review]
Freezes nsIProperties v.1
bugzilla fluff
Attachment #94936 -
Attachment is obsolete: true
Comment 8•22 years ago
|
||
interesting. So the only thing I'm looking at is the whole "set" vs. "define"
thing - seems like there should be a universal "replace" - i.e. one requires the
property to already exist, and the other requires that the property NOT exist.
What if you don't care if the property already exists or not?
otherwise you'd have to write lots of glue code like
function setProperty(properties, key, value) {
if (properties.has(key))
properties.set(key, value);
else
properties.define(key, value);
}
enumeration will be tricky though - because you'd need to get the Key and Value
of each element in the enumeration..
how about either:
void getKeys(out long count, [size_is(count), retval, array] string keys);
which gives you an array of keys, and then the caller could call get() on each key?
or,
nsISimpleEnumerator enumerateKeys();
which returned an enumerator of a bunch of nsISupportsCString objects which
contained the keys.. this seems much bloatier to me though, I guess I'd prefer
the getKeys option.
Assignee | ||
Comment 9•22 years ago
|
||
why even have a define method?
Comment 10•22 years ago
|
||
sounds good to me as long as there are no consumers depending on that situation :)
Assignee | ||
Comment 11•22 years ago
|
||
[scriptable, uuid(xx)]
interface nsIProperties : nsISupports
{
/**
* Gets a property with a given name.
* @return NS_ERROR_FAILURE if a property with that name doesn't
* exist.
*/
void get(in string prop, in nsIIDRef uuid,
[iid_is(uuid),retval] out nsQIResult result);
/**
* Sets a property with a given name to a given value.
* @return NS_ERROR_FAILURE if a property with that name doesn't
* exist.
*/
void set(in string prop, in nsISupports value);
/**
* Returns true if the property with the given name exists.
*/
boolean has(in string prop);
/**
* Returns an array of keys.
*/
void getKeys(out long count, [size_is(count), retval, array] string keys);
};
Assignee | ||
Comment 12•22 years ago
|
||
This is another approach.
Patch only for xpcom/ds without the getKey impl. thoughts?
Comment 13•22 years ago
|
||
/**
* Gets a property with a given name.
* @return NS_ERROR_FAILURE if a property with that name doesn't
* exist.
*/
void get(in string prop, in nsIIDRef uuid,
[iid_is(uuid),retval] out nsQIResult result);
should we return an NS_ERROR_FAILURE if the property fails the QI? (it might
make sense to have different failure values for not found and bad qi)
Comment 14•22 years ago
|
||
Shouldnt the comment for set be changed since it will now create the property if
it doesn't exists since define was removed?
Assignee | ||
Comment 15•22 years ago
|
||
with suggestions.
Attachment #95143 -
Attachment is obsolete: true
Comment 16•22 years ago
|
||
I think undefine() has value - no need to remove that...other than that looks
good...getKeys shouldn't be too hard to implement but obviously not incredibly
important right now.
I don't suppose there's any chance of converting to the new string classes (ah,
blue sky...) :)
Assignee | ||
Comment 17•22 years ago
|
||
I was thinking that:
undefine == set(key, null);
Converting to the new string classes... sure. I can do that. newer patch
coming soon.
Assignee | ||
Comment 18•22 years ago
|
||
Attachment #95150 -
Attachment is obsolete: true
Assignee | ||
Comment 19•22 years ago
|
||
the only concern, probably minor, is that there is a common pattern which may
break embedders:
nsCOMPtr<nsIProperties> directoryService =
do_GetService(NS_DIRECTORY_SERVICE_CONTRACTID, &rv);
NS_ENSURE_SUCCESS(rv,rv);
nsCOMPtr<nsIFile> defaultsDir;
rv = directoryService->Get(someCharStar, <------------------
NS_GET_IID(nsIFile),
getter_AddRefs(defaultsDir));
Furthermore, I think that this kinda breaks with other frozen interfaces such as
the directory services:
[scriptable, uuid(bbf8cab0-d43a-11d3-8cc2-00609792278c)]
interface nsIDirectoryServiceProvider: nsISupports
{
<snip>
nsIFile getFile(in string prop, out PRBool persistent);
};
Switching over creates alot of nsDependentCString classes.
I have a patch for the tree which does convert everything to the new strings,
but I am not sure that is the best thing to do since most of these strings are
just keys. Alec?
Comment 20•22 years ago
|
||
ok cool - it was worth the investigation I guess :)
as for the whole undefine thing, it sounds good but make sure to describe that
in the comments for the interface (cuz it wasn't obvious to me!)
Assignee | ||
Comment 21•22 years ago
|
||
This is the diff of the entire tree. It does not include the string conversion
per my last comment. The changes are simple. However I did not implement
GetKeys.
Attachment #95166 -
Attachment is obsolete: true
Comment 22•22 years ago
|
||
> + * Sets a property with a given name to a given value. To removed a property
change "removed" to "remove"
> + * you must pass nsnull as the value.
in idl we use generics (null) instead of arbitrary language forms (nsnull) ;-)
> */
void set(in string prop, in nsISupports value);
> + * Returns an array of keys.
you probably should change "keys" to "the keys" to indicate it's an array of all
of the keys and not just some of them.
> + */
> + void getKeys(out PRUint32 count, [array, size_is(count), retval] out
string keys);
Assignee | ||
Comment 23•22 years ago
|
||
nits fixed.
Comment 24•22 years ago
|
||
Comment on attachment 95274 [details] [diff] [review]
complete patch v.1
I was thinking, this interface is simple enough that I think that undefine()
(or remove() or delete() or something) is worth keeping in the interface,
instead of relying on the semantics of set()
its a syntax vs. semantics thing, and in this case I think its warranted: the
interface is generic enough that other people might go implement this
themselves (i.e. reusing the interface) but might not reimplement the
set("foo", nsnull) case correctly. Consumers of the interface might also not
realize there is a way to remove keys, simply because they don't RTFM.
Anyway, my point is that I don't see the harm in keeping undefine() or
something similar around and I think it makes the use of it a little clearer.
sr=alecf if you keep that method around, and file a new bug on implementing
getKeys() (cc me on that too, you never know, I might just implement it myself
in exchange for you keeping undefine() :))
Attachment #95274 -
Flags: superreview+
Assignee | ||
Comment 25•22 years ago
|
||
if we add undefine, the question is: do you need to undefine before setting
something that already exists.
Comment 26•22 years ago
|
||
nope! undefine is just there as a way to explictly remove an item - it just
seems counter intuitive to me to use set() to un-set something :) - there are
complex interfaces where yet-another-method seems like overkill, but this
interface is pretty darn simple and so I think its worth keeping.
Assignee | ||
Comment 27•22 years ago
|
||
with timeless's and alec's comments.
Attachment #94937 -
Attachment is obsolete: true
Attachment #95274 -
Attachment is obsolete: true
Comment 28•22 years ago
|
||
Comment on attachment 95486 [details] [diff] [review]
complete v.2
sr=alecf
thanks for re-adding all that undefine code :)
Attachment #95486 -
Flags: superreview+
Comment 29•22 years ago
|
||
I admit, I initially liked using set(prop, null) instead of undefine(prop). I
take it back. In order to undefine something, if you say set(prop, null), that
would have to create an entry for prop and assign null to it if it did not
exist. I don't think that's a desired effect. undefine(prop) should just cause
any trace of prop to cease to exist. Along those lines, should undefine() ever
return failure if what it's asked to undefine doesn't exist? Consider: "delete
NULL;"
Comment 30•22 years ago
|
||
Comment on attachment 95486 [details] [diff] [review]
complete v.2
r=ccarlen
Attachment #95486 -
Flags: review+
Assignee | ||
Comment 31•22 years ago
|
||
patch 95486 checked in. Now what are we going to do about this atom business.
why is there a requirement to freeze the nsIAtom interfaces? chak?
Reporter | ||
Comment 32•22 years ago
|
||
Hmmm..i'm not really sure why i added the nsIAtomService to the freeze list.
nsIAtom showed up in the freeze list since a major embeddor is using this
interface in their code and we do not want to break them the next time if this
interface changes.
Here's how nsIAtom interface's being used in their code:
Usage Scenario 1:
-----------------
nsIFrame *aFrame; //Passed in as a function param
nsCOMPtr<nsIContent> pContent;
aFrame->GetContent(getter_AddRefs(pContent));
if (pContent)
{
// make sure that this is the HTML or BODY element
nsCOMPtr<nsIAtom> tag;
pContent->GetTag(*(getter_AddRefs(tag)));
if (tag)
{
if (tag.get() == nsHTMLAtoms::html || ....
}
}
Usage Scenario 2:
-----------------
nsCOMPtr<nsIFrame> frame;
nsCOMPtr<nsIAtom> parentType;
frame->GetFrameType(getter_AddRefs(parentType));
if (parentType.get() == nsLayoutAtoms::canvasFrame)
{
......
}
Assignee | ||
Comment 33•22 years ago
|
||
Lets not do this. first of all, nsIContent isn't in an IDL. Your going to have
to do some work to make it scriptable. Secondly, atoms have all sorts of
problems if we ever start proxying objects. Ever worse problems in scripts.
Lastly, this usage depends on some concrete class (at least in your example)
which has pointer values of all known atoms. Ick.
Here is what I propose: For the publically consumed interface, DONT use the
nsIAtom's Instead, use strings. This Internally to Gecko, use nsIAtom.
Assignee | ||
Comment 34•22 years ago
|
||
The nsIAtom and the nsIAtomService will not be frozen. There are better ways to
do what you are trying to do. Marking FIXED (since I did fix the nsIProperties).
Status: NEW → RESOLVED
Closed: 22 years ago
Resolution: --- → FIXED
Comment 35•22 years ago
|
||
vrfy nsIProperties is frozen
Status: RESOLVED → VERIFIED
Summary: Freeze additional XPCOM interfaces(nsIAtom, nsIAtomService, ...) → Freeze nsIProperties
You need to log in
before you can comment on or make changes to this bug.
Description
•