[webkit-reviews] review granted: [Bug 217417] Paint CSS highlights over images : [Attachment 411197] Patch

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has granted Megan Gardner
<megan_gardner at apple.com>'s request for review:
Bug 217417: Paint CSS highlights over images
https://bugs.webkit.org/show_bug.cgi?id=217417

Attachment 411197: Patch

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




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

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

I think this patch is OK, but there are quite a few things that should be
improved; not sure if I should have said review+ given the Simon made comments
and did not say that.

> Source/WebCore/rendering/HighlightData.cpp:166
> +bool HighlightData::setRenderRange(HighlightRangeData& rangeData)

Is there a reason you didn’t take Simon’s suggestion of making this "const
HighlightRangeData&"?

> Source/WebCore/rendering/HighlightData.cpp:169
> +    auto startPosition = rangeData.startPosition.value();
> +    auto endPosition = rangeData.endPosition.value();

I suggest checking these for null here in this function, rather than doing it
in the caller.

(Wow, just noticed that startPosition and endPosition in HighlightRangeData are
optional. I don’t understand why. Position already has a null value and I don’t
see why we also need them to be optional. I guess I can come through and clean
that up separately.)

> Source/WebCore/rendering/HighlightData.cpp:175
> +    auto* startRenderer = startPosition.containerNode()->renderer();
> +    auto* endRenderer = endPosition.containerNode()->renderer();
> +    
> +    if (!startRenderer || !endRenderer)
> +	   return false;

I think this also needs to check the containerNode values for nullptr (or check
the positions for null, same thing). Unless there’s a guarantee they will never
be null.

> Source/WebCore/rendering/RenderReplaced.cpp:157
> +	       if (!rangeData->startPosition || !rangeData->endPosition)
> +		   continue;

I suggest moving this check inside setRenderRange.

> Source/WebCore/rendering/RenderReplaced.cpp:223
> +    Color highlightColor = Color();

There’s no need to write "= Color()", that will happen even if we don’t specify
a value explicitly.

> Source/WebCore/rendering/RenderReplaced.cpp:266
> +

Stray blank line here we should delete.

>
LayoutTests/http/wpt/css/css-highlight-api/highlight-image-expected-mismatched.
html:13
> +    <div id="style1"><img src="../../images/blank-highlight.png"></div>
> +    <div id="style2"><img src="../../images/blank-highlight.png"></div>
> +    <div id="style3"><img src="../../images/blank-highlight.png"></div>

In a mismatch test, there’s no point in having three different test cases here.
If any of the three works, then the test will pass. If we want to test three
different things, then we need to find a way to do this as a match test, or do
it with three separate mismatch tests.