[webkit-reviews] review denied: [Bug 45568] WebKit nests font element when applying different font styles : [Attachment 67706] new patch

[bugzilla-daemon at webkit.org]

Tony Chang <tony at chromium.org> has denied Ryosuke Niwa <rniwa at webkit.org>'s
request for review:
Bug 45568: WebKit nests font element when applying different font styles
https://bugs.webkit.org/show_bug.cgi?id=45568

Attachment 67706: new patch
https://bugs.webkit.org/attachment.cgi?id=67706&action=review

------- Additional Comments from Tony Chang <tony at chromium.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=67706&action=review

In general, looks fine, just need some clarification.

> WebCore/editing/ApplyStyleCommand.cpp:1162
> +    if (!needToApplyStyle)
> +	   return false;

What happens if we don't return early?	Do we end up with a redundant tag?

> WebCore/editing/ApplyStyleCommand.cpp:1192
> +    if (!element->parentNode() ||
!element->parentNode()->isContentEditable())
> +	   mode = RemoveNone;

This seems strange to me.  Why would we change the passed in mode?  Should the
method just return false instead or should this check be done by the caller?

> WebCore/editing/ApplyStyleCommand.cpp:1879
> +	   Node* container = fontContainer ? fontContainer : startNode;

Isn't fontContainer always null here?