Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2916463002:
Refactor DocumentMarkerController::Add*Marker() methods (Closed)
DescriptionRefactor DocumentMarkerController::Add*Marker() methods
Currently there's a lot of duplicated code in all the MarkerType-specific
DocumentMarkerController::Add*Marker() methods. This CL factors out the common
logic into an AddMarkerInternal() method, and renames the existing (private)
AddMarker() method to AddMarkerToNode().
This CL also adds some filtering to avoid constructing DocumentMarkers with
start/end offsets that are the same (the current version of the code constructs
these DocumentMarkers, then filters them out before actually adding them), or
when the range returned by TextIterator is not a text node (we don't currently
do this filtering).
BUG=707867
Review-Url: https://codereview.chromium.org/2916463002
Cr-Commit-Position: refs/heads/master@{#475840}
Committed: https://chromium.googlesource.com/chromium/src/+/68adbbbf1a52544dbd52c2248e530f722d638b34
Patch Set 1 #
Total comments: 2
Patch Set 2 : Rebase, fix comment #
Total comments: 2
Patch Set 3 : Add const #Patch Set 4 : Rebase #
Messages
Total messages: 28 (16 generated)
The CQ bit was checked by rlanday@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
rlanday@chromium.org changed reviewers: + xiaochengh@chromium.org, yosin@chromium.org
The filtering in this change was requested by xiaochengh@ in https://codereview.chromium.org/2904093002#msg19
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: ios-device on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...) ios-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...) mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...) mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
Thanks for the patch! https://codereview.chromium.org/2916463002/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/editing/markers/DocumentMarkerController.cpp (right): https://codereview.chromium.org/2916463002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/editing/markers/DocumentMarkerController.cpp:215: // TODO(editing-dev): TextIterator sometimes emits ranges where the current Emitting text with non-text container is legit. For example: <div>foo</div><div>bar</div> where a "\n" is emitted between the two <div>s. Could you make the comment mention the equal-offset issue?
https://codereview.chromium.org/2916463002/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/editing/markers/DocumentMarkerController.cpp (right): https://codereview.chromium.org/2916463002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/editing/markers/DocumentMarkerController.cpp:215: // TODO(editing-dev): TextIterator sometimes emits ranges where the current On 2017/05/30 at 23:51:19, Xiaocheng wrote: > Emitting text with non-text container is legit. For example: > > <div>foo</div><div>bar</div> > > where a "\n" is emitted between the two <div>s. > > Could you make the comment mention the equal-offset issue? It does mention the equal-offset issue; I will update the other part
The CQ bit was checked by rlanday@chromium.org to run a CQ dry run
Updated (rebased, changed comment)
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
lgtm, thanks!
lgtm w/ nits Thanks for doing this! https://codereview.chromium.org/2916463002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/editing/markers/DocumentMarkerController.cpp (right): https://codereview.chromium.org/2916463002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/editing/markers/DocumentMarkerController.cpp:201: for (TextIterator marked_text(range.StartPosition(), range.EndPosition()); FYI: It is nice that we have TextIterator::begin()/end(). https://codereview.chromium.org/2916463002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/editing/markers/DocumentMarkerController.cpp:219: Node* node = marked_text.CurrentContainer(); nit: s/Node*/Node* const/
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/2916463002/#ps40001 (title: "Add const")
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
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/2916463002/#ps60001 (title: "Rebase")
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
The CQ bit was checked by rlanday@chromium.org
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": 60001, "attempt_start_ts": 1496213246658950,
"parent_rev": "0c0fcbe9055460e377217cfe6694f8ce1a2d4eeb", "commit_rev":
"68adbbbf1a52544dbd52c2248e530f722d638b34"}
Message was sent while issue was closed.
Description was changed from ========== Refactor DocumentMarkerController::Add*Marker() methods Currently there's a lot of duplicated code in all the MarkerType-specific DocumentMarkerController::Add*Marker() methods. This CL factors out the common logic into an AddMarkerInternal() method, and renames the existing (private) AddMarker() method to AddMarkerToNode(). This CL also adds some filtering to avoid constructing DocumentMarkers with start/end offsets that are the same (the current version of the code constructs these DocumentMarkers, then filters them out before actually adding them), or when the range returned by TextIterator is not a text node (we don't currently do this filtering). BUG=707867 ========== to ========== Refactor DocumentMarkerController::Add*Marker() methods Currently there's a lot of duplicated code in all the MarkerType-specific DocumentMarkerController::Add*Marker() methods. This CL factors out the common logic into an AddMarkerInternal() method, and renames the existing (private) AddMarker() method to AddMarkerToNode(). This CL also adds some filtering to avoid constructing DocumentMarkers with start/end offsets that are the same (the current version of the code constructs these DocumentMarkers, then filters them out before actually adding them), or when the range returned by TextIterator is not a text node (we don't currently do this filtering). BUG=707867 Review-Url: https://codereview.chromium.org/2916463002 Cr-Commit-Position: refs/heads/master@{#475840} Committed: https://chromium.googlesource.com/chromium/src/+/68adbbbf1a52544dbd52c2248e53... ==========
Message was sent while issue was closed.
Committed patchset #4 (id:60001) as https://chromium.googlesource.com/chromium/src/+/68adbbbf1a52544dbd52c2248e53... |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
