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

[bugzilla-daemon at webkit.org]

Adam Barth <abarth at webkit.org> has granted 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 157834: Patch
https://bugs.webkit.org/attachment.cgi?id=157834&action=review

------- Additional Comments from Adam Barth <abarth at webkit.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=157834&action=review


Below are a few minor comments.  Thanks for being so patient with this patch. 
In the interest of making progress, I'm inclined to land this version of the
patch and then iterate on these details.  Would you be willing to post a follow
up patch addressing these comments (and/or post a comment explaining why I'm
from outer space)?

Thanks again for the patch.

> Source/WebCore/dom/Range.cpp:1964
> +FloatRect Range::transformFriendlyBoundingBox() const

Why don't we need to call updateLayoutIgnorePendingStylesheets like we do
above?

> Source/WebCore/dom/Range.cpp:1966
> +    FloatRect result;

nit: Idiomatically, we'd put this declaration right above the loop that
populates it.

> Source/WebCore/dom/Range.cpp:1971
> +    const size_t n = quads.size();
> +    for (size_t i = 0; i < n; ++i)

nit: You can merge these lines.  There's no advantage to moving the size() call
out of the loop.

> Source/WebCore/dom/Range.h:129
>      FloatRect boundingRect() const;
> +    FloatRect transformFriendlyBoundingBox() const;

This is a bit confusing.  boundingRect() is already listed in the
"Transform-friendly" section, so it's unclear how it differs from
transformFriendlyBoundingBox().

> Source/WebKit/chromium/src/FindInPageCoordinates.cpp:51
> +static FloatRect toNormalizedRect(const FloatRect& absoluteRect, const
RenderObject* renderer, FloatRect* containerBoundingBox = 0)

nit: WebKit typically uses references for out parameters.

> Source/WebKit/chromium/src/WebFrameImpl.cpp:1675
> +#if OS(ANDROID)
> +   
viewImpl()->zoomToFindInPageRect(frameView()->contentsToWindow(enclosingIntRect
(m_activeMatch->transformFriendlyBoundingBox)));
> +#endif

OS(ANDROID) is likely the wrong ifdef to have here.  For example, if there as
an OS(IOS) configuration of PLATFORM(CHROMIUM), we'd likely want this code
there too.  I'm not sure what better to suggest, however.