Changeset 259930 in webkit

Timestamp:

Apr 11, 2020 8:35:04 AM (4 years ago)

Author:

Darin Adler

Message:

Use Node::length to replace Node::maxCharacterOffset and lastOffsetInNode; switch more offsets from int to unsigned
​https://bugs.webkit.org/show_bug.cgi?id=210246

Reviewed by Antti Koivisto.

• The recently-added Node::length, which matches the DOM specification terminology for node offsets as used in ranges, is the same as the existing maxCharacterOffset and lastOffsetInNode functions. Deleted all uses of those and replaced them with calls to Node::length. One of the benefits of this is that Node::length is implemented more efficiently and is not a virtual function. Another is consistently matching the DOM specification terminology.
• Many offsets, including the ones in live ranges, are currently implemented as signed in WebKit, but are specified as unsigned in the DOM and HTML specifications. This has very little observable effect from JavaScript that can affect website compatibility, but it's still helpful to be consistent both with the specification and internally. Accordingly, changed some of these to unsigned; more to come later.

• accessibility/AXObjectCache.cpp:

(WebCore::AXObjectCache::previousBoundary): Use length instead of
maxCharacterOffset.

• dom/CharacterData.cpp:

(WebCore::CharacterData::maxCharacterOffset const): Deleted.

• dom/CharacterData.h: Deleted maxCharacterOffset override.

• dom/DocumentMarkerController.cpp:

(WebCore::DocumentMarkerController::shiftMarkers): Use length instead of
maxCharacterOffset.

• dom/Node.cpp:

(WebCore::Node::maxCharacterOffset const): Deleted.

• dom/Node.h: Deleted maxCharacterOffset.

• dom/Position.cpp:

(WebCore::Position::computeOffsetInContainerNode const): Use length instead
of lastOffsetInNode.

• dom/Position.h:

(WebCore::lastOffsetInNode): Deleted.
(WebCore::lastPositionInNode): Use length instead of lastOffsetInNode.
(WebCore::minOffsetForNode): Use length instead of maxCharacterOffset.
(WebCore::offsetIsBeforeLastNodeOffset): Ditto.

• dom/RangeBoundaryPoint.h:

(WebCore::RangeBoundaryPoint::setToEndOfNode): Use length instead of
maxCharacterOffset.

• editing/ApplyBlockElementCommand.cpp:

(WebCore::isNewLineAtPosition): Use length instead of maxCharacterOffset.
(WebCore::ApplyBlockElementCommand::rangeForParagraphSplittingTextNodesIfNeeded):
Ditto.

• editing/ApplyStyleCommand.cpp:

(WebCore::ApplyStyleCommand::removeInlineStyle): Use length instead of
maxCharacterOffset.

• editing/Editing.cpp:

(WebCore::lastOffsetForEditing): Use length instead of mmaxCharacterOffset
and countChildNodes. Also improved the comment here.

• editing/EditingStyle.cpp:

(WebCore::EditingStyle::styleAtSelectionStart): Use length instead of
maxCharacterOffset.

• editing/InsertListCommand.cpp:

(WebCore::InsertListCommand::doApplyForSingleParagraph): Use length instead
of lastOffsetInNode.

• editing/TextIterator.cpp:

(WebCore::SimplifiedBackwardsTextIterator::SimplifiedBackwardsTextIterator):
USe length instead of lastOffsetInNode.

• editing/VisibleUnits.cpp:

(WebCore::previousBoundary): Use length instead of maxCharacterOffset.

Location:

trunk/Source/WebCore

Files:

• ChangeLog (modified) (1 diff)
• accessibility/AXObjectCache.cpp (modified) (1 diff)
• accessibility/atk/WebKitAccessibleUtil.cpp (modified) (1 diff)
• dom/CharacterData.cpp (modified) (1 diff)
• dom/CharacterData.h (modified) (1 diff)
• dom/DocumentMarkerController.cpp (modified) (2 diffs)
• dom/Node.cpp (modified) (1 diff)
• dom/Node.h (modified) (1 diff)
• dom/Position.cpp (modified) (1 diff)
• dom/Position.h (modified) (6 diffs)
• dom/RangeBoundaryPoint.h (modified) (1 diff)
• editing/ApplyBlockElementCommand.cpp (modified) (4 diffs)
• editing/ApplyStyleCommand.cpp (modified) (1 diff)
• editing/Editing.cpp (modified) (1 diff)
• editing/EditingStyle.cpp (modified) (1 diff)
• editing/InsertListCommand.cpp (modified) (1 diff)
• editing/TextIterator.cpp (modified) (1 diff)
• editing/VisibleUnits.cpp (modified) (1 diff)
• html/HTMLTextFormControlElement.cpp (modified) (1 diff)

