Bug 74864 - There is additional space not belonged to a table between the table cells
Summary: There is additional space not belonged to a table between the table cells
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: DOM (show other bugs)
Version: 528+ (Nightly build)
Hardware: PC Unspecified
: P2 Normal
Assignee: Nobody
URL:
Keywords: HasReduction
: 90181 (view as bug list)
Depends on:
Blocks:
 
Reported: 2011-12-19 07:59 PST by Vladimir
Modified: 2012-08-15 11:07 PDT (History)
11 users (show)

See Also:


Attachments
Obsolete attachment (1.59 MB, application/x-zip-compressed)
2011-12-19 07:59 PST, Vladimir
no flags Details
Html page with to reproduce the issue and a video that demonstrates the issue (440.43 KB, application/x-zip-compressed)
2011-12-21 04:21 PST, Vladimir
no flags Details
Inline repro (from the previous test attachment) - hover over the middle border to see the bug (447 bytes, text/html)
2012-03-18 08:23 PDT, Julien Chaffraix
no flags Details
Patch (10.80 KB, patch)
2012-08-04 04:56 PDT, Arpita Bahuguna
no flags Details | Formatted Diff | Diff
Patch (9.62 KB, text/plain)
2012-08-06 02:42 PDT, Arpita Bahuguna
no flags Details
Patch (9.56 KB, patch)
2012-08-06 03:01 PDT, Arpita Bahuguna
no flags Details | Formatted Diff | Diff
Patch (47.33 KB, patch)
2012-08-07 07:17 PDT, Arpita Bahuguna
no flags Details | Formatted Diff | Diff
Patch (48.55 KB, patch)
2012-08-09 05:14 PDT, Arpita Bahuguna
no flags Details | Formatted Diff | Diff
Patch (48.59 KB, patch)
2012-08-11 01:57 PDT, Arpita Bahuguna
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from gce-cr-linux-05 (610.96 KB, application/zip)
2012-08-11 02:33 PDT, WebKit Review Bot
no flags Details
Patch (48.50 KB, patch)
2012-08-11 03:19 PDT, Arpita Bahuguna
no flags Details | Formatted Diff | Diff
Patch (47.97 KB, patch)
2012-08-13 22:23 PDT, Arpita Bahuguna
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Vladimir 2011-12-19 07:59:54 PST
Created attachment 119864 [details]
Obsolete attachment

If a mouse cursor hovers between cells of the following table:

<head>
    <title></title>
    <style>
        td
        {
        	padding: 5px;
        	cursor: pointer;
        	border: solid 2px black;
        }
    </style>
</head>
<body>
    <table cellpadding="0" cellspacing="0" border="0">
        <tr>
            <td>1</td>
            <td>2</td>
        </tr>
    </table>
</body>
</html>

the cursor is not considered as hovered over the table (see the attached video).
Comment 1 Vladimir 2011-12-21 04:21:12 PST
Created attachment 120170 [details]
Html page with to reproduce the issue and a video that demonstrates the issue
Comment 2 Julien Chaffraix 2012-03-18 08:23:01 PDT
Created attachment 132491 [details]
Inline repro (from the previous test attachment) - hover over the middle border to see the bug

Confirmed on ToT.

It looks like an edge issue on our hit testing code.
Comment 3 Tony Chang 2012-07-30 11:23:30 PDT
*** Bug 90181 has been marked as a duplicate of this bug. ***
Comment 4 Arpita Bahuguna 2012-08-04 04:56:07 PDT
Created attachment 156530 [details]
Patch
Comment 5 Allan Sandfeld Jensen 2012-08-04 16:57:40 PDT
Good catch, but does this solve the same case for a table with cellspacing? I guess this code will send the hit-test on to test the next cell, but the hit-test will still not be caught because it not actually inside the border-box of the next cell? Perhaps both cases should actually hit the row or table-section instead?

