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

bugzilla-daemon at webkit.org bugzilla-daemon at webkit.org
Mon Dec 19 11:28:38 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 119833: Updated patch
https://bugs.webkit.org/attachment.cgi?id=119833&action=review

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


> Source/WebCore/dom/Element.cpp:950
> +    resetsavedLayerScrolledPosition();

This should just be:

    saveLayerScrolledPosition(IntPoint::zero());

To me, this is still a bit suspect. I don’t understand why removal from the
document is the key moment to reset this state. Does testing with other
browsers confirm they follow that rule?

> Source/WebCore/dom/Element.cpp:2043
> +    if (point == IntPoint::zero())
> +	   return;

This is incorrect for overwriting stored scroll position with zero. Instead it
should read:

    if (point == IntPoint::zero() && !hasRareData())

> Source/WebCore/dom/Element.cpp:2051
> +void Element::resetsavedLayerScrolledPosition()
> +{
> +    if (hasRareData())
> +	   rareData()->m_scrolledPosition = IntPoint::zero();
> +}

Please delete this poorly named and unneeded function.

> Source/WebCore/dom/Element.h:369
> +    IntPoint savedLayerScrolledPosition() const;
> +    void saveLayerScrolledPosition(const IntPoint&);

I don’t think “scrolled position” is as good as “scroll position”. I think a
more traditional “set” naming here would be slightly better:

    IntPoint savedLayerScrollPosition() const;
    void setSavedLayerScrollPosition(const IntPoint&);

Another option is to do something closer to what RenderLayer actually needs:

    IntSize savedLayerScrollOffset() const;
    void setSavedLayerScrollOffset(const IntSize&);

Either would be OK.

> Source/WebCore/dom/Element.h:370
> +    void resetsavedLayerScrolledPosition();

The s in “saved” should be capitalized, but it would be better to just delete
this function.

> Source/WebCore/dom/ElementRareData.h:55
> +    IntPoint m_scrolledPosition;

This should be named:

    IntPoint m_savedLayerScrollPosition;

Or:

    IntSize m_savedLayerScrollOffset;

> Source/WebCore/rendering/RenderLayer.cpp:200
> +	   // We save and restore only the scrollOffset as the other scroll
> +	   // values are recalulated.

Would read better on a single line.

> Source/WebCore/rendering/RenderLayer.cpp:1396
> -    
> +

Remove this unneeded whitespace change. For one thing, it will cause a merge
conflict that otherwise would not occur.


More information about the webkit-reviews mailing list