Build Identifier: http://hg.mozilla.org/mozilla-central/rev/88fdbd974f82 Mozilla/5.0 (Windows NT 6.1; WOW64; rv:6.0a1) Gecko/20110506 Firefox/6.0a1 ID:20110506030557 Only entry page is registered in the popup, No entry is registered for each page. Should I filed a bug? Reproducible: Always Steps to Reproduc: 1. Start Minefield with new profile 2. Open URL ( http://www.20thingsilearned.com/ ) in New Tab 3. Click forward button at right side of page 4. Click forward button at right side of page 5. Try, Right ckick on Back/Forward button Actual Results: No entry in Back/Forward button Expected Results: Each Page should be registered in the Back/Forward menu popup Regression window: Works, Entry of each page are registered in the back/Forward popup http://hg.mozilla.org/mozilla-central/rev/6c1a5f4eb350 Mozilla/5.0 (Windows NT 6.1; WOW64; rv:2.2a1pre) Gecko/20110329 Firefox/4.2a1pre ID:20110330085433 Fails, Only front page is registered in the back/Forward popup http://hg.mozilla.org/mozilla-central/rev/422bbd8245a7 Mozilla/5.0 (Windows NT 6.1; WOW64; rv:2.2a1pre) Gecko/20110331 Firefox/4.2a1pre ID:20110331030432 Pushlog: http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=6c1a5f4eb350&tochange=422bbd8245a7

I'm having trouble reproducing, or understanding what's going wrong. Mozilla/5.0 (Macintosh; Intel Mac OS X 10.6; rv:6.0a1) Gecko/20110506 Firefox/6.0a1 I click the page's forward button twice. That takes me to [1]. Now when I right-click on the back button, it displays two entries, as expected. [1] http://www.20thingsilearned.com/foreword/2

It happens on http://hg.mozilla.org/mozilla-central/rev/88fdbd974f82 Mozilla/5.0 (X11; Linux i686; rv:6.0a1) Gecko/20110506 Firefox/6.0a1 ID:20110506030557 (In reply to comment #1) > I'm having trouble reproducing, or understanding what's going wrong. > > Mozilla/5.0 (Macintosh; Intel Mac OS X 10.6; rv:6.0a1) Gecko/20110506 > Firefox/6.0a1 > > I click the page's forward button twice. That takes me to [1]. > > Now when I right-click on the back button, it displays two entries, as > expected. > > [1] http://www.20thingsilearned.com/foreword/2 Page 1 is never registered. And repeat step 2-5 few times(close resume popup, if any), the entry is never registered.

So the problem is that you clicked forward twice but only got two history entries instead of three? If so, I don't think that's a bug -- the page appears to be calling history.replaceState the first time you click forward. Try it in Chrome; it does the same thing.

In Google Chrome 13.0.756.0 canary, It works as I _expected_. When I click foreward button of the page twice. Then, 2 entries are registered. Page 1 Front page so I click Back button og the browser Page 1 appears and click Back button og the browser again Front page appears

s/og/of/g

Oh, huh, I can reproduce what you're seeing now. That page is doing some strange things. Sometimes when I visit it doesn't update the URI on every page change, but sometimes it does. The problem is that the first time you click the right arrow, it doesn't register a new history entry, nor does it change the URI. This leads me to suspect that it's calling replaceState or somesuch. But maybe it's our bug! Do you want to investigate and create a smaller testcase?

