[webkit-reviews] review denied: [Bug 28245] findNextLineBreak crashes when inserting a blank span in front of a long line : [Attachment 45431] Bug fix patch - Removed tabs

[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 45431: Bug fix patch - Removed tabs
https://bugs.webkit.org/attachment.cgi?id=45431&action=review

------- Additional Comments from Darin Adler <darin at apple.com>
(In reply to comment #5)
> As for the indexing, I didn't want to use indexing because "characters"
> actually returns null, not a valid string (I don't know what "text()" does
yet
> without looking further).

I know that you didn't know that!

But text() returns a StringImpl* that does indeed handle the [0] case, as I
said.

> I also wasn't sure if textLength() would ever return a -1

It would not.

It's OK not to know these things when you first wrote the patch. You can learn
them during patch review.

What's not as OK is to just leave the patch as-is without responding to the
comments. I can understand you not changing if you don't agree, but just saying
"I didn't know when I originally wrote this" doesn't justify leaving the patch
as is.

> I looked at 3 or 4 test cases to find one to use as an example but wasn't
sure
> which was best, so I randomly picked one and it had alerts in it.  If you can

> point me to a better example that would be great!

Your attitude is fine, but your difficulty finding an example without alerts is
strange. I started looking in the fast/dom directory alphabetically right where
your new test would go and found:

    inner-text.html
    inner-width-height.html

I can provide many other examples that don't use alert.

> I would rather write a proper test case next time.

Please do it this time, lets not wait for next time.