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

bugzilla-daemon at webkit.org bugzilla-daemon at webkit.org
Thu Jun 10 17:16:45 PDT 2010


--- Comment #21 from Ojan Vafai <ojan at chromium.org>  2010-06-10 17:16:43 PST ---
> Ojan, I can't tell if you're reviewing the patches since you're not changing the review flag.

It's not clear to me whether this patch is incorrect or correct, so I don't think it makes sense for me to r-/r+. Leaving it r? means another reviewer can come in and review. In the meantime, I'm trying to understand the code in question well enough to give an r-/r+.

> Inline comments on that patch are more helpful than just responding to the comments.

Not sure what you're asking for here. The code hasn't changed, so there's only comments to respond to.

> > Before this patch, pos pointed to the deepEquivalent (canonicalized node). After this patch, it just points to m_start, which may or may not be canonicalized. Right?
> Sure, but what canonicalization needs to happen?  I.e., do you have an example test case that this breaks?  Since this is just plain text, I'm not sure we need to canonicalize here.

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.

If this code gets hit in rich-text, I don't have a clear example in my head where this would break. I think it would break if selection.start() is ever not canonicalized when it the selection gets set. Not sure if that's possible.

By "break" here, I just mean that it would do something different than the old code.

> Anyway, shouldn't this be discussed on a different bug?

Sure, that discussion can happen on bug 40342.

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

More information about the webkit-unassigned mailing list