[chromium] Compute occlusion during paint loop
Created attachment 123613 [details] Patch For @enne review. - Computes occluded region in screen space during paint process. - Adds unit test to ensure layers contribute to the occluded region nicely.
Comment on attachment 123613 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=123613&action=review > Source/WebCore/platform/graphics/chromium/LayerChromium.cpp:520 > + if (opaque()) { Don't you need an opacity check here too? Also, how do you handle the fact that that a layer might have an opacity of 1 but is drawn into a render surface with an opacity != 1? Can you add a test for that? > Source/WebCore/platform/graphics/chromium/LayerChromium.cpp:525 > + if (screenSpaceQuad.isRectilinear() && screenSpaceQuad.boundingBox().isExpressibleAsIntRect()) Isn't the first check enough if you're going to use enclosedIntRect? > Source/WebCore/platform/graphics/chromium/TiledLayerChromium.cpp:461 > +static IntRect enclosedIntRect(const FloatRect& rect) Can you not duplicate this function? Either put it in FloatRect or make it a helper on LayerChromium? > Source/WebCore/platform/graphics/chromium/TiledLayerChromium.cpp:475 > + if (opaque()) { Opacity here, as above? > Source/WebCore/platform/graphics/chromium/TiledLayerChromium.cpp:482 > + if (!screenSpaceQuad.isRectilinear() || !screenSpaceQuad.boundingBox().isExpressibleAsIntRect()) No need for the isExpressibleAsIntRect, as above? > Source/WebCore/platform/graphics/chromium/TiledLayerChromium.cpp:491 > + for (CCLayerTilingData::TileMap::const_iterator iter = m_tiler->tiles().begin(); iter != m_tiler->tiles().end(); ++iter) { > + UpdatableTile* tile = static_cast<UpdatableTile*>(iter->second.get()); > + tileRegion.unite(tile->m_opaqueRect); > + } Hash maps can get large. If you're going to intersect with the visibleLayerRect later, can you just iterate through the tiles that intersect with the visibleLayerRect? > Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHost.cpp:427 > + paintContentsIfDirty(renderSurfaceLayer->maskLayer(), paintType, occludedScreenSpace); I think this is wrong. Any surface with a mask is opaque at a given pixel iff the layers drawing into that pixel are opaque, the opacity on the surface is 1, and the mask's alpha value is 1. I think you're only checking the last condition, but I'm not sure that you can check the first. Can you add a test? > Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHost.cpp:432 > + paintContentsIfDirty(replicaLayer, paintType, occludedScreenSpace); Er, is the right transform being applied here for replica layers? Replica layers are the same layer as the layer they're replicating, but use the replica draw transform in CCRenderSurface. Can you write a test?
Created attachment 124139 [details] Patch Reworked this. Occlusion is computed into a single RS target at a time. I'll worry about crossing between RenderSurfaces in another CL. This should be pretty straight-forward as visibleLayerRect will handle clipping up to the target RenderSurface. Comments addressed: - added logic about opacity() == 1 - changed how enclosedIntRect() works so that we can avoid isExpressibleAsIntRect() - pushed enclosedIntRect() out to FloatRect.cpp - only add occlusion from tiles that touch the visibleLayerRect - don't do anything with replica/mask layers, since RS do not contribute to occlusion now (it's reset when you start painting in a new RS).
Created attachment 124150 [details] Patch s/occludedScreenSpace/occludedTargetSpace/
Comment on attachment 124150 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=124150&action=review > Source/WebCore/platform/graphics/chromium/LayerChromium.h:199 > + virtual void addSelfToOccludedRegion(Region* occludedTargetSpace); Please use a reference here instead of a pointer unless passing NULL to this function is valid behavior. > Source/WebCore/platform/graphics/chromium/TiledLayerChromium.cpp:472 > + // contained within each tile's screen-space opaque rect. Merging the tiles might help a bit here but it doesn't really solve the problem. You're still going to have rectangles that touch each other. How about we limit occlusion to layers that pass the isExpressibleAsIntRect() check just like what we're doing to determine if a layer needs anti-aliasing? And we can use a follow up patch to improve this. I don't think a FloatRegion is the solution here. The complexity of such thing would be even more unpredictable than a Region.
Comment on attachment 124150 [details] Patch Attachment 124150 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/11163431
View in context: https://bugs.webkit.org/attachment.cgi?id=124150&action=review >> Source/WebCore/platform/graphics/chromium/LayerChromium.h:199 >> + virtual void addSelfToOccludedRegion(Region* occludedTargetSpace); > > Please use a reference here instead of a pointer unless passing NULL to this function is valid behavior. k >> Source/WebCore/platform/graphics/chromium/TiledLayerChromium.cpp:472 >> + // contained within each tile's screen-space opaque rect. > > Merging the tiles might help a bit here but it doesn't really solve the problem. You're still going to have rectangles that touch each other. How about we limit occlusion to layers that pass the isExpressibleAsIntRect() check just like what we're doing to determine if a layer needs anti-aliasing? And we can use a follow up patch to improve this. I don't think a FloatRegion is the solution here. The complexity of such thing would be even more unpredictable than a Region. it's not rotations that are the problem I am trying to address here, it is scaling where the resulting tile rects have partial pixels in it. if tile boundaries are mid-pixel, then if we take each tile separately, we have to shrink each tile down to drop the partial pixels when deciding what's opaque. and that leaves the pixel shared by the two tiles as marked non-opaque in the end. this solution isn't perfect but at least when you scale a layer you don't get little 1-pixel lines at all tile boundaries which would greatly degrade the usefulness of the occlusion to the layers below. aside: I don't know why a FloatRect would have worse complexity than an IntRect if you comparisons taking numeric_limit::epsilon into account.
Comment on attachment 124150 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=124150&action=review > Source/WebCore/platform/graphics/chromium/LayerChromium.cpp:510 > + if (drawOpacity() != 1 || !isPaintedAxisAlignedInTarget()) > + return; > + > + if (opaque()) { > + FloatRect targetRect = contentToTargetSpaceTransform().mapRect(FloatRect(visibleLayerRect())); > + occludedTargetSpace->unite(enclosedIntRect(targetRect)); > + } nit: put opaque() in the first conditional. > Source/WebCore/platform/graphics/chromium/TiledLayerChromium.cpp:473 > + // FIXME: Create/Use a FloatRegion instead of a Region based on ints to avoid this step and get better accuracy in screen space. How would that help? Aren't the opaque rects on tiles necessarily IntRects? > Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHost.cpp:449 > + occludedTargetSpace = Region(); This looks like if you ever come along something that draws a render surface as a texture, then you reset the occlusion? Why not just not add anything for that surface? > Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHost.cpp:451 > + // Mask and replica layers are not occluded by layers in the surface Just so I understand, this is a FIXME and not a hard constraint, right? It seems like mask and replica layers could be skipped entirely by culling. > Source/WebKit/chromium/tests/CCLayerTreeHostCommonTest.cpp:633 > + EXPECT_EQ_RECT(IntRect(30, 30, 70, 70), occluded.bounds()); nit: Region::bounds() is not necessarily fully covered by the rects in a region unless you also verify that there's only one rect. > Source/WebKit/chromium/tests/CCLayerTreeHostCommonTest.cpp:734 > + // FIXME: when we fix this "root-layer special case" behavior in CCLayerTreeHost, we will have to fix it here, too. I don't know that I agree with this FIXME. The viewport has to be set *somewhere*, I think. > Source/WebKit/chromium/tests/TiledLayerChromiumTest.cpp:426 > + // But a non-axis-aligned transform does not get considered for occlusion. nit: This feels like testing the code rather than testing behavior. We could theoretically support an enclosedIntRect(FloatQuad&) function in the future?
> >> Source/WebCore/platform/graphics/chromium/TiledLayerChromium.cpp:472 > >> + // contained within each tile's screen-space opaque rect. > > > > Merging the tiles might help a bit here but it doesn't really solve the problem. You're still going to have rectangles that touch each other. How about we limit occlusion to layers that pass the isExpressibleAsIntRect() check just like what we're doing to determine if a layer needs anti-aliasing? And we can use a follow up patch to improve this. I don't think a FloatRegion is the solution here. The complexity of such thing would be even more unpredictable than a Region. > > it's not rotations that are the problem I am trying to address here, it is scaling where the resulting tile rects have partial pixels in it. if tile boundaries are mid-pixel, then if we take each tile separately, we have to shrink each tile down to drop the partial pixels when deciding what's opaque. and that leaves the pixel shared by the two tiles as marked non-opaque in the end. > > this solution isn't perfect but at least when you scale a layer you don't get little 1-pixel lines at all tile boundaries which would greatly degrade the usefulness of the occlusion to the layers below. I understand. It takes care of the scale and floating point translation cases when all tiles are opaque. If all tiles are not opaque you can still get a bunch of 1-pixel lines at tile boundaries. Maybe we should only use this optimization when we know that all tiles are opaque? And not bother with the union..
Thanks for review! (In reply to comment #8) > (From update of attachment 124150 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=124150&action=review > > > Source/WebCore/platform/graphics/chromium/LayerChromium.cpp:510 > > + if (drawOpacity() != 1 || !isPaintedAxisAlignedInTarget()) > > + return; > > + > > + if (opaque()) { > > + FloatRect targetRect = contentToTargetSpaceTransform().mapRect(FloatRect(visibleLayerRect())); > > + occludedTargetSpace->unite(enclosedIntRect(targetRect)); > > + } > > nit: put opaque() in the first conditional. done. > > Source/WebCore/platform/graphics/chromium/TiledLayerChromium.cpp:473 > > + // FIXME: Create/Use a FloatRegion instead of a Region based on ints to avoid this step and get better accuracy in screen space. > > How would that help? Aren't the opaque rects on tiles necessarily IntRects? It's the occludedTargetSpace Region I'm talking about here not the one joining the tile rects. The latter could just go away. Made comment more clear. Tile rects translated to the space of the occlusion Region are now FloatRects and may have boundaries inside pixels (rather than between pixels). My comment to Revemen tried to explain what is wrong with the current Int-Region approach if you consider each tile independently. The same problem exists between layers and a Float-Region would resolve that. > > Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHost.cpp:449 > > + occludedTargetSpace = Region(); > > This looks like if you ever come along something that draws a render surface as a texture, then you reset the occlusion? Why not just not add anything for that surface? When you reach a target RS then you are going to paint the target and your target surface will become different for the next painted layer. For now I build occlusion within each target RenderSurface independently, don't consider contributing RenderSurfaces, and start over for a new target. In future: - layers with a contributing RS can bring that RS's occlusion along for the current target. - A RS that is in front of, but not contributing to, a target can occlude the current target. But these are not part of this CL. > > Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHost.cpp:451 > > + // Mask and replica layers are not occluded by layers in the surface > > Just so I understand, this is a FIXME and not a hard constraint, right? It seems like mask and replica layers could be skipped entirely by culling. Yeh. What it says is true IIUC. Layers within a target surface don't occlude the mask/replica since they are applied to those layers. But as you say, other RenderSurfaces could occlude the mask/replica, something not considered for now. I will clarify in comment. > > Source/WebKit/chromium/tests/CCLayerTreeHostCommonTest.cpp:633 > > + EXPECT_EQ_RECT(IntRect(30, 30, 70, 70), occluded.bounds()); > > nit: Region::bounds() is not necessarily fully covered by the rects in a region unless you also verify that there's only one rect. Yeh, I thought it was a safe assumption here, but adding checks for it. > > Source/WebKit/chromium/tests/CCLayerTreeHostCommonTest.cpp:734 > > + // FIXME: when we fix this "root-layer special case" behavior in CCLayerTreeHost, we will have to fix it here, too. > > I don't know that I agree with this FIXME. The viewport has to be set *somewhere*, I think. Me either, I just copied it from the test above. Want me to reword them all? > > Source/WebKit/chromium/tests/TiledLayerChromiumTest.cpp:426 > > + // But a non-axis-aligned transform does not get considered for occlusion. > > nit: This feels like testing the code rather than testing behavior. We could theoretically support an enclosedIntRect(FloatQuad&) function in the future? You're right, I thought about it and wasn't sure what to test though. I have added a FIXME here to justify changing it in the future.
Created attachment 124164 [details] Patch comments addressed.
(In reply to comment #9) > I understand. It takes care of the scale and floating point translation cases when all tiles are opaque. If all tiles are not opaque you can still get a bunch of 1-pixel lines at tile boundaries. Maybe we should only use this optimization when we know that all tiles are opaque? And not bother with the union.. I think this gets the most important cases (fully opaque, opaque interior), and don't want to do anything less effective than that for now. And we definitely will look to improve this in the future.
(In reply to comment #12) > (In reply to comment #9) > > I understand. It takes care of the scale and floating point translation cases when all tiles are opaque. If all tiles are not opaque you can still get a bunch of 1-pixel lines at tile boundaries. Maybe we should only use this optimization when we know that all tiles are opaque? And not bother with the union.. > > I think this gets the most important cases (fully opaque, opaque interior), and don't want to do anything less effective than that for now. And we definitely will look to improve this in the future. Fine with me if others don't mind. I wish we could review and land patch-sets instead of patches as if someone goes back and looks at this change. It won't be obvious that it's a valid change without that optimization. Btw, regarding using a FloatRegion. The computational complexity wont get worse. It's still N^2 for most region operations. The unpredictability of N is the problem when using a Region. I could theoretically create a page that causes N to be page-width * page-height. If we use a FloatRegion, then I could create a page where N=page-width/numeric_limit::epsilon * page-height/numeric_limit::epsilon. I find that uncomfortable.
(In reply to comment #10) > > > Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHost.cpp:449 > > > + occludedTargetSpace = Region(); > > > > This looks like if you ever come along something that draws a render surface as a texture, then you reset the occlusion? Why not just not add anything for that surface? > > When you reach a target RS then you are going to paint the target and your target surface will become different for the next painted layer. For now I build occlusion within each target RenderSurface independently, don't consider contributing RenderSurfaces, and start over for a new target. Ah, gotcha. The iterator is going front-to-back, not by render surface. I know that *any* paint culling is better than no paint culling, but it's kind of unfortunate that if you had a RS in the middle of your layer list that you might lose the ability to cull a lot of layers. Does it make any sense to switch the iterator type here? > > > Source/WebKit/chromium/tests/TiledLayerChromiumTest.cpp:426 > > > + // But a non-axis-aligned transform does not get considered for occlusion. > > > > nit: This feels like testing the code rather than testing behavior. We could theoretically support an enclosedIntRect(FloatQuad&) function in the future? > > You're right, I thought about it and wasn't sure what to test though. I have added a FIXME here to justify changing it in the future. I don't really know either. At best you could add a rotated quad and make sure that it doesn't occlude some other quad that's adjacent/nearby but not overlapping? A FIXME is fine for now. If it gets changed, that test can just get removed in the future.
(In reply to comment #14) > > When you reach a target RS then you are going to paint the target and your target surface will become different for the next painted layer. For now I build occlusion within each target RenderSurface independently, don't consider contributing RenderSurfaces, and start over for a new target. > > Ah, gotcha. The iterator is going front-to-back, not by render surface. > > I know that *any* paint culling is better than no paint culling, but it's kind of unfortunate that if you had a RS in the middle of your layer list that you might lose the ability to cull a lot of layers. Does it make any sense to switch the iterator type here? Ah I see what you mean. We can just track the occluded Region on the target RenderSurface. I was going to have to do this in the future anyway for including contributing RenderSurfaces.
Created attachment 124209 [details] Patch track occlusion in the target surface
Created attachment 124361 [details] Patch - dropped all new state from the RenderSurfaceChromium. - using a stack in the CCLayerTreeHost::paintLayerContents() method to track occlusion in a way that respects clipping from RenderSurfaces - more unit tests in CCLayerTreeHostTest.cpp for this stack approach. - added the occluded Region to paintContentsIfDirty() for the new unit tests. it is not used anywhere except in tests in this CL.
Comment on attachment 124361 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=124361&action=review > Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHost.cpp:432 > + paintContentsIfDirty(replicaLayer, paintType, noOcclusion); Wouldn't this include occluders from the mask? Do you need a new local "Region noOcclusion" within each of these three conditional blocks? > Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHost.cpp:461 > +static void popAndPushTargetRenderSurfaceRegion(Vector<RenderSurfaceRegion>& stack, RenderSurfaceChromium* surface) This function name is really opaque to me (pun not intended). Sorry to bikeshed, but can you name these based on how they should be used and not the internal details of how they work? They're really "start processing new render surface" and "finished processing render surface", yeah? > Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHost.cpp:472 > + // Replace the top of the stack with the new pushed surface. Copy the occluded region to the top. > + stack.last().surface = surface; When does this case happen? > Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHost.cpp:504 > + pushTargetRenderSurfaceRegion(targetSurfaceStack, it->targetRenderSurface()); Mostly asking out of curiosity, but why is this needed here? Shouldn't the it.representsTargetRenderSurface() case above catch this?
(In reply to comment #18) > (From update of attachment 124361 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=124361&action=review > > > Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHost.cpp:432 > > + paintContentsIfDirty(replicaLayer, paintType, noOcclusion); > > Wouldn't this include occluders from the mask? Do you need a new local "Region noOcclusion" within each of these three conditional blocks? The occlusion isnt modified in paintContentsIfDirty. It's a 1-way thing. You get occlusion back from a layer through addSelfToOccludedScreenSpace(). > > Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHost.cpp:461 > > +static void popAndPushTargetRenderSurfaceRegion(Vector<RenderSurfaceRegion>& stack, RenderSurfaceChromium* surface) > > This function name is really opaque to me (pun not intended). Sorry to bikeshed, but can you name these based on how they should be used and not the internal details of how they work? They're really "start processing new render surface" and "finished processing render surface", yeah? Yeh. First is "start b", second is "switch from b to a". Will fix them up. > > Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHost.cpp:472 > > + // Replace the top of the stack with the new pushed surface. Copy the occluded region to the top. > > + stack.last().surface = surface; > > When does this case happen? When the layer with contributing surface is the front-most layer in a target surface. Then the target surface won't be on the stack yet. Conceptually this is: - save Region from top of stack - pop top of stack - push new target to top of stack if not there - merge saved Region. since we know it's not on the stack in this case, merging the region is just a copy, and we can just replace the surface pointer instead. > > Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHost.cpp:504 > > + pushTargetRenderSurfaceRegion(targetSurfaceStack, it->targetRenderSurface()); > > Mostly asking out of curiosity, but why is this needed here? Shouldn't the it.representsTargetRenderSurface() case above catch this? It is FrontToBack, so representsTargetRenderSurface happens last for a given target. This one will always happen before it, if the surface has any layers at all. The reason it is in the it.representsTargetRenderSurface() at all is in the case the surface has no layers in it.
Created attachment 124375 [details] Patch - renamed methods to enterTargetRenderSurface() and leaveTargetRenderSurface()
Comment on attachment 124375 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=124375&action=review > Source/WebCore/platform/graphics/chromium/ContentLayerChromium.h:63 > + virtual void paintContentsIfDirty(const Region& occludedScreenSpace); > virtual void idlePaintContentsIfDirty(); shouldn't we also have the occluded region for idlePaint as well? the fact that we need to duplicate this too makes me wonder if having parallel functions is the best way to do paint / idle paint
(In reply to comment #21) > (From update of attachment 124375 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=124375&action=review > > > Source/WebCore/platform/graphics/chromium/ContentLayerChromium.h:63 > > + virtual void paintContentsIfDirty(const Region& occludedScreenSpace); > > virtual void idlePaintContentsIfDirty(); > > shouldn't we also have the occluded region for idlePaint as well? Yes, the next CL to do the culling adds it to idle function. This CL doesn't actually use the region, but I needed to add it just to be able to subclass for testing purposes in CCLayerTreeHostTest.cpp > the fact that we need to duplicate this too makes me wonder if having parallel functions is the best way to do paint / idle paint It's maybe nice to have separate functions for the logic of choosing what tiles to paint in TiledLayerChromium.
Created attachment 124594 [details] Patch Was discarding occlusion in a RenderSurface subtree when the RS had a mask, but need to consider drawOpacity() also.
Some algorithmic overview for this CL. - Each layer's occlusion is added to a Region which tracks in screen space. - A RenderSurface subtree accumulates occlusion. When leaving a subtree, the occlusion is either kept or tossed away. What is considered when adding a Layer to the accumulated occlusion? - The Layer's transform into its target. - All RS transforms from the Layer to the screen. - The Layer's opacity. This is because we track occlusion in screen space only. What is NOT considered when adding a Layer to the accumulated occlusion? - The RS drawOpacity. - The RS mask. - The RS skipsDraw (this is never true for RSChromium). Instead, when we leave a RS subtree, if the RS mask/opacity change the subtree's occlusion, we discard any contribution the subtree had made to the accumulated occlusion. If a RS is NOT opaque (opacity < 1 or mask): - Layers within the RS subtree can still occlude other layers in the RS subtree. - Layers from outside the RS subtree can occlude inside the RS subtree. If a RS IS opaque (opacity == 1 and !mask): - Layers within the RS subtree will also occlude Layers outside the RS subtree. Picture: RS1 ABCDE | \ | \ RS2 \ FGHIJ \ | RS3 KLMNO If RS2 has a mask and all layers are rectilinear: - J may occlude F. (in the same RS2 subtree) - J may NOT occlude D. (RS2's mask discards RS2 occlusion outside the RS2 subtree) - M may occluded J and D. (RS3 occlusion is kept outside the RS3 subtree) ==== Why this approach? - The primary goal is to catch occlusion in Aura layers which are almost entirely obscured. In the current Aura WM these layers are all rectilinear on the screen. So optimization for rotation is not a priority. ==== Some next steps for future CLs: - Allow occlusion in a RS subtree ignoring the RS transforms. - Allow the mask to modify the RS subtree's occlusion rather than simply discarding it. - Replicas are simply ignored. They do not see nor contribute to occlusion.
Created attachment 124628 [details] Patch rebased
Comment on attachment 124594 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=124594&action=review OK, I think I get it now. There's so much indirection now with the layer iterators and all that I find it really difficult to mentally step through what the actual iteration here is doing. It seems really easy to end up with lots of Regions with lots of spans and segments here and copy them around a lot. Every time we have to resize the targetSurfaceStack vector, for instance, we'll have to copy each intermediate Region, which means copying a pair of Vector<>s on each of those, etc. I suspect that it'll still be pretty fast but it's a cost that can creep up pretty easily and will be hard to notice. Have you looked into what sorts of inline capacities might make sense for these, Dana? R=me but please get a clean EWS run before landing and be sure to run all the compositing tests locally with pixel testing on. You don't need to lose my r+ to get EWS. > Source/WebCore/platform/graphics/chromium/TiledLayerChromium.cpp:478 > + // FIXME: Create/Use a FloatRegion for the occludedScreenSpace, instead of a Region based on ints, to avoid this step and get better accuracy between layers in target space. like David I'm not totally sure this FIXME is the right way to go - if we want to refine this I think we'll be a lot better off tracking occlusion in render surface space as we go rather than trying to do floating point regions > Source/WebKit/chromium/tests/CCLayerTreeHostTest.cpp:1232 > +#define EXPECT_EQ_RECT(a, b) \ aside: i think gmock matchers would be a good way to avoid having to write a bunch of custom comparators like this
Argh bugzilla ate my review
Comment on attachment 124628 [details] Patch Clearing flags on attachment: 124628 Committed r106324: <http://trac.webkit.org/changeset/106324>
All reviewed patches have been landed. Closing bug.
Messed up the unit test when i added the opacity. Should be s/setDrawOpacity/setOpacity/. Thought I ran the unit tests but guess I didn't. Fixed CL incoming.
Created attachment 124671 [details] Patch
Comment on attachment 124671 [details] Patch Rejecting attachment 124671 [details] from commit-queue. Failed to run "['/mnt/git/webkit-commit-queue/Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '-..." exit_code: 1 ERROR: /mnt/git/webkit-commit-queue/Source/WebCore/ChangeLog neither lists a valid reviewer nor contains the string "Unreviewed" or "Rubber stamp" (case insensitive). Full output: http://queues.webkit.org/results/11366998
Created attachment 124743 [details] Patch adding reviewer for cq
Attachment 124743 [details] did not pass style-queue: Failed to run "['Tools/Scripts/update-webkit']" exit_code: 9 Updating OpenSource First, rewinding head to replay your work on top of it... Applying: Fix compilation errors on build-webkit --debug --no-workers on mac. Using index info to reconstruct a base tree... Falling back to patching base and 3-way merge... Auto-merging LayoutTests/ChangeLog CONFLICT (content): Merge conflict in LayoutTests/ChangeLog Auto-merging LayoutTests/platform/qt/Skipped CONFLICT (content): Merge conflict in LayoutTests/platform/qt/Skipped Auto-merging Source/WebCore/ChangeLog CONFLICT (content): Merge conflict in Source/WebCore/ChangeLog Failed to merge in the changes. Patch failed at 0001 Fix compilation errors on build-webkit --debug --no-workers on mac. When you have resolved this problem run "git rebase --continue". If you would prefer to skip this patch, instead run "git rebase --skip". To restore the original branch and stop rebasing run "git rebase --abort". rebase refs/remotes/origin/master: command returned error: 1 Died at Tools/Scripts/update-webkit line 164. If any of these errors are false positives, please file a bug against check-webkit-style.
Created attachment 124751 [details] Patch rebase
Attachment 124751 [details] did not pass style-queue: Failed to run "['Tools/Scripts/update-webkit']" exit_code: 9 Updating OpenSource M Source/WebKit2/UIProcess/qt/QtViewportInteractionEngine.h M Source/WebKit2/UIProcess/qt/QtViewportInteractionEngine.cpp M Source/WebKit2/UIProcess/API/qt/qquickwebview_p.h M Source/WebKit2/UIProcess/API/qt/qquickwebview.cpp M Source/WebKit2/UIProcess/API/qt/qquickwebview_p_p.h M Source/WebKit2/ChangeLog r106362 = ab43d00719700a5da4bc2b60d447b2153c0dbe70 (refs/remotes/origin/master) First, rewinding head to replay your work on top of it... Applying: Fix compilation errors on build-webkit --debug --no-workers on mac. Using index info to reconstruct a base tree... Falling back to patching base and 3-way merge... Auto-merging LayoutTests/ChangeLog CONFLICT (content): Merge conflict in LayoutTests/ChangeLog Auto-merging LayoutTests/platform/qt/Skipped CONFLICT (content): Merge conflict in LayoutTests/platform/qt/Skipped Auto-merging Source/WebCore/ChangeLog CONFLICT (content): Merge conflict in Source/WebCore/ChangeLog Failed to merge in the changes. Patch failed at 0001 Fix compilation errors on build-webkit --debug --no-workers on mac. When you have resolved this problem run "git rebase --continue". If you would prefer to skip this patch, instead run "git rebase --skip". To restore the original branch and stop rebasing run "git rebase --abort". rebase refs/remotes/origin/master: command returned error: 1 Died at Tools/Scripts/update-webkit line 164. If any of these errors are false positives, please file a bug against check-webkit-style.
Comment on attachment 124751 [details] Patch Clearing flags on attachment: 124751 Committed r106383: <http://trac.webkit.org/changeset/106383>