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

Source/core/dom/Range.cpp

 /*
  * (C) 1999 Lars Knoll (knoll@kde.org)
  * (C) 2000 Gunnstein Lye (gunnstein@netcom.no)
  * (C) 2000 Frederik Holljen (frederik.holljen@hig.no)
  * (C) 2001 Peter Kelly (pmk@post.com)
  * Copyright (C) 2004, 2005, 2006, 2007, 2008, 2009, 2010, 2011 Apple Inc. All rights reserved.
  * Copyright (C) 2011 Motorola Mobility. All rights reserved.
  *
  * This library is free software; you can redistribute it and/or
  * modify it under the terms of the GNU Library General Public
@@ skipping 303 matching lines @@
         if (comparePoint(parentNode, nodeIndex + 1, exceptionState) > 0) // ends after the range
             return NODE_BEFORE_AND_AFTER;
         return NODE_BEFORE; // ends before or in the range
     }
     // starts at or after the range start
     if (comparePoint(parentNode, nodeIndex + 1, exceptionState) > 0) // ends after the range
         return NODE_AFTER;
     return NODE_INSIDE; // ends inside the range
 }
 
-short Range::compareBoundaryPoints(CompareHow how, const Range* sourceRange, ExceptionState& exceptionState) const
+short Range::compareBoundaryPoints(CompareHow how, const PassRefPtrWillBeRawPtr<Range> sourceRange, ExceptionState& exceptionState) const

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

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]

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 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]

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.
Yeah, please go ahead and write the change.

 {
-    if (!sourceRange) {
-        exceptionState.throwDOMException(NotFoundError, "The source range provided was null.");
+    if (!(how == START_TO_START || how == START_TO_END || how == END_TO_END || how == END_TO_START)) {
+        exceptionState.throwDOMException(NotSupportedError, "The comparison method provided must be one of 'START_TO_START', 'START_TO_END', 'END_TO_END', or 'END_TO_START'.");
         return 0;
     }
 
     Node* thisCont = commonAncestorContainer();
     Node* sourceCont = sourceRange->commonAncestorContainer();
     if (thisCont->document() != sourceCont->document()) {
         exceptionState.throwDOMException(WrongDocumentError, "The source range is in a different document than this range.");
         return 0;
     }
 
@@ skipping 12 matching lines @@
         case START_TO_START:
             return compareBoundaryPoints(m_start, sourceRange->m_start, exceptionState);
         case START_TO_END:
             return compareBoundaryPoints(m_end, sourceRange->m_start, exceptionState);
         case END_TO_END:
             return compareBoundaryPoints(m_end, sourceRange->m_end, exceptionState);
         case END_TO_START:
             return compareBoundaryPoints(m_start, sourceRange->m_end, exceptionState);
     }
 
-    exceptionState.throwDOMException(SyntaxError, "The comparison method provided must be one of 'START_TO_START', 'START_TO_END', 'END_TO_END', or 'END_TO_START'.");
+    ASSERT_NOT_REACHED();
     return 0;
 }
 
 short Range::compareBoundaryPoints(Node* containerA, int offsetA, Node* containerB, int offsetB, ExceptionState& exceptionState)
 {
     ASSERT(containerA);
     ASSERT(containerB);
 
     if (!containerA)
         return -1;
@@ skipping 1338 matching lines @@
 
 void showTree(const WebCore::Range* range)
 {
     if (range && range->boundaryPointsValid()) {
         range->startContainer()->showTreeAndMark(range->startContainer(), "S", range->endContainer(), "E");
         fprintf(stderr, "start offset: %d, end offset: %d\n", range->startOffset(), range->endOffset());
     }
 }
 
 #endif