Bug 65833

Created attachment 103188 [details] fixes the bug

Comment on attachment 103188 [details] fixes the bug Oops, it appears that I've messed up a boolean expression.

Created attachment 103197 [details] Rebaselined 2 more tests

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

Created attachment 103198 [details] rebaselined 4 more tests

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

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

Created attachment 103272 [details] Updated per review

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