[webkit-reviews] review denied: [Bug 103597] CSS3 Multicolumn: cannot set "auto <length>" to columns property. : [Attachment 188558] Patch

[bugzilla-daemon at webkit.org]

Benjamin Poulain <benjamin at webkit.org> has denied Morten Stenshorne
<mstensho at opera.com>'s request for review:
Bug 103597: CSS3 Multicolumn: cannot set "auto <length>" to columns property.
https://bugs.webkit.org/show_bug.cgi?id=103597

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

------- Additional Comments from Benjamin Poulain <benjamin at webkit.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=188558&action=review


r- because of the test.

I am not familiar with multicolumn so someone will have to look at this.

> Source/WebCore/css/CSSParser.cpp:3346
> +	       /* 'auto' may be a valid value for more than one longhand, and
at this point
> +		  we don't know which longhand(s) it's meant for. We need to
look at the
> +		  other values first. Just ignore 'auto', since it means the
same as setting
> +		  properties to the initial value, which we will take care of
after having
> +		  processed the value list. */

We don't use this style of comments in WebKit.

> Source/WebCore/css/CSSParser.cpp:3359
> +	       // if we didn't find at least one match, this is an
> +	       // invalid shorthand and we have to ignore it

WebKit comments needs to be proper sentences. In this case, it misses the
uppercase for the first character en the ending period.
When editing existing code, do not hesitate to fix the style.

> LayoutTests/fast/multicol/columns-shorthand-parsing-2.html:49
> +	 val[count] = document.getElementById('elm1').style.WebkitColumnCount;
> +	 pass &= val[count++] == 3;
> +	 val[count] = document.getElementById('elm2').style.WebkitColumnWidth;
> +	 pass &= val[count++] == "10em";
> +	 val[count] = document.getElementById('elm4').style.WebkitColumnWidth;
> +	 pass &= val[count++] != "7em";
> +	 val[count] = document.getElementById('elm4').style.WebkitColumnCount;
> +	 pass &= val[count++] != 7;
> +	 val[count] = document.getElementById('elm5').style.WebkitColumnWidth;
> +	 pass &= val[count++] == "7em";
> +	 val[count] = document.getElementById('elm5').style.WebkitColumnCount;
> +	 pass &= val[count++] == 7;
> +	 val[count] = document.getElementById('elm6').style.WebkitColumnWidth;
> +	 pass &= val[count++] == "7em";
> +	 val[count] = document.getElementById('elm6').style.WebkitColumnCount;
> +	 pass &= val[count++] == 7;
> +	 val[count] = document.getElementById('elm7').style.WebkitColumnCount;
> +	 pass &= val[count++] == 3;
> +	 val[count] = document.getElementById('elm8').style.WebkitColumnWidth;
> +	 pass &= val[count++] == "10em";
> +	 var text;
> +	 if (pass)
> +	   text = "PASS";
> +	 else {
> +	   text = "FAIL.<br>";
> +	   for (var i = 0; i < val.length; i++)
> +	     text += val[i] + "<br>";
> +	 }

The layout tests have some scripts to make tests a cleaner than this.
See LayoutTests/fast/js/resources/js-test-pre.js && js-test-post.js	

For the conditions, you can you shouldBe.
The test also need a description, and possibly an explanation.