[webkit-reviews] review denied: [Bug 51389] Assertion failure after removing a selection in keydown handler : [Attachment 84193] fixes the bug

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has denied Ryosuke Niwa <rniwa at webkit.org>'s
request for review:
Bug 51389: Assertion failure after removing a selection in keydown handler
https://bugs.webkit.org/show_bug.cgi?id=51389

Attachment 84193: fixes the bug
https://bugs.webkit.org/attachment.cgi?id=84193&action=review

------- Additional Comments from Darin Adler <darin at apple.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=84193&action=review

I’d like to see these changes land separately; especially if that makes it
clearer which of them causes the change in observed test results. Is there some
reason we can’t do it that way?

I’m going to say review- because there are enough specific things wrong here
that it should not land as-is.

> Source/WebCore/ChangeLog:31
> +2011-02-28  Ryosuke Niwa  <rniwa at webkit.org>

Double change log.

> Source/WebCore/dom/CharacterData.cpp:157
> -    if (document()->frame())
> -	   document()->frame()->selection()->textWillBeReplaced(this,
offsetOfReplacedData, oldLength, newLength);
>      RefPtr<StringImpl> oldData = m_data;
>      m_data = newData;
>      updateRenderer(offsetOfReplacedData, oldLength);
> +    if (document()->frame())
> +	   document()->frame()->selection()->textWillBeReplaced(this,
offsetOfReplacedData, oldLength, newLength);

Would you be willing to land this change separately in its own patch?

> Source/WebCore/editing/SelectionController.cpp:287
> -    if ((type == EndPointIsStart &&
static_cast<unsigned>(position.offsetInContainerNode()) >= offset + oldLength)
> +    if (((type == EndPointIsStart || oldLength) &&
static_cast<unsigned>(position.offsetInContainerNode()) >= offset + oldLength)
>	   || (type == EndPointIsEnd &&
static_cast<unsigned>(position.offsetInContainerNode()) > offset + oldLength))

I find this function and change quite confusing. I can almost understand it
given the change log comments. Could you add some “why” comments to the source
file?

All these calls to static_cast<unsigned>(position.offsetInContainerNode()) make
me think the whole function would be way easier to read if that was put in a
local variable.

> Source/WebCore/editing/SelectionController.cpp:321
> +	   // Since this function can be called while recalculating style, we
do as little as possible.
> +	   // Notify IME of the selection change so that it clears the current
composition.

I don’t understand this comment. The things that need to be done, need to be
done. So what does it mean to do as little as possible? We should *always* do
as little as possible. Maybe it would be clearer if you said something more
specific? I am quite worried about this!

> LayoutTests/ChangeLog:16
> +	   Also fixed
platform/mac/editing/input/selection-change-closes-typing.html
> +	   so that it calls setMarkedText with offset 0 and length 1 as
supposed to
> +	   offset 1 and length 0.

Would you be willing to land this change first in a separate patch?