Improve how DocumentMarkerController updates markers in response to text edits

Improve how DocumentMarkerController updates markers in response to text edits

This is basically https://codereview.chromium.org/2692093003 with the changes to
make DocumentMarkerController use SynchronousMutationObserver split off into a
separate CL (https://codereview.chromium.org/2755953004).

I need DocumentMarkerController to update markers more gracefully in response to
edits in order to implement SuggestionSpan support on Android. For example, if
we have the string "word1 word2" marked with a DocumentMarker and
CompositeEditCommand::replaceTextInNode() is used to replace word1 or word2 with
different text, the new text should still be inside the marked range.

There's currently a method
CompositeEditCommand::replaceTextInNodePreservingMarkers(), which was added to
fix some particular case to support a marker type which no longer exists
(DeletedAutocorrection): https://bugs.webkit.org/show_bug.cgi?id=60555

This is no longer necessary after this CL since we can handle all the updates
more correctly inside DocumentMarkerController. For example, if we have a string
like "Initial text blah" and we add a marker around " text " and delete
"Initial" and "blah", we end up with " text ". Then
DeleteSelectionCommand::fixupWhitespace() runs, and replaces the spaces with
non-breaking spaces. The current behavior is that the NBSPs are no longer in the
marked range, since DocumentMarkerController doesn't currently support keeping
the replaced text in the marked range, but replaceTextInNodePreservingMarkers()
runs and adds new markers to the NBSPs. So we still end up with " text " being
marked but now we have three separate markers, which is undesirable for some
use cases (e.g. text suggestions, because we don't know what range of text we're
actually supposed to replace now). With the changes in this CL,
DocumentMarkerController is able to keep a single marker around " text ".

The changes to the expected output for InputMethodControllerTest may be helpful
in understanding the behavioral changes here.

BUG=672259
Review-Url: https://codereview.chromium.org/2755013004
Cr-Commit-Position: refs/heads/master@{#464236}
Committed: https://chromium.googlesource.com/chromium/src/+/d6ef615a1a2154d3435b9a09afcd8f6cd929f275

[rlanday, 2017-03-17 21:51:59]

The CQ bit was checked by rlanday@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-03-17 21:52:59]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[rlanday, 2017-03-17 22:31:32]

rlanday@chromium.org changed reviewers:
+ xiaochengh@chromium.org, yosin@chromium.org

[rlanday, 2017-03-17 22:31:32]

rlanday@chromium.org changed required reviewers:
+ yosin@chromium.org

[Xiaocheng, 2017-03-17 23:02:54]

Still lgtm.

[commit-bot: I haz the power, 2017-03-18 00:13:27]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-03-18 00:13:28]

Dry run: This issue passed the CQ dry run.

[rlanday, 2017-03-21 21:58:54]

The CQ bit was checked by rlanday@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-03-21 21:59:30]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-03-21 23:44:17]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-03-21 23:44:18]

Dry run: This issue passed the CQ dry run.

[rlanday, 2017-03-22 17:52:50]

The CQ bit was checked by rlanday@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-03-22 17:53:11]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-03-22 18:25:05]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-03-22 18:25:06]

Dry run: Try jobs failed on following builders:
  win_chromium_compile_dbg_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_comp...)
  win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)

[rlanday, 2017-03-22 19:37:22]

The CQ bit was checked by rlanday@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-03-22 19:38:14]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-03-22 21:28:34]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-03-22 21:28:35]

Dry run: This issue passed the CQ dry run.

[yosin_UTC9, 2017-03-23 04:10:26]

almost good shape!

https://codereview.chromium.org/2755013004/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/editing/markers/DocumentMarkerController.cpp
(right):

https://codereview.chromium.org/2755013004/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/editing/markers/DocumentMarkerController.cpp:853:
if (marker.startOffset() > offset) {
It is nice if we can move this logic into function.

if (relocateMarkerIfNeeded(&marker, offset, newLength, oldLength))
  didShiftMarker = true;

It is easier to read.

[rlanday, 2017-03-23 17:28:50]

The CQ bit was checked by rlanday@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-03-23 17:29:23]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[rlanday, 2017-03-23 17:29:52]