(In reply to comment #6) > Oh, huh, I can reproduce what you're seeing now. > > That page is doing some strange things. Sometimes when I visit it doesn't > update the URI on every page change, but sometimes it does. > > The problem is that the first time you click the right arrow, it doesn't > register a new history entry, nor does it change the URI. This leads me to > suspect that it's calling replaceState or somesuch. But maybe it's our bug! > > Do you want to investigate and create a smaller testcase? Thanks, I will discover regression bug by bisection. However It is difficult to create test case because the site is very complicated for me.

FWIW, I'm not convinced that this will show us something useful, since the page's behavior changes as the UA and capabilities of the browser changes.

s/this/bisecting

In local build(from ceder repository) build from cbb7ffa045d3 : fails build from db8e95fbcd39 : fails build from a3b18039e32f : fails build from 4ede7b9b55bc : fails build from e83560952624 : works build from 8053d7723f6e : works build from bc9f0b32325a : works Triggered by: 4ede7b9b55bc Yury — Bug 629875 - Handle negative arguments to drawImage; r=jmuizelaar And I also confirm on m-c that cset 4ede7b9b55bc trigger the problem. In local build(from m-c repository) build from 62941612320d : fails build from 62941612320d and backed out 4ede7b9b55bc : works CC'ing Ms2ger jrmuizel yury

Thanks, Alice! CC'ing Paul Irish Paul, I suspect someone is calling drawImage with zero sw or sh. Could you bug the right people?

Over to the right component. Thanks for your work!

The FF6 returns exception when drawImage is called with image 830x520 and the source region is (809.756104, 0.000000, 20.243902, 520.000000). The 809.756104 + 20.243902 equals 830.000006, which is outside the image boundaries. Chrome, Opera and Safari are okay with that and do not raise any exception, which is wrong from the specification point of view IMHO

Sounds like we might need to fix the spec

> 830.000006, which is outside the image boundaries. Yes, but I bet WebKit and Opera are doing their usual "round to integer" thing an then it works... And yes, it sounds like this needs a spec fix. It also sounds like we may need to back out bug 629875 on Aurora if we don't come up with an Aurora-appropriate fix for this.... We need to track this for Firefox 5.

(In reply to comment #17) > Yes, but I bet WebKit and Opera are doing their usual "round to integer" > thing an then it works... Not exactly. The mochitests (including http://w3c-test.org/html/tests/submission/PhilipTaylor/canvas/2d.drawImage.outsidesource.html) fail if the boundary components rounded to integer. However if those rounded to float, the mochitests and the attached test pass. > > And yes, it sounds like this needs a spec fix. The spec (and the Philip Taylor's test) is clear about usage of the boundary components as double precision numbers. Currently, the Chrome passes the 2d.drawImage.outsidesource test.

> fail if the boundary components rounded to integer Hmm. OK.... Then why does the minimal testcase here not fail in Chrome, exactly?

Looking at http://codesearch.google.com/codesearch/p?hl=en#OAMlx_jo-ck/src/third_party/WebKit/Source/WebCore/html/canvas/CanvasRenderingContext2D.cpp&q=drawImage&exact_package=chromium&l=1337 , Chrome is using float in the canvas context functions implementations: void CanvasRenderingContext2D::drawImage(HTMLCanvasElement* canvas, float sx, float sy, float sw, float sh, float dx, float dy, float dw, float dh, ExceptionCode& ec);

We use float there too. That's just the API. It doesn't say what happens to the numbers once they're passed in. Again, why is Chrome not throwing in the attached minimal testcase?

(In reply to comment #22) > We use float there too. That's just the API. It doesn't say what happens > to the numbers once they're passed in. Good point. Changing doubles to floats in the DrawImage function allows mochitests and the attached minimal test case pass. The DrawImage was the last user of the FloatValidate with double arguments. Those functions were changed to have arguments as floats to avoid unnecessary implicit casts.

Comment on attachment 530942 [details] [diff] [review] Changing doubles to floats inthe DrawImage and FloatValidate functions Looks sane to me.

Comment on attachment 530942 [details] [diff] [review] Changing doubles to floats inthe DrawImage and FloatValidate functions This seems ok to me as a quick-fix but: 1) This needs to be escalated to the spec. 2) There needs to be a less-fragile solution here, imo. I'll do the spec escalation, I guess.

Actually... no. I don't see where the spec says to throw if the source rect is not contained inside the image. Instead it seems to say to fill the part outside the image with image edge pixels: If the original image data is a bitmap image, the value painted at a point in the destination rectangle is computed by filtering the original image data. The user agent may use any filtering algorithm (for example bilinear interpolation or nearest-neighbor). When the filtering algorithm requires a pixel value from outside the original image data, it must instead use the value from the nearest edge pixel. (That is, the filter uses 'clamp-to-edge' behavior.) Am I just missing something here?

Looking at the historical specification ".. If the source rectangle is not entirely within the source image, or if one of the sw or sh arguments is zero, the implementation must raise an INDEX_SIZE_ERR exception..." and the code before bug 629875 : - // 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) So reverting back the rev 4ede7b9b55bc may not give the desired result for other web sites. Also, up-to-date specification makes Philip Taylor's 2d.drawImage.outsidesource incorrect. The patch fixes the code to conform the new specification and it's keeping the double to float change from the previous patch. Also, it's marking as todo the 2d.drawImage.outsidesource section of the test_canvas.html.

> and it's keeping the double to float change from the previous patch Why do we want that change? Does it have observable effects? (I'm not trying to rag on you here; I really appreciate your looking into this. I just want to understand the implications of the changes we're making, and I don't know this code that well.)

