[webkit-reviews] review denied: [Bug 192980] Eliminate line-breaks and disappear of list marker : [Attachment 357940] Patch

[bugzilla-daemon at webkit.org]

Manuel Rego Casasnovas <rego at igalia.com> has denied  review:
Bug 192980: Eliminate line-breaks and disappear of list marker
https://bugs.webkit.org/show_bug.cgi?id=192980

Attachment 357940: Patch

https://bugs.webkit.org/attachment.cgi?id=357940&action=review




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

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

Thanks for the patch, I did an initial review with lots of comments.
I didn't have time to look into the tests yet, but it'd be nice to use WPT
instead to share them with Chromium and Firefox.

This was fixed in Chromium first, could you link the Chromium bug in "See also"
field? Thanks.

> Source/WebCore/ChangeLog:9
> +	   is marker has been added as the child of this overflow block. And
the

Nit: "is that marker has been added"

> Source/WebCore/ChangeLog:11
> +	   sibilng of a block child. To fix these two kinds of issue:

Is there a way to divide these two fixes in two separated patches, or the code
is the same for both things?
I have the feeling that we're fixing two things in this patch one releated to
line breaks and another related to overflow: hidden.

> Source/WebCore/ChangeLog:12
> +	   1. Create a zero-height anonymouse parent for marker(to eleminate
the

Typo: s/anonymouse/anonymous/
Nit: Missing space before "("

> Source/WebCore/ChangeLog:15
> +	   has been layout (marker has to adjust itselt to the content of li)

Typo: s/has/have/

> Source/WebCore/ChangeLog:30
> +	   (WebCore::RenderListItem::alignMarkerInBlockDirection):

As this method is quite complex, it'd be nice to have a summary of what it does
here.

> Source/WebCore/ChangeLog:36
> +	   (WebCore::RenderTreeBuilder::List::updateItemMarker):

And the same in this case.

> Source/WebCore/rendering/RenderListItem.cpp:445
> +// Align marker_inline_box in block direction according to line_box_root's
baseline.

Nit: s/marker_inline_box/markerInlineBox/
Also I guess s/line_box_root/RootInlineBox/ ?

> Source/WebCore/rendering/RenderListItem.cpp:460
> +    }

Nit: This variable could be initialized like:
  backToOriginalBaseline = !baselineProvider ||
baselineProvider->isWritingModeRoot();

If this is true couldn't we just return and avoid doing anything in this
method?
Aren't we keeping the old behavior? Or are we doing something else?

> Source/WebCore/rendering/RenderListItem.cpp:463
> +    RootInlineBox& markerRoot = markerInlineBox->root();

Is it guaranteed that markerInlineBox is never null? Should we add an ASSERT at
least?

> Source/WebCore/rendering/RenderListItem.cpp:465
> +	   // If marker_ and baselineProvider share a same RootInlineBox, no
need to align marker.

Nit: "marker_" I guess that name might come from Blink and it should be
"m_marker" here.

> Source/WebCore/rendering/RenderListItem.cpp:466
> +	   if (downcast<RenderBlockFlow>(baselineProvider)->firstRootBox() ==
&markerRoot)

We don't need two ifs, we can combine both in just one if.

> Source/WebCore/rendering/RenderListItem.cpp:473
> +	   offset = box->firstLineBaseline().value();

Wouldn't we need "valueOr()" to be safe here?

Nit: Couldn't we write this like:
  offest = downcast<RenderBox>(baslineProvider)->firstLineBaseline().value();

> Source/WebCore/rendering/RenderListItem.cpp:477
> +	   baselineProvider = m_marker->containingBlock();

Again I guess we're sure containingBlock() is never null here.

