[Webkit-unassigned] [Bug 36683] For compatibility, execCommand should support deprecated 'useCSS' alias for 'styleWithCSS'

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=36683


Darin Adler <darin at apple.com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
 Attachment #105916|review?                     |review-
               Flag|                            |




--- Comment #9 from Darin Adler <darin at apple.com>  2011-09-01 08:43:49 PST ---
(From update of attachment 105916)
View in context: https://bugs.webkit.org/attachment.cgi?id=105916&action=review

Thanks for contributing a patch.

> Source/WebCore/ChangeLog:8
> +        No new tests. (OOPS!)

We require tests for patches that fix bugs like this one.

> Source/WebCore/editing/EditorCommand.cpp:1020
> +    const String lowercasedValue = value.lower();

The right way to do a case-insensitive comparison is to call equalIgnoringCase, not to call lower on the string. 

Since this patch is fixing a bug where we were case-sensitive before and now need to ignore case:

    1) The change log or bug title needs to mention that.
    2) The patch needs to include tests covering the correct behavior.

> Source/WebCore/editing/EditorCommand.cpp:1024
> +    frame->editor()->setShouldStyleWithCSS(lowercasedValue == "true" ? true : false);

The use of "? true : false" here is not good style. That’s a no-op and should be omitted.

This code should be written so it does two string comparisons, rather than repeating one a second time.

> Source/WebCore/editing/EditorCommand.cpp:1034
> +    frame->editor()->setShouldStyleWithCSS(lowercasedValue == "false" ? true : false);

A good way to write this is !equalIgnoringCase(value, "false"). That use of ? : is not helpful.

> Source/WebCore/editing/EditorCommand.cpp:1537
>          { "StyleWithCSS", { executeStyleWithCSS, supported, enabled, stateStyleWithCSS, valueNull, notTextInsertion, doNotAllowExecutionWhenDisabled } },
> +        { "UseCSS", { executeUseCSS, supported, enabled, stateStyleWithCSS, valueNull, notTextInsertion, doNotAllowExecutionWhenDisabled } },
>          { "Subscript", { executeSubscript, supported, enabledInRichlyEditableText, stateSubscript, valueNull, notTextInsertion, doNotAllowExecutionWhenDisabled } },

This list is alphabetical, so UseCSS should be put in the correct alphabetical location.

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.