Range.compareBoundaryPoints should throw a NotSupportedError when how has not expected value

Range.compareBoundaryPoints should throw a NotSupportedError when how has not expected value

SPEC: http://dom.spec.whatwg.org/#dom-range-compareboundarypoints

This CL also added interface type checking so appropriate TypeError will be thrown.
In addition, 714 failed cases will be passed in
http://w3c-test.org/dom/ranges/Range-compareBoundaryPoints.html

TEST=LayoutTests/fast/dom/Range/compareBoundaryPoints-error.html

Behavior in other browsers.
*)FF: PASS
*)IE: FAIL

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

[kangil_, 2014-07-21 09:06:51]

PTAL

[haraken, 2014-07-21 14:09:38]

LGTM

This change just changes exception types and will have little compatibility
issues.

https://codereview.chromium.org/405083003/diff/1/Source/core/dom/Range.cpp
File Source/core/dom/Range.cpp (right):

https://codereview.chromium.org/405083003/diff/1/Source/core/dom/Range.cpp#ne...
Source/core/dom/Range.cpp:326: if (how < START_TO_START || how > END_TO_START) {

I'd prefer:

  if (!(how == START_TO_START || how == START_TO_END || how == END_TO_END || how
== END_TO_START))

to make the code not rely on the value of these constants.

[kangil_, 2014-07-22 00:27:01]

Thx!!

https://codereview.chromium.org/405083003/diff/1/Source/core/dom/Range.cpp
File Source/core/dom/Range.cpp (right):

https://codereview.chromium.org/405083003/diff/1/Source/core/dom/Range.cpp#ne...
Source/core/dom/Range.cpp:326: if (how < START_TO_START || how > END_TO_START) {
On 2014/07/21 14:09:38, haraken wrote:
> 
> I'd prefer:
> 
>   if (!(how == START_TO_START || how == START_TO_END || how == END_TO_END ||
how
> == END_TO_START))
> 
> to make the code not rely on the value of these constants.

Done.

[kangil_, 2014-07-22 00:27:09]

The CQ bit was checked by kangil.han@samsung.com

[commit-bot: I haz the power, 2014-07-22 00:28:20]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/kangil.han@samsung.com/405083003/20001

[tkent, 2014-07-22 01:40:04]

+editing people

[commit-bot: I haz the power, 2014-07-22 01:50:16]

Change committed as 178610

[Yuta Kitamura, 2014-07-22 06:03:39]

The change itself LGTM

https://codereview.chromium.org/405083003/diff/20001/Source/core/dom/Range.cpp
File Source/core/dom/Range.cpp (right):

https://codereview.chromium.org/405083003/diff/20001/Source/core/dom/Range.cp...
Source/core/dom/Range.cpp:324: short Range::compareBoundaryPoints(CompareHow
how, const PassRefPtrWillBeRawPtr<Range> sourceRange, ExceptionState&
exceptionState) const
nit: const seems unnecessary.

Just curious, TypeChecking=Interface requires PassRefPtr<> here?

The function does not save the Range object to anywhere, so
a pointer should be sufficient here.

[kangil_, 2014-07-22 08:41:49]

https://codereview.chromium.org/405083003/diff/20001/Source/core/dom/Range.cpp
File Source/core/dom/Range.cpp (right):

https://codereview.chromium.org/405083003/diff/20001/Source/core/dom/Range.cp...
Source/core/dom/Range.cpp:324: short Range::compareBoundaryPoints(CompareHow
how, const PassRefPtrWillBeRawPtr<Range> sourceRange, ExceptionState&
exceptionState) const
On 2014/07/22 06:03:39, Yuta Kitamura wrote:
> nit: const seems unnecessary.

IMHO, const is generally good since it protects us from unexpected change. So,
if we don't have specific reason to touch sourceRange, then we can keep as it
is. :)

> Just curious, TypeChecking=Interface requires PassRefPtr<> here?
> 
> The function does not save the Range object to anywhere, so
> a pointer should be sufficient here.

I think you are right. I habitually modified it. Do you want me to get it back
to a pointer as it was?

[Yuta Kitamura, 2014-07-22 09:22:43]

https://codereview.chromium.org/405083003/diff/20001/Source/core/dom/Range.cpp
File Source/core/dom/Range.cpp (right):

https://codereview.chromium.org/405083003/diff/20001/Source/core/dom/Range.cp...
Source/core/dom/Range.cpp:324: short Range::compareBoundaryPoints(CompareHow
how, const PassRefPtrWillBeRawPtr<Range> sourceRange, ExceptionState&
exceptionState) const
On 2014/07/22 08:41:49, kangil_ wrote:
> On 2014/07/22 06:03:39, Yuta Kitamura wrote:
> > nit: const seems unnecessary.
> 
> IMHO, const is generally good since it protects us from unexpected change. So,
> if we don't have specific reason to touch sourceRange, then we can keep as it
> is. :)

I'm not opposing to the use of const in general. I wanted to
point out the fact that |const RefPtr<T>| is not parallel to
|const T*| in terms of its meaning:

    const RefPtr<T>  <===>  T* const
    RefPtr<const T>  <===>  const T*
    (Read: [left hand side] is parallel to [right hand side])

|const RefPtr<T>| only prevents changing the pointer value.
The annotation is only meaningful within the function
implementation; from the caller's point of view, an argument
with type |const T| is exactly same as one declared as |T|.

If you are aware of these details and intentionally writing
this const, I'm fine with it. I just want to make sure you
understand the const usage well.

> 
> > Just curious, TypeChecking=Interface requires PassRefPtr<> here?
> > 
> > The function does not save the Range object to anywhere, so
> > a pointer should be sufficient here.
> 
> I think you are right. I habitually modified it. Do you want me to get it back
> to a pointer as it was?

Yeah, please go ahead and write the change.

[kangil_, 2014-07-23 02:22:38]

Done at https://codereview.chromium.org/409283003/ :)