[webkit-reviews] review granted: [Bug 80322] Implement image-set : [Attachment 133150] Without Platform.h insanity this time

[bugzilla-daemon at webkit.org]

Dean Jackson <dino at apple.com> has granted Beth Dakin <bdakin at apple.com>'s
request for review:
Bug 80322: Implement image-set
https://bugs.webkit.org/show_bug.cgi?id=80322

Attachment 133150: Without Platform.h insanity this time
https://bugs.webkit.org/attachment.cgi?id=133150&action=review

------- Additional Comments from Dean Jackson <dino at apple.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=133150&action=review


> Source/WTF/ChangeLog:10
> +	   For the time being, image-set is opt-in since the implementation is 

> +	   incomplete.
> +	   * wtf/Platform.h:

Missing blank link. Also, after Tim's recent email to webkit-dev, I think this
should mention that it adds an ENABLE flag.

> Source/WebCore/WebCore.xcodeproj/project.pbxproj:13653
> -				FD315FA212B025B100C1A359 /* webaudio */,
> +				FD315FA212B025B100C1A359 /* Modules/webaudio
*/,

Cruft.

> Source/WebCore/WebCore.xcodeproj/project.pbxproj:21184
> -		FD315FA212B025B100C1A359 /* webaudio */ = {
> +		FD315FA212B025B100C1A359 /* Modules/webaudio */ = {

More cruft.

> Source/WebCore/css/CSSParser.cpp:3137
> -	       } else if (isGeneratedImageValue(val)) {
> +	       }
> +#if ENABLE(CSS_IMAGE_SET)
> +		 else if (equalIgnoringCase(val->function->name,
"-webkit-image-set(")) {
> +		   parsedValue = parseImageSet(m_valueList.get());

Indentation on line after #if.

With this, couldn't you leave the existing "} else if" as is and start the bit
inside the #if with "} else if" (but before the existing code)?

> Source/WebCore/css/CSSParser.cpp:5767
> -	       } else if (val->id == CSSValueNone)
> +	       }
> +#if ENABLE(CSS_IMAGE_SET)
> +	       else if (val->unit == CSSParserValue::Function &&
equalIgnoringCase(val->function->name, "-webkit-image-set(")) {

Same comments on the order and changing here.

> Source/WebCore/css/CSSStyleSelector.cpp:4536
> +	   if (current->isImageValue() || current->isImageGeneratorValue()
> +#if ENABLE(CSS_IMAGE_SET)
> +	   || current->isImageSetValue()
> +#endif

I *think* that the indentation is supposed to line up with the term in the
conditional.

> LayoutTests/ChangeLog:31
> +	   * platform/efl/Skipped:
> +	   * platform/gtk/Skipped:

What about the test_expectations files for Chrome?

> LayoutTests/fast/css/script-tests/image-set-parsing.js:84
> +testImageSetRule("Single value for content",
> +				"content",
> +			"url('#a') 1x", 2,
> +		   ["a", "1"]);
> +

some whacky indentation in this test

> LayoutTests/fast/hidpi/image-set-out-of-order.html:35
> +	   width:100px;
> +	   height:100px;

nit: space

> LayoutTests/fast/hidpi/image-set-simple.html:42
> +    <div>This test passes if the div below is a blue 100px square when the
deviceScaleFactor is 1, and if it is a 100px red square when the
deviceScaleFactor is 2.</div>

I wonder if we should use green instead of red. red == evil.