[webkit-reviews] review granted: [Bug 217417] Paint CSS highlights over images : [Attachment 410725] 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 410725: Patch

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




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

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

> Source/WebCore/rendering/RenderReplaced.cpp:159
> +    auto* parentRenderer = parent();

Put this inside the if?

> Source/WebCore/rendering/RenderReplaced.cpp:160
> +    const RenderStyle* parentStyle;

This unused local is why Windows build is failing and should be deleted.

> Source/WebCore/rendering/RenderReplaced.cpp:161
> +    Color highlightColor;

This is unused and should be deleted.

> Source/WebCore/rendering/RenderReplaced.cpp:167
> +		   Position startPosition = rangeData->startPosition.value();

auto?

> Source/WebCore/rendering/RenderReplaced.cpp:169
> +		   auto* startRenderer =
startPosition.deprecatedNode()->renderer();

Should use containerNode here, not deprecatedNode. Same below.

> Source/WebCore/rendering/RenderReplaced.cpp:170
> +		   unsigned startOffset =
startPosition.deprecatedEditingOffset();

Should use computeOffsetInContainerNode here, not deprecatedEditingOffset. Same
below.

> Source/WebCore/rendering/RenderReplaced.cpp:174
> +		       continue;

Used continue here but nested if above. Should pick one style and stick with
it.

> Source/WebCore/rendering/RenderReplaced.cpp:177
> +		   highlightData.setRenderRange({ startRenderer, endRenderer,
startOffset, endOffset });

I think we need a helper function to make an optional render range from range
data to avoid that long 9 lines of code above and the extra if nesting too. But
could land like this and refactor I guess.

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

Extra blank line

> Source/WebCore/rendering/RenderReplaced.cpp:182
> +		       auto renderStyle =
parentRenderer->getUncachedPseudoStyle({ PseudoId::Highlight, highlight.key },
parentStyle);

Put this inside the if? Also could just name it style.

> Source/WebCore/rendering/RenderReplaced.cpp:242
> +    bool drawHighlight = shouldDrawHighlight(paintInfo);

I suggest we use a transparent highlight color to signify this instead of a
separate boolean.

> Source/WebCore/rendering/RenderReplaced.cpp:246
> +	   if (!highlightColor.isVisible())

Just do this isVisible check below instead off checking the boolean.

> Source/WebCore/rendering/RenderReplaced.cpp:252
>	   if (selectionState() == HighlightState::None)

Don’t we need to set drawHighlight to false here? (Or in my new proposal, set
highlightColor to invalid or transparent.) Or maybe optimize to not even call
shouldDrawHighlight or potentialHighlightColor or even shouldDrawSelectionTint
in this case? I am confused about this.

> Source/WebCore/rendering/RenderReplaced.cpp:285
> +	   LayoutRect selectionPaintingRect = localSelectionRect(false);

auto

> Source/WebCore/rendering/RenderReplaced.h:98
> +    virtual bool shouldDrawHighlight(PaintInfo&) const;

Maybe this can just return the color instead of a bool.