[webkit-reviews] review granted: [Bug 134434] [WK2] Add a flatter find-in-page current match indicator style : [Attachment 234058] patch

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has granted Tim Horton <thorton at apple.com>'s
request for review:
Bug 134434: [WK2] Add a flatter find-in-page current match indicator style
https://bugs.webkit.org/show_bug.cgi?id=134434

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

------- Additional Comments from Darin Adler <darin at apple.com>
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.