[webkit-reviews] review denied: [Bug 33413] selection.modify extends too far with 'lineboundary' : [Attachment 50376] patch v0

[bugzilla-daemon at webkit.org]

Alexey Proskuryakov <ap at webkit.org> has denied MORITA Hajime
<morrita at google.com>'s request for review:
Bug 33413: selection.modify extends too far with 'lineboundary'
https://bugs.webkit.org/show_bug.cgi?id=33413

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

------- Additional Comments from Alexey Proskuryakov <ap at webkit.org>
+	      * So we mimic UPSTREAM affinity to prevent to crossing
+	      * line when the offset is at the end of line.

Two comments:
1) We use C++ style comments ("//") in C++ code.
2) I don't understand why this says "mimic". Don't we just plain use it? And
would anything break if you didn't add
"pos.setAffinity(m_selection.affinity());"?

> Or, you can leave the change description minimal and annotate the
files/methods instead.

Yes, that's the preferred way of writing ChangeLogs.

> +	   Re-baseline: editing/selection/extend-selection-expected.txt

"Baseline" is a noun, so using it as a verb is a mistake. You could just say
"updated results" instead.

> I'm not really sure what sentenceboundary/paragraphboundary are used for, 
> i.e. do they correspond to a user-action?

Without actually checking the code, I suspect that you get paragraphboundary
when triple-clicking and dragging, and also when triple-clicking and then
extending the selection from keyboard with Cmd+Shift+Right (but when I tested,
the results from the latter seemed wrong).

I think this needs another iteration, r- for now. I agree with Ojan's comments
about code changes.