[webkit-reviews] review denied: [Bug 38213] Page up / page down moves edited content for the entire height : [Attachment 55433] Addressed the latest set of comments

[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 55433: Addressed the latest set of comments
https://bugs.webkit.org/attachment.cgi?id=55433&action=review

------- Additional Comments from David Levin <levin at chromium.org>
I think you did some changes that didn't make it in this patch. Sorry if I
sound like I'm repeating myself in these comments. I may have missed something
so let me know if somehow I thought you should do something and you already
did.

Thanks!

> Index: WebCore/ChangeLog
> +2010-05-07  Zelidrag Hornung  <zelidrag at chromium.org>
> +
> +	   Reviewed by NOBODY (OOPS!).
> +
> +	   Fixed frame page up/down scrolling calculation. Made sure that the
> +	   cursor moves with page up/down as well. Please note that now
OS(DARWIN)

OS(DARWIN) doesn't match the code anymore.

> +	   implementation will now center the center the cursor on page up/down

> +	   while other platforms will align the cursor with the top of
displayed
> +	   frame.
> +	   https://bugs.webkit.org/show_bug.cgi?id=38213
> +
> +	   Test: editing/input/option-page-up-down.html (fixed)
> +	   Test: editing/input/scroll-viewport-page-up-down.html (added)
> +
> +	   * WebCore.base.exp:
> +	   * editing/EditorCommand.cpp:
> +	   (WebCore::verticalScrollDistance): Fixed page scroll calculation.
Now scroll height is calculated only from the visible portion not the entire
frame height.
> +	   (WebCore::executeMovePageDown): Now it can tell SelectionController
to move the cursor with the page scroll up/down events.

> Index: WebCore/editing/SelectionController.cpp

> +    if (userTriggered) {
> +	   Settings* settings = m_frame ? m_frame->settings() : 0;

settings is unused.

> +	   ScrollAlignment alignment;
> +	   if (m_frame->settings()->editingBehavior() == EditingMacBehavior)
> +	       alignment = (align == ALIGN_CURSOR_ON_SCROLL_ALWAYS) ?
ScrollAlignment::alignCenterAlways : ScrollAlignment::alignCenterIfNeeded;
> +	   else
> +	       alignment = (align == ALIGN_CURSOR_ON_SCROLL_ALWAYS) ?
ScrollAlignment::alignTopAlways : ScrollAlignment::alignToEdgeIfNeeded;
> +
> +	   m_frame->revealSelection(alignment, true);

If m_frame may be 0, then this will crash. (You check for m_frame being 0
above.) I don't see how m_frame can be 0, since this is checked for on line 121
of "SelectionController::setSelection" and the code returns there. Also this
code always has used m_frame without doing a 0 check before.


> Index: WebCore/editing/SelectionController.h
> +    enum CursorAlignOnScroll { ALIGN_CURSOR_ON_SCROLL_IF_NEEDED,
> +				  ALIGN_CURSOR_ON_SCROLL_ALWAYS };

(I tried to mention this before but I must not have been clear... sorry.)
 Enum members should user InterCaps with an initial capital letter. In other
words, it should look like this:

    enum CursorAlignOnScroll { AlignCursorOnScrollAlwaysIfNeeded,
AlignCursorOnScrollAlways };

(You may wrap the line if you wish but you don't have to.)
> +    void moveTo(const VisiblePosition&, bool userTriggered = false,
CursorAlignOnScroll align = ALIGN_CURSOR_ON_SCROLL_IF_NEEDED);

(I mentioned this before but I must not have made it clear what I meant.) 
Please don't include the param name "align" since it add no information. In
other words, the line should look like this:
   void moveTo(const VisiblePosition&, bool userTriggered = false,
CursorAlignOnScroll = ALIGN_CURSOR_ON_SCROLL_IF_NEEDED);

> +    void setSelection(const VisibleSelection&, bool closeTyping = true, bool
clearTypingStyle = true, bool userTriggered = false, CursorAlignOnScroll align
= ALIGN_CURSOR_ON_SCROLL_IF_NEEDED, TextGranularity = CharacterGranularity);

Ditto.

> +    bool modify(EAlteration, int verticalDistance, bool userTriggered =
false, CursorAlignOnScroll align = ALIGN_CURSOR_ON_SCROLL_IF_NEEDED);

Ditto.

> Index: LayoutTests/ChangeLog
> +2010-05-07  Zelidrag Hornung  <zelidrag at chromium.org>
> +
> +	   Reviewed by NOBODY (OOPS!).
> +
> +	   Fixed page up/down scrolling expectations for bug:
> +	   https://bugs.webkit.org/show_bug.cgi?id=38213
> +	   Added a new test that verifies how visible portion of frame changes
> +	   while scrolling the frame. The test will check for different value
on
> +	   OS(DARWIN) than other platforms since edit frame scroll there will

OS(DARWIN) doesn't match the code anymore.

> +	   center the cursor on page up/down while it will align it with the
top
> +	   on other platforms.
> +	   
> +	   * editing/input/option-page-up-down.html: Fixed rigged test that was
masking scrolling and cursor move errors. Added tests to make sure the cursor
does not move on Mac if [option] is not pressed.

Thx! But I don't see that change in this patch?


> Index: LayoutTests/editing/input/scroll-viewport-page-up-down.html
> +function runTest() {
> +    var tolerance = 10;
> +    var frame = frames[0];
> +    var doc = frame.document;
> +    var body = doc.body;
> +    for (var i = 0; i < 10; ++i)
> +	   body.innerHTML += "<p>line " + i + "</p>\n";
> +    frame.focus();
> +    frame.getSelection().setPosition(body.firstChild, 0);
> +
> +    var onMacPlatform = navigator.userAgent.search(/\bMac OS X\b/) != -1;
> +    var modifiers = onMacPlatform ? ["altKey"] : [];
> +    
> +    if (onMacPlatform)
> +	   offsets = [ 51, 159 ];
> +    else 
> +	   offsets = [ 116, 218 ];
> +
> +    if (!window.eventSender)
> +	   return;
> +
> +    eventSender.keyDown("pageDown", modifiers);
> +    if (Math.abs(frame.pageYOffset - offsets[0]) > tolerance) {
> +	   throw "Frame viewport should be around " + offsets[0] +
> +	       "px , not at " + frame.pageYOffset;
> +    }
> +
> +    eventSender.keyDown("pageDown", modifiers);
> +    var line = frame.getSelection().baseNode.nodeValue;
> +    if (Math.abs(frame.pageYOffset - offsets[1]) > tolerance) {
> +	   throw "Frame viewport should be around " + offsets[1] +
> +	       "px , not " + frame.pageYOffset;
> +    }
> +
> +    eventSender.keyDown("pageUp", modifiers);
> +    var line = frame.getSelection().baseNode.nodeValue;
> +    if (Math.abs(frame.pageYOffset - offsets[0]) > tolerance) {
> +	   throw "Frame viewport should be around " + offsets[0] +
> +	       "px , not at " + frame.pageYOffset;
> +    }
> +
> +    document.getElementById("results").innerText = "PASS";
> +}
> +</script>


I don't see how this "ensures that the cursor [is] on the same line
on page up/down event regardless of the platform. "

>
> +<div>On Mac, option+pagedown should move the frame mouse cursor in the
middle of the frame viewport in text areas.
> +On other platforms, pagedown should move the mouse cursor at the top of the
viewport while scroll in text areas.

As noted previously, I don't understand why this talks about the cursor moving
when it doesn't seem to verify that.

> +This test requires DRT to pass.</div>

(As noted previously) Instead of saying that it requires the DRT to pass,
ideally it would tell
someone how to manually repro the test if possible.