Bug 134434 – [WK2] Add a flatter find-in-page current match indicator style

• Component: WebKit2
• Status: RESOLVED FIXED

[Tim Horton]

<rdar://problem/16225673>

[Tim Horton]

Created attachment 234058 [details]
patch

I have a few details to double check but I think this is mostly reviewable.

[Darin Adler]

Comment on attachment 234058 [details]
patch

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

I could't figure out why the Windows EWS build failed. I couldn’t find an error message.

> Source/WebKit2/UIProcess/FindIndicator.cpp:90
> +float flatStyleOutset = 2;
> +float flatShadowOffsetX = 0;
> +float flatShadowOffsetY = 5;
> +float flatShadowBlurRadius = 25.0;
> +float flatRimShadowBlurRadius = 2.0;

These should all be marked const. Not sure why some of these have .0 and others don’t.

> Source/WebKit2/UIProcess/FindIndicator.cpp:216
> +static Color flatHighlightColor()
> +{
> +    // FIXME: This may not be the right color.
> +    return Color(243, 221, 50, 255);
> +}
> +
> +static Color flatRimShadowColor()
> +{
> +    return Color(0, 0, 0, 38);
> +}
> +
> +static Color flatDropShadowColor()
> +{
> +    return Color(0, 0, 0, 51);
> +}

Seems like you should mark these inline.

> Source/WebKit2/UIProcess/FindIndicator.cpp:255
> +    for (FloatRect textRect : m_textRectsInSelectionRectCoordinates) {

Should be FloatRect& or const FloatRect& to avoid an unnecessary copy. Personally I would use auto&.

> Source/WebKit2/UIProcess/FindIndicator.cpp:273
> +            IntRect contentImageRect = enclosingIntRect(textRect);

Why is it enclosingIntRect that we want here? I still don’t know the rules about when we do various types of “pixel snapping” and rounding and floor/ceiling. Of course, enclosing is a kind of floor/ceiling.

> Source/WebKit2/WebProcess/WebPage/FindController.cpp:485
> +        for (IntRect rect : rects) {

IntRect&, const IntRect& or auto& would be slightly more efficient.

> Source/WebKit2/WebProcess/WebPage/FindController.cpp:496
> +    for (IntRect rect : rects) {

Ditto.

[Simon Fraser (smfr)]

Comment on attachment 234058 [details]
patch

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

> Source/WebKit2/UIProcess/FindIndicator.cpp:121
> +#if ENABLE(LEGACY_FIND_INDICATOR_STYLE)
>          indicatorRect.move(-leftBorderThickness, -topBorderThickness);
>          indicatorRect.expand(leftBorderThickness + rightBorderThickness, topBorderThickness + bottomBorderThickness);
> +#else
> +        indicatorRect.inflate(flatStyleOutset);
> +#endif

I think you should wrap this in a helper function since it happens twice.

[Tim Horton]

(In reply to comment #2)
> (From update of attachment 234058 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=234058&action=review
> 
> I could't figure out why the Windows EWS build failed. I couldn’t find an error message.

:(

> > Source/WebKit2/UIProcess/FindIndicator.cpp:273
> > +            IntRect contentImageRect = enclosingIntRect(textRect);
> 
> Why is it enclosingIntRect that we want here? I still don’t know the rules about when we do various types of “pixel snapping” and rounding and floor/ceiling. Of course, enclosing is a kind of floor/ceiling.

In this case, it's just matching the enclosingIntRect() with which the content image was painted in the Web process, so that everything lines up. And, that one uses enclosingIntRect() presumably so that the backing store can contain the entire selection; other rounding modes could end up clipping a tiny part of the text.

[Tim Horton]

Created attachment 234129 [details]
patch

[Tim Horton]

http://trac.webkit.org/changeset/170624