[webkit-reviews] review canceled: [Bug 75898] table tests for CSS3 calc : [Attachment 121736] Patch

[bugzilla-daemon at webkit.org]

Julien Chaffraix <jchaffraix at webkit.org> has canceled Mike Lawther
<mikelawther at chromium.org>'s request for review:
Bug 75898: table tests for CSS3 calc
https://bugs.webkit.org/show_bug.cgi?id=75898

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

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


> LayoutTests/css3/calc/table-calcs-expected.txt:9
> +

I am really not a huge fan of the output. A single FAIL / PASS per line would
be more readable.

Here are some ways to achieve that:
* use several rows with one cell per row.
* remove your tables from your final output and dump into #result.
* add visibility: hidden on your tables and dump into #result.

> LayoutTests/css3/calc/table-calcs.html:4
> +    padding: 0;

Nit: Add the units.

> LayoutTests/css3/calc/table-calcs.html:7
> +    background-color: red;

red usually should be reserved for failure. Here you expect to see the cells so
another color would be welcome.

> LayoutTests/css3/calc/table-calcs.html:71
> +    <div style="height: 416px">
> +	 <table style="height: 25%">
> +	     <tr><td class="height-percent-test">control height:25%</td></tr>
> +	 </table>
> +    </div>
> +    <div style="height: 416px">
> +	 <table style="height: -webkit-calc(25%);">
> +	     <tr><td class="height-percent-test">simple 25%</td></tr>
> +	 </table>
> +    </div>
> +    <div style="height: 416px">
> +	 <table style="height: -webkit-calc(10% * 2 + 5%));">
> +	     <tr><td class="height-percent-test">10% * 2 + 5%</td></tr>
> +	 </table>

If you set border-spacing: 0px on your table (I would argue on all tables in
your style), you would set your div's height to 400px and get the right value.
This would be a lot more readable!

> LayoutTests/css3/calc/table-empty-cells-expected-mismatch.html:1
> +<style> 

The name does not mention quirks mode. Also I don't understand what it is doing
in calc/ (no -webkit-calc in sight) and why we expect mismatch here?

> LayoutTests/css3/calc/table-empty-cells.html:1
> +<style> 

Is quirks mode needed?

> LayoutTests/css3/calc/table-empty-cells.html:8
> +    .bgcolor { background-color: yellow } 

Not sure that it's a useful style.

> LayoutTests/css3/calc/table-empty-cells.html:16
> +	   <td class="cell1"> </td>

There no cell1 class (not repeated in both files).

> LayoutTests/css3/calc/table-empty-cells.html:40
> +    <table cellpadding="0" cellspacing="0" width="100%">

cellpadding and cellspacing are obsolete, please use border-spacing instead.

Also factor out this style into your stylesheet and override it if needed
locally:

table {
    border-spacing: 0px;
    width: 100%;
}