Btw, i think the test-cases could be a little simpler if they used document.elementAtPoint instead of relying on sending and catching events.
Comment 6 Arpita Bahuguna 2012-08-06 02:28:12 PDT
(In reply to comment #5)
> Good catch, but does this solve the same case for a table with cellspacing? I guess this code will send the hit-test on to test the next cell, but the hit-test will still not be caught because it not actually inside the border-box of the next cell? Perhaps both cases should actually hit the row or table-section instead?
> 
Hi Allan, thank-you for your comment and apologies for the delayed response.
You are right in assuming that this change does not fix the issue when a table with cellspacing is specified. My fix is simply for the first pixel of the column not being recognized by our hittest logic.
I can perhaps raise another bug for the cellspacing scenario and work on it separately?

> Btw, i think the test-cases could be a little simpler if they used document.elementAtPoint instead of relying on sending and catching events.
I shall upload another patch by using document.elementAtPoint instead; thanks for pointing it out. :)
Comment 7 Arpita Bahuguna 2012-08-06 02:42:13 PDT
Created attachment 156637 [details]
Patch
Comment 8 Allan Sandfeld Jensen 2012-08-06 02:50:38 PDT
Comment on attachment 156530 [details]
Patch

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

> Source/WebCore/rendering/RenderTableSection.cpp:1121
>      // Find the first columnt that starts after rect left.

