[webkit-reviews] review granted: [Bug 41975] SelectionController::modify should ask EditingBehavior for platform specific behavior : [Attachment 68053] patch v1

[bugzilla-daemon at webkit.org]

Ojan Vafai <ojan at chromium.org> has granted Antonio Gomes
<tonikitoo at webkit.org>'s request for review:
Bug 41975: SelectionController::modify should ask EditingBehavior for platform
specific behavior
https://bugs.webkit.org/show_bug.cgi?id=41975

Attachment 68053: patch v1
https://bugs.webkit.org/attachment.cgi?id=68053&action=review

------- Additional Comments from Ojan Vafai <ojan at chromium.org>
View in context:
https://bugs.webkit.org/attachment.cgi?id=68053&action=prettypatch

> WebCore/editing/SelectionController.cpp:688
> -	   if (!settings || settings->editingBehaviorType() !=
EditingMacBehavior || 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.
> +	   // 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.

I don't think it's beneficial to copy this comment in two places. It just makes
it more likely that they'll get out of sync. I'd just leave the comment in
EditingBehavior.h and remove this one.

> WebCore/editing/SelectionController.cpp:695
> -	   if (!settings || settings->editingBehaviorType() !=
EditingMacBehavior || 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.
> +	   // 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
(m_frame->editor()->behavior().shouldAlwaysGrowSelectionWhenExtendingToBoundary
() && !m_selection.isCaret() && isBoundary(granularity)) {
>	       if (direction == DirectionForward || direction ==
DirectionRight)
>		   setEnd(position, userTriggered);
>	       else
>		   setStart(position, userTriggered);
> -	   }
> +	   } else 
> +	       setExtent(position, userTriggered);

Why reverse the order of this if-else? I think the other order is better.
Having single line else clauses with multi-line if clauses makes for ugly. :)
In fact, I believe Darin gave me the same feedback when I originally wrote this
patch this way.