[Webkit-unassigned] [Bug 28245] findNextLineBreak crashes when inserting a blank span in front of a long line

[bugzilla-daemon at webkit.org]

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


Darin Adler <darin at apple.com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
  Attachment #45394|review?                     |review-
               Flag|                            |




--- Comment #4 from Darin Adler <darin at apple.com>  2009-12-22 12:18:57 PST ---
(From update of attachment 45394)
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

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