Bug 149366

Summary: Subpixel layout: Convert RenderTable* and AutoTableLayout to use LayoutUnit.
Product: WebKit Reporter: zalan <zalan>
Component: Layout and RenderingAssignee: zalan <zalan>
Status: RESOLVED FIXED    
Severity: Normal CC: buildbot, commit-queue, esprehn+autocc, glenn, john.bridges, kondapallykalyan, rniwa, simon.fraser, webkit
Priority: P2    
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on: 150616    
Bug Blocks: 149935    
Attachments:
Description Flags
Patch
none
Archive of layout-test-results from ews102 for mac-mavericks
none
Archive of layout-test-results from ews104 for mac-mavericks-wk2
none
Patch
none
Patch
buildbot: commit-queue-
Archive of layout-test-results from ews103 for mac-mavericks
none
Archive of layout-test-results from ews106 for mac-mavericks-wk2
none
Patch
none
Patch for EWS
buildbot: commit-queue-
Archive of layout-test-results from ews112 for mac-yosemite
none
Archive of layout-test-results from ews101 for mac-mavericks
none
Archive of layout-test-results from ews104 for mac-mavericks-wk2
none
Patch for EWS
none
Patch for EWS
none
Patch (code change only) for reviewing
hyatt: review+, buildbot: commit-queue-
Archive of layout-test-results from ews117 for mac-yosemite
none
Archive of layout-test-results from ews102 for mac-mavericks
none
Archive of layout-test-results from ews104 for mac-mavericks-wk2 none

Description zalan 2015-09-18 20:30:16 PDT
ssia.
Comment 1 zalan 2015-09-18 20:37:35 PDT
Created attachment 261565 [details]
Patch
Comment 2 zalan 2015-09-18 20:37:55 PDT
Comment on attachment 261565 [details]
Patch

EWS testing.
Comment 3 Build Bot 2015-09-18 21:06:47 PDT
Comment on attachment 261565 [details]
Patch

Attachment 261565 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.webkit.org/results/185310

Number of test failures exceeded the failure limit.
Comment 4 Build Bot 2015-09-18 21:06:51 PDT
Created attachment 261568 [details]
Archive of layout-test-results from ews102 for mac-mavericks

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews102  Port: mac-mavericks  Platform: Mac OS X 10.9.5
Comment 5 Build Bot 2015-09-18 21:09:30 PDT
Comment on attachment 261565 [details]
Patch

Attachment 261565 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.webkit.org/results/185311

Number of test failures exceeded the failure limit.
Comment 6 Build Bot 2015-09-18 21:09:34 PDT
Created attachment 261569 [details]
Archive of layout-test-results from ews104 for mac-mavericks-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews104  Port: mac-mavericks-wk2  Platform: Mac OS X 10.9.5
Comment 7 Darin Adler 2015-09-19 15:49:59 PDT
Comment on attachment 261565 [details]
Patch

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

> Source/WebCore/rendering/AutoTableLayout.cpp:246
> +        maxWidth = std::max(maxWidth, std::min<LayoutUnit>(maxNonPercent, tableMaxWidth));
> +        maxWidth = std::max(maxWidth, std::min<LayoutUnit>(maxPercent, tableMaxWidth));

I don’t understand why explicit <LayoutUnit> here is needed or why it’s safe.

> Source/WebCore/rendering/AutoTableLayout.cpp:256
> +        minWidth = maxWidth = std::max<LayoutUnit>(minWidth, tableLogicalWidth.value());

I don’t understand why explicit <LayoutUnit> here is needed or why it’s safe.

> Source/WebCore/rendering/AutoTableLayout.cpp:349
> +                maxLogicalWidth = std::max<LayoutUnit>(maxLogicalWidth, std::max(spanMaxLogicalWidth, cellMaxLogicalWidth) * 100  / cellLogicalWidth.percent());

I don’t understand why explicit <LayoutUnit> here is needed or why it’s safe.

Noticed two spaces between "100" and "/".

