[webkit-reviews] review denied: [Bug 72852] Scroll position is lost after hide/show element : [Attachment 118338] 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 118338: Updated patch
https://bugs.webkit.org/attachment.cgi?id=118338&action=review

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


You *really* need more testing here. Each review brings up some bugs in the
implementation that are not caught by any test. It would be neat to come up
with some new test case covering the pointed bug but at least *more* testing
would give reviewers some confidence that this works outside a very basic
example (which is your current test case). Here is some ideas:

* toggling several times visibility should work. Changing the value in between
should also work. Make sure m_scrollOffset.x() != m_scrollOffset.y() as this
would fine flipping issues.
* look at the different code path and come with a test case (example: the
Node::setDocument change is not tested AFAICT, RenderLayer::styleChanged too).
* use some non default writing modes and direction in a test.
* vary your scroll origin (see RenderLayer::computeScrollDimensions to see what
impacts it).

> Source/WebCore/ChangeLog:8
> +	   Maintain the scroll position of overflow element when the scrollable
renderlayer is being dropped,

overflowing element?

s/renderlayer/RenderLayer/

dropped -> destroyed?

> Source/WebCore/ChangeLog:9
> +	   which can be used to restore the scroll position if same node is
again added to render tree

node -> element

added to *the* render tree

> Source/WebCore/ChangeLog:19
> +	   Helper functions for accessing scroll position from rare data.

It would help readability if you added an empty line between the different
groups covered by a single comment.

> Source/WebCore/dom/Element.cpp:2040
> +    return hasRareData() ? rareData()->scrolledPosition() : IntPoint();

Looking at the rest of the code, it is not fine to call scrolledPosition if we
don't have some rareData. If that's the case, then this should just be:

ASSERT(hasRareData());
return rareData()->scrolledPosition();

> Source/WebCore/dom/Element.h:370
> +    void setScrollPosition(const IntPoint& = IntPoint());

Please remove the default argument, it is confusing and a source of badness in
this case.

> Source/WebCore/dom/ElementRareData.h:56
> +    inline bool scrollableElement() { return (m_scrolledPosition !=
IntPoint()); }

You can use IntPoint::zero() for the comparison here.

> Source/WebCore/dom/ElementRareData.h:58
> +    inline void setScrollPosition(const IntPoint& point) {
m_scrolledPosition = point; } 

You don't have to use the |inline| keyword in the previous functions. Any
function defined in a header is automatically inlined by the compiler.

> Source/WebCore/rendering/RenderLayer.cpp:201
> +	       m_scrollOffset = toSize(element->scrolledPosition());

I think this will break if scrollOrigin() is different from (0, 0) (and any
non-English combinaison of writing mode and directions). You are saving
scrollPosition() into your rare data field which included the scrollOrigin()
*and* m_scrollOffset. However you restore everything into m_scrollOffset. This
means that now scrollPosition will include scrollOrigin() twice.