On 2017/03/23 at 04:10:26, yosin wrote:
> almost good shape!
> 
>
https://codereview.chromium.org/2755013004/diff/60001/third_party/WebKit/Sour...
> File
third_party/WebKit/Source/core/editing/markers/DocumentMarkerController.cpp
(right):
> 
>
https://codereview.chromium.org/2755013004/diff/60001/third_party/WebKit/Sour...
>
third_party/WebKit/Source/core/editing/markers/DocumentMarkerController.cpp:853:
if (marker.startOffset() > offset) {
> It is nice if we can move this logic into function.
> 
> if (relocateMarkerIfNeeded(&marker, offset, newLength, oldLength))
>   didShiftMarker = true;
> 
> It is easier to read.

Ok, I've updated the CL. Note that this code arrangement is going to be changed
again by the DocumentMarkerController refactor (right now I have the shifting
logic as a method on DocumentMarker).

[commit-bot: I haz the power, 2017-03-23 18:27:32]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-03-23 18:27:33]

Dry run: Try jobs failed on following builders:
  win_clang on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_clang/builds/...)

[rlanday, 2017-03-23 22:52:00]

https://codereview.chromium.org/2755013004/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/editing/markers/DocumentMarkerController.cpp
(right):

https://codereview.chromium.org/2755013004/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/editing/markers/DocumentMarkerController.cpp:658:
if (marker->startOffset() > offset) {
I think there's one more tweak we want to make: as written, if an insertion
happens right where a marker starts, the new text will be included in the
marker. E.g. if we have a marker from 5 to 10 and do an insert with offset 5,
oldLength 0, and newLength 5, the new marker will be from 5 to 15 instead of 10
to 15. We can fix this by writing >= here instead of >.

https://codereview.chromium.org/2755013004/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/editing/markers/DocumentMarkerController.cpp:661:
if (marker->startOffset() < offset + oldLength) {
Since we're moving the marker start to the end of the new text, it doesn't
matter if this is <= or < (offset + newLength == marker->startOffset() +
newLength - oldLength if marker->startOffset() == offset + oldLength), so I'm
going to change this back to <= to be consistent with the concept-cd-replace
algorithm

[rlanday, 2017-03-24 00:54:24]

The CQ bit was checked by rlanday@chromium.org to run a CQ dry run

[rlanday, 2017-03-24 00:54:42]

On 2017/03/23 at 22:52:00, rlanday wrote:
>
https://codereview.chromium.org/2755013004/diff/80001/third_party/WebKit/Sour...
> File
third_party/WebKit/Source/core/editing/markers/DocumentMarkerController.cpp
(right):
> 
>
https://codereview.chromium.org/2755013004/diff/80001/third_party/WebKit/Sour...
>
third_party/WebKit/Source/core/editing/markers/DocumentMarkerController.cpp:658:
if (marker->startOffset() > offset) {
> I think there's one more tweak we want to make: as written, if an insertion
happens right where a marker starts, the new text will be included in the
marker. E.g. if we have a marker from 5 to 10 and do an insert with offset 5,
oldLength 0, and newLength 5, the new marker will be from 5 to 15 instead of 10
to 15. We can fix this by writing >= here instead of >.
> 
>
https://codereview.chromium.org/2755013004/diff/80001/third_party/WebKit/Sour...
>
third_party/WebKit/Source/core/editing/markers/DocumentMarkerController.cpp:661:
if (marker->startOffset() < offset + oldLength) {
> Since we're moving the marker start to the end of the new text, it doesn't
matter if this is <= or < (offset + newLength == marker->startOffset() +
newLength - oldLength if marker->startOffset() == offset + oldLength), so I'm
going to change this back to <= to be consistent with the concept-cd-replace
algorithm

Updated with these changes (I also added more test cases)

[commit-bot: I haz the power, 2017-03-24 00:54:44]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[rlanday, 2017-03-24 01:09:49]

On 2017/03/24 at 00:54:42, rlanday wrote:
> On 2017/03/23 at 22:52:00, rlanday wrote:
> >
https://codereview.chromium.org/2755013004/diff/80001/third_party/WebKit/Sour...
> > File
third_party/WebKit/Source/core/editing/markers/DocumentMarkerController.cpp
(right):
> > 
> >
https://codereview.chromium.org/2755013004/diff/80001/third_party/WebKit/Sour...
> >
third_party/WebKit/Source/core/editing/markers/DocumentMarkerController.cpp:658:
if (marker->startOffset() > offset) {
> > I think there's one more tweak we want to make: as written, if an insertion
happens right where a marker starts, the new text will be included in the
marker. E.g. if we have a marker from 5 to 10 and do an insert with offset 5,
oldLength 0, and newLength 5, the new marker will be from 5 to 15 instead of 10
to 15. We can fix this by writing >= here instead of >.
> > 
> >
https://codereview.chromium.org/2755013004/diff/80001/third_party/WebKit/Sour...
> >
third_party/WebKit/Source/core/editing/markers/DocumentMarkerController.cpp:661:
if (marker->startOffset() < offset + oldLength) {
> > Since we're moving the marker start to the end of the new text, it doesn't
matter if this is <= or < (offset + newLength == marker->startOffset() +
newLength - oldLength if marker->startOffset() == offset + oldLength), so I'm
going to change this back to <= to be consistent with the concept-cd-replace
algorithm
> 
> Updated with these changes (I also added more test cases)

Actually this is still not correct, the WhitespaceFixup tests are failing now,
don't bother reviewing this until I update it again :)

[commit-bot: I haz the power, 2017-03-24 01:11:16]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-03-24 01:11:17]

Dry run: Try jobs failed on following builders:
  linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[rlanday, 2017-03-24 01:23:03]

The CQ bit was checked by rlanday@chromium.org to run a CQ dry run

[rlanday, 2017-03-24 01:23:21]

Ok, all the tests are passing now, I think the behavior is correct :)

[commit-bot: I haz the power, 2017-03-24 01:23:35]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-03-24 01:37:32]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-03-24 01:37:33]

Dry run: Try jobs failed on following builders:
  linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[yosin_UTC9, 2017-03-24 01:54:57]

On 2017/03/24 at 01:23:21, rlanday wrote:
> Ok, all the tests are passing now, I think the behavior is correct :)

Compilation error... (T_T)

FAILED:
obj/third_party/WebKit/Source/core/editing/unit_tests/InputMethodControllerTest.obj

ninja -t msvc -e environment.x86 -- E:\b\c\goma_client/gomacc.exe
"E:\b\depot_tools\win_toolchain\vs_files\d3cb0e37bdd120ad0ac4650b674b09e81be45616\VC\bin\amd64_x86/cl.exe"
/nologo /showIncludes /FC
@obj/third_party/WebKit/Source/core/editing/unit_tests/InputMethodControllerTest.obj.rsp
/c ../../third_party/WebKit/Source/core/editing/InputMethodControllerTest.cpp
/Foobj/third_party/WebKit/Source/core/editing/unit_tests/InputMethodControllerTest.obj
/Fd"obj/third_party/WebKit/Source/core/editing/unit_tests_cc.pdb"
e:\b\c\b\win\src\third_party\webkit\source\core\editing\inputmethodcontrollertest.cpp(1887):
error C2039: 'showMarkers': is not a member of 'blink::DocumentMarkerController'
e:\b\c\b\win\src\third_party\webkit\source\core\editing\markers\documentmarkercontroller.h(57):
note: see declaration of 'blink::DocumentMarkerController'

[rlanday, 2017-03-24 02:57:53]

The CQ bit was checked by rlanday@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-03-24 02:58:07]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-03-24 03:35:29]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-03-24 03:35:30]

