Bug 100399

Created attachment 182805 [details] Patch For informal review

Created attachment 182812 [details] Patch For informal review, added FIXME bugs, added a test case for accelerated rendering path

Comment on attachment 182812 [details] Patch I think this could have the shiny happy r=?

(In reply to comment #1) > Created an attachment (id=182805) [details] > Patch > > For informal review I'm no layout expert so it's difficult to judge the correctness of these changes. If the ChangeLog provided an overview of what you're doing, that would help. The FIXME additions don't match the prevalent convention: "FIXME: <explanation>". You don't seem to have included the colon. The changes for RenderBlock::flipFloatForWritingModeForChild() seem to assume that the caller is going to paint the child, rather than do some computation based on its boundary. I assume that's always the case now, but could this become a landmine? I thought xPositionForFloatIncludingMarginForPaint() was a little bit difficult to read because of the #ifdefing. It might be worth collecting the CSS_EXCLUSIONS code in one place: LayoutUnit xPositionForFloatIncludingMarginForPaint(const FloatingObject* child) const { #if ENABLE(CSS_EXCLUSIONS) ExclusionShapeOutsideInfo *shapeOutside = child->renderer()->exclusionShapeOutsideInfo(); if (shapeOutside) return child->x() - (isHorizontalWritingMode() ? shapeOutside->shapeLogicalLeft() : shapeOutside->shapeLogicalTop()); #endif if (isHorizontalWritingMode()) return child->x() + child->renderer()->marginLeft(); return child->x() + marginBeforeForChild(child->renderer()); } Likewise for LayoutUnit yPositionForFloatIncludingMarginForPaint() Shouldn't the test cover vertical-lr writing-mode?

(In reply to comment #4) > (In reply to comment #1) > > Created an attachment (id=182805) [details] [details] > > Patch > > > > For informal review > > I'm no layout expert so it's difficult to judge the correctness of these changes. If the ChangeLog provided an overview of what you're doing, that would help. Ok, I'll add an overview. > > The FIXME additions don't match the prevalent convention: "FIXME: <explanation>". You don't seem to have included the colon. Yeah, I did FIXBE Bug 33333: instead of FIXME: Bug 33333. I wish the check style script would catch that. > > The changes for RenderBlock::flipFloatForWritingModeForChild() seem to assume that the caller is going to paint the child, rather than do some computation based on its boundary. I assume that's always the case now, but could this become a landmine? You're entirely right. I've fixed this by refactoring the code so that there's a bool that changes the behavior of xPositionForFloatIncludingMargin and yPositionForFloatIncludingMargin, since otherwise I think we'd be diving way too deeply into code duplication land. > > I thought xPositionForFloatIncludingMarginForPaint() was a little bit difficult to read because of the #ifdefing. It might be worth collecting the CSS_EXCLUSIONS code in one place: > > LayoutUnit xPositionForFloatIncludingMarginForPaint(const FloatingObject* child) const > { > #if ENABLE(CSS_EXCLUSIONS) > ExclusionShapeOutsideInfo *shapeOutside = child->renderer()->exclusionShapeOutsideInfo(); > if (shapeOutside) > return child->x() - (isHorizontalWritingMode() ? shapeOutside->shapeLogicalLeft() : shapeOutside->shapeLogicalTop()); > #endif > if (isHorizontalWritingMode()) > return child->x() + child->renderer()->marginLeft(); > > return child->x() + marginBeforeForChild(child->renderer()); > } > > Likewise for LayoutUnit yPositionForFloatIncludingMarginForPaint() I agree, that is much better. I've removed those methods, but I kept this refactoring. > > Shouldn't the test cover vertical-lr writing-mode? Sure, I wasn't sure that actually added anything, but I've added the tests.

Created attachment 183571 [details] Patch Updated to respect feedback

Attachment 183571 [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/ChangeLog:8: Line contains tab character. [whitespace/tab] [5] Source/WebCore/ChangeLog:9: Line contains tab character. [whitespace/tab] [5] Source/WebCore/ChangeLog:10: Line contains tab character. [whitespace/tab] [5] Source/WebCore/ChangeLog:11: Line contains tab character. [whitespace/tab] [5] Source/WebCore/ChangeLog:17: Line contains tab character. [whitespace/tab] [5] Source/WebCore/ChangeLog:18: Line contains tab character. [whitespace/tab] [5] Source/WebCore/ChangeLog:31: Line contains tab character. [whitespace/tab] [5] Total errors found: 7 in 7 files If any of these errors are false positives, please file a bug against check-webkit-style.

Created attachment 183572 [details] Patch Updated to get rid of annoying and stupid tabs. Must fix vim's braindamage that thinks that ChangeLogs should have tabs.

Comment on attachment 183572 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=183572&action=review r- > Source/WebCore/rendering/RenderBlock.cpp:3115 > -LayoutPoint RenderBlock::flipFloatForWritingModeForChild(const FloatingObject* child, const LayoutPoint& point) const > +LayoutPoint RenderBlock::flipFloatForWritingModeForChild(const FloatingObject* child, const LayoutPoint& point, bool paintTime) const The "ForChild" here is actually meant to imply the 2 * multiplier. It is analogous to RenderBox flipForWritingModeForChild. I don't think we want to introduce an inscrutable boolean here whose meaning cannot be discerned when looking at call sites. If you want to be consistent with the RenderBox methods, I would just make a second method called flipFloatForWritingMode that doesn't do the double subtraction. I'd also be fine with an enum instead of a boolean so that the purpose of the value at call sites is clear. Either approach works for me.

Created attachment 183814 [details] Patch Update for review feedback

(In reply to comment #9) > (From update of attachment 183572 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=183572&action=review > > r- > > > Source/WebCore/rendering/RenderBlock.cpp:3115 > > -LayoutPoint RenderBlock::flipFloatForWritingModeForChild(const FloatingObject* child, const LayoutPoint& point) const > > +LayoutPoint RenderBlock::flipFloatForWritingModeForChild(const FloatingObject* child, const LayoutPoint& point, bool paintTime) const > > The "ForChild" here is actually meant to imply the 2 * multiplier. It is analogous to RenderBox flipForWritingModeForChild. I don't think we want to introduce an inscrutable boolean here whose meaning cannot be discerned when looking at call sites. > > If you want to be consistent with the RenderBox methods, I would just make a second method called flipFloatForWritingMode that doesn't do the double subtraction. > > I'd also be fine with an enum instead of a boolean so that the purpose of the value at call sites is clear. Either approach works for me. I decided to opt for the flag because I think I will need it in other locations in the future, since even if I did create a flipFloatForWritingMode method, it needs to use a different box for the child depending on if it's looking at the shape (layout) or the float (paint).

Comment on attachment 183814 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=183814&action=review r- > Source/WebCore/rendering/RenderBlock.h:696 > + enum FloatRenderingState { FloatLayout, FloatPaint }; I wouldn't describe this as layout vs. painting. I'd just be more explicit about the math, since really it's kind of a hack that painting does this double subtraction at all. We just need to re-architect paint at some point to get rid of it. I'd just make the enum something like IncludeChildCoordinates vs. DoNotIncludeChildCoordinates. :)

Comment on attachment 183814 [details] Patch r=me

Comment on attachment 183814 [details] Patch Clearing flags on attachment: 183814 Committed r140365: <http://trac.webkit.org/changeset/140365>

All reviewed patches have been landed. Closing bug.