[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