Bug 148776 – Range.isPointInRange check root node before verifying offset

• Status: RESOLVED
• Resolution: FIXED
• Component: DOM

[Ryosuke Niwa]

Range.isPointInRange should check whether the root nodes of boundary point and refNode are the same
and return false if they are different before verifying the offset.
See https://dom.spec.whatwg.org/#dom-range-ispointinrange

This bug was found by the newly added LayoutTests/http/tests/w3c/dom/ranges/Range-isPointInRange.html.

[Radar WebKit Bug Importer]

<rdar://problem/22571620>

[Ryosuke Niwa]

Created attachment 260590 [details]
Fixes the bug

[Ryosuke Niwa]

Created attachment 260591 [details]
Fixed typos

[Darin Adler]

Comment on attachment 260591 [details]
Fixed typos

View in context: https://bugs.webkit.org/attachment.cgi?id=260591&action=review

> Source/WebCore/dom/Range.cpp:246
> +    ExceptionCode ecToBeSupressed = 0;

Spelling error here: The word suppressed has two "p" in it.

I also think it’s a little strange to do this so differently here than above. Above we just use ec and set it to zero afterward.

> Source/WebCore/dom/Range.cpp:-242
> -    return compareBoundaryPoints(refNode, offset, &startContainer(), m_start.offset(), ec) >= 0 && !ec
> -        && compareBoundaryPoints(refNode, offset, &endContainer(), m_end.offset(), ec) <= 0 && !ec;

How about writing it like this?

    bool result = compareBoundaryPoints(refNode, offset, &startContainer(), m_start.offset(), ec) >= 0 && !ec
        && compareBoundaryPoints(refNode, offset, &endContainer(), m_end.offset(), ec) <= 0 && !ec;
    ASSERT(!ec || ec == WRONG_DOCUMENT_ERR);
    ec = 0;
    return result;

I think it’s simpler.

[Ryosuke Niwa]

Comment on attachment 260591 [details]
Fixed typos

View in context: https://bugs.webkit.org/attachment.cgi?id=260591&action=review

>> Source/WebCore/dom/Range.cpp:246
>> +    ExceptionCode ecToBeSupressed = 0;
> 
> Spelling error here: The word suppressed has two "p" in it.
> 
> I also think it’s a little strange to do this so differently here than above. Above we just use ec and set it to zero afterward.

Fixed.

>> Source/WebCore/dom/Range.cpp:-242
>> -        && compareBoundaryPoints(refNode, offset, &endContainer(), m_end.offset(), ec) <= 0 && !ec;
> 
> How about writing it like this?
> 
>     bool result = compareBoundaryPoints(refNode, offset, &startContainer(), m_start.offset(), ec) >= 0 && !ec
>         && compareBoundaryPoints(refNode, offset, &endContainer(), m_end.offset(), ec) <= 0 && !ec;
>     ASSERT(!ec || ec == WRONG_DOCUMENT_ERR);
>     ec = 0;
>     return result;
> 
> I think it’s simpler.

Much cleaner!  Done that.

[Ryosuke Niwa]

Committed r189347: <http://trac.webkit.org/changeset/189347>