Bug 251816 – REGRESSION (258819@main): Wolfram Alpha dropdown box has repaint artifacts

• Status: RESOLVED
• Resolution: FIXED
• Component: Layout and Rendering

[zalan]

<rdar://104966985>

[zalan]

Created attachment 464872 [details]
Patch

[Simon Fraser (smfr)]

Comment on attachment 464872 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=464872&action=review

> Source/WebCore/rendering/RenderBlockFlow.cpp:3762
> +            // However at this point with such high IFC coverage, the subsequent layout is very likely to be IFC again.

This is the kind of comment that will go stale fairly fast. I'd suggest rewriting this in a way that is more future-proof.

[zalan]

(In reply to Simon Fraser (smfr) from comment #2)
> Comment on attachment 464872 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=464872&action=review
> 
> > Source/WebCore/rendering/RenderBlockFlow.cpp:3762
> > +            // However at this point with such high IFC coverage, the subsequent layout is very likely to be IFC again.
> 
> This is the kind of comment that will go stale fairly fast. I'd suggest
> rewriting this in a way that is more future-proof.
I agree such comments may go stale fast in general, but I am somewhat convinced it's not going to be the case here (i.e. we don't go back to performing more legacy line layout) and also in practice this part of the code will be completely removed/restructured very soon with the upcoming "partial layout" work (I consider this as more of a FIXME comment.)

[zalan]

(In reply to zalan from comment #3)
> (In reply to Simon Fraser (smfr) from comment #2)
> > Comment on attachment 464872 [details]
> > Patch
> > 
> > View in context:
> > https://bugs.webkit.org/attachment.cgi?id=464872&action=review
> > 
> > > Source/WebCore/rendering/RenderBlockFlow.cpp:3762
> > > +            // However at this point with such high IFC coverage, the subsequent layout is very likely to be IFC again.
> > 
> > This is the kind of comment that will go stale fairly fast. I'd suggest
> > rewriting this in a way that is more future-proof.
> I agree such comments may go stale fast in general, but I am somewhat
> convinced it's not going to be the case here (i.e. we don't go back to
> performing more legacy line layout) and also in practice this part of the
> code will be completely removed/restructured very soon with the upcoming
> "partial layout" work (I consider this as more of a FIXME comment.)
or I could just move this part to the commit message.

[Darin Adler]

Comment on attachment 464872 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=464872&action=review

>>>> Source/WebCore/rendering/RenderBlockFlow.cpp:3762
>>>> +            // However at this point with such high IFC coverage, the subsequent layout is very likely to be IFC again.
>>> 
>>> This is the kind of comment that will go stale fairly fast. I'd suggest rewriting this in a way that is more future-proof.
>> 
>> I agree such comments may go stale fast in general, but I am somewhat convinced it's not going to be the case here (i.e. we don't go back to performing more legacy line layout) and also in practice this part of the code will be completely removed/restructured very soon with the upcoming "partial layout" work (I consider this as more of a FIXME comment.)
> 
> or I could just move this part to the commit message.

I don’t think that Simon means there will be *less* IFC coverage in the future. But "at this point" is intrinsically about a moment in time. I think he just wants you to reword to say the subsequent layout is likely to be IFC again, without saying "at this point".

That’s future-proof until we get rid of non-IFC entirely, I think.

[Darin Adler]

Comment on attachment 464872 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=464872&action=review

>>>>> Source/WebCore/rendering/RenderBlockFlow.cpp:3762
>>>>> +            // However at this point with such high IFC coverage, the subsequent layout is very likely to be IFC again.
>>>> 
>>>> This is the kind of comment that will go stale fairly fast. I'd suggest rewriting this in a way that is more future-proof.
>>> 
>>> I agree such comments may go stale fast in general, but I am somewhat convinced it's not going to be the case here (i.e. we don't go back to performing more legacy line layout) and also in practice this part of the code will be completely removed/restructured very soon with the upcoming "partial layout" work (I consider this as more of a FIXME comment.)
>> 
>> or I could just move this part to the commit message.
> 
> I don’t think that Simon means there will be *less* IFC coverage in the future. But "at this point" is intrinsically about a moment in time. I think he just wants you to reword to say the subsequent layout is likely to be IFC again, without saying "at this point".
> 
> That’s future-proof until we get rid of non-IFC entirely, I think.

And yes, considering a shorter comment, with a more thorough explanation in the commit message, is a good idea. This comment is long enough to be hard to follow.

[zalan]

alternatively we could shorten the repaint rect by doing something along the lines of

auto repaintRect = clippedOverflowRectForRepaint(containerForRepaint().renderer);
repaintRectangle({ repaintRect.x(), repaintRect.y(), repaintRect.width(), *m_previousModernLineLayoutContentBoxLogicalHeight }, false);
(as opposed to call repaint())

but I don't think a tall container with constantly changing short content is a super common case.

[zalan]

Created attachment 464893 [details]
Patch

[zalan]

Created attachment 464899 [details]
Patch

[EWS]

Committed 260008@main (f5f87a80a556): <https://commits.webkit.org/260008@main>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 464899 [details].