[webkit-reviews] review denied: [Bug 27156] SplitElementCommand shouldn't be duplicating id attribute : [Attachment 62856] fixed style per darin's comment

[bugzilla-daemon at webkit.org]

Ojan Vafai <ojan at chromium.org> has denied Ryosuke Niwa <rniwa at webkit.org>'s
request for review:
Bug 27156: SplitElementCommand shouldn't be duplicating id attribute
https://bugs.webkit.org/show_bug.cgi?id=27156

Attachment 62856: fixed style per darin's comment
https://bugs.webkit.org/attachment.cgi?id=62856&action=review

------- Additional Comments from Ojan Vafai <ojan at chromium.org>
WebCore/editing/ApplyStyleCommand.cpp:1185
 +  bool
ApplyStyleCommand::removeInlineStyleFromElement(CSSMutableStyleDeclaration*
style, HTMLElement* element, bool dontRemove)
Generally, don't name variable names with negatives in them. invert this and
call it shouldRemove instead. Also, Darin will kill me if I let you add a
boolean argument. Use an enum instead. That way, the meaning of the argument is
clear from the calling context.

> +    // If the node was converted to a span, the span may still contain
relevant
> +    // styles which must be removed (e.g. <b style='font-weight: bold'>)
> +    if (removeHTMLFontStyle(style, element, dontRemove))
> +	   removed = true;
> +    if (removeHTMLBidiEmbeddingStyle(style, element, dontRemove))
> +	   removed = true;
> +    if (removeCSSStyle(style, element, dontRemove))
> +	   removed = true;

This can be a single if-statement:
if (removeHTMLFontStyle(style, element, dontRemove)
    || removeHTMLBidiEmbeddingStyle(style, element, dontRemove)
    || removeCSSStyle(style, element, dontRemove))
    removed = true;

> -    if (isEmptyFontTag(elem))
> +    if (isEmptyFontTag(elem) && !dontRemove)
>	   removeNodePreservingChildren(elem);

Shouldn't we set removed=true here?

>      // FIXME: should this be isSpanWithoutAttributesOrUnstyleStyleSpan? 
Need a test.
>      if (isUnstyledStyleSpan(elem))
>	   removeNodePreservingChildren(elem);

Ditto.

> -	   splitTextElementAtStart(start, end);
> +	   if (shouldSplitElement(start.node()->parentElement(), style))
> +	       splitTextElementAtStart(start, end);
> +	   else
> +	       splitTextAtStart(start, end);

> -	   splitTextElementAtEnd(start, end);
> +	   if (shouldSplitElement(end.node()->parentElement(), style))
> +	       splitTextElementAtEnd(start, end);
> +	   else
> +	       splitTextAtEnd(start, end);

Maybe the method should be called shouldSplitTextElement to match
splitTextElementAt*?