[webkit-reviews] review denied: [Bug 51389] Assertion failure after removing a selection in keydown handler : [Attachment 77447] 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 77447: fixes the bug
https://bugs.webkit.org/attachment.cgi?id=77447&action=review

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

Despite the fact that the code changes and test case look quite good to me, I
am going to say review- because I feel the patch does not contain sufficient
change log or source code comments to make it clear why these changes are
correct.

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

I don’t understand this change. It may well be correct, but nothing in the
change log explains it.

This is one reason I like per-function change log comments. I believe I would
have had a better chance of understanding this with a per-function comment. But
a “why” comment in the code would also be welcome.

> WebKit/mac/ChangeLog:8
> +	   Preserve selection when unmarking text.

I don’t understand the relationship of this change to the bug you’re fixing.
Again, I don’t think this is incorrect, but rather than you are not explaining
yourself sufficiently in the patch’s change log or comments.

Under what circumstances should any call site call confirmComposition rather
than confirmCompositionWithoutDisturbingSelection? Why isn’t this one of those
circumstances? What does this have to do with the assertion failure?

Is there a test case that covers this?