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

RESOLVED FIXED

Status

()

Core
DOM
RESOLVED FIXED
6 years ago
6 years ago

People

(Reporter: sicking, Assigned: sicking)

Tracking

({dev-doc-complete})

Trunk
dev-doc-complete
Points:
---

Firefox Tracking Flags

(firefox5 fixed, blocking2.0 Macaw+, status2.0 .1-fixed)

Details

Attachments

(1 attachment)

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.
OS: Mac OS X → All
Hardware: x86 → All
blocking2.0: --- → ?
Created attachment 525718 [details] [diff] [review]
Patch to fix
Assignee: nobody → jonas
Attachment #525718 - Flags: review?(khuey)

Comment 2

6 years ago
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.
(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.
(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 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.
Attachment #525718 - Flags: review?(khuey) → review+
blocking2.0: ? → Macaw+
Comment on attachment 525718 [details] [diff] [review]
Patch to fix

Approved for the mozilla2.0 repository, a=dveditz for release-drivers
Attachment #525718 - Flags: approval2.0+
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
Status: NEW → RESOLVED
Last Resolved: 6 years ago
status2.0: --- → .1-fixed
Resolution: --- → FIXED
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).
Attachment #525718 - Flags: approval-mozilla-aurora?

Updated

6 years ago
Keywords: dev-doc-needed
Is this something that will ship in a Firefox 4.0.1 or something then?

Comment 10

6 years ago
Yes, it will ship in Macaw (which will be 4.0.1 unless a need to have a chemspill between then arises)
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?
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.
Documentation updated:

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

And mentioned on Firefox 5 for developers.
Keywords: dev-doc-needed → dev-doc-complete
Comment on attachment 525718 [details] [diff] [review]
Patch to fix

approved for mozilla-aurora, from triage meeting
Attachment #525718 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
This needs to land RSN now to make the ff5 train...
http://hg.mozilla.org/mozilla-aurora/rev/8c6e0aeb35ee

Checked in to aurora. Sorry for dropping the ball for so long.
status-firefox5: --- → fixed
You need to log in before you can comment on or make changes to this bug.