Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
537943003:
Releasing Track & Thumb UIResources based on their Geometry (Closed)
DescriptionReleasing Track & Thumb UIResources based on their Geometry
When the track rect or scaled track rect is empty, UiResources of Track
and Thumb resources are released, instead of holding in the memory.
These UIResources will be created on PaintedScrollbarLayer::Update when
the Track rect is not empty. Similary When Scrollbar doesnt have Thumb
release UIResources of Thumb.
Bug=410845
Committed: https://crrev.com/8eff0dbb847db5a65dbc6c89466e6c72ec9f11a8
Cr-Commit-Position: refs/heads/master@{#295515}
Patch Set 1 #Patch Set 2 : UI resources changed so push properties is needed #Patch Set 3 : Releasing Track & Thumb UIResources based on their Geometry #
Total comments: 2
Patch Set 4 : Addressed review comments #
Total comments: 16
Patch Set 5 : Addressed Review comments #
Total comments: 2
Patch Set 6 : addressed review comments and modified unittest #
Total comments: 4
Patch Set 7 : updated review comments #
Total comments: 8
Patch Set 8 : Addressed review comments #
Total comments: 1
Messages
Total messages: 29 (9 generated)
sataya.m@samsung.com changed reviewers: + danakj@chromium.org
PTAL. Thanks,
Patchset #3 (id:40001) has been deleted
https://codereview.chromium.org/537943003/diff/60001/cc/layers/painted_scroll... File cc/layers/painted_scrollbar_layer.cc (right): https://codereview.chromium.org/537943003/diff/60001/cc/layers/painted_scroll... cc/layers/painted_scrollbar_layer.cc:212: SetNeedsPushProperties(); these need to make Update() return true also, so I think this needs to integrate with the Update() early outs. Also, tests for this?
Patchset #4 (id:80001) has been deleted
addressed review comments https://codereview.chromium.org/537943003/diff/60001/cc/layers/painted_scroll... File cc/layers/painted_scrollbar_layer.cc (right): https://codereview.chromium.org/537943003/diff/60001/cc/layers/painted_scroll... cc/layers/painted_scrollbar_layer.cc:212: SetNeedsPushProperties(); On 2014/09/04 16:38:10, danakj wrote: > these need to make Update() return true also, so I think this needs to integrate > with the Update() early outs. > > Also, tests for this? Done.Tests Added.
On 2014/09/05 14:44:13, muven wrote: > addressed review comments > > https://codereview.chromium.org/537943003/diff/60001/cc/layers/painted_scroll... > File cc/layers/painted_scrollbar_layer.cc (right): > > https://codereview.chromium.org/537943003/diff/60001/cc/layers/painted_scroll... > cc/layers/painted_scrollbar_layer.cc:212: SetNeedsPushProperties(); > On 2014/09/04 16:38:10, danakj wrote: > > these need to make Update() return true also, so I think this needs to > integrate > > with the Update() early outs. > > > > Also, tests for this? > > Done.Tests Added. friendly ping
https://codereview.chromium.org/537943003/diff/100001/cc/layers/painted_scrol... File cc/layers/painted_scrollbar_layer.cc (right): https://codereview.chromium.org/537943003/diff/100001/cc/layers/painted_scrol... cc/layers/painted_scrollbar_layer.cc:202: ReleaseUIResources(ScrollbarUIResource::Thumb); can you move this into the Update() method after calling this? I feel like this method should only do what its name says. https://codereview.chromium.org/537943003/diff/100001/cc/layers/painted_scrol... cc/layers/painted_scrollbar_layer.cc:207: ScrollbarUIResource ui_resource) { how about "ReleaseThumbAndMaybeTrack(bool release_track)" instead of an enum and switch? https://codereview.chromium.org/537943003/diff/100001/cc/layers/scrollbar_lay... File cc/layers/scrollbar_layer_unittest.cc (right): https://codereview.chromium.org/537943003/diff/100001/cc/layers/scrollbar_lay... cc/layers/scrollbar_layer_unittest.cc:629: class MockLayerTreeHost : public LayerTreeHost { (probably wanna rename this to Fake to avoid name colliding with an actual mock class) https://codereview.chromium.org/537943003/diff/100001/cc/layers/scrollbar_lay... cc/layers/scrollbar_layer_unittest.cc:810: scrollbar_layer->draw_properties().render_target = scrollbar_layer.get(); make this = the root layer https://codereview.chromium.org/537943003/diff/100001/cc/layers/scrollbar_lay... cc/layers/scrollbar_layer_unittest.cc:819: scrollbar_layer->Update(&queue, &occlusion_tracker); can you verify this is calling SetNeedsPushProperties() also? see layer_unittest.cc for example of a MockLayerTreeHost to test similar things. https://codereview.chromium.org/537943003/diff/100001/cc/layers/scrollbar_lay... cc/layers/scrollbar_layer_unittest.cc:831: EXPECT_NE(0, scrollbar_layer->track_resource_id()); can we verify that we push the resource and push a 0 to the impl side appropriately also? maybe see the TiledLayer tests for examples. https://codereview.chromium.org/537943003/diff/100001/cc/layers/scrollbar_lay... cc/layers/scrollbar_layer_unittest.cc:836: EXPECT_NE(0, scrollbar_layer->track_resource_id()); make sure track can be released too? https://codereview.chromium.org/537943003/diff/100001/cc/layers/scrollbar_lay... cc/layers/scrollbar_layer_unittest.cc:851: TestResourceRelease(); what's the reason for the class indirection here? can't its member variables just be locals in the test and all the code lives here?
Patchset #4 (id:100001) has been deleted
Patchset #4 (id:120001) has been deleted
Hi Dana, In confusion i have deleted the patch uploaded previously where the review comments where given. Sorry for that. I have uploaded the patch with comments given by you. I couldnt rewrite test case checking the SetNeedsPushProperties and checking UIResource set to 0 from IMPL side as FakePaintedScrollbarLayer doesnt have IMPL implementation. If you need that i shall write the FakePaintedScrollbarLayer file. Please share your inputs. Once again sorry for deleting the previous patch. Thanks, https://codereview.chromium.org/537943003/diff/140001/cc/layers/painted_scrol... File cc/layers/painted_scrollbar_layer.cc (right): https://codereview.chromium.org/537943003/diff/140001/cc/layers/painted_scrol... cc/layers/painted_scrollbar_layer.cc:218: } To make it simple i have added the reset of the UIResources without having any seperate function for this. Comments by Dana: can you move this into the Update() method after calling this? I feel like this method should only do what its name says. how about "ReleaseThumbAndMaybeTrack(bool release_track)" instead of an enum and switch? https://codereview.chromium.org/537943003/diff/140001/cc/layers/scrollbar_lay... File cc/layers/scrollbar_layer_unittest.cc (right): https://codereview.chromium.org/537943003/diff/140001/cc/layers/scrollbar_lay... cc/layers/scrollbar_layer_unittest.cc:629: class FakeLayerTreeHost : public LayerTreeHost { Done. Comments by dana: (probably wanna rename this to Fake to avoid name colliding with an actual mock class) https://codereview.chromium.org/537943003/diff/140001/cc/layers/scrollbar_lay... cc/layers/scrollbar_layer_unittest.cc:788: scrollbar_layer->Update(&queue, &occlusion_tracker); FakePaintedScrollbarLayer doesnt have Impl implementation to check this. If you want me to do that i shall write FakePaintedScrollbarLayerImpl implementation for this. Comments by dana: can you verify this is calling SetNeedsPushProperties() also? see layer_unittest.cc for example of a MockLayerTreeHost to test similar things. https://codereview.chromium.org/537943003/diff/140001/cc/layers/scrollbar_lay... cc/layers/scrollbar_layer_unittest.cc:789: EXPECT_NE(0, scrollbar_layer->track_resource_id()); Need IMPL implementation for this. comments by dana: can we verify that we push the resource and push a 0 to the impl side appropriately also? maybe see the TiledLayer tests for examples. https://codereview.chromium.org/537943003/diff/140001/cc/layers/scrollbar_lay... cc/layers/scrollbar_layer_unittest.cc:805: EXPECT_NE(0, scrollbar_layer->track_resource_id()); I feel it should be present to give user an idea that on resizing window thumb will be showed on the track.If you are ok to do that we can remove track even. comments by dana: make sure track can be released too? https://codereview.chromium.org/537943003/diff/140001/cc/layers/scrollbar_lay... cc/layers/scrollbar_layer_unittest.cc:841: TestResourceRelease(); I have added the test function to ScrollbarLayerTestResourceCreation, to keep creation and release test case should be under one roof. comments by dana: what's the reason for the class indirection here? can't its member variables just be locals in the test and all the code lives here?
Thanks I like the inline release better, it is easier to see what's going on. https://codereview.chromium.org/537943003/diff/140001/cc/layers/painted_scrol... File cc/layers/painted_scrollbar_layer.cc (right): https://codereview.chromium.org/537943003/diff/140001/cc/layers/painted_scrol... cc/layers/painted_scrollbar_layer.cc:214: if (track_resource_ && thumb_resource_) { && seems wrong here, if the track is the only thing present, we still want to free it when the track rect is empty? can you add a test case for this? https://codereview.chromium.org/537943003/diff/140001/cc/layers/painted_scrol... cc/layers/painted_scrollbar_layer.cc:234: return false; if we dropped resources above, we should return true, so that the commit goes thru and we push them (same as at the bottom of this method) https://codereview.chromium.org/537943003/diff/140001/cc/layers/scrollbar_lay... File cc/layers/scrollbar_layer_unittest.cc (right): https://codereview.chromium.org/537943003/diff/140001/cc/layers/scrollbar_lay... cc/layers/scrollbar_layer_unittest.cc:788: scrollbar_layer->Update(&queue, &occlusion_tracker); On 2014/09/10 15:40:58, muven wrote: > FakePaintedScrollbarLayer doesnt have Impl implementation to check this. If you > want me to do that i shall write FakePaintedScrollbarLayerImpl implementation > for this. > > Comments by dana: > can you verify this is calling SetNeedsPushProperties() also? see > layer_unittest.cc for example of a MockLayerTreeHost to test similar things. how about verifying the Update return value? https://codereview.chromium.org/537943003/diff/140001/cc/layers/scrollbar_lay... cc/layers/scrollbar_layer_unittest.cc:805: EXPECT_NE(0, scrollbar_layer->track_resource_id()); On 2014/09/10 15:40:58, muven wrote: > I feel it should be present to give user an idea that on resizing window thumb > will be showed on the track.If you are ok to do that we can remove track even. > > comments by dana: > make sure track can be released too? Oh,I agree. I meant another test case that shows the track being freed, right now you only show it being created and that the thumb resource can be freed https://codereview.chromium.org/537943003/diff/140001/cc/layers/scrollbar_lay... cc/layers/scrollbar_layer_unittest.cc:841: TestResourceRelease(); On 2014/09/10 15:40:58, muven wrote: > I have added the test function to ScrollbarLayerTestResourceCreation, to keep > creation and release test case should be under one roof. > > comments by dana: > what's the reason for the class indirection here? can't its member variables > just be locals in the test and all the code lives here? The TestResourceUpdate code makes sense in the class since the test calls it with different inputs. I'd rather just put this test inside the TEST_F() { } as that's a lot easier to read than having to redirect to some method in a class above.
Hi Dana, PTAL. Thanks, https://codereview.chromium.org/537943003/diff/140001/cc/layers/painted_scrol... File cc/layers/painted_scrollbar_layer.cc (right): https://codereview.chromium.org/537943003/diff/140001/cc/layers/painted_scrol... cc/layers/painted_scrollbar_layer.cc:214: if (track_resource_ && thumb_resource_) { On 2014/09/10 16:15:03, danakj wrote: > && seems wrong here, if the track is the only thing present, we still want to > free it when the track rect is empty? can you add a test case for this? Done. https://codereview.chromium.org/537943003/diff/140001/cc/layers/painted_scrol... cc/layers/painted_scrollbar_layer.cc:234: return false; On 2014/09/10 16:15:03, danakj wrote: > if we dropped resources above, we should return true, so that the commit goes > thru and we push them (same as at the bottom of this method) Done. https://codereview.chromium.org/537943003/diff/140001/cc/layers/scrollbar_lay... File cc/layers/scrollbar_layer_unittest.cc (right): https://codereview.chromium.org/537943003/diff/140001/cc/layers/scrollbar_lay... cc/layers/scrollbar_layer_unittest.cc:788: scrollbar_layer->Update(&queue, &occlusion_tracker); Instead of verifying on Update i have verified on layer_tree_host_->UIResourceCount, TotalUIResourceCreated, TotalUIResourceDeleted. As we are returning true in Update Api upon reset of resources, i felt this way we can verify it as done in the above method TestResourceUpload. On 2014/09/10 16:15:03, danakj wrote: > On 2014/09/10 15:40:58, muven wrote: > > FakePaintedScrollbarLayer doesnt have Impl implementation to check this. If > you > > want me to do that i shall write FakePaintedScrollbarLayerImpl implementation > > for this. > > > > Comments by dana: > > can you verify this is calling SetNeedsPushProperties() also? see > > layer_unittest.cc for example of a MockLayerTreeHost to test similar things. > > how about verifying the Update return value? https://codereview.chromium.org/537943003/diff/140001/cc/layers/scrollbar_lay... cc/layers/scrollbar_layer_unittest.cc:805: EXPECT_NE(0, scrollbar_layer->track_resource_id()); On 2014/09/10 16:15:03, danakj wrote: > On 2014/09/10 15:40:58, muven wrote: > > I feel it should be present to give user an idea that on resizing window thumb > > will be showed on the track.If you are ok to do that we can remove track even. > > > > comments by dana: > > make sure track can be released too? > > Oh,I agree. I meant another test case that shows the track being freed, right > now you only show it being created and that the thumb resource can be freed Done. https://codereview.chromium.org/537943003/diff/140001/cc/layers/scrollbar_lay... cc/layers/scrollbar_layer_unittest.cc:841: TestResourceRelease(); On 2014/09/10 16:15:03, danakj wrote: > On 2014/09/10 15:40:58, muven wrote: > > I have added the test function to ScrollbarLayerTestResourceCreation, to keep > > creation and release test case should be under one roof. > > > > comments by dana: > > what's the reason for the class indirection here? can't its member variables > > just be locals in the test and all the code lives here? > > The TestResourceUpdate code makes sense in the class since the test calls it > with different inputs. I'd rather just put this test inside the TEST_F() { } as > that's a lot easier to read than having to redirect to some method in a class > above. Done.
https://codereview.chromium.org/537943003/diff/160001/cc/layers/painted_scrol... File cc/layers/painted_scrollbar_layer.cc (right): https://codereview.chromium.org/537943003/diff/160001/cc/layers/painted_scrol... cc/layers/painted_scrollbar_layer.cc:220: return true; but this should return false if you didn't SetNeedsPushProperties or you force a commit to happen when you have nothing new to commit. That's why I was asking you to check the return value of Update() in your tests.
PTAL. Thanks, https://codereview.chromium.org/537943003/diff/160001/cc/layers/painted_scrol... File cc/layers/painted_scrollbar_layer.cc (right): https://codereview.chromium.org/537943003/diff/160001/cc/layers/painted_scrol... cc/layers/painted_scrollbar_layer.cc:220: return true; On 2014/09/11 17:22:01, danakj wrote: > but this should return false if you didn't SetNeedsPushProperties or you force a > commit to happen when you have nothing new to commit. That's why I was asking > you to check the return value of Update() in your tests. Done.
https://codereview.chromium.org/537943003/diff/180001/cc/layers/painted_scrol... File cc/layers/painted_scrollbar_layer.cc (right): https://codereview.chromium.org/537943003/diff/180001/cc/layers/painted_scrol... cc/layers/painted_scrollbar_layer.cc:212: bool needs_update = false; nit: updated https://codereview.chromium.org/537943003/diff/180001/cc/layers/painted_scrol... cc/layers/painted_scrollbar_layer.cc:228: return needs_update; not supposed to return here, you have a track rect and may need to update it.
PTAL. thanks. https://codereview.chromium.org/537943003/diff/180001/cc/layers/painted_scrol... File cc/layers/painted_scrollbar_layer.cc (right): https://codereview.chromium.org/537943003/diff/180001/cc/layers/painted_scrol... cc/layers/painted_scrollbar_layer.cc:212: bool needs_update = false; On 2014/09/11 20:12:56, danakj wrote: > nit: updated Done. https://codereview.chromium.org/537943003/diff/180001/cc/layers/painted_scrol... cc/layers/painted_scrollbar_layer.cc:228: return needs_update; Done. On 2014/09/11 20:12:56, danakj wrote: > not supposed to return here, you have a track rect and may need to update it.
https://codereview.chromium.org/537943003/diff/200001/cc/layers/painted_scrol... File cc/layers/painted_scrollbar_layer.cc (right): https://codereview.chromium.org/537943003/diff/200001/cc/layers/painted_scrol... cc/layers/painted_scrollbar_layer.cc:226: SetNeedsPushProperties(); Thanks, can you add a test that will exercise this code branch and verify we didn't early return? How about first have a track and a thumb, then resize the track and SetNeedsDisplay(), and remove the thumb, and make sure the track resource was resized? You could use LayerTreeHost::GetUIResourceSize to do this. https://codereview.chromium.org/537943003/diff/200001/cc/layers/scrollbar_lay... File cc/layers/scrollbar_layer_unittest.cc (right): https://codereview.chromium.org/537943003/diff/200001/cc/layers/scrollbar_lay... cc/layers/scrollbar_layer_unittest.cc:818: EXPECT_EQ(true, scrollbar_layer->Update(&queue, &occlusion_tracker)); EXPECT_TRUE https://codereview.chromium.org/537943003/diff/200001/cc/layers/scrollbar_lay... cc/layers/scrollbar_layer_unittest.cc:824: EXPECT_EQ(false, scrollbar_layer->Update(&queue, &occlusion_tracker)); EXPECT_FALSE https://codereview.chromium.org/537943003/diff/200001/cc/layers/scrollbar_lay... cc/layers/scrollbar_layer_unittest.cc:826: scrollbar_layer->fake_scrollbar()->set_track_rect(gfx::Rect(30, 10, 50, 10)); Why did you remove the checks for the resources being created/destroyed?
Patchset #8 (id:220001) has been deleted
PTAL. Updated review comments. https://codereview.chromium.org/537943003/diff/200001/cc/layers/painted_scrol... File cc/layers/painted_scrollbar_layer.cc (right): https://codereview.chromium.org/537943003/diff/200001/cc/layers/painted_scrol... cc/layers/painted_scrollbar_layer.cc:226: SetNeedsPushProperties(); On 2014/09/15 18:16:27, danakj wrote: > Thanks, can you add a test that will exercise this code branch and verify we > didn't early return? How about first have a track and a thumb, then resize the > track and SetNeedsDisplay(), and remove the thumb, and make sure the track > resource was resized? You could use LayerTreeHost::GetUIResourceSize to do this. Done. https://codereview.chromium.org/537943003/diff/200001/cc/layers/scrollbar_lay... File cc/layers/scrollbar_layer_unittest.cc (right): https://codereview.chromium.org/537943003/diff/200001/cc/layers/scrollbar_lay... cc/layers/scrollbar_layer_unittest.cc:818: EXPECT_EQ(true, scrollbar_layer->Update(&queue, &occlusion_tracker)); On 2014/09/15 18:16:27, danakj wrote: > EXPECT_TRUE Done. https://codereview.chromium.org/537943003/diff/200001/cc/layers/scrollbar_lay... cc/layers/scrollbar_layer_unittest.cc:824: EXPECT_EQ(false, scrollbar_layer->Update(&queue, &occlusion_tracker)); On 2014/09/15 18:16:27, danakj wrote: > EXPECT_FALSE Done. https://codereview.chromium.org/537943003/diff/200001/cc/layers/scrollbar_lay... cc/layers/scrollbar_layer_unittest.cc:826: scrollbar_layer->fake_scrollbar()->set_track_rect(gfx::Rect(30, 10, 50, 10)); On 2014/09/15 18:16:27, danakj wrote: > Why did you remove the checks for the resources being created/destroyed? Done.
The CQ bit was checked by sataya.m@samsung.com
The CQ bit was unchecked by sataya.m@samsung.com
On 2014/09/16 13:34:31, muven wrote: > PTAL. Updated review comments. > > https://codereview.chromium.org/537943003/diff/200001/cc/layers/painted_scrol... > File cc/layers/painted_scrollbar_layer.cc (right): > > https://codereview.chromium.org/537943003/diff/200001/cc/layers/painted_scrol... > cc/layers/painted_scrollbar_layer.cc:226: SetNeedsPushProperties(); > On 2014/09/15 18:16:27, danakj wrote: > > Thanks, can you add a test that will exercise this code branch and verify we > > didn't early return? How about first have a track and a thumb, then resize the > > track and SetNeedsDisplay(), and remove the thumb, and make sure the track > > resource was resized? You could use LayerTreeHost::GetUIResourceSize to do > this. > > Done. > > https://codereview.chromium.org/537943003/diff/200001/cc/layers/scrollbar_lay... > File cc/layers/scrollbar_layer_unittest.cc (right): > > https://codereview.chromium.org/537943003/diff/200001/cc/layers/scrollbar_lay... > cc/layers/scrollbar_layer_unittest.cc:818: EXPECT_EQ(true, > scrollbar_layer->Update(&queue, &occlusion_tracker)); > On 2014/09/15 18:16:27, danakj wrote: > > EXPECT_TRUE > > Done. > > https://codereview.chromium.org/537943003/diff/200001/cc/layers/scrollbar_lay... > cc/layers/scrollbar_layer_unittest.cc:824: EXPECT_EQ(false, > scrollbar_layer->Update(&queue, &occlusion_tracker)); > On 2014/09/15 18:16:27, danakj wrote: > > EXPECT_FALSE > > Done. > > https://codereview.chromium.org/537943003/diff/200001/cc/layers/scrollbar_lay... > cc/layers/scrollbar_layer_unittest.cc:826: > scrollbar_layer->fake_scrollbar()->set_track_rect(gfx::Rect(30, 10, 50, 10)); > On 2014/09/15 18:16:27, danakj wrote: > > Why did you remove the checks for the resources being created/destroyed? > > Done. friendly ping.
LGTM https://codereview.chromium.org/537943003/diff/240001/cc/layers/scrollbar_lay... File cc/layers/scrollbar_layer_unittest.cc (right): https://codereview.chromium.org/537943003/diff/240001/cc/layers/scrollbar_lay... cc/layers/scrollbar_layer_unittest.cc:828: EXPECT_EQ(expected_created, layer_tree_host_->TotalUIResourceCreated()); oh these checks are great, thanks.
On 2014/09/18 18:04:46, danakj wrote: > LGTM > > https://codereview.chromium.org/537943003/diff/240001/cc/layers/scrollbar_lay... > File cc/layers/scrollbar_layer_unittest.cc (right): > > https://codereview.chromium.org/537943003/diff/240001/cc/layers/scrollbar_lay... > cc/layers/scrollbar_layer_unittest.cc:828: EXPECT_EQ(expected_created, > layer_tree_host_->TotalUIResourceCreated()); > oh these checks are great, thanks. thanks for the review :)
The CQ bit was checked by sataya.m@samsung.com
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/537943003/240001
Message was sent while issue was closed.
Committed patchset #8 (id:240001) as fa8ce90188b0fb2e87f3976a9157f9f559a7b302
Message was sent while issue was closed.
Patchset 8 (id:??) landed as https://crrev.com/8eff0dbb847db5a65dbc6c89466e6c72ec9f11a8 Cr-Commit-Position: refs/heads/master@{#295515} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
