[webkit-reviews] review canceled: [Bug 74864] There is additional space not belonged to a table between the table cells : [Attachment 156642] Patch

[bugzilla-daemon at webkit.org]

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 156642: Patch
https://bugs.webkit.org/attachment.cgi?id=156642&action=review

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


hittest-tablecell-edge.html is a subset of
hittest-tablecell-edge-empty-columns.html. Do we really need those 2 cases or
would hittest-tablecell-edge-empty-columns.html be enough?

I would support landing a test with borders on the table-cells similar to
hittest-tablecell-edge-empty-columns.html.

Also, it would be nice to do testing on several rows (it shouldn't be impacted
by your change but that should be easy to add as part of your change).

Due to the tabs, this change will need another round but it looks good.

> Source/WebCore/rendering/RenderTableSection.cpp:1122
> +    unsigned nextColumn = std::upper_bound(columnPos.begin(),
columnPos.end(), flippedRect.x()) - columnPos.begin();

Please amend the FIXME in spannedRows as it's not true anymore. Don't forget to
mention that this matches other browsers.

> LayoutTests/fast/dom/hittest-tablecell-edge.html:21
> +	['td1', 'td2', 'td3'].forEach(function(a) {
> +		ele[a] = document.getElementById(a);
> +	});
> +	
> +	['td1', 'td2', 'td3'].forEach(function(a) {
> +		hittest(ele[a], a);
> +	});
> +	
> +	['td1', 'td2', 'td3'].forEach(function(a) {
> +		ele[a].innerHTML = '';
> +	});

We can maybe fold all those forEach into one?

> LayoutTests/fast/dom/hittest-tablecell-edge.html:32
> +	y = ele.getBoundingClientRect().bottom/2;
> +	debug('Executing for element: '+orgElement);
> +	debug('Node at edge-1: '+document.elementFromPoint(x-1,y).id);
> +	debug('Node at edge: '+document.elementFromPoint(x,y).id);
> +	debug('Node at edge+1: '+document.elementFromPoint(x+1,y).id);

It's prefered to use WebKit style for tests.

> LayoutTests/fast/dom/hittest-tablecell-edge.html:51
> +			<td id="td3">1</td>

Those are tabs, your patch will be rejected by our commit-hook.