Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2812723002:
Add some tests for DocumentMarkerController::removeMarkers() (Closed)
DescriptionAdd some tests for DocumentMarkerController::removeMarkers()
I'm changing the behavior of removeMarkers() in https://codereview.chromium.org/2806683002 to always remove markers rather than splitting and noticed that the current behavior doesn't seem to be covered by any tests; I'm adding tests for the current behavior to make it clear in that CL how the behavior is being changed.
BUG=707867
Review-Url: https://codereview.chromium.org/2812723002
Cr-Commit-Position: refs/heads/master@{#463899}
Committed: https://chromium.googlesource.com/chromium/src/+/5e71603c48cc0898a933e13d266dbf036d034c42
Patch Set 1 #
Total comments: 1
Patch Set 2 : Don't use PlainTextRange #
Total comments: 3
Patch Set 3 : Remove dependency #Patch Set 4 : Remove dependency #Patch Set 5 : Actually remove dependency #Patch Set 6 : Actually actually remove dependency? #Messages
Total messages: 27 (15 generated)
Description was changed from ========== Add some tests for DocumentMarkerController::removeMarkers() BUG=707867 ========== to ========== Add some tests for DocumentMarkerController::removeMarkers() I'm changing the behavior of removeMarkers() in https://codereview.chromium.org/2806683002 to always remove markers rather than splitting and noticed that the current behavior doesn't seem to be covered by any tests; I'm adding tests for the current behavior to make it clear in that CL how the behavior is being changed. BUG=707867 ==========
rlanday@chromium.org changed reviewers: + xiaochengh@chromium.org, yosin@chromium.org
Test cases themselves are fine. Btw, could you add line breaks to the patch description, so that it's easier to read in terminal. https://codereview.chromium.org/2812723002/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/editing/markers/DocumentMarkerControllerTest.cpp (right): https://codereview.chromium.org/2812723002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/editing/markers/DocumentMarkerControllerTest.cpp:39: #include "core/editing/PlainTextRange.h" Could you set marker ranges directly, so that (at least) the test cases do not depend on text iterator? Though DMC::AddFooMarker depends on text iterator, it's another issue.
On 2017/04/10 at 21:32:28, xiaochengh wrote: > Test cases themselves are fine. > > Btw, could you add line breaks to the patch description, so that it's easier to read in terminal. > > https://codereview.chromium.org/2812723002/diff/1/third_party/WebKit/Source/c... > File third_party/WebKit/Source/core/editing/markers/DocumentMarkerControllerTest.cpp (right): > > https://codereview.chromium.org/2812723002/diff/1/third_party/WebKit/Source/c... > third_party/WebKit/Source/core/editing/markers/DocumentMarkerControllerTest.cpp:39: #include "core/editing/PlainTextRange.h" > Could you set marker ranges directly, so that (at least) the test cases do not depend on text iterator? > > Though DMC::AddFooMarker depends on text iterator, it's another issue. Updated
On 2017/04/10 at 21:32:28, xiaochengh wrote: > Test cases themselves are fine. > > Btw, could you add line breaks to the patch description, so that it's easier to read in terminal. > > https://codereview.chromium.org/2812723002/diff/1/third_party/WebKit/Source/c... > File third_party/WebKit/Source/core/editing/markers/DocumentMarkerControllerTest.cpp (right): > > https://codereview.chromium.org/2812723002/diff/1/third_party/WebKit/Source/c... > third_party/WebKit/Source/core/editing/markers/DocumentMarkerControllerTest.cpp:39: #include "core/editing/PlainTextRange.h" > Could you set marker ranges directly, so that (at least) the test cases do not depend on text iterator? > > Though DMC::AddFooMarker depends on text iterator, it's another issue. Updated
https://codereview.chromium.org/2812723002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/editing/markers/DocumentMarkerControllerTest.cpp (right): https://codereview.chromium.org/2812723002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/editing/markers/DocumentMarkerControllerTest.cpp:33: #include <memory> This is a git cl format thing...
lgtm
The CQ bit was checked by yosin@chromium.org
lgtm https://codereview.chromium.org/2812723002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/editing/markers/DocumentMarkerControllerTest.cpp (right): https://codereview.chromium.org/2812723002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/editing/markers/DocumentMarkerControllerTest.cpp:33: #include <memory> On 2017/04/10 at 22:39:43, rlanday wrote: > This is a git cl format thing... Yes, we need to put standard C/C++ header before user include. https://codereview.chromium.org/2812723002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/editing/markers/DocumentMarkerControllerTest.cpp:293: Node* b_element = GetDocument().body()->FirstChild(); Wow, we have both Node::firstChild() and ContainerNode::FirstChild()!
The CQ bit was unchecked by commit-bot@chromium.org
This CL has an open dependency (Issue 2755013004 Patch 380001). Please resolve the dependency and try again. If you are sure that there is no real dependency, please use one of the options listed in https://goo.gl/9Es4OR to land the CL.
The CQ bit was checked by rlanday@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from yosin@chromium.org, xiaochengh@chromium.org Link to the patchset: https://codereview.chromium.org/2812723002/#ps40001 (title: "Remove dependency")
The CQ bit was unchecked by commit-bot@chromium.org
This CL has an open dependency (Issue 2755013004 Patch 380001). Please resolve the dependency and try again. If you are sure that there is no real dependency, please use one of the options listed in https://goo.gl/9Es4OR to land the CL.
The CQ bit was checked by rlanday@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from yosin@chromium.org, xiaochengh@chromium.org Link to the patchset: https://codereview.chromium.org/2812723002/#ps80001 (title: "Actually remove dependency")
The CQ bit was unchecked by commit-bot@chromium.org
This CL has an open dependency (Issue 2723663002 Patch 1). Please resolve the dependency and try again. If you are sure that there is no real dependency, please use one of the options listed in https://goo.gl/9Es4OR to land the CL.
The CQ bit was checked by rlanday@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from yosin@chromium.org, xiaochengh@chromium.org Link to the patchset: https://codereview.chromium.org/2812723002/#ps100001 (title: "Actually actually remove dependency?")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 100001, "attempt_start_ts": 1491959277596550,
"parent_rev": "8df84fca90d561741cde937b5cd995fe0e1ef7dd", "commit_rev":
"5e71603c48cc0898a933e13d266dbf036d034c42"}
CQ is committing da patch.
Bot data: {"patchset_id": 100001, "attempt_start_ts": 1491959277596550,
"parent_rev": "8df84fca90d561741cde937b5cd995fe0e1ef7dd", "commit_rev":
"5e71603c48cc0898a933e13d266dbf036d034c42"}
Message was sent while issue was closed.
Description was changed from ========== Add some tests for DocumentMarkerController::removeMarkers() I'm changing the behavior of removeMarkers() in https://codereview.chromium.org/2806683002 to always remove markers rather than splitting and noticed that the current behavior doesn't seem to be covered by any tests; I'm adding tests for the current behavior to make it clear in that CL how the behavior is being changed. BUG=707867 ========== to ========== Add some tests for DocumentMarkerController::removeMarkers() I'm changing the behavior of removeMarkers() in https://codereview.chromium.org/2806683002 to always remove markers rather than splitting and noticed that the current behavior doesn't seem to be covered by any tests; I'm adding tests for the current behavior to make it clear in that CL how the behavior is being changed. BUG=707867 Review-Url: https://codereview.chromium.org/2812723002 Cr-Commit-Position: refs/heads/master@{#463899} Committed: https://chromium.googlesource.com/chromium/src/+/5e71603c48cc0898a933e13d266d... ==========
Message was sent while issue was closed.
Committed patchset #6 (id:100001) as https://chromium.googlesource.com/chromium/src/+/5e71603c48cc0898a933e13d266d... |
