[webkit-reviews] review denied: [Bug 57543] -webkit-visual-word does not work on words separated by multiple spaces : [Attachment 93675] patch w/ layout test

[bugzilla-daemon at webkit.org]

Ryosuke Niwa <rniwa at webkit.org> has denied Xiaomei Ji <xji at chromium.org>'s
request for review:
Bug 57543: -webkit-visual-word does not work on words separated by multiple
spaces
https://bugs.webkit.org/show_bug.cgi?id=57543

Attachment 93675: patch w/ layout test
https://bugs.webkit.org/attachment.cgi?id=93675&action=review

------- Additional Comments from Ryosuke Niwa <rniwa at webkit.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=93675&action=review

> Source/WebCore/editing/visible_units.cpp:1182
> +	   // and the "hij opq" (starts at 12 and length is 7). The word breaks
are 
> +	   // "abc |def |    hij |opq". We are traversing these boxes from
right to left. When
> +	   // traversing box "abc def", the word break between "def" and "hij"
(which is position 8)

You need to clarify why we need to special-case this and which one of "abc def"
or "hij opq" is current box.

How about something like this?

We normally catch the line break between "def" and "hij" when we visit the box
that contains "hij opq"
but the line break doesn't exist in the box that contains "hij opq" when there
are multiple spaces.
So we detect it when we're traversing the box that contains "abc def " instead.


> Source/WebCore/editing/visible_units.cpp:1190
> +	      
positionBeforeWord.getInlineBoxAndOffset(boxContainingPreviousWordBreak,
offsetOfWordBreak);

To address the issue below, you should declare boxContainingPreviousWordBreak
here.

> Source/WebCore/editing/visible_units.cpp:1201
> -    InlineBox* boxContainingPreviousWordBreak;
>      wordBreak.getInlineBoxAndOffset(boxContainingPreviousWordBreak,
offsetOfWordBreak);

I don't think we should be reusing variable like this.	Please declare
boxContainingPreviousWordBreak here.

> Source/WebCore/editing/visible_units.cpp:1321
> +	   InlineBox* boxContainingWordBreak;
> +	   int offset;
> +	   positionAfterWord.getInlineBoxAndOffset(boxContainingWordBreak,
offset);
> +    
> +	   if (boxContainingWordBreak == box) {

Can we extract this as a function?  e.g. positionIsInBox(const
VisiblePosition&, InlineBox*)?

> Source/WebCore/editing/visible_units.cpp:1330
> +    previousWordBreakWasAtBoxStart = false;
> +
> +    VisiblePosition wordBreak = (hasSeenWordBreakInThisBox &&
!previousWordBreakWasAtBoxStart) ? previousWordBreak :
Position(box->renderer()->node(), box->caretMinOffset(),
Position::PositionIsOffsetInAnchor);

Huh? Isn't previousWordBreakWasAtBoxStart always false here?  r- because of
this.