Btw, feel free to fix my typo.
Comment 9 Allan Sandfeld Jensen 2012-08-06 02:53:40 PDT
(In reply to comment #7)
> Created an attachment (id=156637) [details]
> Patch

I can't provide an r+ as a non-reviewer, but it looks good to me now and  the test-case is very informative.
Comment 10 Arpita Bahuguna 2012-08-06 03:01:04 PDT
Created attachment 156642 [details]
Patch
Comment 11 Arpita Bahuguna 2012-08-06 03:05:28 PDT
(In reply to comment #9)
> (In reply to comment #7)
> > Created an attachment (id=156637) [details] [details]
> > Patch
> 
> I can't provide an r+ as a non-reviewer, but it looks good to me now and  the test-case is very informative.

(In reply to comment #8)
> (From update of attachment 156530 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=156530&action=review
> 
> > Source/WebCore/rendering/RenderTableSection.cpp:1121
> >      // Find the first columnt that starts after rect left.
> 
> Btw, feel free to fix my typo.

Done! and thanks for your positive feedback. :)
Also, I was looking at perhaps raising another bug which removes the following FIXME from the code (in RenderTableSection::spannedRows()):

 // FIXME: Upper_bound might not be the correct algorithm here since it might skip empty rows, but it is
    // consistent with behavior in the former point based hit-testing (but inconsistent with spannedColumns).

I could add testcases with empty <TR>s to verify upper_bound() algorithm's behavior.
Would appreciate your thoughts on the same.
Comment 12 Allan Sandfeld Jensen 2012-08-06 03:40:05 PDT
> (In reply to comment #8)
> Also, I was looking at perhaps raising another bug which removes the following FIXME from the code (in RenderTableSection::spannedRows()):
> 
>  // FIXME: Upper_bound might not be the correct algorithm here since it might skip empty rows, but it is
>     // consistent with behavior in the former point based hit-testing (but inconsistent with spannedColumns).
> 
> I could add testcases with empty <TR>s to verify upper_bound() algorithm's behavior.

Make a test-case verifying or invalidating the problem would be great.

For this patch though, a guess the comment actually needs to be updated to remove the claim that it is inconsistant with spannedColumns, since you changed that.

But talking about what happens with empty-cells made me take another look at your empty-cell test-case. Since 'empty-cells' default to 'show' which means td3 and td4 renders their own border, doesn't your test-case actually demonstrating a bug when it says elementAtPoint on the borders of td3 hits td5? And a really big bug when hitting the td3 border from td4 suddenly hits td2?
Comment 13 Allan Sandfeld Jensen 2012-08-06 03:49:02 PDT
(In reply to comment #12)
> But talking about what happens with empty-cells made me take another look at your empty-cell test-case. Since 'empty-cells' default to 'show' which means td3 and td4 renders their own border, doesn't your test-case actually demonstrating a bug when it says elementAtPoint on the borders of td3 hits td5? And a really big bug when hitting the td3 border from td4 suddenly hits td2?

Okay, so there are no borders on the table which is why it is correct. I guess that demonstrates the concern we raised about collapsed rows works correctly with collapsed cells.
Comment 14 Arpita Bahuguna 2012-08-06 04:29:03 PDT
(In reply to comment #12)
> > (In reply to comment #8)
> > Also, I was looking at perhaps raising another bug which removes the following FIXME from the code (in RenderTableSection::spannedRows()):
> > 
> >  // FIXME: Upper_bound might not be the correct algorithm here since it might skip empty rows, but it is
> >     // consistent with behavior in the former point based hit-testing (but inconsistent with spannedColumns).
> > 
> > I could add testcases with empty <TR>s to verify upper_bound() algorithm's behavior.
> 
> Make a test-case verifying or invalidating the problem would be great.
> 
> For this patch though, a guess the comment actually needs to be updated to remove the claim that it is inconsistant with spannedColumns, since you changed that.
> 
I agree, shall take care of updating this comment as well.

> But talking about what happens with empty-cells made me take another look at your empty-cell test-case. Since 'empty-cells' default to 'show' which means td3 and td4 renders their own border, doesn't your test-case actually demonstrating a bug when it says elementAtPoint on the borders of td3 hits td5? And a really big bug when hitting the td3 border from td4 suddenly hits td2?
> 
> Okay, so there are no borders on the table which is why it is correct. I guess that demonstrates the concern we raised about collapsed rows works correctly with collapsed cells.
>
I verified the empty-columns testcase, modifying it by adding borders for all table-cells. Even in that case our behavior remains consistent with that of other browsers (Opera, FF). The o/p on all browsers was the following:
Executing for element: td1
Node at edge-1: td1
Node at edge: 
Node at edge+1: 

Executing for element: td2
Node at edge-1: td2
Node at edge: 
Node at edge+1: 

Executing for element: td3
Node at edge-1: td3
Node at edge: 
Node at edge+1: 

Executing for element: td4
Node at edge-1: td4
Node at edge: 
Node at edge+1: 

Executing for element: td5
Node at edge-1: td5
Node at edge: 
Node at edge+1: 
So I suppose the change to upper_bound() does not affect in the spannedColumns() case. I need to verify for spannedRows() though.
Comment 15 Julien Chaffraix 2012-08-06 10:38:59 PDT
Comment on attachment 156642 [details]
Patch

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.
Comment 16 Arpita Bahuguna 2012-08-07 07:17:42 PDT
Created attachment 156934 [details]
Patch
Comment 17 Arpita Bahuguna 2012-08-07 07:27:40 PDT
(In reply to comment #15)
> (From update of attachment 156642 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=156642&action=review

Thank-you for the review Julien. Have uploaded another patch as per your comments.
> 
> 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).

Have now added 4 testcases, all containing empty table cells and multiple rows, that carry out the hittest on the edge of adjoining table cells, both on the right edge as well as on the bottom edge (this has been added just to verify the rows behavior).

Also, have added separate testcases that have borders specified for the table-cells to highlight the difference in the hittesting results on the edge, with and without borders.

Please note that for all these cases, our behavior is now exactly similar to that of Opera and Firefox.


> Please amend the FIXME in spannedRows as it's not true anymore. Don't forget to mention that this matches other browsers.
> 
Have removed the FIXME in spannedRows and also added a comment in spannedColumns.

> > 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?
> 
Done.

> > 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.
> 
Changed these to use shouldBe() instead.

> > 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.
I apologize for this oversight. Shall take care of this in the future.
Comment 18 Allan Sandfeld Jensen 2012-08-07 08:58:56 PDT
(In reply to comment #17)
> (In reply to comment #15)
> > > 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.
> > 
> Changed these to use shouldBe() instead.
> 
I think he meant also coding style. The style-checker doesn't enforce WebKit coding style in javascript, but it is a good idea to use it where it make sense, like adding a space after comma or around binary operators.
Comment 19 Julien Chaffraix 2012-08-07 12:18:25 PDT
Comment on attachment 156934 [details]
Patch

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)
Comment 20 Arpita Bahuguna 2012-08-09 05:14:27 PDT
Created attachment 157438 [details]
Patch
Comment 21 Arpita Bahuguna 2012-08-09 05:19:59 PDT
(In reply to comment #18)
> (In reply to comment #17)
> > (In reply to comment #15)
> > > > 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.
> > > 
> > Changed these to use shouldBe() instead.
> > 
> I think he meant also coding style. The style-checker doesn't enforce WebKit coding style in javascript, but it is a good idea to use it where it make sense, like adding a space after comma or around binary operators.

Thanks for the clarification Allan. :)
Comment 22 Arpita Bahuguna 2012-08-09 05:28:31 PDT
(In reply to comment #19)
> (From update of attachment 156934 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=156934&action=review
> 
> Allan was right, sorry for being unclear.
> 
Ah! I completely misunderstood. Have made the necessary changes now. Shall also take care of this in the future.

> > 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.
> 
Have tried to add an explanatory comment as to why upper_bound is preferred over lower_bound.

> > 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.
> 
Understand. Have removed the same.

> > 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)
Made the required changes.
Comment 23 Julien Chaffraix 2012-08-09 18:12:07 PDT
Comment on attachment 157438 [details]
Patch

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

> Source/WebCore/ChangeLog:17
> +               fast/dom/hittest-tablecell-with-borders-right-edge.html

I didn't notice that those tests were in fast/dom. It doesn't make sense to put them there - you are not testing a dom API - and they would be better with the other table tests in fast/tables. If you prefer you could even put them in a sub-directory (fast/table/testing).

> Source/WebCore/rendering/RenderTableSection.cpp:1129
> +    // of other browsers.

Woua, that's a long explanation! It's also half true as spanningRows / spanningColumns are used for hit testing but also painting.

Let's try to shorten the comment (feel free to adapt):
// lower_bound doesn't handle the edge between 2 cells properly as it would wrongly return the cell on the logical top / left.
// upper_bound properly return the cell on the logical bottom / right, which matches other browsers.

> LayoutTests/fast/dom/hittest-tablecell-bottom-edge-expected.txt:41
> +PASS document.elementFromPoint(middleX, edge - 1).id is ''

This result is interesting. Note that this means that there is a 1px gap (or more) around an empty rows. This matches the empty column case too. As this patch makes us only progress, I am in favor of filing a follow-up bug about that.

> LayoutTests/fast/dom/hittest-tablecell-with-borders-right-edge-expected.txt:47
> +PASS document.elementFromPoint(edge - 1, middleY).id is ''
> +PASS document.elementFromPoint(edge, middleY).id is ''
> +PASS document.elementFromPoint(edge + 1, middleY).id is ''

I don't think this is fine to return nothing in all those cases but you are just validating our existing behavior here.
Comment 24 Arpita Bahuguna 2012-08-11 01:57:04 PDT
Created attachment 157860 [details]
Patch
Comment 25 Arpita Bahuguna 2012-08-11 02:14:39 PDT
(In reply to comment #23)
> (From update of attachment 157438 [details])

Thanks for the review Julien.

> View in context: https://bugs.webkit.org/attachment.cgi?id=157438&action=review
> 
> > Source/WebCore/ChangeLog:17
> > +               fast/dom/hittest-tablecell-with-borders-right-edge.html
> 
> I didn't notice that those tests were in fast/dom. It doesn't make sense to put them there - you are not testing a dom API - and they would be better with the other table tests in fast/tables. If you prefer you could even put them in a sub-directory (fast/table/testing).
> 
> > Source/WebCore/rendering/RenderTableSection.cpp:1129
> > +    // of other browsers.
> 
Have added the testcases under fast/table/testing now.

> Woua, that's a long explanation! It's also half true as spanningRows / spanningColumns are used for hit testing but also painting.
> 
> Let's try to shorten the comment (feel free to adapt):
> // lower_bound doesn't handle the edge between 2 cells properly as it would wrongly return the cell on the logical top / left.
> // upper_bound properly return the cell on the logical bottom / right, which matches other browsers.
> 
Made the change.

> > LayoutTests/fast/dom/hittest-tablecell-bottom-edge-expected.txt:41
> > +PASS document.elementFromPoint(middleX, edge - 1).id is ''
> 
> This result is interesting. Note that this means that there is a 1px gap (or more) around an empty rows. This matches the empty column case too. As this patch makes us only progress, I am in favor of filing a follow-up bug about that.
> 
Yes, that's true. I shall post a bug for the same and update.
Also, currently FF seems to be the only browser returning a TableRow element for a hit test on a cellspacing area. Other browsers including WebKit do not return any underlying element. Should that be treated as a bug? Are we expected to return the TableRow element in such a case. If so, I shall raise another bug for it as well.

> > LayoutTests/fast/dom/hittest-tablecell-with-borders-right-edge-expected.txt:47
> > +PASS document.elementFromPoint(edge - 1, middleY).id is ''
> > +PASS document.elementFromPoint(edge, middleY).id is ''
> > +PASS document.elementFromPoint(edge + 1, middleY).id is ''
> 
> I don't think this is fine to return nothing in all those cases but you are just validating our existing behavior here.
Am uncertain as to what changes, if required, are to be made for these cases. Did you perhaps wish for these cases to be removed?
Comment 26 WebKit Review Bot 2012-08-11 02:33:51 PDT
Comment on attachment 157860 [details]
Patch

Attachment 157860 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/13473640

New failing tests:
fast/table/testing/hittest-tablecell-with-borders-right-edge.html
fast/table/testing/hittest-tablecell-bottom-edge.html
fast/table/testing/hittest-tablecell-right-edge.html
fast/table/testing/hittest-tablecell-with-borders-bottom-edge.html
Comment 27 WebKit Review Bot 2012-08-11 02:33:56 PDT
Created attachment 157861 [details]
Archive of layout-test-results from gce-cr-linux-05

The attached test failures were seen while running run-webkit-tests on the chromium-ews.
Bot: gce-cr-linux-05  Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'>  Platform: Linux-2.6.39-gcg-201203291735-x86_64-with-Ubuntu-10.04-lucid
Comment 28 Arpita Bahuguna 2012-08-11 03:19:41 PDT
Created attachment 157865 [details]
Patch
Comment 29 Allan Sandfeld Jensen 2012-08-11 03:31:42 PDT
(In reply to comment #28)
> Created an attachment (id=157865) [details]
> Patch

Once you have gotten an r+, you don't have to ask for it again. Just upload the patch with a Changelog that already has the reviewer filled out.
Comment 30 Arpita Bahuguna 2012-08-11 04:38:55 PDT
(In reply to comment #29)
> (In reply to comment #28)
> > Created an attachment (id=157865) [details] [details]
> > Patch
> 
> Once you have gotten an r+, you don't have to ask for it again. Just upload the patch with a Changelog that already has the reviewer filled out.
Oh! okay. I wasn't aware of that. Thanks for the info.
Shall keep that in mind henceforth.
Comment 31 Julien Chaffraix 2012-08-13 07:12:50 PDT
Comment on attachment 157865 [details]
Patch

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

> Source/WebCore/ChangeLog:17
> +               fast/table/testing/hittest-tablecell-with-borders-right-edge.html

testing is too generic and is misleading (how can a test not be testing). hit-testing is what you want if you think you need a subdirectory. Alternatively you can just put it directly under fast/table.
Comment 32 Arpita Bahuguna 2012-08-13 22:23:28 PDT
Created attachment 158215 [details]
Patch
Comment 33 Arpita Bahuguna 2012-08-13 22:44:04 PDT
(In reply to comment #31)
> (From update of attachment 157865 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=157865&action=review
> 
> > Source/WebCore/ChangeLog:17
> > +               fast/table/testing/hittest-tablecell-with-borders-right-edge.html
> 
> testing is too generic and is misleading (how can a test not be testing). hit-testing is what you want if you think you need a subdirectory. Alternatively you can just put it directly under fast/table.

Have uploaded another patch Julien, moving the testcases directly under fast/table. Perhaps I misunderstood your previous review comment regarding moving them to a new sub-directory (fast/table/testing). Apologize for the same.
Comment 34 Julien Chaffraix 2012-08-15 10:17:16 PDT
Comment on attachment 158215 [details]
Patch

Great, sorry for being unclear in my comment (I was thinking of fast/table/hit-testing or just fast/table).
Comment 35 WebKit Review Bot 2012-08-15 11:01:22 PDT
Comment on attachment 158215 [details]
Patch

Clearing flags on attachment: 158215

Committed r125684: <http://trac.webkit.org/changeset/125684>
Comment 36 WebKit Review Bot 2012-08-15 11:01:28 PDT
All reviewed patches have been landed.  Closing bug.
Comment 37 Arpita Bahuguna 2012-08-15 11:07:02 PDT
(In reply to comment #34)
> (From update of attachment 158215 [details])
> Great, sorry for being unclear in my comment (I was thinking of fast/table/hit-testing or just fast/table).

Thank-you for your time and the review Julien, Allan.