[webkit-reviews] review denied: [Bug 60430] selectstart is not fired when selection was created by arrow keys : [Attachment 106717] Updated patch

[bugzilla-daemon at webkit.org]

Ryosuke Niwa <rniwa at webkit.org> has denied Arko Saha <nghq36 at motorola.com>'s
request for review:
Bug 60430: selectstart is not fired when selection was created by arrow keys
https://bugs.webkit.org/show_bug.cgi?id=60430

Attachment 106717: Updated patch
https://bugs.webkit.org/attachment.cgi?id=106717&action=review

------- Additional Comments from Ryosuke Niwa <rniwa at webkit.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=106717&action=review


> Source/WebCore/editing/FrameSelection.cpp:821
> +	   if (alter == AlterationExtend && m_selection.isCaret() &&
!dispatchSelectStart())

You should check that new selection is range. You can do that by using
trialFrameSelection below.

> Source/WebCore/editing/FrameSelection.cpp:1896
> +    Node* selectStartTarget =
m_frame->document()->getSelection()->focusNode();

This seems like an unnecessary indirection. I'd just do
extent().containerNode().

> Source/WebCore/editing/FrameSelection.h:247
> +    bool dispatchSelectStart();
> +

This method should be private for now.

> LayoutTests/fast/events/selectstart-by-arrow-keys.html:11
> +if (window.layoutTestController) {
> +    layoutTestController.dumpAsText();
> +}

No curly brackets around one line statement.

> LayoutTests/fast/events/selectstart-by-arrow-keys.html:16
> +div.addEventListener('selectstart', function (event) { passed = true; });

We should probably ensure that selectstart event is fired exactly once (not
twice or three times).