Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2905753002:
[DMC #17] Make TextMatchMarkers constructible in single step (Closed)
Description[Reland] Make TextMatchMarkers constructible in single step
Currently, we only construct instances of TextMatchMarker (previously
RenderedDocumentMarker) inside TextMatchMarkerListImpl, by calling
TextMatchMarker::Create() on an instance of DocumentMarker. This CL changes this
so callers directly construct instances of TextMatchMarker. This sets us up to
eventually move the storage and accessor methods for whether or not the marker
is active out of DocumentMarker and into TextMatchMarker.
BUG=707867
Review-Url: https://codereview.chromium.org/2905753002
Cr-Original-Commit-Position: refs/heads/master@{#475724}
Committed: https://chromium.googlesource.com/chromium/src/+/6b0a494122ebe2fc48099a9549b0acbf0b28571d
Review-Url: https://codereview.chromium.org/2905753002
Cr-Commit-Position: refs/heads/master@{#475781}
Committed: https://chromium.googlesource.com/chromium/src/+/be1de5ecae843ecb1fe73577a5f6fb4e51c3f529
Patch Set 1 #Patch Set 2 : Rebase #Patch Set 3 : Rebase #
Total comments: 2
Patch Set 4 : Rebase #Patch Set 5 : Fix build error #
Total comments: 1
Messages
Total messages: 51 (31 generated)
Description was changed from ========== ere# Enter a description of the change. Make TextMatchMarkers constructible in single step Currently, we only construct instances of TextMatchMarker (previously RenderedDocumentMarker) inside TextMatchMarkerListImpl, by calling TextMatchMarker::Create() on an instance of DocumentMarker. This CL changes this so callers directly construct instances of TextMatchMarker. This sets us up to eventually move the storage and accessor methods for whether or not the marker is active out of DocumentMarker and into TextMatchMarker. BUG=707867 ========== to ========== Make TextMatchMarkers constructible in single step Currently, we only construct instances of TextMatchMarker (previously RenderedDocumentMarker) inside TextMatchMarkerListImpl, by calling TextMatchMarker::Create() on an instance of DocumentMarker. This CL changes this so callers directly construct instances of TextMatchMarker. This sets us up to eventually move the storage and accessor methods for whether or not the marker is active out of DocumentMarker and into TextMatchMarker. BUG=707867 ==========
rlanday@chromium.org changed reviewers: + xiaochengh@chromium.org, yosin@chromium.org
lgtm
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-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...)
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...
lgtm https://codereview.chromium.org/2905753002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/editing/markers/DocumentMarkerTest.cpp (right): https://codereview.chromium.org/2905753002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/editing/markers/DocumentMarkerTest.cpp:17: return new TextMatchMarker(startOffset, endOffset, nit: It seems safer to use a fake marker type in unit tests, since real marker types have more details and restrictions which we don't want to involve in unit tests.
On 2017/05/25 at 21:38:04, xiaochengh wrote: > lgtm > > https://codereview.chromium.org/2905753002/diff/40001/third_party/WebKit/Sour... > File third_party/WebKit/Source/core/editing/markers/DocumentMarkerTest.cpp (right): > > https://codereview.chromium.org/2905753002/diff/40001/third_party/WebKit/Sour... > third_party/WebKit/Source/core/editing/markers/DocumentMarkerTest.cpp:17: return new TextMatchMarker(startOffset, endOffset, > nit: It seems safer to use a fake marker type in unit tests, since real marker types have more details and restrictions which we don't want to involve in unit tests. Ok, I'll follow-up on this in another CL
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...)
On 2017/05/25 at 22:40:15, rlanday wrote: > On 2017/05/25 at 21:38:04, xiaochengh wrote: > > lgtm > > > > https://codereview.chromium.org/2905753002/diff/40001/third_party/WebKit/Sour... > > File third_party/WebKit/Source/core/editing/markers/DocumentMarkerTest.cpp (right): > > > > https://codereview.chromium.org/2905753002/diff/40001/third_party/WebKit/Sour... > > third_party/WebKit/Source/core/editing/markers/DocumentMarkerTest.cpp:17: return new TextMatchMarker(startOffset, endOffset, > > nit: It seems safer to use a fake marker type in unit tests, since real marker types have more details and restrictions which we don't want to involve in unit tests. > > Ok, I'll follow-up on this in another CL Note that I would have to pick a real marker type for the fake marker to have...is that OK? I'm not sure there's a huge advantage here.
On 2017/05/26 at 01:24:48, rlanday wrote: > On 2017/05/25 at 22:40:15, rlanday wrote: > > On 2017/05/25 at 21:38:04, xiaochengh wrote: > > > lgtm > > > > > > https://codereview.chromium.org/2905753002/diff/40001/third_party/WebKit/Sour... > > > File third_party/WebKit/Source/core/editing/markers/DocumentMarkerTest.cpp (right): > > > > > > https://codereview.chromium.org/2905753002/diff/40001/third_party/WebKit/Sour... > > > third_party/WebKit/Source/core/editing/markers/DocumentMarkerTest.cpp:17: return new TextMatchMarker(startOffset, endOffset, > > > nit: It seems safer to use a fake marker type in unit tests, since real marker types have more details and restrictions which we don't want to involve in unit tests. > > > > Ok, I'll follow-up on this in another CL > > Note that I would have to pick a real marker type for the fake marker to have...is that OK? I'm not sure there's a huge advantage here. I was thinking adding a fake marker index/type for the fake marker class. Maybe this is too much work compared to the not too much gain... Anyway this is minor.
https://codereview.chromium.org/2905753002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/editing/markers/DocumentMarkerTest.cpp (right): https://codereview.chromium.org/2905753002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/editing/markers/DocumentMarkerTest.cpp:17: return new TextMatchMarker(startOffset, endOffset, On 2017/05/25 at 21:38:04, Xiaocheng wrote: > nit: It seems safer to use a fake marker type in unit tests, since real marker types have more details and restrictions which we don't want to involve in unit tests. Since DocumentMarker using MarketType. Introducing test only marker or fake marker requires having to type value in MarkerType. I may not want to have test only type.
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.
On 2017/05/26 at 02:12:27, yosin wrote: > Since DocumentMarker using MarketType. Introducing test only marker or fake marker requires having to type value in MarkerType. > I may not want to have test only type. The current version of this CL just uses TextMatchMarker; is this fine?
On 2017/05/29 at 16:46:36, rlanday wrote:
> On 2017/05/26 at 02:12:27, yosin wrote:
> > Since DocumentMarker using MarketType. Introducing test only marker or fake
marker requires having to type value in MarkerType.
> > I may not want to have test only type.
>
> The current version of this CL just uses TextMatchMarker; is this fine?
Yes. Each marker implementation has a test for DocumentMarker::GetType() ==
${marker_type}
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/2905753002/#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": 1496188766722640,
"parent_rev": "93fb0767a75b443bf25bc99b1b97f48f7b074ccd", "commit_rev":
"6b0a494122ebe2fc48099a9549b0acbf0b28571d"}
Message was sent while issue was closed.
Description was changed from ========== Make TextMatchMarkers constructible in single step Currently, we only construct instances of TextMatchMarker (previously RenderedDocumentMarker) inside TextMatchMarkerListImpl, by calling TextMatchMarker::Create() on an instance of DocumentMarker. This CL changes this so callers directly construct instances of TextMatchMarker. This sets us up to eventually move the storage and accessor methods for whether or not the marker is active out of DocumentMarker and into TextMatchMarker. BUG=707867 ========== to ========== Make TextMatchMarkers constructible in single step Currently, we only construct instances of TextMatchMarker (previously RenderedDocumentMarker) inside TextMatchMarkerListImpl, by calling TextMatchMarker::Create() on an instance of DocumentMarker. This CL changes this so callers directly construct instances of TextMatchMarker. This sets us up to eventually move the storage and accessor methods for whether or not the marker is active out of DocumentMarker and into TextMatchMarker. BUG=707867 Review-Url: https://codereview.chromium.org/2905753002 Cr-Commit-Position: refs/heads/master@{#475724} Committed: https://chromium.googlesource.com/chromium/src/+/6b0a494122ebe2fc48099a9549b0... ==========
Message was sent while issue was closed.
Committed patchset #4 (id:60001) as https://chromium.googlesource.com/chromium/src/+/6b0a494122ebe2fc48099a9549b0...
Message was sent while issue was closed.
A revert of this CL (patchset #4 id:60001) has been created in https://codereview.chromium.org/2912133003/ by tyoshino@chromium.org. The reason for reverting is: This broke the build. https://build.chromium.org/p/chromium.webkit/builders/WebKit%20Mac%20Builder%....
Message was sent while issue was closed.
On 2017/05/31 at 01:48:21, tyoshino wrote: > A revert of this CL (patchset #4 id:60001) has been created in https://codereview.chromium.org/2912133003/ by tyoshino@chromium.org. > > The reason for reverting is: This broke the build. > > https://build.chromium.org/p/chromium.webkit/builders/WebKit%20Mac%20Builder%.... Ouch! I believe this is due to a conflict with https://codereview.chromium.org/2903413003, which recently landed. Will update this CL and re-land.
Findit (https://goo.gl/kROfz5) confirmed this CL at revision 475724 as the culprit for failures in the build cycles as shown on: https://findit-for-me.appspot.com/waterfall/culprit?key=ag9zfmZpbmRpdC1mb3Itb...
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...
Updated to fix build error that necessitated a revert https://codereview.chromium.org/2905753002/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/editing/markers/DocumentMarkerTest.cpp (right): https://codereview.chromium.org/2905753002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/core/editing/markers/DocumentMarkerTest.cpp:17: return new TextMatchMarker(startOffset, endOffset, This is the fix: use the new TextMatchMarker constructor here instead of the now-private DocumentMarker constructor
The CQ bit was unchecked by rlanday@chromium.org
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/2905753002/#ps80001 (title: "Fix build error")
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 rlanday@chromium.org
Description was changed from ========== Make TextMatchMarkers constructible in single step Currently, we only construct instances of TextMatchMarker (previously RenderedDocumentMarker) inside TextMatchMarkerListImpl, by calling TextMatchMarker::Create() on an instance of DocumentMarker. This CL changes this so callers directly construct instances of TextMatchMarker. This sets us up to eventually move the storage and accessor methods for whether or not the marker is active out of DocumentMarker and into TextMatchMarker. BUG=707867 Review-Url: https://codereview.chromium.org/2905753002 Cr-Commit-Position: refs/heads/master@{#475724} Committed: https://chromium.googlesource.com/chromium/src/+/6b0a494122ebe2fc48099a9549b0... ========== to ========== [Reland] Make TextMatchMarkers constructible in single step Currently, we only construct instances of TextMatchMarker (previously RenderedDocumentMarker) inside TextMatchMarkerListImpl, by calling TextMatchMarker::Create() on an instance of DocumentMarker. This CL changes this so callers directly construct instances of TextMatchMarker. This sets us up to eventually move the storage and accessor methods for whether or not the marker is active out of DocumentMarker and into TextMatchMarker. BUG=707867 Review-Url: https://codereview.chromium.org/2905753002 Cr-Commit-Position: refs/heads/master@{#475724} Committed: https://chromium.googlesource.com/chromium/src/+/6b0a494122ebe2fc48099a9549b0... ==========
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": 80001, "attempt_start_ts": 1496196874472610,
"parent_rev": "c8cb20003e641f8979f9e0a3be24239d5fe46a23", "commit_rev":
"8c11f0edca942d3dc3e81f0d42fe4b342fa69e90"}
CQ is committing da patch.
Bot data: {"patchset_id": 80001, "attempt_start_ts": 1496196874472610,
"parent_rev": "3b48f129b47381570c7f4372c9fe0c5785fd59da", "commit_rev":
"be1de5ecae843ecb1fe73577a5f6fb4e51c3f529"}
Message was sent while issue was closed.
Description was changed from ========== [Reland] Make TextMatchMarkers constructible in single step Currently, we only construct instances of TextMatchMarker (previously RenderedDocumentMarker) inside TextMatchMarkerListImpl, by calling TextMatchMarker::Create() on an instance of DocumentMarker. This CL changes this so callers directly construct instances of TextMatchMarker. This sets us up to eventually move the storage and accessor methods for whether or not the marker is active out of DocumentMarker and into TextMatchMarker. BUG=707867 Review-Url: https://codereview.chromium.org/2905753002 Cr-Commit-Position: refs/heads/master@{#475724} Committed: https://chromium.googlesource.com/chromium/src/+/6b0a494122ebe2fc48099a9549b0... ========== to ========== [Reland] Make TextMatchMarkers constructible in single step Currently, we only construct instances of TextMatchMarker (previously RenderedDocumentMarker) inside TextMatchMarkerListImpl, by calling TextMatchMarker::Create() on an instance of DocumentMarker. This CL changes this so callers directly construct instances of TextMatchMarker. This sets us up to eventually move the storage and accessor methods for whether or not the marker is active out of DocumentMarker and into TextMatchMarker. BUG=707867 Review-Url: https://codereview.chromium.org/2905753002 Cr-Original-Commit-Position: refs/heads/master@{#475724} Committed: https://chromium.googlesource.com/chromium/src/+/6b0a494122ebe2fc48099a9549b0... Review-Url: https://codereview.chromium.org/2905753002 Cr-Commit-Position: refs/heads/master@{#475781} Committed: https://chromium.googlesource.com/chromium/src/+/be1de5ecae843ecb1fe73577a5f6... ==========
Message was sent while issue was closed.
Committed patchset #5 (id:80001) as https://chromium.googlesource.com/chromium/src/+/be1de5ecae843ecb1fe73577a5f6... |
