[webkit-reviews] review granted: [Bug 179933] FELighting cleanup and optimization : [Attachment 327438] Patch

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has granted Simon Fraser (smfr)
<simon.fraser at apple.com>'s request for review:
Bug 179933: FELighting cleanup and optimization
https://bugs.webkit.org/show_bug.cgi?id=179933

Attachment 327438: Patch

https://bugs.webkit.org/attachment.cgi?id=327438&action=review




--- Comment #3 from Darin Adler <darin at apple.com> ---
Comment on attachment 327438
  --> https://bugs.webkit.org/attachment.cgi?id=327438
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=327438&action=review

Looks great. Hope the test coverage is already good.

> Source/WebCore/platform/graphics/filters/DistantLightSource.cpp:45
> +	   cosf(azimuth) * cosf(elevation),
> +	   sinf(azimuth) * cosf(elevation),
> +	   sinf(elevation)

Seems nicer to use the overloaded std::cos and std::sin rather than the
float-only sinf/cosf.

> Source/WebCore/platform/graphics/filters/FELighting.cpp:270
> +	       factorX * static_cast<float>(normal2DVector.width()) *
data.surfaceScale,
> +	       factorY * static_cast<float>(normal2DVector.height()) *
data.surfaceScale,

While they are not new, I don’t think these static_cast<float> are helpful or
needed. I think the compiler knows how to do "float * int * float -> float"
without casting help.

> Source/WebCore/platform/graphics/filters/FELighting.cpp:296
> +    unsigned char pixelValue[4] = {

Why "4" here, but "3" below in the call to setRange? Should be 3.

In code below you use uint8_t for this kind of thing, as does much new code. I
think you should use it here too. Benefits: uint8_t is shorter and helps
clarify that these bytes are not "characters". Disadvantages: type definition
rather than built-in name for the type.

> Source/WebCore/platform/graphics/filters/FELighting.cpp:299
> +	   static_cast<unsigned char>(lightStrength *
lightingData.colorVector.x()),
> +	   static_cast<unsigned char>(lightStrength *
lightingData.colorVector.y()),
> +	   static_cast<unsigned char>(lightStrength *
lightingData.colorVector.z()),

Truncation is what we want here, not rounding?

> Source/WebCore/platform/graphics/filters/FELighting.cpp:307
> +void FELighting::platformApplyGenericPaint(const LightingData& data, const
LightSource::PaintingData& paintingData, int startY, int endY)

The names start/end don’t make it clear to me that this stops *before* the end.
Wish there was consistent naming for this.

> Source/WebCore/platform/graphics/filters/FELighting.cpp:309
> +    ASSERT(startY);

Why is this an important assertion?

> Source/WebCore/platform/graphics/filters/FELighting.cpp:396
> +    TimingScope scope("FELighting::drawLighting", 100);

I think you want to remove this before landing?

> Source/WebCore/platform/graphics/filters/FELighting.cpp:403
> +    paintingData.intialLightingData.colorVector =
FloatPoint3D(m_lightingColor.red(), m_lightingColor.green(),
m_lightingColor.blue());

I think it would be nice to use a helper function rather than writing out the
integer-to-float conversion here, so this can work properly with ExtendedColor.
Although I suppose that would only be sufficient to initialize the color
vector, not necessarily enough to do the rest. One confusing aspect is that
there are at least two kinds of floating point color channels, 0.0->1.0 and
0.0->255.0. Or is the second one 0.0->255.99999999? So using Color::getRGBA
would not help.

> Source/WebCore/platform/graphics/filters/FELighting.h:68
> +	   uint8_t alpha[3][3] { };

Might consider using std::array instead.

> Source/WebCore/platform/graphics/filters/FELighting.h:70
> +	   // The implemtations are lined up to make comparing indices easier.

Misspelling of implementations here.

> Source/WebCore/platform/graphics/filters/FELighting.h:85
> +	   uint8_t topLeft() const	       { return alpha[0][0]; }
> +	   uint8_t left() const 	       { return alpha[1][0]; }
> +	   uint8_t bottomLeft() const	       { return alpha[2][0]; }
> +
> +	   uint8_t top() const		       { return alpha[0][1]; }
> +	   uint8_t center() const	       { return alpha[1][1]; }
> +	   uint8_t bottom() const	       { return alpha[2][1]; }
> +
> +	   void setTop(uint8_t value)	       { alpha[0][1] = value; }
> +	   void setCenter(uint8_t value)       { alpha[1][1] = value; }
> +	   void setBottom(uint8_t value)       { alpha[2][1] = value; }
> +
> +	   void setTopRight(uint8_t value)     { alpha[0][2] = value; }
> +	   void setRight(uint8_t value)        { alpha[1][2] = value; }
> +	   void setBottomRight(uint8_t value)  { alpha[2][2] = value; }

Our coding style guidelines say we don’t line things up vertically like this.

> Source/WebCore/platform/graphics/filters/FELighting.h:87
> +	   static void shiftRow(uint8_t alpha[3])

Using std::array& here would give us better type checking, but this function is
only used in shift() below, so I guess that’s no big deal.

> Source/WebCore/platform/graphics/filters/PointLightSource.cpp:54
> +    return {
> +	   lightVector,
> +	   { },
> +	   lightVector.length()
> +    };

I would write this on a single line. Not sure these always need to be vertical.
Maybe even the same thing for the lightVector above.

> Source/WebCore/platform/graphics/filters/SpotLightSource.cpp:86
> +	   return {
> +	       lightVector,
> +	       { },
> +	       lightVectorLength
> +	   };

Ditto.