[webkit-reviews] review denied: [Bug 24303] Using keyboard select RTL text, Highlight goes to opposite direction from FF&IE : [Attachment 28747] patch w/ Layout test

[bugzilla-daemon at webkit.org]

mitz at webkit.org has denied Xiaomei Ji <xji at chromium.org>'s request for review:
Bug 24303: Using keyboard select RTL text, Highlight goes to opposite direction
from FF&IE
https://bugs.webkit.org/show_bug.cgi?id=24303

Attachment 28747: patch w/ Layout test
https://bugs.webkit.org/attachment.cgi?id=28747&action=review

------- Additional Comments from mitz at webkit.org

> +2009-03-18  Xiaomei Ji  <xji at chromium.org>
> +
> +	   Reviewed by NOBODY (OOPS!).
> +
> +	   Tests:
editing/selection/extend-selection-bidi-start-with-english-in-rtl.html
> +		 
editing/selection/extend-selection-bidi-start-with-hebrew-in-rtl.html
> +		  editing/selection/extend-selection-english-in-rtl.html
> +		  editing/selection/extend-selection-hebrew-in-rtl.html
> +
> +	   * editing/SelectionController.cpp:
> +	   (WebCore::SelectionController::modifyExtendingRightForward):
> +	   (WebCore::SelectionController::modifyExtendingForward):
> +	   (WebCore::SelectionController::modifyExtendingLeftBackward):
> +	   (WebCore::SelectionController::modifyExtendingBackward):
> +	   * editing/SelectionController.h:
> +

The change log entry needs to include the bug's title and a link to it. It
should also list the changes to each function.

>  VisiblePosition
SelectionController::modifyExtendingRightForward(TextGranularity granularity)
>  {
>      VisiblePosition pos(m_sel.extent(), m_sel.affinity());
> +
> +#if PLATFORM(WIN_OS)
> +    WebCore::Node *n = pos.deepEquivalent().node();
> +    if (n) {
> +	   WebCore::RenderObject* renderer = n->renderer();
> +	   if (renderer) {
> +	       if (renderer->style()->direction() == WebCore::RTL) {
> +		   return modifyExtendingBackward(granularity, pos);
> +	       }
> +	   }  
> +    }
> +#endif

modifyExtendingRightForward() is called from both the RIGHT and FORWARD cases
of modify(), but this new behavior is the "extend right" behavior, so it is
wrong to do it in the FORWARD case. Like I said before, I think you need to
define both modifyExtendingRight() and modifyExtendinForward(), implementing
the new behavior in the former (and leaving existing behavior in the latter),
and then change modify() to call the appropriate function in every case.

I don't think these changes should be limited to WIN_OS or to any specific
platform. For now, this is supposed to be an improvement over the existing
behavior for all platforms.

You are checking the direction of the node's renderer, but is this what Firefox
does? I was under the impression that it uses the direction of the enclosing
block. You can see if your patch behaves like Firefox in these cases:
<div>Lorem <span style="direction: rtl">ipsum dolor sit</span> amet</div>
<div>Lorem <span dir="rtl">ipsum dolor sit</span> amet</div>

> Index: LayoutTests/ChangeLog
> ===================================================================
> --- LayoutTests/ChangeLog	(revision 41823)
> +++ LayoutTests/ChangeLog	(working copy)
> @@ -1,3 +1,32 @@
> +2009-03-18  Xiaomei Ji  <xji at chromium.org>
> +
> +	   Reviewed by NOBODY (OOPS!).
> +

This should also reference the bug.

> +	   *
editing/selection/extend-selection-bidi-start-with-english-in-rtl.html: Added.
> +	   *
editing/selection/extend-selection-bidi-start-with-hebrew-in-rtl.html: Added.
> +	   * editing/selection/extend-selection-english-in-rtl.html: Added.
> +	   * editing/selection/extend-selection-hebrew-in-rtl.html: Added.

Can these be combined into a single test?

> +	   *
platform/mac/editing/selection/extend-selection-bidi-start-with-english-in-rtl-
expected.checksum: Added.
> +	   *
platform/mac/editing/selection/extend-selection-bidi-start-with-english-in-rtl-
expected.png: Added.
> +	   *
platform/mac/editing/selection/extend-selection-bidi-start-with-english-in-rtl-
expected.txt: Added.

These should be text-only tests (using layoutTestController.dumpAsText). And
since the change should not be platform-specific, there shouldn't be
platform-specific results.

> +	   extendSelectionForwardByCharacterCommand();

As mentioned above, the behavior of "forward" should not change. You should be
testing the behavior of extending "right" and "left". You are not testing word,
sentence, line and paragraph granularities. If you change them (like this patch
does) then you should test all of them. But are you sure you want to change the
behavior for all granularities?