[webkit-reviews] review granted: [Bug 36539] shift+home/end and cmd+shift+left/right don't extend the selection correctly : [Attachment 51683] Patch

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has granted Ojan Vafai <ojan at chromium.org>'s
request for review:
Bug 36539: shift+home/end and cmd+shift+left/right don't extend the selection
correctly
https://bugs.webkit.org/show_bug.cgi?id=36539

Attachment 51683: Patch
https://bugs.webkit.org/attachment.cgi?id=51683&action=review

------- Additional Comments from Darin Adler <darin at apple.com>
> +    Settings* settings = m_frame ? m_frame->settings() : 0;
> +    // The trialSelectionController does not have access to an m_frame, so
we need to get the editing behavior first.
> +    isEditingMacBehavior = isEditingMacBehavior || settings &&
settings->editingBehavior() == EditingMacBehavior;

Adding yet another parameter to the already complex modify function is annoying
and available.

The use of "||" here seems a bit haphazard to me. There's no real need for an
"or" here.

Instead it would be better to break the modify function into a public one with
the same interface as today, and a private function, probably with a different
name, that takes an additional argument. The cleanest type for the additional
argument would be EditingBehavior, but you might not want to do all the work to
separate out the EditingBehavior enum so it can be included in the
SelectionController.h header file without pulling in all of Settings.h. So you
the extra argument to the private function could be a Settings*.

> +	       // This matches NSTextView behavior. When extending a selection
via LineBoundary, ParagraphBoundary or
> +	       // DocumentBoundary, the selection always grows instead of just
moving the extent.
> +	       if (isEditingMacBehavior && !m_selection.isCaret()
> +		   && (granularity == LineBoundary || granularity ==
ParagraphBoundary || granularity == DocumentBoundary)
> +		   && ((dir == FORWARD || dir == RIGHT) &&
!m_selection.isBaseFirst()
> +		       || (dir == LEFT || dir == BACKWARD) &&
m_selection.isBaseFirst()))

This long expression with nested boolean logic is hard to read.

A couple helper functions might collapse this and make it readable, for example
one tells if a granularity is one of the "boundary" styles instead of listing
them all.

Another kind of helper function that could make the logic clearer would be
setStart and setEnd functions that decide whether to modify the base or extent.
Using them instead of having logic here with all that isBaseFirst stuff could
make it a lot easier to see what the code is doing.

    if (!isEditingMacBehavior || m_selection.isCaret() ||
!isBoundary(granularity))
	setExtent(position, userTriggered);
    else {
	// Standard Mac behavior when extending to a boundary is grow the
selection rather
	// than leaving the base in place and moving the extent. Matches
NSTextView.
	if (direction == FORWARD || direction == RIGHT)
	    setEnd(position, userTriggered);
	else
	    setStart(position, userTriggered);
    }

I think the code I wrote above is easier to understand.

I'm going to say review+, but I think both of my comments represent
opportunities to make this code significantly clearer. Please consider my
suggestions.