| Summary: | [chromium] Simplify updateContentRect, removing rect parameter, refactor unit tests. | ||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| Product: | WebKit | Reporter: | Eric Penner <epenner> | ||||||||||
| Component: | New Bugs | Assignee: | Eric Penner <epenner> | ||||||||||
| Status: | RESOLVED FIXED | ||||||||||||
| Severity: | Normal | CC: | aelias, cc-bugs, danakj, enne, jamesr, webkit.review.bot | ||||||||||
| Priority: | P2 | ||||||||||||
| Version: | 528+ (Nightly build) | ||||||||||||
| Hardware: | Unspecified | ||||||||||||
| OS: | Unspecified | ||||||||||||
| Attachments: |
|
||||||||||||
|
Description
Eric Penner
2012-08-15 18:10:18 PDT
Created attachment 158677 [details]
Patch
Comment on attachment 158677 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=158677&action=review > Source/WebCore/platform/graphics/chromium/ContentLayerChromium.cpp:-104 > - if (drawsContent()) This is the only material change in this CL. Unfortunately this comment says the 'what' but not the 'why'. I assume it still wants to reset m_skipsDraw and m_failedPaint flags but do nothing else, so I moved those into resetUpdateState(). Comment on attachment 158677 [details]
Patch
Not much to this one. If you are okay with moving the reset of skipsDraw/failedPaint flags, and the early return on !drawsContent() then this is ready to go.
Comment on attachment 158677 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=158677&action=review >> Source/WebCore/platform/graphics/chromium/ContentLayerChromium.cpp:-104 >> - if (drawsContent()) > > This is the only material change in this CL. Unfortunately this comment says the 'what' but not the 'why'. > I assume it still wants to reset m_skipsDraw and m_failedPaint flags but do nothing else, so I moved those into resetUpdateState(). What about the call to updateBounds()? Do we want to call that for a non-drawsContent layer still? If you want to do this, personally I'd feel better doing the early out here in another CL so it's easy to bisect and revert without losing this change, if we need to. > Source/WebKit/chromium/tests/TiledLayerChromiumTest.cpp:316 > + IntRect(0, 0, 100, 200)); // Visible > > - // We should only have the first tile since the other tile was invalidated but not painted. > + // We should have both tiles on the impl side. > EXPECT_TRUE(layerImpl->hasTileAt(0, 0)); > - EXPECT_FALSE(layerImpl->hasTileAt(0, 1)); > + EXPECT_TRUE(layerImpl->hasTileAt(0, 1)); Could this be 0,0,100,100 as the visible rect for the update, so then the expectations won't change? > Source/WebKit/chromium/tests/TiledLayerChromiumTest.cpp:329 > + bool needsUpdate = UpdateAndPush(layer.get(), layerImpl.get(), > + IntSize(500, 500), // Bounds > + IntRect(200, 200, 100, 100)); // Visible The comment above says it invalidates and paints, but this doesn't call the InvalidateUpdateAndPush method. They are invalid since they are new already right? Maybe just update the comment? > Source/WebKit/chromium/tests/TiledLayerChromiumTest.cpp:-354 > // We should only have the first tile from layer2 since it failed to idle update. > EXPECT_TRUE(layerImpl2->hasTileAt(0, 0)); > EXPECT_TRUE(layerImpl2->hasTextureIdForTileAt(0, 0)); > - EXPECT_FALSE(layerImpl2->hasTileAt(0, 1)); > - EXPECT_FALSE(layerImpl2->hasTileAt(0, 2)); This layer used to get a single tile, now I see it expecting 2 tiles, though the comment contradicts. What happened here? > Source/WebKit/chromium/tests/TiledLayerChromiumTest.cpp:435 > + EXPECT_EQ(1, occluded.overdrawMetrics().tilesCulledForUpload()); Nice addition. > Source/WebKit/chromium/tests/TiledLayerChromiumTest.cpp:600 > - > + new whitespace Comment on attachment 158677 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=158677&action=review >>> Source/WebCore/platform/graphics/chromium/ContentLayerChromium.cpp:-104 >>> - if (drawsContent()) >> >> This is the only material change in this CL. Unfortunately this comment says the 'what' but not the 'why'. >> I assume it still wants to reset m_skipsDraw and m_failedPaint flags but do nothing else, so I moved those into resetUpdateState(). > > What about the call to updateBounds()? Do we want to call that for a non-drawsContent layer still? > > If you want to do this, personally I'd feel better doing the early out here in another CL so it's easy to bisect and revert without losing this change, if we need to. Do you know why this might have been done in the first place? Adding updateBounds can't hurt, but it would be great to know exactly why the empty rect was passed. That would still pre-paint, for example, which seems like a bug. On second though, I can move this early out condition into updateContent(). No reason that it shouldn't be the same for all derived classes. Regarding CL, if someone were to revert that CL after this CL lands, updateContentRect will not have the ability to pass the empty rect, and so the revert wouldn't restore the old behavior. So I think it's better to keep it in here. >> Source/WebKit/chromium/tests/TiledLayerChromiumTest.cpp:316 >> + EXPECT_TRUE(layerImpl->hasTileAt(0, 1)); > > Could this be 0,0,100,100 as the visible rect for the update, so then the expectations won't change? Sounds good. >> Source/WebKit/chromium/tests/TiledLayerChromiumTest.cpp:329 >> + IntRect(200, 200, 100, 100)); // Visible > > The comment above says it invalidates and paints, but this doesn't call the InvalidateUpdateAndPush method. They are invalid since they are new already right? Maybe just update the comment? Sounds good. >> Source/WebKit/chromium/tests/TiledLayerChromiumTest.cpp:-354 >> - EXPECT_FALSE(layerImpl2->hasTileAt(0, 2)); > > This layer used to get a single tile, now I see it expecting 2 tiles, though the comment contradicts. What happened here? I think this was a throwback to when there was different memory limits for visible tiles, and the original code/comment is wrong. See above "layer1 exhausts most texture memory, leaving room for 2 more tiles from layer2". The old code was just lucky to work because it only did one pre-paint pass without checking needsIdlePaint. The new code will fail when it tries the paint the last tile. I'll fix the last comment. Comment on attachment 158677 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=158677&action=review >>>> Source/WebCore/platform/graphics/chromium/ContentLayerChromium.cpp:-104 >>>> - if (drawsContent()) >>> >>> This is the only material change in this CL. Unfortunately this comment says the 'what' but not the 'why'. >>> I assume it still wants to reset m_skipsDraw and m_failedPaint flags but do nothing else, so I moved those into resetUpdateState(). >> >> What about the call to updateBounds()? Do we want to call that for a non-drawsContent layer still? >> >> If you want to do this, personally I'd feel better doing the early out here in another CL so it's easy to bisect and revert without losing this change, if we need to. > > Do you know why this might have been done in the first place? Adding updateBounds can't hurt, but it would be great to know exactly why the empty rect was passed. That would still pre-paint, for example, which seems like a bug. On second though, I can move this early out condition into updateContent(). No reason that it shouldn't be the same for all derived classes. > > Regarding CL, if someone were to revert that CL after this CL lands, updateContentRect will not have the ability to pass the empty rect, and so the revert wouldn't restore the old behavior. So I think it's better to keep it in here. What I mean to say is that we could call updateContent() always here, as we used to. And early out within as before, based on visibleContentRect(). Then move the early-out here in another CL. But I like the point about all derived classes, which sounds like exactly the same solution. :) Created attachment 158907 [details]
Patch
Comment on attachment 158677 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=158677&action=review >>>>> Source/WebCore/platform/graphics/chromium/ContentLayerChromium.cpp:-104 >>>>> - if (drawsContent()) >>>> >>>> This is the only material change in this CL. Unfortunately this comment says the 'what' but not the 'why'. >>>> I assume it still wants to reset m_skipsDraw and m_failedPaint flags but do nothing else, so I moved those into resetUpdateState(). >>> >>> What about the call to updateBounds()? Do we want to call that for a non-drawsContent layer still? >>> >>> If you want to do this, personally I'd feel better doing the early out here in another CL so it's easy to bisect and revert without losing this change, if we need to. >> >> Do you know why this might have been done in the first place? Adding updateBounds can't hurt, but it would be great to know exactly why the empty rect was passed. That would still pre-paint, for example, which seems like a bug. On second though, I can move this early out condition into updateContent(). No reason that it shouldn't be the same for all derived classes. >> >> Regarding CL, if someone were to revert that CL after this CL lands, updateContentRect will not have the ability to pass the empty rect, and so the revert wouldn't restore the old behavior. So I think it's better to keep it in here. > > What I mean to say is that we could call updateContent() always here, as we used to. And early out within as before, based on visibleContentRect(). Then move the early-out here in another CL. > > But I like the point about all derived classes, which sounds like exactly the same solution. :) Okay I see, it could keep exactly the same behavior too. However, looking at it I can't see how pre-painting an animated layer but nothing else is correct, so I kept it as an early out moved into updateContent(). >>> Source/WebKit/chromium/tests/TiledLayerChromiumTest.cpp:316 >>> + EXPECT_TRUE(layerImpl->hasTileAt(0, 1)); >> >> Could this be 0,0,100,100 as the visible rect for the update, so then the expectations won't change? > > Sounds good. done. >>> Source/WebKit/chromium/tests/TiledLayerChromiumTest.cpp:329 >>> + IntRect(200, 200, 100, 100)); // Visible >> >> The comment above says it invalidates and paints, but this doesn't call the InvalidateUpdateAndPush method. They are invalid since they are new already right? Maybe just update the comment? > > Sounds good. done. >>> Source/WebKit/chromium/tests/TiledLayerChromiumTest.cpp:-354 >>> - EXPECT_FALSE(layerImpl2->hasTileAt(0, 2)); >> >> This layer used to get a single tile, now I see it expecting 2 tiles, though the comment contradicts. What happened here? > > I think this was a throwback to when there was different memory limits for visible tiles, and the original code/comment is wrong. See above "layer1 exhausts most texture memory, leaving room for 2 more tiles from layer2". > > The old code was just lucky to work because it only did one pre-paint pass without checking needsIdlePaint. The new code will fail when it tries the paint the last tile. > > I'll fix the last comment. done. >> Source/WebKit/chromium/tests/TiledLayerChromiumTest.cpp:600 >> + > > new whitespace done. Comment on attachment 158907 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=158907&action=review > Source/WebCore/platform/graphics/chromium/TiledLayerChromium.h:86 > // Prepare data needed to update textures that intersect with contentRect. > - void updateContentRect(CCTextureUpdateQueue&, const IntRect& contentRect, const CCOcclusionTracker*, CCRenderingStats&); > + void updateContent(CCTextureUpdateQueue&, const CCOcclusionTracker*, CCRenderingStats&); Comment out of date. Also, it kind of seems like this function should just be virtual void update(...) OVERRIDE instead of the different function updateContent, since it takes the same parameters and is essentially doing the same thing. This would let you eliminate FakeTiledLayerChromium::update too. > Source/WebKit/chromium/tests/TiledLayerChromiumTest.cpp:81 > + , m_textureManager(CCPrioritizedTextureManager::create(60*1024*1024, 1024, CCRenderer::ContentPool)) > + , m_occlusion(0) Ooh, nice. :) > Source/WebKit/chromium/tests/TiledLayerChromiumTest.cpp:98 > + bool InvalidateUpdateAndPush(FakeTiledLayerChromium* layer1, I know this reduces total lines of code, but I find this change to be less readable. There's a shedload of implicit parameter values and now callsites become so ambiguous that you need to leave '// invalidateRect' and '// visibleRect' comments to make it clear. There may be some way to clean up repeated code here, but I don't think this is it. > Source/WebKit/chromium/tests/TiledLayerChromiumTest.cpp:411 > - // We should only have the first tile from layer2 since it failed to idle update. > + // We should only have the first two tiles from layer2 since > + // it failed to idle update the last tile. Why does this change in this patch? Comment on attachment 158907 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=158907&action=review >> Source/WebCore/platform/graphics/chromium/TiledLayerChromium.h:86 >> + void updateContent(CCTextureUpdateQueue&, const CCOcclusionTracker*, CCRenderingStats&); > > Comment out of date. Also, it kind of seems like this function should just be virtual void update(...) OVERRIDE instead of the different function updateContent, since it takes the same parameters and is essentially doing the same thing. This would let you eliminate FakeTiledLayerChromium::update too. Ahh, didn't realize it had been simplified to that point already. >> Source/WebKit/chromium/tests/TiledLayerChromiumTest.cpp:98 >> + bool InvalidateUpdateAndPush(FakeTiledLayerChromium* layer1, > > I know this reduces total lines of code, but I find this change to be less readable. There's a shedload of implicit parameter values and now callsites become so ambiguous that you need to leave '// invalidateRect' and '// visibleRect' comments to make it clear. There may be some way to clean up repeated code here, but I don't think this is it. Yeah, this is overboard. Would it help if I did the following? - Move the commit flow (UpdateAndPush) part into it's own function that just takes the layers as parameters. - Set the parameters individually, or via setup functions with clear names. The reason I clumped all of this in here is that the tests are all very similar. They appear to depend on a lot of control flow, but actually in the end they all boil down to data driven input->output tests once the commit flow is removed, so maybe they can start to be merged into fewer, more data-driven tests. >> Source/WebKit/chromium/tests/TiledLayerChromiumTest.cpp:411 >> + // it failed to idle update the last tile. > > Why does this change in this patch? I think it was just lucky to work before. See the comment at the top "layer1 exhausts most texture memory, leaving room for *2* more tiles from layer2, but not all three tiles". So it was just the lack of pre-painting passes to paint that extra tile, and lack of checking needsIdlePaint() to insure pre-painting was finished. Created attachment 159952 [details]
Patch
Comment on attachment 158907 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=158907&action=review >>> Source/WebCore/platform/graphics/chromium/TiledLayerChromium.h:86 >>> + void updateContent(CCTextureUpdateQueue&, const CCOcclusionTracker*, CCRenderingStats&); >> >> Comment out of date. Also, it kind of seems like this function should just be virtual void update(...) OVERRIDE instead of the different function updateContent, since it takes the same parameters and is essentially doing the same thing. This would let you eliminate FakeTiledLayerChromium::update too. > > Ahh, didn't realize it had been simplified to that point already. Done. >>> Source/WebKit/chromium/tests/TiledLayerChromiumTest.cpp:98 >>> + bool InvalidateUpdateAndPush(FakeTiledLayerChromium* layer1, >> >> I know this reduces total lines of code, but I find this change to be less readable. There's a shedload of implicit parameter values and now callsites become so ambiguous that you need to leave '// invalidateRect' and '// visibleRect' comments to make it clear. There may be some way to clean up repeated code here, but I don't think this is it. > > Yeah, this is overboard. Would it help if I did the following? > - Move the commit flow (UpdateAndPush) part into it's own function that just takes the layers as parameters. > - Set the parameters individually, or via setup functions with clear names. > > The reason I clumped all of this in here is that the tests are all very similar. They appear to depend on a lot of control flow, but actually in the end they all boil down to data driven input->output tests once the commit flow is removed, so maybe they can start to be merged into fewer, more data-driven tests. Done. Same or even further code reduction without this mess. Comment on attachment 159952 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=159952&action=review R=me. > Source/WebKit/chromium/tests/TiledLayerChromiumTest.cpp:98 > + bool UpdateAndPush(FakeTiledLayerChromium* layer1, Thanks for changing this. This makes way more sense to me. s/UpdateAndPush/updateAndPush/ Created attachment 159982 [details]
Patch
Comment on attachment 159982 [details]
Patch
R=me(again).
Comment on attachment 159952 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=159952&action=review >> Source/WebKit/chromium/tests/TiledLayerChromiumTest.cpp:98 >> + bool UpdateAndPush(FakeTiledLayerChromium* layer1, > > Thanks for changing this. This makes way more sense to me. > > s/UpdateAndPush/updateAndPush/ Oops, done. Comment on attachment 159952 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=159952&action=review >>> Source/WebKit/chromium/tests/TiledLayerChromiumTest.cpp:98 >>> + bool UpdateAndPush(FakeTiledLayerChromium* layer1, >> >> Thanks for changing this. This makes way more sense to me. >> >> s/UpdateAndPush/updateAndPush/ > > Oops, done. I like this lots more too, nice job eric. (In reply to comment #17) > (From update of attachment 159952 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=159952&action=review > > >>> Source/WebKit/chromium/tests/TiledLayerChromiumTest.cpp:98 > >>> + bool UpdateAndPush(FakeTiledLayerChromium* layer1, > >> > >> Thanks for changing this. This makes way more sense to me. > >> > >> s/UpdateAndPush/updateAndPush/ > > > > Oops, done. > > I like this lots more too, nice job eric. It started reasonable but grew to be a big mess by the time I was done changing several tests! Thanks for the wake-up call on that, much better now. Comment on attachment 159982 [details] Patch Clearing flags on attachment: 159982 Committed r126340: <http://trac.webkit.org/changeset/126340> All reviewed patches have been landed. Closing bug. |