[webkit-reviews] review canceled: [Bug 84167] CSS 2.1 failure: inline-table-001 fails : [Attachment 138394] Patch

[bugzilla-daemon at webkit.org]

Julien Chaffraix <jchaffraix at webkit.org> has canceled Robert Hogan
<robert at webkit.org>'s request for review:
Bug 84167: CSS 2.1 failure: inline-table-001 fails
https://bugs.webkit.org/show_bug.cgi?id=84167

Attachment 138394: Patch
https://bugs.webkit.org/attachment.cgi?id=138394&action=review

------- Additional Comments from Julien Chaffraix <jchaffraix at webkit.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=138394&action=review


> Source/WebCore/rendering/RenderTable.cpp:1221
> +static LayoutUnit getLineBoxBaseline(const RenderTable* table, bool
firstLineBox)

Please no magical boolean. It makes the code less readable to read, especially
since firstLineBox == false means that you return the *last* line box baseline
which is not really obvious if you are looking at the caller.

> Source/WebCore/rendering/RenderTable.cpp:1238
> +    // The 'first' linebox baseline in a table in the absence of any text in
the first section
> +    // is the top of the table.
> +    if (firstLineBox)
> +	   return topNonEmptySection->logicalTop();

Shouldn't we query somehow the first row of your section instead of returning
the section's "baseline" here?

Here is the CSS 2.1 part that prompted this question: "The baseline of an
’inline-table’ is the baseline of the first row of the table." (which is also
what RenderTableSection::firstLineBoxBaseline does btw).

> Source/WebCore/rendering/RenderTable.h:241
> +    virtual LayoutUnit lastLineBoxBaseline() const;

OVERRIDE please.

> LayoutTests/ChangeLog:64
> +	   * platform/chromium-linux/fast/inline-block/001-expected.png:
> +	       Progression, the boxes now line up in the same way as FF and
Opera.

I am wondering if the text in capital letters at the beginning of the page
shouldn't be removed as we now matches other browsers. Also it may be the best
time to do that as we require all ports to rebaseline both the text and the
image.