> Source/WebCore/rendering/AutoTableLayout.cpp:377
> +                    LayoutUnit cellLogicalWidth = std::max<LayoutUnit>(m_layoutStruct[pos].effectiveMinLogicalWidth, cellMinLogicalWidth * m_layoutStruct[pos].logicalWidth.value() / fixedWidth);

I don’t understand why explicit <LayoutUnit> here is needed or why it’s safe.
Comment 8 zalan 2015-09-26 18:09:52 PDT
Created attachment 261981 [details]
Patch
Comment 9 zalan 2015-09-26 18:10:20 PDT
EWS testing.
Comment 10 zalan 2015-09-26 18:46:18 PDT
Created attachment 261982 [details]
Patch
Comment 11 Build Bot 2015-09-26 19:29:53 PDT
Comment on attachment 261982 [details]
Patch

Attachment 261982 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.webkit.org/results/215728

Number of test failures exceeded the failure limit.
Comment 12 Build Bot 2015-09-26 19:29:57 PDT
Created attachment 261983 [details]
Archive of layout-test-results from ews103 for mac-mavericks

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews103  Port: mac-mavericks  Platform: Mac OS X 10.9.5
Comment 13 Build Bot 2015-09-26 19:34:50 PDT
Comment on attachment 261982 [details]
Patch

Attachment 261982 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.webkit.org/results/215732

Number of test failures exceeded the failure limit.
Comment 14 Build Bot 2015-09-26 19:34:53 PDT
Created attachment 261984 [details]
Archive of layout-test-results from ews106 for mac-mavericks-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews106  Port: mac-mavericks-wk2  Platform: Mac OS X 10.9.5
Comment 15 zalan 2015-10-19 15:47:52 PDT
Created attachment 263521 [details]
Patch
Comment 16 zalan 2015-10-20 10:49:10 PDT
Created attachment 263591 [details]
Patch for EWS
Comment 17 WebKit Commit Bot 2015-10-20 11:50:42 PDT
Attachment 263591 [details] did not pass style-queue:

Traceback (most recent call last):
  File "/Volumes/Data/StyleQueue/WebKit/Tools/Scripts/webkit-patch", line 84, in <module>
    main()
  File "/Volumes/Data/StyleQueue/WebKit/Tools/Scripts/webkit-patch", line 79, in main
    WebKitPatch(os.path.abspath(__file__)).main()
  File "/Volumes/Data/StyleQueue/WebKit/Tools/Scripts/webkitpy/tool/multicommandtool.py", line 305, in main
    result = command.check_arguments_and_execute(options, args, self)
  File "/Volumes/Data/StyleQueue/WebKit/Tools/Scripts/webkitpy/tool/multicommandtool.py", line 123, in check_arguments_and_execute
    return self.execute(options, args, tool) or 0
  File "/Volumes/Data/StyleQueue/WebKit/Tools/Scripts/webkitpy/tool/commands/abstractsequencedcommand.py", line 54, in execute
    self._sequence.run_and_handle_errors(tool, options, state)
  File "/Volumes/Data/StyleQueue/WebKit/Tools/Scripts/webkitpy/tool/commands/stepsequence.py", line 73, in run_and_handle_errors
    self._run(tool, options, state)
  File "/Volumes/Data/StyleQueue/WebKit/Tools/Scripts/webkitpy/tool/commands/stepsequence.py", line 67, in _run
    step(tool, options).run(state)
  File "/Volumes/Data/StyleQueue/WebKit/Tools/Scripts/webkitpy/tool/steps/checkstyle.py", line 65, in run
    self._tool.executive.run_and_throw_if_fail(self._tool.deprecated_port().check_webkit_style_command() + args, cwd=self._tool.scm().checkout_root)
  File "/Volumes/Data/StyleQueue/WebKit/Tools/Scripts/webkitpy/common/system/executive.py", line 131, in run_and_throw_if_fail
    exit_code = self._run_command_with_teed_output(args, child_stdout, **kwargs)
  File "/Volumes/Data/StyleQueue/WebKit/Tools/Scripts/webkitpy/common/system/executive.py", line 103, in _run_command_with_teed_output
    **kwargs)
  File "/Volumes/Data/StyleQueue/WebKit/Tools/Scripts/webkitpy/common/system/executive.py", line 506, in popen
    return subprocess.Popen(string_args, **kwargs)
  File "/System/Library/Frameworks/Python.framework/Versions/2.7/lib/python2.7/subprocess.py", line 711, in __init__
    errread, errwrite)
  File "/System/Library/Frameworks/Python.framework/Versions/2.7/lib/python2.7/subprocess.py", line 1308, in _execute_child
    raise child_exception