Dry run: Try jobs failed on following builders:
  linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[rlanday, 2017-03-24 03:47:23]

On 2017/03/24 at 01:54:57, yosin wrote:
> On 2017/03/24 at 01:23:21, rlanday wrote:
> > Ok, all the tests are passing now, I think the behavior is correct :)
> 
> Compilation error... (T_T)
> 
> FAILED:
obj/third_party/WebKit/Source/core/editing/unit_tests/InputMethodControllerTest.obj

> ninja -t msvc -e environment.x86 -- E:\b\c\goma_client/gomacc.exe
"E:\b\depot_tools\win_toolchain\vs_files\d3cb0e37bdd120ad0ac4650b674b09e81be45616\VC\bin\amd64_x86/cl.exe"
/nologo /showIncludes /FC
@obj/third_party/WebKit/Source/core/editing/unit_tests/InputMethodControllerTest.obj.rsp
/c ../../third_party/WebKit/Source/core/editing/InputMethodControllerTest.cpp
/Foobj/third_party/WebKit/Source/core/editing/unit_tests/InputMethodControllerTest.obj
/Fd"obj/third_party/WebKit/Source/core/editing/unit_tests_cc.pdb"
>
e:\b\c\b\win\src\third_party\webkit\source\core\editing\inputmethodcontrollertest.cpp(1887):
error C2039: 'showMarkers': is not a member of 'blink::DocumentMarkerController'
>
e:\b\c\b\win\src\third_party\webkit\source\core\editing\markers\documentmarkercontroller.h(57):
note: see declaration of 'blink::DocumentMarkerController'

