322831 - Extra parameters in properties not preserved

Status: RESOLVED FIXED

(Calendar :: Internal Components, defect)

Description

[Joey Minta]

If we try to roundtrip something like
LOCATION;X-LOCATION-ID=[e-mail]:Delta Center

the X-LOCATION-ID part is lost.  It comes out as
LOCATION:Delta Center

Comment 1

[Dan Mosedale (:dmosedale, :dmose)]

Right now, all properties are essentially objects implementing nsIVariant.  In JS, we're essentially handing around properties as regular objects of various types (both native JS types and JS interface implementors) and letting XPConnect do the conversion to nsIVariant.  

I think a possible approach to fix this is make it so that property objects that have X- parameters also implement nsIPropertyBag in addition to nsIVariant.  Initially we could do this by creating JS wrappers that do this only for properties that arrive from the ICS service with X- params so that they can be preserved.  Arbitrary creation/editing/adding of such things could come later.  One possible issue with this approach is performance.

Comment 2

[Joey Minta]

(In reply to comment #1)
> I think a possible approach to fix this is make it so that property objects
> that have X- parameters also implement nsIPropertyBag in addition to
> nsIVariant.  
Wouldn't this approach require us to fix all consumers of properties to QI to get the regular property value?  My thought was to keep a simple behind-the-scenes js-object of objects, which we can keep throwing properties+parameters at, something like:

propObject[propName][paramName] = paramValue;

Then we can add methods to calItemBase
  AString getPropertyParam(in AString aPropName, in AString aParamName);
  void setPropertyParam(in AString aPropName, in AString aParamName, in AString aNewValue);

This has the advantage of not creating a new interface and not requiring us to change our consumers.  Since params will be found/used relatively rarely, I think the perf impact will also be less.  Thoughts?

Comment 3

[Reed Loden [:reed]]

The bugspam monkeys have struck again. They are currently chewing on default assignees for Calendar. Be afraid for your sanity!

Comment 4

[Joey Minta]

Attachment: using js-objects

No one seemed to object to the js-object proposal, so I've gone ahead and implemented it.  As far as I can tell, there's no need to really expose these values through an interface, so simply keeping them around behind the scenes should be sufficient.

Comment 5

[Matthew (lilmatt) Willis]

Comment on attachment 233174 [details] [diff] [review]
using js-objects

Index: calendar/base/src/calItemBase.js
+dump("checking property:"+prop.propertyName+'\n');


Let's lose the dump before checkin.

Otherwise, this appears to work!

r=lilmatt

Comment 6

[Dan Mosedale (:dmosedale, :dmose)]

Comment on attachment 233174 [details] [diff] [review]
using js-objects

An elegant architectural choice because of its simplicity; nice work.

A couple of minor things:

It is my belief that basic manipulation of stuff descended from calIItemBase is (at least almost) entirely scriptable via any XPIDL accessible language.  Given how trivial it is to maintain that invariant, I'd like to propose that we do so.

>+     this.mPropertyParams[iprop.name][paramName]

In most (all?) uses of the above pattern in this patch, the only dereference (out of the four being made) that is actually interesting to the reader is the very last one (paramName).  This suggests that the code would be significantly easier to read with judicious use of JS 1.7's let statement.

Comment 7

[Stefan Sitter [:ssitter]]

(In reply to comment #6)
> This suggests that the code would be significantly
> easier to read with judicious use of JS 1.7's let statement.

(Without knowing what the JS 1.7's let statement does): We should ensure that the code also works with Lightning in Thunderbird 1.5 aka 1.8.0 branch.

Comment 8

[Joey Minta]

Attachment: extra vars and idl

Patch updated to include IDL access to the params and extra variable declarations to avoid too much [] notation.

Comment 9

[Dan Mosedale (:dmosedale, :dmose)]

Comment on attachment 235335 [details] [diff] [review]
extra vars and idl

r=dmose; nice work.  Please file another bug for implementing other relevant parameter methods (enumerate, exists(?), set).

Comment 10

[Matthew (lilmatt) Willis]

Patch checked in on MOZILLA_1_8_BRANCH and trunk.

-> FIXED