OSError: [Errno 7] Argument list too long


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 18 Build Bot 2015-10-20 13:21:10 PDT
Comment on attachment 263591 [details]
Patch for EWS

Attachment 263591 [details] did not pass mac-debug-ews (mac):
Output: http://webkit-queues.webkit.org/results/312727

New failing tests:
fast/repaint/table-hover-on-link.html
tables/mozilla_expected_failures/bugs/bug89315.html
tables/mozilla_expected_failures/bugs/bug8499.html
Comment 19 Build Bot 2015-10-20 13:21:18 PDT
Created attachment 263608 [details]
Archive of layout-test-results from ews112 for mac-yosemite

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews112  Port: mac-yosemite  Platform: Mac OS X 10.10.5
Comment 20 Build Bot 2015-10-20 13:25:47 PDT
Created attachment 263609 [details]
Archive of layout-test-results from ews101 for mac-mavericks

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews101  Port: mac-mavericks  Platform: Mac OS X 10.9.5
Comment 21 Build Bot 2015-10-20 13:30:13 PDT
Created attachment 263610 [details]
Archive of layout-test-results from ews104 for mac-mavericks-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews104  Port: mac-mavericks-wk2  Platform: Mac OS X 10.9.5
Comment 22 zalan 2015-10-20 15:57:12 PDT
Created attachment 263627 [details]
Patch for EWS
Comment 23 zalan 2015-10-20 19:32:22 PDT
Created attachment 263651 [details]
Patch for EWS
Comment 24 WebKit Commit Bot 2015-10-20 19:39:26 PDT
Attachment 263651 [details] did not pass style-queue:

Traceback (most recent call last):
  File "/Volumes/Data/StyleQueue/WebKit/Tools/Scripts/webkit-patch", line 84, in <module>
    main()
  File "/Volumes/Data/StyleQueue/WebKit/Tools/Scripts/webkit-patch", line 79, in main
    WebKitPatch(os.path.abspath(__file__)).main()
  File "/Volumes/Data/StyleQueue/WebKit/Tools/Scripts/webkitpy/tool/multicommandtool.py", line 305, in main
    result = command.check_arguments_and_execute(options, args, self)
  File "/Volumes/Data/StyleQueue/WebKit/Tools/Scripts/webkitpy/tool/multicommandtool.py", line 123, in check_arguments_and_execute
    return self.execute(options, args, tool) or 0
  File "/Volumes/Data/StyleQueue/WebKit/Tools/Scripts/webkitpy/tool/commands/abstractsequencedcommand.py", line 54, in execute
    self._sequence.run_and_handle_errors(tool, options, state)
  File "/Volumes/Data/StyleQueue/WebKit/Tools/Scripts/webkitpy/tool/commands/stepsequence.py", line 73, in run_and_handle_errors
    self._run(tool, options, state)
  File "/Volumes/Data/StyleQueue/WebKit/Tools/Scripts/webkitpy/tool/commands/stepsequence.py", line 67, in _run
    step(tool, options).run(state)
  File "/Volumes/Data/StyleQueue/WebKit/Tools/Scripts/webkitpy/tool/steps/checkstyle.py", line 65, in run
    self._tool.executive.run_and_throw_if_fail(self._tool.deprecated_port().check_webkit_style_command() + args, cwd=self._tool.scm().checkout_root)
  File "/Volumes/Data/StyleQueue/WebKit/Tools/Scripts/webkitpy/common/system/executive.py", line 131, in run_and_throw_if_fail
    exit_code = self._run_command_with_teed_output(args, child_stdout, **kwargs)
  File "/Volumes/Data/StyleQueue/WebKit/Tools/Scripts/webkitpy/common/system/executive.py", line 103, in _run_command_with_teed_output
    **kwargs)
  File "/Volumes/Data/StyleQueue/WebKit/Tools/Scripts/webkitpy/common/system/executive.py", line 506, in popen
    return subprocess.Popen(string_args, **kwargs)
  File "/System/Library/Frameworks/Python.framework/Versions/2.7/lib/python2.7/subprocess.py", line 711, in __init__
    errread, errwrite)
  File "/System/Library/Frameworks/Python.framework/Versions/2.7/lib/python2.7/subprocess.py", line 1308, in _execute_child
    raise child_exception
