nsIAtom nsIAtomService nsIProperties

*** 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);

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?

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?

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?

Comment on attachment 94935 [details] [diff] [review] Freezes nsIProperties v.1 bugzilla fluff

Comment on attachment 94936 [details] [diff] [review] Freezes nsIProperties v.1 bugzilla fluff

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.

why even have a define method?

sounds good to me as long as there are no consumers depending on that situation :)

[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); };

This is another approach. Patch only for xpcom/ds without the getKey impl. thoughts?

/** * 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)

Shouldnt the comment for set be changed since it will now create the property if it doesn't exists since define was removed?

with suggestions.

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...) :)

I was thinking that: undefine == set(key, null); Converting to the new string classes... sure. I can do that. newer patch coming soon.

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?

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!)

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.

> + * 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);

nits fixed.

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() :))

if we add undefine, the question is: do you need to undefine before setting something that already exists.

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.

with timeless's and alec's comments.

Comment on attachment 95486 [details] [diff] [review] complete v.2 sr=alecf thanks for re-adding all that undefine code :)

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 on attachment 95486 [details] [diff] [review] complete v.2 r=ccarlen

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?

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) { ...... }

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.

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).

vrfy nsIProperties is frozen