[webkit-reviews] review denied: [Bug 38213] Page up / page down moves edited content for the entire height : [Attachment 54491] Fix for scroll height calc for page up/down key handling in Editor

[bugzilla-daemon at webkit.org]

David Levin <levin at chromium.org> has denied Zelidrag Hornung
<zelidrag at chromium.org>'s request for review:
Bug 38213: Page up / page down moves edited content for the entire height
https://bugs.webkit.org/show_bug.cgi?id=38213

Attachment 54491: Fix for scroll height calc for page up/down key handling in
Editor
https://bugs.webkit.org/attachment.cgi?id=54491&action=review

------- Additional Comments from David Levin <levin at chromium.org>
This looks like it is heading on a good path. I have some general comments and
when they are done either I or someone else should verify the patch in greater
detail.

Ideally, the ChangeLog should have per function comments to explain why the
change was done in that function.

For example, I don't know why the code in verticalScrollDistance changed and
the ChangeLog should explain this.
 
When you find yourself adding a bool and then passing in true/false for it, it
is a bad sign. It is especially bad if it is with a bunch of other bools.
Please change the bool to an enum that describes the desired behavior
(something like alignToTop or alignToEdgeIfNeeded?).

Also, it looks like Tony had a bunch of good comments which should be
addressed.