Bug 41975 - SelectionController::modify should ask EditingBehavior for platform specific behavior
Summary: SelectionController::modify should ask EditingBehavior for platform specific ...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: HTML Editing (show other bugs)
Version: 528+ (Nightly build)
Hardware: PC All
: P2 Normal
Assignee: Antonio Gomes
URL:
Keywords:
Depends on: 39854
Blocks: 36539
  Show dependency treegraph
 
Reported: 2010-07-09 12:30 PDT by Antonio Gomes
Modified: 2010-09-20 18:57 PDT (History)
3 users (show)

See Also:


Attachments
(committed with r67909, r=ojan) patch v1 (6.68 KB, patch)
2010-09-19 22:38 PDT, Antonio Gomes
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Antonio Gomes 2010-07-09 12:30:53 PDT
... instead, it is accessing EditingMacBehavior directly today.
Comment 1 Antonio Gomes 2010-09-19 19:13:37 PDT
Finally back on it. 

So, this bug is about the following snippet:

669	        case EXTEND: 
670	 	            setExtent(pos, userTriggered); 
671	 	            break; 
681	            if (!settings || settings->editingBehavior() != EditingMacBehavior || m_selection.isCaret() || !isBoundary(granularity)) 
682	                setExtent(position, userTriggered); 
683	            else { 
684	                // Standard Mac behavior when extending to a boundary is grow the selection rather 
685	                // than leaving the base in place and moving the extent. Matches NSTextView. 
686	                if (direction == FORWARD || direction == RIGHT) 
687	                    setEnd(position, userTriggered); 
688	                else 
689	                    setStart(position, userTriggered); 
690	            } 

I tried to come up with a name for this specific editing behavior based on the comment. It was: 

bool shouldGrowSelectionWhenExtendingToBoundary() const { return m_type == EditingMacBehavior; }

... and Darin asked "If you don't grow the selection when extending to a boundary, what do you do? Shrink the selection?"

Better suggestions?
Comment 2 Ojan Vafai 2010-09-19 19:20:58 PDT
Maybe shouldAlwaysGrowSelectionWhenExtendingToBoundary? The thing with win/linux is that they sometimes grow, sometimes shrink, but on mac it always grows.

Another option would be to name it based on the win/linux behavior. Something like shouldAlwaysMoveExtentWhenExtendingToBoundary. I think the former is better.
Comment 3 Antonio Gomes 2010-09-19 22:38:05 PDT
Created attachment 68053 [details]
(committed with r67909, r=ojan) patch v1

Based on Ojan's naming suggestion.
Comment 4 Ojan Vafai 2010-09-19 23:07:17 PDT
Comment on attachment 68053 [details]
(committed with r67909, r=ojan) patch v1

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.
Comment 5 Antonio Gomes 2010-09-20 18:54:24 PDT
Comment on attachment 68053 [details]
(committed with r67909, r=ojan) patch v1

Clearing flags on attachment: 68053

Committed r67909 <http://trac.webkit.org/changeset/67909>