[gap-decorations] Implement Paint for multicol GapDecorations [chromium/src : main]
Javier Contreras (Gerrit)
New activity on the change![]( "Open in Gerrit")
Related details
- Alison Maher
- Sam Davis Omekara
Code-Coverage
Code-Owners
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Alison Maher (Gerrit)
Alison Maher added 10 comments![]( "Open in Gerrit")
spanner_main_gaps_indices_.push_back(main_gaps_.size() - 1);Instead of storing these in a temp var, can we just emplace them directly in the gap geometry instead?
void AdvanceMulticolRunningIndices(bool& should_add_content_end) const;This could likely use a comment
void SetSpannerMainGapsIndices(Vector<wtf_size_t>&& indices) {As mentioned above, maybe AddSpannerMainGapIndex() and emplace back here directly
// of each column gap that intersects this main gap, namely this the"is"
if (GetMainGaps()[gap_index].IsSpannerMainGap()) {
return Vector<LayoutUnit>();
}If we don't use these at paint, do we need to store them at all?
const CrossGap& cross_gap = GetCrossGaps()[i];nit: var not needed, can be plugged in directly below
if (next_spanner_main_gap_index_ < spanner_main_gaps_indices_.size() &&
main_gaps_[main_gap_running_index_].HasCrossGapsBefore() &&
gap_index >
main_gaps_[main_gap_running_index_].GetCrossGapBeforeEnd()) {This check is quite complex and could likely use a comment
if (spanner_main_gaps_indices_.empty() ||
next_spanner_main_gap_index_ == kNotFound) {
end_main_gap = main_gaps_.size() - 1;
should_add_content_end = true;
} else {
CHECK_LT(next_spanner_main_gap_index_,
spanner_main_gaps_indices_.size());
end_main_gap = spanner_main_gaps_indices_[next_spanner_main_gap_index_];
}
CHECK_LT(end_main_gap, main_gaps_.size());
for (wtf_size_t i = main_gap_running_index_; i <= end_main_gap; ++i) {
intersections.push_back(main_gaps_[i].GetGapOffset());
}What if there are multiple spanners? Will this work?
Or are we only getting the cross intersections for the current main gap?
if (main_gap_running_index_ == main_gaps_.size() - 1 &&
main_gaps_.back().IsSpannerMainGap()) {
should_add_content_end = true;
}Do we need this as an out param or can we do this in the method above?
// Make sure we skip any multicol `MainGap`s generated by spanners.Might be worth expanding on why
Related details
- Javier Contreras
- Sam Davis Omekara
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Javier Contreras (Gerrit)
Javier Contreras added 10 comments![]( "Open in Gerrit")
spanner_main_gaps_indices_.push_back(main_gaps_.size() - 1);Instead of storing these in a temp var, can we just emplace them directly in the gap geometry instead?
The gap geometry is not created until after we have finish laying everything out, and this is the pattern we use in flex and grid as well, so we can't do this.
void AdvanceMulticolRunningIndices(bool& should_add_content_end) const;This could likely use a comment
Done
void SetSpannerMainGapsIndices(Vector<wtf_size_t>&& indices) {As mentioned above, maybe AddSpannerMainGapIndex() and emplace back here directly
Ditto as my response above, the gap geometry isn't created until after we have finished laying out.
// of each column gap that intersects this main gap, namely this theJavier Contreras"is"
Done
if (GetMainGaps()[gap_index].IsSpannerMainGap()) {
return Vector<LayoutUnit>();
}If we don't use these at paint, do we need to store them at all?
We don't paint them, but we do use them at paint. They are used to know from where to where a CrossGap should be painted. See `GenereateCrossIntersectionList` below for the multicol case.
nit: var not needed, can be plugged in directly below
Done
if (next_spanner_main_gap_index_ < spanner_main_gaps_indices_.size() &&
main_gaps_[main_gap_running_index_].HasCrossGapsBefore() &&
gap_index >
main_gaps_[main_gap_running_index_].GetCrossGapBeforeEnd()) {This check is quite complex and could likely use a comment
Done
if (spanner_main_gaps_indices_.empty() ||
next_spanner_main_gap_index_ == kNotFound) {
end_main_gap = main_gaps_.size() - 1;
should_add_content_end = true;
} else {
CHECK_LT(next_spanner_main_gap_index_,
spanner_main_gaps_indices_.size());
end_main_gap = spanner_main_gaps_indices_[next_spanner_main_gap_index_];
}
CHECK_LT(end_main_gap, main_gaps_.size());
for (wtf_size_t i = main_gap_running_index_; i <= end_main_gap; ++i) {
intersections.push_back(main_gaps_[i].GetGapOffset());
}What if there are multiple spanners? Will this work?
Or are we only getting the cross intersections for the current main gap?
It will work, there are two tests `multicol-gap-decorations-015.html` and `-016.html` that test similar scenarios.
`AdvanceMulticolRunningIndices` is what is used to advance the indices (with which we decide what `end_main_gap` should be) so that we know from what main gap to what main gap we should paint.
if (main_gap_running_index_ == main_gaps_.size() - 1 &&
main_gaps_.back().IsSpannerMainGap()) {
should_add_content_end = true;
}Do we need this as an out param or can we do this in the method above?
need it as an out param since it needs to be modified inside this loop sometimes, and read/used outside of this method
// Make sure we skip any multicol `MainGap`s generated by spanners.Might be worth expanding on why
- Alison Maher
- Sam Davis Omekara
Code-Coverage
Code-Owners
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Alison Maher (Gerrit)
Alison Maher voted and added 6 comments![]( "Open in Gerrit")
Votes added by Alison Maher
| Code-Review | +1 |
6 comments
spanner_main_gaps_indices_.push_back(main_gaps_.size() - 1);Javier ContrerasInstead of storing these in a temp var, can we just emplace them directly in the gap geometry instead?
The gap geometry is not created until after we have finish laying everything out, and this is the pattern we use in flex and grid as well, so we can't do this.
Something we can maybe discuss offline and doesn't need to be in this change, but I wonder if we should just create the gap geometry up front across the board, then we can emplace these directly there, and then at the end finalize any remaining info needed. But maybe there is a good reason for the temp var that I'm blanking on
// towards the next spanner main gap that we will paint until.nit: "up until"?
// move `main_gap_running_index_` forward as we progress.
// After we are done painting a cross gap that goes from one spanner to thenit: This can either be re-wrapped or add a // spacer between the two paragraphs if meant to be a separate paragraph
if (main_gap_running_index_ == main_gaps_.size() - 1 &&
main_gaps_.back().IsSpannerMainGap()) {
should_add_content_end = true;
}Javier ContrerasDo we need this as an out param or can we do this in the method above?
need it as an out param since it needs to be modified inside this loop sometimes, and read/used outside of this method
To confirm, if we hit this, is there a chance the loop will continue? I would have guessed this only happens at the end, in which case, we could check this outside the loop
// These is because those `MainGap`s are not painted, only used to generatenit: and
// These is because those `MainGap`s are not painted, only used to generatenit: "This"
Related details
- Javier Contreras
- Sam Davis Omekara
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Sam Davis Omekara (Gerrit)
Sam Davis Omekara voted and added 1 comment![]( "Open in Gerrit")
Votes added by Sam Davis Omekara
| Code-Review | +1 |
1 comment
if (track_direction == kForRows &&
gap_index < gap_geometry.GetMainGaps().size() &&
gap_geometry.GetMainGaps()[gap_index].IsSpannerMainGap()) {
continue;
}I believe the intent here is to avoid painting spanners as main gaps for multi col.
It might be neater if gap_geometry tells us this. Something along the lines of,
```
GapGeometry::IsMultiColSpanner(track_direction) {
if (container_type == MultiCol && IsMainDirection(track_direction))
{ return main_gaps_[gap_index].IsSpannerMainGap();
}
return false;
}
```
We would then use `gap_geometry.IsMultiColSpanner()` and `continue` accordingly.
Related details
- Javier Contreras
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Sam Davis Omekara (Gerrit)
Sam Davis Omekara added 1 comment![]( "Open in Gerrit")
# These tests are failing for Multicol because they are testing rule breaking behaviorActually re-looking at these, the code for painting with rule breaking behavior has landed so I'm guessing these should pass now right?
Javier Contreras (Gerrit)
Javier Contreras added 8 comments![]( "Open in Gerrit")
spanner_main_gaps_indices_.push_back(main_gaps_.size() - 1);Javier ContrerasInstead of storing these in a temp var, can we just emplace them directly in the gap geometry instead?
Alison MaherThe gap geometry is not created until after we have finish laying everything out, and this is the pattern we use in flex and grid as well, so we can't do this.
Something we can maybe discuss offline and doesn't need to be in this change, but I wonder if we should just create the gap geometry up front across the board, then we can emplace these directly there, and then at the end finalize any remaining info needed. But maybe there is a good reason for the temp var that I'm blanking on
I do agree with this though, I do think we should instead go that route but also can't remember exactly why we originally went down this route. Adding this to the bug for clean up tasks.
// towards the next spanner main gap that we will paint until.Javier Contrerasnit: "up until"?
Done
// move `main_gap_running_index_` forward as we progress.
// After we are done painting a cross gap that goes from one spanner to thenit: This can either be re-wrapped or add a // spacer between the two paragraphs if meant to be a separate paragraph
Done
if (main_gap_running_index_ == main_gaps_.size() - 1 &&
main_gaps_.back().IsSpannerMainGap()) {
should_add_content_end = true;
}Javier ContrerasDo we need this as an out param or can we do this in the method above?
Alison Maherneed it as an out param since it needs to be modified inside this loop sometimes, and read/used outside of this method
To confirm, if we hit this, is there a chance the loop will continue? I would have guessed this only happens at the end, in which case, we could check this outside the loop
Ah I see what you meant, I thought you meant in the method above as in outside this method.
// These is because those `MainGap`s are not painted, only used to generateJavier Contrerasnit: "This"
Done
// These is because those `MainGap`s are not painted, only used to generateJavier Contrerasnit: and
Done
if (track_direction == kForRows &&
gap_index < gap_geometry.GetMainGaps().size() &&
gap_geometry.GetMainGaps()[gap_index].IsSpannerMainGap()) {
continue;
}I believe the intent here is to avoid painting spanners as main gaps for multi col.
It might be neater if gap_geometry tells us this. Something along the lines of,
```
GapGeometry::IsMultiColSpanner(track_direction) {
if (container_type == MultiCol && IsMainDirection(track_direction))
{ return main_gaps_[gap_index].IsSpannerMainGap();
}return false;
}
```We would then use `gap_geometry.IsMultiColSpanner()` and `continue` accordingly.
Great idea, much cleaner.
# These tests are failing for Multicol because they are testing rule breaking behaviorActually re-looking at these, the code for painting with rule breaking behavior has landed so I'm guessing these should pass now right?
They do not pass, there is some tweaking needed to the code so that they pass. I am addressing that in a follow up CL.
Related details
Code-Coverage
Code-Owners
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |