266085 - nsIWritablePropertyBag interface

Status: RESOLVED FIXED

(Core :: XPCOM, defect)

Description

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

Following up on the comment in xpcom/ds/nsIPropertyBag.idl to add a
nsIWritablePropertyBag interface

Comment 1

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

Attachment: 266085-nsiwritablepropertybag-0.patch

nsIWritablePropertyBag impl.  I don't see a reason for a nsIWritableProperty --
getProperty already takes a name and returns a variant (as opposed to a
nsIProperty), so set can do the same thing.  nsIProperty can continue to be
used for iterating over existing properties in a PropertyBag.

Comment 2

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

Attachment: 266085-nsiwritablepropertybag-1.patch

Er, it helps to be able to delete properties.

1) Should it be deleteProperty or removeProperty?  (The latter would be
analogous to setAttribute/getAttribute/remoteAttribute, but I don't think that
the analogy makes sense.)
2) Should setProperty take a flag indicating whether it should overwrite, or
whether it should throw a NAME_ERROR if the property with the given name
already exists?  (I'm thinking no.)

Comment 3

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

Attachment: 266085-hashpropertybag.patch

Implementation of WritablePropertyBag using a hash table as the storage.  Note
that the enumerator causes a huge mess; alternative implementation ideas
appreciated.  (I think I actually had one, but didn't get around to doing it..)

Comment 4

[Mike Shaver (:shaver emeritus)]

Comment on attachment 165351 [details] [diff] [review]
266085-hashpropertybag.patch

I don't think nsTHashTable is frozen, we could make DeleteProperty do what you
crave, at possibly some cost to the rest of the callers.  Another entry point
would be a second option.

Comment 5

[Darin Fisher]

Comment on attachment 163394 [details] [diff] [review]
266085-nsiwritablepropertybag-1.patch

>Index: nsIPropertyBag.idl

>+    /**
>+     * Set a property with the given name to the given value.  If
>+     * a property already exists with the given name, it is
>+     * overwritten.
>+     */
>+    nsIVariant setProperty(in AString name, in nsIVariant value);

What does this return?

Comment 6

[Darin Fisher]

Comment on attachment 165351 [details] [diff] [review]
266085-hashpropertybag.patch

>Index: nsHashPropertyBag.h

>+    // nsISupports interface
>+    NS_DECL_ISUPPORTS
>+
>+    // nsIPropertyBag interface
>+    NS_DECL_NSIPROPERTYBAG
>+
>+    // nsIWritablePropertyBag interface
>+    NS_DECL_NSIWRITABLEPROPERTYBAG

I'm not a big fan of these comments myself since it's pretty obvious
what NS_DECL_NSIFOO means.


>+    // a hash table of string -> nsIVariant, used for
>+    // lookups

nit: maybe put this comment on one line?


>+    // this is used for iterating.  Note that mPropertyValueArray DOES
>+    // NOT hold a ref to the nsIVariant; the only refs are held by the
>+    // hash table above.

Will this cause problems if the enumerator is used after the
nsIPropertyBag has been destroyed?


>+extern "C" nsresult
>+NS_NewHashPropertyBag(nsIWritablePropertyBag* *_retval);

This won't be exported on Windows.  You need to declare it like this:

  extern "C" NS_COM nsresult
  NS_NewHashPropertyBag(nsIWritablePropertyBag* *_retval);

And, then you need to add a caller to xpcom/build/dlldeps.cpp


Should this class be available via the component manager for use from
javascript?


>Index: nsHashPropertyBag.cpp

>+nsHashPropertyBag::nsHashPropertyBag()
>+{
>+    mPropertyHash.Init();

And if Init() should fail?

Should nsIWritablePropertyBag have a parameter to specify
the expected number of elements so that you can pass that
value to the hashtable's init method?



>+    nsIVariant *value;
>+    PRBool isFound = mPropertyHash.Get(name, &value);
>+    if (!isFound)
>+        return NS_ERROR_FAILURE;
>+
>+    NS_IF_ADDREF(*_retval = value);
>+
>+    return NS_OK;
>+}

This leaks value!  Maybe just pass _retval to .Get()


>+        PRInt32 oldIndex = mPropertyNameArray.IndexOf(name);

It might be nice to avoid generating this list until we need it, 
but this is fine.


>+        NS_ASSERTION(mPropertyHash.Count() == mPropertyNameArray.Count(), "Number of hash entries != name array entries");
>+        NS_ASSERTION(mPropertyNameArray.Count() == mPropertyValueArray.Count(), "Number of name array entries != number of value array entries");

nit: please wrap these to 80 columns.


>+    nsSimpleProperty *prop = new nsSimpleProperty (

maybe we should have a nsIPropertyEnumerator instead:

  interface nsIPropertyEnumerator : nsISupports
  {
    boolean hasMoreElements();
    nsIVariant getNext(out AString name);
  };

That would save on the extra nsSimpleProperty crap.  Of course you could
also make this nsIPropertyVisitor (see for example the other visitor
interfaces in necko), then you could greatly simplify the implementation
of this class.	Is the enumerator pattern really better than the visitor
pattern?

Comment 7

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

Attachment: 266085-hash-property-bag-1.patch

Obsoleting both previous patches, as I got rid of the nsIVariant return from
setProperty (was intended to return the same variant that was passed in, but I
realize that that's not that useful).

I changed the HashPropertyBag to just build an array on-demand for the
enumerator; this greatly simplified the code, and also ensures that the
enumerator stays valid at all times.

Also added contractid's and other build bits to make win32 and script happy.

Comment 8

[Darin Fisher]

Comment on attachment 166799 [details] [diff] [review]
266085-hash-property-bag-1.patch

>Index: ds/nsIPropertyBag.idl

>+     * @return NS_ERROR_FAILURE if a property with that name doesn't
>+     * exist.
>+     */
>+    void deleteProperty(in AString name);

This comment is wrong since the method clearly doesn't return
a value.  Use @throws instead ;-)

nsIPropertyBag has the same problem, so if you could fix those
too that'd be even better.


>Index: ds/nsHashPropertyBag.cpp

>+nsHashPropertyBag::nsHashPropertyBag()
>+{
>+}

Just put this in the header file perhaps?  Then maybe it'll
just be inlined into NS_NewHashPropertyBag.


>+NS_IMPL_ISUPPORTS2(nsHashPropertyBag, nsIWritablePropertyBag, nsIPropertyBag)

It might be good to mention in the header file that
NS_NewHashPropertyBag returns a non-threadsafe impl.


>+    // is it too much to ask for ns*Hashtable to return
>+    // a boolean indicating whether RemoveEntry succeeded
>+    // or not?!?!
>+    PRBool isFound = mPropertyHash.Get(name, nsnull);
>+    if (!isFound)
>+        return NS_ERROR_FAILURE;

please file a bug if you have not done so already.


r=darin

Comment 9

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

Attachment: 266085-hash-property-bag-2.patch

Updated with Darin's comments and also to apply cleanly to cvs.

Filed bug 271343 for RemoveElement issue.

Comment 10

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

Comment on attachment 166835 [details] [diff] [review]
266085-hash-property-bag-2.patch

Carrying over darin's r=.

Comment 11

[[e-mail]]

It would be nice if you could do
aWritablePropertyBag[foo] = bar;
delete aWritablePropertyBag[foo];
as per foo = aPropertyBag[bar];

Comment 12

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

(In reply to comment #11)
> It would be nice if you could do
> aWritablePropertyBag[foo] = bar;
> delete aWritablePropertyBag[foo];
> as per foo = aPropertyBag[bar];

In JS?  It's on my todo list, though if you get a chance, file a bug on me to
remind me.

This landed today, so -> Fixed