[webkit-reviews] review canceled: [Bug 74864] There is additional space not belonged to a table between the table cells : [Attachment 156934] Patch
bugzilla-daemon at webkit.org
bugzilla-daemon at webkit.org
Tue Aug 7 12:18:00 PDT 2012
Julien Chaffraix <jchaffraix at webkit.org> has canceled Arpita Bahuguna
<arpitabahuguna at gmail.com>'s request for review:
Bug 74864: There is additional space not belonged to a table between the table
cells
https://bugs.webkit.org/show_bug.cgi?id=74864
Attachment 156934: Patch
https://bugs.webkit.org/attachment.cgi?id=156934&action=review
------- Additional Comments from Julien Chaffraix <jchaffraix at webkit.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=156934&action=review
Allan was right, sorry for being unclear.
> Source/WebCore/rendering/RenderTableSection.cpp:1121
> + // The use of upper_bound algorithm makes the behavior of hittest (on
table-cell edges) in accordance
> + // with other browsers.
I think you should substantiate on the 'why' we match other browsers (it's good
to show that you tested it and found us in compliance but it's a bit dry). How
is upper_bound better than lower_bound here? This would be a high value comment
for anyone reading this code.
> Source/WebCore/rendering/RenderTableSection.cpp:1122
> + // https://bugs.webkit.org/show_bug.cgi?id=74864
I don't think adding the bug number adds any information to the change. My rule
of thumb for including a bug number is either to track an existing failure or
because the bug has a discussion that adds a lot to the understanding of how
the code is structured or why it behaves this way.
> LayoutTests/fast/dom/hittest-tablecell-bottom-edge.html:24
> + x = ele.getBoundingClientRect().right/2;
Nit: middleX may be more readable in the output. (not repeated for the other
tests)
More information about the webkit-reviews
mailing list