[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
bugzilla-daemon at webkit.org
Mon Aug 8 11:09:43 PDT 2011
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.
More information about the webkit-reviews
mailing list