Bug 94343 – [Chromium] Find-in-page coordinates should use containingBlock

• Component: WebKit Misc.
• Status: RESOLVED FIXED

[Leandro Graciá Gil]

The current find-in-page coordinates implementation uses the container method to climb the render tree. However, it would be more correct and convenient to use containerBlock instead.

[Leandro Graciá Gil]

(In reply to comment #0)
> The current find-in-page coordinates implementation uses the container method to climb the render tree. However, it would be more correct and convenient to use containerBlock instead.

(In reply to comment #0)
> The current find-in-page coordinates implementation uses the container method to climb the render tree. However, it would be more correct and convenient to use containerBlock instead.

containerBlock -> containingBlock.

[Leandro Graciá Gil]

Created attachment 159112 [details]
Patch

[Julien Chaffraix]

Comment on attachment 159112 [details]
Patch

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

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

ASSERT(container || renderer->isRenderView()); ?

> Source/WebKit/chromium/src/FindInPageCoordinates.cpp:104
> +            FloatRect normalizedBoxRect = toNormalizedRect(renderer->absoluteBoundingBoxRect(), renderer);

You don't need to pass renderer->absoluteBoundingBoxRect() here. You could compute it in toNormalizedRect after you passed the |container| NULL-check.

> Source/WebKit/chromium/tests/WebFrameTest.cpp:978
> +    // Results 13, 12 and 14 should be one above the other in that order because of the rowspan.

and because HTML table cells have vertical-align: middle set (CSS table cells have vertical-align: baseline which yields to a different result).

[Leandro Graciá Gil]

Created attachment 159524 [details]
Patch

[Leandro Graciá Gil]

Comment on attachment 159112 [details]
Patch

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

>> Source/WebKit/chromium/src/FindInPageCoordinates.cpp:56
>> +    const RenderBlock* container = renderer->containingBlock();
> 
> ASSERT(container || renderer->isRenderView()); ?

Done.

>> Source/WebKit/chromium/src/FindInPageCoordinates.cpp:104
>> +            FloatRect normalizedBoxRect = toNormalizedRect(renderer->absoluteBoundingBoxRect(), renderer);
> 
> You don't need to pass renderer->absoluteBoundingBoxRect() here. You could compute it in toNormalizedRect after you passed the |container| NULL-check.

toNormalizedRect is also used above with the input rect from the range. That's why that call is not already inside toNormalizedRect.

>> Source/WebKit/chromium/tests/WebFrameTest.cpp:978
>> +    // Results 13, 12 and 14 should be one above the other in that order because of the rowspan.
> 
> and because HTML table cells have vertical-align: middle set (CSS table cells have vertical-align: baseline which yields to a different result).

True, good point. Thanks.

[Leandro Graciá Gil]

Any pending issues here?

(In reply to comment #5)
> (From update of attachment 159112 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=159112&action=review
> 
> >> Source/WebKit/chromium/src/FindInPageCoordinates.cpp:56
> >> +    const RenderBlock* container = renderer->containingBlock();
> > 
> > ASSERT(container || renderer->isRenderView()); ?
> 
> Done.
> 
> >> Source/WebKit/chromium/src/FindInPageCoordinates.cpp:104
> >> +            FloatRect normalizedBoxRect = toNormalizedRect(renderer->absoluteBoundingBoxRect(), renderer);
> > 
> > You don't need to pass renderer->absoluteBoundingBoxRect() here. You could compute it in toNormalizedRect after you passed the |container| NULL-check.
> 
> toNormalizedRect is also used above with the input rect from the range. That's why that call is not already inside toNormalizedRect.
> 
> >> Source/WebKit/chromium/tests/WebFrameTest.cpp:978
> >> +    // Results 13, 12 and 14 should be one above the other in that order because of the rowspan.
> > 
> > and because HTML table cells have vertical-align: middle set (CSS table cells have vertical-align: baseline which yields to a different result).
> 
> True, good point. Thanks.

[WebKit Review Bot]

Comment on attachment 159524 [details]
Patch

Clearing flags on attachment: 159524

Committed r126204: <http://trac.webkit.org/changeset/126204>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.