Make sure this unit test is covered http://trac.webkit.org/browser/trunk/Source/WebKit/chromium/tests/TiledLayerChromiumTest.cpp?rev=109584#L647
Created attachment 130650 [details] Patch
Bugs filed for fixmes: http://code.google.com/p/chromium/issues/detail?id=117203 http://code.google.com/p/chromium/issues/detail?id=117165
Created attachment 130848 [details] Patch Missing a virtual. And rebased for paint-culling CL landing.
Comment on attachment 130848 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=130848&action=review > Source/WebCore/platform/graphics/chromium/cc/CCLayerTilingData.cpp:120 > + // FIXME: Don't lose pixels with tile boundaries inside them. This is because of the enclosedIntRect, yeah? Can you just transform post-unite rather than pre-unite? > Source/WebCore/platform/graphics/chromium/cc/CCLayerTilingData.cpp:127 > + if (tile) { style nit: "if (!tile) continue;" (early outs and less indenting is better) > Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHost.cpp:543 > + occlusionTracker.setUsePaintTracking(false); // FIXME: Remove this to turn on paint tracking for paint culling Is there some larger picture I'm missing as to why you want to land this with paint culling turned off? (I understand leaving a switch to make it easily disabled if need be, just curious about the initial state of that switch.) > Source/WebCore/platform/graphics/chromium/cc/CCTiledLayerImpl.h:59 > + virtual Region opaqueContentsRegion(const TransformationMatrix& contentSpaceTransform) const; I find it a little bit awkward that this function takes a transform. I'm guessing you're trying to save recalculating the transform since you have it in the occlusion tracker. However, isn't this value always calculated from values on the layer itself, and you'll never pass anything but that same value to a given layer? If so, is there some way to clean this up? Can you also leave some comments about what spaces this function is dealing with?
Comment on attachment 130848 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=130848&action=review >> Source/WebCore/platform/graphics/chromium/cc/CCLayerTilingData.cpp:120 >> + // FIXME: Don't lose pixels with tile boundaries inside them. > > This is because of the enclosedIntRect, yeah? Can you just transform post-unite rather than pre-unite? Yes, I could stick them all in a Region, then transform each rect in the region. That is probably what I'd like to do, but I'd like to in a follow up CL? >> Source/WebCore/platform/graphics/chromium/cc/CCLayerTilingData.cpp:127 >> + if (tile) { > > style nit: "if (!tile) continue;" (early outs and less indenting is better) k! >> Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHost.cpp:543 >> + occlusionTracker.setUsePaintTracking(false); // FIXME: Remove this to turn on paint tracking for paint culling > > Is there some larger picture I'm missing as to why you want to land this with paint culling turned off? (I understand leaving a switch to make it easily disabled if need be, just curious about the initial state of that switch.) Mostly for bisect. I'd like to watch paint culling not cause bugs for a little bit on canaries, then turn it on, and we can bisect to the switch flip for problems. >> Source/WebCore/platform/graphics/chromium/cc/CCTiledLayerImpl.h:59 >> + virtual Region opaqueContentsRegion(const TransformationMatrix& contentSpaceTransform) const; > > I find it a little bit awkward that this function takes a transform. I'm guessing you're trying to save recalculating the transform since you have it in the occlusion tracker. However, isn't this value always calculated from values on the layer itself, and you'll never pass anything but that same value to a given layer? If so, is there some way to clean this up? > > Can you also leave some comments about what spaces this function is dealing with? I'll add a comment. The reason is the function can take both a contentToTargetSpace and a contentToScreenSpace transform. Taking a transform as parameter lets one function do both. I could pass a bool and compute the transform inside, but that feels less intuitive to me, wdyt?
(In reply to comment #5) > > I find it a little bit awkward that this function takes a transform. I'm guessing you're trying to save recalculating the transform since you have it in the occlusion tracker. However, isn't this value always calculated from values on the layer itself, and you'll never pass anything but that same value to a given layer? If so, is there some way to clean this up? > > > > Can you also leave some comments about what spaces this function is dealing with? > > I'll add a comment. The reason is the function can take both a contentToTargetSpace and a contentToScreenSpace transform. Taking a transform as parameter lets one function do both. > > I could pass a bool and compute the transform inside, but that feels less intuitive to me, wdyt? Further.. The reason I pass in a transform at all is because I wanted to do this transform before constructing a Region to prevent having to use an intermediate Region in the content space. If we're going to stick them all together in content space to avoid losing pixels, then I could just return that and transform it elsewhere. Better?
Created attachment 131051 [details] Patch
- Removed transform. - Removed FIXME for missing pixels.
Comment on attachment 131051 [details] Patch Thanks for those changes! That looks much simpler to my eyes. I'm still a little confused about the flag switch, since this patch seems like a no-op to me without it turned on. What bugs are you hoping to catch?
You're right about no-op, but I'm hoping to land draw side using this soonish. And wanted to do so without flipping paint side. I'm not sure.. I don't really expect any bugs, but one never does.. Just trying to be a little conservative with it.
(In reply to comment #10) > You're right about no-op, but I'm hoping to land draw side using this soonish. And wanted to do so without flipping paint side. > > I'm not sure.. I don't really expect any bugs, but one never does.. Just trying to be a little conservative with it. Ok! I just wanted to make sure that I was clear on the larger picture. :)
Comment on attachment 131051 [details] Patch Clearing flags on attachment: 131051 Committed r110317: <http://trac.webkit.org/changeset/110317>
All reviewed patches have been landed. Closing bug.