[Webkit-unassigned] [Bug 61346] --webkit-visual-word: ctrl-arrow is not able to reach the boundary of line

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=61346





--- Comment #6 from Ryosuke Niwa <rniwa at webkit.org>  2011-07-11 16:04:35 PST ---
(From update of attachment 100372)
View in context: https://bugs.webkit.org/attachment.cgi?id=100372&action=review

I'm concerned that this code is really losing the original benefit of not doing linear search.  I feel like we can get a cleaner & more efficient code by collecting all word break in a visual position.

> Source/WebCore/ChangeLog:6
> +	Added the word boundary corresponding to the logical end-of-line to the
> +	word boundary vector; previously, this word boundary wasn't collected.

Nit: Tab.  Also, the description should appear after a blank line following "Reviewed by NOBODY" (followed by a blank line before "* editing/visible_units.cpp:").

> Source/WebCore/editing/visible_units.cpp:1347
> +            return Position(static_cast<Text*>(currentLTRBox->renderer()->node()), currentLTRBox->caretMaxOffset());

This isn't really the leftmost position, is it?  This is the leftmost position in the current LTR run, right?

> Source/WebCore/editing/visible_units.cpp:1447
> +    // Check if box is visually last in block; if so, append the end of line
> +    // position to the word break vector

This comment repeats the code.

> Source/WebCore/editing/visible_units.cpp:1448
> +    if (isLastVisualBoxInBlock(box, blockDirection)) {

Perhaps you want to call this function isBoxVisuallyLastInBlock?

> Source/WebCore/editing/visible_units.cpp:1482
> +        WordBoundaryEntry wordBoundaryEntry(endOfBlock, offsetOfEndOfBlock);
> +        orderedWordBoundaries.append(wordBoundaryEntry);

Nit: can't we just do orderedWordBoundaries.append(WordBoundaryEntry(endOfBlock, offsetOfEndOfBlock)); ?

> Source/WebCore/editing/visible_units.cpp:1517
> -    }        
> +    }

Please revert these whitespace changes.  It's bloating the patch needlessly.

> Source/WebCore/editing/visible_units.cpp:-1426
>      return VisiblePosition();
>  }
> -    

Ditto.

> Source/WebCore/editing/visible_units.cpp:1635
> +    // Check for empty div edge case.

This comment repeats what code says.

> Source/WebCore/editing/visible_units.cpp:1710
> -}
> +} // namespace WebCore

Let's not include these changes in one patch.

> LayoutTests/ChangeLog:5
> +	Modified two tests to test for visual word movement to end-of-line.

Nit: Tab

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.