|
|
DescriptionEliminate 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
Messages
Total messages: 47 (23 generated)
rlanday@chromium.org changed reviewers: + xiaochengh@chromium.org, yosin@chromium.org
https://codereview.chromium.org/2904093002/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/editing/markers/DocumentMarker.cpp (right): https://codereview.chromium.org/2904093002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/editing/markers/DocumentMarker.cpp:193: if (GetType() != DocumentMarker::kTextMatch) Note the behavioral change here: calling SetIsActiveMatch() on a non-TextMatchMarker currently overwrites the DocumentMarkerDetails pointer with one that holds TextMatchMarker-specific data. I'm changing it to be a no-op in this case. In a follow-up CL, I'll force callers to cast to TextMatchMarker to call this method, avoiding the issue entirely.
lgtm https://codereview.chromium.org/2904093002/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/editing/markers/DocumentMarker.cpp (right): https://codereview.chromium.org/2904093002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/editing/markers/DocumentMarker.cpp:193: if (GetType() != DocumentMarker::kTextMatch) On 2017/05/25 at 21:00:48, rlanday wrote: > Note the behavioral change here: calling SetIsActiveMatch() on a non-TextMatchMarker currently overwrites the DocumentMarkerDetails pointer with one that holds TextMatchMarker-specific data. I'm changing it to be a no-op in this case. In a follow-up CL, I'll force callers to cast to TextMatchMarker to call this method, avoiding the issue entirely. We should be fine here, since DM::SetIsActiveMatch is called only by DMC::SetTextMatchMarkersActive. There shouldn't be real behavioral change. Anyway, cleaning it up is good.
https://codereview.chromium.org/2904093002/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/editing/markers/TextMatchMarker.h (right): https://codereview.chromium.org/2904093002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/editing/markers/TextMatchMarker.h:79: MatchStatus status_; nit: s/status_/match_status_/ It is confusing to have both |state_| and |status_|. We may also want to rename |state_| to |layout_state_|.
The CQ bit was checked by rlanday@chromium.org to run a CQ dry run
On 2017/05/26 at 04:39:47, yosin wrote: > https://codereview.chromium.org/2904093002/diff/1/third_party/WebKit/Source/c... > File third_party/WebKit/Source/core/editing/markers/TextMatchMarker.h (right): > > https://codereview.chromium.org/2904093002/diff/1/third_party/WebKit/Source/c... > third_party/WebKit/Source/core/editing/markers/TextMatchMarker.h:79: MatchStatus status_; > nit: s/status_/match_status_/ > > It is confusing to have both |state_| and |status_|. > > We may also want to rename |state_| to |layout_state_|. I renamed them both...I agree, it's really confusing as-is
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: cast_shell_android on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/cast_shell_a...) ios-simulator on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...) 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...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2904093002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/editing/markers/DocumentMarker.cpp (right): https://codereview.chromium.org/2904093002/diff/40001/third_party/WebKit/Sour... 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/Sour... third_party/WebKit/Source/core/editing/markers/DocumentMarker.cpp:111: details_(nullptr) {} nit: s/details_(nullptr)// Since |details_| is |Member<DocumetnMarkerDetaisl>>|, we don't need to initialize to |nullptr|. https://codereview.chromium.org/2904093002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/editing/markers/TextMatchMarker.h (right): https://codereview.chromium.org/2904093002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/editing/markers/TextMatchMarker.h:45: layout_state_(State::kInvalid) {} Let's initialize |layout_state_| inline, .e.g |State layout_state_ = State::kInvalid| https://codereview.chromium.org/2904093002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/editing/markers/TextMatchMarker.h:80: nit: No need to have blank line.
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...
https://codereview.chromium.org/2904093002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/editing/markers/DocumentMarker.cpp (right): https://codereview.chromium.org/2904093002/diff/40001/third_party/WebKit/Sour... 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 in case, let's add > > DCHECK_LT(start_offset, end_offset) This causes a test case failure in InputMethodControllerTest.InsertTextWithNewline(): https://cs.chromium.org/chromium/src/third_party/WebKit/Source/core/editing/I... Apparently TextIterator can create zero-length ranges for certain nodes. What do you think we should do (e.g., not add the DCHECK, filter the results from TextIterator to not create these zero-length ranges)?
https://codereview.chromium.org/2904093002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/editing/markers/DocumentMarker.cpp (right): https://codereview.chromium.org/2904093002/diff/40001/third_party/WebKit/Sour... 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 2017/05/29 at 05:05:33, yosin_UTC9 wrote: > > Just in case, let's add > > > > DCHECK_LT(start_offset, end_offset) > > This causes a test case failure in InputMethodControllerTest.InsertTextWithNewline(): > https://cs.chromium.org/chromium/src/third_party/WebKit/Source/core/editing/I... > > Apparently TextIterator can create zero-length ranges for certain nodes. What do you think we should do (e.g., not add the DCHECK, filter the results from TextIterator to not create these zero-length ranges)? TextIterator shouldn't create zero-length ranges. Let me check what happened since I'm working on TI these days...
https://codereview.chromium.org/2904093002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/editing/markers/DocumentMarker.cpp (right): https://codereview.chromium.org/2904093002/diff/40001/third_party/WebKit/Sour... 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 2017/05/30 at 21:28:11, rlanday wrote: > > On 2017/05/29 at 05:05:33, yosin_UTC9 wrote: > > > Just in case, let's add > > > > > > DCHECK_LT(start_offset, end_offset) > > > > This causes a test case failure in InputMethodControllerTest.InsertTextWithNewline(): > > https://cs.chromium.org/chromium/src/third_party/WebKit/Source/core/editing/I... > > > > Apparently TextIterator can create zero-length ranges for certain nodes. What do you think we should do (e.g., not add the DCHECK, filter the results from TextIterator to not create these zero-length ranges)? > > TextIterator shouldn't create zero-length ranges. > > Let me check what happened since I'm working on TI these days... DMC should filter results from TextIterator so that marker is created only when both of the follow are true: 1. The anchor node is a text node; TI can emit text anchored at non-text nodes (e.g., it emits a "\n" between two <div>s) 2. Start offset < end offset. There are some cases where TI emits non-empty the same start and end offset. Since there's no spec/documentation for TI, I'm still investigating what it means when TI emits non-empty text with a collapsed range. Anyway, this should be an issue of TI, which can be tricky to handle... In order not to block DMC, I think the best solution is to let DMC filter TI's output, and add comments of: 1. Why the filtering is needed 2. TODO(editing-dev): investigate if it's a problem of TI
On 2017/05/30 at 22:24:37, Xiaocheng wrote: > https://codereview.chromium.org/2904093002/diff/40001/third_party/WebKit/Sour... > File third_party/WebKit/Source/core/editing/markers/DocumentMarker.cpp (right): > > https://codereview.chromium.org/2904093002/diff/40001/third_party/WebKit/Sour... > 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 2017/05/30 at 21:28:11, rlanday wrote: > > > On 2017/05/29 at 05:05:33, yosin_UTC9 wrote: > > > > Just in case, let's add > > > > > > > > DCHECK_LT(start_offset, end_offset) > > > > > > This causes a test case failure in InputMethodControllerTest.InsertTextWithNewline(): > > > https://cs.chromium.org/chromium/src/third_party/WebKit/Source/core/editing/I... > > > > > > Apparently TextIterator can create zero-length ranges for certain nodes. What do you think we should do (e.g., not add the DCHECK, filter the results from TextIterator to not create these zero-length ranges)? > > > > TextIterator shouldn't create zero-length ranges. > > > > Let me check what happened since I'm working on TI these days... > > DMC should filter results from TextIterator so that marker is created only when both of the follow are true: > 1. The anchor node is a text node; TI can emit text anchored at non-text nodes (e.g., it emits a "\n" between two <div>s) > 2. Start offset < end offset. There are some cases where TI emits non-empty the same start and end offset. Since there's no spec/documentation for TI, I'm still investigating what it means when TI emits non-empty text with a collapsed range. Anyway, this should be an issue of TI, which can be tricky to handle... > > In order not to block DMC, I think the best solution is to let DMC filter TI's output, and add comments of: > 1. Why the filtering is needed > 2. TODO(editing-dev): investigate if it's a problem of TI Please associate the TODO with crbug.com/727929.
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/30 at 22:28:42, xiaochengh wrote: > On 2017/05/30 at 22:24:37, Xiaocheng wrote: > > In order not to block DMC, I think the best solution is to let DMC filter TI's output, and add comments of: > > 1. Why the filtering is needed > > 2. TODO(editing-dev): investigate if it's a problem of TI > > Please associate the TODO with crbug.com/727929. We actually already do this filtering...after the DocumentMarker is constructed. See https://chromium.googlesource.com/chromium/src/+/68bd4c5b8ab7bb2d11adc3175419... I'll refactor DMC::AddMarker() to reduce code duplication between the different MarkerType-specific Add() methods, and avoid constructing DocumentMarkers with the same start and end offset.
On 2017/05/30 at 22:53:12, rlanday wrote: > On 2017/05/30 at 22:28:42, xiaochengh wrote: > > On 2017/05/30 at 22:24:37, Xiaocheng wrote: > > > In order not to block DMC, I think the best solution is to let DMC filter TI's output, and add comments of: > > > 1. Why the filtering is needed > > > 2. TODO(editing-dev): investigate if it's a problem of TI > > > > Please associate the TODO with crbug.com/727929. > > We actually already do this filtering...after the DocumentMarker is constructed. See > https://chromium.googlesource.com/chromium/src/+/68bd4c5b8ab7bb2d11adc3175419... This filtering is not very efficient, because an unused DocumentMarker is still created. Besides, we don't want to add markers to non-text nodes, which is not filtered yet. > > I'll refactor DMC::AddMarker() to reduce code duplication between the different MarkerType-specific Add() methods, and avoid constructing DocumentMarkers with the same start and end offset.
Here's the CL to add filtering: https://codereview.chromium.org/2916463002
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...
Ok, I've updated this CL with the DCHECKs now that we have the filtering and the tests should run without crashing...
lgtm w/ nit https://codereview.chromium.org/2904093002/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/editing/markers/DocumentMarker.h (right): https://codereview.chromium.org/2904093002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/core/editing/markers/DocumentMarker.h:180: MarkerType type_; nit: s/MarkerType/const MarkerType/
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)
The CQ bit was checked by rlanday@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from xiaochengh@chromium.org, yosin@chromium.org Link to the patchset: https://codereview.chromium.org/2904093002/#ps100001 (title: "Add const, rebase")
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
A question about wording: what's the difference between "state" and "status"? It seems inconsistent to have |match_status_| and |layout_state_| in the same class. https://codereview.chromium.org/2904093002/diff/100001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/editing/markers/TextMatchMarker.h (right): https://codereview.chromium.org/2904093002/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/core/editing/markers/TextMatchMarker.h:82: State layout_state_; I think |State| should also be renamed to reduce confusion.
On 2017/05/31 at 18:09:02, xiaochengh wrote: > A question about wording: what's the difference between "state" and "status"? > > It seems inconsistent to have |match_status_| and |layout_state_| in the same class. It's somewhat subtle, but I think "status" implies there's a smallish set of options (e.g., active, inactive) and "state" implies that there's some more substantial information being recorded.
On 2017/05/31 at 18:11:44, rlanday wrote: > On 2017/05/31 at 18:09:02, xiaochengh wrote: > > A question about wording: what's the difference between "state" and "status"? > > > > It seems inconsistent to have |match_status_| and |layout_state_| in the same class. > > It's somewhat subtle, but I think "status" implies there's a smallish set of options (e.g., active, inactive) and "state" implies that there's some more substantial information being recorded. From Longman Dictionary: State: the physical or mental condition that someone or something is in Status: a situation at a particular time, especially in an argument, discussion etc Seems that there's no clear border between them. For consistency, let's keep only one. And rename |State| to |LayoutState/Status|.
On 2017/05/31 at 18:23:54, xiaochengh wrote: > Seems that there's no clear border between them. For consistency, let's keep only one. And rename |State| to |LayoutState/Status|. They're both enums, so I think we should use MatchStatus and LayoutStatus.
On 2017/05/31 at 18:43:57, rlanday wrote: > On 2017/05/31 at 18:23:54, xiaochengh wrote: > > Seems that there's no clear border between them. For consistency, let's keep only one. And rename |State| to |LayoutState/Status|. > > They're both enums, so I think we should use MatchStatus and LayoutStatus. (Will follow-up in a separate CL, I want to get this landed since it's blocking some other CLs)
On 2017/05/31 at 19:06:13, rlanday wrote: > On 2017/05/31 at 18:43:57, rlanday wrote: > > On 2017/05/31 at 18:23:54, xiaochengh wrote: > > > Seems that there's no clear border between them. For consistency, let's keep only one. And rename |State| to |LayoutState/Status|. > > > > They're both enums, so I think we should use MatchStatus and LayoutStatus. > > (Will follow-up in a separate CL, I want to get this landed since it's blocking some other CLs) Sure!
The CQ bit was unchecked by commit-bot@chromium.org
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_...)
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": 1496261345354500, "parent_rev": "9a182ab9892e7f016117751a60a02b93e4ddcaec", "commit_rev": "c59068590b02e4c25544b62a5a9a5cf6863252b3"}
Message was sent while issue was closed.
Description was changed from ========== 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 ========== to ========== 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/+/c59068590b02e4c25544b62a5a9a... ==========
Message was sent while issue was closed.
Committed patchset #6 (id:100001) as https://chromium.googlesource.com/chromium/src/+/c59068590b02e4c25544b62a5a9a... |