Bug 234578 – Grid may be empty in certain scenarios

• Component: WebCore Misc.
• Status: RESOLVED FIXED

[Brandon]

Grid may be empty in certain scenarios

[Brandon]

<rdar://86153015>

[Brandon]

Created attachment 447760 [details]
Patch

[Sergio Villar Senin]

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.

[Sergio Villar Senin]

(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.

[Brandon]

Created attachment 449432 [details]
patch

[Sergio Villar Senin]

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.

[Brandon]

Created attachment 449536 [details]
patch

[Brandon]

(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

[Sergio Villar Senin]

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...

[Brandon]

Created attachment 450546 [details]
Patch

[Brandon]

Created attachment 450557 [details]
Patch

[Sergio Villar Senin]

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".

[Brandon]

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

[Brandon]

Created attachment 450712 [details]
Patch

[Darin Adler]

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?

[Darin Adler]

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.

[Brandon]

Created attachment 450834 [details]
Patch

[Darin Adler]

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.

[Darin Adler]

Comment on attachment 450834 [details]
Patch

Still no test. Need to include a test demonstrating what this fixes.

[Brandon]

Created attachment 450932 [details]
Patch

[Brandon]

(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.

[Darin Adler]

Still no test. Need to include a test demonstrating what this fixes.

[Darin Adler]

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.

[Brandon]

Created attachment 451288 [details]
Patch

[Brandon]

(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.

[Brandon]

Created attachment 451295 [details]
Patch for landing

[Darin Adler]

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.

[Brandon]

Created attachment 451302 [details]
Patch

[EWS]

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].