[webkit-reviews] review denied: [Bug 25195] mouse-based selections are always directional on Window/Linux : [Attachment 51110] Patch

[bugzilla-daemon at webkit.org]

David Levin <levin at chromium.org> has denied Ojan Vafai <ojan at chromium.org>'s
request for review:
Bug 25195: mouse-based selections are always directional on Window/Linux
https://bugs.webkit.org/show_bug.cgi?id=25195

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

------- Additional Comments from David Levin <levin at chromium.org>
Nothing huge but some changes may be significant so r- for one more round.


> diff --git a/LayoutTests/editing/selection/5195166-1.html
b/LayoutTests/editing/selection/5195166-1.html

Since you're changing the image result, would you consider making this a
dumpAsText test? (I think it already tests everything needed without any
changes.)

The plus side is that you won't have to generate the mac result which is
missing (but must be incorrect after this change.)


> diff --git
a/LayoutTests/platform/win/editing/selection/extend-after-mouse-selection-expec
ted.txt
b/LayoutTests/platform/win/editing/selection/extend-after-mouse-selection-expec
ted.txt

> +PASS Selection is [anchorNode: [object Text](bc) anchorOffset: 0 focusNode:
[object Text](ef) focusOffset: 1 isCollapsed: false]
> +PASS Selection is [anchorNode: [object Text](bc) anchorOffset: 0 focusNode:
[object Text](ef) focusOffset: 2 isCollapsed: false]
> +PASS Selection is [anchorNode: [object Text](bc) anchorOffset: 0 focusNode:
[object Text]( ) focusOffset: 1 isCollapsed: false]
> +PASS Selection is [anchorNode: [object Text](bc) anchorOffset: 0 focusNode:
[object HTMLElement](null) focusOffset: 4 isCollapsed: false]

From
http://trac.webkit.org/browser/trunk/LayoutTests/editing/selection/extend-after
-mouse-selection.html
  " // Seleciton goes from the end of "ef" to the first BR element in the
contentEditable region."

Is the text of the test incorrect because it refers to "ef"? (If you change
this line, please fix this typo "Seleciton" as well.)


   "assertSelectionAt(endTarget.firstChild, 2, document.getElementById('test'),
3);"

I also don't understand how the assert in the test passes.


> diff --git a/WebCore/ChangeLog b/WebCore/ChangeLog
> +2010-03-18  Ojan Vafai  <ojan at chromium.org>
> +
> +	   Reviewed by NOBODY (OOPS!).
> +
> +	   mouse-based selections are always directional on Windows/Linux
> +	   https://bugs.webkit.org/show_bug.cgi?id=25195
> +
> +	   Change m_lastChangeWasHorizontalExtension to m_isDirectional
> +	   and make m_isDirectional always be true for Windows/Linux.
> +
> +	   * editing/SelectionController.cpp:
> +	   (WebCore::SelectionController::SelectionController):
> +	   (WebCore::SelectionController::setSelection):
> +	   (WebCore::SelectionController::setIsDirectional):
> +	   (WebCore::SelectionController::willBeModified):
> +	   When double-clicking the base/extent will be in the middle

When double-clicking,
(Added ,)

> +	   of the seleciton instead of the start/end of it. Changed to

typo: seleciton

> +void SelectionController::setIsDirectional(bool isDirectional)
> +{
> +    Settings* settings = m_frame ? m_frame->settings() : 0;
> +    if (settings && settings->editingBehavior() == EditingMacBehavior)
> +	   m_isDirectional = isDirectional;
> +    else
> +	   m_isDirectional = true;

How do you feel about this?
	m_isDirectional = !settings || (settings->editingBehavior() ==
EditingMacBehavior && isDirectional);