738910 - Use DataView in PropertyListUtils

Status: RESOLVED FIXED

(Toolkit :: General, defect)

Description

[Asaf Romano (gone)]

It seems bug 575688 will land in the next few days.  Once that's done, DataView can be used in PropretyListUtils, rather than the messy bits-inverting that is there now.

Comment 1

[Asaf Romano (gone)]

Attachment: Time to post that patch...

DataView has landed!

Notes:
1. This may help the performance of the safari migrator, but I didn't bother measuring.
2. test_propertyListUtils covers everything that needs to be tested with regard to this change, as far as I can tell.

Comment 2

[Marco Bonardo [:mak]]

Comment on attachment 621353 [details] [diff] [review]
Time to post that patch...

Review of attachment 621353 [details] [diff] [review]:
-----------------------------------------------------------------

This is a bit premature, the implementation bug is not even marked fixed, do we have information about this being shipped actually, or may happen the same with Map and Set (that were disabled in beta)?
While the patch is fine (apart a minor thing) I'm not sure we should rely on stuff that did not yet make a release.

::: toolkit/content/PropertyListUtils.jsm
@@ +285,2 @@
>    _readSignedInt64: function BPLR__readSignedInt64(aByteOffset) {
> +    let lo = this._dataView.getUint32(aByteOffset + 4, false);

is not the littleEndian argument optional per spec?

this is valid everywhere you used it.

Comment 3

[Asaf Romano (gone)]

Attachment: updated

I want this fixed before I go ahead with uint24 support in bug 1018679.

Comment 4

[Marco Bonardo [:mak]]

Comment on attachment 8433368 [details] [diff] [review]
updated

Review of attachment 8433368 [details] [diff] [review]:
-----------------------------------------------------------------

::: toolkit/modules/PropertyListUtils.jsm
@@ +585,5 @@
>        }
>  
>        case this.OBJECT_TYPE_BITS.DATA: {
>          let [offset, bytesCount] = this._readDataOffsetAndCount(objOffset);
> +        value = new Uint8Array(this._readUnsignedInts(offset, 1, bytesCount));

so, in this case we return a Uint8Array for Data, would it make more sense to return a DataView?

Comment 5

[Asaf Romano (gone)]

(In reply to Marco Bonardo [:mak] from comment #4)
> Comment on attachment 8433368 [details] [diff] [review]
> updated
> 
> Review of attachment 8433368 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: toolkit/modules/PropertyListUtils.jsm
> @@ +585,5 @@
> >        }
> >  
> >        case this.OBJECT_TYPE_BITS.DATA: {
> >          let [offset, bytesCount] = this._readDataOffsetAndCount(objOffset);
> > +        value = new Uint8Array(this._readUnsignedInts(offset, 1, bytesCount));
> 
> so, in this case we return a Uint8Array for Data, would it make more sense
> to return a DataView?

This is a "binary" data getter, so Uiny8Array makes sense, I think.

Comment 6

[Asaf Romano (gone)]

https://hg.mozilla.org/integration/fx-team/rev/18235d048be3

Comment 7

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/18235d048be3