[rlanday, 2017-03-24 03:48:02]

Oops, hit "Send Message" too quick...

I had accidentally left some logging code in, the latest version compiles
properly (except for two trybots that appear to be acting up)

[rlanday, 2017-03-27 18:11:39]

The CQ bit was checked by rlanday@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-03-27 18:12:47]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-03-27 19:55:19]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-03-27 19:55:20]

Dry run: This issue passed the CQ dry run.

[rlanday, 2017-03-27 20:58:24]

On 2017/03/24 at 03:48:02, rlanday wrote:
> Oops, hit "Send Message" too quick...
> 
> I had accidentally left some logging code in, the latest version compiles
properly (except for two trybots that appear to be acting up)

[rlanday, 2017-03-27 20:59:06]

I got the trybots working (and slipped on the "Send Message" button again..."

[rlanday, 2017-03-27 20:59:12]

I got the trybots working (and slipped on the "Send Message" button again..."

[rlanday, 2017-03-29 00:55:28]

The CQ bit was checked by rlanday@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-03-29 00:56:14]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-03-29 02:57:47]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-03-29 02:57:48]

Dry run: Try jobs failed on following builders:
  ios-simulator on master.tryserver.chromium.mac (JOB_TIMED_OUT, build hasn't
started yet, builder probably lacks capacity)

[Xiaocheng, 2017-03-29 18:31:02]

There are two seemingly orthogonal change: Removal of
CEC::replaceTextInNodePreservingMarkers, and modification to DMC in its
didUpdateCharacterData callback. Could you clarify which one changes which
behavior, and if possible, split the patch?

Sorry for lengthening the iteration...

https://codereview.chromium.org/2755013004/diff/180001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/editing/InputMethodControllerTest.cpp
(right):

