[webkit-reviews] review denied: [Bug 39140] Add support for 4 and 8 hexit CSS hexcolor values : [Attachment 56115] Patch with tests

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has denied Tab Atkins <tabatkins at google.com>'s
request for review:
Bug 39140: Add support for 4 and 8 hexit CSS hexcolor values
https://bugs.webkit.org/show_bug.cgi?id=39140

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

------- Additional Comments from Darin Adler <darin at apple.com>
The patch has tabs in it, so it can't be landed.

What's the compatibility implication of this? What do existing browsers do when
confronted with colors in these formats? Can we prove to ourselves that there
are not some sites unknowingly depending on this? Have any other browsers added
support for this yet.

> +	if (length == 6) {
> +		alpha = 0xFF;
> +	}

> +	if (length == 3) {
> +		alpha = 0xF;
> +	}

WebKit coding style does not use braces around single line if statement bodies.


> +var testElement = document.createElement('div');
> +for (var i = 0; i < inputs.length; ++i) {
> +    testElement.style.color = inputs[i];
> +    shouldBeEqualToString('testElement.style.color', expected[i]);
> +}

This is not a great way to write a test, because the test output won't contain
any indication of what's being tested. Instead, I suggest writing this:

    shouldBeEqualToString('testElement.style.color = "' + inputs[i] + '";
testElement.style.color", expected[i]);

Then the test output will show what's being tested. Or you can make it even
cleaner by using a function:

    shouldBeEqualToString('testColorRoundTrip("' + inputs[i] + '")",
expected[i]);

There also was something wrong with the expected test result you attached here.
Not sure what went wrong exactly.

review- because of the tabs. Please consider my other comments.