[webkit-reviews] review denied: [Bug 15365] [CSS2.1] Anonymous tables should be inline/block-level based off tbody's parent box : [Attachment 114834] proposed patch. Most of it being test removal.

[bugzilla-daemon at webkit.org]

Julien Chaffraix <jchaffraix at webkit.org> has denied Kang-Hao (Kenny) Lu
<kennyluck at csail.mit.edu>'s request for review:
Bug 15365: [CSS2.1] Anonymous tables should be inline/block-level based off
tbody's parent box
https://bugs.webkit.org/show_bug.cgi?id=15365

Attachment 114834: proposed patch. Most of it being test removal.
https://bugs.webkit.org/attachment.cgi?id=114834&action=review

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


I would like to see some testing with continuations (which seems to work with
your table patch).

> Source/WebCore/ChangeLog:17
> +	   (WebCore::RenderInline::addChildIgnoringContinuation): where "block
in inline" is dealt with

I wonder what this means. Understand that people do read ChangeLogs so they
need to understand them.

> Source/WebCore/rendering/RenderInline.cpp:245
> +    if (!newChild->isInline() && !newChild->isFloatingOrPositioned() 
> +	   && !newChild->isTableCell() && !newChild->isTableRow() 
> +	   && !newChild->isTableSection() && !newChild->isTableCol()) {

I think this needs some explanations either with a comment or in the ChangeLog.
The whole logic works because we will create the needed wrappers and thus
recursively call RenderInline::addChild.

Also your checks don't strictly match the ones in RenderObject::addChild which
makes me think you are missing some cases:
* isTableCol() does not guarantee that we have a table wrapper. If this part
was tested, we would have seen it.
* the table caption case is not handled.

> Source/WebCore/rendering/RenderObject.cpp:321
> +	       if (style()->display() != INLINE)
> +		   newStyle->setDisplay(TABLE);
> +	       else
> +		   newStyle->setDisplay(INLINE_TABLE);

Preferably use an inline version:

newStyle->setDisplay(style()->display() == INLINE ? INLINE_TABLE : TABLE);

> LayoutTests/ChangeLog:8
> +	   *
fast/dynamic/insert-before-table-part-in-continuation-expected.txt: Added.

This change is wrong: there are different baselines for different platforms
because of subtle differences between them (mostly text related in this case).
By providing a unique baseline, you will make the test now fails on all 
platforms except the one for which you created the baselines.

> LayoutTests/ChangeLog:39
> +	   *
platform/chromium-linux/fast/dynamic/insert-before-table-part-in-continuation-e
xpected.png: Removed.
> +	   *
platform/chromium-linux/tables/mozilla/bugs/bug3037-1-expected.png: Removed.
> +	   *
platform/chromium-mac-leopard/fast/dynamic/insert-before-table-part-in-continua
tion-expected.png: Removed.
> +	   *
platform/chromium-mac-leopard/tables/mozilla/bugs/bug3037-1-expected.png:
Removed.
> +	   *
platform/chromium-mac/fast/dynamic/insert-before-table-part-in-continuation-exp
ected.png: Removed.
> +	   * platform/chromium-mac/tables/mozilla/bugs/bug3037-1-expected.png:
Removed.
> +	   *
platform/chromium-win/fast/dynamic/insert-before-table-part-in-continuation-exp
ected.png: Removed.
> +	   *
platform/chromium-win/fast/dynamic/insert-before-table-part-in-continuation-exp
ected.txt: Removed.
> +	   * platform/chromium-win/tables/mozilla/bugs/bug3037-1-expected.png:
Removed.
> +	   * platform/chromium-win/tables/mozilla/bugs/bug3037-1-expected.txt:
Removed.
> +	   *
platform/efl/fast/dynamic/insert-before-table-part-in-continuation-expected.png
: Removed.
> +	   *
platform/efl/fast/dynamic/insert-before-table-part-in-continuation-expected.txt
: Removed.
> +	   * platform/efl/tables/mozilla/bugs/bug3037-1-expected.png: Removed.
> +	   * platform/efl/tables/mozilla/bugs/bug3037-1-expected.txt: Removed.
> +	   *
platform/gtk/fast/dynamic/insert-before-table-part-in-continuation-expected.png
: Removed.
> +	   *
platform/gtk/fast/dynamic/insert-before-table-part-in-continuation-expected.txt
: Removed.
> +	   * platform/gtk/tables/mozilla/bugs/bug3037-1-expected.png: Removed.
> +	   * platform/gtk/tables/mozilla/bugs/bug3037-1-expected.txt: Removed.
> +	   *
platform/mac-leopard/fast/dynamic/insert-before-table-part-in-continuation-expe
cted.png: Removed.
> +	   * platform/mac-leopard/tables/mozilla/bugs/bug3037-1-expected.png:
Removed.
> +	   *
platform/mac/fast/dynamic/insert-before-table-part-in-continuation-expected.png
: Removed.
> +	   *
platform/mac/fast/dynamic/insert-before-table-part-in-continuation-expected.txt
: Removed.
> +	   * platform/mac/tables/mozilla/bugs/bug3037-1-expected.png: Removed.
> +	   * platform/mac/tables/mozilla/bugs/bug3037-1-expected.txt: Removed.
> +	   *
platform/qt-wk2/fast/dynamic/insert-before-table-part-in-continuation-expected.
png: Removed.
> +	   *
platform/qt/fast/dynamic/insert-before-table-part-in-continuation-expected.png:
Removed.
> +	   *
platform/qt/fast/dynamic/insert-before-table-part-in-continuation-expected.txt:
Removed.
> +	   * platform/qt/tables/mozilla/bugs/bug3037-1-expected.png: Removed.
> +	   * platform/qt/tables/mozilla/bugs/bug3037-1-expected.txt: Removed.

Removing all the baselines is not a good idea. We prefer to have contributors
rebaseline any platforms they can and mark the others as needing a new baseline
in test_expectations.txt for port maintainer.

> LayoutTests/fast/table/table-anonymous-inline-table-expected.txt:9
> +	   RenderTable at (12,0) size 11x18

If you look at the output, the table is shifted 4px toward the top edge. It
doesn't match Firefox or Opera in my testing. Looks like it is a bug in WebKit
but nowhere do I see mention of that.