649672 - File.slice has different semantics from Array.slice and String.slice

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, defect)

Description

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

File.slice takes start+length, Array.slice and String.slice takes start+end.

The proposed way to fix it for now is to remove the unprefixed function and add a prefixed function which uses the correct semantics. If we're quick about this we can hopefully get chrome and opera to follow.

A few months down the road we should be able to unprefix with the correct semantics.

Comment 1

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Attachment: Patch to fix

Comment 2

[Paul Rouget [:paul]]

So we will break the websites that uses .slice(), right?
I don't think there's a lot, but just want to make sure we spread the word.

Comment 3

[Mounir Lamouri (:mounir)]

(In reply to comment #2)
> So we will break the websites that uses .slice(), right?
> I don't think there's a lot, but just want to make sure we spread the word.

If a website use .slice with no feature detection, yes. But they shouldn't. Given that Firefox has .slice since a few weeks, that should be very common.

Comment 4

[Mounir Lamouri (:mounir)]

(In reply to comment #3)
> Given that Firefox has .slice since a few weeks, that should be very common.

... shouldn't be very common.

Comment 5

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Comment on attachment 525718 [details] [diff] [review]
Patch to fix

>@@ -240,44 +241,80 @@ nsDOMFile::GetType(nsAString &aType)
>     AppendUTF8toUTF16(mimeType, mContentType);
>   }
> 
>   aType = mContentType;
> 
>   return NS_OK;
> }
> 
>-// Makes sure that aStart and aStart + aLength is less then or equal to aSize
>+// Makes sure that aStart and aEnd is less then or equal to aSize and greater
>+// than 0
> void
>-ClampToSize(PRUint64 aSize, PRUint64& aStart, PRUint64& aLength)
>+ClampToSize(PRInt64 aSize, PRInt64& aStart, PRInt64& aEnd)

Now that we're handling negative indices, lets call this "ParseSize" or something similar.  Update the comment too.

> {
>-  if (aStart > aSize) {
>-    aStart = aLength = 0;
>+  CheckedInt64 newStartOffset = aStart;
>+  if (aStart < -aSize) {
>+    newStartOffset = 0;
>   }
>-  CheckedUint64 endOffset = aStart;
>-  endOffset += aLength;
>-  if (!endOffset.valid() || endOffset.value() > aSize) {
>-    aLength = aSize - aStart;
>+  else if (aStart < 0) {
>+    newStartOffset += aSize;
>+  }
>+  else if (aStart > aSize) {
>+    newStartOffset = aSize;
>+  }
>+  CheckedInt64 newEndOffset = aEnd;
>+  if (aEnd < -aSize) {
>+    newEndOffset = 0;
>+  }
>+  else if (aEnd < 0) {
>+    newEndOffset += aSize;
>+  }
>+  else if (aEnd > aSize) {
>+    newEndOffset = aSize;
>+  }
>+
>+  if (!newStartOffset.valid() || !newEndOffset.valid() ||
>+      newStartOffset.value() >= newEndOffset.value()) {
>+    aStart = aEnd = 0;
>+  }
>+  else {
>+    aStart = newStartOffset.value();
>+    aEnd = newEndOffset.value();
>   }
> }

It took me a minute or two, but I believe the logic here and in the rest of the patch is correct.  Test it thoroughly please :-)

r=me with that.

Comment 6

[Daniel Veditz [:dveditz]]

Comment on attachment 525718 [details] [diff] [review]
Patch to fix

Approved for the mozilla2.0 repository, a=dveditz for release-drivers

Comment 7

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Checked in on 2.0 and m-c:

http://hg.mozilla.org/mozilla-central/rev/dae833f4d934
http://hg.mozilla.org/releases/mozilla-2.0/rev/dbbd7e5541cf

Comment 8

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Comment on attachment 525718 [details] [diff] [review]
Patch to fix

Would like to fix this on aurora for parity with FF4. This will help us fix a bad spec issue which was found late in the game (after FF4 shipped and aurora branched).

Comment 9

[Eric Shepherd [:sheppy]]

Is this something that will ship in a Firefox 4.0.1 or something then?

Comment 10

[LegNeato]

Yes, it will ship in Macaw (which will be 4.0.1 unless a need to have a chemspill between then arises)

Comment 11

[Eric Shepherd [:sheppy]]

So this is removing blob.slice() (and by extension File.slice()) and replacing it with mozSlice() with updated syntax, if I'm reading this code right?

Comment 12

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Yes, that is correct.

It also makes the second argument optional (defaulting to "end of the file") as well as allows negative indexes.

If an index is negative it's treated as an offset from the end.

Comment 13

[Eric Shepherd [:sheppy]]

Documentation updated:

https://developer.mozilla.org/en/DOM/Blob
https://developer.mozilla.org/en/DOM/File

And mentioned on Firefox 5 for developers.

Comment 14

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 525718 [details] [diff] [review]
Patch to fix

approved for mozilla-aurora, from triage meeting

Comment 15

[Johnathan Nightingale [:johnath]]

This needs to land RSN now to make the ff5 train...

Comment 16

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

http://hg.mozilla.org/mozilla-aurora/rev/8c6e0aeb35ee

Checked in to aurora. Sorry for dropping the ball for so long.