Bug 55349

Created attachment 84201 [details] fixes the bug

Comment on attachment 84201 [details] fixes the bug 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.

(In reply to comment #2) > (From update of attachment 84201 [details]) > 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. It's only for historical reasons. We can fix in a separate patch. Is there a particular test case you're thinking of? It'll be good to have a regression test. > > 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. That's an excellent point. I'd just set value = 0 if it was value && !value->isValueList().

Thanks for the review. I'll be following up on the bug 51389 now.

Committed r80060: <http://trac.webkit.org/changeset/80060>

http://trac.webkit.org/changeset/80060 might have broken GTK Linux 64-bit Debug The following tests are not passing: fast/viewport/viewport-112.html fast/viewport/viewport-121.html fast/viewport/viewport-122.html fast/viewport/viewport-125.html fast/viewport/viewport-129.html fast/viewport/viewport-35.html fast/viewport/viewport-46.html fast/viewport/viewport-52.html fast/viewport/viewport-53.html fast/viewport/viewport-54.html fast/viewport/viewport-55.html fast/viewport/viewport-66.html fast/viewport/viewport-67.html fast/viewport/viewport-68.html fast/viewport/viewport-69.html fast/viewport/viewport-70.html fast/viewport/viewport-71.html fast/viewport/viewport-72.html fast/viewport/viewport-73.html fast/viewport/viewport-74.html fast/viewport/viewport-75.html fast/viewport/viewport-77.html fast/viewport/viewport-78.html fast/viewport/viewport-79.html fast/viewport/viewport-83.html fast/xsl/xslt-mismatched-tags-in-xslt.xml fast/xsl/xslt-missing-namespace-in-xslt.xml http/tests/security/xss-DENIED-xsl-document-redirect.xml http/tests/security/xss-DENIED-xsl-external-entity-redirect.xml http/tests/xmlviewer/dumpAsText/wml.xml

http://trac.webkit.org/changeset/80061 might have broken GTK Linux 64-bit Debug The following tests are not passing: fast/viewport/viewport-112.html fast/viewport/viewport-121.html fast/viewport/viewport-122.html fast/viewport/viewport-125.html fast/viewport/viewport-129.html fast/viewport/viewport-35.html fast/viewport/viewport-46.html fast/viewport/viewport-52.html fast/viewport/viewport-53.html fast/viewport/viewport-54.html fast/viewport/viewport-55.html fast/viewport/viewport-66.html fast/viewport/viewport-67.html fast/viewport/viewport-68.html fast/viewport/viewport-69.html fast/viewport/viewport-70.html fast/viewport/viewport-71.html fast/viewport/viewport-72.html fast/viewport/viewport-73.html fast/viewport/viewport-74.html fast/viewport/viewport-75.html fast/viewport/viewport-77.html fast/viewport/viewport-78.html fast/viewport/viewport-79.html fast/viewport/viewport-83.html fast/xsl/xslt-mismatched-tags-in-xslt.xml fast/xsl/xslt-missing-namespace-in-xslt.xml http/tests/security/xss-DENIED-xsl-document-redirect.xml http/tests/security/xss-DENIED-xsl-external-entity-redirect.xml http/tests/xmlviewer/dumpAsText/wml.xml