trunk/Source/WebCore/ChangeLog

@@ +1 @@
+2020-04-08  Darin Adler  <darin@apple.com>
+
+        Use Node::length to replace Node::maxCharacterOffset and lastOffsetInNode; switch more offsets from int to unsigned
+        https://bugs.webkit.org/show_bug.cgi?id=210246
+
+        Reviewed by Antti Koivisto.
+
+        - The recently-added Node::length, which matches the DOM specification terminology
+          for node offsets as used in ranges, is the same as the existing maxCharacterOffset
+          and lastOffsetInNode functions. Deleted all uses of those and replaced them
+          with calls to Node::length. One of the benefits of this is that Node::length is
+          implemented more efficiently and is not a virtual function. Another is consistently
+          matching the DOM specification terminology.
+        - Many offsets, including the ones in live ranges, are currently implemented as signed
+          in WebKit, but are specified as unsigned in the DOM and HTML specifications. This
+          has very little observable effect from JavaScript that can affect website compatibility,
+          but it's still helpful to be consistent both with the specification and internally.
+          Accordingly, changed some of these to unsigned; more to come later.
+
+        * accessibility/AXObjectCache.cpp:
+        (WebCore::AXObjectCache::previousBoundary): Use length instead of
+        maxCharacterOffset.
+
+        * dom/CharacterData.cpp:
+        (WebCore::CharacterData::maxCharacterOffset const): Deleted.
+        * dom/CharacterData.h: Deleted maxCharacterOffset override.
+
+        * dom/DocumentMarkerController.cpp:
+        (WebCore::DocumentMarkerController::shiftMarkers): Use length instead of
+        maxCharacterOffset.
+
+        * dom/Node.cpp:
+        (WebCore::Node::maxCharacterOffset const): Deleted.
+        * dom/Node.h: Deleted maxCharacterOffset.
+
+        * dom/Position.cpp:
+        (WebCore::Position::computeOffsetInContainerNode const): Use length instead
+        of lastOffsetInNode.
+
+        * dom/Position.h:
+        (WebCore::lastOffsetInNode): Deleted.
+        (WebCore::lastPositionInNode): Use length instead of lastOffsetInNode.
+        (WebCore::minOffsetForNode): Use length instead of maxCharacterOffset.
+        (WebCore::offsetIsBeforeLastNodeOffset): Ditto.
+
+        * dom/RangeBoundaryPoint.h:
+        (WebCore::RangeBoundaryPoint::setToEndOfNode): Use length instead of
+        maxCharacterOffset.
+
+        * editing/ApplyBlockElementCommand.cpp:
+        (WebCore::isNewLineAtPosition): Use length instead of maxCharacterOffset.
+        (WebCore::ApplyBlockElementCommand::rangeForParagraphSplittingTextNodesIfNeeded):
+        Ditto.
+
+        * editing/ApplyStyleCommand.cpp:
+        (WebCore::ApplyStyleCommand::removeInlineStyle): Use length instead of
+        maxCharacterOffset.
+
+        * editing/Editing.cpp:
+        (WebCore::lastOffsetForEditing): Use length instead of mmaxCharacterOffset
+        and countChildNodes. Also improved the comment here.
+
+        * editing/EditingStyle.cpp:
+        (WebCore::EditingStyle::styleAtSelectionStart): Use length instead of
+        maxCharacterOffset.
+
+        * editing/InsertListCommand.cpp:
+        (WebCore::InsertListCommand::doApplyForSingleParagraph): Use length instead
+        of lastOffsetInNode.
+
+        * editing/TextIterator.cpp:
+        (WebCore::SimplifiedBackwardsTextIterator::SimplifiedBackwardsTextIterator):
+        USe length instead of lastOffsetInNode.
+
+        * editing/VisibleUnits.cpp:
+        (WebCore::previousBoundary): Use length instead of maxCharacterOffset.
+
 2020-04-10  Alex Christensen  <achristensen@webkit.org>
 

trunk/Source/WebCore/accessibility/AXObjectCache.cpp

@@ -2665 +2665 @@
         return startOrEndCharacterOffsetForRange(rangeForNodeContents(nextSibling), false);
 
