Remove redundant inline styles from the pasted contents more aggressively

• Component: HTML Editing
• Status: RESOLVED FIXED

[Ryosuke Niwa]

This is the first half of my patch for the bug 34564.  This patch will improve the code in ReplaceSelectionCommand::removeRedundantStylesAndKeepStyleSpanInline to more aggressively remove redundant style spans and reduce their attributes.

[Ryosuke Niwa]

Created attachment 103188 [details]
fixes the bug

[Ryosuke Niwa]

Comment on attachment 103188 [details]
fixes the bug

Oops, it appears that I've messed up a boolean expression.

[Ryosuke Niwa]

Created attachment 103197 [details]
Rebaselined 2 more tests

[Ryosuke Niwa]

(In reply to comment #3)
> Created an attachment (id=103197) [details]
> Rebaselined 2 more tests

Unfortunately, we have to rebaseline 4 more tests after bugs 65836 and 65837 are fixed.

[Ryosuke Niwa]

Created attachment 103198 [details]
rebaselined 4 more tests

[Tony Chang]

Comment on attachment 103198 [details]
rebaselined 4 more tests

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.

[Ryosuke Niwa]

Comment on attachment 103198 [details]
rebaselined 4 more tests

View in context: https://bugs.webkit.org/attachment.cgi?id=103198&action=review

Thanks for the review.

>> 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.

Okay, will do that.

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

I'll assert that matchedAttributes <= map->length() so that I can use equality here.

>> 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()?

This function is to become public and used in markup.cpp so I'd like to keep these checks or at least !m_mutableStyle->length()

>> 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?

Huh, I guess I should keep that comment.

>> 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?

We always did.  See lines 506-507 below that have been removed.  I'm simply moving the ending condition here.

>> Source/WebCore/editing/ReplaceSelectionCommand.cpp:512

> 
> 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.

No, node can be detached from the document (see removeNodePreservingChildren on line 506) in which case node-> traverseNextNode() will be 0.  The only reason I check node is to make sure we don't dereference stale element pointer.

[Tony Chang]

(In reply to comment #7)
> (From update of attachment 103198 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=103198&action=review
> >> Source/WebCore/editing/ReplaceSelectionCommand.cpp:512
> 
> > 
> > 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.
> 
> No, node can be detached from the document (see removeNodePreservingChildren on line 506) in which case node-> traverseNextNode() will be 0.  The only reason I check node is to make sure we don't dereference stale element pointer.

I see.  That explains why we have |next|, but shouldn't the node always be non-NULL even when detached?  It's a refptr.

[Ryosuke Niwa]

Comment on attachment 103198 [details]
rebaselined 4 more tests

View in context: https://bugs.webkit.org/attachment.cgi?id=103198&action=review

>>>> 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.
>> 
>> 
> 
> I see.  That explains why we have |next|, but shouldn't the node always be non-NULL even when detached?  It's a refptr.

That's a good point actually.  Will remove the check.

[Ryosuke Niwa]

Created attachment 103272 [details]
Updated per review

[Ryosuke Niwa]

Committed r92620: <http://trac.webkit.org/changeset/92620>