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

[bugzilla-daemon at webkit.org]

MORITA Hajime <morrita at google.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 117883: Updated patch
https://bugs.webkit.org/attachment.cgi?id=117883&action=review

------- Additional Comments from MORITA Hajime <morrita at google.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=117883&action=review


> Source/WebCore/dom/Element.cpp:2035
> +    return ensureRareData()->scrollableElement();

Please be conservative about instantiating ElementRareData object. you can
check its existence using hasRareData().

> Source/WebCore/dom/Element.cpp:2040
> +    return ensureRareData()->scrolledPosition();

Same as above.

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

Same as above.

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

The method should be const

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

The parameter should be a const reference.

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

Please follow the naming convention.
Having this in ElementRareData seems good idea for me.

> Source/WebCore/dom/ElementRareData.h:57
> +    inline IntPoint scrolledPosition() { return scrollPosition; }

Should return const reference. the method should be const too.

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

the paramter should be const.

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

You can do this check inside resetScrollPosition().

> Source/WebCore/rendering/RenderLayer.cpp:1382
> +	       element->resetScrollPosition();

This reset should happen outside the if (box) clause.

> LayoutTests/ChangeLog:10
> +

Could you add another test case that verify the style recalculation invalidate
the scroll postion cache?
(That is for covering the change on RenderLayer::styleChanged())