Get rid of false assert from CompositeEditCommand::moveParagraphContentsToNewBlockIfNecessary

Source/core/editing/CompositeEditCommand.cpp

 /*
  * Copyright (C) 2005, 2006, 2007, 2008 Apple Inc. All rights reserved.
  *
  * Redistribution and use in source and binary forms, with or without
  * modification, are permitted provided that the following conditions
  * are met:
  * 1. Redistributions of source code must retain the above copyright
  *    notice, this list of conditions and the following disclaimer.
  * 2. Redistributions in binary form must reproduce the above copyright
  *    notice, this list of conditions and the following disclaimer in the
@@ skipping 905 matching lines @@
             // block but don't try and move content into it, since there's nothing for moveParagraphs to move.
             if (!Position::hasRenderedNonAnonymousDescendantsWithHeight(upstreamStart.deprecatedNode()->renderer()))
                 return insertNewDefaultParagraphElementAt(upstreamStart);
         } else if (isBlock(upstreamEnd.deprecatedNode())) {
             if (!upstreamEnd.deprecatedNode()->isDescendantOf(upstreamStart.deprecatedNode())) {
                 // If the paragraph end is a descendant of paragraph start, then we need to run
                 // the rest of this function. If not, we can bail here.
                 return nullptr;
             }
         } else if (enclosingBlock(upstreamEnd.deprecatedNode()) != upstreamStart.deprecatedNode()) {
-            // The visibleEnd.  It must be an ancestor of the paragraph start.
-            // We can bail as we have a full block to work with.
-            ASSERT(upstreamStart.deprecatedNode()->isDescendantOf(enclosingBlock(upstreamEnd.deprecatedNode())));

[Yuta Kitamura, 2014/05/28 09:14:10]

I tried to follow the code but couldn't understand why
this deletion is safe... Could you give a bit more context,
please?

[yosin_UTC9, 2014/05/29 04:04:39]

My reasoning is rather simple. We have a case breaking this ASSERT. The author
who add this ASSERT didn't consider empty list but visible.

[Yuta Kitamura, 2014/05/29 09:00:24]

I thought about this for some time but I couldn't reach the conclusion
that this ASSERT is bogus; it looks like a legit assumption to me.

Rather, I think there's something wrong in positions creation logic in
lines 896-900, because it's weird that |upstreamStart| and |upstreamEnd|
belong to different paragraphs.

[Yuta Kitamura, 2014/05/30 04:40:45]

I started to feel like this ASSERT is indeed bogus.

So I'm fine with removing the ASSERT, but could you un-delete the portion
of the comment explaining this "if" condition? It looks useful. (I'm okay
with removing the text "The visibleEnd." which does not make any sense
to me.)

[yosin_UTC9, 2014/05/30 05:04:26]

Done.

             return nullptr;
         } else if (isEndOfEditableOrNonEditableContent(visibleEnd)) {
             // At the end of the editable region. We can bail here as well.
             return nullptr;
         }
     }
 
     RefPtr<Node> newBlock = insertNewDefaultParagraphElementAt(upstreamStart);
 
     bool endWasBr = isHTMLBRElement(*visibleParagraphEnd.deepEquivalent().deprecatedNode());
@@ skipping 531 matching lines @@
 
     return node.release();
 }
 
 PassRefPtrWillBeRawPtr<Element> createBlockPlaceholderElement(Document& document)
 {
     return document.createElement(brTag, false);
 }
 
 } // namespace WebCore