[webkit-reviews] review granted: [Bug 68590] Floats pushed to next page don't reposition properly. : [Attachment 108270] Patch

[bugzilla-daemon at webkit.org]

Adam Roben (:aroben) <aroben at apple.com> has granted Dave Hyatt
<hyatt at apple.com>'s request for review:
Bug 68590: Floats pushed to next page don't reposition properly.
https://bugs.webkit.org/show_bug.cgi?id=68590

Attachment 108270: Patch
https://bugs.webkit.org/attachment.cgi?id=108270&action=review

------- Additional Comments from Adam Roben (:aroben) <aroben at apple.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=108270&action=review


> Source/WebCore/rendering/RenderBlock.cpp:3272
> +LayoutPoint RenderBlock::computeLogicalLocationForFloat(FloatingObject*
floatingObject, LayoutUnit floatLogicalTop, LayoutUnit leftOffset, LayoutUnit
rightOffset, LayoutUnit floatLogicalWidth)

Can this be a const member function? Can it take a const FloatingObject*?

Is "location" specific enough in this function name? Should it say "top left"
instead?

It's confusing that you pass a value named floatLogicalTop into a function that
is supposedly calculating that value for you. Is there a clearer way we can
name the parameter that makes it clear what the caller has to do when
calculating that value vs. what the function will do based on that value?

> LayoutTests/ChangeLog:11
> +	   *
fast/regions/webkit-flow-floats-inside-regions-bounds-expected.txt:

Don't you need to update pixel results, too?