Bug 108921 – Range::collapsed callers should explicitly ASSERT_NO_EXCEPTION.

• Status: RESOLVED
• Resolution: FIXED
• Component: WebCore Misc.

[Mike West]

In https://bugs.webkit.org/show_bug.cgi?id=108773, Darin noted that I'd incorrectly relied on the default value of 'ec' in Range::collapsed. He suggested creating a Range::isCollapsed method instead: https://bugs.webkit.org/show_bug.cgi?id=108773#c8.

I grepped quickly through the code, and there are only three callsites that rely on the current behavior. I'd suggest instead that we overload Range::collapsed in the same way that Range::startContainer, etc. are overloaded, and simply update those three callsites to explicitly call ASSERT_NO_EXCEPTION.

[Mike West]

Created attachment 186579 [details]
Patch

[EFL EWS Bot]

Comment on attachment 186579 [details]
Patch

Attachment 186579 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/16375661

[WebKit Review Bot]

Comment on attachment 186579 [details]
Patch

Attachment 186579 [details] did not pass cr-linux-debug-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/16367679

[Mike West]

Created attachment 186590 [details]
Patch

[Alexey Proskuryakov]

Comment on attachment 186590 [details]
Patch

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

> Source/WebCore/dom/Range.h:61
> +    bool collapsed() const { return m_start == m_end; }

Should this have an ASSERT(m_start)?

Otherwise, the two versions of collapsed have different semantics - one says yes for null range, and another does not say yes.

[Mike West]

(In reply to comment #5)
> (From update of attachment 186590 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=186590&action=review
> 
> > Source/WebCore/dom/Range.h:61
> > +    bool collapsed() const { return m_start == m_end; }
> 
> Should this have an ASSERT(m_start)?
> 
> Otherwise, the two versions of collapsed have different semantics - one says yes for null range, and another does not say yes.

Hrm. You're right, I missed that detail.

Perhaps it would be more appropriate not to ASSERT, but to `return m_start.container() && m_start == m_end;`. I think that would end up with the same semantics for the case I missed.

[Mike West]

Created attachment 186831 [details]
Patch

[Darin Adler]

Comment on attachment 186831 [details]
Patch

I like the idea of having a collapsed function. But I don’t like the idea of it returning false for detached ranges. I think true is a more useful value to return for both the value with the exception and without. In the bindings, the return value when there is an exception is undetectable, so there is no “public” repercussion of changing the sense of the boolean.

Further, I don’t think it’s so great to have our internal functions just be the same as the public DOM functions, only without exception codes. It’s better to put some thought into the design and naming of internal functions and have them be distinct. That’s why I suggest naming this isCollapsed instead of collapsed.

[Eric Seidel (no email)]

Comment on attachment 186831 [details]
Patch

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

> Source/WebCore/dom/Range.h:61
> +    bool collapsed() const { return startContainer() && m_start == m_end; }

So this will return "false negatives"?  In that we will count detached ranges as non-collapsed?  I guess that matches our previous behavior?  Or we could have a function which returns an enum instead?  Detached, Caret, Span?

[Darin Adler]

(In reply to comment #9)
> So this will return "false negatives"?  In that we will count detached ranges as non-collapsed?

That's right.

> I guess that matches our previous behavior?

True, but I think it’s not convenient behavior to program with, and it has no effect on the JavaScript DOM bindings case, since the return value is ignored if there’s an exception. Generally speaking, what a DOM function returns when it raises an exception has no effect on the JavaScript DOM, but affects our internal WebKit code that uses that function and possibly some non-JavaScript bindings.

> Or we could have a function which returns an enum instead? Detached, Caret, Span?

If we’re going to start really rethinking the Range interface I think we might just want to make a new class and thin out Range to make it only really used for the public DOM. More the pattern that Antti followed with the CSS DOM where we don’t build our internals on top of possibly-wonky DOM APIs.

[Darin Adler]

Comment on attachment 186831 [details]
Patch

Since it's not entirely obvious what an exception-free version of collapsed should do, lets not add it right now.

I think for now a better idea would be to remove ASSERT_NO_EXCEPTION from the header, put IGNORE_EXCEPTION at call sites currently ignoring the exception, ASSERT_NO_EXCEPTION at call sites currently asserting no exception, table the question of a better design for later, perhaps looking at the bigger picture of the entire Range class.

[Mike West]

(In reply to comment #11)
> (From update of attachment 186831 [details])
> Since it's not entirely obvious what an exception-free version of collapsed should do, lets not add it right now.
> 
> I think for now a better idea would be to remove ASSERT_NO_EXCEPTION from the header, put IGNORE_EXCEPTION at call sites currently ignoring the exception, ASSERT_NO_EXCEPTION at call sites currently asserting no exception, table the question of a better design for later, perhaps looking at the bigger picture of the entire Range class.

Easy enough.

[Mike West]

Created attachment 187430 [details]
Patch

[Build Bot]

Comment on attachment 187430 [details]
Patch

Attachment 187430 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://queues.webkit.org/results/16467710

New failing tests:
compositing/checkerboard.html
accessibility/anchor-linked-anonymous-block-crash.html
http/tests/cache/cancel-multiple-post-xhrs.html
animations/3d/state-at-end-event-transform.html
animations/animation-add-events-in-handler.html
animations/3d/replace-filling-transform.html
http/tests/cache/history-only-cached-subresource-loads.html
compositing/bounds-in-flipped-writing-mode.html
accessibility/accessibility-node-reparent.html
animations/animation-border-overflow.html
accessibility/accessibility-object-detached.html
accessibility/anonymous-render-block-in-continuation-causes-crash.html
animations/animation-controller-drt-api.html
compositing/absolute-position-changed-with-composited-parent-layer.html
compositing/absolute-position-changed-in-composited-layer.html
http/tests/cache/iframe-304-crash.html
animations/3d/transform-perspective.html
http/tests/cache/cancel-during-failure-crash.html
canvas/philip/tests/2d.clearRect+fillRect.alpha0.5.html
canvas/philip/tests/2d.clearRect.basic.html
animations/3d/transform-origin-vs-functions.html
animations/animation-css-rule-types.html
canvas/philip/tests/2d.clearRect+fillRect.basic.html
canvas/philip/tests/2d.clearRect+fillRect.alpha0.html

Comment on attachment 187430 [details]
Patch

ok

[WebKit Review Bot]

Comment on attachment 187430 [details]
Patch

Clearing flags on attachment: 187430

Committed r142461: <http://trac.webkit.org/changeset/142461>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.