[webkit-reviews] review granted: [Bug 83981] [Qt][WK2] Fixed elements position is wrong after zooming. : [Attachment 137261] Patch.

[bugzilla-daemon at webkit.org]

Kenneth Rohde Christiansen <kenneth at webkit.org> has granted Yael
<yael.aharon at nokia.com>'s request for review:
Bug 83981: [Qt][WK2] Fixed elements position is wrong after zooming.
https://bugs.webkit.org/show_bug.cgi?id=83981

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

------- Additional Comments from Kenneth Rohde Christiansen
<kenneth at webkit.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=137261&action=review


> ManualTests/remove-add-fixed-position.html:5
> +.d1{position:fixed;top:5;right:5;z-index:2;overflow:hidden;}
> +.o {background:green;height:40px;width:200px;}

Some spacing here would be nice :-) or even some newlines.

> Source/WebCore/ChangeLog:8
> +	   When setFixedVisibleContentRect is called, we mark all fixed
elements in the frame, for layout.

I don't get the commas here :-)

> Source/WebCore/ChangeLog:11
> +	   They are added and removed at the same time that they are added and
removed from their parent RenderBlock.
> +	   The idea is taken from the iOS5.1 branch, at opensource.apple.com.

So you didn't reuse code? Based on opensource code from the iOS port. ?

> Source/WebCore/page/FrameView.cpp:1704
>  {
> +    if (visibleContentRect.size() != this->fixedVisibleContentRect().size())
{

Maybe this deserves a little comment: 

// When the viewport size changes or the content is scaled, we need to
// reposition the fixed positioned elements.

as it is not so obvious that this is called when we scale the content

> Source/WebCore/rendering/RenderBlock.cpp:3423
> +    if (o->style()->position() == FixedPosition) {
> +	   if (view())

why not merge those two if's ? if (view() && o->style()->position() ==
FixedPosition)

> Source/WebCore/rendering/RenderBlock.cpp:3434
> +    if (view())
> +	   view()->removeFixedPositionedObject(o);

So this will fail in most cases right.

> Source/WebCore/rendering/RenderView.cpp:923
> +void RenderView::setFixedPositionedObjectsNeedLayout()

need*S* no?