Last Comment Bug 162114 - Freeze nsIProperties
: Freeze nsIProperties
Status: VERIFIED FIXED
:
Product: Core
Classification: Components
Component: XPCOM (show other bugs)
: Trunk
: x86 Windows XP
: -- normal (vote)
: ---
Assigned To: Doug Turner (:dougt)
: Scott Collins
: Nathan Froyd [:froydnj]
Mentors:
http://lxr.mozilla.org/seamonkey/sour...
: 162113 (view as bug list)
Depends on:
Blocks: 157137
  Show dependency treegraph
 
Reported: 2002-08-10 15:28 PDT by Chak Nanga
Modified: 2002-08-29 17:05 PDT (History)
5 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Freezes nsIProperties v.1 (2.66 KB, patch)
2002-08-12 09:55 PDT, Doug Turner (:dougt)
no flags Details | Diff | Splinter Review
Freezes nsIProperties v.1 (2.66 KB, patch)
2002-08-12 09:55 PDT, Doug Turner (:dougt)
no flags Details | Diff | Splinter Review
Freezes nsIProperties v.1 (2.66 KB, patch)
2002-08-12 09:57 PDT, Doug Turner (:dougt)
no flags Details | Diff | Splinter Review
Minor Cleanup xpcom/ds (6.08 KB, patch)
2002-08-13 12:03 PDT, Doug Turner (:dougt)
no flags Details | Diff | Splinter Review
patch for xpcom (8.14 KB, patch)
2002-08-13 13:05 PDT, Doug Turner (:dougt)
no flags Details | Diff | Splinter Review
takes alec's string conversion suggestion (xpcom only) (8.81 KB, patch)
2002-08-13 14:55 PDT, Doug Turner (:dougt)
no flags Details | Diff | Splinter Review
complete patch v.1 (12.10 KB, patch)
2002-08-14 10:53 PDT, Doug Turner (:dougt)
alecf: superreview+
Details | Diff | Splinter Review
complete v.2 (10.50 KB, patch)
2002-08-15 17:09 PDT, Doug Turner (:dougt)
ccarlen: review+
alecf: superreview+
Details | Diff | Splinter Review

Description Chak Nanga 2002-08-10 15:28:31 PDT
nsIAtom
nsIAtomService
nsIProperties
Comment 1 Bradley Baetz (:bbaetz) 2002-08-10 17:04:33 PDT
*** Bug 162113 has been marked as a duplicate of this bug. ***
Comment 2 timeless 2002-08-10 18:14:05 PDT
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);
Comment 3 Doug Turner (:dougt) 2002-08-12 09:55:01 PDT
Created attachment 94935 [details] [diff] [review]
Freezes nsIProperties v.1

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 4 Doug Turner (:dougt) 2002-08-12 09:55:11 PDT
Created attachment 94936 [details] [diff] [review]
Freezes nsIProperties v.1

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 5 Doug Turner (:dougt) 2002-08-12 09:57:01 PDT
Created attachment 94937 [details] [diff] [review]
Freezes nsIProperties v.1

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 6 Doug Turner (:dougt) 2002-08-12 16:08:06 PDT
Comment on attachment 94935 [details] [diff] [review]
Freezes nsIProperties v.1

bugzilla fluff
Comment 7 Doug Turner (:dougt) 2002-08-12 16:08:21 PDT
Comment on attachment 94936 [details] [diff] [review]
Freezes nsIProperties v.1

bugzilla fluff
Comment 8 Alec Flett 2002-08-13 11:06:27 PDT
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.
Comment 9 Doug Turner (:dougt) 2002-08-13 11:30:14 PDT
why even have a define method?
Comment 10 Alec Flett 2002-08-13 11:43:30 PDT
sounds good to me as long as there are no consumers depending on that situation :)
Comment 11 Doug Turner (:dougt) 2002-08-13 11:54:20 PDT
[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);
};
Comment 12 Doug Turner (:dougt) 2002-08-13 12:03:30 PDT
Created attachment 95143 [details] [diff] [review]
Minor Cleanup xpcom/ds

This is another approach. 

Patch only for xpcom/ds without the getKey impl.  thoughts?
Comment 13 timeless 2002-08-13 12:30:25 PDT
    /**
     * 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 Mike Kaply [:mkaply] 2002-08-13 12:52:46 PDT
Shouldnt the comment for set be changed since it will now create the property if
it doesn't exists since define was removed?
Comment 15 Doug Turner (:dougt) 2002-08-13 13:05:42 PDT
Created attachment 95150 [details] [diff] [review]
patch for xpcom

with suggestions.
Comment 16 Alec Flett 2002-08-13 14:29:36 PDT
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...) :)
Comment 17 Doug Turner (:dougt) 2002-08-13 14:43:25 PDT
I was thinking that:
  undefine == set(key, null);

Converting to the new string classes... sure.  I can do that.  newer patch
coming soon.
Comment 18 Doug Turner (:dougt) 2002-08-13 14:55:36 PDT
Created attachment 95166 [details] [diff] [review]
takes alec's string conversion suggestion (xpcom only)
Comment 19 Doug Turner (:dougt) 2002-08-13 16:26:07 PDT
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 Alec Flett 2002-08-14 10:23:44 PDT
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!)
Comment 21 Doug Turner (:dougt) 2002-08-14 10:53:34 PDT
Created attachment 95274 [details] [diff] [review]
complete patch v.1

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.
Comment 22 timeless 2002-08-14 13:10:17 PDT
> +     * 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);
Comment 23 Doug Turner (:dougt) 2002-08-14 13:11:52 PDT
nits fixed.
Comment 24 Alec Flett 2002-08-15 12:59:09 PDT
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() :))
Comment 25 Doug Turner (:dougt) 2002-08-15 15:38:45 PDT
if we add undefine, the question is: do you need to undefine before setting
something that already exists.  
Comment 26 Alec Flett 2002-08-15 15:49:45 PDT
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.
Comment 27 Doug Turner (:dougt) 2002-08-15 17:09:10 PDT
Created attachment 95486 [details] [diff] [review]
complete v.2

with timeless's and alec's comments.
Comment 28 Alec Flett 2002-08-15 17:19:37 PDT
Comment on attachment 95486 [details] [diff] [review]
complete v.2

sr=alecf
thanks for re-adding all that undefine code :)
Comment 29 Conrad Carlen (not reading bugmail) 2002-08-15 19:30:33 PDT
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 Conrad Carlen (not reading bugmail) 2002-08-26 12:58:48 PDT
Comment on attachment 95486 [details] [diff] [review]
complete v.2

r=ccarlen
Comment 31 Doug Turner (:dougt) 2002-08-26 13:49:17 PDT
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?
Comment 32 Chak Nanga 2002-08-28 17:15:14 PDT
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)
{
  ......
}
Comment 33 Doug Turner (:dougt) 2002-08-28 17:53:30 PDT
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.  

Comment 34 Doug Turner (:dougt) 2002-08-29 13:24:06 PDT
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).
Comment 35 timeless 2002-08-29 17:05:11 PDT
vrfy nsIProperties is frozen

Note You need to log in before you can comment on or make changes to this bug.