[webkit-reviews] review denied: [Bug 93111] [Chromium] Implement the find-in-page match rects API : [Attachment 157223] Patch

[bugzilla-daemon at webkit.org]

Darin Fisher (:fishd, Google) <fishd at chromium.org> has denied Leandro Graciá
Gil <leandrogracia at chromium.org>'s request for review:
Bug 93111: [Chromium] Implement the find-in-page match rects API
https://bugs.webkit.org/show_bug.cgi?id=93111

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

------- Additional Comments from Darin Fisher (:fishd, Google)
<fishd at chromium.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=157223&action=review


>> Source/WebCore/dom/Range.h:128
>> +	IntRect boundingBox(bool useTransforms = false) const;
> 
> Can we use an enum rather than a raw boolean here?  Raw boolean parameters
are hard to read at call sites.

Or how about just a completely separate function?  The implementation of
useTransforms=true and useTransforms=false share no code!

> Source/WebCore/rendering/FindInPageCoordinates.h:44
> +// This coordinate system is designed to give consistent tickmarks in cases
where find matches

The "tickmarks" feature corresponds to the Chromium UI feature of decorating
the scrollbars with tickmarks.	This is probably not a well known feature to
non-Chromium folks hacking WebCore.  Can we avoid mentioning it here?

I would probably put this file in WebKit/chromium/src instead since it is only
needed by code at that level.  It isn't needed by WebCore.

> Source/WebKit/chromium/src/WebFrameImpl.h:349
> +    int nearestFindMatch(const WebFloatPoint&, float& distanceSquared);

WebFloatPoint -> FloatPoint

> Source/WebKit/chromium/src/WebFrameImpl.h:355
> +    int selectFindMatch(unsigned index, WebRect* selectionRect);

WebRect -> IntRect

> Source/WebKit/chromium/src/WebFrameImpl.h:363
> +    void appendFindMatchRects(Vector<WebFloatRect>& frameRects);

internal functions should really use non-WebKit API types.  you should use
FloatRect instead of WebFloatRect unless you are implementation an API
function.

> Source/WebKit/chromium/src/WebFrameImpl.h:477
> +    WebSize m_contentsSizeForCurrentFindMatchRects;

WebSize -> IntSize


Is this patch missing some changes to WebFrame.h?