[Webkit-unassigned] [Bug 192980] Eliminate line-breaks and disappear of list marker

[bugzilla-daemon at webkit.org]

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

--- Comment #47 from Manuel Rego Casasnovas <rego at igalia.com> ---
Comment on attachment 358114
  --> https://bugs.webkit.org/attachment.cgi?id=358114
Patch

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

I have added a few extra comments on the patch, anyway I'd wait for WPT tests before merging this.

The results in the rebaselined tests look much better.

> Source/WebCore/ChangeLog:37
> +

Nit: You need to regenrate ChangeLog as some changes from the patch are missing (for example the change in RenderBlock::isSelfCollapsingBlock()).

> Source/WebCore/rendering/RenderListItem.cpp:472
> +        assert(baselineProvider);

Why assert and not ASSERT?

> Source/WebCore/rendering/RenderListItem.cpp:474
> +        RenderBox* box = baselineProvider;
> +        offset = box->firstLineBaseline();

So this is done because RenderBlock::firstLineBaseline() is protected,
wouldn't make sense to make it public?
Specially if RenderBox::firstLineBaseline() is already public.

Is this ending up calling RenderBlock::firstLineBaseline() or it only calls RenderBox::firstLineBaseline(),
if it only goes to the RenderBox method we don't need to actually call it as it only returns "Optional<int>()".

> Source/WebCore/rendering/RenderListItem.cpp:496
> +        for (RenderBox* o = m_marker->parentBox(); o != this; o = o->parentBox())
> +            offsetValue -= o->logicalTop();

This is confusing, on a first sight I thought it was doing the same than the first loop.
But one is using baselineProvider and another the m_marker->parentBox(), which I guess can be different.
Could we put these two loops closer in the code and explain what they're doing
we could even avoid that if baselineProvider == m_marker->parentBox(), but I don't know if that ever happens.

> Source/WebCore/rendering/updating/RenderTreeBuilderList.cpp:37
> +    bool inside = is<RenderListMarker>(marker) ? downcast<RenderListMarker>(marker).isInside() : false;

Why we need a variable for this if it's only used in one place.

> Source/WebCore/rendering/updating/RenderTreeBuilderList.cpp:53
> +        if (!inside) {
> +            if (current.hasOverflowClip())
> +                return ¤t;
> +            if (is<RenderBlock>(child) && (!is<RenderBlockFlow>(child) || (is<RenderBox>(child) && downcast<RenderBox>(child).isWritingModeRoot())))
> +                return &downcast<RenderBlock>(child);

Could you add a comment explaining this change?

> Source/WebCore/rendering/updating/RenderTreeBuilderList.cpp:84
> +    element.setStyle(WTFMove(style));

If this is only needed to trigger needs layout, why not doing:
  element.setLogicalHeight(WTFMove(height));
  element.setNeedsLayout();

If this work I'd rename the method to something like:
  setLogicalHeightAndForceLayout()

> Source/WebCore/rendering/updating/RenderTreeBuilderList.cpp:137
> +                // E.g: <li><span><div>text<div><span></li>

This particular case seems to be already working "fine" in WebKit and different from Firefox and Chromium.
Check the following example:
  <ul>
    <li><span></span><div>text</div></li>
    <li><span>foo</span><div>text</div></li>
  </ul>

-- 
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/20181228/4f39f819/attachment.html>