Range: node offsets should be unsigned.

third_party/WebKit/Source/core/dom/RangeBoundaryPoint.h

 /*
  * Copyright (C) 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 26 matching lines @@
 
  public:
   explicit RangeBoundaryPoint(Node* container);
 
   explicit RangeBoundaryPoint(const RangeBoundaryPoint&);
 
   bool isConnected() const;
   const Position toPosition() const;
 
   Node* container() const;
-  int offset() const;
+  unsigned offset() const;
   Node* childBefore() const;
 
   void clear();
 
-  void set(Node* container, int offset, Node* childBefore);
-  void setOffset(int);
+  void set(Node* container, unsigned offset, Node* childBefore);
+  void setOffset(unsigned);
 
   void setToBeforeChild(Node&);
   void setToStartOfNode(Node&);
   void setToEndOfNode(Node&);
 
   void childBeforeWillBeRemoved();
   void invalidateOffset();
   void markValid() const;
 
   DEFINE_INLINE_TRACE() {
     visitor->trace(m_containerNode);
     visitor->trace(m_childBeforeBoundary);
   }
 
  private:
   uint64_t domTreeVersion() const;
   void ensureOffsetIsValid() const;
   bool isOffsetValid() const;
 
-  static const int invalidOffset = -1;
+  static const unsigned invalidOffset = static_cast<unsigned>(-1);

[Sunny, 2017/02/21 12:32:05]

Hi tkent@, I've tried working on this issue before and have a question.

In this file do you think it's acceptable to cast -1 to unsigned as a indicator
of invalid offset?

My solution is use a boolean as indicator:
https://codereview.chromium.org/2673303002/diff/20001/third_party/WebKit/Sour...

Do you think it's unnecessary?

[tkent, 2017/02/22 02:08:43]

Adding a boolean flag is a more graceful way.
However, static_cast<unsigned>(-1) as an invalid value is more efficient than
the boolean flag in terms of memory consumption.
With the static_cast<unsigned>(-1), we can't handle offset=4294967295 correctly.
 But it's not a practical problem because Blink can't handle such large nodes
anyway.

 
   Member<Node> m_containerNode;
   Member<Node> m_childBeforeBoundary;
   mutable uint64_t m_domTreeVersion;
-  mutable int m_offsetInContainer;
+  mutable unsigned m_offsetInContainer;
 };
 
 inline RangeBoundaryPoint::RangeBoundaryPoint(Node* container)
     : m_containerNode(container),
       m_childBeforeBoundary(nullptr),
       m_domTreeVersion(domTreeVersion()),
       m_offsetInContainer(0) {}
 
 inline RangeBoundaryPoint::RangeBoundaryPoint(const RangeBoundaryPoint& other)
     : m_containerNode(other.container()),
@@ skipping 39 matching lines @@
 }
 
 inline const Position RangeBoundaryPoint::toPosition() const {
   ensureOffsetIsValid();
   // TODO(yosin): We should return |Position::beforeAnchor| when
   // |m_containerNode| isn't |Text| node.
   return Position::editingPositionOf(m_containerNode.get(),
                                      m_offsetInContainer);
 }
 
-inline int RangeBoundaryPoint::offset() const {
+inline unsigned RangeBoundaryPoint::offset() const {
   ensureOffsetIsValid();
   return m_offsetInContainer;
 }
 
 inline void RangeBoundaryPoint::clear() {
   m_containerNode.clear();
   m_offsetInContainer = 0;
   m_childBeforeBoundary = nullptr;
   m_domTreeVersion = 0;
 }
 
 inline void RangeBoundaryPoint::set(Node* container,
-                                    int offset,
+                                    unsigned offset,
                                     Node* childBefore) {
   DCHECK(container);
-  DCHECK_GE(offset, 0);
+  DCHECK_GE(offset, 0u);
   DCHECK_EQ(childBefore,
             offset ? NodeTraversal::childAt(*container, offset - 1) : 0);
   m_containerNode = container;
   m_offsetInContainer = offset;
   m_childBeforeBoundary = childBefore;
   markValid();
 }
 
-inline void RangeBoundaryPoint::setOffset(int offset) {
+inline void RangeBoundaryPoint::setOffset(unsigned offset) {
   DCHECK(m_containerNode);
   DCHECK(m_containerNode->isCharacterDataNode());
-  DCHECK_GE(m_offsetInContainer, 0);
+  DCHECK_GE(m_offsetInContainer, 0u);
   DCHECK(!m_childBeforeBoundary);
   m_offsetInContainer = offset;
   markValid();
 }
 
 inline void RangeBoundaryPoint::setToBeforeChild(Node& child) {
   DCHECK(child.parentNode());
   m_childBeforeBoundary = child.previousSibling();
   m_containerNode = child.parentNode();
   m_offsetInContainer = m_childBeforeBoundary ? invalidOffset : 0;
@@ skipping 16 matching lines @@
     m_childBeforeBoundary = m_containerNode->lastChild();
     m_offsetInContainer = m_childBeforeBoundary ? invalidOffset : 0;
   }
   markValid();
 }
 
 inline void RangeBoundaryPoint::childBeforeWillBeRemoved() {
   m_childBeforeBoundary = m_childBeforeBoundary->previousSibling();
   if (!isOffsetValid())
     return;
-  DCHECK_GT(m_offsetInContainer, 0);
+  DCHECK_GT(m_offsetInContainer, 0u);
   if (!m_childBeforeBoundary)
     m_offsetInContainer = 0;
   else if (m_offsetInContainer > 0)
     --m_offsetInContainer;
   markValid();
 }
 
 inline void RangeBoundaryPoint::invalidateOffset() {
   m_offsetInContainer = invalidOffset;
 }
@@ skipping 12 matching lines @@
   } else {
     if (a.offset() != b.offset())
       return false;
   }
   return true;
 }
 
 }  // namespace blink
 
 #endif