[webkit-reviews] review denied: [Bug 86605] Rect-based hittesting doesn't work in tables. : [Attachment 144048] Patch

Fri May 25 13:14:36 PDT 2012

Julien Chaffraix <jchaffraix at webkit.org> has denied Allan Sandfeld Jensen
<allan.jensen at nokia.com>'s request for review:
Bug 86605: Rect-based hittesting doesn't work in tables.
https://bugs.webkit.org/show_bug.cgi?id=86605

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

------- Additional Comments from Julien Chaffraix <jchaffraix at webkit.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=144048&action=review

> Source/WebCore/ChangeLog:9
> +	   After binary look-up of row and cell, continue to test following
> +	   cells that intersect with the area of the hit-test point.

This is not a useful description of what you are fixing nor really how.

Btw, you do a binary search on the rows and *columns*, not cells.

> Source/WebCore/rendering/RenderTableSection.cpp:1346
> +    LayoutUnit offsetStartInColumnDirection, offsetEndInColumnDirection;

This is not the rule in WebKit to define 2 variables on one line. Those should
be split on 2 lines.

> Source/WebCore/rendering/RenderTableSection.cpp:1363
> +    if (style()->isHorizontalWritingMode()) {
> +	   if (!style()->isFlippedBlocksWritingMode()) {
> +	       offsetStartInColumnDirection = hitTestRect.y();
> +	       offsetEndInColumnDirection = hitTestRect.maxY();
> +	   } else {
> +	       offsetStartInColumnDirection = height() - hitTestRect.maxY();
> +	       offsetEndInColumnDirection = height() - hitTestRect.y();
> +	   }
> +    } else {
> +	   if (!style()->isFlippedBlocksWritingMode()) {
> +	       offsetStartInColumnDirection = hitTestRect.x();
> +	       offsetEndInColumnDirection = hitTestRect.maxX();
> +	   } else {
> +	       offsetStartInColumnDirection = width() - hitTestRect.maxX();
> +	       offsetEndInColumnDirection = width() - hitTestRect.x();
> +	   }
>      }

AFAICT this should use the table's style as we don't allow sections to have a
writing mode different from the table.

Also why is this not using RenderBox::flipForWritingMode?

> Source/WebCore/rendering/RenderTableSection.cpp:1392
> +    }

Same 2 comments as above.

> Source/WebCore/rendering/RenderTableSection.cpp:1417
> +	       ++hitColumn;

The inner loop should go into its own function. That should make the code more
readable.

> Source/WebCore/rendering/RenderTableSection.cpp:1418
> +	   } while (hitColumn < (columnPos.size() - 1) && columnPos[hitColumn]
<= offsetEndInRowDirection);

How is a do ... while any better than a for?

For the record, this made the code so confusing that I missed that you kept the
same direction of look-ups above.

> LayoutTests/fast/dom/nodesFromRect-table.html:1
> +<html>

Please add the proper doctype as you are not testing quirks mode:
<!DOCTYPE html>

> LayoutTests/fast/dom/nodesFromRect-table.html:22
> +<body id="body">

is this id used anywhere?

> LayoutTests/fast/dom/nodesFromRect-table.html:48
> +    <script type="application/javascript">

No need for the type, this is HTML.

> LayoutTests/fast/dom/nodesFromRect-table.html:53
> +		   layoutTestController.dumpAsText();
> +		   layoutTestController.waitUntilDone();

You don't need those 2 lines: js-test-pre.js already calls the former, the
latter is unneeded as we stop the test _after_ the load event handler.

> LayoutTests/fast/dom/nodesFromRect-table.html:85
> +	       check(190, 175, 10, 20, 10, 20, [e.td22, e.td23, e.testtable]);
> +	       check(260, 275, 10, 20, 10, 20, [e.td43, e.td44, e.testtable]);
> +	       check(175, 190, 20, 10, 20, 10, [e.td22, e.td32, e.testtable]);
> +	       check(275, 260, 20, 10, 20, 10, [e.td34, e.td44, e.testtable]);
> +	       check(190, 190, 20, 20, 20, 20, [e.td22, e.td23, e.td32, e.td33,
e.testtable]);

Interestingly you don't get the rows, I am not 100% this is expected here as
you have some spacing between your cells. I think you should define a table
section by adding a <tbody id="tbody"> around your rows (here it's anonymous
and maybe that's why you don't see it in your results).

> LayoutTests/fast/dom/nodesFromRect-table.html:98
> +	       if (window.layoutTestController)
> +		   layoutTestController.notifyDone();

This is unneeded too.