Bug 148776

Summary: Range.isPointInRange check root node before verifying offset
Product: WebKit Reporter: Ryosuke Niwa <rniwa>
Component: DOMAssignee: Ryosuke Niwa <rniwa>
Status: RESOLVED FIXED    
Severity: Normal CC: cdumez, commit-queue, darin, esprehn+autocc, kangil.han, webkit-bug-importer
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Fixes the bug
none
Fixed typos darin: review+

Description Ryosuke Niwa 2015-09-03 19:45:58 PDT 
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.
Comment 1 Radar WebKit Bug Importer 2015-09-03 19:46:24 PDT 
<rdar://problem/22571620>
Comment 2 Ryosuke Niwa 2015-09-04 07:15:07 PDT 
Created attachment 260590 [details]
Fixes the bug
Comment 3 Ryosuke Niwa 2015-09-04 08:52:35 PDT 
Created attachment 260591 [details]
Fixed typos
Comment 4 Darin Adler 2015-09-04 08:55:49 PDT 
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.
Comment 5 Ryosuke Niwa 2015-09-04 09:19:37 PDT 
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.
Comment 6 Ryosuke Niwa 2015-09-04 09:25:05 PDT 
Committed r189347: <http://trac.webkit.org/changeset/189347>