> Source/WebCore/rendering/RenderListItem.cpp:482
> +    if (offset != -1) {

I'd change this for something like:
  if (offset == -1)
      return;

> Source/WebCore/rendering/RenderListItem.cpp:489
> +	   // BaselinePosition is workable when marker is an image.

What do you mean here by "is workable"?

> Source/WebCore/rendering/RenderListItem.cpp:491
> +	   // So use markerFontMetrics.Ascent when marker is text.

Nit: s/Ascent/ascent/

> Source/WebCore/rendering/RenderListItem.cpp:492
> +	   if (m_marker->isImage())

I see that RenderListMarker has a method baselinePosition() why we cannot use
that?
I guess it provides wrong information, then why we don't modify it?

> Source/WebCore/rendering/RenderListItem.cpp:496
> +	       const FontMetrics& markerFontMetrics =
m_marker->style().fontMetrics();
> +	       offset -= markerFontMetrics.ascent(markerRoot.baselineType());

Nit: I'd write this as:
  offset -= m_marker->style().fontMetrics().ascent(markerRoot.baselineType());

> Source/WebCore/rendering/RenderListItem.h:64
> +    void setNeedBlockDirectionAlign(bool need) { m_needBlockDirectionAlign =
need; }

Nit: s/setNeedBlockDirectionAlign/setNeedsBlockDirectionAlign/ and
s/m_needBlockDirectionAlign/m_needsBlockDirectionAlign/

> Source/WebCore/rendering/RenderListItem.h:65
> +    bool needBlockDirectionAlign() const { return m_needBlockDirectionAlign;
}

Nit: s/needBlockDirectionAlign/needsBlockDirectionAlign/

> Source/WebCore/rendering/updating/RenderTreeBuilderList.cpp:52
> -	   if (!is<RenderBlock>(child) || is<RenderTable>(child) ||
is<RenderRubyAsBlock>(child))
> -	       break;
> +	   if (current.hasOverflowClip())
> +	       return ¤t;
> +
> +	   if (is<RenderBlock>(child) && (!is<RenderBlockFlow>(child) ||
(is<RenderBox>(child) && downcast<RenderBox>(child).isWritingModeRoot())))
> +	       return &downcast<RenderBlock>(child);

I don't understand this change very well.
For the case of isWritingModeRoot() it was breaking before (so it returned
nullptr) and now it's returning the child...
Is this the expected bahvavior? Doesn't it affect other cases?

> Source/WebCore/rendering/updating/RenderTreeBuilderList.cpp:114
> +    // 1. Place marker as a child of <li>. Make sure don't share parent with
empty inline elements which don't generate InlineBox.

Typo: "Make sure it doesn't share parent"

> Source/WebCore/rendering/updating/RenderTreeBuilderList.cpp:120
> +    // Prepare for block direction align

Nit: Comments should finish in a "dot".

> Source/WebCore/rendering/updating/RenderTreeBuilderList.cpp:126
> +	       // So add IsInside() here.

Nit: s/IsInside/isInisde/

> Source/WebCore/rendering/updating/RenderTreeBuilderList.cpp:130
> +		       forceLogicalHeight(*currentParent,
Length(style.logicalHeight()));

Isn't enough just calling setLogicalHeigth()?

> Source/WebCore/rendering/updating/RenderTreeBuilderList.cpp:133
> +		   // We need to remove marker here.E.g:
<li><span><div>text<div><span></li>

Mmmm, what's the expected behavior for this example?

> Source/WebCore/rendering/updating/RenderTreeBuilderList.cpp:139
> +		   forceLogicalHeight(*currentParent, Length(0, Fixed));

Same question than before regarding setLogicalHeight().

> Source/WebCore/rendering/updating/RenderTreeBuilderList.cpp:152
> +		       forceLogicalHeight(*markerContainer, Length(0, Fixed));

Ditto.

> LayoutTests/ChangeLog:10
> +	   marker in li + block overflow:hiddlen child, and testing if the
acting

s/acting/behavior/

> LayoutTests/fast/lists/add-inline-child-after-marker-expected.html:1
> +<head></head><body><ul>

Tests need to start with <!DOCTYPE html> unless you're explicitly testing
quirks mode (which I don't think is the case).