Last Comment Bug 649672 - File.slice has different semantics from Array.slice and String.slice
: File.slice has different semantics from Array.slice and String.slice
Status: RESOLVED FIXED
: dev-doc-complete
Product: Core
Classification: Components
Component: DOM (show other bugs)
: Trunk
: All All
: -- normal (vote)
: ---
Assigned To: Jonas Sicking (:sicking) PTO Until July 5th
:
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2011-04-13 09:54 PDT by Jonas Sicking (:sicking) PTO Until July 5th
Modified: 2011-05-11 14:29 PDT (History)
13 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
fixed
Macaw+
.1-fixed


Attachments
Patch to fix (17.29 KB, patch)
2011-04-13 10:26 PDT, Jonas Sicking (:sicking) PTO Until July 5th
khuey: review+
dbaron: approval‑mozilla‑aurora+
dveditz: approval2.0+
Details | Diff | Splinter Review

Description Jonas Sicking (:sicking) PTO Until July 5th 2011-04-13 09:54:25 PDT
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) PTO Until July 5th 2011-04-13 10:26:54 PDT
Created attachment 525718 [details] [diff] [review]
Patch to fix
Comment 2 Paul Rouget [:paul] 2011-04-13 10:33:03 PDT
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) 2011-04-13 10:37:16 PDT
(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) 2011-04-13 10:38:01 PDT
(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 [:khuey] (khuey@mozilla.com) 2011-04-13 10:51:37 PDT
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] 2011-04-13 11:06:10 PDT
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) PTO Until July 5th 2011-04-13 14:57:53 PDT
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) PTO Until July 5th 2011-04-13 15:00:08 PDT
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] 2011-04-14 11:21:17 PDT
Is this something that will ship in a Firefox 4.0.1 or something then?
Comment 10 christian 2011-04-14 11:27:37 PDT
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] 2011-04-14 12:24:56 PDT
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) PTO Until July 5th 2011-04-14 12:50:52 PDT
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] 2011-04-14 14:11:56 PDT
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+2 (review requests must explain patch) 2011-04-19 15:59:30 PDT
Comment on attachment 525718 [details] [diff] [review]
Patch to fix

approved for mozilla-aurora, from triage meeting
Comment 15 Johnathan Nightingale [:johnath] 2011-05-11 11:33:47 PDT
This needs to land RSN now to make the ff5 train...
Comment 16 Jonas Sicking (:sicking) PTO Until July 5th 2011-05-11 14:29:52 PDT
http://hg.mozilla.org/mozilla-aurora/rev/8c6e0aeb35ee

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

Note You need to log in before you can comment on or make changes to this bug.