Last Comment Bug 629875 - Handle negative arguments to drawImage
: Handle negative arguments to drawImage
Status: RESOLVED DUPLICATE of bug 655328
: dev-doc-complete, html5
Product: Core
Classification: Components
Component: Canvas: 2D (show other bugs)
: Trunk
: All All
: -- normal (vote)
: mozilla5
Assigned To: Nobody; OK to take it and work on it
:
: Milan Sreckovic [:milan]
Mentors:
http://www.whatwg.org/html/#dom-conte...
: 635380 974367 (view as bug list)
Depends on: 655328
Blocks: 622842 669366
  Show dependency treegraph
 
Reported: 2011-01-29 04:59 PST by :Ms2ger (⌚ UTC+1/+2)
Modified: 2015-09-11 12:34 PDT (History)
15 users (show)
ehsan: in‑testsuite+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
+
fixed
fixed


Attachments
Fix to pass 2d.drawImage.negativeXXX w3c tests (plus test_canvas fixes) (11.15 KB, patch)
2011-02-19 17:48 PST, Yury Delendik (:yury)
Ms2ger: feedback+
Details | Diff | Splinter Review
Fix to pass 2d.drawImage.negativeXXX w3c tests (plus test_canvas fixes) v2 (12.69 KB, patch)
2011-02-20 13:47 PST, Yury Delendik (:yury)
jmuizelaar: review+
Details | Diff | Splinter Review
Patch for checkin (12.87 KB, patch)
2011-03-27 07:15 PDT, :Ms2ger (⌚ UTC+1/+2)
no flags Details | Diff | Splinter Review
Backout while semantic issues are being resolved (12.76 KB, patch)
2011-05-20 13:01 PDT, Jeff Muizelaar [:jrmuizel]
bzbarsky: feedback+
dveditz: approval‑mozilla‑aurora+
dveditz: approval‑mozilla‑beta+
mounir: checkin+
Details | Diff | Splinter Review

Comment 1 David Humphrey (:humph) 2011-02-18 14:25:44 PST
*** Bug 635380 has been marked as a duplicate of this bug. ***
Comment 2 Yury Delendik (:yury) 2011-02-19 17:48:06 PST
Created attachment 513824 [details] [diff] [review]
Fix to pass 2d.drawImage.negativeXXX w3c tests (plus test_canvas fixes)

Fixes DrawImage arugments checks. Also, changes TODO's in test_canvas.html to regular checks, adds 2d.drawImage.negativedir
Comment 3 :Ms2ger (⌚ UTC+1/+2) 2011-02-20 07:13:01 PST
Comment on attachment 513824 [details] [diff] [review]
Fix to pass 2d.drawImage.negativeXXX w3c tests (plus test_canvas fixes)

>+    // Making sw, sh, dw and dh positive for 2d.drawImage.negativeXXX

How about

    // Handle negative sw, sh, dw and dh by flipping the rectangle over in the
    // relevant direction.

This patch fails 2d.drawImage.wrongtype, because that has a bogus assertion.
I filed <http://www.w3.org/Bugs/Public/show_bug.cgi?id=12142>. Please fix the
test in our tree.

Also, our version of 2d.voidreturn is out of date. Please replace

ok(ctx.drawImage(canvas, 0, 0, 0, 0, 0, 0, 0, 0) === undefined, "ctx.drawImage(canvas, 0, 0, 0, 0, 0, 0, 0, 0) === undefined");

with

ok(ctx.drawImage(canvas, 0, 0, 1, 1, 0, 0, 0, 0) === undefined, "ctx.drawImage(canvas, 0, 0, 1, 1, 0, 0, 0, 0) === undefined");

Beside that, looks good to me.
Comment 4 Yury Delendik (:yury) 2011-02-20 13:47:42 PST
Created attachment 513887 [details] [diff] [review]
Fix to pass 2d.drawImage.negativeXXX w3c tests (plus test_canvas fixes) v2

Addressed the "Handle negative sw, sh, dw and dh..."-comment and 2d.voidreturn items.

Ms2ger, I was not able to replicate the failure of the 2d.drawImage.wrongtype test. Fixing the test_canvas.html to the following causes the failure on my configuration.

var _thrown = undefined; try {
  ctx.drawImage(undefined, 0, 0);
} catch (e) { _thrown = e }; ok(_thrown && _thrown.code == DOMException.TYPE_MISMATCH_ERR, "should throw TYPE_MISMATCH_ERR");

