Remove Range.compareNode() as it is not in the spec

Remove Range.compareNode() as it is not in the spec

https://dom.spec.whatwg.org/#interface-range

Intent to Remove :
https://groups.google.com/a/chromium.org/forum/#!topic/blink-dev/5dNJaHFNFGM

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

[deepak.s, 2015-06-08 08:29:41]

deepak.s@samsung.com changed reviewers:
+ dominicc@chromium.org, philipj@opera.com, vivekg@chromium.org

[deepak.s, 2015-06-08 08:29:42]

PTAL?
Thanks!

[philipj_slow, 2015-06-08 10:12:28]

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

https://codereview.chromium.org/1149373003/diff/20001/Source/core/dom/Range.i...
Source/core/dom/Range.idl:81: const unsigned short NODE_BEFORE           = 0;
Please also remove these contants, as they are only used as the return values
for compareNode().

https://codereview.chromium.org/1149373003/diff/20001/Source/core/editing/htm...
File Source/core/editing/htmlediting.cpp (right):

https://codereview.chromium.org/1149373003/diff/20001/Source/core/editing/htm...
Source/core/editing/htmlediting.cpp:1105: bool nodeFullySelected =
Range::compareBoundaryPoints(node.parentNode(), node.nodeIndex(),
selectedRange.startContainer(), selectedRange.startOffset(), IGNORE_EXCEPTION)
>= 0
Could this be written as a static helper and used in both places where you had
to change the code?

https://codereview.chromium.org/1149373003/diff/20001/Source/core/frame/UseCo...
File Source/core/frame/UseCounter.cpp (left):

https://codereview.chromium.org/1149373003/diff/20001/Source/core/frame/UseCo...
Source/core/frame/UseCounter.cpp:851: case RangeCompareNode:
Also remove the entry from UseCounter.h

[deepak.s, 2015-06-08 11:44:03]

Thanks for the review, Philip!

I have updated the patch as per review comments.
Please have another look.
Thanks!

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

https://codereview.chromium.org/1149373003/diff/20001/Source/core/dom/Range.i...
Source/core/dom/Range.idl:81: const unsigned short NODE_BEFORE           = 0;
On 2015/06/08 10:12:27, philipj wrote:
> Please also remove these contants, as they are only used as the return values
> for compareNode().

Done.

https://codereview.chromium.org/1149373003/diff/20001/Source/core/editing/htm...
File Source/core/editing/htmlediting.cpp (right):

https://codereview.chromium.org/1149373003/diff/20001/Source/core/editing/htm...
Source/core/editing/htmlediting.cpp:1105: bool nodeFullySelected =
Range::compareBoundaryPoints(node.parentNode(), node.nodeIndex(),
selectedRange.startContainer(), selectedRange.startOffset(), IGNORE_EXCEPTION)
>= 0
On 2015/06/08 10:12:27, philipj wrote:
> Could this be written as a static helper and used in both places where you had
> to change the code?

Done.

https://codereview.chromium.org/1149373003/diff/20001/Source/core/frame/UseCo...
File Source/core/frame/UseCounter.cpp (left):

https://codereview.chromium.org/1149373003/diff/20001/Source/core/frame/UseCo...
Source/core/frame/UseCounter.cpp:851: case RangeCompareNode:
On 2015/06/08 10:12:27, philipj wrote:
> Also remove the entry from UseCounter.h

Done.

[philipj_slow, 2015-06-08 11:50:32]

https://codereview.chromium.org/1149373003/diff/40001/Source/core/dom/Range.h
File Source/core/dom/Range.h (right):

https://codereview.chromium.org/1149373003/diff/40001/Source/core/dom/Range.h...
Source/core/dom/Range.h:86: static bool isNodeFullySelected(Node*, const Range*
selectedRange);
Having seen what the resulting code is like, I would change my suggestion to a
non-static |bool isNodeFullyContained(Node&)|, since you have a range at both
call sites and the Node* is never null, right? This should make the call sites
slightly more readable, I think.

