[webkit-reviews] review requested: [Bug 61344] --webkit-visual-word does not work in multi-line : [Attachment 100589] patch w/ layout test

[bugzilla-daemon at webkit.org]

Xiaomei Ji <xji at chromium.org> has asked  for review:
Bug 61344: --webkit-visual-word does not work in multi-line
https://bugs.webkit.org/show_bug.cgi?id=61344

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

------- Additional Comments from Xiaomei Ji <xji at chromium.org>
Thanks for the review!

(In reply to comment #8)
> (From update of attachment 96341 [details])
> View in context:
https://bugs.webkit.org/attachment.cgi?id=96341&action=review
> 
> Great!  New patch looks much better.	But it'll be nice if rendering experts
like mitz or dhyatt could give us some feedback.
> 
> > Source/WebCore/editing/htmlediting.h:144
> > +inline Position createPosition(Node* node, int offset)
> 
> This function name sounds too general.  How about
createPosiitonAvoidingIgnoredNode?

done.

> 
> > Source/WebCore/editing/visible_units.cpp:1506
> > +	     RenderObject* previousBlockRenderer = renderer->previousSibling();

> > +	     while (previousBlockRenderer) {
> > +		 const RenderBlock* blockWithLineBoxes =
lastChildBlockWithLineBoxes(toRenderBlock(previousBlockRenderer));
> > +		 if (blockWithLineBoxes)
> > +		     return blockWithLineBoxes->lastRootBox();
> > +
> > +		 previousBlockRenderer =
previousBlockRenderer->previousSibling();
> > +	     }
> 
> It seems like this loop is almost identical to the one in
lastChildBlockWithLineBoxes. Can we combine these two loops?

Thanks for the code snippet. Done.

> 
> > Source/WebCore/editing/visible_units.cpp:1564
> > +	 const InlineFlowBox* leftLineBox = (blockDirection == LTR) ?
rootBox->prevLineBox() :
> > +								     
rootBox->nextLineBox();
> 
> I wonder it makes a sense to add a helper function on InlineTextBox.

no change per hyatt's feedback.