[webkit-reviews] review denied: [Bug 57806] continue experiment with moving caret by word in visual order : [Attachment 88319] patch w/ layout test
bugzilla-daemon at webkit.org
bugzilla-daemon at webkit.org
Wed Apr 6 06:58:12 PDT 2011
Ryosuke Niwa <rniwa at webkit.org> has denied Xiaomei Ji <xji at chromium.org>'s
request for review:
Bug 57806: continue experiment with moving caret by word in visual order
https://bugs.webkit.org/show_bug.cgi?id=57806
Attachment 88319: patch w/ layout test
https://bugs.webkit.org/attachment.cgi?id=88319&action=review
------- Additional Comments from Ryosuke Niwa <rniwa at webkit.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=88319&action=review
Okay, I think leftmostPositionInRTLBoxInLTRBlock,
rightmostPositionInLTRBoxInRTLBlock, & lastWordBreakInBox look pretty sane to
me. They should be good to go after this iteration. The rest seems to require a
couple more iterations. Thanks for patiently improving the patch :)
> Source/WebCore/editing/visible_units.cpp:1179
> + VisiblePosition leftmost;
This variable doesn't seem to be ever used. Please remove.
> Source/WebCore/editing/visible_units.cpp:1183
> + if (previousLeaf && !previousLeaf->isLeftToRightDirection())
Let's put a blank line right above this line to improve the readability.
> Source/WebCore/editing/visible_units.cpp:1196
> + VisiblePosition rightmost;
Ditto; can't find any reference.
> Source/WebCore/editing/visible_units.cpp:1200
> + if (nextLeaf && nextLeaf->isLeftToRightDirection())
Ditto about inserting a blank line here.
> Source/WebCore/editing/visible_units.cpp:1220
> + if (boundaryPosition.isNotNull()) { // boundaryPosition is not null only
in the above 2 cases.
Please do an early return instead.
> Source/WebCore/editing/visible_units.cpp:1232
> +static bool positionIsVisuallyOrderedInBox(const VisiblePosition& wordBreak,
const InlineBox* box, int& offsetOfWordBreak)
I don't think positionIsVisuallyOrderedInBox is a descriptive name here. It
explains what the function is checking but it doesn't explain anything in the
context of nextWordBreakInBoxInsideBlockWithDifferentDirectionality. Please
rename.
> Source/WebCore/editing/visible_units.cpp:1237
> + // "previousOffset < offsetOfWordBreak" is to guarantee that the word
breaks collected are visually ordered.
I don't think this comment is helpful. It doesn't explain why "previousOffset
< offsetOfWordBreak" guarantees that the words breaks are visually ordered.
> Source/WebCore/editing/visible_units.cpp:1262
> + isLastWordBreakInBox = false;
> + if (wordBreak == previousWordBreak) {
> + isLastWordBreakInBox = true;
It seems redundant to set isLastWordBreakInBox to false if we're immediately
setting it back to true. I'd move isLastWordBreakInBox = false below the if
clause.
> Source/WebCore/editing/visible_units.cpp:1304
> +static VisiblePosition wordBreakInBox(const InlineBox* box, TextDirection
blockDirection, const VisiblePosition& previousWordBreak, int&
offsetOfWordBreak, bool& isLastWordBreakInBox)
> +{
> + isLastWordBreakInBox = false;
> + VisiblePosition wordBreak;
> + if (box->direction() == blockDirection) {
> + wordBreak =
previousWordBreakInBoxInsideBlockWithSameDirectionality(box, previousWordBreak,
offsetOfWordBreak);
> + if (wordBreak.isNull())
> + isLastWordBreakInBox = true;
> + } else
> + wordBreak =
nextWordBreakInBoxInsideBlockWithDifferentDirectionality(box,
previousWordBreak, offsetOfWordBreak, isLastWordBreakInBox);
> + return wordBreak;
> +}
> +
> +static void collectWordBreaksInBox(const InlineBox* box, TextDirection
blockDirection, Vector<std::pair<VisiblePosition, int>, 50>& wordBoundaries)
> +{
> + wordBoundaries.clear();
> + int offset = invalidOffset;
> + bool isLastWordBreakInBox = false;
> + VisiblePosition wordBreak = wordBreakInBox(box, blockDirection,
VisiblePosition(), offset, isLastWordBreakInBox);
> + while (!isLastWordBreakInBox) {
> + wordBoundaries.append(std::make_pair(wordBreak, offset));
> + wordBreak = wordBreakInBox(box, blockDirection, wordBreak, offset,
isLastWordBreakInBox);
> + }
> + if (!wordBreak.isNull())
> + wordBoundaries.append(std::make_pair(wordBreak, offset));
> +}
As I said in the previous comment, I don't see why these two functions can't be
merged. In fact, I just did it for you (please verify):
static void collectWordBreaksInBox(const InlineBox* box, TextDirection
blockDirection, Vector<std::pair<VisiblePosition, int>, 50>& wordBoundaries)
{
wordBoundaries.clear();
VisiblePosition wordBreak;
int offsetOfWordBreak = invalidOffset;
while (1) {
if (box->direction() == blockDirection) {
wordBreak =
previousWordBreakInBoxInsideBlockWithSameDirectionality(box, wordBreak,
offsetOfWordBreak);
if (wordBreak.isNull())
break;
wordBoundaries.append(std::make_pair(wordBreak, offset));
} else {
bool isLastWordBreakInBox = false;
wordBreak =
nextWordBreakInBoxInsideBlockWithDifferentDirectionality(box, wordBreak,
offsetOfWordBreak, isLastWordBreakInBox);
if (wordBreak.isNotNull())
wordBoundaries.append(std::make_pair(wordBreak, offset));
if (isLastWordBreakInBox)
break;
}
}
}
Once I did that, it was obvious to me that we should have two different
functions for same directionality and different directionality as in:
static void collectWordBreaksInBoxInsideBlockWithSameDirectionality(const
InlineBox* box, Vector<std::pair<VisiblePosition, int>, 50>& wordBoundaries)
{
wordBoundaries.clear();
VisiblePosition wordBreak;
int offsetOfWordBreak = invalidOffset;
while (1) {
wordBreak =
previousWordBreakInBoxInsideBlockWithSameDirectionality(box, wordBreak,
offsetOfWordBreak);
if (wordBreak.isNull())
break;
wordBoundaries.append(std::make_pair(wordBreak, offset));
}
}
static void collectWordBreaksInBoxInsideBlockWithDifferntDirectionality(const
InlineBox* box, Vector<std::pair<VisiblePosition, int>, 50>& wordBoundaries)
{
wordBoundaries.clear();
VisiblePosition wordBreak;
int offsetOfWordBreak = invalidOffset;
while (1) {
bool isLastWordBreakInBox = false;
wordBreak =
nextWordBreakInBoxInsideBlockWithDifferentDirectionality(box, wordBreak,
offsetOfWordBreak, isLastWordBreakInBox);
if (wordBreak.isNotNull())
wordBoundaries.append(std::make_pair(wordBreak, offset));
if (isLastWordBreakInBox)
break;
}
}
> Source/WebCore/editing/visible_units.cpp:1322
> +static int greatestValueUnder(int offset, bool boxAndBlockInSameDirection,
const Vector<std::pair<VisiblePosition, int>, 50>& wordBoundaries)
You should rename wordBoundaries to something like orderedWordBoundaries to
signify the fact they're ordered. Or otherwise things doesn't make sense.
Also I would have much preferred if you defined a struct instead of using a
pair here so that I know what a pair represents.
Nit: s/boxAndBlockInSameDirection/boxAndBlockAreInSameDirection/.
> Source/WebCore/editing/visible_units.cpp:1394
> + if (box == boxOfWordBreak && offsetOfWordBreak != box->caretMaxOffset()
&& offsetOfWordBreak != box->caretMinOffset())
> + return true;
> + return false;
Nit: we should do:
return box == boxOfWordBreak && offsetOfWordBreak != box->caretMaxOffset() &&
offsetOfWordBreak != box->caretMinOffset();
instead.
> Source/WebCore/editing/visible_units.cpp:1397
> +static VisiblePosition positionAfterNextWord(const VisiblePosition&
position)
Oops, this should probably named as positionAfterWord instead. Since if we're
in "he^llo world", then this function will return "hello^ world".
> Source/WebCore/editing/visible_units.cpp:1400
> + VisiblePosition next = nextWordPosition(position);
> + VisiblePosition nextNext = nextWordPosition(next);
next and nextNext don't make any sense to me. Next what? It's much clearer to
call next as beforeNextWord because it signifies the fact nextWordPosition
returns the position between the next word and the whitespace immediately
before the next word. It then becomes clear from your other comment that we
must move forward again to obtain beforeWordAfterNextWord or afterNextWord &
backward in order obtain the position after the current word.
> Source/WebCore/editing/visible_units.cpp:1406
> +static VisiblePosition positionBeforePreviousWord(const VisiblePosition&
position)
Ditto. This should be named as positionBeforeWord
> Source/WebCore/editing/visible_units.cpp:1409
> + VisiblePosition previous = previousWordPosition(position);
> + VisiblePosition previousPrevious = previousWordPosition(previous);
Ditto about local variable names here.
> Source/WebCore/editing/visible_units.cpp:1446
> + // Otherwise, first collect all word breaks in terms of
(VisiblePosition, offset) inside the box.
Please get rid of this comment. It repeats the function name
collectWordBreaksInBox.
> Source/WebCore/editing/visible_units.cpp:1447
> + Vector<std::pair<VisiblePosition, int>, 50> wordBoundaries;
Why 50? That sounds like an arbitrary limit to me. I'd rather not to specify
any number here.
> Source/WebCore/editing/visible_units.cpp:1449
> + // Then, search for word boundaries inside the box.
Please remove this comment. It repeats what code says.
> Source/WebCore/editing/visible_units.cpp:1455
> + // If not found, search for word boundaries in visually adjacent boxes.
Please remove this comment. It repeats the code.
> Source/WebCore/editing/visible_units.cpp:1456
> + return leftWordBoundary(box->prevLeafChild(), invalidOffset,
blockDirection);
Mn... this is a tail recursion. I wonder if we can make it iterative in a some
followup patch.
More information about the webkit-reviews
mailing list