[webkit-reviews] review granted: [Bug 55349] WebKit does not merge text decorations in the typing style and the selected element properly : [Attachment 84201] fixes the bug

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has granted Ryosuke Niwa <rniwa at webkit.org>'s
request for review:
Bug 55349: WebKit does not merge text decorations in the typing style and the
selected element properly
https://bugs.webkit.org/show_bug.cgi?id=55349

Attachment 84201: fixes the bug
https://bugs.webkit.org/attachment.cgi?id=84201&action=review

------- Additional Comments from Darin Adler <darin at apple.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=84201&action=review

> Source/WebCore/editing/ApplyStyleCommand.cpp:1284
> +    if (node->isHTMLElement() &&
static_cast<HTMLElement*>(node)->inlineStyleDecl()) {
> +	   newInlineStyle = style->copy();
> +	  
newInlineStyle->mergeInlineStyleOfElement(static_cast<HTMLElement*>(node));
>      }

Why is this code specific to HTMLElement? Can’t this be done on a custom
element that is technically not an HTML element? This code would treat
<subsection>, for example, differently from <section>. I am not sure I
understand why we have so much code specific to HTMLElement.

> Source/WebCore/editing/EditingStyle.cpp:714
> +	   bool shouldAddTextDecorations = false;
> +
> +	   RefPtr<CSSValue> value;

I think it would be clearer to combine these two local variables. The fact that
“value” is specifically a text decorations value is unclear given the current
name, and I think that the value being a null pointer would be just as good an
indicator as the boolean if it had a good name.

Or maybe not.