Remove support for -webkit-color-correction (which we've never supported on

Remove support for -webkit-color-correction (which we've never supported on
the Skia platform) and the associated plumbing.

BUG=28005
NOTRY=true

Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=151868

[jbroman, 2013-06-04 20:50:06]

Per thread on blink-dev. Layout test failures so far appear unrelated.

[abarth-chromium, 2013-06-04 20:50:50]

Looks like a CL for eseidel

[eseidel, 2013-06-04 20:57:35]

I'm not sure we really want to remove all this colorspace information from our
code?  I guess if we believe it to be fully dead, then there is an argument for
removing dead code.  It just seems like we're going to end up adding a lot of
work for ourselves for adding this back.

I can't imagine that the web is going to remain color-space ignorant for another
10 years.

[eseidel, 2013-06-04 20:58:35]

I really don't want to discoruage you from removing dead code!  I just think
this CL is a bit agressive.  Should we start by removing teh dead CSS property
and separately consider removing all the colorspace plumbing?

I can certainly be convinced that this broad of a change is the right place to
start... but that's not immediately obvious to me.

[danakj, 2013-06-04 21:03:11]

On Tue, Jun 4, 2013 at 4:58 PM, <eseidel@chromium.org> wrote:

> I really don't want to discoruage you from removing dead code!  I just
> think
> this CL is a bit agressive.  Should we start by removing teh dead CSS
> property
> and separately consider removing all the colorspace plumbing?
>
> I can certainly be convinced that this broad of a change is the right
> place to
> start... but that's not immediately obvious to me.
>

From what I've seen, we've been pretty consistent in pushing for dead code
to be removed. This has been dead for all of chromium since it was added 4
years ago (except the mac port until it switched to skia a year and some
ago). Someone could always use this as a base for adding it back if it was
desirable. Do you feel this is a special case in some way?



>
>
https://codereview.chromium.**org/16357011/<https://codereview.chromium.org/1...
>

[jbroman, 2013-06-04 21:12:20]

On 2013/06/04 20:58:35, eseidel wrote:
> I really don't want to discoruage you from removing dead code!  I just think
> this CL is a bit agressive.  Should we start by removing teh dead CSS property
> and separately consider removing all the colorspace plumbing?
> 
> I can certainly be convinced that this broad of a change is the right place to
> start... but that's not immediately obvious to me.

I suppose my view is that (as far as I can tell by code archaeology):
1. This code hasn't been touched in some time, and seems to have been dead for
years.
2. It can be restored if desired by starting with a revert of this CL. Yes, this
will involve some merging. But that's just work we're hiding now by having a
colorSpace field that we don't test at all.
3. In the short-to-medium term, there doesn't appear to be anyone motivated to
make this feature work in Blink (as evidenced by the lack of activity on the bug
and lack of any response so far to my blink-dev email).
4. If the web does become less color-space-ignorant in the future, will this
spec and this implementation be the right way forward at that point? Will we not
be better able to judge that then?

I'm not super-strongly for the elimination of this code, but I do want to be
clear why we want to keep it (if indeed we do). Lacking any such reason,
unmaintained half-finished features strike me as a code smell.

I'm happy to discuss this further if you'd like.

[Noel Gordon, 2013-06-05 00:55:21]

I believe Stephen had very sound plans for this 
https://bugs.webkit.org/show_bug.cgi?id=81974#c97 so please do
speak to Stephen about that before you do this.

[eseidel, 2013-06-05 07:30:41]

I agree with your arguments against dead code.  We can start by reverting this
CL.  lgtm.  Stephen can revert this if he has near-term plans for this code.

[Stephen White, 2013-06-05 15:11:16]

On 2013/06/05 07:30:41, eseidel wrote:
> I agree with your arguments against dead code.  We can start by reverting this
> CL.  lgtm.  Stephen can revert this if he has near-term plans for this code.

For posterity, here's what I just sent to blink-dev on this subject:

Even in the absence of -webkit-color-correction, there is some benefit to
implementing correct color space support.

Most graphics operations (even simple source-over compositing) assume a linear
color space, and it is mathematically wrong to be applying them to sRGB images,
as we often do. The correct thing to do is to convert input sRGB images to
linear, apply the operation(s), and convert to display gamma (or back to sRGB as
an approximation, if a display gamma is unavailable). Of course, if there are
multiple operations to be performed in linear space, an obvious optimization is
to only apply a conversion at the beginning and end of processing.

The ColorSpace parameter is intended to convey precisely this: every ImageBuffer
and Image is tagged with its current color space, and conversions are applied
when necessary (before operations which expect to operate in linear space).
Since CoreGraphics has native color space support, the CG WebKit port maps these
values through and does the correct thing. The downsides to this correct
mathematical treatment of gamma are increased processing time (each conversion
requires a per-pixel LUT lookup), and it can introduce banding issues in some
cases.

All that said, I don't think anyone is actively working on implementation of
color space in Skia, and if the current non-functional blink parameter is viewed
as technical debt, it could be removed and reimplemented once someone takes on
the appropriate Skia work.

[jbroman, 2013-06-05 17:03:01]

On 2013/06/05 15:11:16, Stephen White wrote:
> On 2013/06/05 07:30:41, eseidel wrote:
> > I agree with your arguments against dead code.  We can start by reverting
this
> > CL.  lgtm.  Stephen can revert this if he has near-term plans for this code.
> 
> For posterity, here's what I just sent to blink-dev on this subject:
> 
> Even in the absence of -webkit-color-correction, there is some benefit to
> implementing correct color space support.
> 
> Most graphics operations (even simple source-over compositing) assume a linear
> color space, and it is mathematically wrong to be applying them to sRGB
images,
> as we often do. The correct thing to do is to convert input sRGB images to
> linear, apply the operation(s), and convert to display gamma (or back to sRGB
as
> an approximation, if a display gamma is unavailable). Of course, if there are
> multiple operations to be performed in linear space, an obvious optimization
is
> to only apply a conversion at the beginning and end of processing.
> 
> The ColorSpace parameter is intended to convey precisely this: every
ImageBuffer
> and Image is tagged with its current color space, and conversions are applied
> when necessary (before operations which expect to operate in linear space).
> Since CoreGraphics has native color space support, the CG WebKit port maps
these
> values through and does the correct thing. The downsides to this correct
> mathematical treatment of gamma are increased processing time (each conversion
> requires a per-pixel LUT lookup), and it can introduce banding issues in some
> cases.
> 
> All that said, I don't think anyone is actively working on implementation of
> color space in Skia, and if the current non-functional blink parameter is
viewed
> as technical debt, it could be removed and reimplemented once someone takes on
> the appropriate Skia work.

Acknowledged. Note that this CL does not modify ImageBuffer::transformColorSpace
or its call sites, which deals with the treatment of linear/device RGB
conversion in filters (which we *do* actually use).

[commit-bot: I haz the power, 2013-06-05 17:03:19]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/jbroman@chromium.org/16357011/11001

[commit-bot: I haz the power, 2013-06-05 17:03:51]

Failed to apply patch for Source/core/rendering/InlineTextBox.cpp:
While running patch -p1 --forward --force --no-backup-if-mismatch;
  patching file Source/core/rendering/InlineTextBox.cpp
  Hunk #10 succeeded at 1154 (offset -4 lines).
  Hunk #11 succeeded at 1171 with fuzz 1 (offset -6 lines).
  Hunk #12 FAILED at 1201.
  Hunk #13 FAILED at 1220.
  Hunk #14 succeeded at 1322 (offset -15 lines).
  Hunk #15 succeeded at 1430 (offset -15 lines).
  2 out of 15 hunks FAILED -- saving rejects to file
Source/core/rendering/InlineTextBox.cpp.rej

Patch:       Source/core/rendering/InlineTextBox.cpp
Index: Source/core/rendering/InlineTextBox.cpp
diff --git a/Source/core/rendering/InlineTextBox.cpp
b/Source/core/rendering/InlineTextBox.cpp
index
a1a2eff99e6a42141691a7c883c646252c347d73..7ffacd2fd7ebe32990054f3c3d424a2c11939e60
100644
--- a/Source/core/rendering/InlineTextBox.cpp
+++ b/Source/core/rendering/InlineTextBox.cpp
@@ -350,7 +350,7 @@ Color correctedTextColor(Color textColor, Color
backgroundColor)
     return textColor.light();
 }
 
