Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(697)

Issue 1149373003: Remove Range.compareNode() as it is not in the spec (Closed)

Created:
5 years, 7 months ago by deepak.s
Modified:
5 years, 5 months ago
CC:
blink-reviews, arv+blink, vivekg, sof, eae+blinkwatch, blink-reviews-dom_chromium.org, dglazkov+blink, blink-reviews-events_chromium.org, vivekg_samsung, Inactive, rwlbuis
Base URL:
https://chromium.googlesource.com/chromium/blink.git@master
Target Ref:
refs/heads/master
Project:
blink
Visibility:
Public.

Description

Patch Set 1 #

Patch Set 2 : layout tests changed #

Total comments: 6

Patch Set 3 : Addressing comments #

Total comments: 1

Patch Set 4 : removed static #

Patch Set 5 : rebased #

Total comments: 4

Patch Set 6 : updated isNodeFullyContained #

Total comments: 10

Patch Set 7 : Exception handling updated #

Patch Set 8 : #

Patch Set 9 : updated webexposed/global-interface-listing.html #

Unified diffs Side-by-side diffs Delta from patch set Stats (+19 lines, -348 lines) Patch
M LayoutTests/fast/dom/Range/invalid-arguments.html View 1 1 chunk +0 lines, -1 line 0 comments Download
M LayoutTests/fast/dom/Range/invalid-arguments-expected.txt View 1 1 chunk +0 lines, -2 lines 0 comments Download
D LayoutTests/fast/dom/Range/range-compareNode.html View 1 1 chunk +0 lines, -240 lines 0 comments Download
D LayoutTests/fast/dom/Range/range-compareNode-expected.txt View 1 1 chunk +0 lines, -22 lines 0 comments Download
M LayoutTests/platform/android/virtual/stable/webexposed/global-interface-listing-expected.txt View 1 2 3 4 5 6 7 8 2 chunks +0 lines, -5 lines 0 comments Download
M LayoutTests/platform/android/webexposed/global-interface-listing-expected.txt View 1 2 3 4 5 6 7 8 2 chunks +0 lines, -5 lines 0 comments Download
M LayoutTests/virtual/stable/webexposed/global-interface-listing-expected.txt View 1 2 3 4 5 6 7 8 2 chunks +0 lines, -5 lines 0 comments Download
M LayoutTests/webexposed/global-interface-listing-expected.txt View 1 2 3 4 5 6 7 8 2 chunks +0 lines, -5 lines 0 comments Download
M Source/core/dom/Range.h View 1 2 3 4 5 6 7 1 chunk +2 lines, -2 lines 0 comments Download
M Source/core/dom/Range.cpp View 1 2 3 4 5 6 7 2 chunks +9 lines, -44 lines 0 comments Download
M Source/core/dom/Range.idl View 1 2 1 chunk +1 line, -6 lines 0 comments Download
M Source/core/editing/htmlediting.cpp View 1 2 3 4 5 6 1 chunk +1 line, -2 lines 0 comments Download
M Source/core/frame/UseCounter.h View 1 2 3 4 5 6 7 1 chunk +0 lines, -1 line 0 comments Download
M Source/core/frame/UseCounter.cpp View 1 2 3 4 5 6 7 1 chunk +0 lines, -3 lines 0 comments Download
M Source/core/input/EventHandler.cpp View 1 2 3 4 5 6 7 1 chunk +6 lines, -5 lines 0 comments Download

Messages

Total messages: 26 (6 generated)
deepak.s
PTAL? Thanks!
5 years, 6 months ago (2015-06-08 08:29:42 UTC) #2
philipj_slow
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.idl#newcode81 Source/core/dom/Range.idl:81: const unsigned short NODE_BEFORE = 0; Please also remove ...
5 years, 6 months ago (2015-06-08 10:12:28 UTC) #3
deepak.s
Thanks for the review, Philip! I have updated the patch as per review comments. Please ...
5 years, 6 months ago (2015-06-08 11:44:03 UTC) #4
philipj_slow
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#newcode86 Source/core/dom/Range.h:86: static bool isNodeFullySelected(Node*, const Range* selectedRange); Having seen what ...
5 years, 6 months ago (2015-06-08 11:50:32 UTC) #5
deepak.s
PTAL? Thanks!
5 years, 6 months ago (2015-06-11 04:04:23 UTC) #6
philipj_slow
LGTM % cosmetic changes https://chromiumcodereview.appspot.com/1149373003/diff/80001/Source/core/dom/Range.cpp File Source/core/dom/Range.cpp (right): https://chromiumcodereview.appspot.com/1149373003/diff/80001/Source/core/dom/Range.cpp#newcode298 Source/core/dom/Range.cpp:298: return Range::compareBoundaryPoints(node.parentNode(), node.nodeIndex(), startContainer(), startOffset(), ...
5 years, 6 months ago (2015-06-11 08:39:14 UTC) #7
dominicc (has gone to gerrit)
Nice work, LGTM too. I agree with philipj's incisive review comments.
5 years, 6 months ago (2015-06-22 01:25:24 UTC) #8
deepak.s
Updated the patch. Apologies for the delay! Was Out of town for couple of weeks. ...
5 years, 6 months ago (2015-06-25 10:52:37 UTC) #9
deepak.s
Ping!
5 years, 5 months ago (2015-06-30 06:46:58 UTC) #10
philipj_slow
Sorry for the delay, many reviews going on. With a fresh look at this, I ...
5 years, 5 months ago (2015-07-01 08:22:01 UTC) #11
deepak.s
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.cpp#newcode299 Source/core/dom/Range.cpp:299: int nodeIndex = node.nodeIndex(); On 2015/07/01 08:22:01, ...
5 years, 5 months ago (2015-07-07 05:41:32 UTC) #12
philipj_slow
OK, just a small problem with isNodeFullyContained left and then this should be good to ...
5 years, 5 months ago (2015-07-07 08:27:29 UTC) #13
deepak.s
Patch Updated. PTAL? Thanks! https://chromiumcodereview.appspot.com/1149373003/diff/100001/Source/core/dom/Range.cpp File Source/core/dom/Range.cpp (right): https://chromiumcodereview.appspot.com/1149373003/diff/100001/Source/core/dom/Range.cpp#newcode306 Source/core/dom/Range.cpp:306: if (comparePoint(parentNode, nodeIndex, exceptionState) == ...
5 years, 5 months ago (2015-07-08 04:57:02 UTC) #14
philipj_slow
lgtm All done, thanks for suffering through my many comments!
5 years, 5 months ago (2015-07-08 08:19:38 UTC) #16
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1149373003/140001
5 years, 5 months ago (2015-07-08 08:20:03 UTC) #18
commit-bot: I haz the power
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)
5 years, 5 months ago (2015-07-08 09:25:10 UTC) #20
philipj_slow
On 2015/07/08 09:25:10, commit-bot: I haz the power wrote: > Try jobs failed on following ...
5 years, 5 months ago (2015-07-08 09:56:39 UTC) #21
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1149373003/160001
5 years, 5 months ago (2015-07-09 06:58:19 UTC) #24
commit-bot: I haz the power
Committed patchset #9 (id:160001) as https://src.chromium.org/viewvc/blink?view=rev&revision=198584
5 years, 5 months ago (2015-07-09 08:02:39 UTC) #25
deepak.s
5 years, 5 months ago (2015-07-09 08:21:31 UTC) #26
Message was sent while issue was closed.
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 :)

Powered by Google App Engine
This is Rietveld 408576698