[webkit-reviews] review granted: [Bug 51001] Moving or selecting backwards by words jumps to start of contenteditable region if contenteditable=false span is encountered : [Attachment 77653] Patch

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has granted Benjamin (Ben) Kalman
<kalman at chromium.org>'s request for review:
Bug 51001: Moving or selecting backwards by words jumps to start of
contenteditable region if contenteditable=false span is encountered
https://bugs.webkit.org/show_bug.cgi?id=51001

Attachment 77653: Patch
https://bugs.webkit.org/attachment.cgi?id=77653&action=review

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

For now I’m setting commit-queue- because you might want to make changes based
on my comments.

> WebCore/editing/TextIterator.cpp:1239
> +bool SimplifiedBackwardsTextIterator::setNodeIfNotNull(Node* node)

All this function does is an assignment statement and a null check. I think
both call sites would be clearer if this was written there instead of using a
function.

> WebCore/editing/visible_units.cpp:52
> +static Position rightmostEquivalent(Position p)

In WebKit code normally don’t use letters for names. The word “position” would
be better than “p” given our usual style. Although there are quite a few
counterexamples in this source file. For new code it would be better to use
words.