[webkit-reviews] review granted: [Bug 31196] Implement -webkit-color-correction for CSS colors : [Attachment 42846] New Patch

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has granted Beth Dakin <bdakin at apple.com>'s
request for review:
Bug 31196: Implement -webkit-color-correction for CSS colors
https://bugs.webkit.org/show_bug.cgi?id=31196

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

------- Additional Comments from Darin Adler <darin at apple.com>
Have you tested the performance impact of this yet?

>      case CSSPropertyWebkitBoxSizing:
>	   valid_primitive = id == CSSValueBorderBox || id ==
CSSValueContentBox;
>	   break;
> +    case CSSPropertyWebkitColorCorrection:
> +	   if (id == CSSValueSrgb || id == CSSValueDefault)
> +	       valid_primitive = true;
> +	   break;

I notice that the previous case uses just an assignment, no if statement. Is
there  agood reason you chose a different size?

> +    case CSSPropertyWebkitColorCorrection: {
> +	   if (isInherit) 
> +	       m_style->setColorSpace(m_parentStyle->colorSpace());
> +	   else if (isInitial)
> +	       m_style->setColorSpace(DeviceColorSpace);
> +	   else {
> +	       if (!primitiveValue)
> +		   return;
> +	       m_style->setColorSpace(*primitiveValue);
> +	   }
> +	   return;
> +    }

The outer braces are not required.

>	   Color caretColor = Color::black;
> +	   ColorSpace colorSpace = DeviceColorSpace;
>	   Element* element = rootEditableElement();
> -	   if (element && element->renderer())
> +	   if (element && element->renderer()) {
>	       caretColor = element->renderer()->style()->color();
> +	       colorSpace = element->renderer()->style()->colorSpace();
> +	   }
>  
> -	   p->fillRect(caret, caretColor);
> +	   p->fillRect(caret, caretColor, colorSpace);

Code like this really cries out for a single structure that contains both color
and color space.

> -    ctxt->fillRect(dstRect, color);
> +    ctxt->fillRect(dstRect, color, DeviceColorSpace);

Seems to me that this already does the wrong thing for images that have a color
profile. In that case, the solid color optimization needs to be
color-corrected, not device. A bug, not caused by your change.

> -    if (mode & cTextFill && fillColor != context->fillColor())
> -	   context->setFillColor(fillColor);
> +    if (mode & cTextFill && (fillColor != context->fillColor()
> +	   || colorSpace != context->fillColorSpace()))
> +	   context->setFillColor(fillColor, colorSpace);

I think this would read better all on one line. And also, I think a single
struct for color and color space would make this so much more readable!

>  static void paintTextWithShadows(GraphicsContext* context, const Font& font,
const TextRun& textRun, int startOffset, int endOffset, const IntPoint&
textOrigin, int x, int y, int w, int h, ShadowData* shadow, bool stroked)
>  {
>      Color fillColor = context->fillColor();
> +    ColorSpace fillColorSpace = context->fillColorSpace();
>      bool opaque = fillColor.alpha() == 255;
>      if (!opaque)
> -	   context->setFillColor(Color::black);
> +	   context->setFillColor(Color::black, DeviceColorSpace);

I'm not sure DeviceColorSpace is right for this; maybe fillColorSpace would be
right. What test case would demonstrate the difference?

>  // Helper functions shared by InlineTextBox / SVGRootInlineBox
> -void updateGraphicsContext(GraphicsContext* context, const Color& fillColor,
const Color& strokeColor, float strokeThickness);
> +void updateGraphicsContext(GraphicsContext* context, const Color& fillColor,
const Color& strokeColor, float strokeThickness, ColorSpace colorSpace);

The context and colorSpace argument names should not be here.

> -		   context->fillRoundedRect(fillRect, topLeft, topRight,
bottomLeft, bottomRight, Color::black);
> +		   context->fillRoundedRect(fillRect, topLeft, topRight,
bottomLeft, bottomRight, Color::black, DeviceColorSpace);

I think we might want to use the color space from the style here, not
DeviceColorSpace.

> @@ -1258,7 +1258,7 @@ void RenderBoxModelObject::paintBoxShado
>  
>		   if (!rectToClipOut.isEmpty())
>		       context->clipOut(rectToClipOut);
> -		   context->fillRect(fillRect, Color::black);
> +		   context->fillRect(fillRect, Color::black, DeviceColorSpace);


Ditto.

>	       // Draw an outline rect where the image should be.
>	       context->setStrokeStyle(SolidStroke);
> -	       context->setStrokeColor(Color::lightGray);
> -	       context->setFillColor(Color::transparent);
> +	       context->setStrokeColor(Color::lightGray, DeviceColorSpace);
> +	       context->setFillColor(Color::transparent, DeviceColorSpace);

I think we want the style of the image element to control this rather than
hard-coding DeviceColorSpace.

> -    context->fillRect(absRect, Color::white);
> +    context->fillRect(absRect, Color::white, DeviceColorSpace);

These corners may need to respect the color space setting for the element they
are for rather than DeviceColorSpace.

> +	   ColorSpace colorSpace = element->renderStyle() ?
element->renderStyle()->colorSpace() : DeviceColorSpace;

When would element->renderStyle() be 0? In that case, why are we sure
DeviceColorSpace is right?

>	   [[textColor colorUsingColorSpaceName:NSDeviceRGBColorSpace]
getRed:&red green:&green blue:&blue alpha:&alpha];
> -	   graphicsContext.setFillColor(makeRGBA(red * 255, green * 255, blue *
255, alpha * 255));
> +	   graphicsContext.setFillColor(makeRGBA(red * 255, green * 255, blue *
255, alpha * 255), DeviceColorSpace);

Eventually, this code should change to respect the color space in the NSColor.
If it's device, then stay with device, but if it’s not, then color-correct to
sRGB values.

We've got to find a way to have more test coverage. We should have a separate
test for each call site where we are passing in a color space, and some easy
way to construct the tests. This might include some DumpRenderTree
enhancements.

r=me as-is -- this clearly all will work fine when nobody uses the new feature

But lets add a lot more test cases, and use the test cases to drive more
clarity on whether these explicit DeviceColorSpace call sites are correct. I
suspect at least half of them are not.