Bug 93111 – Implement the find-in-page match rects API

• Component: WebKit API
• Status: RESOLVED FIXED

[Leandro Graciá Gil]

The Android port allows tapping on the find-in-page result tickmarks taking the user to the corresponding matches. The required new methods should be added as stubs by bug 93110. This patch implements the functionality of such methods.

[Leandro Graciá Gil]

Created attachment 156414 [details]
Patch

[Leandro Graciá Gil]

Comment on attachment 156414 [details]
Patch

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

> Source/WebKit/chromium/public/WebFrame.h:580
> +    // Returns a counter that is incremented when the find-in-page markers are

This patch needs to be rebased over the stubs introduced by 93110. Therefore, there will be no WebFrame.h changes. This will go away in the next update.

[WebKit Review Bot]

Please wait for approval from [e-mail], [e-mail], [e-mail], [e-mail] or [e-mail] before submitting, as this patch contains changes to the Chromium public API. See also https://trac.webkit.org/wiki/ChromiumWebKitAPI.

[Darin Fisher (:fishd, Google)]

Comment on attachment 156414 [details]
Patch

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

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

code in WebCore/platform is not allowed to depend on code in WebCore/rendering

[Leandro Graciá Gil]

Comment on attachment 156414 [details]
Patch

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

>> Source/WebCore/platform/chromium/FindInPageCoordinates.cpp:50
>> +static FloatRect toNormalizedRect(const FloatRect& absoluteRect, const RenderObject* renderer, FloatRect* containerBoundingBox = 0)
> 
> code in WebCore/platform is not allowed to depend on code in WebCore/rendering

Np, I will move it. Where do you think I should add a file like this, that is only expected to be used for now by the chromium port? Perhaps something like page/chromium or rendering/chromium?

[Adam Barth]

Comment on attachment 156414 [details]
Patch

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

>>> Source/WebCore/platform/chromium/FindInPageCoordinates.cpp:50
>>> +static FloatRect toNormalizedRect(const FloatRect& absoluteRect, const RenderObject* renderer, FloatRect* containerBoundingBox = 0)
>> 
>> code in WebCore/platform is not allowed to depend on code in WebCore/rendering
> 
> Np, I will move it. Where do you think I should add a file like this, that is only expected to be used for now by the chromium port? Perhaps something like page/chromium or rendering/chromium?

There's nothing Chromium-specific about this file.  Perhaps just in WebCore/rendering?

[Leandro Graciá Gil]

