[Masonry] Plumb break token data through the layout algorithm [chromium/src : main]
Morten Stenshorne (Gerrit)
Morten Stenshorne added 1 comment![]( "Open in Gerrit")
Just wondering... would it be possible to take this CL as step further, so that it actually adds some behavior changes, and a test or two (or seven)? When adding new code, it's always nice if it's also testable somehow.
Related details
- Alison Maher
- Ian Kilpatrick
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Alison Maher (Gerrit)
Alison Maher added 1 comment![]( "Open in Gerrit")
Just wondering... would it be possible to take this CL as step further, so that it actually adds some behavior changes, and a test or two (or seven)? When adding new code, it's always nice if it's also testable somehow.
Good question!
Technically some of the existing container fragmentation WPT tests are hitting this code (since I hit a crash when the `total_intrinsic_block_size` wasn't set correctly when setting up the fragmentation block in this change) but since we don't do anything interesting with the data yet, this one probably isn't super interesting to test on its own.
I was planning to aggregate the GridLaneData vector in the next CL which I think will be interesting to add unit tests for to make sure that is aggregating as expected. Happy to lump that in here, but figured this may be easier to read the diff without that.
I can also wait to land this one until I've gotten that follow up in place and have it reviewed with those tests (to keep the reviews scoped, but have tests in place exercising this path more explicitly in the follow up)
Related details
- Ian Kilpatrick
- Morten Stenshorne
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Morten Stenshorne (Gerrit)
Morten Stenshorne added 1 comment![]( "Open in Gerrit")
Alison MaherJust wondering... would it be possible to take this CL as step further, so that it actually adds some behavior changes, and a test or two (or seven)? When adding new code, it's always nice if it's also testable somehow.
Good question!
Technically some of the existing container fragmentation WPT tests are hitting this code (since I hit a crash when the `total_intrinsic_block_size` wasn't set correctly when setting up the fragmentation block in this change) but since we don't do anything interesting with the data yet, this one probably isn't super interesting to test on its own.
I was planning to aggregate the GridLaneData vector in the next CL which I think will be interesting to add unit tests for to make sure that is aggregating as expected. Happy to lump that in here, but figured this may be easier to read the diff without that.
I can also wait to land this one until I've gotten that follow up in place and have it reviewed with those tests (to keep the reviews scoped, but have tests in place exercising this path more explicitly in the follow up)
Maybe try lumping it in, and I'll see if I can wrap my head around it. 😊
Related details
- Alison Maher
- Ian Kilpatrick
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Alison Maher (Gerrit)
Alison Maher added 1 comment![]( "Open in Gerrit")
Alison MaherJust wondering... would it be possible to take this CL as step further, so that it actually adds some behavior changes, and a test or two (or seven)? When adding new code, it's always nice if it's also testable somehow.
Morten StenshorneGood question!
Technically some of the existing container fragmentation WPT tests are hitting this code (since I hit a crash when the `total_intrinsic_block_size` wasn't set correctly when setting up the fragmentation block in this change) but since we don't do anything interesting with the data yet, this one probably isn't super interesting to test on its own.
I was planning to aggregate the GridLaneData vector in the next CL which I think will be interesting to add unit tests for to make sure that is aggregating as expected. Happy to lump that in here, but figured this may be easier to read the diff without that.
I can also wait to land this one until I've gotten that follow up in place and have it reviewed with those tests (to keep the reviews scoped, but have tests in place exercising this path more explicitly in the follow up)
Maybe try lumping it in, and I'll see if I can wrap my head around it. 😊
So I uploaded the follow up branch I have https://chromium-review.googlesource.com/c/chromium/src/+/8135980 and this one ended up getting pretty large, so I split out a few TODOs from that one for dense packing, alignment and reversal data handling for even further future CLs.
It also ended up being a bit weird to test since we now skip adding the item layout results in that branch, deferring to a later placement pass. I added a hacky way to test with a TODO to update once we can rely on the full layout run.
Lmk once you've glanced it over if you'd still prefer to merge these and I can CP that change over here tomorrow
Related details
- Ian Kilpatrick
- Morten Stenshorne
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Morten Stenshorne (Gerrit)
Morten Stenshorne added 1 comment![]( "Open in Gerrit")
Alison MaherJust wondering... would it be possible to take this CL as step further, so that it actually adds some behavior changes, and a test or two (or seven)? When adding new code, it's always nice if it's also testable somehow.
Morten StenshorneGood question!
Technically some of the existing container fragmentation WPT tests are hitting this code (since I hit a crash when the `total_intrinsic_block_size` wasn't set correctly when setting up the fragmentation block in this change) but since we don't do anything interesting with the data yet, this one probably isn't super interesting to test on its own.
I was planning to aggregate the GridLaneData vector in the next CL which I think will be interesting to add unit tests for to make sure that is aggregating as expected. Happy to lump that in here, but figured this may be easier to read the diff without that.
I can also wait to land this one until I've gotten that follow up in place and have it reviewed with those tests (to keep the reviews scoped, but have tests in place exercising this path more explicitly in the follow up)
Alison MaherMaybe try lumping it in, and I'll see if I can wrap my head around it. 😊
So I uploaded the follow up branch I have https://chromium-review.googlesource.com/c/chromium/src/+/8135980 and this one ended up getting pretty large, so I split out a few TODOs from that one for dense packing, alignment and reversal data handling for even further future CLs.
It also ended up being a bit weird to test since we now skip adding the item layout results in that branch, deferring to a later placement pass. I added a hacky way to test with a TODO to update once we can rely on the full layout run.
Lmk once you've glanced it over if you'd still prefer to merge these and I can CP that change over here tomorrow
I don't think this looks too scary. Just lump it together, and I'll review it.
Related details
- Alison Maher
- Ian Kilpatrick
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Alison Maher (Gerrit)
Alison Maher added 1 comment![]( "Open in Gerrit")
Alison MaherJust wondering... would it be possible to take this CL as step further, so that it actually adds some behavior changes, and a test or two (or seven)? When adding new code, it's always nice if it's also testable somehow.
Morten StenshorneGood question!
Technically some of the existing container fragmentation WPT tests are hitting this code (since I hit a crash when the `total_intrinsic_block_size` wasn't set correctly when setting up the fragmentation block in this change) but since we don't do anything interesting with the data yet, this one probably isn't super interesting to test on its own.
I was planning to aggregate the GridLaneData vector in the next CL which I think will be interesting to add unit tests for to make sure that is aggregating as expected. Happy to lump that in here, but figured this may be easier to read the diff without that.
I can also wait to land this one until I've gotten that follow up in place and have it reviewed with those tests (to keep the reviews scoped, but have tests in place exercising this path more explicitly in the follow up)
Alison MaherMaybe try lumping it in, and I'll see if I can wrap my head around it. 😊
Morten StenshorneSo I uploaded the follow up branch I have https://chromium-review.googlesource.com/c/chromium/src/+/8135980 and this one ended up getting pretty large, so I split out a few TODOs from that one for dense packing, alignment and reversal data handling for even further future CLs.
It also ended up being a bit weird to test since we now skip adding the item layout results in that branch, deferring to a later placement pass. I added a hacky way to test with a TODO to update once we can rely on the full layout run.
Lmk once you've glanced it over if you'd still prefer to merge these and I can CP that change over here tomorrow
I don't think this looks too scary. Just lump it together, and I'll review it.
Sounds good, cherry picked over and updated the description to reflect the combined changes
Related details
- Ian Kilpatrick
- Morten Stenshorne
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Morten Stenshorne (Gerrit)
Morten Stenshorne voted and added 2 comments![]( "Open in Gerrit")
Votes added by Morten Stenshorne
| Code-Review | +1 |
2 comments
grid_layout_subtree =Only used if inside block fragmentation. Maybe only create it then?
if (!out_grid_lanes) {Maybe explain this in a comment?
Related details
- Alison Maher
- Ian Kilpatrick
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Alison Maher (Gerrit)
Alison Maher voted and added 2 comments![]( "Open in Gerrit")
Votes added by Alison Maher
| Commit-Queue | +2 |
2 comments
Only used if inside block fragmentation. Maybe only create it then?
Good point, moved!
Maybe explain this in a comment?
- Ian Kilpatrick
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Alison Maher (Gerrit)
Alison Maher voted Commit-Queue+2![]( "Open in Gerrit")
| Commit-Queue | +2 |