Darin Fisher Reporter
	Description • 20 years ago

Create nsIProperties2

I'd like to make all necko channels implement a generic interface for
associating named values.  The idea is to make it easy for consumers to add meta
data to a channel, and also to make it easier to pass new information to necko
and expose new information from necko.

To solve bug 265135, we made several necko channels support nsIProperties. 
However, to make the approach more useful, I feel that nsIProperties could be
improved.

In particular, I think it'd make sense to expose simple getters and setters for
basic types like: int, int64, AUTF8String, etc.  That way, it is not necessary
to allocate "nsISupportsPrimitives" when needing to store simple types.

It'd also be nice if there was a |clone| method to make things like this easier:
http://lxr.mozilla.org/mozilla/source/netwerk/protocol/http/src/nsHttpChannel.cpp#1853

Here's a proposal for nsIPropertie2:

[scriptable, uuid(...)]
interface nsIProperties2 : nsIProperties
{
  PRInt32     getInt         (in string prop);
  PRInt64     getInt64       (in string prop);
  double      getDouble      (in string prop);
  AString     getAString     (in string prop);
  ACString    getACString    (in string prop);
  AUTF8String getAUTF8String (in string prop);

  void        setInt         (in string prop, in PRInt32 value);
  void        setInt64       (in string prop, in PRInt64 value);
  void        setDouble      (in string prop, in double value);
  void        setAString     (in string prop, in AString value);
  void        setACString    (in string prop, in ACString value);
  void        setAUTF8String (in string prop, in AUTF8String value);

  const PRUint32 TYPE_INTERFACE   = 0;
  const PRUint32 TYPE_INT         = 1;
  const PRUint32 TYPE_INT64       = 2;
  const PRUint32 TYPE_DOUBLE      = 3;
  const PRUint32 TYPE_ASTRING     = 4;
  const PRUint32 TYPE_ACSTRING    = 5;
  const PRUint32 TYPE_AUTF8STRING = 6;

  PRUint32 getType(in string prop);

  nsIProperties2 clone();
};

Thoughts?

	Christian :Biesinger (don't email me, ping me on IRC) Assignee
	Comment 1 • 20 years ago

  const PRUint32 TYPE_INTERFACE   = 0;

why this? you don't have an interface getter/setter function...

is there a need for the ACString method? I.e. can we get by with always storing
UTF8 or UTF16?

	Darin Fisher Reporter
	Comment 2 • 20 years ago

> why this? you don't have an interface getter/setter function...

nsIProperties2 extends nsIProperties! :)


> is there a need for the ACString method? I.e. can we get by with always storing
> UTF8 or UTF16?

Maybe.  I wasn't sure.  It could be useful in some cases.

	Mike Shaver (:shaver -- probably not reading bugmail closely)
	Comment 3 • 20 years ago

What about using nsIVariant here?

	Darin Fisher Reporter
	Comment 4 • 20 years ago

because then i have to allocate a nsIVariant for each value that i wish to
return.  that's no better than using the supports primitives.  the proposed
interface is like nsIVariant except that the getters and setters take a string
valued key, so there is no need to allocate a temporary object to contain the
primitive value.

