Bug 76000

Created attachment 121922 [details] Patch

Comment on attachment 121922 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=121922&action=review I suggest writing this test using LayoutTests/fast/js/resources/js-test-{pre, post}.js, which provide convenience functions to assert equality with descriptive output. Using these scripts would also make the output of this test more consistent with the output other PASS/FAIL tests. One example of using these scripts to write a test is: <http://trac.webkit.org/browser/trunk/LayoutTests/fast/css/aspect-ratio-inheritance.html>. > LayoutTests/css3/calc/margin.html:75 > + var error = []; > + if (width != expectedWidth) > + error.push("wrong width"); > + if (height != expectedHeight) > + error.push("wrong height"); > + It would be beneficial to print the expected width/height and actual width/height in the expected results so as to make it more straight forward to diagnose the reason for test failure should one or more the sub tests fail.

I agree about printing the actual vs the expected. The problem I had was that cr-linux had a different default margin than WebKit Mac, and since calc() doesn't work yet, the expected values were different. I didn't want to have to maintain two sets of results though. Maybe I'll try setting a margin, and getting calc() to override it instead.

Created attachment 121978 [details] Patch

Hi Daniel - I've changed the test to use the pre/post js libraries. I've also defaulted the margin to 0px, so it should pass OK on cr-linux (my results were generated on the Mac port).

Created attachment 122106 [details] Patch

Comment on attachment 122106 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=122106&action=review > LayoutTests/css3/calc/margin.html:29 > + <div><p id="percent-all">This element should have an overall margin of 25 pixels (10% of parent width of 300px minus 5px).</p></div><br/> > + <div><p id="percent-left">This element should have a left margin of 25 pixels (10% of parent width of 300px minus 5px).</p></div><br/> > + <div><p id="percent-right">This element should have a right margin of 25 pixels (10% of parent width of 300px minus 5px).</p></div><br/> > + <div><p id="percent-top">This element should have a top margin of 25 pixels (10% of parent width of 300px minus 5px).</p></div><br/> > + <div><p id="percent-bottom">This element should have a bottom margin of 25 pixels (10% of parent width of 300px minus 5px).</p></div><br/> Are these calculations correct with respect the definitions in <style> above? Notice that the style for the <p> specifies a width and height of 200px and 120px, respectively. Wouldn't a percentage value in the -webkit-calc expression be with respect to the height/width of the <p> (since it explicitly defines such dimensions) instead of its containing block? For example, looking at <p id="percent-left">, I would expect that the value of the margin-left property is -webkit-calc(10% - 5px) = 15px = 10% * 200px - 5px.

(In reply to comment #7) > [...] Wouldn't a percentage value in the -webkit-calc expression be with respect to the height/width of the <p> (since it explicitly defines such dimensions) instead of its containing block? I meant to write: Wouldn't a percentage value in the -webkit-calc expression be with respect to the height/width of the <p> (since it explicitly defines such dimensions) instead of the height/width of <div id="wrapper">?

(In reply to comment #8) > [...] > Wouldn't a percentage value in the -webkit-calc expression be with respect to the height/width of the <p> (since it explicitly defines such dimensions) instead of the height/width of <div id="wrapper">? Never mind. I'm not sure why I was thinking this.

Thanks for the review Daniel! I wrote this test a while ago, and margins were one of the trickiest things to measure, hence the contortions this test goes through. If you can think of a better way to measure margins, I'll happily update this test.

Comment on attachment 122106 [details] Patch Clearing flags on attachment: 122106 Committed r104783: <http://trac.webkit.org/changeset/104783>

All reviewed patches have been landed. Closing bug.