Introduce VisiblePosition::isValid

In general, functions taking VisiblePosition parameters must be called
in clean layout and before any DOM mutation. This patch introduces
VisiblePosition::isValid to check this.

This patch is a preparation for pruning createVisiblePositionDeprecated.

BUG=647219

Review-Url: https://codereview.chromium.org/2354893002
Cr-Commit-Position: refs/heads/master@{#420024}

third_party/WebKit/Source/core/editing/FrameSelection.cpp

@@ -179,10 +179,8 @@ void FrameSelection::moveTo(const Position &pos, TextAffinity affinity)
 template <typename Strategy>
 static void adjustEndpointsAtBidiBoundary(VisiblePositionTemplate<Strategy>& visibleBase, VisiblePositionTemplate<Strategy>& visibleExtent)
 {
-    // TODO(xiaochengh): Replace it with |DCHECK(visibleBase.isValid())| and
-    // |DCHECK(visibleExtent.isValid())| once |VisiblePosition::isValid| is
-    // implemented.
-    DCHECK(visibleBase.isNull() || !visibleBase.deepEquivalent().document()->needsLayoutTreeUpdate());
+    DCHECK(visibleBase.isValid());
+    DCHECK(visibleExtent.isValid());
 
     RenderedPosition base(visibleBase);
     RenderedPosition extent(visibleExtent);

third_party/WebKit/Source/core/editing/VisiblePosition.cpp

@@ -45,12 +45,20 @@ using namespace HTMLNames;
 
 template <typename Strategy>
 VisiblePositionTemplate<Strategy>::VisiblePositionTemplate()
+#if DCHECK_IS_ON()
+    : m_domTreeVersion(0)
+    , m_styleVersion(0)
+#endif
 {
 }
 
 template <typename Strategy>
 VisiblePositionTemplate<Strategy>::VisiblePositionTemplate(const PositionWithAffinityTemplate<Strategy>& positionWithAffinity)
     : m_positionWithAffinity(positionWithAffinity)
+#if DCHECK_IS_ON()
+    , m_domTreeVersion(positionWithAffinity.position().document()->domTreeVersion())
+    , m_styleVersion(positionWithAffinity.position().document()->styleVersion())
+#endif
 {
 }
 
@@ -214,6 +222,19 @@ void VisiblePositionTemplate<Strategy>::showTreeForThis() const
 
 #endif
 
+template <typename Strategy>
+bool VisiblePositionTemplate<Strategy>::isValid() const
+{
+#if DCHECK_IS_ON()
+    if (isNull())
+        return true;
+    Document& document = *m_positionWithAffinity.position().document();
+    return m_domTreeVersion == document.domTreeVersion() && m_styleVersion == document.styleVersion() && !document.needsLayoutTreeUpdate();
+#else
+    return true;
+#endif
+}
+
 template class CORE_TEMPLATE_EXPORT VisiblePositionTemplate<EditingStrategy>;
 template class CORE_TEMPLATE_EXPORT VisiblePositionTemplate<EditingInFlatTreeStrategy>;
 

third_party/WebKit/Source/core/editing/VisiblePosition.h

@@ -82,6 +82,11 @@ public:
     bool operator==(const VisiblePositionTemplate&) const = delete;
     bool operator!=(const VisiblePositionTemplate&) const = delete;
 
+    bool isValid() const;
+
+    // TODO(xiaochengh): We should have |DCHECK(isValid())| in the following
+    // functions. However, there are some clients storing a VisiblePosition and
+    // inspecting its properties after mutation. This should be fixed.
     bool isNull() const { return m_positionWithAffinity.isNull(); }
     bool isNotNull() const { return m_positionWithAffinity.isNotNull(); }
     bool isOrphan() const { return deepEquivalent().isOrphan(); }
@@ -111,6 +116,11 @@ private:
     explicit VisiblePositionTemplate(const PositionWithAffinityTemplate<Strategy>&);
 
     PositionWithAffinityTemplate<Strategy> m_positionWithAffinity;
+
+#if DCHECK_IS_ON()
+    uint64_t m_domTreeVersion;
+    uint64_t m_styleVersion;
+#endif
 };
 
 extern template class CORE_EXTERN_TEMPLATE_EXPORT VisiblePositionTemplate<EditingStrategy>;

third_party/WebKit/Source/core/editing/VisiblePositionTest.cpp

@@ -4,6 +4,7 @@
 
 #include "core/editing/VisiblePosition.h"
 
+#include "core/css/CSSStyleDeclaration.h"
 #include "core/editing/EditingTestBase.h"
 #include "core/editing/VisibleUnits.h"
 
@@ -36,4 +37,68 @@ TEST_F(VisiblePositionTest, ShadowV0DistributedNodes)
     EXPECT_EQ(PositionInFlatTree(four->firstChild(), 2), createVisiblePosition(PositionInFlatTree(two, 0)).deepEquivalent());
 }
 
