[webkit-reviews] review granted: [Bug 13490] Implement execCommand("styleWithCSS", ...) : [Attachment 27285] patch

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has granted Justin Garcia
<justin.garcia at apple.com>'s request for review:
Bug 13490: Implement execCommand("styleWithCSS", ...)
https://bugs.webkit.org/show_bug.cgi?id=13490

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

------- Additional Comments from Darin Adler <darin at apple.com>
> +    if (value != "false" && value != "true")
> +	   return false;

Would be good to have test cases that involve strings like "0" and "1" and
"undefined" to check how this compares with other browsers' behavior.

> +    void setStyleWithCSS(bool flag) { m_styleWithCSS = flag; }
> +    bool styleWithCSS() const { return m_styleWithCSS; }

Maybe add "should" to this name?

> +    if (position.node())
> +	   if (Document* document = position.node()->document())
> +	       m_usesLegacyFormattingTags =
!document->frame()->editor()->styleWithCSS();

Should have braces around this.

> -StyleChange::ELegacyHTMLStyles StyleChange::styleModeForParseMode(bool
isQuirksMode)
> -{
> -    return isQuirksMode ? UseLegacyHTMLStyles : DoNotUseLegacyHTMLStyles;
> -}

Do we want to preserve the non-quirks mode rule here? If so, then the style
with CSS flag should default to true for non-quirks documents. We should at
least have a test case for this.

I'm going to say r=me for now, but it seems this is under-tested.