Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2900573002:
[DMC #12] Move some method impls from DocumentMarkerController.cpp to final place (Closed)
DescriptionMove some method impls from DocumentMarkerController.cpp to final place
In https://codereview.chromium.org/2896703003, I'm splitting off some code in
some DocumentMarkerController methods into new methods in
TextMatchMarkerListImpl, but leaving the implementations of the new methods in
DocumentMarkerController.cpp to make that CL smaller. In this CL, I'm moving
these implementations to their final location in TextMatchMarkerListImpl.cpp.
I'm also combining the static method UpdateMarkerRenderedRect() in
DocumentMarkerController into
TextMatchMarkerListImpl::UpdateMarkerRenderedRectIfNeeded().
BUG=707867
Review-Url: https://codereview.chromium.org/2900573002
Cr-Commit-Position: refs/heads/master@{#474599}
Committed: https://chromium.googlesource.com/chromium/src/+/746e2408f9f35f9acc8d9ecb991e1de9be729db8
Patch Set 1 #Patch Set 2 : Rebase #Patch Set 3 : Rebase #Patch Set 4 : Rebase #
Total comments: 2
Patch Set 5 : Rebase, fix variable naming #Patch Set 6 : Fix rebase #
Depends on Patchset: Dependent Patchsets: Messages
Total messages: 40 (31 generated)
Description was changed from ========== Move some method impls from DocumentMarkerController.cpp to final place In https://codereview.chromium.org/2896703003, I'm splitting off some code in some DocumentMarkerController methods into new methods in TextMatchMarkerListImpl, but leaving the implementations of the new methods in DocumentMarkerController.cpp to make that CL smaller. In this CL, I'm moving these implementations to their final location in TextMatchMarkerListImpl.cpp. BUG=707867 ========== to ========== Move some method impls from DocumentMarkerController.cpp to final place In https://codereview.chromium.org/2896703003, I'm splitting off some code in some DocumentMarkerController methods into new methods in TextMatchMarkerListImpl, but leaving the implementations of the new methods in DocumentMarkerController.cpp to make that CL smaller. In this CL, I'm moving these implementations to their final location in TextMatchMarkerListImpl.cpp. I'm also combining the static method UpdateMarkerRenderedRect() in DocumentMarkerController into TextMatchMarkerListImpl::UpdateMarkerRenderedRectIfNeeded(). BUG=707867 ==========
rlanday@chromium.org changed reviewers: + xiaochengh@chromium.org, yosin@chromium.org
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...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
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...
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-device-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-xcode-...) 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_...)
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...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: 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...)
https://codereview.chromium.org/2900573002/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/editing/markers/TextMatchMarkerListImpl.cpp (right): https://codereview.chromium.org/2900573002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/editing/markers/TextMatchMarkerListImpl.cpp:81: const Position startPosition(&const_cast<Node&>(node), marker.StartOffset()); Is it useful to have DomcumentMaker::AsRange(const Node&) -> EphemeralRange? WDYT?
https://codereview.chromium.org/2900573002/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/editing/markers/TextMatchMarkerListImpl.cpp (right): https://codereview.chromium.org/2900573002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/editing/markers/TextMatchMarkerListImpl.cpp:81: const Position startPosition(&const_cast<Node&>(node), marker.StartOffset()); On 2017/05/24 at 09:21:50, yosin_UTC9 wrote: > Is it useful to have DomcumentMaker::AsRange(const Node&) -> EphemeralRange? > WDYT? Might be useful in a few places. I still wish DocumentMarker knew which Node it was associated with so then we could do stuff like have AsRange() without having to pass a Node, and have MarkersInRange() with a clean API. But I suppose it might not be worth the added memory usage/code complexity when moving DocumentMarkers between Nodes. Also, I just noticed, in this code I'm moving, I think we should have start_position and end_position, not startPosition and endPosition
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...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: ios-device-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-xcode-...) 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_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
The CQ bit was checked by rlanday@chromium.org to run a CQ dry run
Updated
Dry run: 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
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-device-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-xcode-...) ios-simulator on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...) 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_...)
On 2017/05/24 at 20:25:06, rlanday wrote: > https://codereview.chromium.org/2900573002/diff/60001/third_party/WebKit/Sour... > File third_party/WebKit/Source/core/editing/markers/TextMatchMarkerListImpl.cpp (right): > > https://codereview.chromium.org/2900573002/diff/60001/third_party/WebKit/Sour... > third_party/WebKit/Source/core/editing/markers/TextMatchMarkerListImpl.cpp:81: const Position startPosition(&const_cast<Node&>(node), marker.StartOffset()); > On 2017/05/24 at 09:21:50, yosin_UTC9 wrote: > > Is it useful to have DomcumentMaker::AsRange(const Node&) -> EphemeralRange? > > WDYT? > > Might be useful in a few places. I still wish DocumentMarker knew which Node it was associated with so then we could do stuff like have AsRange() without having to pass a Node, and have MarkersInRange() with a clean API. But I suppose it might not be worth the added memory usage/code complexity when moving DocumentMarkers between Nodes. > Actually, I don't think this would be that helpful. The only other places I can find where we need to construct a range are: https://chromium.googlesource.com/chromium/src/+/d541129f1b272eb3e611b59b5e12... (really we need two Positions here) https://chromium.googlesource.com/chromium/src/+/a34d0750b0cd0db8bdfef1e6a5f5... (really we need a Range here, not an EphemeralRange) https://chromium.googlesource.com/chromium/src/+/3639139a6216af6f66840f834862... So...probably not that helpful :-/
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...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by yosin@chromium.org
lgtm
The CQ bit was unchecked by commit-bot@chromium.org
This CL has an open dependency (Issue 2896703003 Patch 120001). 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
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": 1495696424088660,
"parent_rev": "58a3e83e1325c8a366941564ca8ce096bb1345df", "commit_rev":
"746e2408f9f35f9acc8d9ecb991e1de9be729db8"}
Message was sent while issue was closed.
Description was changed from ========== Move some method impls from DocumentMarkerController.cpp to final place In https://codereview.chromium.org/2896703003, I'm splitting off some code in some DocumentMarkerController methods into new methods in TextMatchMarkerListImpl, but leaving the implementations of the new methods in DocumentMarkerController.cpp to make that CL smaller. In this CL, I'm moving these implementations to their final location in TextMatchMarkerListImpl.cpp. I'm also combining the static method UpdateMarkerRenderedRect() in DocumentMarkerController into TextMatchMarkerListImpl::UpdateMarkerRenderedRectIfNeeded(). BUG=707867 ========== to ========== Move some method impls from DocumentMarkerController.cpp to final place In https://codereview.chromium.org/2896703003, I'm splitting off some code in some DocumentMarkerController methods into new methods in TextMatchMarkerListImpl, but leaving the implementations of the new methods in DocumentMarkerController.cpp to make that CL smaller. In this CL, I'm moving these implementations to their final location in TextMatchMarkerListImpl.cpp. I'm also combining the static method UpdateMarkerRenderedRect() in DocumentMarkerController into TextMatchMarkerListImpl::UpdateMarkerRenderedRectIfNeeded(). BUG=707867 Review-Url: https://codereview.chromium.org/2900573002 Cr-Commit-Position: refs/heads/master@{#474599} Committed: https://chromium.googlesource.com/chromium/src/+/746e2408f9f35f9acc8d9ecb991e... ==========
Message was sent while issue was closed.
Committed patchset #6 (id:100001) as https://chromium.googlesource.com/chromium/src/+/746e2408f9f35f9acc8d9ecb991e... |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
