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

bugzilla-daemon at webkit.org bugzilla-daemon at webkit.org
Wed Dec 14 16:53:27 PST 2011


Darin Adler <darin at apple.com> 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 119240: Updated patch
https://bugs.webkit.org/attachment.cgi?id=119240&action=review

------- Additional Comments from Darin Adler <darin at apple.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=119240&action=review


> Source/WebCore/dom/Element.cpp:2045
> +    return hasRareData() && rareData()->m_scrolledPosition !=
IntPoint::zero();

This does not answer the question of whether an element is scrollable. It
answers the question of whether is is scrolled, which is not the same thing.

> Source/WebCore/dom/Element.cpp:2052
> +IntPoint Element::scrolledPosition() const
> +{
> +    ASSERT(scrollableElement());
> +    return rareData()->m_scrolledPosition;
> +}

This seems wrong. Should just return IntPoint::zero() if there is no rare data.


> Source/WebCore/dom/Element.cpp:2056
> +    ensureRareData()->m_scrolledPosition = point;

This should not create new rare data if called with a zero point on an object
that doesn’t already have rare data.

> Source/WebCore/dom/Element.cpp:2062
> +    if (scrollableElement())
> +	   rareData()->m_scrolledPosition = IntPoint();

This should just check hasRareData(). No reason to check the position before
resetting it to zero.

In fact, I think it should just call setScrollPosition(IntPoint::zero()) rather
than having its own logic.

> Source/WebCore/dom/Element.h:367
> +    bool scrollableElement() const;

A function named “scrollable element” would return an element, not an answer to
the question “is this scrollable”. I suggest just dropping this function.

> Source/WebCore/dom/Element.h:368
> +    IntPoint scrolledPosition() const;

It doesn’t make sense to call the getter “scrolled position” and then setter
“scroll position”. Please choose one term and stick to it.

I think it’s misleading to store something called the “scroll position” that is
only accurate when an element has no renderer. Any code that got at this to try
to ask what the current scroll position of an element was would get a bad
result. If the use of the data is so specific, then I suggest we make the name
specific as well. Maybe “saved layer scroll position” is a good name.

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

This is too low level for code like this and there is no need to put
Element-specific code into Node here. There is already a virtual function
called willMoveToNewOwnerDocument, and if this is needed, this code can go
there.

But I don’t understand why moving to a new document is a time to do this. Is
that really right? Why would adoptNode be the one thing that would cause us to
reset the scroll position when moving a node from one part of the document to
another would not?

> Source/WebCore/rendering/RenderLayer.cpp:200
> +	   if (element->scrollableElement()) {

This if is unnecessary. It should be save to save off a zero scroll offset and
then reset the scroll offset to zero.


More information about the webkit-reviews mailing list