Add [TypeChecking=Interface] to Range interface

Reland: Add [TypeChecking=Interface] to Range interface

All functions already threw exceptions for invalid argument values, but
DOMException exceptions instead of TypeError exceptions.

Throwing TypeError exceptions matches Firefox's behavior.

Note that the null checks are kept in the implementation since many of
the functions are called from C++ code that in some cases depend on the
existing error handling of invalid input.

BUG=462561
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=191442

[Jens Widell, 2015-03-05 10:18:07]

jl@opera.com changed reviewers:
+ philipj@opera.com

[Jens Widell, 2015-03-05 10:18:07]

PTAL

[philipj_slow, 2015-03-05 13:21:00]

Nice cleanup! The bots still look a little sad, but LGTM if that's just
flakiness or some trivial test change needed.

[Jens Widell, 2015-03-05 13:25:10]

On 2015/03/05 13:21:00, philipj_UTC7 wrote:
> Nice cleanup! The bots still look a little sad, but LGTM if that's just
> flakiness or some trivial test change needed.

The linux_blink_rel failure is a flake, I think, since win_blink_rel passed, and
the previous run only had one test failing. I'll retry some more. :-)

[Jens Widell, 2015-03-05 14:16:48]

The CQ bit was checked by jl@opera.com

[commit-bot: I haz the power, 2015-03-05 14:16:57]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/974823002/40001

[commit-bot: I haz the power, 2015-03-05 14:19:55]

Committed patchset #3 (id:40001) as
https://src.chromium.org/viewvc/blink?view=rev&revision=191363

[Dirk Pranke, 2015-03-05 23:34:58]

A revert of this CL (patchset #3 id:40001) has been created in
https://codereview.chromium.org/983923002/ by dpranke@chromium.org.

The reason for reverting is: This is causing two Clipboard browser_tests to
crash on multiple bots. I confirmed that reverting this change caused the tests
to go back to passing.

See, e.g.:

https://build.chromium.org/p/chromium.webkit/builders/Linux%20Tests%20%28dbg%....

[Jens Widell, 2015-03-06 09:53:25]

I've updated the patch to reintroduce the checks in the implementation (only now
throwing TypeError exceptions instead, to reduce confusion.)

Should make the patch safer. PTAL?

[philipj_slow, 2015-03-06 11:24:03]

Hmm, I think I would still be confused by this state of things when I see a
TypeError thrown that doesn't match the message in Range.cpp. Are there any
other somewhat sane alternatives, like just returning with no exception and
commenting that this case is for internal use only?

It would be cool if the bindings code could eventually call functions with Node&
references or some other type that cannot be null, so that any internal callers
are forced to consider the issue.

[Jens Widell, 2015-03-06 11:35:47]

On 2015/03/06 11:24:03, philipj_UTC7 wrote:
> Hmm, I think I would still be confused by this state of things when I see a
> TypeError thrown that doesn't match the message in Range.cpp. Are there any
> other somewhat sane alternatives, like just returning with no exception and
> commenting that this case is for internal use only?

The call site that caused the regression (frameContentAsPlainText() in
WebLocalFrameImpl.cpp) actually passes in a TrackExceptionState object and
detects if an exception is thrown. It's somewhat likely in that particular case
that leaving the range empty (no-op) and not throwing an exception would have
the same result.

But that's just one call site. There are many call sites. Some call with an
ExceptionState that simply ignores exceptions, and some with the (default)
ExceptionState that asserts that exceptions aren't thrown.

I think the only other viable alternative is to remove the (bindings-wise
redundant) exception throwing, leaving checks and returns unchanged. This has
potential to break things, but is probably the least likely to do so (of the
other alternatives.)

> It would be cool if the bindings code could eventually call functions with
Node&
> references or some other type that cannot be null, so that any internal
callers
> are forced to consider the issue.

Yes, that would make sense. You'd then get Node& for non-nullable and Node* for
nullable arguments, which would be nice, once you can rely on it.

Rather big change, though. :-)

[philipj_slow, 2015-03-06 11:56:25]

Hmm, so in the interest of making progress [TypeChecking=Interface] PS5 LGTM,
but if you don't consider it too pedantic, can you add a FIXME to each redundant
check saying about how great an idea it would be to make all the call sites
check for null instead? Then these cases can be removed one by one.

[Jens Widell, 2015-03-06 12:49:51]

On 2015/03/06 11:56:25, philipj_UTC7 wrote:
> Hmm, so in the interest of making progress [TypeChecking=Interface] PS5 LGTM,
> but if you don't consider it too pedantic, can you add a FIXME to each
redundant
> check saying about how great an idea it would be to make all the call sites
> check for null instead? Then these cases can be removed one by one.

FIXME comments added.

[philipj_slow, 2015-03-06 15:34:59]

On 2015/03/06 12:49:51, Jens Widell wrote:
> On 2015/03/06 11:56:25, philipj_UTC7 wrote:
> > Hmm, so in the interest of making progress [TypeChecking=Interface] PS5
LGTM,
> > but if you don't consider it too pedantic, can you add a FIXME to each
> redundant
> > check saying about how great an idea it would be to make all the call sites
> > check for null instead? Then these cases can be removed one by one.
> 
> FIXME comments added.

Even more LGTM :)

[Jens Widell, 2015-03-06 15:43:44]

The CQ bit was checked by jl@opera.com

[commit-bot: I haz the power, 2015-03-06 15:44:46]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/974823002/100001

[commit-bot: I haz the power, 2015-03-06 15:47:46]

Committed patchset #6 (id:100001) as
https://src.chromium.org/viewvc/blink?view=rev&revision=191442