Closed Bug 162114 Opened 18 years ago Closed 18 years ago

Freeze nsIProperties

Categories

(Core :: XPCOM, defect)

x86
Windows XP
defect
Not set
normal

Tracking

()

VERIFIED FIXED

People

(Reporter: chak, Assigned: dougt)

References

()

Details

Attachments

(1 file, 7 obsolete files)

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);
Attached patch Freezes nsIProperties v.1 (obsolete) — Splinter Review
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?
Attached patch Freezes nsIProperties v.1 (obsolete) — Splinter Review
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?
Attached patch Freezes nsIProperties v.1 (obsolete) — Splinter Review
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
Attachment #94935 - Attachment is obsolete: true
Comment on attachment 94936 [details] [diff] [review]
Freezes nsIProperties v.1

bugzilla fluff
Attachment #94936 - Attachment is obsolete: true
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);
};
Attached patch Minor Cleanup xpcom/ds (obsolete) — Splinter Review
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?
Attached patch patch for xpcom (obsolete) — Splinter Review
with suggestions.
Attachment #95143 - Attachment is obsolete: true
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.
Attachment #95150 - Attachment is obsolete: true
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!)
Attached patch complete patch v.1 (obsolete) — Splinter Review
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
> +     * 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.
Blocks: 157137
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+
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.
Attached patch complete v.2Splinter Review
with timeless's and alec's comments.
Attachment #94937 - Attachment is obsolete: true
Attachment #95274 - Attachment is obsolete: true
Comment on attachment 95486 [details] [diff] [review]
complete v.2

sr=alecf
thanks for re-adding all that undefine code :)
Attachment #95486 - Flags: superreview+
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
Attachment #95486 - Flags: review+
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).
Status: NEW → RESOLVED
Closed: 18 years ago
Resolution: --- → FIXED
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.