[Webkit-unassigned] [Bug 192980] [css-lists] Fix list marker issues related to line breaks and markers disappearance

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=192980

--- Comment #65 from cathiechen <cathiechen at igalia.com> ---
Comment on attachment 358881
  --> https://bugs.webkit.org/attachment.cgi?id=358881
Patch

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

Hi Javier,

Thanks a lot for the review:)

>> Source/WebCore/rendering/RenderBlock.cpp:519
>> +        if (logicalHeightLength.isFixed() && isAnonymous() && parent() && is<RenderListItem>(parent())) {
> 
> is<> already checks for nullptr, so there is no need for the parent() clause.

Done

>> Source/WebCore/rendering/RenderBlock.cpp:521
>> +            if (child && is<RenderListMarker>(child) && !child->nextSibling())
> 
> ditto

Done

>> Source/WebCore/rendering/RenderBlock.h:-354
>> -    Optional<int> firstLineBaseline() const override;
> 
> Why we need to change visibility of this function ? As far as I understood, m_baselineProvides is a RenderBlock, so protected visibility should be enough.

The compiler complains. I think the reason is that for m_baselineProvides RenderListItem isn't the inside of own class.

>> Source/WebCore/rendering/RenderListItem.cpp:461
>> +    if (baselineProvider && is<RenderBlockFlow>(baselineProvider) && downcast<RenderBlockFlow>(baselineProvider)->firstRootBox() == &markerRoot)
> 
> No need to check for nullptr here if we already use the is<> template.

Done

>> Source/WebCore/rendering/RenderListItem.cpp:465
>> +    if (!backToOriginalBaseline)
> 
> Why not doing it this way instead ? 
> 
>  Optional<int> offset = backToOriginalBaseline ? nullopt : baselineProvider->firstLineBaseline();
> 
> Then in the if below we can just have one clause:
> 
>    if (!offset) {

Done. Thanks!

>> Source/WebCore/rendering/RenderListItem.cpp:474
>> +    if (offset) {
> 
> Maybe we can early return here in case of !offset instead.

Done

-- 
You are receiving this mail because:
You are the assignee for the bug.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.webkit.org/pipermail/webkit-unassigned/attachments/20190114/5e86f82c/attachment.html>