Drop [UsePointersEvenForNonNullableObjectArguments] from Range
Created attachment 276884 [details] Patch
Created attachment 276888 [details] Patch
Created attachment 276889 [details] Patch
Created attachment 276890 [details] Patch
Created attachment 276892 [details] Patch
Created attachment 276894 [details] Patch
Created attachment 276895 [details] Patch
Comment on attachment 276895 [details] Patch r=me, some nits below. View in context: https://bugs.webkit.org/attachment.cgi?id=276895&action=review > Source/WebCore/accessibility/AXObjectCache.cpp:1639 > + return !range2 || range1->compareBoundaryPoints(Range::START_TO_START, *range2, IGNORE_EXCEPTION) <= 0; Do you know what is preventing range1 to be null? Should we check it/assert it? > Source/WebCore/accessibility/AXObjectCache.cpp:1758 > copyRange = Range::create(range->ownerDocument(), &range->startContainer(), range->startOffset(), &range->endContainer(), range->endOffset()); Is it possible to make copyRange a Ref<Range> and/or limit ref count churn here? > Source/WebCore/accessibility/AXObjectCache.cpp:1761 > + copyRange->setStart(*node, nodeStartOffset.startIndex); Why not making node a Node& instead of a Node*? > Source/WebCore/accessibility/atk/WebKitAccessibleUtil.cpp:162 > Node* node = coreObject->node(); Add ASSERT(node)? > Source/WebCore/editing/htmlediting.cpp:1171 > return VisiblePosition(firstPositionInOrBeforeNode(node), DOWNSTREAM); Could visiblePositionForIndexUsingCharacterIterator take a Node& instead of a Node*, like you did for isNodeVisiblyContainedWithin?
Comment on attachment 276895 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=276895&action=review >> Source/WebCore/accessibility/AXObjectCache.cpp:1639 >> + return !range2 || range1->compareBoundaryPoints(Range::START_TO_START, *range2, IGNORE_EXCEPTION) <= 0; > > Do you know what is preventing range1 to be null? > Should we check it/assert it? range1 was already being dereferenced so this is not new. An assertion would not bring much here because we would anyway crash right away when dereferencing range1 if range1 were null. We would add a null-check for range1 but considering there was not one before and my patch did not change range1 in any way, I decided to leave it alone. >> Source/WebCore/accessibility/AXObjectCache.cpp:1758 >> copyRange = Range::create(range->ownerDocument(), &range->startContainer(), range->startOffset(), &range->endContainer(), range->endOffset()); > > Is it possible to make copyRange a Ref<Range> and/or limit ref count churn here? I don't see why not. >> Source/WebCore/accessibility/AXObjectCache.cpp:1761 >> + copyRange->setStart(*node, nodeStartOffset.startIndex); > > Why not making node a Node& instead of a Node*? Ok. >> Source/WebCore/accessibility/atk/WebKitAccessibleUtil.cpp:162 >> Node* node = coreObject->node(); > > Add ASSERT(node)? It would anyway crash right away upon dereferencing in the line below. >> Source/WebCore/editing/htmlediting.cpp:1171 >> return VisiblePosition(firstPositionInOrBeforeNode(node), DOWNSTREAM); > > Could visiblePositionForIndexUsingCharacterIterator take a Node& instead of a Node*, like you did for isNodeVisiblyContainedWithin? Ok.
Created attachment 276932 [details] Patch
Committed r199817: <http://trac.webkit.org/changeset/199817>