[webkit-reviews] review denied: [Bug 57806] continue experiment with moving caret by word in visual order : [Attachment 88162] patch w/ layout test
bugzilla-daemon at webkit.org
bugzilla-daemon at webkit.org
Tue Apr 5 10:42:18 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 88162: patch w/ layout test
https://bugs.webkit.org/attachment.cgi?id=88162&action=review
------- Additional Comments from Ryosuke Niwa <rniwa at webkit.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=88162&action=review
> Source/WebCore/editing/visible_units.cpp:1185
> + return previousLeaf ? Position(previousLeaf->renderer()->node(),
previousLeaf->caretMaxOffset(), Position::PositionIsOffsetInAnchor) :
> + Position(node, 0, Position::PositionIsOffsetInAnchor);
I'd use the regular if statement instead of a tertiary operator here since the
statement is really long.
> Source/WebCore/editing/visible_units.cpp:1206
> + const InlineBox* box, const VisiblePosition& previousWordBreak, int&
offsetOfWordBreak, bool& last)
what does last mean here? last in the box?
Also, this function has very few blank lines. It's hard to grasp the logic
when there is one giant block of code. Please insert blank lines as needed to
separate blocks of logic. I'm likely to have to come back to this function
again later because I'm yet to fully understand how it works. But extracting
functions and refactoring a bit should help.
> Source/WebCore/editing/visible_units.cpp:1233
> + int previousOffset = offsetOfWordBreak;
> + InlineBox* boxOfWordBreak;
> + wordBreak.getInlineBoxAndOffset(boxOfWordBreak, offsetOfWordBreak);
> + // "previousOffset < offsetOfWordBreak" is to check the offset values
are in ascending order.
> + // For example, RTL box "FED CBA" (in visual display) in LTR block, when
cursor is at the left of "CBA",
> + // nextWordPosition returns position right of "CBA", which needs to be
skipped to keep the the offsets ordered from right to left.
> + if (boxOfWordBreak == box && (!hasSeenWordBreakInThisBox ||
previousOffset < offsetOfWordBreak))
> + return wordBreak;
I'd extract this condition as a function instead of adding comment here like
this.
> Source/WebCore/editing/visible_units.cpp:1237
> + bool hasRTLPreviousBox = previousLeaf &&
!previousLeaf->isLeftToRightDirection();
I don't think we need to have a variable for this. It's pretty self-evident
what the code is doing.
> Source/WebCore/editing/visible_units.cpp:1240
> + VisiblePosition boundaryPosition;
What does boundaryPosition mean? I don't think it's descriptive.
> Source/WebCore/editing/visible_units.cpp:1244
> + boundaryPosition = leftmostPositionInRTLBoxInLTRBlock(box,
previousLeaf, nextLeaf);
> + else if (box->direction() == LTR && !hasLTRNextBox)
> + boundaryPosition = rightmostPositionInLTRBoxInRTLBlock(box,
previousLeaf, nextLeaf);
These two functions don't really need to take previous/next leaf, right? We
should let those functions call nextLeafChild and prevLeafChild themselves so
that we don't have to worry about the possibility of someone accidentally
calling them with wrong boxes (i.e. box->nextLeafChild() != nextLeaf, etc...)
> Source/WebCore/editing/visible_units.cpp:1245
> + if (!boundaryPosition.isNull()) {
Perhaps, you need to explain when this case is reached.
> Source/WebCore/editing/visible_units.cpp:1281
> +static VisiblePosition collectOneWordBreakInBox(const InlineBox* box,
TextDirection blockDirection, const VisiblePosition& previousWordBreak, int&
offsetOfWordBreak, bool& last)
> +{
> + last = false;
> + VisiblePosition wordBreak;
> + if (box->direction() == blockDirection) {
> + wordBreak =
previousWordBreakInBoxInsideBlockWithSameDirectionality(box, previousWordBreak,
offsetOfWordBreak);
> + if (wordBreak.isNull())
> + last = true;
> + } else
> + wordBreak =
nextWordBreakInBoxInsideBlockWithDifferentDirectionality(box,
previousWordBreak, offsetOfWordBreak, last);
> + return wordBreak;
> +}
> +
> +static void collectWordBreaksInBox(const InlineBox* box, TextDirection
blockDirection, Vector<std::pair<VisiblePosition, int>, 50>& wordBoundaries)
> +{
> + wordBoundaries.clear();
> + int offset;
> + bool last = false;
> + VisiblePosition wordBreak = collectOneWordBreakInBox(box,
blockDirection, VisiblePosition(), offset, last);
> + while (!last) {
> + wordBoundaries.append(std::make_pair(wordBreak, offset));
> + wordBreak = collectOneWordBreakInBox(box, blockDirection, wordBreak,
offset, last);
> + }
> + if (!wordBreak.isNull())
> + wordBoundaries.append(std::make_pair(wordBreak, offset));
> +}
I would merge these two functions since collecting "one word break" doesn't
make much sense to me.
> Source/WebCore/editing/visible_units.cpp:1299
> +static int greatestLessOffset(int offset, bool boxAndBlockInSameDirection,
const Vector<std::pair<VisiblePosition, int>, 50>& wordBoundaries)
We should give this function a better name. How about greatestValueUnder?
(credit: leviw).
> Source/WebCore/editing/visible_units.cpp:1303
> + return index;
return invalidOffset instead.
> Source/WebCore/editing/visible_units.cpp:1309
> + index = i;
> + break;
You should just "return index" here instead of assigning i to index.
> Source/WebCore/editing/visible_units.cpp:1312
> + } else {
return invalidOffset here instead so that you don't need the else clause.
> Source/WebCore/editing/visible_units.cpp:1316
> + index = i;
> + break;
Early return here agian.
> Source/WebCore/editing/visible_units.cpp:1320
> + return index;
Return invalidOffset here.
> Source/WebCore/editing/visible_units.cpp:1323
> +static int leastGreaterOffset(int offset, bool boxAndBlockInSameDirection,
const Vector<std::pair<VisiblePosition, int>, 50>& wordBoundaries)
smallestOffsetAbove?
> Source/WebCore/editing/visible_units.cpp:1327
> + return index;
return invalidOffset
> Source/WebCore/editing/visible_units.cpp:1333
> + index = i;
> + break;
Same comment about early return.
> Source/WebCore/editing/visible_units.cpp:1335
> + }
return invalidOffset here as well
> Source/WebCore/editing/visible_units.cpp:1336
> + } else {
so that you don't need else here.
> Source/WebCore/editing/visible_units.cpp:1340
> + index = i;
> + break;
early return
> Source/WebCore/editing/visible_units.cpp:1344
> + return index;
return invalidOffset
> Source/WebCore/editing/visible_units.cpp:1375
> +static VisiblePosition wordBreakCandidate(TextDirection boxDirection,
TextDirection blockDirection, WebCore::SelectionDirection searchDirection,
const VisiblePosition& visiblePosition)
> +{
> + VisiblePosition wordBreak;
> + if (boxDirection == blockDirection) {
> + if ((boxDirection == LTR && searchDirection ==
WebCore::DirectionRight)
> + || (boxDirection == RTL && searchDirection ==
WebCore::DirectionLeft)) {
> + // The word boundary at the right of space if space is used to
segment words.
> + VisiblePosition next = nextWordPosition(visiblePosition);
> + VisiblePosition nextNext = nextWordPosition(next);
> + if (next == nextNext)
> + return visiblePosition;
> + wordBreak = previousWordPosition(nextNext);
> + } else
> + wordBreak = previousWordPosition(visiblePosition);
> + } else {
> + if ((boxDirection == LTR && searchDirection ==
WebCore::DirectionLeft)
> + || (boxDirection == RTL && searchDirection ==
WebCore::DirectionRight)) {
> + // The word boundary at the left of space if space is used to
segment words.
> + VisiblePosition previous =
previousWordPosition(visiblePosition);
> + VisiblePosition previousPrevious =
previousWordPosition(previous);
> + if (previous == previousPrevious)
> + wordBreak = previous;
> + else
> + wordBreak = nextWordPosition(previousPrevious);
> + } else
> + wordBreak = nextWordPosition(visiblePosition);
> + }
> + return wordBreak;
> +}
I'd break this function into two pieces as we did last time. In fact, I'd
embed the following code in rightWordPosition:
if (boxDirection == blockDirection) {
if (boxDirection == LTR)
wordBreakCandidate = positionAfterNextWord(visiblePosition)
else
wordBreakCandidate = previousWordPosition(visiblePosition);
} else {
if (boxDirection == RTL)
wordBreakCandidate = positionBeforePreviousWord(visiblePosition)
else
wordBreakCandidate = nextWordPosition(visiblePosition);
}
where
static VisiblePosition positionAfterNextWord(VisiblePosition position) {
VisiblePosition beforeNextWord = nextWordPosition(position);
VisiblePosition beforeWordAfterNextWord = nextWordPosition(beforeNextWord);
if (beforeNextWord == beforeWordAfterNextWord)
return VisiblePosition();
return previousWordPosition(beforeWordAfterNextWord);
}
static VisiblePosition positionBeforePreviousWord(VisiblePosition position) {
VisiblePosition afterNextWord = previousWordPosition(position);
VisiblePosition afterWordAfterNextWord =
previousWordPosition(afterNextWord);
if (afterNextWord == afterWordAfterNextWord)
return VisiblePosition();
return nextWordPosition(previousPrevious);
}
for leftWordBoundary, I'd add do:
if (boxDirection == blockDirection) {
if (boxDirection == RTL)
wordBreakCandidate = positionAfterNextWord(visiblePosition);
else
wordBreakCandidate = previousWordPosition(visiblePosition);
} else {
if (boxDirection == LTR)
wordBreakCandidate = positionBeforePreviousWord(visiblePosition);
else
wordBreakCandidate = nextWordPosition(visiblePosition);
}
> Source/WebCore/editing/visible_units.cpp:1424
> + InlineBox* boxOfWordBreak;
> + int offsetOfWordBreak;
> + wordBreak.getInlineBoxAndOffset(boxOfWordBreak, offsetOfWordBreak);
> + if (box == boxOfWordBreak && offsetOfWordBreak !=
boxOfWordBreak->caretMaxOffset() && offsetOfWordBreak !=
boxOfWordBreak->caretMinOffset())
I would extract this as a function that takes a VisiblePosition and a box.
Maybe positionIsInsideBox?
More information about the webkit-reviews
mailing list