[webkit-reviews] review denied: [Bug 28245] findNextLineBreak crashes when inserting a blank span in front of a long line : [Attachment 45394] Fix for bug

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has denied Mike Moretti <webkit9 at mordent.com>'s
request for review:
Bug 28245: findNextLineBreak crashes when inserting a blank span in front of a
long line
https://bugs.webkit.org/show_bug.cgi?id=28245

Attachment 45394: Fix for bug
https://bugs.webkit.org/attachment.cgi?id=45394&action=review

------- Additional Comments from Darin Adler <darin at apple.com>
Looks good. There are tabs in the test case, so we won't be able to land it.

> -			       if (lBreak.obj &&
shouldPreserveNewline(lBreak.obj) && lBreak.obj->isText() &&
!toRenderText(lBreak.obj)->isWordBreak() &&
toRenderText(lBreak.obj)->characters()[lBreak.pos] == '\n') {
> +			       if (lBreak.obj &&
shouldPreserveNewline(lBreak.obj) && lBreak.obj->isText() &&
toRenderText(lBreak.obj)->textLength() > 0 &&
!toRenderText(lBreak.obj)->isWordBreak() &&
toRenderText(lBreak.obj)->characters()[lBreak.pos] == '\n') {

Another way to write this is:

    (*toRenderText(lBreak.obj)->text())[0] == '\n'

Because subscripting with StringImpl does a range check. It's not
super-elegant, but a little less wordy.

Normally in WebKit code we do not write "> 0" for cases like this. It would
just be "textLength() &&".

> +	   alert(div.innerHTML);

Typically we prefer to put the results of the test into an element rather than
using alerts. This makes it smoother to run the test in the web browser.

You can set the textContent of some other element to the innerHTML of this
element.

If you are doing the alert to trigger the crash, then there's a chance the
layout will not be triggered and the won't happen inside DumpRenderTree, since
alert may not force layout. Did you check that the test does indeed crash in
DumpRenderTree without the patch? If it does not, you may want to do something
else to trigger layout.

review- only because of the tabs -- the patch is otherwise OK, but please
consider my comments for possible refinements too