[webkit-reviews] review granted: [Bug 17594] queryCommandState returns false for Underline command when no selection is made : [Attachment 67935] Patch

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has granted Ryosuke Niwa <rniwa at webkit.org>'s
request for review:
Bug 17594: queryCommandState returns false for Underline command when no
selection is made
https://bugs.webkit.org/show_bug.cgi?id=17594

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

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

This change seems OK, and I am going to say r=me. But I am concerned that we
are missing some edge case. It’s great that this fixes underlining, but I am
not sure that merging two style objects captures the same behavior that
inheritance does. Adding only one test case worries me. I suspect this will
cause regressions.

> WebCore/ChangeLog:13
> +	   Fixed the bug by making selectionComputedStyle directly marge the
computed style of the current

Typo here: "marge".

> WebCore/editing/ApplyStyleCommand.cpp:391
> -RefPtr<CSSMutableStyleDeclaration>
getPropertiesNotInComputedStyle(CSSStyleDeclaration* style,
CSSComputedStyleDeclaration* computedStyle)
> +RefPtr<CSSMutableStyleDeclaration> getPropertiesNotIn(CSSStyleDeclaration*
style, CSSStyleDeclaration* computedStyle)

If this argument is allowed to be something other than a computed styled, then
computedStyle seems like a bad name for the argument.

> WebCore/editing/ApplyStyleCommand.h:129
> -RefPtr<CSSMutableStyleDeclaration>
getPropertiesNotInComputedStyle(CSSStyleDeclaration* style,
CSSComputedStyleDeclaration* computedStyle);
> +RefPtr<CSSMutableStyleDeclaration> getPropertiesNotIn(CSSStyleDeclaration*
style, CSSStyleDeclaration* computedStyle);

Same comment about the computed style argument name.

The function is now easy to use wrong because both arguments have the same
type.

> WebCore/editing/Editor.cpp:865
> -static TriState triStateOfStyleInComputedStyle(CSSStyleDeclaration*
desiredStyle, CSSComputedStyleDeclaration* computedStyle, bool
ignoreTextOnlyProperties = false)
> +static TriState triStateOfStyleInComputedStyle(CSSStyleDeclaration*
desiredStyle, CSSStyleDeclaration* computedStyle, bool ignoreTextOnlyProperties
= false)

Same comment about the computed style argument name. But also, it’s not
appropriate to have ComputedStyle in the function name given the change.

> WebCore/editing/Editor.cpp:3264
> +    mutableStyle->merge(typingStyle.get());

Is a merge function call here really the same thing as inheriting style? I
assume the old code must have handled at least one case differently from
calling merge.