https://codereview.chromium.org/2755013004/diff/180001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/editing/InputMethodControllerTest.cpp:1786:
TEST_F(InputMethodControllerTest, Marker_Deletions) {
Could you split it into three tests?

https://codereview.chromium.org/2755013004/diff/180001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/editing/InputMethodControllerTest.cpp:1868:
TEST_F(InputMethodControllerTest, Marker_Insertions) {
Could you split it into two tests?

[Xiaocheng, 2017-03-29 18:35:19]

https://codereview.chromium.org/2755013004/diff/180001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/editing/markers/DocumentMarkerController.cpp
(right):

https://codereview.chromium.org/2755013004/diff/180001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/editing/markers/DocumentMarkerController.cpp:649:
bool DocumentMarkerController::relocateMarkerIfNeeded(
Can we implement this function directly in DocumentMarker, so that we don't need
to move it in the DocumentMarkerList patch?

[Xiaocheng, 2017-03-29 19:37:55]

Talked offline. This patch moves the functionality of the removed function to
DMC, so the two changes are not orthogonal.

Still looks good. The only requested change is to implement the offset
calculation for marker shifting directly in DocumentMarker.

[rlanday, 2017-03-29 20:30:37]

The CQ bit was checked by rlanday@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-03-29 20:31:15]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[rlanday, 2017-03-29 20:31:18]

On 2017/03/29 at 19:37:55, xiaochengh wrote:
> Talked offline. This patch moves the functionality of the removed function to
DMC, so the two changes are not orthogonal.
> 
> Still looks good. The only requested change is to implement the offset
calculation for marker shifting directly in DocumentMarker.

Ok, updated

[commit-bot: I haz the power, 2017-03-29 22:03:06]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-03-29 22:03:07]

Dry run: This issue passed the CQ dry run.

[Xiaocheng, 2017-03-29 23:25:08]

https://codereview.chromium.org/2755013004/diff/200001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/editing/markers/DocumentMarker.h (right):

https://codereview.chromium.org/2755013004/diff/200001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/editing/markers/DocumentMarker.h:170:
ShiftMarkerResult getShiftedMarkerPosition(unsigned offset,
Please also move its unit tests to this patch.

[rlanday, 2017-03-30 00:09:32]

The CQ bit was checked by rlanday@chromium.org to run a CQ dry run

[rlanday, 2017-03-30 00:09:44]

On 2017/03/29 at 23:25:08, xiaochengh wrote:
>
https://codereview.chromium.org/2755013004/diff/200001/third_party/WebKit/Sou...
> File third_party/WebKit/Source/core/editing/markers/DocumentMarker.h (right):
> 
>
https://codereview.chromium.org/2755013004/diff/200001/third_party/WebKit/Sou...
> third_party/WebKit/Source/core/editing/markers/DocumentMarker.h:170:
ShiftMarkerResult getShiftedMarkerPosition(unsigned offset,
> Please also move its unit tests to this patch.

Oops! Fixed

[commit-bot: I haz the power, 2017-03-30 00:10:22]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Xiaocheng, 2017-03-30 00:14:49]

lgtm

[yosin_UTC9, 2017-03-30 01:46:08]

https://codereview.chromium.org/2755013004/diff/220001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/editing/markers/DocumentMarker.h (right):

https://codereview.chromium.org/2755013004/diff/220001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/editing/markers/DocumentMarker.h:170:
ShiftMarkerResult getShiftedMarkerPosition(unsigned offset,
Since this function is not getter, please use another name, e.g.
computeShiftAmoutAt(), or another better name.
How about using Optional<T>, == std::optional?

We could write:
struct ShiftAmount { unsigned newStartOffset; unsinged newEndOffset };

Optional<ShiftAmount> cmputeShiftAmount(...) {
  ...
  if (no need to shift)
    return WTF::nullopt;
  return shiftAmount;
}

[commit-bot: I haz the power, 2017-03-30 02:12:55]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-03-30 02:12:56]

Dry run: Try jobs failed on following builders:
  ios-simulator on master.tryserver.chromium.mac (JOB_TIMED_OUT, build hasn't
started yet, builder probably lacks capacity)

[rlanday, 2017-03-30 02:28:00]

The CQ bit was checked by rlanday@chromium.org to run a CQ dry run

[rlanday, 2017-03-30 02:28:05]

On 2017/03/30 at 01:46:08, yosin wrote:
>
https://codereview.chromium.org/2755013004/diff/220001/third_party/WebKit/Sou...
> File third_party/WebKit/Source/core/editing/markers/DocumentMarker.h (right):
> 
>
https://codereview.chromium.org/2755013004/diff/220001/third_party/WebKit/Sou...
> third_party/WebKit/Source/core/editing/markers/DocumentMarker.h:170:
ShiftMarkerResult getShiftedMarkerPosition(unsigned offset,
> Since this function is not getter, please use another name, e.g.
computeShiftAmoutAt(), or another better name.
> How about using Optional<T>, == std::optional?
> 
> We could write:
> struct ShiftAmount { unsigned newStartOffset; unsinged newEndOffset };
> 
> Optional<ShiftAmount> cmputeShiftAmount(...) {
>   ...
>   if (no need to shift)
>     return WTF::nullopt;
>   return shiftAmount;
> }

Updated

[commit-bot: I haz the power, 2017-03-30 02:28:46]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-03-30 02:35:21]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-03-30 02:35:22]

Dry run: Try jobs failed on following builders:
  android_arm64_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_arm6...)
  linux_chromium_compile_dbg_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  win_clang on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_clang/builds/...)

[rlanday, 2017-03-30 02:55:46]