[deepak.s, 2015-06-11 04:04:23]

PTAL?
Thanks!

[philipj_slow, 2015-06-11 08:39:14]

LGTM % cosmetic changes

https://chromiumcodereview.appspot.com/1149373003/diff/80001/Source/core/dom/...
File Source/core/dom/Range.cpp (right):

https://chromiumcodereview.appspot.com/1149373003/diff/80001/Source/core/dom/...
Source/core/dom/Range.cpp:298: return
Range::compareBoundaryPoints(node.parentNode(), node.nodeIndex(),
startContainer(), startOffset(), IGNORE_EXCEPTION) >= 0
Now that this isn't static, could comparePoint() be used just like in the
now-removed compareNode()? Looks like that would make this more readable.

https://chromiumcodereview.appspot.com/1149373003/diff/80001/Source/core/dom/...
File Source/core/dom/Range.h (right):

https://chromiumcodereview.appspot.com/1149373003/diff/80001/Source/core/dom/...
Source/core/dom/Range.h:86: bool isNodeFullySelected(Node&) const;
I would still prefer the name isNodeFullyContained, as the Range interface
itself may be used for things other than selections.

[dominicc (has gone to gerrit), 2015-06-22 01:25:24]

Nice work, LGTM too. I agree with philipj's incisive review comments.

[deepak.s, 2015-06-25 10:52:37]

Updated the patch.
Apologies for the delay! Was Out of town for couple of weeks.

Please review the updated patch.
Thanks!

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

https://codereview.chromium.org/1149373003/diff/80001/Source/core/dom/Range.c...
Source/core/dom/Range.cpp:298: return
Range::compareBoundaryPoints(node.parentNode(), node.nodeIndex(),
startContainer(), startOffset(), IGNORE_EXCEPTION) >= 0
On 2015/06/11 08:39:14, philipj wrote:
> Now that this isn't static, could comparePoint() be used just like in the
> now-removed compareNode()? Looks like that would make this more readable.

Done.

https://codereview.chromium.org/1149373003/diff/80001/Source/core/dom/Range.h
File Source/core/dom/Range.h (right):

https://codereview.chromium.org/1149373003/diff/80001/Source/core/dom/Range.h...
Source/core/dom/Range.h:86: bool isNodeFullySelected(Node&) const;
On 2015/06/11 08:39:14, philipj wrote:
> I would still prefer the name isNodeFullyContained, as the Range interface
> itself may be used for things other than selections.

Done.

[deepak.s, 2015-06-30 06:46:58]

Ping!

[philipj_slow, 2015-07-01 08:22:01]

Sorry for the delay, many reviews going on. With a fresh look at this, I noticed
some problems around exception handling.

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

https://codereview.chromium.org/1149373003/diff/100001/Source/core/dom/Range....
Source/core/dom/Range.cpp:299: int nodeIndex = node.nodeIndex();
Move this down to after the parentNode check, just before it is used.

https://codereview.chromium.org/1149373003/diff/100001/Source/core/dom/Range....
Source/core/dom/Range.cpp:302: exceptionState.throwDOMException(NotFoundError,
"The node provided has no parent.");
This exception is ignored on both call sites, so just remove the ExceptionState&
argument and return false here.

