Bug 75898

Created attachment 121736 [details] Patch

Comment on attachment 121736 [details] Patch 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%; }

Comment on attachment 121736 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=121736&action=review Thanks for your review Julien! I've updated these tests, and in the meantime as I've been adding actual calc functionality, these are passing. So the expectations have been updated as well. >> 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. Done. I've used pre/post js for this. >> LayoutTests/css3/calc/table-calcs.html:4 >> + padding: 0; > > Nit: Add the units. Done. >> 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. The idea was that it would be red by default, and replaced by green if the test passed. >> LayoutTests/css3/calc/table-calcs.html:71 >> + </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! Good call! Done. >> 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? We talked about this offline - but this was an expected failure. Now that more calc functionality has landed, I've changed this to an expected. >> LayoutTests/css3/calc/table-empty-cells.html:1 >> +<style> > > Is quirks mode needed? No. Fixed. >> LayoutTests/css3/calc/table-empty-cells.html:8 >> + .bgcolor { background-color: yellow } > > Not sure that it's a useful style. Deleted. >> LayoutTests/css3/calc/table-empty-cells.html:16 >> + <td class="cell1"> </td> > > There no cell1 class (not repeated in both files). Good catch. Fixed by deleting existing cell1 and then replacing cell2 with cell1. >> 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%; > } Thanks - good idea. Done.

Created attachment 129812 [details] Patch

Comment on attachment 129812 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=129812&action=review Much better, thanks! > LayoutTests/css3/calc/table-empty-cells-expected.html:17 > + <table width="100%"> Unneeded width = 100% on all your tables as you factored it in the style :) [not repeated below] > LayoutTests/css3/calc/table-empty-cells.html:17 > + <table width="100%"> Unneeded width = 100% on all your tables as you factored it in the style :) [not repeated below]

Comment on attachment 129812 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=129812&action=review >> LayoutTests/css3/calc/table-empty-cells-expected.html:17 >> + <table width="100%"> > > Unneeded width = 100% on all your tables as you factored it in the style :) [not repeated below] Oops - good catch, thanks! Fixed. >> LayoutTests/css3/calc/table-empty-cells.html:17 >> + <table width="100%"> > > Unneeded width = 100% on all your tables as you factored it in the style :) [not repeated below] Oops - good catch, thanks! Fixed.

Created attachment 130029 [details] Patch for landing

Comment on attachment 130029 [details] Patch for landing Clearing flags on attachment: 130029 Committed r109680: <http://trac.webkit.org/changeset/109680>

All reviewed patches have been landed. Closing bug.