The CQ bit was checked by rlanday@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-03-30 02:56:25]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-03-30 04:46:22]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-03-30 04:46:23]

Dry run: This issue passed the CQ dry run.

[rlanday, 2017-04-06 20:50:44]

The CQ bit was checked by rlanday@chromium.org to run a CQ dry run

[rlanday, 2017-04-06 20:52:09]

The CQ bit was unchecked by rlanday@chromium.org

[commit-bot: I haz the power, 2017-04-06 20:52:25]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[rlanday, 2017-04-07 00:49:20]

The CQ bit was checked by rlanday@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-04-07 00:49:42]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[rlanday, 2017-04-07 00:50:58]

This CL has no dependencies, are there any remaining issues? I just split up the
test cases since I think I'm going to change one of the behaviors (whether or
not markers are split when removing text) in another CL

[commit-bot: I haz the power, 2017-04-07 03:26:42]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-04-07 03:26:43]

Dry run: Try jobs failed on following builders:
  linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[yosin_UTC9, 2017-04-10 03:22:09]

On 2017/04/07 at 00:50:58, rlanday wrote:
> This CL has no dependencies, are there any remaining issues? I just split up
the test cases since I think I'm going to change one of the behaviors (whether
or not markers are split when removing text) in another CL

Could you rebase and upload for grate blink renaming?

[rlanday, 2017-04-10 19:00:59]

On 2017/04/10 at 03:22:09, yosin wrote:
> Could you rebase and upload for grate blink renaming?

Ok, I've done that.

I also moved one more behavioral change from the "don't ever split
DocumentMarker" CL (https://codereview.chromium.org/2806683002) into this CL
(since xiaochengh asked me to): we're no longer splitting markers on a
deletion/zero-length replacement. Instead, we keep the endpoints in the same
location relative to the surrounding text and just remove the text in the
middle. So before if we had a marker on "abc" and deleted "b", we'd have "ac"
with markers on "a" and "c". Now we'll have "ac" with one marker on "ac".

[rlanday, 2017-04-10 20:09:52]

The CQ bit was checked by rlanday@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-04-10 20:10:22]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Xiaocheng, 2017-04-10 21:14:21]

still lgtm (seems to be the third one I gave to this CL)

[yosin_UTC9, 2017-04-12 01:47:36]

lgtm

[rlanday, 2017-04-12 18:21:23]

The CQ bit was checked by rlanday@chromium.org

[rlanday, 2017-04-12 18:21:24]

The patchset sent to the CQ was uploaded after l-g-t-m from
xiaochengh@chromium.org
Link to the patchset: https://codereview.chromium.org/2755013004/#ps400001
(title: "Rebase")

[commit-bot: I haz the power, 2017-04-12 18:22:27]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-04-12 19:44:45]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-04-12 19:44:47]

Try jobs failed on following builders:
  linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[rlanday, 2017-04-12 21:48:01]

The CQ bit was checked by rlanday@chromium.org

[commit-bot: I haz the power, 2017-04-12 21:49:27]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-04-12 23:27:52]

CQ is committing da patch.

Bot data: {"patchset_id": 400001, "attempt_start_ts": 1492033681918480,
"parent_rev": "12281bd8755744a457842d926b0df999dfd5db9e", "commit_rev":
"05cf1a871bb88c8bc0a14fd9034443dd8eae4305"}

[commit-bot: I haz the power, 2017-04-13 00:44:43]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-04-13 00:44:47]

Prior attempt to commit was detected, but we were not able to check whether the
issue was successfully committed. Please check Git history manually and re-check
CQ or close this issue as needed.

[rlanday, 2017-04-13 01:04:20]

The CQ bit was checked by rlanday@chromium.org

[aelias_OOO_until_Jul13, 2017-04-13 01:04:36]

The CQ bit was checked by aelias@chromium.org

[commit-bot: I haz the power, 2017-04-13 01:06:31]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-04-13 01:48:42]

CQ is committing da patch.

Bot data: {"patchset_id": 400001, "attempt_start_ts": 1492045476581410,
"parent_rev": "ede160c50d85e5fd546ec8134714b35c71c0e2f1", "commit_rev":
"d6ef615a1a2154d3435b9a09afcd8f6cd929f275"}

