Closed Bug 738910 Opened 8 years ago Closed 6 years ago

Use DataView in PropertyListUtils

Categories

(Toolkit :: General, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla33

People

(Reporter: mano, Assigned: mano)

References

Details

Attachments

(1 file, 1 obsolete file)

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.
Attached patch Time to post that patch... (obsolete) — Splinter Review
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.
Attachment #621353 - Flags: review?(mak77)
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.
Attachment #621353 - Flags: review?(mak77)
Attached patch updatedSplinter Review
I want this fixed before I go ahead with uint24 support in bug 1018679.
Attachment #621353 - Attachment is obsolete: true
Attachment #8433368 - Flags: review?(mak77)
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?
Attachment #8433368 - Flags: review?(mak77) → review+
(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.
https://hg.mozilla.org/mozilla-central/rev/18235d048be3
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla33
You need to log in before you can comment on or make changes to this bug.