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

[bugzilla-daemon at webkit.org]

Xiaomei Ji <xji at chromium.org> has canceled 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 93152: patch w/ layout test
https://bugs.webkit.org/attachment.cgi?id=93152&action=review

------- Additional Comments from Xiaomei Ji <xji at chromium.org>
(In reply to comment #20)
> (From update of attachment 93152 [details])
> View in context:
https://bugs.webkit.org/attachment.cgi?id=93152&action=review
> 
> > Source/WebCore/editing/visible_units.cpp:1183
> > +	     // "abc |def |    hij |opq". We are traversing these boxes from
right to left. The word 
> > +	     // break between "def" and "hij" (which is position 8) is not in
box "hij opq", and it 
> > +	     // should be added as a word break (the rightmost word break) when
traversing box "abc def".
> 
> It's still not clear which of these two boxes correspond to this "box"
(current one).

Changed to:
	// "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)
	// is added as the rightmost word break.


> 
> > Source/WebCore/editing/visible_units.cpp:1277
> > +	 if (wordBreak == previousWordBreak)
> > +	     return VisiblePosition();	  
> > +	
> 
> Why do we need this early exit?

You are right. They are actually not needed.

> 
> > Source/WebCore/editing/visible_units.cpp:1327
> > +		 offsetOfWordBreak = offset;
> > +		 isLastWordBreakInBox = false;
> > +		 previousWordBreakWasAtBoxStart = true;
> > +		 return positionAfterWord;
> 
> This makes me think that we might want to bundle up all these functions as a
class.

that could be a good idea. Filed https://bugs.webkit.org/show_bug.cgi?id=60910
for record.