[commit-bot: I haz the power, 2017-04-13 01:49:30]

Description was changed from

==========
Improve how DocumentMarkerController updates markers in response to text edits

This is basically https://codereview.chromium.org/2692093003 with the changes to
make DocumentMarkerController use SynchronousMutationObserver split off into a
separate CL (https://codereview.chromium.org/2755953004).

I need DocumentMarkerController to update markers more gracefully in response to
edits in order to implement SuggestionSpan support on Android. For example, if
we have the string "word1 word2" marked with a DocumentMarker and
CompositeEditCommand::replaceTextInNode() is used to replace word1 or word2 with
different text, the new text should still be inside the marked range.

There's currently a method
CompositeEditCommand::replaceTextInNodePreservingMarkers(), which was added to
fix some particular case to support a marker type which no longer exists
(DeletedAutocorrection): https://bugs.webkit.org/show_bug.cgi?id=60555

This is no longer necessary after this CL since we can handle all the updates
more correctly inside DocumentMarkerController. For example, if we have a string
like "Initial text blah" and we add a marker around " text " and delete
"Initial" and "blah", we end up with " text ". Then
DeleteSelectionCommand::fixupWhitespace() runs, and replaces the spaces with
non-breaking spaces. The current behavior is that the NBSPs are no longer in the
marked range, since DocumentMarkerController doesn't currently support keeping
the replaced text in the marked range, but replaceTextInNodePreservingMarkers()
runs and adds new markers to the NBSPs. So we still end up with " text " being
marked but now we have three separate markers, which is undesirable for some
use cases (e.g. text suggestions, because we don't know what range of text we're
actually supposed to replace now). With the changes in this CL,
DocumentMarkerController is able to keep a single marker around " text ".

The changes to the expected output for InputMethodControllerTest may be helpful
in understanding the behavioral changes here.

BUG=672259
==========

to

==========
Improve how DocumentMarkerController updates markers in response to text edits

This is basically https://codereview.chromium.org/2692093003 with the changes to
make DocumentMarkerController use SynchronousMutationObserver split off into a
separate CL (https://codereview.chromium.org/2755953004).

I need DocumentMarkerController to update markers more gracefully in response to
edits in order to implement SuggestionSpan support on Android. For example, if
we have the string "word1 word2" marked with a DocumentMarker and
CompositeEditCommand::replaceTextInNode() is used to replace word1 or word2 with
different text, the new text should still be inside the marked range.

There's currently a method
CompositeEditCommand::replaceTextInNodePreservingMarkers(), which was added to
fix some particular case to support a marker type which no longer exists
(DeletedAutocorrection): https://bugs.webkit.org/show_bug.cgi?id=60555

This is no longer necessary after this CL since we can handle all the updates
more correctly inside DocumentMarkerController. For example, if we have a string
like "Initial text blah" and we add a marker around " text " and delete
"Initial" and "blah", we end up with " text ". Then
DeleteSelectionCommand::fixupWhitespace() runs, and replaces the spaces with
non-breaking spaces. The current behavior is that the NBSPs are no longer in the
marked range, since DocumentMarkerController doesn't currently support keeping
the replaced text in the marked range, but replaceTextInNodePreservingMarkers()
runs and adds new markers to the NBSPs. So we still end up with " text " being
marked but now we have three separate markers, which is undesirable for some
use cases (e.g. text suggestions, because we don't know what range of text we're
actually supposed to replace now). With the changes in this CL,
DocumentMarkerController is able to keep a single marker around " text ".

The changes to the expected output for InputMethodControllerTest may be helpful
in understanding the behavioral changes here.

BUG=672259

Review-Url: https://codereview.chromium.org/2755013004
Cr-Commit-Position: refs/heads/master@{#464236}
Committed:
https://chromium.googlesource.com/chromium/src/+/d6ef615a1a2154d3435b9a09afcd...
==========

[commit-bot: I haz the power, 2017-04-13 01:49:33]

Committed patchset #21 (id:400001) as
https://chromium.googlesource.com/chromium/src/+/d6ef615a1a2154d3435b9a09afcd...