https://codereview.chromium.org/1149373003/diff/100001/Source/core/dom/Range....
Source/core/dom/Range.cpp:306: if (comparePoint(parentNode, nodeIndex,
exceptionState) == 0 // starts in the middle of this range, or on the boundary
points
To actually handle these exceptions, you'll have to add
exceptionState.hadException() after each comparePoint() call. I'd say just make
this a big return statement like in Range::isPointInRange.

https://codereview.chromium.org/1149373003/diff/100001/Source/core/dom/Range.h
File Source/core/dom/Range.h (right):

https://codereview.chromium.org/1149373003/diff/100001/Source/core/dom/Range....
Source/core/dom/Range.h:86: bool isNodeFullyContained(Node&, ExceptionState&)
const;
I'd move this up to before isPointInRange, it's odd to have it in the middle of
the compareBoundaryPoints block of methods.

[deepak.s, 2015-07-07 05:41:32]

PTAL?
Thanks!

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

https://codereview.chromium.org/1149373003/diff/100001/Source/core/dom/Range....
Source/core/dom/Range.cpp:299: int nodeIndex = node.nodeIndex();
On 2015/07/01 08:22:01, philipj wrote:
> Move this down to after the parentNode check, just before it is used.

Done.

https://codereview.chromium.org/1149373003/diff/100001/Source/core/dom/Range....
Source/core/dom/Range.cpp:302: exceptionState.throwDOMException(NotFoundError,
"The node provided has no parent.");
On 2015/07/01 08:22:01, philipj wrote:
> This exception is ignored on both call sites, so just remove the
ExceptionState&
> argument and return false here.

Done.

https://codereview.chromium.org/1149373003/diff/100001/Source/core/dom/Range....
Source/core/dom/Range.cpp:306: if (comparePoint(parentNode, nodeIndex,
exceptionState) == 0 // starts in the middle of this range, or on the boundary
points
On 2015/07/01 08:22:01, philipj wrote:
> To actually handle these exceptions, you'll have to add
> exceptionState.hadException() after each comparePoint() call. I'd say just
make
> this a big return statement like in Range::isPointInRange.

Removed exception handling and passed IGNORE_EXCEPTION.
Made it as one big return as well.

https://codereview.chromium.org/1149373003/diff/100001/Source/core/dom/Range.h
File Source/core/dom/Range.h (right):

https://codereview.chromium.org/1149373003/diff/100001/Source/core/dom/Range....
Source/core/dom/Range.h:86: bool isNodeFullyContained(Node&, ExceptionState&)
const;
On 2015/07/01 08:22:01, philipj wrote:
> I'd move this up to before isPointInRange, it's odd to have it in the middle
of
> the compareBoundaryPoints block of methods.

Done.

[philipj_slow, 2015-07-07 08:27:29]

OK, just a small problem with isNodeFullyContained left and then this should be
good to go.

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

https://codereview.chromium.org/1149373003/diff/100001/Source/core/dom/Range....
Source/core/dom/Range.cpp:306: if (comparePoint(parentNode, nodeIndex,
exceptionState) == 0 // starts in the middle of this range, or on the boundary
points
On 2015/07/07 05:41:32, deepak.s wrote:
> On 2015/07/01 08:22:01, philipj wrote:
> > To actually handle these exceptions, you'll have to add
> > exceptionState.hadException() after each comparePoint() call. I'd say just
> make
> > this a big return statement like in Range::isPointInRange.
> 
> Removed exception handling and passed IGNORE_EXCEPTION.
> Made it as one big return as well.

There are exception cases in Range::comparePoint that return 0 that will now be
treated as the node being fully contained, you would need
exceptionState.hadException() interleaved in the return statement, similar to in
Range::isPointInRange.

Alternatively, you could simply use isPointInRange, which already returns false
in all exception cases, making this method simply:

bool Range::isNodeFullyContained(Node& node) const
{
    ContainerNode* parentNode = node.parentNode();
    int nodeIndex = node.nodeIndex();
    return isPointInRange(parentNode, nodeIndex, IGNORE_EXCEPTION)
        && isPointInRange(parentNode, nodeIndex + 1, IGNORE_EXCEPTION);
}

You'll have to make Range::isPointInRange const for it to compile.

Please also move it to before Range::isPointInRange to match the header and to
make it easier to see read both together.

[deepak.s, 2015-07-08 04:57:02]

Patch Updated.
PTAL? Thanks!

https://chromiumcodereview.appspot.com/1149373003/diff/100001/Source/core/dom...
File Source/core/dom/Range.cpp (right):

https://chromiumcodereview.appspot.com/1149373003/diff/100001/Source/core/dom...
Source/core/dom/Range.cpp:306: if (comparePoint(parentNode, nodeIndex,
exceptionState) == 0 // starts in the middle of this range, or on the boundary
points
On 2015/07/07 08:27:29, philipj wrote:
> On 2015/07/07 05:41:32, deepak.s wrote:
> > On 2015/07/01 08:22:01, philipj wrote:
> > > To actually handle these exceptions, you'll have to add
> > > exceptionState.hadException() after each comparePoint() call. I'd say just
> > make
> > > this a big return statement like in Range::isPointInRange.
> > 
> > Removed exception handling and passed IGNORE_EXCEPTION.
> > Made it as one big return as well.
> 
> There are exception cases in Range::comparePoint that return 0 that will now
be
> treated as the node being fully contained, you would need
> exceptionState.hadException() interleaved in the return statement, similar to
in
> Range::isPointInRange.
> 
> Alternatively, you could simply use isPointInRange, which already returns
false
> in all exception cases, making this method simply:
> 
> bool Range::isNodeFullyContained(Node& node) const
> {
>     ContainerNode* parentNode = node.parentNode();
>     int nodeIndex = node.nodeIndex();
>     return isPointInRange(parentNode, nodeIndex, IGNORE_EXCEPTION)
>         && isPointInRange(parentNode, nodeIndex + 1, IGNORE_EXCEPTION);
> }
> 
> You'll have to make Range::isPointInRange const for it to compile.
> 
> Please also move it to before Range::isPointInRange to match the header and to
> make it easier to see read both together.

Updated method by using isPointInRange.

[philipj_slow, 2015-07-08 08:19:38]

The CQ bit was checked by philipj@opera.com

[philipj_slow, 2015-07-08 08:19:38]

lgtm

All done, thanks for suffering through my many comments!

[philipj_slow, 2015-07-08 08:19:39]

The patchset sent to the CQ was uploaded after l-g-t-m from
dominicc@chromium.org
Link to the patchset:
https://chromiumcodereview.appspot.com/1149373003/#ps140001 (title: " ")

[commit-bot: I haz the power, 2015-07-08 08:20:03]

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

[commit-bot: I haz the power, 2015-07-08 09:25:08]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-07-08 09:25:10]

Try jobs failed on following builders:
  win_blink_rel on tryserver.blink (JOB_FAILED,
http://build.chromium.org/p/tryserver.blink/builders/win_blink_rel/builds/69671)

(exceeded global retry quota)

[philipj_slow, 2015-07-08 09:56:39]

On 2015/07/08 09:25:10, commit-bot: I haz the power wrote:
> Try jobs failed on following builders:
>   win_blink_rel on tryserver.blink (JOB_FAILED,
>
http://build.chromium.org/p/tryserver.blink/builders/win_blink_rel/builds/69671)
> 
> (exceeded global retry quota)

webexposed/global-interface-listing.html strikes again :)

[deepak.s, 2015-07-09 06:57:47]

The CQ bit was checked by deepak.s@samsung.com

[deepak.s, 2015-07-09 06:57:48]

The patchset sent to the CQ was uploaded after l-g-t-m from philipj@opera.com,
dominicc@chromium.org
Link to the patchset: https://codereview.chromium.org/1149373003/#ps160001
(title: "updated webexposed/global-interface-listing.html")

[commit-bot: I haz the power, 2015-07-09 06:58:19]

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

[commit-bot: I haz the power, 2015-07-09 08:02:39]

Committed patchset #9 (id:160001) as
https://src.chromium.org/viewvc/blink?view=rev&revision=198584

[deepak.s, 2015-07-09 08:21:31]

On 2015/07/09 08:02:39, commit-bot: I haz the power wrote:
> Committed patchset #9 (id:160001) as
> https://src.chromium.org/viewvc/blink?view=rev&revision=198584

Finally!
Thanks for your valuable comments :)