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

Issue 2904093002: [DMC #18] Eliminate DocumentMarkerTextMatch (subclass of DocumentMarkerDetails) (Closed)

Created:
3 years, 7 months ago by rlanday
Modified:
3 years, 6 months ago
Reviewers:
yosin_UTC9, Xiaocheng
CC:
blink-reviews, chromium-reviews
Target Ref:
refs/heads/master
Project:
chromium
Visibility:
Public.

Description

Eliminate DocumentMarkerTextMatch (subclass of DocumentMarkerDetails) I eventually want to eliminate all the DocumentMarkerDetails subclasses and move the storage for MarkerType-specific information into the type-specific subclasses of DocumentMarker. This CL does this for TextMatchMarker. In a later CL, I will remove IsMarkerActive() and SetIsMarkerActive() from DocumentMarker and force callers to cast to TextMatchMarker to call these methods. BUG=707867 Review-Url: https://codereview.chromium.org/2904093002 Cr-Commit-Position: refs/heads/master@{#476026} Committed: https://chromium.googlesource.com/chromium/src/+/c59068590b02e4c25544b62a5a9a5cf6863252b3

Patch Set 1 #

Total comments: 3

Patch Set 2 : Rename state_ and status_ #

Patch Set 3 : Rebase #

Total comments: 7

Patch Set 4 : Address nits #

Patch Set 5 : Add DCHECKs, more nits #

Total comments: 1

Patch Set 6 : Add const, rebase #

Total comments: 1
Unified diffs Side-by-side diffs Delta from patch set Stats (+35 lines, -67 lines) Patch
M third_party/WebKit/Source/core/editing/markers/DocumentMarker.h View 1 2 3 4 5 2 chunks +2 lines, -3 lines 0 comments Download
M third_party/WebKit/Source/core/editing/markers/DocumentMarker.cpp View 1 2 3 6 chunks +14 lines, -53 lines 0 comments Download
M third_party/WebKit/Source/core/editing/markers/TextMatchMarker.h View 1 2 3 1 chunk +19 lines, -11 lines 1 comment Download

Messages

Total messages: 47 (23 generated)
rlanday
https://codereview.chromium.org/2904093002/diff/1/third_party/WebKit/Source/core/editing/markers/DocumentMarker.cpp File third_party/WebKit/Source/core/editing/markers/DocumentMarker.cpp (right): https://codereview.chromium.org/2904093002/diff/1/third_party/WebKit/Source/core/editing/markers/DocumentMarker.cpp#newcode193 third_party/WebKit/Source/core/editing/markers/DocumentMarker.cpp:193: if (GetType() != DocumentMarker::kTextMatch) Note the behavioral change here: ...
3 years, 7 months ago (2017-05-25 21:00:48 UTC) #2
Xiaocheng
lgtm https://codereview.chromium.org/2904093002/diff/1/third_party/WebKit/Source/core/editing/markers/DocumentMarker.cpp File third_party/WebKit/Source/core/editing/markers/DocumentMarker.cpp (right): https://codereview.chromium.org/2904093002/diff/1/third_party/WebKit/Source/core/editing/markers/DocumentMarker.cpp#newcode193 third_party/WebKit/Source/core/editing/markers/DocumentMarker.cpp:193: if (GetType() != DocumentMarker::kTextMatch) On 2017/05/25 at 21:00:48, ...
3 years, 7 months ago (2017-05-26 00:46:25 UTC) #3
yosin_UTC9
https://codereview.chromium.org/2904093002/diff/1/third_party/WebKit/Source/core/editing/markers/TextMatchMarker.h File third_party/WebKit/Source/core/editing/markers/TextMatchMarker.h (right): https://codereview.chromium.org/2904093002/diff/1/third_party/WebKit/Source/core/editing/markers/TextMatchMarker.h#newcode79 third_party/WebKit/Source/core/editing/markers/TextMatchMarker.h:79: MatchStatus status_; nit: s/status_/match_status_/ It is confusing to have ...
3 years, 7 months ago (2017-05-26 04:39:47 UTC) #4
rlanday
On 2017/05/26 at 04:39:47, yosin wrote: > https://codereview.chromium.org/2904093002/diff/1/third_party/WebKit/Source/core/editing/markers/TextMatchMarker.h > File third_party/WebKit/Source/core/editing/markers/TextMatchMarker.h (right): > > https://codereview.chromium.org/2904093002/diff/1/third_party/WebKit/Source/core/editing/markers/TextMatchMarker.h#newcode79 ...
3 years, 7 months ago (2017-05-26 18:32:03 UTC) #6
yosin_UTC9
https://codereview.chromium.org/2904093002/diff/40001/third_party/WebKit/Source/core/editing/markers/DocumentMarker.cpp File third_party/WebKit/Source/core/editing/markers/DocumentMarker.cpp (right): https://codereview.chromium.org/2904093002/diff/40001/third_party/WebKit/Source/core/editing/markers/DocumentMarker.cpp#newcode109 third_party/WebKit/Source/core/editing/markers/DocumentMarker.cpp:109: start_offset_(start_offset), Just in case, let's add DCHECK_LT(start_offset, end_offset) https://codereview.chromium.org/2904093002/diff/40001/third_party/WebKit/Source/core/editing/markers/DocumentMarker.cpp#newcode111 ...
3 years, 6 months ago (2017-05-29 05:05:33 UTC) #14
rlanday
https://codereview.chromium.org/2904093002/diff/40001/third_party/WebKit/Source/core/editing/markers/DocumentMarker.cpp File third_party/WebKit/Source/core/editing/markers/DocumentMarker.cpp (right): https://codereview.chromium.org/2904093002/diff/40001/third_party/WebKit/Source/core/editing/markers/DocumentMarker.cpp#newcode109 third_party/WebKit/Source/core/editing/markers/DocumentMarker.cpp:109: start_offset_(start_offset), On 2017/05/29 at 05:05:33, yosin_UTC9 wrote: > Just ...
3 years, 6 months ago (2017-05-30 21:28:11 UTC) #17
Xiaocheng
https://codereview.chromium.org/2904093002/diff/40001/third_party/WebKit/Source/core/editing/markers/DocumentMarker.cpp File third_party/WebKit/Source/core/editing/markers/DocumentMarker.cpp (right): https://codereview.chromium.org/2904093002/diff/40001/third_party/WebKit/Source/core/editing/markers/DocumentMarker.cpp#newcode109 third_party/WebKit/Source/core/editing/markers/DocumentMarker.cpp:109: start_offset_(start_offset), On 2017/05/30 at 21:28:11, rlanday wrote: > On ...
3 years, 6 months ago (2017-05-30 21:31:36 UTC) #18
Xiaocheng
https://codereview.chromium.org/2904093002/diff/40001/third_party/WebKit/Source/core/editing/markers/DocumentMarker.cpp File third_party/WebKit/Source/core/editing/markers/DocumentMarker.cpp (right): https://codereview.chromium.org/2904093002/diff/40001/third_party/WebKit/Source/core/editing/markers/DocumentMarker.cpp#newcode109 third_party/WebKit/Source/core/editing/markers/DocumentMarker.cpp:109: start_offset_(start_offset), On 2017/05/30 at 21:31:36, Xiaocheng wrote: > On ...
3 years, 6 months ago (2017-05-30 22:24:37 UTC) #19
Xiaocheng
On 2017/05/30 at 22:24:37, Xiaocheng wrote: > https://codereview.chromium.org/2904093002/diff/40001/third_party/WebKit/Source/core/editing/markers/DocumentMarker.cpp > File third_party/WebKit/Source/core/editing/markers/DocumentMarker.cpp (right): > > https://codereview.chromium.org/2904093002/diff/40001/third_party/WebKit/Source/core/editing/markers/DocumentMarker.cpp#newcode109 ...
3 years, 6 months ago (2017-05-30 22:28:42 UTC) #20
rlanday
On 2017/05/30 at 22:28:42, xiaochengh wrote: > On 2017/05/30 at 22:24:37, Xiaocheng wrote: > > ...
3 years, 6 months ago (2017-05-30 22:53:12 UTC) #23
Xiaocheng
On 2017/05/30 at 22:53:12, rlanday wrote: > On 2017/05/30 at 22:28:42, xiaochengh wrote: > > ...
3 years, 6 months ago (2017-05-30 23:00:35 UTC) #24
rlanday
Here's the CL to add filtering: https://codereview.chromium.org/2916463002
3 years, 6 months ago (2017-05-30 23:36:23 UTC) #25
rlanday
Ok, I've updated this CL with the DCHECKs now that we have the filtering and ...
3 years, 6 months ago (2017-05-31 07:00:39 UTC) #28
yosin_UTC9
lgtm w/ nit https://codereview.chromium.org/2904093002/diff/80001/third_party/WebKit/Source/core/editing/markers/DocumentMarker.h File third_party/WebKit/Source/core/editing/markers/DocumentMarker.h (right): https://codereview.chromium.org/2904093002/diff/80001/third_party/WebKit/Source/core/editing/markers/DocumentMarker.h#newcode180 third_party/WebKit/Source/core/editing/markers/DocumentMarker.h:180: MarkerType type_; nit: s/MarkerType/const MarkerType/
3 years, 6 months ago (2017-05-31 07:06:54 UTC) #29
commit-bot: I haz the power
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2904093002/100001
3 years, 6 months ago (2017-05-31 18:07:46 UTC) #34
Xiaocheng
A question about wording: what's the difference between "state" and "status"? It seems inconsistent to ...
3 years, 6 months ago (2017-05-31 18:09:02 UTC) #35
rlanday
On 2017/05/31 at 18:09:02, xiaochengh wrote: > A question about wording: what's the difference between ...
3 years, 6 months ago (2017-05-31 18:11:44 UTC) #36
Xiaocheng
On 2017/05/31 at 18:11:44, rlanday wrote: > On 2017/05/31 at 18:09:02, xiaochengh wrote: > > ...
3 years, 6 months ago (2017-05-31 18:23:54 UTC) #37
rlanday
On 2017/05/31 at 18:23:54, xiaochengh wrote: > Seems that there's no clear border between them. ...
3 years, 6 months ago (2017-05-31 18:43:57 UTC) #38
rlanday
On 2017/05/31 at 18:43:57, rlanday wrote: > On 2017/05/31 at 18:23:54, xiaochengh wrote: > > ...
3 years, 6 months ago (2017-05-31 19:06:13 UTC) #39
Xiaocheng
On 2017/05/31 at 19:06:13, rlanday wrote: > On 2017/05/31 at 18:43:57, rlanday wrote: > > ...
3 years, 6 months ago (2017-05-31 19:07:48 UTC) #40
commit-bot: I haz the power
Try jobs failed on following builders: linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_rel_ng/builds/468601)
3 years, 6 months ago (2017-05-31 20:02:25 UTC) #42
commit-bot: I haz the power
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2904093002/100001
3 years, 6 months ago (2017-05-31 20:10:06 UTC) #44
commit-bot: I haz the power
3 years, 6 months ago (2017-05-31 21:19:07 UTC) #47
Message was sent while issue was closed.
Committed patchset #6 (id:100001) as
https://chromium.googlesource.com/chromium/src/+/c59068590b02e4c25544b62a5a9a...

Powered by Google App Engine
This is Rietveld 408576698