[webkit-reviews] review granted: [Bug 39989] Applying inline style to a text node whose parent is an inline editable root causes crash : [Attachment 62586] simple fix with large test

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has granted Ryosuke Niwa <rniwa at webkit.org>'s
request for review:
Bug 39989: Applying inline style to a text node whose parent is an inline
editable root causes crash
https://bugs.webkit.org/show_bug.cgi?id=39989

Attachment 62586: simple fix with large test
https://bugs.webkit.org/attachment.cgi?id=62586&action=review

------- Additional Comments from Darin Adler <darin at apple.com>
> +    Node* parent = start.node()->parentNode();
> +    if (parent && parent->parentElement() &&
parent->parentElement()->isContentEditable()) {
> +	   int endOffsetAdjustment = start.node() == end.node() ?
start.deprecatedEditingOffset() : 0;
> +	   Text* text = static_cast<Text*>(start.node());
> +	   splitTextNodeContainingElement(text,
start.deprecatedEditingOffset());
> +	   updateStartEnd(Position(start.node()->parentNode(),
start.node()->nodeIndex()), Position(end.node(), end.deprecatedEditingOffset()
- endOffsetAdjustment));
> +    } else {
> +	   splitTextAtStart(start, end);
> +    }

In WebKit we often take the "early exit" approach, checking for an unusual case
and handling that case, then using a return. The non-unusual case then
continues without be indented inside an if statement. Doing that here would
have two benefits besides the usual ones:

    1) The diff would be smaller because we wouldn't be re-indenting all the
code.
    2) The body of the if statement would be two lines so you could use braces
around it, unlike the one line else body.

It’s not WebKit style to have braces around a 1-line else body, but such things
are pretty ugly without the braces, so I think it would be nice to avoid it.

Patch otherwise looks fine, but I think it would be better with early exit.