After drawing the canvas code uses calls to didDraw to invalidate what has changed. For composite modes which affect the whole canvas (e.g. Source-In), the whole canvas needs to be invalidated. This isn't happening for fill or fillRect, which do implement the composite modes correctly. These operations, and any other drawing operations which do implement these composite modes correctly should call didDraw with the whole canvas.
Created attachment 113124 [details] Patch
Comment on attachment 113124 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=113124&action=review Need tests > Source/WebCore/ChangeLog:8 > + No new tests. the way we normally test invalidation rects like this is via repaint tests - paint once, then do a draw, then paint again and see if the repainted area is correct. we probably already have some for canvas draws already.
Created attachment 113129 [details] Patch with test
Comment on attachment 113129 [details] Patch with test View in context: https://bugs.webkit.org/attachment.cgi?id=113129&action=review > LayoutTests/ChangeLog:10 > + * platform/chromium-gpu-linux/fast/canvas/canvas-composite-fill-repaint-expected.png: Added. Hmm. This result image looks wrong, but is probably not your fault -- the compositor doesn't support repaint tests, IIRC. Since apparently we're switching to a policy of checking in bad results anyway, this is ok, but you should probably add a line to test_expectations documenting the problem.
(In reply to comment #4) > Hmm. This result image looks wrong, but is probably not your fault -- the compositor doesn't support repaint tests, IIRC. > > Since apparently we're switching to a policy of checking in bad results anyway, this is ok, but you should probably add a line to test_expectations documenting the problem. The GPU compositor always redraws the whole screen (if I understand James' comment on bug 67760 correctly). The problem being fixed is that not enough of the canvas is redrawn after some operations. Redrawing too much is correct, but may be inefficient. Is that right? If so the expected GPU result is good. If its not right (i.e. the GPU result is a failure) ... I wasn't aware of a policy to check in incorrect results. Is this how I would document the problem in test_expectations: // GPU doesn't support repaint tests; checked in expected result is not correct. BUG67760 GPU : fast/canvas/canvas-composite-fill-repaint-expected.png = PASS Seems more complicated than putting in an expected image failure.
That sounds correct. I don't see any reason to put a line in test_expectations, that png is what we expect for this test.
Comment on attachment 113129 [details] Patch with test R=me
Comment on attachment 113129 [details] Patch with test cq+ following jamesr's r+
Comment on attachment 113129 [details] Patch with test Clearing flags on attachment: 113129 Committed r99043: <http://trac.webkit.org/changeset/99043>
All reviewed patches have been landed. Closing bug.
(In reply to comment #6) > That sounds correct. I don't see any reason to put a line in test_expectations, that png is what we expect for this test. Hmm. My thought was that we could get repaint tests to work by having the compositor draw the grey rectangle over the layer of interest, rather than on the compositor's result. Then, the compositor would upload the whole layer (since it would be invalidated), but it would contain the grey rectangle in the non-redrawn regions. Would that not work? If not, running repaint tests on the compositor seems like a waste of time, since they're not telling us what we want them to tell us, namely that the invalidated portion (and only the invalidated portion) is being redrawn.
I think the repaint tests still "work" in that we can verify if the final pixel output is what we want. The repaint tests are for two things: 1.) What is the final pixel output? 2.) What portion of the final output was painted on the last frame? We always paint every pixel every frame in compositing mode so part (2) isn't really tested, but part 1 is still nice.