Grid may be empty in certain scenarios
<rdar://86153015>
Created attachment 447760 [details] Patch
Comment on attachment 447760 [details] Patch This is not correct as this should never happen. If there are no grid items then we cannot have items in the asepectRatioBlockSizeDependentGridItems Vector. If you're hitting this scenario then we are not properly handling some weird scenario. You have to look for the root of the cause.
(In reply to Sergio Villar Senin from comment #3) > Comment on attachment 447760 [details] > Patch > > This is not correct as this should never happen. If there are no grid items > then we cannot have items in the asepectRatioBlockSizeDependentGridItems > Vector. If you're hitting this scenario then we are not properly handling > some weird scenario. You have to look for the root of the cause. For the *root cause of the issue* I meant.
Created attachment 449432 [details] patch
Comment on attachment 449432 [details] patch View in context: https://bugs.webkit.org/attachment.cgi?id=449432&action=review There is one test failing. Check it out. > Source/WebCore/rendering/RenderGrid.cpp:178 > Perhaps add a comment here explaining that this must be run after calling updateLogicalWidth() otherwise the shouldSkipChild() won't properly work. It'd be better to have an ASSERT for it, but I don't think it'd be easy to come up with any. > Source/WebCore/rendering/RenderGrid.cpp:179 > +void RenderGrid::computeAspectRatioDependantAndBaselineItems(Vector<RenderBox*> &aspectRatioBlockSizeDependentGridItems) Nit: the & should be placed right next to the type not the variable name > Source/WebCore/rendering/RenderGrid.h:194 > + void computeAspectRatioDependantAndBaselineItems(Vector<RenderBox*> &aspectRatioBlockSizeDependentGridItems); Nit: the & should be right next to the type.
Created attachment 449536 [details] patch
(In reply to Sergio Villar Senin from comment #6) > Comment on attachment 449432 [details] > patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=449432&action=review > > There is one test failing. Check it out. > > > Source/WebCore/rendering/RenderGrid.cpp:178 > > > > Perhaps add a comment here explaining that this must be run after calling > updateLogicalWidth() otherwise the shouldSkipChild() won't properly work. > It'd be better to have an ASSERT for it, but I don't think it'd be easy to > come up with any. added a comment here > > > Source/WebCore/rendering/RenderGrid.cpp:179 > > +void RenderGrid::computeAspectRatioDependantAndBaselineItems(Vector<RenderBox*> &aspectRatioBlockSizeDependentGridItems) > > Nit: the & should be placed right next to the type not the variable name fixed > > > Source/WebCore/rendering/RenderGrid.h:194 > > + void computeAspectRatioDependantAndBaselineItems(Vector<RenderBox*> &aspectRatioBlockSizeDependentGridItems); > > Nit: the & should be right next to the type. fixed
Comment on attachment 449536 [details] patch View in context: https://bugs.webkit.org/attachment.cgi?id=449536&action=review It's unfortunate that we need to add an extra loop to the layout phase which could potentially worsen the performance. I'd like to know about the weird interaction between setting the height and clearing the overriding sizes before approving this. > Source/WebCore/rendering/RenderGrid.cpp:179 > +void RenderGrid::clearChildrenOverridingLogicialHeight() Logicial -> Logical > Source/WebCore/rendering/RenderGrid.cpp:182 > + // The clearOverridingLogicalHeight() affects the behavior of resetLogicalHeightBeforeLayoutIfNeeded(). That seems weird. The resetLogical... only sets the height to 0. How that could interact with clearing the overriding sizes is not clear to me...
Created attachment 450546 [details] Patch
Created attachment 450557 [details] Patch
Comment on attachment 450557 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=450557&action=review > Source/WebCore/ChangeLog:8 > + Include check if a child should be excluded from the layout. Let's improve this message. I'd write something like "Use OrderIterator's shouldSkipChild() to filter out items that should not be included in the aspect ratio dependant nor in the baseline lists. Just using isOutOfFlowPositioned() is not enough as there are some other cases of direct child of a grid element that are not grid items.". Feel free to use your own words and/or improve the grammar. > Source/WebCore/rendering/RenderGrid.cpp:189 > +void RenderGrid::clearChildrenOverridingLogicalHeight() Nit: since we're filtering out non-grid items (like OOF children) I'd call this clearGridItemsOverriding... > Source/WebCore/rendering/RenderGrid.cpp:223 > + child->clearOverridingLogicalHeight(); This block is duplicated > Source/WebCore/rendering/RenderGrid.cpp:240 > + // context during the track sizing algorithm. I know this comes from the original code, but let's use longer lines instead of wrapping text so many times. I think we can use 3 lines instead of the current 6. > Source/WebCore/rendering/RenderGrid.cpp:270 > + // instead of here. Perhaps append "to save an extra loop over all grid items".
Comment on attachment 450557 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=450557&action=review >> Source/WebCore/ChangeLog:8 >> + Include check if a child should be excluded from the layout. > > Let's improve this message. I'd write something like "Use OrderIterator's shouldSkipChild() to filter out items that should not be included in the aspect ratio dependant nor in the baseline lists. Just using isOutOfFlowPositioned() is not enough as there are some other cases of direct child of a grid element that are not grid items.". Feel free to use your own words and/or improve the grammar. Updated the description. >> Source/WebCore/rendering/RenderGrid.cpp:189 >> +void RenderGrid::clearChildrenOverridingLogicalHeight() > > Nit: since we're filtering out non-grid items (like OOF children) I'd call this clearGridItemsOverriding... Updated. >> Source/WebCore/rendering/RenderGrid.cpp:223 >> + child->clearOverridingLogicalHeight(); > > This block is duplicated Removed. Thanks for catching this. >> Source/WebCore/rendering/RenderGrid.cpp:240 >> + // context during the track sizing algorithm. > > I know this comes from the original code, but let's use longer lines instead of wrapping text so many times. I think we can use 3 lines instead of the current 6. Reduced it down to 4 lines. 120 character line lengths. >> Source/WebCore/rendering/RenderGrid.cpp:270 >> + // instead of here. > > Perhaps append "to save an extra loop over all grid items". Updated comment
Created attachment 450712 [details] Patch
Comment on attachment 450712 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=450712&action=review > Source/WebCore/ChangeLog:3 > + Grid may be empty in certain scenarios Why no new tests and no change in test results?
Comment on attachment 450712 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=450712&action=review >> Source/WebCore/ChangeLog:3 >> + Grid may be empty in certain scenarios > > Why no new tests and no change in test results? Let me say this more clearly.:WebKit patches that change behavior must include regression tests covering the improvement. Please submit a new patch that includes a test.
Created attachment 450834 [details] Patch
Comment on attachment 450834 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=450834&action=review > Source/WebCore/rendering/RenderGrid.h:194 > + void computeAspectRatioDependantAndBaselineItems(Vector<RenderBox*>& aspectRatioBlockSizeDependentGridItems); This should use a return value rather than an out argument. Also, "dependent" is misspelled here.
Comment on attachment 450834 [details] Patch Still no test. Need to include a test demonstrating what this fixes.
Created attachment 450932 [details] Patch
(In reply to Darin Adler from comment #18) > Comment on attachment 450834 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=450834&action=review > > > Source/WebCore/rendering/RenderGrid.h:194 > > + void computeAspectRatioDependantAndBaselineItems(Vector<RenderBox*>& aspectRatioBlockSizeDependentGridItems); > > This should use a return value rather than an out argument. > > Also, "dependent" is misspelled here. Updated the function to return the grid items and addressed the spelling mistake.
Still no test. Need to include a test demonstrating what this fixes.
Comment on attachment 450932 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=450932&action=review > Source/WebCore/rendering/RenderGrid.cpp:191 > + Vector<RenderBox*> aspectRatioBlockSizeDependentGridItems; I don’t think our local vector needs such a long name.
Created attachment 451288 [details] Patch
(In reply to Darin Adler from comment #23) > Comment on attachment 450932 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=450932&action=review > > > Source/WebCore/rendering/RenderGrid.cpp:191 > > + Vector<RenderBox*> aspectRatioBlockSizeDependentGridItems; > > I don’t think our local vector needs such a long name. Shortened the local vector name.
Created attachment 451295 [details] Patch for landing
Comment on attachment 451295 [details] Patch for landing View in context: https://bugs.webkit.org/attachment.cgi?id=451295&action=review > Source/WebCore/ChangeLog:6 > + Reviewed by NOBODY (OOPS!). You need to put the reviewer name here, otherwise commit-queue will not land it.
Created attachment 451302 [details] Patch
Committed r289437 (246990@main): <https://commits.webkit.org/246990@main> All reviewed patches have been landed. Closing bug and clearing flags on attachment 451302 [details].