[webkit-reviews] review denied: [Bug 113059] [SVG] Incorrect light source positioning : [Attachment 229009] Patch

[bugzilla-daemon at webkit.org]

Dirk Schulze <krit at webkit.org> has denied Adenilson Cavalcanti Silva
<savagobr at yahoo.com>'s request for review:
Bug 113059: [SVG] Incorrect light source positioning
https://bugs.webkit.org/show_bug.cgi?id=113059

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

------- Additional Comments from Dirk Schulze <krit at webkit.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=229009&action=review


Please take a look at the style bot output as well.

> Source/WebCore/ChangeLog:20
> +	   direction for feSpotLight, position for fePointlight.

Could you add more descriptions what exactly you are doing to fix this please?

> Source/WebCore/ChangeLog:31
> +	   * platform/graphics/cpu/arm/filters/FELightingNEON.h:
> +	   (WebCore::FELighting::platformApplyNeon):
> +	   * platform/graphics/filters/DistantLightSource.cpp:
> +	   (WebCore::DistantLightSource::initPaintingData):
> +	   * platform/graphics/filters/DistantLightSource.h:
> +	   * platform/graphics/filters/FELighting.cpp:
> +	   (WebCore::FELighting::drawLighting):
> +	   * platform/graphics/filters/Filter.h:

Please do more detailed descriptions behind the function names here. It really
would help to understand.

> Source/WebCore/platform/graphics/cpu/arm/filters/FELightingNEON.h:120
> +	   FloatPoint3D position = pointLightSource->position(filter());

I wonder why you pass a filter() here. Need to check.I guess I can imagine why.


> Source/WebCore/platform/graphics/cpu/arm/filters/FELightingNEON.h:124
> +	   floatArguments.lightX = position.x();
> +	   floatArguments.lightY = position.y();
> +	   floatArguments.lightZ = position.z();
>	   floatArguments.padding2 = 0;

What is float arguments and why doesn't it simply take a FloatPoint3D?

> Source/WebCore/platform/graphics/filters/DistantLightSource.cpp:40
> +void DistantLightSource::initPaintingData(PaintingData& paintingData, const
Filter*)

There should always be a Filter. therefore, you should be able to pass a
reference.

Why do you pass it at all? Can't see an instance of the virtual function that
is using it?

> Source/WebCore/platform/graphics/filters/LightSource.h:101
> +    FloatPoint3D mapPointToFilter(const FloatPoint3D& point, const Filter*
filter) const
> +    {
> +	   ASSERT(filter);
> +	   float horizontalUnit = filter->applyHorizontalScale(1);
> +	   float verticalUnit = filter->applyVerticalScale(1);
> +
> +	   float adjustedX = point.x() * horizontalUnit -
filter->filterOffset().width();
> +	   float adjustedY = point.y() * verticalUnit -
filter->filterOffset().height();
> +	   float adjustedZ = point.z() * sqrtf((horizontalUnit * horizontalUnit
+ verticalUnit * verticalUnit) / 2);
> +
> +	   return FloatPoint3D(adjustedX, adjustedY, adjustedZ);
> +    }

We usually do scaling operations in Filter itself. Examples are
applyHorizontalScale and applyVerticalScale actually. Can't you move this
function to Filter as well? Also, don't initialize 3 floats but then create a
FloatPoint3D. Create the FloatPoint3D from the beginning:

return FloatPoint3D(
    adjX,
    adjY,
    adjZ);

Also, can you pass point.x() directly to applyHorizontalScale()? Ditto for
point.y()

> Source/WebCore/platform/graphics/filters/PointLightSource.cpp:40
> +void PointLightSource::initPaintingData(PaintingData&, const Filter* filter)


Ok, missed this one.

> Source/WebCore/platform/graphics/filters/PointLightSource.cpp:43
> +    if (m_filterDataIsDirty)
> +	   updateFilterData(filter);

Is that some kind of optimization? Can you elaborate more?

> Source/WebCore/platform/graphics/filters/PointLightSource.h:38
> +    const FloatPoint3D& position(const Filter*) const;

reference, no pointer.

> Source/WebCore/platform/graphics/filters/PointLightSource.h:56
> +    void updateFilterData(const Filter*) const;

Ditto.

> Source/WebCore/platform/graphics/filters/PointLightSource.h:61
> +    mutable FloatPoint3D m_filterPosition;
> +    mutable bool m_filterDataIsDirty : 1;

: 1 doesn't make much sense if it is the only argument you want to put in a
word.

Why is m_filterPosition a member variable? And why does it need to be mutable?

> Source/WebCore/platform/graphics/filters/SpotLightSource.cpp:200
> +    m_filterPosition = mapPointToFilter(m_position, filter);
> +    m_filterDirection = mapPointToFilter(m_direction, filter);
> +    m_filterDataIsDirty = false;

Even more new member variables. Why do they need to be mem variables?

> Source/WebCore/platform/graphics/filters/SpotLightSource.cpp:215
> +const FloatPoint3D& SpotLightSource::position(const Filter* filter) const
> +{
> +    if (m_filterDataIsDirty)
> +	   updateFilterData(filter);
> +    return m_filterPosition;
> +}
> +
> +const FloatPoint3D& SpotLightSource::direction(const Filter* filter) const
> +{
> +    if (m_filterDataIsDirty)
> +	   updateFilterData(filter);
> +    return m_filterDirection;
> +}

Is that the optimization why you need to variables? How often do we compute
that per run? Is it really that costly?

> Source/WebCore/platform/graphics/filters/SpotLightSource.h:82
> +    mutable bool m_filterDataIsDirty : 1;

Ditto. See above.