Bug 94343 - [Chromium] Find-in-page coordinates should use containingBlock
Summary: [Chromium] Find-in-page coordinates should use containingBlock
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit Misc. (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Leandro Graciá Gil
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2012-08-17 06:47 PDT by Leandro Graciá Gil
Modified: 2012-08-21 16:05 PDT (History)
2 users (show)

See Also:


Attachments
Patch (11.70 KB, patch)
2012-08-17 06:53 PDT, Leandro Graciá Gil
no flags Details | Formatted Diff | Diff
Patch (11.90 KB, patch)
2012-08-20 14:34 PDT, Leandro Graciá Gil
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Leandro Graciá Gil 2012-08-17 06:47:06 PDT
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.
Comment 1 Leandro Graciá Gil 2012-08-17 06:49:51 PDT
(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.
Comment 2 Leandro Graciá Gil 2012-08-17 06:53:55 PDT
Created attachment 159112 [details]
Patch
Comment 3 Julien Chaffraix 2012-08-20 11:33:44 PDT
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).
Comment 4 Leandro Graciá Gil 2012-08-20 14:34:47 PDT
Created attachment 159524 [details]
Patch
Comment 5 Leandro Graciá Gil 2012-08-20 14:35:05 PDT
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.
Comment 6 Leandro Graciá Gil 2012-08-21 11:47:42 PDT
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.
Comment 7 WebKit Review Bot 2012-08-21 16:05:43 PDT
Comment on attachment 159524 [details]
Patch

Clearing flags on attachment: 159524

Committed r126204: <http://trac.webkit.org/changeset/126204>
Comment 8 WebKit Review Bot 2012-08-21 16:05:46 PDT
All reviewed patches have been landed.  Closing bug.