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

[bugzilla-daemon at webkit.org]

Julien Chaffraix <jchaffraix at webkit.org> has granted 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 134258: Updated patch
https://bugs.webkit.org/attachment.cgi?id=134258&action=review

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


Very close but the ChangeLog really doesn't cut it.

> Source/WebCore/ChangeLog:7
> +	   Maintain the scrolled position of overflowing element when the
scrollable RenderLayer is destroyed,
> +	   which can be used to restore the scroll position if the same element
is again added to the render tree.

Explaining where you store the offset would be neat (in the ElementRareData as
we don't expect this to happen a lot).

> Source/WebCore/ChangeLog:8
> +	   We are matching Firefox's behaviour.

This would definitely use more context here as this doesn't give much. For
example, what's the difference with the other browsers? Think of someone
watching the commit at home who doesn't know the gory details.

> Source/WebCore/ChangeLog:16
> +	   Resetting the saved scroll offset if node is moved to other location
in same document or other document.

We usually prefer ChangeLog to be written in good English. Here it feels weird
even to my foreigner's ear due to missing some words. A better writing would
be:

Reset the saved scroll offset if the node is moved to another location in the
same document or another one.

> Source/WebCore/ChangeLog:21
> +	   Helper functions for accessing layer scroll offset from rare data.

Add helper functions to access the layer scroll offset from the element's rare
data.

> Source/WebCore/ChangeLog:25
> +	   Maintain the scroll offset.

Add the scroll offset book-keeping.