OSError: [Errno 7] Argument list too long


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 25 zalan 2015-10-21 09:15:51 PDT
Created attachment 263683 [details]
Patch (code change only) for reviewing
Comment 26 Build Bot 2015-10-21 09:55:27 PDT
Comment on attachment 263683 [details]
Patch (code change only) for reviewing

Attachment 263683 [details] did not pass mac-debug-ews (mac):
Output: http://webkit-queues.webkit.org/results/316512

Number of test failures exceeded the failure limit.
Comment 27 Build Bot 2015-10-21 09:55:31 PDT
Created attachment 263686 [details]
Archive of layout-test-results from ews117 for mac-yosemite

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews117  Port: mac-yosemite  Platform: Mac OS X 10.10.5
Comment 28 Build Bot 2015-10-21 09:56:07 PDT
Comment on attachment 263683 [details]
Patch (code change only) for reviewing

Attachment 263683 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.webkit.org/results/316519

Number of test failures exceeded the failure limit.
Comment 29 Build Bot 2015-10-21 09:56:10 PDT
Created attachment 263687 [details]
Archive of layout-test-results from ews102 for mac-mavericks

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews102  Port: mac-mavericks  Platform: Mac OS X 10.9.5
Comment 30 Build Bot 2015-10-21 09:58:04 PDT
Comment on attachment 263683 [details]
Patch (code change only) for reviewing

Attachment 263683 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.webkit.org/results/316518

Number of test failures exceeded the failure limit.
Comment 31 Build Bot 2015-10-21 09:58:08 PDT
Created attachment 263688 [details]
Archive of layout-test-results from ews104 for mac-mavericks-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews104  Port: mac-mavericks-wk2  Platform: Mac OS X 10.9.5
Comment 32 Dave Hyatt 2015-10-21 11:02:19 PDT
Comment on attachment 263683 [details]
Patch (code change only) for reviewing

r=me
Comment 33 zalan 2015-10-22 10:49:02 PDT
I'll land this patch soon after I have the Win results.
Comment 34 zalan 2015-10-28 10:48:47 PDT
*** Bug 149332 has been marked as a duplicate of this bug. ***
Comment 36 John Bridges 2016-09-28 17:31:10 PDT
Still appears to be rounding the start X/Y of each cell to integer.
View this in current iOS Safari, pinch/zoom and you will see hairlines appear between rows. Same with other WebKit browsers/Email clients.
If if were only off by 1/64th of a pixel, the lines would be far more faint.

<!DOCTYPE html>
<html><body><table width="120" cellspacing="0" cellpadding="0" height="99"><tbody>
<tr><td bgcolor="#005486"></td></tr>
<tr><td bgcolor="#005486"></td></tr>
<tr><td bgcolor="#005486"></td></tr>
<tr><td bgcolor="#005486"></td></tr>
<tr><td bgcolor="#005486"></td></tr>
<tr><td bgcolor="#005486"></td></tr>
<tr><td bgcolor="#005486"></td></tr>
<tr><td bgcolor="#005486"></td></tr>
<tr><td bgcolor="#005486"></td></tr>
</tbody></table></body></html>