-    if ((!suffixLength && node.isTextNode() && static_cast<int>(next) <= node.maxCharacterOffset()) || (node.renderer() && node.renderer()->isBR() && !next)) {
+    if ((!suffixLength && node.isTextNode() && next <= node.length()) || (node.renderer() && node.renderer()->isBR() && !next)) {
         // The next variable contains a usable index into a text node
         if (node.isTextNode())

trunk/Source/WebCore/accessibility/atk/WebKitAccessibleUtil.cpp

@@ -169 +169 @@
     auto& node = *coreObject->node();
     auto* lastDescendant = node.lastDescendant();
-    unsigned lastOffset = lastOffsetInNode(lastDescendant);
+    unsigned lastOffset = lastDescendant->length();
     auto intersectsResult = range->intersectsNode(node);
     return !intersectsResult.hasException()

trunk/Source/WebCore/dom/CharacterData.cpp

@@ -240 +240 @@
 }
 
-int CharacterData::maxCharacterOffset() const
-{
-    return static_cast<int>(length());
-}
-
 } // namespace WebCore

trunk/Source/WebCore/dom/CharacterData.h

@@ -64 +64 @@
     ExceptionOr<void> setNodeValue(const String&) final;
     bool virtualIsCharacterData() const final { return true; }
-    int maxCharacterOffset() const final;
     void setDataAndUpdate(const String&, unsigned offsetOfReplacedData, unsigned oldLength, unsigned newLength);
     void notifyParentAfterChange(ContainerNode::ChildChangeSource);

trunk/Source/WebCore/dom/DocumentMarkerController.cpp

@@ -598 +598 @@
         int targetStartOffset = marker.startOffset() + delta;
         int targetEndOffset = marker.endOffset() + delta;
-        if (targetStartOffset >= node.maxCharacterOffset() || targetEndOffset <= 0) {
+        if (static_cast<unsigned>(targetStartOffset) >= node.length() || targetEndOffset <= 0) {
             list->remove(i);
             continue;
@@ -616 +616 @@
                 continue;
             }
-            marker.setEndOffset(targetEndOffset < node.maxCharacterOffset() ? targetEndOffset : node.maxCharacterOffset());
+            marker.setEndOffset(std::min<unsigned>(targetEndOffset, node.length()));
             didShiftMarker = true;
         }

trunk/Source/WebCore/dom/Node.cpp

@@ -1103 +1103 @@
 }
 
-int Node::maxCharacterOffset() const
-{
-    ASSERT_NOT_REACHED();
-    return 0;
-}
-
 // FIXME: Shouldn't these functions be in the editing code?  Code that asks questions about HTML in the core DOM class
 // is obviously misplaced.

trunk/Source/WebCore/dom/Node.h

@@ -378 +378 @@
     bool containsIncludingShadowDOM(const Node*) const;
 
-    // Number of DOM 16-bit units contained in node. Note that rendered text length can be different - e.g. because of
-    // css-transform:capitalize breaking up precomposed characters and ligatures.
-    virtual int maxCharacterOffset() const;
-
     // Whether or not a selection can be started in this object
     virtual bool canStartSelection() const;

trunk/Source/WebCore/dom/Position.cpp

@@ -208 +208 @@
         return 0;
     case PositionIsAfterChildren:
-        return lastOffsetInNode(m_anchorNode.get());
+        return m_anchorNode->length();
     case PositionIsOffsetInAnchor:
         return minOffsetForNode(m_anchorNode.get(), m_offset);

trunk/Source/WebCore/dom/Position.h

@@ -26 +26 @@
 #pragma once
 
+#include "CharacterData.h"
 #include "ContainerNode.h"
 #include "EditingBoundary.h"
@@ -238 +239 @@
 Position positionAfterNode(Node* anchorNode);
 
-int lastOffsetInNode(Node*);
-
 // firstPositionInNode and lastPositionInNode return parent-anchored positions, lastPositionInNode construction is O(n) due to countChildNodes()
 Position firstPositionInNode(Node* anchorNode);
@@ -307 +306 @@
 }
 
-inline int lastOffsetInNode(Node* node)
-{
-    return node->isCharacterDataNode() ? node->maxCharacterOffset() : node->countChildNodes();
-}
-
 // firstPositionInNode and lastPositionInNode return parent-anchored positions, lastPositionInNode construction is O(n) due to countChildNodes()
 inline Position firstPositionInNode(Node* anchorNode)
@@ -323 +317 @@
 {
     if (anchorNode->isTextNode())
-        return Position(anchorNode, lastOffsetInNode(anchorNode), Position::PositionIsOffsetInAnchor);
+        return Position(anchorNode, anchorNode->length(), Position::PositionIsOffsetInAnchor);
     return Position(anchorNode, Position::PositionIsAfterChildren);
 }
