When we read the downloads.json storage we read the known fields but discard unknown ones, which means that when the file is serialized again to disk, those fields will be gone. It's important that current versions do not discard data (even if it's unknown to them) in order to allow future versions to add new fields and not break when a user goes back and forth between different version using the same profile.

I'm marking this as tracking release, this is not strictly a blocker but more of a nice-to-have for forward compatibility. This should also count as an add-on extensibility feature, to attach serializable data to Download objects.

I worked on a partial patch for this which I'll finish up.

Comment on attachment 800840 [details] [diff] [review] preserveprops The approach looks good to me, I'd also be interested in what Felipe thinks since he worked on a similar feature for the Add-ons back-end. Can we also use this as a mechanism that can be used by add-ons or other Mozilla products to store their own properties that get serialized? Something like calling the "unknownProperties" property with the name "data", or extraData or something along the line. I wonder if there might be a case where we _don't_ want a property we serialize in a newer version to be kept after the profile has been used in a previous version? In any case, as an alternative, we might also break compatibility completely if we introduce a backwards-incompatible feature.

Comment on attachment 800840 [details] [diff] [review] preserveprops Review of attachment 800840 [details] [diff] [review]: ----------------------------------------------------------------- Looks good to me too. Maintaining the list of known properties will be a bit of a pain. To deal with that what we did something in the AddonRepository work was to use the Obj's prototype to get the list of known properties. I don't know how doable it would be to do here.. those objects though were mostly data and the functions were all prepended with "_", so we used it to skip any property or functions that we didn't want serialized. In any case, I wonder if it's possible to write a test that would prevent us from adding new properties and forgetting to update the known properties list and it ending up in the unknwn properties

(In reply to :Felipe Gomes from comment #5) > In any case, I wonder if it's possible to write a test that would prevent us > from adding new properties and forgetting to update the known properties > list and it ending up in the unknwn properties It won't end up in the unknown properties list. New properties just won't be serialized.

Comment on attachment 800840 [details] [diff] [review] preserveprops Review of attachment 800840 [details] [diff] [review]: ----------------------------------------------------------------- After thinking more about it, I concluded that this mechanism should be used only internally for future unknown properties. An add-on extensibility mechanism, if required, should be implemented as a separate key-value store with an API that prevents large data from being saved in the JSON file, similarly to the session store mechanism. To indicate that the object shouldn't be used externally, we should make the name private by prefixing it with an underscore, like "_unknownProperties". With regard to setting arbitrary properties using "createDownload", I'm not sure, but we could just allow it for better compatibility in case an add-on is used on multiple versions of the browser. This patch should be rebased on top of bug 906134, where we fixed the "startTime" serialization. ::: toolkit/components/jsdownloads/test/unit/test_DownloadStore.js @@ +274,5 @@ > + do_check_eq(itemsForLoad[2].target.unknownProperties.target2, "download3target2"); > + > + do_check_eq(Object.keys(itemsForLoad[2].saver.unknownProperties).length, 2); > + do_check_eq(itemsForLoad[2].saver.unknownProperties.saver1, "download3saver1"); > + do_check_eq(itemsForLoad[2].saver.unknownProperties.saver2, "download3saver2"); nit: please indent to stay within 80 characters.