[webkit-reviews] review denied: [Bug 72852] Scroll position is lost after hide/show element : [Attachment 118183] Updated patch

[bugzilla-daemon at webkit.org]

Julien Chaffraix <jchaffraix at webkit.org> has denied Rakesh
<rakesh.kn at motorola.com>'s request for review:
Bug 72852: Scroll position is lost after hide/show element
https://bugs.webkit.org/show_bug.cgi?id=72852

Attachment 118183: Updated patch
https://bugs.webkit.org/attachment.cgi?id=118183&action=review

------- Additional Comments from Julien Chaffraix <jchaffraix at webkit.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=118183&action=review


You really need more testing for us to see that you won't break scrolling after
resetting your visibility.

> Source/WebCore/dom/Element.cpp:2035
> +    return hasRareData() ? rareData()->scrollableElement() : false;

It should be:

return hasRareData() && rareData()->scrollableElement();

> Source/WebCore/dom/Element.cpp:2045
> +    ElementRareData* data = hasRareData() ? rareData() : ensureRareData();

No need for checking hasRareData here. Just do:

ensureRareData()->setScrollPosition(point);

> Source/WebCore/dom/Element.cpp:2052
> +	   rareData()->setScrollPosition(IntPoint());

The whole logic is flawed. If you set up scrolledPosition() once, you will
always reset the scrollOffset in updateScrollInfoAfterLayout to something
(IntPoint() if you have called reset in the meantime) thus overriding a scroll
offset set by JS or the user. This is not covered by your tests unfortunately.

> Source/WebCore/dom/Node.cpp:461
> +	   toElement(this)->resetScrollPosition();

There is nothing that warranties that |this| is an Element AFAICT. This will
crash pretty badly if you move a node from one Document to another one.

> Source/WebCore/rendering/RenderLayer.cpp:2275
> +    }

It really looks like an overkill to check for that at every layout. I would
feel better if we restored it in the constructor of RenderLayer as to reset the
scrollOffset once.

> Source/WebCore/rendering/RenderLayer.cpp:4297
> +	   toElement(m_renderer->node())->resetScrollPosition();

Again what guarantees our node to be an Element?

> LayoutTests/fast/overflow/scroll-div-hide-show-expected.txt:19
> +FAIL e.scrollTop should be 100. Was 0.

This is failing, you need to mention why and we *need* a bug tracking this
failure at some point as it will go unnoticed in our tree.