@@ -329 +323 @@
 inline int minOffsetForNode(Node* anchorNode, unsigned offset)
 {
-    if (anchorNode->isCharacterDataNode())
-        return std::min<unsigned>(offset, anchorNode->maxCharacterOffset());
+    if (is<CharacterData>(*anchorNode))
+        return std::min(offset, downcast<CharacterData>(*anchorNode).length());
 
     unsigned newOffset = 0;
     for (Node* node = anchorNode->firstChild(); node && newOffset < offset; node = node->nextSibling())
         newOffset++;
-    
     return newOffset;
 }
@@ -341 +334 @@
 inline bool offsetIsBeforeLastNodeOffset(unsigned offset, Node* anchorNode)
 {
-    if (anchorNode->isCharacterDataNode())
-        return offset < static_cast<unsigned>(anchorNode->maxCharacterOffset());
+    if (is<CharacterData>(*anchorNode))
+        return offset < downcast<CharacterData>(*anchorNode).length();
 
     unsigned currentOffset = 0;
     for (Node* node = anchorNode->firstChild(); node && currentOffset < offset; node = node->nextSibling())
         currentOffset++;
-    
-    
     return offset < currentOffset;
 }

trunk/Source/WebCore/dom/RangeBoundaryPoint.h

@@ -148 +148 @@
 {
     m_containerNode = WTFMove(container);
-    if (m_containerNode->isCharacterDataNode()) {
-        m_offsetInContainer = m_containerNode->maxCharacterOffset();
+    if (is<CharacterData>(*m_containerNode)) {
+        m_offsetInContainer = downcast<CharacterData>(*m_containerNode).length();
         m_childBeforeBoundary = nullptr;
     } else {

trunk/Source/WebCore/editing/ApplyBlockElementCommand.cpp

@@ -176 +176 @@
 {
     Node* textNode = position.containerNode();
-    int offset = position.offsetInContainerNode();
-    if (!is<Text>(textNode) || offset < 0 || offset >= textNode->maxCharacterOffset())
+    unsigned offset = position.offsetInContainerNode();
+    if (!is<Text>(textNode) || offset >= downcast<Text>(*textNode).length())
         return false;
     return downcast<Text>(*textNode).data()[offset] == '\n';
@@ -233 +233 @@
         bool isEndAndEndOfLastParagraphOnSameNode = renderStyleOfEnclosingTextNode(m_endOfLastParagraph) && end.deprecatedNode() == m_endOfLastParagraph.deprecatedNode();
         // Include \n at the end of line if we're at an empty paragraph
-        if (endStyle->preserveNewline() && start == end && end.offsetInContainerNode() < end.containerNode()->maxCharacterOffset()) {
-            int endOffset = end.offsetInContainerNode();
+        unsigned endOffset = end.offsetInContainerNode();
+        if (endStyle->preserveNewline() && start == end && endOffset < end.containerNode()->length()) {
             if (!isNewLineAtPosition(end.previous()) && isNewLineAtPosition(end))
-                end = Position(end.containerText(), endOffset + 1);
+                end = Position(end.containerText(), ++endOffset);
             if (isEndAndEndOfLastParagraphOnSameNode && end.offsetInContainerNode() >= m_endOfLastParagraph.offsetInContainerNode())
                 m_endOfLastParagraph = end;
@@ -242 +242 @@
 
         // If end is in the middle of a text node and the text node is editable, split.
-        if (endStyle->userModify() != UserModify::ReadOnly && !endStyle->collapseWhiteSpace() && end.offsetInContainerNode() && end.offsetInContainerNode() < end.containerNode()->maxCharacterOffset()) {
+        if (endStyle->userModify() != UserModify::ReadOnly && !endStyle->collapseWhiteSpace() && endOffset && endOffset < end.containerNode()->length()) {
             RefPtr<Text> endContainer = end.containerText();
-            splitTextNode(*endContainer, end.offsetInContainerNode());
+            splitTextNode(*endContainer, endOffset);
             if (is<Text>(endContainer) && !endContainer->previousSibling()) {
                 start = { };
@@ -253 +253 @@
                 start = firstPositionInOrBeforeNode(endContainer->previousSibling());
             if (isEndAndEndOfLastParagraphOnSameNode) {
-                if (m_endOfLastParagraph.offsetInContainerNode() == end.offsetInContainerNode())
+                if (static_cast<unsigned>(m_endOfLastParagraph.offsetInContainerNode()) == endOffset)
                     m_endOfLastParagraph = lastPositionInOrAfterNode(endContainer->previousSibling());
                 else
-                    m_endOfLastParagraph = Position(endContainer.get(), m_endOfLastParagraph.offsetInContainerNode() - end.offsetInContainerNode());
+                    m_endOfLastParagraph = Position(endContainer.get(), m_endOfLastParagraph.offsetInContainerNode() - endOffset);
             }
             end = lastPositionInNode(endContainer->previousSibling());

trunk/Source/WebCore/editing/ApplyStyleCommand.cpp

@@ -1092 +1092 @@
     // e.g. if pushDownStart was at Position("hello", 5) in <b>hello<div>world</div></b>, we want Position("world", 0) instead.
     auto* pushDownStartContainer = pushDownStart.containerNode();
-    if (is<Text>(pushDownStartContainer) && pushDownStart.computeOffsetInContainerNode() == pushDownStartContainer->maxCharacterOffset())
+    if (is<Text>(pushDownStartContainer) && static_cast<unsigned>(pushDownStart.computeOffsetInContainerNode()) == downcast<Text>(*pushDownStartContainer).length())
         pushDownStart = nextVisuallyDistinctCandidate(pushDownStart);
     // If pushDownEnd is at the start of a text node, then this node is not fully selected.

trunk/Source/WebCore/editing/Editing.cpp

@@ -378 +378 @@
 int lastOffsetForEditing(const Node& node)
 {
-    if (node.isCharacterDataNode())
-        return node.maxCharacterOffset();
-
-    if (node.hasChildNodes())
-        return node.countChildNodes();
-
-    // NOTE: This should preempt the countChildNodes() for, e.g., select nodes.
-    // FIXME: What does the comment above mean?
-    if (editingIgnoresContent(node))
-        return 1;
-
-    return 0;
+    if (node.isCharacterDataNode() || node.hasChildNodes())
+        return node.length();
+
+    // FIXME: Might be more helpful to return 1 for any node where editingIgnoresContent is true, even one that happens to have child nodes, like a select element with option node children.
+    return editingIgnoresContent(node) ? 1 : 0;
 }
 

trunk/Source/WebCore/editing/EditingStyle.cpp

@@ -1498 +1498 @@
     // We only do this for range because caret at Position("hello", 5) in <b>hello</b>world should give you font-weight: bold. 
     Node* positionNode = position.containerNode(); 
-    if (selection.isRange() && positionNode && positionNode->isTextNode() && position.computeOffsetInContainerNode() == positionNode->maxCharacterOffset()) 
-        position = nextVisuallyDistinctCandidate(position); 
+    if (selection.isRange() && is<Text>(positionNode) && static_cast<unsigned>(position.computeOffsetInContainerNode()) == downcast<Text>(*positionNode).length())
+        position = nextVisuallyDistinctCandidate(position);
 
     Element* element = position.element();

trunk/Source/WebCore/editing/InsertListCommand.cpp

@@ -257 +257 @@
                 currentSelection->setStart(*newList, 0);
             if (rangeEndIsInList && newList)
-                currentSelection->setEnd(*newList, lastOffsetInNode(newList.get()));
+                currentSelection->setEnd(*newList, newList->length());
 
             setEndingSelection(VisiblePosition(firstPositionInNode(newList.get())));

trunk/Source/WebCore/editing/TextIterator.cpp

@@ -1135 +1135 @@
         if (endOffset > 0 && endOffset <= endNode->countChildNodes()) {
             endNode = endNode->traverseToChildAt(endOffset - 1);
-            endOffset = lastOffsetInNode(endNode);
+            endOffset = endNode->length();
         }
     }

trunk/Source/WebCore/editing/VisibleUnits.cpp

@@ -638 +638 @@
 
     auto& node = it.atEnd() ? searchRange->startContainer() : it.range().start.container.get();
-    if ((!suffixLength && node.isTextNode() && static_cast<int>(next) <= node.maxCharacterOffset()) || (node.renderer() && node.renderer()->isBR() && !next)) {
+    if ((!suffixLength && is<Text>(node) && next <= downcast<Text>(node).length()) || (node.renderer() && node.renderer()->isBR() && !next)) {
         // The next variable contains a usable index into a text node
         return VisiblePosition(createLegacyEditingPosition(&node, next), DOWNSTREAM);

trunk/Source/WebCore/html/HTMLTextFormControlElement.cpp

@@ -469 +469 @@
         ASSERT(!node->firstChild());
         ASSERT(node->isTextNode() || node->hasTagName(brTag));
-        int length = node->isTextNode() ? lastOffsetInNode(node.get()) : 1;
+        int length = is<Text>(*node) ? downcast<Text>(*node).length() : 1;
 
         if (offset <= start && start <= offset + length)