[webkit-reviews] review requested: [Bug 28245] findNextLineBreak crashes when inserting a blank span in front of a long line : [Attachment 45466] Updated fix - removed alerts in test case, minor recommended code change

[bugzilla-daemon at webkit.org]

Mike Moretti <webkit9 at mordent.com> has asked  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 45466: Updated fix - removed alerts in test case, minor recommended
code change
https://bugs.webkit.org/attachment.cgi?id=45466&action=review

------- Additional Comments from Mike Moretti <webkit9 at mordent.com>
Sorry for any miscommunication/misunderstanding that may have occurred.  I was
working under the assumption that the sole reason the patch was rejected was
for the tabs in the test case.	I had considered your suggestions as you
requested, but did not know they were required to commit the fix.  Considering
that the bug was actually fixed, and the review comment was that it was an OK
fix, I chose to move on to fix another bug rather than spend an extra day or so
recoding the fix and testing it again, but keeping your comments in mind for
future fixes.

After the current review rejection, I went back into the code and tried to
implement the text()[] recommendation.	However, unfortunately, it does'nt
compile, giving an error about a non-existent == operator for
...->text()[lBreak.pos].  I am not sure I can implement this recommendation
without spending much more time trying to research it.

As for the test case without alerts, as was mentioned previously, there were at
least 3 completely different test cases I had originally looked at and was not
sure which to use to base my test on.  After your suggestion, rather than going
and randomly picking another test case that had no alerts but may still not be
a good base, I was hoping you could point me to one that was a good example to
use for all my future test cases so it would save review time later.

I've updated the test case to remove alerts as per your recommendation, and
have removed the > 0.  However, I have not implemented the recommended text()[]
fix as I do not currently have the knowledge to get it working.