-void updateGraphicsContext(GraphicsContext* context, const Color& fillColor,
const Color& strokeColor, float strokeThickness, ColorSpace colorSpace)
+void updateGraphicsContext(GraphicsContext* context, const Color& fillColor,
const Color& strokeColor, float strokeThickness)
 {
     TextDrawingModeFlags mode = context->textDrawingMode();
     if (strokeThickness > 0) {
@@ -360,13 +360,13 @@ void updateGraphicsContext(GraphicsContext* context, const
Color& fillColor, con
             mode = newMode;
         }
     }
-    
-    if (mode & TextModeFill && (fillColor != context->fillColor() || colorSpace
!= context->fillColorSpace()))
-        context->setFillColor(fillColor, colorSpace);
+
+    if (mode & TextModeFill && fillColor != context->fillColor())
+        context->setFillColor(fillColor);
 
     if (mode & TextModeStroke) {
         if (strokeColor != context->strokeColor())
-            context->setStrokeColor(strokeColor, colorSpace);
+            context->setStrokeColor(strokeColor);
         if (strokeThickness != context->strokeThickness())
             context->setStrokeThickness(strokeThickness);
     }
@@ -737,7 +737,7 @@ void InlineTextBox::paint(PaintInfo& paintInfo, const
LayoutPoint& paintOffset,
         // effect, so only when we know we're stroking, do a save/restore.
         GraphicsContextStateSaver stateSaver(*context, textStrokeWidth > 0);
 
-        updateGraphicsContext(context, textFillColor, textStrokeColor,
textStrokeWidth, styleToUse->colorSpace());
+        updateGraphicsContext(context, textFillColor, textStrokeColor,
textStrokeWidth);
         if (!paintSelectedTextSeparately || ePos <= sPos) {
             // FIXME: Truncate right-to-left text correctly.
             paintTextWithShadows(context, font, textRun, nullAtom, 0, 0,
length, length, textOrigin, boxRect, textShadow, textStrokeWidth > 0,
isHorizontal());
@@ -745,7 +745,7 @@ void InlineTextBox::paint(PaintInfo& paintInfo, const
LayoutPoint& paintOffset,
             paintTextWithShadows(context, font, textRun, nullAtom, 0, ePos,
sPos, length, textOrigin, boxRect, textShadow, textStrokeWidth > 0,
isHorizontal());
 
         if (!emphasisMark.isEmpty()) {
-            updateGraphicsContext(context, emphasisMarkColor, textStrokeColor,
textStrokeWidth, styleToUse->colorSpace());
+            updateGraphicsContext(context, emphasisMarkColor, textStrokeColor,
textStrokeWidth);
 
             DEFINE_STATIC_LOCAL(TextRun, objectReplacementCharacterTextRun,
(&objectReplacementCharacter, 1));
             TextRun& emphasisMarkTextRun = combinedText ?
objectReplacementCharacterTextRun : textRun;
@@ -768,10 +768,10 @@ void InlineTextBox::paint(PaintInfo& paintInfo, const
LayoutPoint& paintOffset,
         // paint only the text that is selected
         GraphicsContextStateSaver stateSaver(*context, selectionStrokeWidth >
0);
 
-        updateGraphicsContext(context, selectionFillColor,
selectionStrokeColor, selectionStrokeWidth, styleToUse->colorSpace());
+        updateGraphicsContext(context, selectionFillColor,
selectionStrokeColor, selectionStrokeWidth);
         paintTextWithShadows(context, font, textRun, nullAtom, 0, sPos, ePos,
length, textOrigin, boxRect, selectionShadow, selectionStrokeWidth > 0,
isHorizontal());
         if (!emphasisMark.isEmpty()) {
-            updateGraphicsContext(context, selectionEmphasisMarkColor,
textStrokeColor, textStrokeWidth, styleToUse->colorSpace());
+            updateGraphicsContext(context, selectionEmphasisMarkColor,
textStrokeColor, textStrokeWidth);
 
             DEFINE_STATIC_LOCAL(TextRun, objectReplacementCharacterTextRun,
(&objectReplacementCharacter, 1));
             TextRun& emphasisMarkTextRun = combinedText ?
objectReplacementCharacterTextRun : textRun;
@@ -789,7 +789,7 @@ void InlineTextBox::paint(PaintInfo& paintInfo, const
LayoutPoint& paintOffset,
     // Paint decorations
     TextDecoration textDecorations = styleToUse->textDecorationsInEffect();
     if (textDecorations != TextDecorationNone && paintInfo.phase !=
PaintPhaseSelection) {
-        updateGraphicsContext(context, textFillColor, textStrokeColor,
textStrokeWidth, styleToUse->colorSpace());
+        updateGraphicsContext(context, textFillColor, textStrokeColor,
textStrokeWidth);
         if (combinedText)
             context->concatCTM(rotation(boxRect, Clockwise));
         paintDecoration(context, boxOrigin, textDecorations,
styleToUse->textDecorationStyle(), textShadow);
@@ -876,8 +876,8 @@ void InlineTextBox::paintSelection(GraphicsContext* context,
const FloatPoint& b
         c = Color(0xff - c.red(), 0xff - c.green(), 0xff - c.blue());
 
     GraphicsContextStateSaver stateSaver(*context);
-    updateGraphicsContext(context, c, c, 0, style->colorSpace());  // Don't
draw text at all!
-    
+    updateGraphicsContext(context, c, c, 0); // Don't draw text at all!
+
     // If the text is truncated, let the thing being painted in the truncation
     // draw its own highlight.
     int length = m_truncation != cNoTruncation ? m_truncation : m_len;
@@ -906,7 +906,7 @@ void InlineTextBox::paintSelection(GraphicsContext* context,
const FloatPoint& b
 
     context->clip(clipRect);
 
-    context->drawHighlightForText(font, textRun, localOrigin, selHeight, c,
style->colorSpace(), sPos, ePos);
+    context->drawHighlightForText(font, textRun, localOrigin, selHeight, c,
sPos, ePos);
 }
 
 void InlineTextBox::paintCompositionBackground(GraphicsContext* context, const
FloatPoint& boxOrigin, RenderStyle* style, const Font& font, int startPos, int
endPos)
@@ -921,13 +921,13 @@ void
InlineTextBox::paintCompositionBackground(GraphicsContext* context, const F
     GraphicsContextStateSaver stateSaver(*context);
 
     Color c = Color(225, 221, 85);
-    
-    updateGraphicsContext(context, c, c, 0, style->colorSpace()); // Don't draw
text at all!
+
+    updateGraphicsContext(context, c, c, 0); // Don't draw text at all!
 
     int deltaY = renderer()->style()->isFlippedLinesWritingMode() ?
selectionBottom() - logicalBottom() : logicalTop() - selectionTop();
     int selHeight = selectionHeight();
     FloatPoint localOrigin(boxOrigin.x(), boxOrigin.y() - deltaY);
-    context->drawHighlightForText(font, constructTextRun(style, font),
localOrigin, selHeight, c, style->colorSpace(), sPos, ePos);
+    context->drawHighlightForText(font, constructTextRun(style, font),
localOrigin, selHeight, c, sPos, ePos);
 }
 
 static StrokeStyle textDecorationStyleToStrokeStyle(TextDecorationStyle
decorationStyle)
@@ -1158,8 +1158,6 @@ void InlineTextBox::paintDecoration(GraphicsContext*
context, const FloatPoint&
         localOrigin.move(0, extraOffset);
     }
 
-    ColorSpace colorSpace = renderer()->style()->colorSpace();
-
     do {
         if (shadow) {
             if (!shadow->next()) {
@@ -1179,7 +1177,7 @@ void InlineTextBox::paintDecoration(GraphicsContext*
context, const FloatPoint&
 #endif // CSS3_TEXT
        
context->setStrokeStyle(textDecorationStyleToStrokeStyle(decorationStyle));
         if (deco & TextDecorationUnderline) {
-            context->setStrokeColor(underline, colorSpace);
+            context->setStrokeColor(underline);
 #if ENABLE(CSS3_TEXT)
             TextUnderlinePosition underlinePosition =
styleToUse->textUnderlinePosition();
             const int underlineOffset =
computeUnderlineOffset(underlinePosition, styleToUse->fontMetrics(), this,
textDecorationThickness);
@@ -1203,7 +1201,7 @@ void InlineTextBox::paintDecoration(GraphicsContext*
context, const FloatPoint&
 #endif // CSS3_TEXT
         }
         if (deco & TextDecorationOverline) {
-            context->setStrokeColor(overline, colorSpace);
+            context->setStrokeColor(overline);
 #if ENABLE(CSS3_TEXT)
             switch (decorationStyle) {
             case TextDecorationStyleWavy: {
@@ -1222,7 +1220,7 @@ void InlineTextBox::paintDecoration(GraphicsContext*
context, const FloatPoint&
 #endif // CSS3_TEXT
         }
         if (deco & TextDecorationLineThrough) {
-            context->setStrokeColor(linethrough, colorSpace);
+            context->setStrokeColor(linethrough);
 #if ENABLE(CSS3_TEXT)
             switch (decorationStyle) {
             case TextDecorationStyleWavy: {
@@ -1339,16 +1337,16 @@ void
InlineTextBox::paintTextMatchMarker(GraphicsContext* pt, const FloatPoint&
     IntRect markerRect = enclosingIntRect(font.selectionRectForText(run,
IntPoint(x(), selectionTop()), selHeight, sPos, ePos));
     markerRect =
renderer()->localToAbsoluteQuad(FloatRect(markerRect)).enclosingBoundingBox();
     toRenderedDocumentMarker(marker)->setRenderedRect(markerRect);
-    
+
     // Optionally highlight the text
     if (renderer()->frame()->editor()->markedTextMatchesAreHighlighted()) {
         Color color = marker->activeMatch() ?
             renderer()->theme()->platformActiveTextSearchHighlightColor() :
             renderer()->theme()->platformInactive…
(message too large)

[Stephen White, 2013-06-05 17:08:04]

On 2013/06/05 17:03:01, jbroman wrote:
> On 2013/06/05 15:11:16, Stephen White wrote:
> > On 2013/06/05 07:30:41, eseidel wrote:
> > > I agree with your arguments against dead code.  We can start by reverting
> this
> > > CL.  lgtm.  Stephen can revert this if he has near-term plans for this
code.
> > 
> > For posterity, here's what I just sent to blink-dev on this subject:
> > 
> > Even in the absence of -webkit-color-correction, there is some benefit to
> > implementing correct color space support.
> > 
> > Most graphics operations (even simple source-over compositing) assume a
linear
> > color space, and it is mathematically wrong to be applying them to sRGB
> images,
> > as we often do. The correct thing to do is to convert input sRGB images to
> > linear, apply the operation(s), and convert to display gamma (or back to
sRGB
> as
> > an approximation, if a display gamma is unavailable). Of course, if there
are
> > multiple operations to be performed in linear space, an obvious optimization
> is
> > to only apply a conversion at the beginning and end of processing.
> > 
> > The ColorSpace parameter is intended to convey precisely this: every
> ImageBuffer
> > and Image is tagged with its current color space, and conversions are
applied
> > when necessary (before operations which expect to operate in linear space).
> > Since CoreGraphics has native color space support, the CG WebKit port maps
> these
> > values through and does the correct thing. The downsides to this correct
> > mathematical treatment of gamma are increased processing time (each
conversion
> > requires a per-pixel LUT lookup), and it can introduce banding issues in
some
> > cases.
> > 
> > All that said, I don't think anyone is actively working on implementation of
> > color space in Skia, and if the current non-functional blink parameter is
> viewed
> > as technical debt, it could be removed and reimplemented once someone takes
on
> > the appropriate Skia work.
> 
> Acknowledged. Note that this CL does not modify
ImageBuffer::transformColorSpace
> or its call sites, which deals with the treatment of linear/device RGB
> conversion in filters (which we *do* actually use).

Oh yes, I'm well aware of the filters case. :)

[commit-bot: I haz the power, 2013-06-05 17:23:37]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/jbroman@chromium.org/16357011/22001

[commit-bot: I haz the power, 2013-06-05 19:03:58]

Retried try job too often on win_layout_rel for step(s) webkit_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_layout...

[jbroman, 2013-06-05 21:51:56]

On 2013/06/05 19:03:58, I haz the power (commit-bot) wrote:
> Retried try job too often on win_layout_rel for step(s) webkit_tests
>
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_layout...

I've already tried to CQ this twice and had diff-applying problems (in the
latter case, while win_layout_rel was waiting to run). Given that
linux_layout_rel and mac_layout_rel are green on the latest patch set (which is
just a straightforward merge), and the other trybots have already gone green on
previous patch sets, I'm going to commit this with NOTRY=true.

[commit-bot: I haz the power, 2013-06-05 21:52:51]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/jbroman@chromium.org/16357011/40001

[commit-bot: I haz the power, 2013-06-05 21:54:38]

Change committed as 151868