Bug 99605

Created attachment 169203 [details] Patch

Please wait for approval from abarth@webkit.org, dglazkov@chromium.org, fishd@chromium.org, jamesr@chromium.org or tkent@chromium.org before submitting, as this patch contains changes to the Chromium public API. See also https://trac.webkit.org/wiki/ChromiumWebKitAPI.

Before adding support for repaint tracking in composited layers I wanted to make chromium use the same FrameView repaint tracking mechanism as the other ports. This does involve rebaselining, because we now display individual rects instead of their union. I'm open to suggestions on how to go about this without getting murdered by gardeners.

Comment on attachment 169203 [details] Patch This looks great. Any idea how many tests will require rebaselines?

Also, how do other ports deal with scrolling invalidations? Currently on windows anything that comes out as a scroll (instead of a repaint) is left un-highlighted whereas on other ports scrolls turn into normal invalidations and are thus greyed out. The inconsistency is infuriating, but the windows behavior is somewhat nice in that you can tell when something was scrolled vs repainted.

I only checked fast/repaint locally and that amounted to ~200 failures. I'll see how scrolling is handled next.

Ah well. I think it's worth the effort personally and will help with the rebaselining when it gets to that point.

I may be misunderstanding something (and please correct me if I am), but I think that this will make supporting repaint testing for composited layers tougher rather than easier. In fact, I think we should be moving the other ports to the chromium approach rather than the other way around. In a nutshell, with this new approach we create a grey mask and cut out holes where we paint. But imagine that we're using composited layers, the layers on our page are a deck of cards tossed in the air, and we go and repaint rectangles on each of the cards. These rectangles will have perspective effects applied, will be clipped, will be occluded, have backface culling applied, etc. Even if we have stored these repaint rects and the respective draw transforms, it will be a non-trivial, and very brittle process to turn this information into the correct set of holes in the mask (and the holes may become arbitrarily complex polygons). If we take the chromium approach, we will paint all layers transparent grey. If we then invalidate and paint sections of these layers, these sections will not be grey. It's an extremely simple and robust way of showing where we've painted. I have been working on a patch based on your earlier attempt at repaint testing in the chromium way, and it's been working great for chromium (actually, I have just been working on getting DRT for mac to do things the chromium way). In the ChangeLog, you mentioned that chromium's method detects the entire view as having been modified whenever the compositor renders, but I haven't seen this. With my patch, if I trigger the repaint mask code on poster circle, for example, the page continues to animate and to composite and it remains grey. It only stops being grey when I force it to paint by selecting sections of the text on the page. I think that the mask-with-holes approach of the other ports is going to be a liability for them going forward as we have more complex situations with composited layers.

Comment on attachment 169203 [details] Patch Attachment 169203 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/14412247 New failing tests: compositing/checkerboard.html animations/3d/matrix-transform-type-animation.html animations/3d/state-at-end-event-transform.html compositing/direct-image-compositing.html animations/additive-transform-animations.html animations/3d/replace-filling-transform.html compositing/scrollbar-painting.html compositing/clip-change.html compositing/text-on-large-layer.html compositing/layers-inside-overflow-scroll.html compositing/animation/state-at-end-event-transform-layer.html compositing/sibling-positioning.html compositing/generated-content.html compositing/fixed-position-changed-in-composited-layer.html compositing/self-painting-layers.html animations/cross-fade-webkit-mask-box-image.html animations/3d/change-transform-in-end-event.html compositing/absolute-position-changed-with-composited-parent-layer.html compositing/absolute-position-changed-in-composited-layer.html compositing/flat-with-transformed-child.html compositing/backface-visibility/backface-visibility-3d.html animations/cross-fade-list-style-image.html animations/cross-fade-background-image.html animations/cross-fade-border-image-source.html compositing/preserve-3d-toggle.html compositing/fixed-position-scroll-offset-history-restore.html compositing/compositing-visible-descendant.html animations/cross-fade-webkit-mask-image.html compositing/fixed-position-changed-within-composited-parent-layer.html compositing/animation/busy-indicator.html

(In reply to comment #8) I think that's a fair summary Ian, but let me clear up some terms to avoid confusion: 1. The "Chromium way" in this patch refers to old way of drawing a transparent mask over the WebView. Does not support accelerated compositing. 2. The "Chromium way" in comment #8 refers to the patch in bug 96087 which works by painting the repaint mask to each layer's RenderLayerBacking. 3. Finally, the mask-with-holes approach used by this patch and the other ports works by accumulating a list of repaint rects in FrameView and using it to compose a mask that highlights repaints. Currently only supports tracking repaints to the root layer. I'm starting to feel that extending option 3 with non-root layer support is the way to go. More precisely, I'd make RenderLayerBacking register paint rects as absolute FloatQuads (instead of IntRects) on their enclosing FrameView. DRT would then use them to make a mask with holes. You mentioned clipping, occlusion, backface culling, etc., as issues, but actually I think not having those operations is an advantage :) Being able to see painting happening outside overflow clips or in backfacing or occluded layers is useful for spotting bugs in other parts of the system and for testing optimizations like the one in bug 96087. I don't think having complex holes in the mask is an issue either. Ports already need to handle complex clipping paths for canvas painting. One disadvantage of the mask-with-holes approach is that it's harder to have an interactive mode. The compositor would need to do a second rendering pass to apply the mask.

> You mentioned clipping, occlusion, backface culling, etc., as issues, but actually I think not having those operations is an advantage :) Being able to see painting happening outside overflow clips or in backfacing or occluded layers is useful for spotting bugs in other parts of the system and for testing optimizations like the one in bug 96087. That's a very good point. I retract my previous objection :) > > I don't think having complex holes in the mask is an issue either. Ports already need to handle complex clipping paths for canvas painting. > In this case, I wasn't referring to the complex holes that result from taking the union of rects or quads, but the fact that we would have to subtract bits of these holes because of transformed layers that may clip or occlude them. But as you mentioned above, it's probably better than we not do this clipping or occluding anyway.

Re: scrolling; with this patch we get the same behavior as other ports, i.e., scroll invalidations aren't tracked and simply scrolling a page shows everything grayed out. Once we have tracking for non-root layer paints, (composited) scrolling will start to make more sense as the newly exposed tiles will get highlighted.

The other thing to keep in mind is that we'd really like to do to repaint tests by dumping a text representation of the repainted rects instead of using pixels to verify, so anything that moves us closer to that is moving in the right direction. (In reply to comment #12) > Re: scrolling; with this patch we get the same behavior as other ports, i.e., scroll invalidations aren't tracked and simply scrolling a page shows everything grayed out. Once we have tracking for non-root layer paints, (composited) scrolling will start to make more sense as the newly exposed tiles will get highlighted. Great! I think that's the best behavior. Having this be consistent across chromium ports might let you even delete some baselines that currently are different between cr-win vs cr-mac/cr-linux

Marked LayoutTest bugs, bugs with Chromium IDs, and some others as WontFix. Test failure bugs still are trackable via TestExpectations or disabled unit tests.