[Webkit-unassigned] [Bug 30116] WebCore::InsertLineBreakCommand::shouldUseBreakElement ReadAV at NULL

[bugzilla-daemon at webkit.org]

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


Ojan Vafai <ojan at chromium.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |darin at apple.com




--- Comment #25 from Ojan Vafai <ojan at chromium.org>  2010-06-10 18:34:42 PST ---
(In reply to comment #24)
> (In reply to comment #23)
> > (In reply to comment #21)
> > > Is this code really only hit in plaintext regions? Doesn't non-plaintext contentEditable hit this codepath as well? If it really is just for plain text, then I think we don't need to worry about canonicalization.
> > 
> > Oh, I see, you're saying in other cases, we may need to canonicalize.  Hmm, maybe, but it's not obvious to me that it is necessary without a test case.

Right. Unfortunately, this is a problem with the editing code in general. The test coverage isn't that great. So places where we make changes like this may cause regressions without breaking tests and the canonicalization code is really difficult to reason about.

This code is not clearly wrong to me, but it does set off a red flag. I'm not sure what the correct path forward in cases like this are. My intuition is that we check this in as is and, if it causes a regression, it'll get reported and we can add a test + fix then. Adding Darin to this bug for his opinion on this.

> And if we are concerned about that, then we should just go with the original patch that null checks |caret|.  It won't insert a line, but that's bug 40342.

I'm definitely OK with adding a null-check and a FIXME pointing to bug 40342. Not inserting the line-break here is wrong IMO, but it's obviously much better than crashing and definitely won't cause regressions.

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