[webkit-reviews] review denied: [Bug 65833] Remove redundant inline styles from the pasted contents more aggressively : [Attachment 103198] rebaselined 4 more tests

[bugzilla-daemon at webkit.org]

Tony Chang <tony at chromium.org> has denied Ryosuke Niwa <rniwa at webkit.org>'s
request for review:
Bug 65833: Remove redundant inline styles from the pasted contents more
aggressively
https://bugs.webkit.org/show_bug.cgi?id=65833

Attachment 103198: rebaselined 4 more tests
https://bugs.webkit.org/attachment.cgi?id=103198&action=review

------- Additional Comments from Tony Chang <tony at chromium.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=103198&action=review


Looks fine, just had some questions and style nits.

> Source/WebCore/editing/ApplyStyleCommand.cpp:78
> +    NamedNodeMap* map = element->attributes(true); // true for read-only

Nit: I have a slight preference for |bool readOnly = true;| and passing that in
as a param, but the comment is fine too.

> Source/WebCore/editing/ApplyStyleCommand.cpp:88
> +    return matchedAttributes >= map->length();

Would == be sufficient?

> Source/WebCore/editing/EditingStyle.cpp:894
> +    if (!m_mutableStyle || !m_mutableStyle->length())

Nit: The !m_mutableStyle check seems to duplicated with the calling code. 
Maybe just move the length() check to the caller and remove the checks in
removePropertiesInElementDefaultStyle()?

> Source/WebCore/editing/ReplaceSelectionCommand.cpp:-482
> -	   // FIXME: <rdar://problem/5371536> Style rules that match pasted
content can change it's appearance

Is this no longer true?

> Source/WebCore/editing/ReplaceSelectionCommand.cpp:483
> +    for (RefPtr<Node> node = m_firstNodeInserted.get(); node && node !=
pastEndNode; node = next) {

We now stop on pastEndNode.  Why didn't we do that before?  This seems more
correct; does the new test test this change?

> Source/WebCore/editing/ReplaceSelectionCommand.cpp:512
> +	   if (node && isStyleSpan(element)) {

It looks like node is always non-NULL here.  If so, it seems like you can get
rid of |next| and put in as the third clause of the for loop.