Bug 108921 - Range::collapsed callers should explicitly ASSERT_NO_EXCEPTION.
Summary: Range::collapsed callers should explicitly ASSERT_NO_EXCEPTION.
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebCore Misc. (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Mike West
URL:
Keywords:
Depends on:
Blocks: 108180
  Show dependency treegraph
 
Reported: 2013-02-05 01:21 PST by Mike West
Modified: 2013-02-11 05:57 PST (History)
7 users (show)

See Also:


Attachments
Patch (10.47 KB, patch)
2013-02-05 02:07 PST, Mike West
no flags Details | Formatted Diff | Diff
Patch (10.56 KB, patch)
2013-02-05 03:05 PST, Mike West
no flags Details | Formatted Diff | Diff
Patch (10.59 KB, patch)
2013-02-06 04:52 PST, Mike West
no flags Details | Formatted Diff | Diff
Patch (3.39 KB, patch)
2013-02-09 07:02 PST, Mike West
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Mike West 2013-02-05 01:21:44 PST
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.
Comment 1 Mike West 2013-02-05 02:07:40 PST
Created attachment 186579 [details]
Patch
Comment 2 EFL EWS Bot 2013-02-05 02:33:29 PST
Comment on attachment 186579 [details]
Patch

Attachment 186579 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/16375661
Comment 3 WebKit Review Bot 2013-02-05 02:41:15 PST
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
Comment 4 Mike West 2013-02-05 03:05:47 PST
Created attachment 186590 [details]
Patch
Comment 5 Alexey Proskuryakov 2013-02-05 10:50:25 PST
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.
Comment 6 Mike West 2013-02-05 10:59:19 PST
(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.
Comment 7 Mike West 2013-02-06 04:52:04 PST
Created attachment 186831 [details]
Patch
Comment 8 Darin Adler 2013-02-06 09:34:06 PST
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.
Comment 9 Eric Seidel (no email) 2013-02-06 12:47:08 PST
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?
Comment 10 Darin Adler 2013-02-08 09:39:13 PST
(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.
Comment 11 Darin Adler 2013-02-08 09:41:29 PST
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.
Comment 12 Mike West 2013-02-09 06:58:32 PST
(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.
Comment 13 Mike West 2013-02-09 07:02:54 PST
Created attachment 187430 [details]
Patch
Comment 14 Build Bot 2013-02-09 19:59:46 PST
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 15 jochen 2013-02-11 05:15:24 PST
Comment on attachment 187430 [details]
Patch

ok
Comment 16 WebKit Review Bot 2013-02-11 05:56:56 PST
Comment on attachment 187430 [details]
Patch

Clearing flags on attachment: 187430

Committed r142461: <http://trac.webkit.org/changeset/142461>
Comment 17 WebKit Review Bot 2013-02-11 05:57:00 PST
All reviewed patches have been landed.  Closing bug.