[webkit-reviews] review granted: [Bug 108773] Cleanup: Use exceptionless Range::* methods rather than ignoring exceptions. : [Attachment 186321] Patch

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has granted Mike West <mkwst at chromium.org>'s
request for review:
Bug 108773: Cleanup: Use exceptionless Range::* methods rather than ignoring
exceptions.
https://bugs.webkit.org/show_bug.cgi?id=108773

Attachment 186321: Patch
https://bugs.webkit.org/attachment.cgi?id=186321&action=review

------- Additional Comments from Darin Adler <darin at apple.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=186321&action=review


Making a change like this is a good way to finish what I started when I created
the non-exception versions of some of these functions for internal use.

It might possibly be a defensible design for the DOM Range to have a special
detached state that throws exceptions whenever you try to do anything other
than overwriting the range. However, when you think about it it's not great. It
violates the Liskov substitution principle: A Range that can’t do any of the
things that a Range normally does is kind of irritating to program with.

For use inside WebKit, it is better to have a class that has a more typical
design for our project rather than this unusually strict contract that was
chosen for the DOM. For a detached range, functions should do something logical
rather than always throwing exceptions or asserting so we can program without
always having to write extra code for exceptional cases. For example, the start
container function can return null, offsets can be zero, and other functions
can do things that are sensible for an empty range with nothing in it. That’s
what I had in mind when creating the startContainer function with no exception
argument. An alternative would creating a separate Range class for the many
internal uses inside WebKit. Those have little reason to be burdened with the
strange design choices of the public DOM Range class. That something we may
still need to do soon for other reasons, if we change internal representation
so that expressing everything with DOM nodes is costly.

But Range::collapsed is not like the other functions here. The
ASSERT_NO_EXCEPTION in the header creates a function that asserts if the Range
is detached. That's not the behavior that is most useful for an internal class
and there’s a reason we don’t assert in the startContainer function. Instead,
it would be helpful to have a function returns true when called on a detached
Range. I suggest creating a new function named isCollapsed with that behavior,
and removing the ASSERT_NO_EXCEPTION from the collapsed function that we have
left there for the DOM’s benefit. Or we could change the return value from
collapsed for the detached case, since it’s ignored by the JavaScript language
bindings anyway, and continue to call collapsed, maybe with IGNORE_EXCEPTION
instead of ASSERT_NO_EXCEPTION.

> Source/WebCore/editing/EditorCommand.cpp:274
> +    return Range::create(a->startContainer()->ownerDocument(),
start->startContainer(), start->startOffset(), end->endContainer(),
end->endOffset());

It's nutty that this code says a->startContainer()->ownerDocument() here. It
could be a->ownerDocument() or a->startContainer()->document() or even
start->startContainer()->document().

> Source/WebCore/editing/TextCheckingHelper.cpp:278
> +		   ASSERT(misspellingRange->startContainer());

Good in theory to write a new assertion to replace the old one. But this
exception isn’t helpful -- you should just omit it.

> Source/WebCore/editing/TextCheckingHelper.cpp:442
> +	       ASSERT(badGrammarRange->startContainer());

Good in theory to write a new assertion to replace the old one. But this
exception isn’t helpful -- you should just omit it.