Bug 97024 - Remove isStartColumn in the border collapsing code
Summary: Remove isStartColumn in the border collapsing code
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Tables (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Julien Chaffraix
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2012-09-18 09:29 PDT by Julien Chaffraix
Modified: 2012-09-20 08:05 PDT (History)
6 users (show)

See Also:

Attachments
Proposed change: remove isEndColumn / isStartColumn and clean up the code. (7.03 KB, patch)
2012-09-18 10:02 PDT, Julien Chaffraix 		no flags Details | Formatted Diff | Diff
Change v2. Keep isEndColumn and explain why it's needed. (6.75 KB, patch)
2012-09-19 10:33 PDT, Julien Chaffraix 		no flags Details | Formatted Diff | Diff
Patch for landing. (6.89 KB, patch)
2012-09-19 19:00 PDT, Julien Chaffraix 		no flags Details | Formatted Diff | Diff
Show Obsolete (2) View All Add an attachment (proposed patch, testcase, etc.)

Note You need to log in before you can comment on or make changes to this bug.
Description Julien Chaffraix 2012-09-18 09:29:10 PDT 
Those 2 booleans are duplicating the information stored in prevCell / nextCell.

As we have to compute the previous / next cell in the common case, it's probably better to stick with them.

Patch forthcoming.
Comment 1 Julien Chaffraix 2012-09-18 10:02:50 PDT 
Created attachment 164580 [details]
Proposed change: remove isEndColumn / isStartColumn and clean up the code.
Comment 2 Julien Chaffraix 2012-09-18 15:57:08 PDT 
Comment on attachment 164580 [details]
Proposed change: remove isEndColumn / isStartColumn and clean up the code.

The cr-linux EWS is sick but I confirm that it regresses fast/repaint/table-collapsed-border.html. Holding the patch until I can investigate.
Comment 3 WebKit Review Bot 2012-09-18 21:06:44 PDT 
Comment on attachment 164580 [details]
Proposed change: remove isEndColumn / isStartColumn and clean up the code.

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

New failing tests:
fast/repaint/table-collapsed-border.html
Comment 4 Julien Chaffraix 2012-09-19 09:41:13 PDT 
> The cr-linux EWS is sick but I confirm that it regresses fast/repaint/table-collapsed-border.html. Holding the patch until I can investigate.

OK, one of the assumption I made is wrong: isEndColumn == !cellAfter is just untrue because the table doesn't have to be regular in structure.

<table>
<tr>
  <td></td>
  <td></td>
</tr>
<tr>
  <td></td>
</tr>
</table>

The previous table is valid but we would wrongly computes the second row's cell's end border.
Comment 5 Julien Chaffraix 2012-09-19 10:12:55 PDT 
We need isEndColumn for the previous reason but isStartColumn is fully embedded into cellBefore as we add the cells in order. Renaming the bug.
Comment 6 Julien Chaffraix 2012-09-19 10:33:46 PDT 
Created attachment 164754 [details]
Change v2. Keep isEndColumn and explain why it's needed.
Comment 7 Julien Chaffraix 2012-09-19 19:00:13 PDT 
Created attachment 164822 [details]
Patch for landing.
Comment 8 WebKit Review Bot 2012-09-20 08:05:08 PDT 
Comment on attachment 164822 [details]
Patch for landing.

Clearing flags on attachment: 164822

Committed r129136: <http://trac.webkit.org/changeset/129136>
Comment 9 WebKit Review Bot 2012-09-20 08:05:12 PDT 
All reviewed patches have been landed.  Closing bug.