[webkit-reviews] review denied: [Bug 27476] execCommand('underline' / 'strikeThrough') doesn't work properly with multiple styles in text-decoration : [Attachment 33278] fixes the bug

[bugzilla-daemon at webkit.org]

Eric Seidel <eric at webkit.org> has denied Ryosuke Niwa <rniwa at webkit.org>'s
request for review:
Bug 27476: execCommand('underline' / 'strikeThrough') doesn't work properly
with multiple styles in text-decoration
https://bugs.webkit.org/show_bug.cgi?id=27476

Attachment 33278: fixes the bug
https://bugs.webkit.org/attachment.cgi?id=33278&action=review

------- Additional Comments from Eric Seidel <eric at webkit.org>
/CSSComputedStyleDeclaration.cpp change needs testing.
window.getComputedStyle(foo).getPropertyCSSValue('text-decoration')


   RefPtr<CSSValueList> list = CSSValueList::createCommaSeparated();
 1055		  RefPtr<CSSValueList> list =
CSSValueList::createSpaceSeparated();
needs test too.
foo.style.textDecoration

 73 void CSSValueList::remove(CSSValue* val)
should be called removeFirst or removeAll or should at least be documented as
to its behavior.

 78	    if (m_values.at(index)->cssText() == val->cssText())

is really really sad.

consider adding a bool return to remove and then you don't need hasValue

 93 static bool applyCommandToFrame(Frame* frame, EditorCommandSource source,
EditAction action, CSSMutableStyleDeclaration* style)
is a great change and could even be made in a separate patch.  it's OK to do in
this one though.

YOu can just delete:
110116 static bool executeApplyStyle(Frame* frame, EditorCommandSource source,
EditAction action, int propertyID, const char* propertyValue)

Spacing:
 130	 Node* nodeToRemove=0;

This looks great!  We just need more tests!