Bug 65833 - Remove redundant inline styles from the pasted contents more aggressively
Summary: Remove redundant inline styles from the pasted contents more aggressively
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: HTML Editing (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Ryosuke Niwa
URL:
Keywords:
Depends on: 65836 65837
Blocks: 34564
  Show dependency treegraph
 
Reported: 2011-08-07 15:54 PDT by Ryosuke Niwa
Modified: 2011-08-08 12:15 PDT (History)
6 users (show)

See Also:


Attachments
fixes the bug (17.21 KB, patch)
2011-08-07 16:09 PDT, Ryosuke Niwa
no flags Details | Formatted Diff | Diff
Rebaselined 2 more tests (18.79 KB, patch)
2011-08-07 19:17 PDT, Ryosuke Niwa
no flags Details | Formatted Diff | Diff
rebaselined 4 more tests (23.52 KB, patch)
2011-08-07 21:34 PDT, Ryosuke Niwa
no flags Details | Formatted Diff | Diff
Updated per review (23.51 KB, patch)
2011-08-08 12:02 PDT, Ryosuke Niwa
tony: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Ryosuke Niwa 2011-08-07 15:54:45 PDT
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.
Comment 1 Ryosuke Niwa 2011-08-07 16:09:55 PDT
Created attachment 103188 [details]
fixes the bug
Comment 2 Ryosuke Niwa 2011-08-07 17:20:35 PDT
Comment on attachment 103188 [details]
fixes the bug

Oops, it appears that I've messed up a boolean expression.
Comment 3 Ryosuke Niwa 2011-08-07 19:17:15 PDT
Created attachment 103197 [details]
Rebaselined 2 more tests
Comment 4 Ryosuke Niwa 2011-08-07 19:21:22 PDT
(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.
Comment 5 Ryosuke Niwa 2011-08-07 21:34:48 PDT
Created attachment 103198 [details]
rebaselined 4 more tests
Comment 6 Tony Chang 2011-08-08 11:09:43 PDT
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.
Comment 7 Ryosuke Niwa 2011-08-08 11:23:29 PDT
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.
Comment 8 Tony Chang 2011-08-08 11:28:19 PDT
(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.
Comment 9 Ryosuke Niwa 2011-08-08 11:33:49 PDT
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.
Comment 10 Ryosuke Niwa 2011-08-08 12:02:54 PDT
Created attachment 103272 [details]
Updated per review
Comment 11 Ryosuke Niwa 2011-08-08 12:15:24 PDT
Committed r92620: <http://trac.webkit.org/changeset/92620>