Bug 156805 – Drop [UsePointersEvenForNonNullableObjectArguments] from Range

• Component: Bindings
• Status: RESOLVED FIXED

[Chris Dumez]

Drop [UsePointersEvenForNonNullableObjectArguments] from Range

[Chris Dumez]

Created attachment 276884 [details]
Patch

[Chris Dumez]

Created attachment 276888 [details]
Patch

[Chris Dumez]

Created attachment 276889 [details]
Patch

[Chris Dumez]

Created attachment 276890 [details]
Patch

[Chris Dumez]

Created attachment 276892 [details]
Patch

[Chris Dumez]

Created attachment 276894 [details]
Patch

[Chris Dumez]

Created attachment 276895 [details]
Patch

[youenn fablet]

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?

[Chris Dumez]

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.

[Chris Dumez]

Created attachment 276932 [details]
Patch

[Chris Dumez]

Committed r199817: <http://trac.webkit.org/changeset/199817>