Since the spec does not require check on the source boundary rectangle, so I remove the that check. (Which caused failures on 2d.drawImage.outsidesource tests, had to adjust test_canvas) The code below, after that check, uses translate/scale matrix for the gfxPattern, so I count on the Clip to not display stuff that off the source boundary rectangle. Visible effects: attached test case and succeeded checks in the test_canvas. Do you think it worth to add pixel check tests?

> so I remove the that check. Yes, that part sounds correct to me. What I'm not sure is why we want the change from double to float.

In the DrawImage body, the gfxMatrix and other surface methods use gfxRect type in arguments, so it does not make sense to keep the doubles: - in DrawImage, to convert from float to double and then back to float just to perform simple arithmetic operations; - in FloatValidate, after changing double to float in DrawImage, there is no users of FloatValidate with double arguments, but there are a lot of users of FloatValidate with float arguments. That removes unnecessary casts in other functions calls.

OK. So the change to double is just for internal code cleanliness and has no observable behavior effects on web content?

Looks like it... just trying to avoid other double/float edge case. Do you want to split "code cleanliness" part and actual fix into two different patches?

(In reply to comment #33) > Looks like it... just trying to avoid other double/float edge case. > > Do you want to split "code cleanliness" part and actual fix into two > different patches? Yes please.

regression on the trunk after 4, we should try to get this in to 5 if it's not too scary.

(In reply to comment #35) > Created attachment 531231 [details] [diff] [review] [review] > Removing source boundary check plus tests To confirm this reverts us to the behavior of Firefox 4 which matches what other browsers do?

(In reply to comment #38) > (In reply to comment #35) > > Created attachment 531231 [details] [diff] [review] [review] [review] > > Removing source boundary check plus tests > > To confirm this reverts us to the behavior of Firefox 4 which matches what > other browsers do? Jeff, Comment 27 shows behavior of the Firefox 4. That behavior is wrong per specification (see comment 26). Comment 23 fixes the behavior to match behavior of the Chrome (WebKit passes 2d.drawImage.outsidesource test), this test is invalid per spec as well. The attachment 531231 [details] [diff] [review] removes the check that was introduced in rev 4ede7b9b55bc (similar and less restrictive check was in FF4). That check was introduces by older specification and does not matter to the code below it.

Comment on attachment 531231 [details] [diff] [review] Removing source boundary check plus tests I thought I had already responded to this but apparently not. Why are the tests being changed to todo? Aren't they wrong now?

Jeff, The test still present in Philip Taylor's tests. However, the specification does not require to perform the source boundary check (at was in older versions of the spec). Not sure what do with that test, so I mark it as to do: 1) in case in specification will be changed back, 2) have some "feedback" in case if the boundary check will be reverted back somehow. Do you want to remove this test?

(In reply to comment #41) > Jeff, > > The test still present in Philip Taylor's tests. However, the specification > does not require to perform the source boundary check (at was in older > versions of the spec). Not sure what do with that test, so I mark it as to > do: 1) in case in specification will be changed back, 2) have some > "feedback" in case if the boundary check will be reverted back somehow. > > Do you want to remove this test? Why not have change them to 'fail' and add a comment about the situation? This way we still test the behavior and don't give the impression that the code is incomplete or unintentional.

The test_2d.drawImage.outsidesource.html section was removed from the test_canvas.html. The new test was added to test the behavior described in the comment #26 (and the spec): > ... When the filtering algorithm requires a > pixel value from outside the original image data, it must instead use the > value from the nearest edge pixel. > (That is, the filter uses 'clamp-to-edge' behavior.) This test is a more common case of the "minimal test case to replicate the issue", and opposite to the obsolete 2d.drawImage.outsidesource.html test.

Time is running out for Firefox 5 and this regression is something we'd like to see fixed. Should we consider just backing out bug 629875 since we do not yet have a fully reviewed and tested on m-c patch for this regression?

I ran this patch against try server and the new test fails: http://tbpl.mozilla.org/?tree=Try&rev=c6595914ccf9 e.g. http://tinderbox.mozilla.org/showlog.cgi?log=Try/1305831631.1305832400.20048.gz I think at this point we should seriously consider backing bug 629875 out.

This failure may come from the switch to using EXTEND_PAD in canvas that happened yesterday. EXTEND_PAD doesn't work properly on OS X which may be why the test passes on that platform.

Two patches are not obsolete. Which one should be pushed? Both?

(In reply to comment #46) > This failure may come from the switch to using EXTEND_PAD in canvas that > happened yesterday. EXTEND_PAD doesn't work properly on OS X which may be > why the test passes on that platform. That's correct, the using EXTEND_PAD in canvas caused the failures. The test_bug655328.html was invalid, and the landing of the bug 620216 helped to find that out. The test was fixed. Also, the portion of the tests is marked as todo for the MacOS platform. Here is a try server run for this patch: http://tbpl.mozilla.org/?tree=Try&rev=841818ca41e5

(In reply to comment #48) > Created attachment 533887 [details] [diff] [review] [review] > Removing source boundary check plus tests (v3) > > (In reply to comment #46) > > This failure may come from the switch to using EXTEND_PAD in canvas that > > happened yesterday. EXTEND_PAD doesn't work properly on OS X which may be > > why the test passes on that platform. > > That's correct, the using EXTEND_PAD in canvas caused the failures. The > test_bug655328.html was invalid, and the landing of the bug 620216 helped to > find that out. > > The test was fixed. Also, the portion of the tests is marked as todo for the > MacOS platform. Here is a try server run for this patch: > http://tbpl.mozilla.org/?tree=Try&rev=841818ca41e5 I'm concerned about this. From my understanding of canvas, working EXTEND_PAD shouldn't be need to implement canvas. drawImage shouldn't be drawing outside of the bounds of the image and it sounds like with this patch we are.

IRC log from yesterday: [15:34:20] <joe> clamp-to-edge == EXTEND_PAD [15:35:06] <yury> ok [15:35:21] <joe> so we should be testing that we use extend_pad [15:36:28] <yury> joe, the test is http://hg.mozilla.org/try/diff/a7e871889f7c/content/canvas/test/test_bug655328.html [15:38:53] <yury> probably, i'm not reading the spec right way: shall there, in the middle, be green? [15:40:54] <joe> hm [15:41:10] <joe> it looks like you draw over the same area in the canvas 4 times? [15:43:25] <joe> did you reverse source and destination accidentally? [15:44:01] <yury> it shall take data from outside of the image [15:44:11] <joe> okay [15:44:20] <joe> and draw it to the same position in the canvas? [15:44:33] <yury> yep [15:44:49] <joe> ok good [15:44:57] <joe> in that case [15:45:00] <yury> looks like I missunderstand EXTEND_PAD [15:45:02] <joe> everything should be green [15:45:09] <joe> since we're clamping everything to edge [15:45:22] <joe> so you draw 4 100x50 green rectangles to (0, 0) in the canvas [15:45:53] <yury> good, thanks [15:46:00] <joe> however!! [15:46:04] <joe> this test is going to fail on OS X [15:46:13] <joe> because EXTEND_PAD doesn't work there properly [15:47:33] -*- Philip agrees the spec says it should be drawing 100x50 green four times [15:47:49] -*- Philip hopes that spec behaviour isn't causing compatibility problems [15:48:00] <joe> yury: perhaps you should draw the rectangles to different positions on the canvas? [15:48:07] <joe> to test all the different combinations [15:48:34] -*- Philip really needs to get around to updating all his canvas tests, and then importing them back into Mozilla's test suite somehow... [15:48:40] <yury> so i should remove pixel tests for outside source pixels [15:48:45] <joe> no [15:48:52] <joe> you should keep those pixel tests [15:48:59] <joe> just ignore their results on OS X It's consistent with the comment 26 statements.

> drawImage shouldn't be drawing outside of the bounds of the image Jeff, that's not what the current spec says... Or more precisely the spec is unclear on what happens if the source rect extends outside the image (something we should bring up), but does have the bit from comment 26.

(In reply to comment #51) > > drawImage shouldn't be drawing outside of the bounds of the image > > Jeff, that's not what the current spec says... Or more precisely the spec > is unclear on what happens if the source rect extends outside the image > (something we should bring up), but does have the bit from comment 26. Chrome and Safari don't seem to follow that part of the spec and throw an exception on the proposed test case. I'm not really convinced that we want support drawing outside of the bounds of an image.

Then that brings us back to the original question: why is Chrome, say, not throwing an exception on the original testcase for this bug? If it's just a matter of intermediate computations on floats losing precision compared to those on doubles and hence things that shouldn't be equal becoming equal, then the spec needs to explicitly define all the floating-point operations here, which would be a huge PITA and very fragile for web authors, right?

Also, (In reply to comment #45) > I think at this point we should seriously consider backing bug 629875 out. (In reply to comment #52) > I'm not really convinced that we want > support drawing outside of the bounds of an image. As indicated in comment #27, the code before landing of the bug 629875 looked like: > - // 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) By reverting rev 4ede7b9b55bc (the code above), we will still get some drawing outside of the bounds of an image (when sx + sw > height) and incorrect boundary check. Is the "regression" keyword good label for this issue? Neither the boundary check nor drawing outside of the bounds of an image were implemented property in FF4.

(In reply to comment #53) > Then that brings us back to the original question: why is Chrome, say, not > throwing an exception on the original testcase for this bug? If it's just a > matter of intermediate computations on floats losing precision compared to > those on doubles and hence things that shouldn't be equal becoming equal, > then the spec needs to explicitly define all the floating-point operations > here, which would be a huge PITA and very fragile for web authors, right? Chrome doesn't throw an exception because of floats losing precision. I'm not sure that's so bad. Anytime we have this kind of comparison we're going to run into the problem. It perhaps would have been better if the src rect was defined as p1, p2 instead of p + size which would have avoided this problem somewhat.

> I'm not sure that's so bad. It's terrible, because it makes whether an exception is thrown dependent on a browser's internal representations and order of operations. How would you spec that? > Anytime we have this kind of comparison we're going to run into the problem. Not if we don't ever throw. I believe that's the direction we should be heading; the spec seems to agree.

(In reply to comment #56) > > I'm not sure that's so bad. > > It's terrible, because it makes whether an exception is thrown dependent on > a browser's internal representations and order of operations. How would you > spec that? It's not _that_ hard to spec. i.e. it's basically just a test of whether x + width is less than an integer converted to float. There isn't really any different way of evaluating it than that. I think that's it's better for us to follow the other browsers here than to follow the spec if were going to be the only one following the spec. Certainly if there's an indication that everyone else will switch to follow the spec that would be good, but I'd be a little bit surprised. Also, with regards to CoreGraphics if we switch to the EXTEND_PAD semantics we're basically trading throwing an exception with a performance cliff because requiring EXTEND_PAD will not let us use CGContextDrawImage anymore.

Also, does anyone know why the spec changed?

> it's basically just a test of whether x + width is less than an integer > converted to float. No, it's a test of whether x+width with the addition done in a very particular way with very particular precision is less than that integer. > I think that's it's better for us to follow the other browsers here I think they just haven't updated to follow the spec yet is all... The performance issue is bad; raise that with the spec? > Also, does anyone know why the spec changed? Worth checking, yes. It does have commit logs. I think we should back out bug 629875 on beta and aurora effective immediately, while we think about how to deal with this on trunk. Sound reasonable?

(In reply to comment #59) > > it's basically just a test of whether x + width is less than an integer > > converted to float. > > No, it's a test of whether x+width with the addition done in a very > particular way with very particular precision is less than that integer. Well, the types of x and width are already specified to be float so it's natural that the addition be done as float. > > I think that's it's better for us to follow the other browsers here > > I think they just haven't updated to follow the spec yet is all... The > performance issue is bad; raise that with the spec? > > > Also, does anyone know why the spec changed? > > Worth checking, yes. It does have commit logs. > > I think we should back out bug 629875 on beta and aurora effective > immediately, while we think about how to deal with this on trunk. Sound > reasonable? Very much so.

> Well, the types of x and width are already specified to be float so it's > natural that the addition be done as float. Sort of. For example, these two tests: |x + width > foo|, |x > foo - width| are not equivalent on floats. So the spec would have to be _very_ explicit about the _exact_ arithmetic operations involved in the check. And any changes to client code could break something that happened to work before... I really really don't think we want to go there in the spec. Yury, can you please file a bug with a backout patch for aurora/beta? Or do you want me or Jeff to do that?

(In reply to comment #61) > > Well, the types of x and width are already specified to be float so it's > > natural that the addition be done as float. > > Sort of. For example, these two tests: |x + width > foo|, |x > foo - width| > are not equivalent on floats. So the spec would have to be _very_ explicit > about the _exact_ arithmetic operations involved in the check. And any > changes to client code could break something that happened to work before... > > I really really don't think we want to go there in the spec. > > Yury, can you please file a bug with a backout patch for aurora/beta? Or do > you want me or Jeff to do that? I've already attached a backout patch to bug 629875. Should it be a separate bug?

No, that's fine as long as we track it landing....

(In reply to comment #57) > Also, with regards to CoreGraphics if we switch to the EXTEND_PAD semantics > we're basically trading throwing an exception with a performance cliff > because requiring EXTEND_PAD will not let us use CGContextDrawImage anymore. My understanding was that CGContextDrawImage gave us the slowest DrawImage performance on a "modern" system. I assumed we'd want to replace that with something else anyway, in which case there doesn't seem to be any reason to choose desired behavior based on peculiarities of that function. (Most graphics hardware seems to know how to do clamp-to-edge.)

(In reply to comment #58) > Also, does anyone know why the spec changed? http://html5.org/r/5373 replaced the exception behaviour with: <p>Pixels of the source rectangle that are not entirely within the source image must be treated as transparent black.</p> <!-- see CORE-32111 http://krijnhoetmer.nl/irc-logs/whatwg/20100818#l-737 --> because Opera broke on a page due to float rounding. (The general principle is that a small change in input (e.g. from float imprecision) should only cause a small change in output (never something as drastic as an exception) - hopefully the rest of the canvas spec follows that principle too.) That later changed to clamp-to-edge behaviour as part of http://www.w3.org/Bugs/Public/show_bug.cgi?id=10799 (mainly since it better matched the filtering behaviour of IE9 and Opera, I think).

> because Opera broke on a page due to float rounding. Precisely the point I was trying to make!

What if instead of throwing an exception the source rect was clamped to the size of source? This would allow implementations to avoid reading beyond the source and avoid exceptions from float rounding.

I would be fine with that, from my non-graphics perspective.

What would that gain? EXTEND_PAD-like behaviour would still be required in order to get the specified clamp-to-edge filtering effect near the edges of a (scaled) image, so (as I understand it) it probably wouldn't make the implementation simpler.

(In reply to comment #67) > What if instead of throwing an exception the source rect was clamped to the > size of source? This would allow implementations to avoid reading beyond the > source and avoid exceptions from float rounding. Variant of the attachment 533887 [details] [diff] [review] that clamps the source rectangle to the size of source. Not sure what's blocking this bug and bug 629875 at the moment. It seams everyone agrees that check/exception shall be removed. The attachment 533887 [details] [diff] [review] + original bug 629875 patch will brings the FF up-to-data with the specification and the documentation on https://developer.mozilla.org/en/DOM/CanvasRenderingContext2D#Notes

(In reply to comment #69) > What would that gain? EXTEND_PAD-like behaviour would still be required in > order to get the specified clamp-to-edge filtering effect near the edges of > a (scaled) image, so (as I understand it) it probably wouldn't make the > implementation simpler. It means that implementations don't need to deal with reading pixels outside of the source and would let the actual filtering on the edges be implementation defined because the affected pixels would only be a narrow band along the edge.

Comment on attachment 536088 [details] [diff] [review] Removing source boundary check plus tests (clamps to source size) These seems overly complicated. Can't we just intersect the source rect with this rect: (0,0, imgSize.width, imgSize.height)?

(In reply to comment #72) > Comment on attachment 536088 [details] [diff] [review] [review] > Removing source boundary check plus tests (clamps to source size) > > These seems overly complicated. Can't we just intersect the source rect with > this rect: (0,0, imgSize.width, imgSize.height)? The intersection of the source rect is a simple part, but at the same time the destination rect has to be adjusted proportionally taking the dw/sw and dh/sh ratios into account. I could not find the methods in the gfxRect class to do that. It will not be a simplest solution (so I would agree with comment 69).

(In reply to comment #71) > It means that implementations don't need to deal with reading pixels outside > of the source and would let the actual filtering on the edges be > implementation defined because the affected pixels would only be a narrow > band along the edge. I'd prefer to be moving away from implementation-defined behaviour, not adding more of it on purpose, and http://www.w3.org/Bugs/Public/show_bug.cgi?id=10799 asked for this to be specified. The edge filtering would make a significant difference when someone e.g. draws two images side-by-side (in a tile-based game or whatever) then scales everything up by 2x - it's an interoperability problem if they get unwanted transparent seams between the images in some implementations and not others.

(In reply to comment #74) > (In reply to comment #71) > > It means that implementations don't need to deal with reading pixels outside > > of the source and would let the actual filtering on the edges be > > implementation defined because the affected pixels would only be a narrow > > band along the edge. > > I'd prefer to be moving away from implementation-defined behaviour, not > adding more of it on purpose, and > http://www.w3.org/Bugs/Public/show_bug.cgi?id=10799 asked for this to be > specified. Sure, but having something spec'ed that implementations wont follow isn't much good either. > The edge filtering would make a significant difference when > someone e.g. draws two images side-by-side (in a tile-based game or > whatever) then scales everything up by 2x - it's an interoperability problem > if they get unwanted transparent seams between the images in some > implementations and not others. Can you explain this example in more detail (perhaps attach an example), I'm not sure I understand the issue.

(In reply to comment #73) > (In reply to comment #72) > > Comment on attachment 536088 [details] [diff] [review] [review] [review] > > Removing source boundary check plus tests (clamps to source size) > > > > These seems overly complicated. Can't we just intersect the source rect with > > this rect: (0,0, imgSize.width, imgSize.height)? > > The intersection of the source rect is a simple part, but at the same time > the destination rect has to be adjusted proportionally taking the dw/sw and > dh/sh ratios into account. I could not find the methods in the gfxRect class > to do that. It will not be a simplest solution (so I would agree with > comment 69). Alternatively, we could keep the dest rect the same size. I'm sort of leaning toward this solution. It's certainly easier to spec.

(In reply to comment #75) > (In reply to comment #74) > > The edge filtering would make a significant difference when > > someone e.g. draws two images side-by-side (in a tile-based game or > > whatever) then scales everything up by 2x - it's an interoperability problem > > if they get unwanted transparent seams between the images in some > > implementations and not others. > > Can you explain this example in more detail (perhaps attach an example), I'm > not sure I understand the issue. http://philip.html5.org/demos/canvas/image-tile.html - in Firefox 4.0 on Linux there's a light line between each repetition of the tile; in other browsers there isn't. When using canvas it's easy to work around that problem once you know about it (e.g. use patterns instead; or if you're going to scale by factor N then add an N pixel border to the original image file and crop out the border in the drawImage call). But leaving the behaviour implementation-defined means people would have to work around every browser's different filtering method, or more likely have their code break (with visible rendering errors) in some browsers. I don't particularly care exactly which behaviour the spec requires, but I'd like browsers to eventually converge on the same behaviour.

as far as tracking firefox 5, this is fixed by backout at 629875.

Are we going to need to back out for 6 like we did 5? Time is short for 6 and it's pretty late to be taking any invasive changes.

I _believe_ this was backed out of 6 as well, since the June 2 backout happened on aurora as well as beta. But someone should double-check this. On the other hand, for 7 we still need to do the backout. Imo. Setting the tracking nomination to track that...

> But someone should double-check this. I'm not sure what changeset you're looking for, but is it this the one? http://hg.mozilla.org/releases/mozilla-beta/rev/5ae373ff7a19

No, that was the backout for 5. The part I was looking for, I think is http://hg.mozilla.org/releases/mozilla-beta/rev/72b45a62587a (which was merged from aurora 6). But the question is whether that merge worked as it should and whether as a result the code is correct. As in, whether this bug is actually reproducible in beta 6.

Tracking for 7. We need a backout patch for 7 (aurora) and confirmation that it was backed out on 6 (beta). If it has been already covered on beta, please set the status-firefox6 to fixed.

Pinged Bas & Jeff via email.

Yes this is fixed on 6: http://hg.mozilla.org/releases/mozilla-beta/rev/72b45a62587a

Should we back this out everywhere? We're about to uplift Aurora to Beta again and this is still hanging around. How about back it out from Aurora and m-c try again.

This looks like it is backed out on mozilla-beta (7) as well, but jeff please check to make sure.

(In reply to Christian Legnitto [:LegNeato] from comment #87) > This looks like it is backed out on mozilla-beta (7) as well, but jeff > please check to make sure. This has also been backed out of FF7

I marked this as blocking gecko-games because I have a Cocos2d-html5 game at hand which unfortunately bumps into this in issue. This incompatibility happens because (for anyone interested): 1. Making a sprite frame (in Cocos2d-html5, cc.SpriteFrame) requires specifying an image and a rectangle. 2. The designer changes the images without keeping the sizes of the images same (but not to much that you need to re-specify the rectangle). I've come to realize that designers don't take the size of a picture to be an invariant when they create a new skin. 3. The rectangle now has parts outside the image. Boom!

Comment on attachment 533887 [details] [diff] [review] Removing source boundary check plus tests (v3) The spec ended up with clipping the destination bounds based on the source image. (You're second patch)

Comment on attachment 536088 [details] [diff] [review] Removing source boundary check plus tests (clamps to source size) Review of attachment 536088 [details] [diff] [review]: ----------------------------------------------------------------- This is the right idea but will need rebasing as the implementation has changed substantially since you wrote it.

Cleaned up variant of the original patches in this thread. It clips the source rectangle and then clips the destination rectangle in the same proportion as mandated by the canvas spec. It gets rid of the non-standard behavior of throwing IndexSizeError when the source or dest is out of bounds, since that is now handled by the clipping.

We have a WPT test for this case now, but it was not enabled since we were expecting it to be broken before... but not anymore!

This test only checks that our behavior of throwing IndexSizeError when the source rect is outside the canvas is correct. However, we no longer do, so the test has no more reason to exist.

Comment on attachment 8659908 [details] [diff] [review] part 1 - clip canvas drawImage source/dest rectangles instead of throwing IndexSizeError Review of attachment 8659908 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/canvas/CanvasRenderingContext2D.cpp @@ +4314,5 @@ > // > > +static void > +ClipImageRect(double& aSourceCoord, double& aSourceSize, int32_t aImageSize, > + double& aDestCoord, double& aDestSize) ClipImageDimension seems like a better name than Rect which implies we're doing both axes.

Cosmetic change only - just renamed ClipImageRect to ClipImageDimension.

Try run: https://treeherder.mozilla.org/#/jobs?repo=try&revision=ab765cedc709

Hi Lee, part 3 failed to apply: adding 655328 to series file renamed 655328 -> remove-drawimage-outsidesource-test.diff applying remove-drawimage-outsidesource-test.diff patching file dom/canvas/test/test_canvas.html Hunk #1 FAILED at 3545 1 out of 2 hunks FAILED -- saving rejects to file dom/canvas/test/test_canvas.html.rej patch failed, unable to continue (try -v) patch failed, rejects left in working directory errors during apply, please fix and refresh remove-drawimage-outsidesource-test.diff could you take a look, thanks!

(In reply to Carsten Book [:Tomcat] from comment #99) > Hi Lee, > > part 3 failed to apply: > > adding 655328 to series file > renamed 655328 -> remove-drawimage-outsidesource-test.diff > applying remove-drawimage-outsidesource-test.diff > patching file dom/canvas/test/test_canvas.html > Hunk #1 FAILED at 3545 > 1 out of 2 hunks FAILED -- saving rejects to file > dom/canvas/test/test_canvas.html.rej > patch failed, unable to continue (try -v) > patch failed, rejects left in working directory > errors during apply, please fix and refresh > remove-drawimage-outsidesource-test.diff > > could you take a look, thanks! The patches from bug 1074733 must be applied before the patches from this bug, and then this patch will apply just fine; I just verified. That's why I marked bug 1074733 as a blocker for this bug, as I was under the impression such dependencies were obeyed on checkins.

https://hg.mozilla.org/integration/mozilla-inbound/rev/6c34a4705aa9 https://hg.mozilla.org/integration/mozilla-inbound/rev/83ada9133617 https://hg.mozilla.org/integration/mozilla-inbound/rev/ad99d8bf748d

(In reply to Lee Salzman [:eihrul] from comment #100) yeah as mentioned on irc by the other sheriffs, some information about the checkin (like only part x from y) or like "checkin bug xyz first" help a lot :)

https://hg.mozilla.org/mozilla-central/rev/6c34a4705aa9 https://hg.mozilla.org/mozilla-central/rev/83ada9133617 https://hg.mozilla.org/mozilla-central/rev/ad99d8bf748d