Comment on attachment 83517[details]
Part 1: Make containing block width/height for positioned elements work with writing modes.
View in context: https://bugs.webkit.org/attachment.cgi?id=83517&action=review> Source/WebCore/ChangeLog:5
> + https://bugs.webkit.org/show_bug.cgi?id=46500, make positioned elements work with vertical text.
Do you plan to add test cases?
> Source/WebCore/rendering/RenderBox.cpp:2077
> + return containingBlockLogicalWidthForPositioned(containingBlock, false);
I hate booleans for this kind of thing! I usually ask people to use enums for something where you always are passing a constant to it.
Comment on attachment 83517[details]
Part 1: Make containing block width/height for positioned elements work with writing modes.
View in context: https://bugs.webkit.org/attachment.cgi?id=83517&action=review> Source/WebCore/rendering/RenderBox.cpp:2046
> + return containingBlockLogicalHeightForPositioned(containingBlock, false);
An enum rather than a bool would make this more readable.
> Source/WebCore/rendering/RenderBox.cpp:2093
> + IntRect boundingBox = toRenderInline(containingBlock)->linesBoundingBox();
Why not use 'flow' here?
> Source/WebCore/rendering/RenderBox.h:431
>
Shame to add new bool params.
I think the boolean is fine in this case, since the "public" callers don't use it, and its internal value is obvious given that the two functions are right there in the code next to each other.
http://trac.webkit.org/changeset/79467 might have broken Qt Linux Release
The following tests are not passing:
fast/parser/test-unicode-characters-in-attribute-name.html
Attachment 83550[details] did not pass style-queue:
Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCor..." exit_code: 1
Source/WebCore/rendering/RenderBox.cpp:2307: One space before end of line comments [whitespace/comments] [5]
Total errors found: 1 in 3 files
If any of these errors are false positives, please file a bug against check-webkit-style.
fast/table/fixed-with-auto-with-colspan-vertical.html also seems to be failing on Snow Leopard since these two patches are landed.
Should we just rebaseline them?
Comment on attachment 83715[details]
Part 7: Fixes for vertical-rl positioning and finally some tests!
> vertical-lr/001-expected.png
> vertical-rl/001-expected.png
If you removed the text, this would be cross-platform. Is the black necessary?
> vertical-lr/002-expected.png
> vertical-rl/002-expected.png
Avoid the scrollbar in pixel results.
The test cases are clones of their horizontal counterparts. For my own testing at least, it's valuable that they be identical to the horizontal versions (even if the horizontal versions are badly written) so that I can easily see if the rendering is the same (just rotated).
After r79618 these 3 tests started to fail in GTK bots
fast/table/colspanMinWidth-vertical.html
fast/table/fixed-with-auto-with-colspan-vertical.html
tables/mozilla_expected_failures/marvin/table_overflow_dirty_reflow_tbody.html
The last one fails inttermitently but the other two constantly.
Taking a look at some diffs it does not seem to be a matter of a rebaseline:
- layer at (0,0) size 385x400
- RenderBlock (positioned) {DIV} at (0,0) size 385x400
+ layer at (415,0) size 385x400
+ RenderBlock (positioned) {DIV} at (415,0) size 385x400
I'll skip these three for the moment
(In reply to comment #25)
> After r79618 these 3 tests started to fail in GTK bots
>
> fast/table/colspanMinWidth-vertical.html
> fast/table/fixed-with-auto-with-colspan-vertical.html
> tables/mozilla_expected_failures/marvin/table_overflow_dirty_reflow_tbody.html
>
> The last one fails inttermitently but the other two constantly.
>
> Taking a look at some diffs it does not seem to be a matter of a rebaseline:
>
> - layer at (0,0) size 385x400
> - RenderBlock (positioned) {DIV} at (0,0) size 385x400
> + layer at (415,0) size 385x400
> + RenderBlock (positioned) {DIV} at (415,0) size 385x400
>
> I'll skip these three for the moment
:m actually looks like the rebaseline would be correct http://trac.webkit.org/changeset/79640
Attachment 83829[details] did not pass style-queue:
Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCor..." exit_code: 1
Source/WebCore/rendering/RenderBox.cpp:2794: One space before end of line comments [whitespace/comments] [5]
Source/WebCore/rendering/RenderBox.cpp:2796: One space before end of line comments [whitespace/comments] [5]
Total errors found: 2 in 2 files
If any of these errors are false positives, please file a bug against check-webkit-style.
Attachment 83864[details] did not pass style-queue:
Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/fast..." exit_code: 1
Source/WebCore/rendering/RenderBox.cpp:2569: Boolean expressions that span multiple lines should have their operators on the left side of the line instead of the right side. [whitespace/operators] [4]
Total errors found: 1 in 9 files
If any of these errors are false positives, please file a bug against check-webkit-style.
http://trac.webkit.org/changeset/79725 might have broken GTK Linux 32-bit Release, GTK Linux 32-bit Debug, and Qt Linux Release
The following tests are not passing:
fast/table/fixed-with-auto-with-colspan-vertical.html
(In reply to comment #37)
> Created an attachment (id=83928) [details]
> rebaseline fast/table/fixed-with-auto-with-colspan-vertical.html
I think new result is better but I'm going to ask for a review since I'm not that familiar with CSS layout. Note that this failure is currently blocking the commit queue to land patches so it's crucial that we do this rebaseline or fix the layout.
(In reply to comment #39)
> (From update of attachment 83928[details])
> Sigh. The cq can't land this if its blocked.
Oops! :( I'm out of office already. Would you be able to land my patch on behalf of me?
(In reply to comment #37)
> Created an attachment (id=83928) [details]
> rebaseline fast/table/fixed-with-auto-with-colspan-vertical.html
Thanks for fixing it.
Comment on attachment 83928[details]
rebaseline fast/table/fixed-with-auto-with-colspan-vertical.html
Rejecting attachment 83928[details] from commit-queue.
Failed to run "['./Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '--bot-id=cr-jail-8', 'apply-..." exit_code: 2
Last 500 characters of output:
tTests/platform/mac/fast/table/fixed-with-auto-with-colspan-vertical-expected.txt.rej
patching file LayoutTests/platform/qt/fast/table/fixed-with-auto-with-colspan-vertical-expected.txt
Hunk #1 FAILED at 3.
Hunk #2 FAILED at 82.
2 out of 2 hunks FAILED -- saving rejects to file LayoutTests/platform/qt/fast/table/fixed-with-auto-with-colspan-vertical-expected.txt.rej
Failed to run "[u'/mnt/git/webkit-commit-queue/Tools/Scripts/svn-apply', u'--reviewer', u'Eric Seidel', u'--force']" exit_code: 1
Full output: http://queues.webkit.org/results/8019759
Attachment 84123[details] did not pass style-queue:
Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/fast..." exit_code: 1
Source/WebCore/rendering/RenderBlockLineLayout.cpp:1301: Should only a single space after a punctuation in a comment. [whitespace/comments] [5]
Source/WebCore/rendering/RenderBlockLineLayout.cpp:1305: Should only a single space after a punctuation in a comment. [whitespace/comments] [5]
Source/WebCore/rendering/RenderBlockLineLayout.cpp:1306: Should only a single space after a punctuation in a comment. [whitespace/comments] [5]
Total errors found: 3 in 54 files
If any of these errors are false positives, please file a bug against check-webkit-style.
Comment on attachment 84123[details]
Part 11: Make the static distance computations when left/right/top/bottom are auto writing mode aware.
View in context: https://bugs.webkit.org/attachment.cgi?id=84123&action=review
I am a little confused by the handling of static inline position for RTL containing blocks. Maybe it was already wrong, but it looks like it’s changing. Aren’t there tests covering this?
> Source/WebCore/rendering/RenderBox.cpp:2461
> + int staticTop = child->layer()->staticBlockPosition() - containerBlock->borderBefore();
May want to call this staticLogicalTop
> Source/WebCore/rendering/RenderFlexibleBox.cpp:420
> + child->layer()->setStaticBlockPosition(yPos);
Can use childLayer here too.
> Source/WebCore/rendering/RenderFlexibleBox.cpp:679
> - if (child->style()->hasStaticX()) {
> - if (style()->isLeftToRightDirection())
> - child->layer()->setStaticX(borderLeft()+paddingLeft());
> - else
> - child->layer()->setStaticX(borderRight()+paddingRight());
> - }
> - if (child->style()->hasStaticY()) {
> + if (child->style()->hasStaticInlinePosition(style()->isHorizontalWritingMode()))
> + child->layer()->setStaticInlinePosition(borderLeft() + paddingLeft());
Was the RTL code wrong?
> Source/WebCore/rendering/RenderFlexibleBox.cpp:683
> + child->layer()->setStaticBlockPosition(height());
Can use childLayer.
2011-02-23 11:48 PST, Dave Hyatt
2011-02-23 13:30 PST, Dave Hyatt
2011-02-23 13:58 PST, Dave Hyatt
2011-02-23 14:20 PST, Dave Hyatt
2011-02-23 14:44 PST, Dave Hyatt
2011-02-23 15:21 PST, Dave Hyatt
2011-02-24 13:55 PST, Dave Hyatt
2011-02-24 13:57 PST, Dave Hyatt
2011-02-25 10:07 PST, Dave Hyatt
2011-02-25 11:00 PST, Dave Hyatt
2011-02-25 11:01 PST, Dave Hyatt
2011-02-25 13:05 PST, Dave Hyatt
2011-02-25 22:21 PST, Ryosuke Niwa
2011-02-28 14:35 PST, Dave Hyatt