Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
616543004:
cc: Use visible_rect_for_tile_priority_ where approriate (Closed)
Descriptioncc: Use visible_rect_for_tile_priority_ where approriate
|visible_rect_for_tile_priority_| was added for Android WebView to
replace content_visible_rect(). The latter may not always be valid for
in Android WebView for tile prioritization or activation considerations.
This partially reverts https://codereview.chromium.org/517893002
BUG=418834
Committed: https://crrev.com/7473f7f5b1f5eec2c161bf26f0545e87e321da53
Cr-Commit-Position: refs/heads/master@{#297659}
Patch Set 1 #
Total comments: 1
Patch Set 2 : partial revert of https://codereview.chromium.org/517893002 #
Total comments: 14
Patch Set 3 : review #Patch Set 4 : UpdateTiles #
Total comments: 7
Patch Set 5 : review #
Total comments: 2
Patch Set 6 : Offset #
Messages
Total messages: 27 (2 generated)
PTAL I'm up for debate for a better name than "_for_tile_prioritization_", since this CL is all concerned about activation logic. But feels like we had this name debate before...
boliu@chromium.org changed reviewers: + danakj@chromium.org
PTAL I'm up for debate for a better name than "_for_tile_prioritization_", since this CL is all concerned about activation logic. But feels like we had this name debate before...
https://codereview.chromium.org/616543004/diff/1/cc/layers/picture_layer_impl.cc File cc/layers/picture_layer_impl.cc (right): https://codereview.chromium.org/616543004/diff/1/cc/layers/picture_layer_impl... cc/layers/picture_layer_impl.cc:454: visible_rect_for_tile_priority_ = visible_content_rect(); Realized still needs to check for resourceless_software_draw for this line. So need to partially revert https://codereview.chromium.org/517893002 :( My bad for not thinking about this case, generating all this churn. I'll do this tomorrow
PTAL. Added the partial revert of https://codereview.chromium.org/517893002
https://codereview.chromium.org/616543004/diff/20001/cc/layers/picture_layer_... File cc/layers/picture_layer_impl.cc (right): https://codereview.chromium.org/616543004/diff/20001/cc/layers/picture_layer_... cc/layers/picture_layer_impl.cc:455: viewport_rect_for_tile_priority_ = the assign before this is completely overridden here, do you mean if else, or what's going on here?
https://codereview.chromium.org/616543004/diff/20001/cc/layers/picture_layer_... File cc/layers/picture_layer_impl.cc (right): https://codereview.chromium.org/616543004/diff/20001/cc/layers/picture_layer_... cc/layers/picture_layer_impl.cc:455: viewport_rect_for_tile_priority_ = On 2014/09/30 20:38:37, danakj wrote: > the assign before this is completely overridden here, do you mean if else, or > what's going on here? oh visible/viewport, reading how does it work?
https://codereview.chromium.org/616543004/diff/20001/cc/layers/picture_layer_... File cc/layers/picture_layer_impl.cc (right): https://codereview.chromium.org/616543004/diff/20001/cc/layers/picture_layer_... cc/layers/picture_layer_impl.cc:1506: gfx::Rect rect(visible_rect_for_tile_priority_); Why isn't this using GetViewportForTilePriorityInContentSpace() intersect this rect, like when we mark required? can you explain either way the background for this decision? Maybe this should be a comment if it should be different from marking. https://codereview.chromium.org/616543004/diff/20001/cc/layers/picture_layer_... File cc/layers/picture_layer_impl_unittest.cc (right): https://codereview.chromium.org/616543004/diff/20001/cc/layers/picture_layer_... cc/layers/picture_layer_impl_unittest.cc:487: active_layer_->visible_rect_for_tile_priority()); comment to explain this expectation https://codereview.chromium.org/616543004/diff/20001/cc/layers/picture_layer_... cc/layers/picture_layer_impl_unittest.cc:505: EXPECT_FALSE(visible_rect_for_tile_priority == EXPECT_NE can you find something to EXPECT_EQ instead? or show these are EQ before your last SetExternalDrawConstraints? https://codereview.chromium.org/616543004/diff/20001/cc/layers/picture_layer_... cc/layers/picture_layer_impl_unittest.cc:1441: pending_layer_->set_visible_rect_for_tile_priority(gfx::Rect(0, 0, 100, 200)); can you set the viewport on the host_impl_ instead and have UpdateDrawProps create this instead of adding a setter for this test? and just EXPECT_EQ the visible_rect_for_t_p() here? https://codereview.chromium.org/616543004/diff/20001/cc/layers/picture_layer_... cc/layers/picture_layer_impl_unittest.cc:3228: EXPECT_FALSE(visible_rect_for_tile_priority == same comments
https://codereview.chromium.org/616543004/diff/20001/cc/layers/picture_layer_... File cc/layers/picture_layer_impl.cc (right): https://codereview.chromium.org/616543004/diff/20001/cc/layers/picture_layer_... cc/layers/picture_layer_impl.cc:452: if (!layer_tree_impl()->resourceless_software_draw()) { can we pass this to UpdateTiles instead of making it call back to LayerTreeImpl, where it just came from?
https://codereview.chromium.org/616543004/diff/20001/cc/layers/picture_layer_... File cc/layers/picture_layer_impl.cc (right): https://codereview.chromium.org/616543004/diff/20001/cc/layers/picture_layer_... cc/layers/picture_layer_impl.cc:1506: gfx::Rect rect(visible_rect_for_tile_priority_); On 2014/09/30 21:05:53, danakj wrote: > Why isn't this using GetViewportForTilePriorityInContentSpace() intersect this > rect, like when we mark required? can you explain either way the background for > this decision? Maybe this should be a comment if it should be different from > marking. omg this is an existing bug! It should be GetViewportForTilePriorityInContentSpace afaict. I missed this method when in this change, when I changed the activation rect in https://codereview.chromium.org/475233002/ Looks like no one has noticed yet...
Did not do the UpdateTiles change yet in PS3. All other comments addressed I think. https://codereview.chromium.org/616543004/diff/20001/cc/layers/picture_layer_... File cc/layers/picture_layer_impl.cc (right): https://codereview.chromium.org/616543004/diff/20001/cc/layers/picture_layer_... cc/layers/picture_layer_impl.cc:452: if (!layer_tree_impl()->resourceless_software_draw()) { On 2014/09/30 21:06:37, danakj wrote: > can we pass this to UpdateTiles instead of making it call back to LayerTreeImpl, > where it just came from? This is *not* done yet in PS3. Looks like going to involve a bit of churn, so gonna do it on top of PS3 and see how this goes. https://codereview.chromium.org/616543004/diff/20001/cc/layers/picture_layer_... File cc/layers/picture_layer_impl_unittest.cc (right): https://codereview.chromium.org/616543004/diff/20001/cc/layers/picture_layer_... cc/layers/picture_layer_impl_unittest.cc:487: active_layer_->visible_rect_for_tile_priority()); On 2014/09/30 21:05:53, danakj wrote: > comment to explain this expectation Done. https://codereview.chromium.org/616543004/diff/20001/cc/layers/picture_layer_... cc/layers/picture_layer_impl_unittest.cc:505: EXPECT_FALSE(visible_rect_for_tile_priority == On 2014/09/30 21:05:53, danakj wrote: > EXPECT_NE > > can you find something to EXPECT_EQ instead? or show these are EQ before your > last SetExternalDrawConstraints? Yes, found things to EXPECT_EQ to. Although need to do the translate(1,1) to match transfrom above by hand here. https://codereview.chromium.org/616543004/diff/20001/cc/layers/picture_layer_... cc/layers/picture_layer_impl_unittest.cc:1441: pending_layer_->set_visible_rect_for_tile_priority(gfx::Rect(0, 0, 100, 200)); On 2014/09/30 21:05:53, danakj wrote: > can you set the viewport on the host_impl_ instead and have UpdateDrawProps > create this instead of adding a setter for this test? > > and just EXPECT_EQ the visible_rect_for_t_p() here? Done. https://codereview.chromium.org/616543004/diff/20001/cc/layers/picture_layer_... cc/layers/picture_layer_impl_unittest.cc:3228: EXPECT_FALSE(visible_rect_for_tile_priority == On 2014/09/30 21:05:53, danakj wrote: > same comments Done same as above.
And PS4 passes resourceless draw into UpdateTiles directly from LTI. Not as much chrun as I thought, most of it in test files :)
Thanks this LGTM w/ 1 concern about the name https://codereview.chromium.org/616543004/diff/50001/cc/layers/picture_layer_... File cc/layers/picture_layer_impl.cc (right): https://codereview.chromium.org/616543004/diff/50001/cc/layers/picture_layer_... cc/layers/picture_layer_impl.cc:445: bool draw_properties_valid) { nit: this name seems like a bit of a like, like the screen_space_transform() is still used for instance. or what do you think? maybe visible_content_rect_is_valid? or can you explain why this name is suitable? https://codereview.chromium.org/616543004/diff/50001/cc/layers/picture_layer_... File cc/layers/picture_layer_impl_unittest.cc (right): https://codereview.chromium.org/616543004/diff/50001/cc/layers/picture_layer_... cc/layers/picture_layer_impl_unittest.cc:488: EXPECT_RECT_EQ(viewport, active_layer_->viewport_rect_for_tile_priority()); nit: you can EXPECT_EQ instead of EXPECT_RECT_EQ now everywhere https://codereview.chromium.org/616543004/diff/50001/cc/layers/picture_layer_... cc/layers/picture_layer_impl_unittest.cc:515: EXPECT_RECT_EQ(viewport, viewport_rect_in_layer); nit: slightly prefer modifying the expected instead of the actual value: EXPECT_EQ(viewport - gfx::Vector2d(1, 1), viewport_rect_in_layer);
https://codereview.chromium.org/616543004/diff/50001/cc/layers/picture_layer_... File cc/layers/picture_layer_impl.cc (right): https://codereview.chromium.org/616543004/diff/50001/cc/layers/picture_layer_... cc/layers/picture_layer_impl.cc:445: bool draw_properties_valid) { On 2014/10/01 02:05:14, danakj wrote: > nit: this name seems like a bit of a like, like the screen_space_transform() is > still used for instance. or what do you think? maybe > visible_content_rect_is_valid? or can you explain why this name is suitable? a bit of a lie* i mean
https://codereview.chromium.org/616543004/diff/50001/cc/layers/picture_layer_... File cc/layers/picture_layer_impl.cc (right): https://codereview.chromium.org/616543004/diff/50001/cc/layers/picture_layer_... cc/layers/picture_layer_impl.cc:445: bool draw_properties_valid) { On 2014/10/01 02:05:37, danakj wrote: > On 2014/10/01 02:05:14, danakj wrote: > > nit: this name seems like a bit of a like, like the screen_space_transform() > is > > still used for instance. or what do you think? maybe > > visible_content_rect_is_valid? or can you explain why this name is suitable? > > a bit of a lie* i mean Umm, external_draw_constraints_invalid? But only the "draw" constraints are invalid: transform, viewport, clip.
https://codereview.chromium.org/616543004/diff/50001/cc/layers/picture_layer_... File cc/layers/picture_layer_impl.cc (right): https://codereview.chromium.org/616543004/diff/50001/cc/layers/picture_layer_... cc/layers/picture_layer_impl.cc:445: bool draw_properties_valid) { On 2014/10/01 02:14:58, boliu wrote: > On 2014/10/01 02:05:37, danakj wrote: > > On 2014/10/01 02:05:14, danakj wrote: > > > nit: this name seems like a bit of a like, like the screen_space_transform() > > is > > > still used for instance. or what do you think? maybe > > > visible_content_rect_is_valid? or can you explain why this name is suitable? > > > > a bit of a lie* i mean > > Umm, external_draw_constraints_invalid? But only the "draw" constraints are > invalid: transform, viewport, clip. From the POV of this function it seems like the viewport is always valid. If an external viewport is set, it'll use it no matter what this bool says. I don't see clip in here anywhere, where do I look for that?
https://codereview.chromium.org/616543004/diff/50001/cc/layers/picture_layer_... File cc/layers/picture_layer_impl.cc (right): https://codereview.chromium.org/616543004/diff/50001/cc/layers/picture_layer_... cc/layers/picture_layer_impl.cc:445: bool draw_properties_valid) { On 2014/10/01 02:23:42, danakj wrote: > On 2014/10/01 02:14:58, boliu wrote: > > On 2014/10/01 02:05:37, danakj wrote: > > > On 2014/10/01 02:05:14, danakj wrote: > > > > nit: this name seems like a bit of a like, like the > screen_space_transform() > > > is > > > > still used for instance. or what do you think? maybe > > > > visible_content_rect_is_valid? or can you explain why this name is > suitable? > > > > > > a bit of a lie* i mean > > > > Umm, external_draw_constraints_invalid? But only the "draw" constraints are > > invalid: transform, viewport, clip. > > From the POV of this function it seems like the viewport is always valid. If an > external viewport is set, it'll use it no matter what this bool says. I don't > see clip in here anywhere, where do I look for that? I was referring to LTHI::SetExternalDrawConstraints parameters. |viewport|, |clip|, and |transform| are used for draws and can't be used here. Of course the two "_for_tile_priority" parameters are ok. visible_content_rect() is derived from |viewport|. Anything else in draw properties that are derived from the "external draw constraints" are not ok either, in theory. And I guess this also means we should s/SetExternalDrawConstraints/SetExternalConstraints/
On 2014/10/01 02:28:47, boliu wrote: > https://codereview.chromium.org/616543004/diff/50001/cc/layers/picture_layer_... > File cc/layers/picture_layer_impl.cc (right): > > https://codereview.chromium.org/616543004/diff/50001/cc/layers/picture_layer_... > cc/layers/picture_layer_impl.cc:445: bool draw_properties_valid) { > On 2014/10/01 02:23:42, danakj wrote: > > On 2014/10/01 02:14:58, boliu wrote: > > > On 2014/10/01 02:05:37, danakj wrote: > > > > On 2014/10/01 02:05:14, danakj wrote: > > > > > nit: this name seems like a bit of a like, like the > > screen_space_transform() > > > > is > > > > > still used for instance. or what do you think? maybe > > > > > visible_content_rect_is_valid? or can you explain why this name is > > suitable? > > > > > > > > a bit of a lie* i mean > > > > > > Umm, external_draw_constraints_invalid? But only the "draw" constraints are > > > invalid: transform, viewport, clip. > > > > From the POV of this function it seems like the viewport is always valid. If > an > > external viewport is set, it'll use it no matter what this bool says. I don't > > see clip in here anywhere, where do I look for that? > > I was referring to LTHI::SetExternalDrawConstraints parameters. |viewport|, > |clip|, and |transform| are used for draws and can't be used here. Of course the > two "_for_tile_priority" parameters are ok. > > visible_content_rect() is derived from |viewport|. Anything else in draw > properties that are derived from the "external draw constraints" are not ok > either, in theory. > > And I guess this also means we should > s/SetExternalDrawConstraints/SetExternalConstraints/ Ah.... hm. It's complicated. it's like draw_properties_derived_from_external_draw_constraints_valid. I think we need to do a bit better than that. You could potentially name this variable "resourceless_software_draw" or whatever it's called in LTI. And then add a big comment explaining all of these things we've discussed in here. I think that's the best I've got right now to avoid confusion/abiguity. wdyt?
On 2014/10/01 02:52:26, danakj wrote: > Ah.... hm. It's complicated. it's like > draw_properties_derived_from_external_draw_constraints_valid. I think we need to > do a bit better than that. > > You could potentially name this variable "resourceless_software_draw" or > whatever it's called in LTI. And then add a big comment explaining all of these > things we've discussed in here. I think that's the best I've got right now to > avoid confusion/abiguity. wdyt? sgtm
On 2014/10/01 02:55:42, boliu wrote: > On 2014/10/01 02:52:26, danakj wrote: > > Ah.... hm. It's complicated. it's like > > draw_properties_derived_from_external_draw_constraints_valid. I think we need > to > > do a bit better than that. > > > > You could potentially name this variable "resourceless_software_draw" or > > whatever it's called in LTI. And then add a big comment explaining all of > these > > things we've discussed in here. I think that's the best I've got right now to > > avoid confusion/abiguity. wdyt? > > sgtm And all done. Shoved the big comment in PictureLayerImpl::UpdateTiles.
On 2014/10/01 03:12:15, boliu wrote: > On 2014/10/01 02:55:42, boliu wrote: > > On 2014/10/01 02:52:26, danakj wrote: > > > Ah.... hm. It's complicated. it's like > > > draw_properties_derived_from_external_draw_constraints_valid. I think we > need > > to > > > do a bit better than that. > > > > > > You could potentially name this variable "resourceless_software_draw" or > > > whatever it's called in LTI. And then add a big comment explaining all of > > these > > > things we've discussed in here. I think that's the best I've got right now > to > > > avoid confusion/abiguity. wdyt? > > > > sgtm > > And all done. Shoved the big comment in PictureLayerImpl::UpdateTiles. Oh and reminder that "resourceless_software_draw" basically reversed the bool condition from PS4
LGTM https://codereview.chromium.org/616543004/diff/70001/cc/layers/picture_layer_... File cc/layers/picture_layer_impl_unittest.cc (right): https://codereview.chromium.org/616543004/diff/70001/cc/layers/picture_layer_... cc/layers/picture_layer_impl_unittest.cc:3251: viewport_rect_in_layer.Offset(1, 1); // Match translate in |transform|. can you make this look the same as what you did above?
Thanks for the review :) https://codereview.chromium.org/616543004/diff/70001/cc/layers/picture_layer_... File cc/layers/picture_layer_impl_unittest.cc (right): https://codereview.chromium.org/616543004/diff/70001/cc/layers/picture_layer_... cc/layers/picture_layer_impl_unittest.cc:3251: viewport_rect_in_layer.Offset(1, 1); // Match translate in |transform|. On 2014/10/01 14:27:14, danakj wrote: > can you make this look the same as what you did above? Oops. Done.
The CQ bit was checked by boliu@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/616543004/90001
Message was sent while issue was closed.
Committed patchset #6 (id:90001) as 3e0c0967d9f5386a3b35f4c3e1799a7f7c64342e
Message was sent while issue was closed.
Patchset 6 (id:??) landed as https://crrev.com/7473f7f5b1f5eec2c161bf26f0545e87e321da53 Cr-Commit-Position: refs/heads/master@{#297659} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