That item was not addressed by the new patch.
Comment 5 :Ms2ger (⌚ UTC+1/+2) 2011-02-22 09:24:01 PST
Er, of course that wouldn't be your fault. Not sure where my head was.
Comment 6 Jeff Muizelaar [:jrmuizel] 2011-02-22 11:10:04 PST
What's the status of other browsers on these tests?
Comment 7 :Ms2ger (⌚ UTC+1/+2) 2011-02-22 13:54:06 PST
IE9 and Chrome pass.
Comment 8 Jeff Muizelaar [:jrmuizel] 2011-02-25 08:07:51 PST
Comment on attachment 513887 [details] [diff] [review]
Fix to pass 2d.drawImage.negativeXXX w3c tests (plus test_canvas fixes) v2

Thanks for looking into all of these cases and sorry for not getting to this review sooner.


> 
>+    if (sw == 0.0 || sh == 0.0) {
>+        // zero-sized source -- failure !?
>+        rv = NS_ERROR_DOM_INDEX_SIZE_ERR;
>+        goto FINISH;
>+    }

Does this change the behaviour for sw/sh == 0?
If so do we have tests for the new behaviour?

>     if (dw == 0.0 || dh == 0.0) {
>         rv = NS_OK;
>         // not really failure, but nothing to do --
>         // and noone likes a divide-by-zero
>         goto FINISH;
>     }
> 
>-    if (!FloatValidate(sx, sy, sw, sh) || !FloatValidate(dx, dy, dw, dh)) {
>+    // The following check might do the validation of the float arguments:
>+    //   (!FloatValidate(sx, sy, sw, sh) || !FloatValidate(dx, dy, dw, dh))
>+    // but we would also need to validate some sums for overflow (e.g. sx + sw).
>+    if (!FloatValidate(sx + sw, sy + sh, dx + dw, dy + dh)) {
>         rv = NS_ERROR_DOM_SYNTAX_ERR;
>         goto FINISH;
>     }
> 
>-    // check args
>-    if (sx < 0.0 || sy < 0.0 ||
>-        sw < 0.0 || sw > (double) imgSize.width ||
>-        sh < 0.0 || sh > (double) imgSize.height ||
>-        dw < 0.0 || dh < 0.0)
>-    {
>-        // XXX ERRMSG we need to report an error to developers here! (bug 329026)
>+    // Handle negative sw, sh, dw and dh by flipping the rectangle over in the
>+    // relevant direction.
>+    if (sw < 0.0) {
>+      sx += sw;
>+      sw = -sw;
>+    }
>+    if (sh < 0.0) {
>+      sy += sh;
>+      sh = -sh;
>+    }
>+    if (dw < 0.0) {
>+      dx += dw;
>+      dw = -dw;
>+    }
>+    if (dh < 0.0) {
>+      dy += dh;
>+      dh = -dh;
>+    }
>+
>+    // Checking source image boundaries.
>+    if (sx < 0 || sx + sw > (double) imgSize.width || 
>+        sy < 0 || sy + sh > (double) imgSize.height) {
>         rv = NS_ERROR_DOM_INDEX_SIZE_ERR;
>         goto FINISH;
>     }

This also looks like a behaviour change too?

> 
>+<!-- [[[ test_2d.drawImage.negativedir.html ]]] -->
>+
>+<p>Canvas test: 2d.drawImage.negativedir</p>
>+<canvas id="c117a" width="100" height="50"><p class="fallback">FAIL (fallback content)</p></canvas>
>+<script>


It looks like a bunch of different test changes are happening here. It might be easier to review if they were split into separate patches. Can you give a summary of the end result?
Comment 9 Yury Delendik (:yury) 2011-02-26 12:57:10 PST
> > 
> >+    if (sw == 0.0 || sh == 0.0) {
> >+        // zero-sized source -- failure !?
> >+        rv = NS_ERROR_DOM_INDEX_SIZE_ERR;
> >+        goto FINISH;
> >+    }
> 
> Does this change the behaviour for sw/sh == 0?
> If so do we have tests for the new behaviour?

This was a TODO in the test_canvas.html the test_2d_drawImage_outsidesource function and this particular function has modification that were made in scope of this bug. The modification was made to match the exiting w3c test files. See details below. It looks like this particular patch belong to the bug 629876 (I found out that later).

> 
> This also looks like a behaviour change too?
> 

As I understand content/canvas/test/test_canvas.html is combination of the w3c tests. Some of the checks marked as TODOs, some of them are outdated. I just changed the tests to match the w3c test referred in the bug description, also I reviewed all .drawImage() check/tests within test_canvas.html that a marked as TODO and converted those that are applicable to this bug into the normal checks.


> > 
> >+<!-- [[[ test_2d.drawImage.negativedir.html ]]] -->
> >+
> >+<p>Canvas test: 2d.drawImage.negativedir</p>
> >+<canvas id="c117a" width="100" height="50"><p class="fallback">FAIL (fallback content)</p></canvas>
> >+<script>
> 

This file/tested was not included in the test_canvas.html, but present in W3C tests and referred by this bug.

> 
> It looks like a bunch of different test changes are happening here. It might be
> easier to review if they were split into separate patches. Can you give a
> summary of the end result?

All test_canvas.html's TODOs, that were converted to normal checks, are passed with this patch. Shall I split the existing patch into two patches: for code and for tests?
Comment 10 Jeff Muizelaar [:jrmuizel] 2011-03-01 14:59:42 PST
No just splitting the tests changes out doesn't help. Splitting out the things like the behaviour change for bug 629876 would though. i.e. Ideally each behaviour change would be a separate patch. However, I'll try to take a look at your patch again tomorrow.
Comment 11 :Ms2ger (⌚ UTC+1/+2) 2011-03-27 07:15:18 PDT
Created attachment 522206 [details] [diff] [review]
Patch for checkin
Comment 12 Dão Gottwald [:dao] 2011-03-30 07:33:14 PDT
http://hg.mozilla.org/projects/cedar/rev/4ede7b9b55bc
Comment 14 :Ms2ger (⌚ UTC+1/+2) 2011-03-31 11:39:11 PDT
*** Bug 629876 has been marked as a duplicate of this bug. ***
Comment 15 Eric Shepherd [:sheppy] 2011-05-03 12:25:26 PDT
Note added to:

https://developer.mozilla.org/en/DOM/CanvasRenderingContext2D#Notes
Comment 16 Jeff Muizelaar [:jrmuizel] 2011-05-20 12:54:10 PDT
It looks like we're going to have to back this out well we resolve some semantic issues.
Comment 17 Jeff Muizelaar [:jrmuizel] 2011-05-20 13:01:46 PDT
Created attachment 534076 [details] [diff] [review]
Backout while semantic issues are being resolved
Comment 18 Daniel Veditz [:dveditz] 2011-05-23 15:23:03 PDT
Comment on attachment 534076 [details] [diff] [review]
Backout while semantic issues are being resolved

Approved for the mozilla-beta and mozilla-aurora repositories, a=dveditz for release-drivers
Comment 19 Asa Dotzler [:asa] 2011-05-31 18:09:43 PDT
I don't see this yet in mozilla-beta and time is running out for Firefox 5. Please land this ASAP. Thanks.
Comment 20 :Ehsan Akhgari 2011-06-01 13:04:49 PDT
Comment on attachment 534076 [details] [diff] [review]
Backout while semantic issues are being resolved

This patch does not apply cleanly to aurora, but it does apply cleanly on beta.
Comment 21 Jeff Muizelaar [:jrmuizel] 2011-06-02 09:15:43 PDT
This has been run on try and should be checked in with the following message:

Bug 629875. Backout 4ede7b9b55bc until semantics issues are resolved. f=bzbarsky, a=dveditz

We haven't decided what the behaviour for source rects larger than
the source image should be, and 4ede7b9b55bc breaks content like
20thingsilearned.com.
Comment 23 j.j. 2011-06-02 18:40:37 PDT
(In reply to comment #15)
> https://developer.mozilla.org/en/DOM/CanvasRenderingContext2D#Notes

Canceling needed due to backout
Comment 24 Joe Drew (not getting mail) 2011-07-22 12:43:19 PDT
I pushed this reversion and bug 669366 to aurora:

http://hg.mozilla.org/releases/mozilla-aurora/rev/20e76c984d82

and to inbound:

http://hg.mozilla.org/integration/mozilla-inbound/rev/b9ad0b9119b7

Reopening because we're not sure what we want to do here.
Comment 25 Marco Bonardo [::mak] 2011-07-25 06:00:24 PDT
http://hg.mozilla.org/mozilla-central/rev/b9ad0b9119b7

leaving open based on previous comment
Comment 26 Masatoshi Kimura [:emk] 2014-05-06 23:27:09 PDT
*** Bug 974367 has been marked as a duplicate of this bug. ***
Comment 27 Lee Salzman [:lsalzman] 2015-09-11 12:34:04 PDT

*** This bug has been marked as a duplicate of bug 655328 ***

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