+#if DCHECK_IS_ON()
+
+TEST_F(VisiblePositionTest, NullIsValid)
+{
+    EXPECT_TRUE(VisiblePosition().isValid());
+}
+
+TEST_F(VisiblePositionTest, NonNullIsValidBeforeMutation)
+{
+    setBodyContent("<p>one</p>");
+
+    Element* paragraph = document().querySelector("p");
+    Position position(paragraph->firstChild(), 1);
+    EXPECT_TRUE(createVisiblePosition(position).isValid());
+}
+
+TEST_F(VisiblePositionTest, NonNullInvalidatedAfterDOMChange)
+{
+    setBodyContent("<p>one</p>");
+
+    Element* paragraph = document().querySelector("p");
+    Position position(paragraph->firstChild(), 1);
+    VisiblePosition nullVisiblePosition;
+    VisiblePosition nonNullVisiblePosition = createVisiblePosition(position);
+
+    Element* div = document().createElement("div", ASSERT_NO_EXCEPTION);
+    document().body()->appendChild(div);
+
+    EXPECT_TRUE(nullVisiblePosition.isValid());
+    EXPECT_FALSE(nonNullVisiblePosition.isValid());
+
+    updateAllLifecyclePhases();
+
+    // Invalid VisiblePosition can never become valid again.
+    EXPECT_FALSE(nonNullVisiblePosition.isValid());
+}
+
+TEST_F(VisiblePositionTest, NonNullInvalidatedAfterStyleChange)
+{
+    setBodyContent("<div>one</div><p>two</p>");
+
+    Element* paragraph = document().querySelector("p");
+    Element* div = document().querySelector("div");
+    Position position(paragraph->firstChild(), 1);
+
+    VisiblePosition visiblePosition1 = createVisiblePosition(position);
+    div->style()->setProperty("color", "red", "important", ASSERT_NO_EXCEPTION);
+    EXPECT_FALSE(visiblePosition1.isValid());
+
+    updateAllLifecyclePhases();
+
+    VisiblePosition visiblePosition2 = createVisiblePosition(position);
+    div->style()->setProperty("display", "none", "important", ASSERT_NO_EXCEPTION);
+    EXPECT_FALSE(visiblePosition2.isValid());
+
+    updateAllLifecyclePhases();
+
+    // Invalid VisiblePosition can never become valid again.
+    EXPECT_FALSE(visiblePosition1.isValid());
+    EXPECT_FALSE(visiblePosition2.isValid());
+}
+
+#endif
+
 } // namespace blink

third_party/WebKit/Source/core/editing/VisibleUnits.cpp

@@ -1337,14 +1337,14 @@ static inline LayoutPoint absoluteLineDirectionPointToLocalPointInBlock(RootInli
 
 VisiblePosition previousLinePosition(const VisiblePosition& visiblePosition, LayoutUnit lineDirectionPoint, EditableType editableType)
 {
+    DCHECK(visiblePosition.isValid());
+
     Position p = visiblePosition.deepEquivalent();
     Node* node = p.anchorNode();
 
     if (!node)
         return VisiblePosition();
 
-    DCHECK(!node->document().needsLayoutTreeUpdate());
-
     LayoutObject* layoutObject = node->layoutObject();
     if (!layoutObject)
         return VisiblePosition();
@@ -1390,14 +1390,14 @@ VisiblePosition previousLinePosition(const VisiblePosition& visiblePosition, Lay
 
 VisiblePosition nextLinePosition(const VisiblePosition& visiblePosition, LayoutUnit lineDirectionPoint, EditableType editableType)
 {
+    DCHECK(visiblePosition.isValid());
+
     Position p = visiblePosition.deepEquivalent();
     Node* node = p.anchorNode();
 
     if (!node)
         return VisiblePosition();
 
-    DCHECK(!node->document().needsLayoutTreeUpdate());
-
     LayoutObject* layoutObject = node->layoutObject();
     if (!layoutObject)
         return VisiblePosition();