(In reply to comment #6)
> (From update of attachment 156414 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=156414&action=review
> 
> >>> Source/WebCore/platform/chromium/FindInPageCoordinates.cpp:50
> >>> +static FloatRect toNormalizedRect(const FloatRect& absoluteRect, const RenderObject* renderer, FloatRect* containerBoundingBox = 0)
> >> 
> >> code in WebCore/platform is not allowed to depend on code in WebCore/rendering
> > 
> > Np, I will move it. Where do you think I should add a file like this, that is only expected to be used for now by the chromium port? Perhaps something like page/chromium or rendering/chromium?
> 
> There's nothing Chromium-specific about this file.  Perhaps just in WebCore/rendering?

Sounds good to me. Should it be done as part of this patch (In reply to comment #6)
> (From update of attachment 156414 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=156414&action=review
> 
> >>> Source/WebCore/platform/chromium/FindInPageCoordinates.cpp:50
> >>> +static FloatRect toNormalizedRect(const FloatRect& absoluteRect, const RenderObject* renderer, FloatRect* containerBoundingBox = 0)
> >> 
> >> code in WebCore/platform is not allowed to depend on code in WebCore/rendering
> > 
> > Np, I will move it. Where do you think I should add a file like this, that is only expected to be used for now by the chromium port? Perhaps something like page/chromium or rendering/chromium?
> 
> There's nothing Chromium-specific about this file.  Perhaps just in WebCore/rendering?

Sounds good to me. Thanks.

[Leandro Graciá Gil]

Created attachment 156440 [details]
Patch

[WebKit Review Bot]

Comment on attachment 156440 [details]
Patch

Attachment 156440 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/13421922

[Adam Barth]

Comment on attachment 156440 [details]
Patch

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

> Source/WebKit/chromium/src/WebFrameImpl.cpp:523
> +    , m_rect(0, 0, 0, 0)

Presumably m_rect will initialize itself to zeros automatically.

> Source/WebKit/chromium/src/WebFrameImpl.cpp:2003
> +            it->m_rect = FloatRect(0, 0, 0, 0);

FloatRect(0, 0, 0, 0) -> FloatRect()

> Source/WebKit/chromium/src/WebFrameImpl.cpp:2278
> +    , m_contentsSizeForCurrentFindMatchRects(0, 0)

m_contentsSizeForCurrentFindMatchRects will probably init itself.

> Source/WebKit/chromium/src/WebFrameImpl.h:279
> +    PassRefPtr<WebCore::Range> activeMatch() const { return m_activeMatch; }

This should just return a WebCore::Range*

[Adam Barth]

@fishd: Who should review this patch?

[Leandro Graciá Gil]

Adding Hans as he's looking for patches to do informal reviews.

[Leandro Graciá Gil]

Created attachment 156703 [details]
Patch

[Leandro Graciá Gil]

Comment on attachment 156440 [details]
Patch

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

Also fixing the unit test.

>> Source/WebKit/chromium/src/WebFrameImpl.cpp:523
>> +    , m_rect(0, 0, 0, 0)
> 
> Presumably m_rect will initialize itself to zeros automatically.

Good point. Fixed.

>> Source/WebKit/chromium/src/WebFrameImpl.cpp:2003
>> +            it->m_rect = FloatRect(0, 0, 0, 0);
> 
> FloatRect(0, 0, 0, 0) -> FloatRect()

Fixed.

>> Source/WebKit/chromium/src/WebFrameImpl.cpp:2278
>> +    , m_contentsSizeForCurrentFindMatchRects(0, 0)
> 
> m_contentsSizeForCurrentFindMatchRects will probably init itself.

Fixed.

>> Source/WebKit/chromium/src/WebFrameImpl.h:279
>> +    PassRefPtr<WebCore::Range> activeMatch() const { return m_activeMatch; }
> 
> This should just return a WebCore::Range*

Fixed.

[Tony Chang]

Maybe Finnur would like to do an informal review?  He implemented the find-in-page functions years ago.

[Tony Chang]

Comment on attachment 156703 [details]
Patch

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

Just some style nits.

> Source/WebCore/ChangeLog:35
> +        * rendering/FindInPageCoordinates.cpp: Added.
> +        (WebCore):
> +        (WebCore::toNormalizedRect):
> +        (WebCore::FindInPageCoordinates::fromAbsoluteRect):
> +        (WebCore::FindInPageCoordinates::fromRange):

Please fill in some details here.

> Source/WebKit/chromium/ChangeLog:20
> +        (WebKit::WebFrameImpl::find):
> +        (WebKit::WebFrameImpl::stopFinding):
> +        (WebKit::WebFrameImpl::scopeStringMatches):
> +        (WebKit::WebFrameImpl::increaseMatchCount):

This would benefit from some comments as well.

> Source/WebCore/dom/Range.cpp:1953
> +    if (quads.isEmpty())
> +        return FloatRect();

Nit: Is the early return necessary? Looks like we'll get the same result without this check.

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

Nit: It would be nice if the names of these functions explained how they are different. Also, it's a bit weird that one is a BoundingBox and one is a BoundingRect.  Maybe you could just merge into a single function that takes an enum param for whether or not to include border?

> Source/WebCore/rendering/FindInPageCoordinates.h:51
> +class FindInPageCoordinates {
> +public:

Nit: I would remove the class. I don't think it's common for utility methods to be in a class in WebCore (I only see this pattern in some Chromium code).  Maybe try to make the function names something like findInPageRectFrom(...)?

> Source/WebKit/chromium/tests/WebFrameTest.cpp:818
> +    virtual void reportFindInPageMatchCount(int requestId, int count, bool finalUpdate) OVERRIDE

Nit: WebKit style is to omit unused parameters (in this case, requestId and count).

> Source/WebKit/chromium/tests/WebFrameTest.cpp:858
> +    EXPECT_EQ(webMatchRects.size(), static_cast<size_t>(kNumResults));

Nit: This should probably be an ASSERT_EQ since if you get the wrong size, we'll crash below.

> Source/WebKit/chromium/tests/WebFrameTest.cpp:885
> +    // All results after the first two ones should be below between them in scroll-independent normalized main frame coordinates.
> +    // This is because results 2 to 9 are inside an iframe located between results 0 and 1. This applies to the fixed div too.
> +    EXPECT_TRUE(webMatchRects[0].y < webMatchRects[1].y);
> +    EXPECT_TRUE(webMatchRects[0].y < webMatchRects[2].y);

Nit: I might use some loops for this, but this is also OK.

[Leandro Graciá Gil]

Comment on attachment 156703 [details]
Patch

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

>> Source/WebCore/ChangeLog:35
>> +        (WebCore::FindInPageCoordinates::fromRange):
> 
> Please fill in some details here.

Done.

>> Source/WebKit/chromium/ChangeLog:20
>> +        (WebKit::WebFrameImpl::increaseMatchCount):
> 
> This would benefit from some comments as well.

Done.

>> Source/WebCore/dom/Range.cpp:1953
>> +        return FloatRect();
> 
> Nit: Is the early return necessary? Looks like we'll get the same result without this check.

Fixed.

>> Source/WebCore/dom/Range.h:129
>>      FloatRect boundingRect() const;
> 
> Nit: It would be nice if the names of these functions explained how they are different. Also, it's a bit weird that one is a BoundingBox and one is a BoundingRect.  Maybe you could just merge into a single function that takes an enum param for whether or not to include border?

I've considered merging them, but they seem to return different coordinate systems and units. boundingRect calls Document::adjustFloatQuadsForScrollAndAbsoluteZoomAndFrameScale which as the name suggests involves scrolling positions, zoom and the page scale factor, while enclosingBoundingBox does nothing of the sort and it's simply the union of the absolute quads from the text. Since this latter behaviour is just the transform-friendly equivalent of boundingBox I'm merging these two and adding a boolean value to use transforms defaulting to false as other existing code in WebCore does.

>> Source/WebCore/rendering/FindInPageCoordinates.h:51
>> +public:
> 
> Nit: I would remove the class. I don't think it's common for utility methods to be in a class in WebCore (I only see this pattern in some Chromium code).  Maybe try to make the function names something like findInPageRectFrom(...)?

Done as suggested.

>> Source/WebKit/chromium/tests/WebFrameTest.cpp:818
>> +    virtual void reportFindInPageMatchCount(int requestId, int count, bool finalUpdate) OVERRIDE
> 
> Nit: WebKit style is to omit unused parameters (in this case, requestId and count).

Fixed.

>> Source/WebKit/chromium/tests/WebFrameTest.cpp:858
>> +    EXPECT_EQ(webMatchRects.size(), static_cast<size_t>(kNumResults));
> 
> Nit: This should probably be an ASSERT_EQ since if you get the wrong size, we'll crash below.

Fixed.

>> Source/WebKit/chromium/tests/WebFrameTest.cpp:885
>> +    EXPECT_TRUE(webMatchRects[0].y < webMatchRects[2].y);
> 
> Nit: I might use some loops for this, but this is also OK.

Fixed.

[Leandro Graciá Gil]

Created attachment 157223 [details]
Patch

[Finnur Thorarinsson]

Intriguing. I like the general idea of being able to click on a tickmark and jump to it. 

I took a look at this from a high level point of view, and I'm fine with adding this (from a Chrome Find in page standpoint). Unfortunately, my WebKit foo is getting a bit rusty, so I'm not reviewing the patch for correctness (and besides, I'm not a WebKit reviewer so my LG isn't worth much here). :)

[Adam Barth]

Thanks Finnur.  I appreciate your taking a look.

[Adam Barth]

Comment on attachment 157223 [details]
Patch

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

> Source/WebCore/dom/Range.h:128
> +    // FIXME: eventually useTransforms should flip to true by default.
> +    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.

> Source/WebCore/rendering/FindInPageCoordinates.h:56
> +FloatRect findInPageRectFromRange(PassRefPtr<Range>);

PassRefPtr<Range>  ->  Range*

This function doesn't need to take ownership of the Range so there's no advantage to using PassRefPtr.  http://www.webkit.org/coding/RefPtr.html has some more information about RefPtr and PassRefPtr if you're interested.

> Source/WebKit/chromium/src/WebFrameImpl.cpp:1996
> +    const WebFrameImpl* mainFrameImpl = viewImpl()->mainFrameImpl();
> +    const WebFrameImpl* activeMatchFrame = mainFrameImpl->activeMatchFrame();

We usually don't bother with the const in "const WebFrameImpl".  It's not really sensible for a WebFrameImpl to be const.

> Source/WebKit/chromium/src/WebFrameImpl.cpp:1998
> +            && activeMatchFrame->frame()->tree()->isDescendantOf(mainFrameImpl->frame());

four-space indent.

> Source/WebKit/chromium/src/WebFrameImpl.cpp:2009
> +    unsigned deadMatches = 0;

unsigned -> size_t

It looks like this number can be as large as Vector<...>::size(), which is a size_t.

> Source/WebKit/chromium/src/WebFrameImpl.cpp:2035
> +    if (!m_findMatchRectsAreValid)
> +        for (WebFrame* child = firstChild(); child; child = child->nextSibling())
> +            static_cast<WebFrameImpl*>(child)->m_findMatchRectsAreValid = false;

Do we need to do a recursive walk of all the subframes?

> Source/WebKit/chromium/src/WebFrameImpl.cpp:2042
> +    // This function should only be called on the mainframe.

This comment is redundant with the line below.  :)

> Source/WebKit/chromium/src/WebFrameImpl.cpp:2053
> +    // This function should only be called on the mainframe.

ditto

> Source/WebKit/chromium/src/WebFrameImpl.cpp:2075
> +    // This function should only be called on the mainframe.

ditto

> Source/WebKit/chromium/src/WebFrameImpl.cpp:2142
> +        // Clear any user selection, to make sure Find Next continues on from the match we just activated.
> +        executeCommand(WebString::fromUTF8("Unselect"));

Is there a more direct way to do this?  It seems odd to have to go back through the API with a string like this.

> Source/WebKit/chromium/src/WebFrameImpl.cpp:2145
> +        if (frame()->document())

frame()->document() shouldn't ever be 0

> Source/WebKit/chromium/src/WebFrameImpl.cpp:2365
> +void WebFrameImpl::didChangeContentsSize(const IntSize& size)

Do we need to call the base class here?

> Source/WebKit/chromium/src/WebFrameImpl.h:252
> +    // Called when the size of the contained document changes.

This comment doesn't add any value and should be removed.

> Source/WebKit/chromium/src/WebFrameImpl.h:253
> +    void didChangeContentsSize(const WebCore::IntSize&);

I couldn't find the call site for this function.  Is it a virtual function override?  If so, we should mark it virtual and add the OVERRIDE keyword.

> Source/WebKit/chromium/tests/WebFrameTest.cpp:847
> +    { }

Technically these brackets should be on separate lines.

> Source/WebKit/chromium/tests/WebFrameTest.cpp:934
> +    // Resizing should update the rects version.
> +    mainFrame->didChangeContentsSize(WebSize(800, 600));

Shouldn't we call resize on the view and have this function called automatically?

[Adam Barth]

Comment on attachment 157223 [details]
Patch

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

> Source/WebKit/chromium/src/WebFrameImpl.cpp:2135
> +        viewImpl()->mainFrameImpl()->m_currentActiveMatchFrame = this;

This looks like it might become a dangling pointer given that no one appears to clear this pointer if/when |this| is destroyed.  However, that bug appears to already exist regardless of your patch.  Would you be willing to file a bug on this issue so we don't forget to fix it?

[Adam Barth]

Comment on attachment 157223 [details]
Patch

I don't feel confident in FindInPageCoordinates.* ...  I'm going to ask someone more expert in the rendering tree to take a look.

Modulo that and the minor comments above, I'm ok with this patch.  After we get the chromium-android branch upstream, we should spend some time factoring find-in-page out of WebFrameImpl.cpp.  I'm happy to help with that, and I don't think it should hold up this patch.

[Darin Fisher (:fishd, Google)]

Comment on attachment 157223 [details]
Patch

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?

[Adam Barth]

> 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.

That's where the code is in the chromium-android branch.  I asked Leandro to move it into WebCore, but perhaps keeping it in the WebKit layer is best.

> Is this patch missing some changes to WebFrame.h?

We landed those as stubs in a separate patch:

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

[Leandro Graciá Gil]

Comment on attachment 157223 [details]
Patch

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

Replying to a few comments. Taking care of the rest tomorrow.

>> 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.

This file was originally in WebKit (in downstream code), but we found that it was composed only of WebCore dependencies. Structurally makes more sense in WebCore more than in WebKit, although I agree that chromium is the only port requiring it now. I'm happy to take whatever approach.

Adam, what do you think about this? I think you were one who suggested moving this to WebCore.

>> Source/WebKit/chromium/src/WebFrameImpl.h:477
>> +    WebSize m_contentsSizeForCurrentFindMatchRects;
> 
> WebSize -> IntSize
> 
> 
> Is this patch missing some changes to WebFrame.h?

These were added separately by bug 93110 in order to achieve WebKit API compatibility for the Android port.

>> Source/WebKit/chromium/tests/WebFrameTest.cpp:934
>> +    mainFrame->didChangeContentsSize(WebSize(800, 600));
> 
> Shouldn't we call resize on the view and have this function called automatically?

Actually, it's not automatically called when calling resize upstream. It seems that the difference in enabled features of the Android port might be preventing it. Since other tests in this file explicitly call methods like this one I did the same for the sake of simplicity.

[Adam Barth]

> Adam, what do you think about this? I think you were one who suggested moving this to WebCore.

I don't have a strong opinion.  If fishd thinks this code should be in the WebKit layer, let's put it there.

> >> Source/WebKit/chromium/tests/WebFrameTest.cpp:934
> >> +    mainFrame->didChangeContentsSize(WebSize(800, 600));
> > 
> > Shouldn't we call resize on the view and have this function called automatically?
> 
> Actually, it's not automatically called when calling resize upstream. It seems that the difference in enabled features of the Android port might be preventing it. Since other tests in this file explicitly call methods like this one I did the same for the sake of simplicity.

Ok.

[Leandro Graciá Gil]

Comment on attachment 157223 [details]
Patch

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!

Done.

>>> 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.
> 
> This file was originally in WebKit (in downstream code), but we found that it was composed only of WebCore dependencies. Structurally makes more sense in WebCore more than in WebKit, although I agree that chromium is the only port requiring it now. I'm happy to take whatever approach.
> 
> Adam, what do you think about this? I think you were one who suggested moving this to WebCore.

Moved back to WebKit/chromium/src.

>> Source/WebCore/rendering/FindInPageCoordinates.h:56
>> +FloatRect findInPageRectFromRange(PassRefPtr<Range>);
> 
> PassRefPtr<Range>  ->  Range*
> 
> This function doesn't need to take ownership of the Range so there's no advantage to using PassRefPtr.  http://www.webkit.org/coding/RefPtr.html has some more information about RefPtr and PassRefPtr if you're interested.

Done.

>> Source/WebKit/chromium/src/WebFrameImpl.cpp:1996
>> +    const WebFrameImpl* activeMatchFrame = mainFrameImpl->activeMatchFrame();
> 
> We usually don't bother with the const in "const WebFrameImpl".  It's not really sensible for a WebFrameImpl to be const.

Fixed.

>> Source/WebKit/chromium/src/WebFrameImpl.cpp:1998
>> +            && activeMatchFrame->frame()->tree()->isDescendantOf(mainFrameImpl->frame());
> 
> four-space indent.

Fixed.

>> Source/WebKit/chromium/src/WebFrameImpl.cpp:2009
>> +    unsigned deadMatches = 0;
> 
> unsigned -> size_t
> 
> It looks like this number can be as large as Vector<...>::size(), which is a size_t.

Fixed.

>> Source/WebKit/chromium/src/WebFrameImpl.cpp:2035
>> +            static_cast<WebFrameImpl*>(child)->m_findMatchRectsAreValid = false;
> 
> Do we need to do a recursive walk of all the subframes?

No, we only need to update these with an invalidated ancestor. It's done this way in order to avoid repeating operations unnecessarily.

>> Source/WebKit/chromium/src/WebFrameImpl.cpp:2042
>> +    // This function should only be called on the mainframe.
> 
> This comment is redundant with the line below.  :)

Removing it. This came verbatim from other similar find-in-page methods.

>> Source/WebKit/chromium/src/WebFrameImpl.cpp:2053
>> +    // This function should only be called on the mainframe.
> 
> ditto

Done.

>> Source/WebKit/chromium/src/WebFrameImpl.cpp:2075
>> +    // This function should only be called on the mainframe.
> 
> ditto

Done.

>> Source/WebKit/chromium/src/WebFrameImpl.cpp:2135
>> +        viewImpl()->mainFrameImpl()->m_currentActiveMatchFrame = this;
> 
> This looks like it might become a dangling pointer given that no one appears to clear this pointer if/when |this| is destroyed.  However, that bug appears to already exist regardless of your patch.  Would you be willing to file a bug on this issue so we don't forget to fix it?

Sure. Here you go: https://bugs.webkit.org/show_bug.cgi?id=93732 .

>> Source/WebKit/chromium/src/WebFrameImpl.cpp:2142
>> +        executeCommand(WebString::fromUTF8("Unselect"));
> 
> Is there a more direct way to do this?  It seems odd to have to go back through the API with a string like this.

This was probably based in some early WebFrameImpl::find code. I'll be updating it to frame()->selection->clear().

>> Source/WebKit/chromium/src/WebFrameImpl.cpp:2145
>> +        if (frame()->document())
> 
> frame()->document() shouldn't ever be 0

Fixed.

>> Source/WebKit/chromium/src/WebFrameImpl.cpp:2365
>> +void WebFrameImpl::didChangeContentsSize(const IntSize& size)
> 
> Do we need to call the base class here?

No base class related: it's called from ChromeClientImpl (see below).

>> Source/WebKit/chromium/src/WebFrameImpl.h:252
>> +    // Called when the size of the contained document changes.
> 
> This comment doesn't add any value and should be removed.

Done.

>> Source/WebKit/chromium/src/WebFrameImpl.h:253
>> +    void didChangeContentsSize(const WebCore::IntSize&);
> 
> I couldn't find the call site for this function.  Is it a virtual function override?  If so, we should mark it virtual and add the OVERRIDE keyword.

Found it. I was missing a one-line change in ChromeClientImpl.cpp. That's the one calling this method directly to WebFrameImpl.

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

Done.

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

This one is providing the output of the API function selectNearestFindMatch. Using IntRect leads to an incompatible pointer that enforces a pointless local conversion.

>> 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.

This one should use WebFloatRect. The reason is that this method appends the local results of the frame to the global result of the API function findMatchRects. If we use FloatRect here we will enforce a pointless conversion of all the rects on every frame.

>>> Source/WebKit/chromium/src/WebFrameImpl.h:477
>>> +    WebSize m_contentsSizeForCurrentFindMatchRects;
>> 
>> WebSize -> IntSize
>> 
>> 
>> Is this patch missing some changes to WebFrame.h?
> 
> These were added separately by bug 93110 in order to achieve WebKit API compatibility for the Android port.

WebSize -> IntSize changed.

>> Source/WebKit/chromium/tests/WebFrameTest.cpp:847
>> +    { }
> 
> Technically these brackets should be on separate lines.

Fixed.

>>> Source/WebKit/chromium/tests/WebFrameTest.cpp:934
>>> +    mainFrame->didChangeContentsSize(WebSize(800, 600));
>> 
>> Shouldn't we call resize on the view and have this function called automatically?
> 
> Actually, it's not automatically called when calling resize upstream. It seems that the difference in enabled features of the Android port might be preventing it. Since other tests in this file explicitly call methods like this one I did the same for the sake of simplicity.

Fixed the issue: a one-line change was missing in ChromeClientImpl. It should be working with Resize now.

[Leandro Graciá Gil]

Created attachment 157791 [details]
Patch

[WebKit Review Bot]

Comment on attachment 157791 [details]
Patch

Attachment 157791 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/13481014

New failing tests:
WebFrameTest.FindInPageMatchRects

[WebKit Review Bot]

Created attachment 157824 [details]
Archive of layout-test-results from gce-cr-linux-08

The attached test failures were seen while running run-webkit-tests on the chromium-ews.
Bot: gce-cr-linux-08  Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'>  Platform: Linux-2.6.39-gcg-201203291735-x86_64-with-Ubuntu-10.04-lucid

[Leandro Graciá Gil]

Created attachment 157834 [details]
Patch

[Leandro Graciá Gil]

(In reply to comment #32)
> Created an attachment (id=157834) [details]
> Patch

I can't reproduce the unit test failure locally. I'm adding an extra webkit_support::RunAllPendingMessages() after resizing just in case. If this doesn't solve the problem I'll revert to calling WebFrameImpl::didChangeContentsSize directly as done in previous patches.

[Adam Barth]

Comment on attachment 157834 [details]
Patch

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.

[WebKit Review Bot]

Comment on attachment 157834 [details]
Patch

Clearing flags on attachment: 157834

Committed r125379: <http://trac.webkit.org/changeset/125379>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Yuta Kitamura]

Android build fix commited: http://trac.webkit.org/changeset/125385

[Leandro Graciá Gil]

Comment on attachment 157834 [details]
Patch

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

Here's the new CL for the pending nits. Let's continue the review there: https://bugs.webkit.org/show_bug.cgi?id=93817

>> Source/WebCore/dom/Range.cpp:1964
>> +FloatRect Range::transformFriendlyBoundingBox() const
> 
> Why don't we need to call updateLayoutIgnorePendingStylesheets like we do above?

Adding it sounds good to me. I'll fix it.

>> Source/WebCore/dom/Range.cpp:1966
>> +    FloatRect result;
> 
> nit: Idiomatically, we'd put this declaration right above the loop that populates it.

Fixing it in the next iteration of the patch.

>> Source/WebCore/dom/Range.cpp:1971
>> +    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.

Fixing it in the next iteration of the patch.

>> Source/WebCore/dom/Range.h:129
>> +    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().

IMO the problem here is not the name of "transformFriendlyBoundingBox", as that's exactly what it does. The confusion here comes from the rather ambiguous name of boundingRect. The main difference between both is that boundingRect uses getBorderAndTextQuads, which involves Document::adjustFloatQuadsForScrollAndAbsoluteZoomAndFrameScale. This latter method involves the visible rect of the view, the frame scale factor and the zoom, therefore returning something very different to what the usual bounding box methods return across different WebCore objects (just raw absolute coordinates without any view or scale consideration).

I'm not sure if the correct solution would be to rename boundingRect, given the fact that I'm not sure what ports actually use it or for what exact purpose. I'm also not sure what a proper name for it would be. Any suggestions?

>> 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.

I guess it's fine to change it now. In previous iterations, where this method was exposed to WebFrameImpl, the output parameter was optional and therefore a pointer. I'll fix it.

>> Source/WebKit/chromium/src/WebFrameImpl.cpp:1675
>> +#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.

Me neither. I'm happy to change this if anybody has a better suggestion. In any case we still could add more cases to the #if if we need so.

[Adam Barth]

> >> Source/WebCore/dom/Range.h:129
> >> +    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().
> 
> IMO the problem here is not the name of "transformFriendlyBoundingBox", as that's exactly what it does. The confusion here comes from the rather ambiguous name of boundingRect. The main difference between both is that boundingRect uses getBorderAndTextQuads, which involves Document::adjustFloatQuadsForScrollAndAbsoluteZoomAndFrameScale. This latter method involves the visible rect of the view, the frame scale factor and the zoom, therefore returning something very different to what the usual bounding box methods return across different WebCore objects (just raw absolute coordinates without any view or scale consideration).
> 
> I'm not sure if the correct solution would be to rename boundingRect, given the fact that I'm not sure what ports actually use it or for what exact purpose. I'm also not sure what a proper name for it would be. Any suggestions?

Perhaps rniwa has a suggestion here?

[Leandro Graciá Gil]

rniwa: this patch added a transformFriendlyBoundingBox method to Range as we require it to achieve CSS transform compatibility in other features of this patch. However we found an existing method called boundingRect which despite its ambiguous name involves page scaling, zooming and the view rect. We were wondering what do you think it should be the correct naming approach for these two.

Note that transformFriendlyBoundingBox is just the same as boundingBox but using quads and its enclosing rect. Also note that boundingRect was not provided by this patch and we're not sure which ports use it, in case a rename is appropriate.

I'll address any suggestions you might have in a new patch.
Thanks.

[Julien Chaffraix]

Comment on attachment 157834 [details]
Patch

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

Sorry for not looking at this change sooner. Overall, it's OK but there are some rough edges or cases that would need more attention.

I believe your logic to work with different writing-modes or direction. Unfortunately the testing doesn't cover these cases so it would need to be extended.

> Source/WebKit/chromium/src/FindInPageCoordinates.cpp:55
> +    const RenderObject* container = renderer->container();

I think this should use containingBlock and not container. The difference in behavior is subtle (it has to do with container being used for invalidations vs containingBlock when you need your CSS 'containing block'). The other difference is that containingBlock doesn't work on unrooted tree which shouldn't be an issue here.

Using containingBlock() would also simplify the code as:
* you can ASSERT that you have a containing block (or that you are the RenderView).
* containingBlock returns a RenderBlock so no need to actually to check if it's a RenderBox.

> Source/WebKit/chromium/src/FindInPageCoordinates.cpp:58
> +        if (containerBoundingBox)
> +            *containerBoundingBox = FloatRect();

The containingBoundingBox logic is pretty confusing. You clear it here but then if you have no container, it's probably that you are the RenderView which means you will reset the value in the while loop below.

> Source/WebKit/chromium/src/FindInPageCoordinates.cpp:89
> +    // Fixed positions do not make sense in this coordinate system, but need to leave consistent tickmarks.
> +    // So, use their position when the view is not scrolled, like an absolute position.

Doesn't that mean that fixed positioned object's tickmarks are badly placed?

> Source/WebKit/chromium/src/FindInPageCoordinates.cpp:106
> +    renderer = renderer->container();

Let's avoid parameter re-use, it makes the code less readable IMHO. On top of that, you have lost the original renderer.

> Source/WebKit/chromium/src/FindInPageCoordinates.cpp:112
> +        for (const RenderObject* container = renderer->container(); container; renderer = container, container = container->container()) {

As part of this loop, you will skip any <iframe> (as you never actually call toNormalizedRect on the frame owner). Is the <iframe> case properly handled? Ideally this should be tested.

[Leandro Graciá Gil]

Comment on attachment 157834 [details]
Patch

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

Thanks for your comments regarding the coordinates code. One detail that you should know: there has been a small follow-up patch to address Adam's comments. It should not affect very much your review, but in any case you can find it here: https://bugs.webkit.org/show_bug.cgi?id=93817

>> Source/WebKit/chromium/src/FindInPageCoordinates.cpp:55
>> +    const RenderObject* container = renderer->container();
> 
> I think this should use containingBlock and not container. The difference in behavior is subtle (it has to do with container being used for invalidations vs containingBlock when you need your CSS 'containing block'). The other difference is that containingBlock doesn't work on unrooted tree which shouldn't be an issue here.
> 
> Using containingBlock() would also simplify the code as:
> * you can ASSERT that you have a containing block (or that you are the RenderView).
> * containingBlock returns a RenderBlock so no need to actually to check if it's a RenderBox.

Sounds good to me. Just one question: overflows are defined in RenderBox rather than RenderBlock so, is it possible that going up the render tree using containingBlock might skip a RenderBox that is not a RenderBlock?

>> Source/WebKit/chromium/src/FindInPageCoordinates.cpp:58
>> +            *containerBoundingBox = FloatRect();
> 
> The containingBoundingBox logic is pretty confusing. You clear it here but then if you have no container, it's probably that you are the RenderView which means you will reset the value in the while loop below.

The while loop below should prevent you to call toNormalizedRect if you don't have a container. This is just a safeguard.

>> Source/WebKit/chromium/src/FindInPageCoordinates.cpp:89
>> +    // So, use their position when the view is not scrolled, like an absolute position.
> 
> Doesn't that mean that fixed positioned object's tickmarks are badly placed?

It means that fixed positions would change as the scroll does, but that implies that we should send new tickmarks on every scroll change. We do not want to do that as our approach is to get the tickmarks from the moment we asked for them. However, we still want to have consistent tickmarks in the meaning that while finding next any active match should appear in the positions expected for the find results (assuming no dynamic changes in the page).

So, to prevent the active rect not matching its expected tickmark for fixed positions we treat these as a special case by always providing the tickmark for the no scroll case.

>> Source/WebKit/chromium/src/FindInPageCoordinates.cpp:106
>> +    renderer = renderer->container();
> 
> Let's avoid parameter re-use, it makes the code less readable IMHO. On top of that, you have lost the original renderer.

In this case it simply means that the starting renderer in the loop is the container of the original one, but I can change it if you want. Maybe rename this original renderer and use a for loop with a local variable?

>> Source/WebKit/chromium/src/FindInPageCoordinates.cpp:112
>> +        for (const RenderObject* container = renderer->container(); container; renderer = container, container = container->container()) {
> 
> As part of this loop, you will skip any <iframe> (as you never actually call toNormalizedRect on the frame owner). Is the <iframe> case properly handled? Ideally this should be tested.

The <iframe> is precisely one of the tested cases. Could you elaborate the details of the problem?

When reaching the frame's RenderView it should go up to the owner renderer, which (and please correct me if I'm wrong) should be the one corresponding to the <iframe> element in the parent frame. There it will call toNormalizedRect to compose the results form the <iframe> with the container of the <iframe> element renderer.

[Julien Chaffraix]

Comment on attachment 157834 [details]
Patch

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

>>> Source/WebKit/chromium/src/FindInPageCoordinates.cpp:55
>>> +    const RenderObject* container = renderer->container();
>> 
>> I think this should use containingBlock and not container. The difference in behavior is subtle (it has to do with container being used for invalidations vs containingBlock when you need your CSS 'containing block'). The other difference is that containingBlock doesn't work on unrooted tree which shouldn't be an issue here.
>> 
>> Using containingBlock() would also simplify the code as:
>> * you can ASSERT that you have a containing block (or that you are the RenderView).
>> * containingBlock returns a RenderBlock so no need to actually to check if it's a RenderBox.
> 
> Sounds good to me. Just one question: overflows are defined in RenderBox rather than RenderBlock so, is it possible that going up the render tree using containingBlock might skip a RenderBox that is not a RenderBlock?

It's possible in theory to skip a RenderBox. I can only think of a table context where this can happen and it would be good in this case (table parts are laid out relative to the containing table, which should be the one tracking the overflow). In CSS 'overflow' applies to blocks as it only makes sense to track the overflow on them.

Btw you don't test tables which would be a good addition to your testing as they tend to work differently.

>>> Source/WebKit/chromium/src/FindInPageCoordinates.cpp:106
>>> +    renderer = renderer->container();
>> 
>> Let's avoid parameter re-use, it makes the code less readable IMHO. On top of that, you have lost the original renderer.
> 
> In this case it simply means that the starting renderer in the loop is the container of the original one, but I can change it if you want. Maybe rename this original renderer and use a for loop with a local variable?

Sounds good to me.

>>> Source/WebKit/chromium/src/FindInPageCoordinates.cpp:112
>>> +        for (const RenderObject* container = renderer->container(); container; renderer = container, container = container->container()) {
>> 
>> As part of this loop, you will skip any <iframe> (as you never actually call toNormalizedRect on the frame owner). Is the <iframe> case properly handled? Ideally this should be tested.
> 
> The <iframe> is precisely one of the tested cases. Could you elaborate the details of the problem?
> 
> When reaching the frame's RenderView it should go up to the owner renderer, which (and please correct me if I'm wrong) should be the one corresponding to the <iframe> element in the parent frame. There it will call toNormalizedRect to compose the results form the <iframe> with the container of the <iframe> element renderer.

OK, I missed the iframe case. I thought you would need to call toNormalizedRect on the frame owner but it seems to work as expected.

[Ryosuke Niwa]

Comment on attachment 157834 [details]
Patch

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

>>> Source/WebCore/dom/Range.h:129
>>> +    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().
> 
> IMO the problem here is not the name of "transformFriendlyBoundingBox", as that's exactly what it does. The confusion here comes from the rather ambiguous name of boundingRect. The main difference between both is that boundingRect uses getBorderAndTextQuads, which involves Document::adjustFloatQuadsForScrollAndAbsoluteZoomAndFrameScale. This latter method involves the visible rect of the view, the frame scale factor and the zoom, therefore returning something very different to what the usual bounding box methods return across different WebCore objects (just raw absolute coordinates without any view or scale consideration).
> 
> I'm not sure if the correct solution would be to rename boundingRect, given the fact that I'm not sure what ports actually use it or for what exact purpose. I'm also not sure what a proper name for it would be. Any suggestions?

Why are we adding a new bounding box to Range? We're trying to get rid of renderer dependencies from Range :(
This is horrible.

[Ryosuke Niwa]

Please don't put [chromium] if you're modifying WebCore code at all. I had ignored this bug because I thought it only affected code in Source/WebKit/chromium.

[Ryosuke Niwa]

+smfr for the bounding rect change.

[Darin Adler]

What does “transform-friendly” mean? Does it mean “transformed”?

[Darin Adler]

I think the use of the term “friendly” here is confusing and “unfriendly”.

[Simon Fraser (smfr)]

I hate "friendly" too. It means "respecting CSS transforms, which should be the default behavior most of the time.

[Adam Barth]

The function was moved/renamed in http://trac.webkit.org/changeset/126074

The term "transform-friendly" term still exists in the comment block above these functions however.

[Leandro Graciá Gil]

(In reply to comment #50)
> The function was moved/renamed in http://trac.webkit.org/changeset/126074
> 
> The term "transform-friendly" term still exists in the comment block above these functions however.

That comment was not introduced by this patch. In fact it was one of the reasons why the method was originally named "transformFriendlyBoundingBox".