i was thinking though that for primitive values, a support primitive could be
allocated if needed to return the value via nsIProperties::get.

	Christian :Biesinger (don't email me, ping me on IRC) Assignee
	Comment 5 • 20 years ago

-> me

	Christian :Biesinger (don't email me, ping me on IRC) Assignee
	Comment 7 • 20 years ago

ok... nsIPropertyBag2 as discussed on IRC. has no clone() method yet. this is
just the interface and an implementation in nsHashPropertyBag.cpp.

	Darin Fisher Reporter
	Comment 8 • 20 years ago

Comment on attachment 175246 [details] [diff] [review]
patch

>Index: nsIPropertyBag.idl

>+[scriptable, uuid(bc4f7805-fa89-462c-8c72-36a772c86aa8)]
>+interface nsIPropertyBag2 : nsIPropertyBag

each interface file should live in its own IDL file, especially
if we are going to be freezing these interfaces.  it is a bug
that nsIWritablePropertyBag is declared in this same IDL file.


this is my suggested interface, but i'm going to still tear it
apart:


>+{
>+  PRInt32     getInt         (in AString prop);
>+  PRInt64     getInt64       (in AString prop);
>+  double      getDouble      (in AString prop);
>+  AString     getAString     (in AString prop);
>+  ACString    getACString    (in AString prop);
>+  AUTF8String getAUTF8String (in AString prop);

we should probably cover the entire range of primitive types
to be consistent with nsISupportsPrimitives and nsIVariant, 
though nsIVariant seems to provide more types than we really
need (more than nsISupportsPrimitives at least).

also, i'd prefer if these methods were named like this:

  getPropertyAsInt32
  getPropertyAsInt64
  ...

i know it is a bit more verbose, but that's ok i think.  being
clear and consistent with "getProperty" that returns variant
seems good to me.


>+  void        setInt         (in AString prop, in PRInt32 value);
>+  void        setInt64       (in AString prop, in PRInt64 value);
>+  void        setDouble      (in AString prop, in double value);
>+  void        setAString     (in AString prop, in AString value);
>+  void        setACString    (in AString prop, in ACString value);
>+  void        setAUTF8String (in AString prop, in AUTF8String value);

these should probably live on nsIWritablePropertyBag2.


>+
>+  /**
>+   * The type of the specified property. The values correspond to the
>+   * nsIDataType interface.
>+   */
>+  PRUint16 getType(in AString prop);

so, this is a shortcut to nsIVariant::dataType.  it feels like a good
thing to have, but i'm not sure when we'd need it.  or rather, i'm 
not sure when we'd desire the optimization of not having to go through
the nsIVariant interface.

	Christian :Biesinger (don't email me, ping me on IRC) Assignee
	Comment 9 • 20 years ago

- put each interface in its own file
- nsIWritablePropertyBag2
- new functions for unsigned integers and interfaces

- export nsHashPropertyBag from xpcom, so other modules can have classes
inheriting from it

	Christian :Biesinger (don't email me, ping me on IRC) Assignee
	Comment 10 • 20 years ago

Comment on attachment 175379 [details] [diff] [review]
patch v2

this patch doesn't quite work... when I try to get a property when no property
is set, I crash (uninitialized hashtable)

	Christian :Biesinger (don't email me, ping me on IRC) Assignee
	Comment 11 • 20 years ago

- add an Init() function which initializes the hashtable, and call it from the
NS_New... function as well as the factory constructor.

- forgot to mention: the previous patch removed the type attribute too... seems
to me like it won't be that commonly used, so getting the variant and reading
its type seems ok

	Christian :Biesinger (don't email me, ping me on IRC) Assignee
	Comment 12 • 20 years ago

(that patch also removes the littered checks whether the hashtable is
initialized, since Init now takes care of that)

	Christian :Biesinger (don't email me, ping me on IRC) Assignee
	Comment 13 • 20 years ago

forgot this in the previous patch... with the file split, other files need to
include the new files (and also with the replacement of #include with
forward-decls)

Should I have put the extensions/webservices include changes all in
wspprivate.h instead of in the various files? (in xpconnect I had to, since
xpcprivate.h contains a class decl inheriting from nsIProperty)

	Christian :Biesinger (don't email me, ping me on IRC) Assignee
	Comment 14 • 20 years ago

Comment on attachment 175383 [details] [diff] [review]
patch v3

oops. this is missing a rather critical line.

	Christian :Biesinger (don't email me, ping me on IRC) Assignee
	Comment 16 • 20 years ago

Comment on attachment 175451 [details] [diff] [review]
patch v4

oops, this patch crashes on getting a null interface value

	Christian :Biesinger (don't email me, ping me on IRC) Assignee
	Comment 17 • 20 years ago

return null/NS_OK for null interface pointers

	Darin Fisher Reporter
	Comment 18 • 20 years ago

if i get a prop as a variant, and then i modify the prop using one of the new
methods (e.g., setPropertyAsUint32), then will that new value be reflected as
the value of the variant?  should it?  in either case, we'll need to nail that
behavior down before we freeze these interfaces.

	Christian :Biesinger (don't email me, ping me on IRC) Assignee
	Comment 19 • 20 years ago

you mean this?
js> y=x.getProperty("foo");
bar
js> x.setProperty("foo", 42);
js> y
bar

so no... that doesn't update the variant. I think that's a good thing... it
means that the value of the variant does not change unexpectedly for the caller.
what do you think? 

(doing getProperty and changing that variant probably will modify the property,
but that was always the case. I'd try that out, but xpconnect won't give me the
nsIVariant...)

	Benjamin Smedberg
	Comment 20 • 20 years ago

I think that setting the simple getter should definitely modify the variant.
That would make reflecting this interface directly into a JS object much more
useful.

	Christian :Biesinger (don't email me, ping me on IRC) Assignee
	Comment 21 • 20 years ago

bsmedberg: you mean if I call setPropertyAsInt32("foo", 42) and then call
getProperty("foo")? this patch does give you 42 for that code.

	Mike Shaver (:shaver -- probably not reading bugmail closely)
	Comment 22 • 20 years ago

Updating the variant won't help JS, because we'll have unwrapped it when we
first returned it.

	Darin Fisher Reporter
	Comment 23 • 20 years ago

I am interested in being able to optimize the implementation of nsPropertyBag to
avoid allocating new nsVariant objects when an existing property is modified. 
It'd be nice if we could just modify the value of the stored nsVariant object
directly when setPropertyAsInt32 is called.  Otherwise, we have to assume that
the nsVariant has already been given out via getProperty, and we would be forced
to allocate a new nsVariant object to satisfy the setPropertyAsInt32 function. 
Make sense?  So, I think we should document that the variant objects returned
from getProperty are "live" in the DOM sense :-)

	Darin Fisher Reporter
	Comment 24 • 20 years ago

If JS unwraps the nsIVariant right away then that's fine.  We can and should
document that behavior as well.

	Mike Shaver (:shaver -- probably not reading bugmail closely)
	Comment 25 • 20 years ago

I don't think that we should document them as live, or as dead.  Either choice
constrains implementations (like necko?) which want to optimize for the
non-variant API case, in cooperation with their dominant callers.  I think we
should document that the liveness of the nsIVariant returned is not guaranteed,
and that callers should not set the variant's contents directly.  We could even
hand back a neutered nsIVariant that threw on set, if we wanted to.

(Shades of the clone/isMutable wars we fought a few months back in
mozilla/calendar/base!)

	Darin Fisher Reporter
	Comment 26 • 20 years ago

Requiring consumers to not make assumptions about that behavior is fine by me. 
I just want to make sure that we document it well in either case.  The last
thing I want to see happen is a bunch of code get written to undocumented
assumptions.  I also don't want to prevent optimization due to such undocumented
yet defacto/prolific API requirements :-)

	Darin Fisher Reporter
	Comment 27 • 20 years ago

I think it might be good to add a getPropertyAsBool.  I expect that some people
might argue that we should exactly reflect the entire set of nsIVariant::getAs
methods, but I for one am fine with the reduced set.  However, that said...
there isn't that much overhead in making it more comprehensive since we'll have
a shared implementation.  Thoughts?

	Mike Shaver (:shaver -- probably not reading bugmail closely)
	Comment 28 • 20 years ago

Pick one liveness behaviour for DEBUG and another for non-DEBUG, and I bet
people will depend on the behaviour less.  (You could also split Unix/Win, or
even do it randomly!)

	Christian :Biesinger (don't email me, ping me on IRC) Assignee
	Comment 29 • 20 years ago

Comment on attachment 175457 [details] [diff] [review]
patch v5

I'll make a patch that uses operator new to allocate the variants, instead of
CI

	Christian :Biesinger (don't email me, ping me on IRC) Assignee
	Comment 30 • 20 years ago

I'm not doing the full set of nsIVariant values here. I don't feel it's worth
all the code. people can always get the variant and access the value that way.

	Mike Shaver (:shaver -- probably not reading bugmail closely)
	Comment 31 • 20 years ago

Comment on attachment 177020 [details] [diff] [review]
patch v6

I was going to ask questions about threadsafety issues with GetWeak, but I
suppose the NS_NewHashPropertyBag comment does lay out the risk.  I wonder
where else that warning should be, if anywhere.

r=shaver, sorry for the wait.

	Mike Shaver (:shaver -- probably not reading bugmail closely)
	Comment 32 • 20 years ago

Comment on attachment 175435 [details] [diff] [review]
fix other files

r=shaver

	Christian :Biesinger (don't email me, ping me on IRC) Assignee
	Comment 33 • 20 years ago

checked in. if anyone wants the full range of nsIVariant types available from
nsIPropertyBag2, feel free to file a new bug (don't assign to me).

	Stelian Pop
	Comment 34 • 20 years ago

For some reason, this patch breaks (at least) sunbird.

Sunbird does (in calIItemBase.js) something like:
       this.mProperties = Components.classes["@mozilla.org/hash-property-bag;1"].
                     createInstance(Components.interfaces.nsIWritablePropertyBag);
and later:
       this.mProperties.setProperty("DESCRIPTION", "nonono");

The previous line causes sunbird to crash with:
  ###!!! ASSERTION: nsTHashtable was not initialized properly.:  
'mTable.entrySize'                                                   , file  
nsTHashtable.h, line 179
  Break: at file nsTHashtable.h, line 179

Tracing it a bit shows that
xpcom/ds/nsHashPropertyBag.cpp:NS_NewHashPropertyBag() is never called, meaning
that Init() is never called, and mPropertyHash.Init() is not called either.

Then nsHashPropertyBag::SetProperty() calls mPropertyHash.Put() which triggers
the assert.

I don't know xpcom enough to debug this issue, but it seems like there are
problems in the mapping contractid -> constructor...

	Christian :Biesinger (don't email me, ping me on IRC) Assignee
	Comment 35 • 20 years ago

yeah, I suck... forgot to include xpcom/build in the patch. anyway, fixed by
this checkin:
2005-03-23 09:13	vladimir%pobox.com 	mozilla/ xpcom/ build/ nsXPComInit.cpp 
1.208 	1/1  	Change HashPropertyBag instance constructor to call Init, r=shaver