Bug 134434

Summary: [WK2] Add a flatter find-in-page current match indicator style
Product: WebKit Reporter: Tim Horton <thorton>
Component: WebKit2Assignee: Tim Horton <thorton>
Status: RESOLVED FIXED    
Severity: Normal CC: bunhere, cdumez, commit-queue, gyuyoung.kim, sam, sergio, simon.fraser
Priority: P2 Keywords: InRadar
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
patch
darin: review+
patch simon.fraser: review+

Description Tim Horton 2014-06-29 00:40:24 PDT
<rdar://problem/16225673>
Comment 1 Tim Horton 2014-06-29 01:11:33 PDT
Created attachment 234058 [details]
patch

I have a few details to double check but I think this is mostly reviewable.
Comment 2 Darin Adler 2014-06-29 09:26:32 PDT
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.
Comment 3 Simon Fraser (smfr) 2014-06-30 10:52:05 PDT
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.
Comment 4 Tim Horton 2014-06-30 16:19:49 PDT
(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.
Comment 5 Tim Horton 2014-06-30 17:45:21 PDT
Created attachment 234129 [details]
patch
Comment 6 Tim Horton 2014-06-30 17:50:38 PDT
http://trac.webkit.org/changeset/170624