Project: chromium Issues People Development process History Sign in
New issue
Advanced search Search tips
Issue 707867 Refactor DocumentMarkerController
Starred by 2 users Project Member Reported by rlanday@chromium.org, Apr 3 Back to list
Status: Started
Owner:
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: ----
Pri: 3
Type: Bug

Blocking:
issue 672259



Sign in to add a comment
Original design doc:
https://goo.gl/VVJlDz

Revised design doc:
https://goo.gl/PCxjw2
 
Blocking: 672259
Components: -Blink
Description: Show this description
Project Member Comment 3 by bugdroid1@chromium.org, Apr 5
The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/2cfc514b3ee3bc1c29d7e853421fe96fb7a9d7ed

commit 2cfc514b3ee3bc1c29d7e853421fe96fb7a9d7ed
Author: rlanday <rlanday@chromium.org>
Date: Wed Apr 05 20:05:35 2017

Rename DocumentMarker::activeMatch() to IsActiveMatch()

What: Rename DocumentMarker::activeMatch() to IsActiveMatch()
Why: Adding "Is" makes it very clear that this is an accessor returning a boolean value, and aligns with other boolean predicates, e.g. isFocused() etc.
Outcome: Improve readability and code health

Suggested by yosin: https://codereview.chromium.org/2801483002#msg20

BUG=707867
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2

Review-Url: https://codereview.chromium.org/2789373003
Cr-Commit-Position: refs/heads/master@{#462190}

[modify] https://crrev.com/2cfc514b3ee3bc1c29d7e853421fe96fb7a9d7ed/third_party/WebKit/Source/core/editing/markers/DocumentMarker.cpp
[modify] https://crrev.com/2cfc514b3ee3bc1c29d7e853421fe96fb7a9d7ed/third_party/WebKit/Source/core/editing/markers/DocumentMarker.h
[modify] https://crrev.com/2cfc514b3ee3bc1c29d7e853421fe96fb7a9d7ed/third_party/WebKit/Source/core/editing/markers/DocumentMarkerController.cpp
[modify] https://crrev.com/2cfc514b3ee3bc1c29d7e853421fe96fb7a9d7ed/third_party/WebKit/Source/core/paint/InlineTextBoxPainter.cpp
[modify] https://crrev.com/2cfc514b3ee3bc1c29d7e853421fe96fb7a9d7ed/third_party/WebKit/Source/core/paint/SVGInlineTextBoxPainter.cpp
[modify] https://crrev.com/2cfc514b3ee3bc1c29d7e853421fe96fb7a9d7ed/third_party/WebKit/Source/core/testing/Internals.cpp

Project Member Comment 4 by bugdroid1@chromium.org, Apr 5
The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/81e0a769c432ef75f388feca117cd6fac2a516a4

commit 81e0a769c432ef75f388feca117cd6fac2a516a4
Author: rlanday <rlanday@chromium.org>
Date: Wed Apr 05 20:20:57 2017

Rename DocumentMarker::setActiveMatch() to setIsActiveMatch()

Suggested by @yosin here:
https://codereview.chromium.org/2801483002#msg20

What: Rename DM::setActiveMatch() to setIsActiveMatch()
Why: Align with the rename of accessor method activeMatch() to IsActiveMatch() in https://codereview.chromium.org/2789373003
Outcome: Improve readability and code health

BUG=707867

Review-Url: https://codereview.chromium.org/2802533002
Cr-Commit-Position: refs/heads/master@{#462198}

[modify] https://crrev.com/81e0a769c432ef75f388feca117cd6fac2a516a4/third_party/WebKit/Source/core/editing/markers/DocumentMarker.cpp
[modify] https://crrev.com/81e0a769c432ef75f388feca117cd6fac2a516a4/third_party/WebKit/Source/core/editing/markers/DocumentMarker.h
[modify] https://crrev.com/81e0a769c432ef75f388feca117cd6fac2a516a4/third_party/WebKit/Source/core/editing/markers/DocumentMarkerController.cpp

Project Member Comment 5 by bugdroid1@chromium.org, Apr 6
The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/c198feadcc54ea92134df2dc3a5f4f390fcd9493

commit c198feadcc54ea92134df2dc3a5f4f390fcd9493
Author: rlanday <rlanday@chromium.org>
Date: Thu Apr 06 01:00:27 2017

Add DocumentMarker::MatchStatus enum for text match markers

Suggested by @yosin here:
https://codereview.chromium.org/2801483002#msg20

Modify the DocumentMarker() constructor for text match markers and DocumentMarkerController::addTextMatchMarker() to take an enum for whether or not the text match is active instead of a bool. This will make it more clear in call sites what the parameter means.

BUG=707867

Review-Url: https://codereview.chromium.org/2801483002
Cr-Commit-Position: refs/heads/master@{#462303}

[modify] https://crrev.com/c198feadcc54ea92134df2dc3a5f4f390fcd9493/third_party/WebKit/Source/core/editing/markers/DocumentMarker.cpp
[modify] https://crrev.com/c198feadcc54ea92134df2dc3a5f4f390fcd9493/third_party/WebKit/Source/core/editing/markers/DocumentMarker.h
[modify] https://crrev.com/c198feadcc54ea92134df2dc3a5f4f390fcd9493/third_party/WebKit/Source/core/editing/markers/DocumentMarkerController.cpp
[modify] https://crrev.com/c198feadcc54ea92134df2dc3a5f4f390fcd9493/third_party/WebKit/Source/core/editing/markers/DocumentMarkerController.h
[modify] https://crrev.com/c198feadcc54ea92134df2dc3a5f4f390fcd9493/third_party/WebKit/Source/core/editing/markers/DocumentMarkerControllerTest.cpp
[modify] https://crrev.com/c198feadcc54ea92134df2dc3a5f4f390fcd9493/third_party/WebKit/Source/core/testing/Internals.cpp
[modify] https://crrev.com/c198feadcc54ea92134df2dc3a5f4f390fcd9493/third_party/WebKit/Source/web/TextFinder.cpp

Project Member Comment 6 by bugdroid1@chromium.org, Apr 6
The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/3ca300989422ff59471397342e4789fb659290cf

commit 3ca300989422ff59471397342e4789fb659290cf
Author: rlanday <rlanday@chromium.org>
Date: Thu Apr 06 01:54:11 2017

Update window.internals.addTextMatchMarker() to take enum instead of bool

Suggested by @yosin here:
https://codereview.chromium.org/2801483002#msg20

We are changing DocumentMarkerController::addTextMatchMarker() to take an enum in https://codereview.chromium.org/2801483002 since it's unclear from looking at a callsite what "true" and "false" mean. This CL handles the window.internals changes separately since a bunch of tests have to be updated.

BUG=707867

Review-Url: https://codereview.chromium.org/2802543002
Cr-Commit-Position: refs/heads/master@{#462319}

[modify] https://crrev.com/3ca300989422ff59471397342e4789fb659290cf/third_party/WebKit/LayoutTests/fast/dom/documentmarker-add-adjacent-text.html
[modify] https://crrev.com/3ca300989422ff59471397342e4789fb659290cf/third_party/WebKit/LayoutTests/fast/dom/documentmarker-set-active.html
[modify] https://crrev.com/3ca300989422ff59471397342e4789fb659290cf/third_party/WebKit/LayoutTests/fast/scrolling/scrollbar-tickmarks-hittest.html
[modify] https://crrev.com/3ca300989422ff59471397342e4789fb659290cf/third_party/WebKit/LayoutTests/fast/scrolling/scrollbar-tickmarks-styled-after-onload.html
[modify] https://crrev.com/3ca300989422ff59471397342e4789fb659290cf/third_party/WebKit/LayoutTests/fast/scrolling/scrollbar-tickmarks-styled.html
[modify] https://crrev.com/3ca300989422ff59471397342e4789fb659290cf/third_party/WebKit/LayoutTests/paint/invalidation/compositing/text-match-highlight.html
[modify] https://crrev.com/3ca300989422ff59471397342e4789fb659290cf/third_party/WebKit/LayoutTests/paint/invalidation/text-match-document-change.html
[modify] https://crrev.com/3ca300989422ff59471397342e4789fb659290cf/third_party/WebKit/LayoutTests/paint/invalidation/text-match-pre-wrapped-text.html
[modify] https://crrev.com/3ca300989422ff59471397342e4789fb659290cf/third_party/WebKit/LayoutTests/paint/invalidation/text-match-svg.html
[modify] https://crrev.com/3ca300989422ff59471397342e4789fb659290cf/third_party/WebKit/LayoutTests/paint/invalidation/text-match-transparent-text.html
[modify] https://crrev.com/3ca300989422ff59471397342e4789fb659290cf/third_party/WebKit/LayoutTests/paint/invalidation/text-match.html
[modify] https://crrev.com/3ca300989422ff59471397342e4789fb659290cf/third_party/WebKit/LayoutTests/paint/text/text-match-highlights-big-line-height.html
[modify] https://crrev.com/3ca300989422ff59471397342e4789fb659290cf/third_party/WebKit/LayoutTests/svg/custom/text-match-highlight.html
[modify] https://crrev.com/3ca300989422ff59471397342e4789fb659290cf/third_party/WebKit/Source/core/testing/Internals.cpp
[modify] https://crrev.com/3ca300989422ff59471397342e4789fb659290cf/third_party/WebKit/Source/core/testing/Internals.h
[modify] https://crrev.com/3ca300989422ff59471397342e4789fb659290cf/third_party/WebKit/Source/core/testing/Internals.idl

Project Member Comment 7 by bugdroid1@chromium.org, Apr 11
The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/474939fc1d9344c7fdbbb12638d4d318e9c57192

commit 474939fc1d9344c7fdbbb12638d4d318e9c57192
Author: rlanday <rlanday@chromium.org>
Date: Tue Apr 11 00:10:22 2017

Add SplitTextNodeCommandTest

I am planning to modify the behavior of DocumentMarkerController::copyMarkers()
(to move the markers to the new list instead of leaving pointers to the same
markers, which have possibly been modified, in the source list) and decided to
write a test to make sure we don't break SplitTextNodeCommand.

BUG=707867

Review-Url: https://codereview.chromium.org/2802993007
Cr-Commit-Position: refs/heads/master@{#463459}

[modify] https://crrev.com/474939fc1d9344c7fdbbb12638d4d318e9c57192/third_party/WebKit/Source/core/editing/BUILD.gn
[modify] https://crrev.com/474939fc1d9344c7fdbbb12638d4d318e9c57192/third_party/WebKit/Source/core/editing/commands/SplitTextNodeCommand.h
[add] https://crrev.com/474939fc1d9344c7fdbbb12638d4d318e9c57192/third_party/WebKit/Source/core/editing/commands/SplitTextNodeCommandTest.cpp

Project Member Comment 8 by bugdroid1@chromium.org, Apr 11
The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/0dea0097ea24907ed4c0f30fb4180191f378f00e

commit 0dea0097ea24907ed4c0f30fb4180191f378f00e
Author: rlanday <rlanday@chromium.org>
Date: Tue Apr 11 02:58:57 2017

Clean up DocumentMarkerController::copyMarkers() (now it's moveMarkers())

This method is kind of weird since it leaves the markers in the source list but
sometimes changes one of them (if it spans a node boundary). In this CL, I'm
cleaning it up so it removes the markers from the source list after adding them
to the destination list.

BUG=707867

Review-Url: https://codereview.chromium.org/2810463002
Cr-Commit-Position: refs/heads/master@{#463509}

[modify] https://crrev.com/0dea0097ea24907ed4c0f30fb4180191f378f00e/third_party/WebKit/Source/core/editing/commands/SplitTextNodeCommand.cpp
[modify] https://crrev.com/0dea0097ea24907ed4c0f30fb4180191f378f00e/third_party/WebKit/Source/core/editing/markers/DocumentMarkerController.cpp
[modify] https://crrev.com/0dea0097ea24907ed4c0f30fb4180191f378f00e/third_party/WebKit/Source/core/editing/markers/DocumentMarkerController.h

Project Member Comment 9 by bugdroid1@chromium.org, Apr 12
The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/5e71603c48cc0898a933e13d266dbf036d034c42

commit 5e71603c48cc0898a933e13d266dbf036d034c42
Author: rlanday <rlanday@chromium.org>
Date: Wed Apr 12 02:14:46 2017

Add some tests for DocumentMarkerController::removeMarkers()

I'm changing the behavior of removeMarkers() in https://codereview.chromium.org/2806683002 to always remove markers rather than splitting and noticed that the current behavior doesn't seem to be covered by any tests; I'm adding tests for the current behavior to make it clear in that CL how the behavior is being changed.

BUG=707867

Review-Url: https://codereview.chromium.org/2812723002
Cr-Commit-Position: refs/heads/master@{#463899}

[modify] https://crrev.com/5e71603c48cc0898a933e13d266dbf036d034c42/third_party/WebKit/Source/core/editing/markers/DocumentMarkerControllerTest.cpp

Project Member Comment 10 by bugdroid1@chromium.org, Apr 13
The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/98f1927737cd1c3fd5d471b74cc94333e9e37972

commit 98f1927737cd1c3fd5d471b74cc94333e9e37972
Author: rlanday <rlanday@chromium.org>
Date: Thu Apr 13 20:22:26 2017

Don't ever split DocumentMarkers on remove

Right now some of the DocumentMarkerController::removeMarkers() methods take an
enum flag, RemovePartiallyOverlappingMarkerOrNot that controls what the method
does with markers that partially intersect the passed-in range but do not fall
entirely within it. If the value DoNotRemovePartiallyOverlappingMarker is
passed, the section in the passed-in range is removed and the remaining piece(s)
of the marker are left in place (otherwise the entire marker is removed). If the
passed-in range contains part of the interior of the marker but neither the
start nor end position, this means the marker is split into two pieces, which
requires copying the marker. yosin thinks copying markers is weird and we should
try to avoid ever having to do it. I believe this is possible if we eliminate
this flag and just never split markers on removal.

I don't think any of the currently-existing marker types (Spelling, Grammar,
Composition, TextMatch) ever make sense to split, and neither do the new types
I'm going to add (Suggestion, SuggestionHighlight). The only test cases I had to
update after this change are ones I added based on the current behavior.

BUG=707867

Review-Url: https://codereview.chromium.org/2806683002
Cr-Commit-Position: refs/heads/master@{#464524}

[modify] https://crrev.com/98f1927737cd1c3fd5d471b74cc94333e9e37972/third_party/WebKit/Source/core/editing/markers/DocumentMarkerController.cpp
[modify] https://crrev.com/98f1927737cd1c3fd5d471b74cc94333e9e37972/third_party/WebKit/Source/core/editing/markers/DocumentMarkerController.h
[modify] https://crrev.com/98f1927737cd1c3fd5d471b74cc94333e9e37972/third_party/WebKit/Source/core/editing/markers/DocumentMarkerControllerTest.cpp
[modify] https://crrev.com/98f1927737cd1c3fd5d471b74cc94333e9e37972/third_party/WebKit/Source/core/editing/spellcheck/HotModeSpellCheckRequester.cpp
[modify] https://crrev.com/98f1927737cd1c3fd5d471b74cc94333e9e37972/third_party/WebKit/Source/core/editing/spellcheck/SpellChecker.cpp

Project Member Comment 11 by bugdroid1@chromium.org, Apr 17
The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/73e6a569acfd3b3b5ecc4f0ec2c0945740e9cd2c

commit 73e6a569acfd3b3b5ecc4f0ec2c0945740e9cd2c
Author: rlanday <rlanday@chromium.org>
Date: Mon Apr 17 08:52:08 2017

Remove unused DocumentMarker comparison operators

It was pointed out in a code review that these are unused:
https://codereview.chromium.org/2780313002/diff/20001/third_party/WebKit/Source/core/editing/markers/DocumentMarker.h#newcode166

BUG=707867

Review-Url: https://codereview.chromium.org/2785023002
Cr-Commit-Position: refs/heads/master@{#464908}

[modify] https://crrev.com/73e6a569acfd3b3b5ecc4f0ec2c0945740e9cd2c/third_party/WebKit/Source/core/editing/markers/DocumentMarker.h

Project Member Comment 12 by bugdroid1@chromium.org, Apr 18
The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/b59bf346862b89daee2a4f06f02f5d7df59f9164

commit b59bf346862b89daee2a4f06f02f5d7df59f9164
Author: rlanday <rlanday@chromium.org>
Date: Tue Apr 18 01:50:38 2017

Refactor DocumentMarkerController on top of new DocumentMarkerListEditor class

Based on yosin's comment on a previous version of this CL:
https://codereview.chromium.org/2812423002#msg7

This CL is step 1, "Introduce DMListEditor with in-place changes of DMC: Except for DMListEditor.h, changes are small". In this CL, I'm pulling out some of the
DocumentMarkerController implementation code into a DocumentMarkerListEditor class.
In a future CL, I will move this class into its own .h/.cpp files (I'm leaving it
in place for now to make the patch smaller).

BUG=707867

Review-Url: https://codereview.chromium.org/2812423002
Cr-Commit-Position: refs/heads/master@{#465092}

[modify] https://crrev.com/b59bf346862b89daee2a4f06f02f5d7df59f9164/third_party/WebKit/Source/core/editing/markers/DocumentMarkerController.cpp
[modify] https://crrev.com/b59bf346862b89daee2a4f06f02f5d7df59f9164/third_party/WebKit/Source/core/editing/markers/DocumentMarkerController.h

Project Member Comment 13 by bugdroid1@chromium.org, Apr 18
The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/a92abb9ad049f637596371997cc125f182ae3cc3

commit a92abb9ad049f637596371997cc125f182ae3cc3
Author: rlanday <rlanday@chromium.org>
Date: Tue Apr 18 03:19:14 2017

Refactor DocumentMarkerController::MoveMarkers()

As we (yosin, xiaochengh, and I) discussed, I'm splitting the implementation
changes for this method I previously was including in
https://codereview.chromium.org/2812423002 into a separate CL.

We want to separate out the implementation changes we need to pull out the main
part of this method into a method on DocumentMarkerListEditor, and actually
adding that method.

This CL needs the AddMarker() method I'm adding in that CL, so I've removed the
MoveMarkers() changes entirely from that one and will upload a third CL that
adds DocumentMarkerListEditor::MoveMarkers().

BUG=707867

Review-Url: https://codereview.chromium.org/2819173002
Cr-Commit-Position: refs/heads/master@{#465124}

[modify] https://crrev.com/a92abb9ad049f637596371997cc125f182ae3cc3/third_party/WebKit/Source/core/editing/markers/DocumentMarkerController.cpp

Project Member Comment 14 by bugdroid1@chromium.org, Apr 18
The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/b5a0bdbe8b745676efac4996f50c0558bb17b839

commit b5a0bdbe8b745676efac4996f50c0558bb17b839
Author: rlanday <rlanday@chromium.org>
Date: Tue Apr 18 03:38:37 2017

Add DocumentMarkerController::ListForType()

Helper method that makes it slightly cleaner to look up the MarkerList* for a
given type when we're holding the MarkerLists* for a Node. Alternatively, we
could have a method that takes a Node* instead of a MarkerLists*, but then we'd
lose the optimization in the call sites where we can return early if there's no
MarkerLists* for the given Node.

I found two places to use this in the current version of the code, and there
will be more after we rewrite some of the loops to use the DocumentMarkerList
interface being added in https://codereview.chromium.org/2820633002 .

BUG=707867

Review-Url: https://codereview.chromium.org/2822963002
Cr-Commit-Position: refs/heads/master@{#465127}

[modify] https://crrev.com/b5a0bdbe8b745676efac4996f50c0558bb17b839/third_party/WebKit/Source/core/editing/markers/DocumentMarkerController.cpp
[modify] https://crrev.com/b5a0bdbe8b745676efac4996f50c0558bb17b839/third_party/WebKit/Source/core/editing/markers/DocumentMarkerController.h

Project Member Comment 15 by bugdroid1@chromium.org, Apr 20
The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/954977d31f907b814ca4aef51bbd76fb9a55f1f5

commit 954977d31f907b814ca4aef51bbd76fb9a55f1f5
Author: rlanday <rlanday@chromium.org>
Date: Thu Apr 20 03:56:13 2017

Add DocumentMarkerListEditor::MoveMarkers()

Follow-up to https://codereview.chromium.org/2812423002/ (add
DocumentMarkerListEditor class) and https://codereview.chromium.org/2819173002
(refactor DocumentMarkerController::MoveMarkers()): this CL moves some of the
DocumentMarkerController::MoveMarkers() code into a new
DocumentMarkerListEditor::MoveMarkers() method

BUG=707867

Review-Url: https://codereview.chromium.org/2820983002
Cr-Commit-Position: refs/heads/master@{#465894}

[modify] https://crrev.com/954977d31f907b814ca4aef51bbd76fb9a55f1f5/third_party/WebKit/Source/core/editing/markers/DocumentMarkerController.cpp
[modify] https://crrev.com/954977d31f907b814ca4aef51bbd76fb9a55f1f5/third_party/WebKit/Source/core/editing/markers/DocumentMarkerController.h

Project Member Comment 16 by bugdroid1@chromium.org, Apr 20
The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/74ad4fde9304fc6f71a165462360662c5bfe7924

commit 74ad4fde9304fc6f71a165462360662c5bfe7924
Author: rlanday <rlanday@chromium.org>
Date: Thu Apr 20 09:48:32 2017

Convert static comparator functions in DocumentMarkerController to lambdas

After https://codereview.chromium.org/2818873002, we will need to use
EndsBefore() in more than one file. yosin advised that these functions are
small enough that they should be lambdas rather than trying to add them e.g. as
member functions on DocumentMarker to avoid duplication.

BUG=707867

Review-Url: https://codereview.chromium.org/2828093004
Cr-Commit-Position: refs/heads/master@{#465960}

[modify] https://crrev.com/74ad4fde9304fc6f71a165462360662c5bfe7924/third_party/WebKit/Source/core/editing/markers/DocumentMarkerController.cpp

Project Member Comment 18 by bugdroid1@chromium.org, Apr 21 (6 days ago)
The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/2d9f45d2abbc726557c6f00ba76e6128c478416e

commit 2d9f45d2abbc726557c6f00ba76e6128c478416e
Author: rlanday <rlanday@chromium.org>
Date: Fri Apr 21 08:48:32 2017

Rewrite loops in DocumentMarkerController to use ListForType()

xiaochengh pointed out in another code review that this method is not being used
consistently:
https://codereview.chromium.org/2820633002#msg26

After this CL, it will be used consistently.

BUG=707867

Review-Url: https://codereview.chromium.org/2830033002
Cr-Commit-Position: refs/heads/master@{#466290}

[modify] https://crrev.com/2d9f45d2abbc726557c6f00ba76e6128c478416e/third_party/WebKit/Source/core/editing/markers/DocumentMarkerController.cpp

Project Member Comment 19 by bugdroid1@chromium.org, Apr 21 (6 days ago)
The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/27f9eb47ba02349a8b5e850c597d4778b6c38846

commit 27f9eb47ba02349a8b5e850c597d4778b6c38846
Author: rlanday <rlanday@chromium.org>
Date: Fri Apr 21 10:22:10 2017

Rename DocumentMarkerController::RemoveMarkers() to RemoveMarkersInRange()

yosin pointed out that it's confusing to have so many methods named
RemoveMarkers():
https://codereview.chromium.org/2820633002#msg30

This is the first of several renaming CLs I'm going to upload

BUG=707867

Review-Url: https://codereview.chromium.org/2830213002
Cr-Commit-Position: refs/heads/master@{#466302}

[modify] https://crrev.com/27f9eb47ba02349a8b5e850c597d4778b6c38846/third_party/WebKit/Source/core/editing/markers/DocumentMarkerController.cpp
[modify] https://crrev.com/27f9eb47ba02349a8b5e850c597d4778b6c38846/third_party/WebKit/Source/core/editing/markers/DocumentMarkerController.h
[modify] https://crrev.com/27f9eb47ba02349a8b5e850c597d4778b6c38846/third_party/WebKit/Source/core/editing/markers/DocumentMarkerControllerTest.cpp
[modify] https://crrev.com/27f9eb47ba02349a8b5e850c597d4778b6c38846/third_party/WebKit/Source/core/editing/spellcheck/HotModeSpellCheckRequester.cpp
[modify] https://crrev.com/27f9eb47ba02349a8b5e850c597d4778b6c38846/third_party/WebKit/Source/core/editing/spellcheck/SpellCheckRequester.cpp
[modify] https://crrev.com/27f9eb47ba02349a8b5e850c597d4778b6c38846/third_party/WebKit/Source/core/editing/spellcheck/SpellChecker.cpp

Project Member Comment 20 by bugdroid1@chromium.org, Apr 21 (6 days ago)
The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/c0f93a9a83297a1046670164881e6bd0ce2b66d6

commit c0f93a9a83297a1046670164881e6bd0ce2b66d6
Author: rlanday <rlanday@chromium.org>
Date: Fri Apr 21 10:58:19 2017

Rename DocumentMarkerController::RemoveMarkers() to RemoveMarkersInternal()

yosin pointed out that it's confusing to have so many methods named
RemoveMarkers():
https://codereview.chromium.org/2820633002#msg30

This is the second of several renaming CLs I'm going to upload

BUG=707867

Review-Url: https://codereview.chromium.org/2829723006
Cr-Commit-Position: refs/heads/master@{#466309}

[modify] https://crrev.com/c0f93a9a83297a1046670164881e6bd0ce2b66d6/third_party/WebKit/Source/core/editing/markers/DocumentMarkerController.cpp
[modify] https://crrev.com/c0f93a9a83297a1046670164881e6bd0ce2b66d6/third_party/WebKit/Source/core/editing/markers/DocumentMarkerController.h

Project Member Comment 21 by bugdroid1@chromium.org, Apr 22 (5 days ago)
The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/ad6b1f378b7706541d97b227638ba93c446fc8f1

commit ad6b1f378b7706541d97b227638ba93c446fc8f1
Author: rlanday <rlanday@chromium.org>
Date: Sat Apr 22 00:57:29 2017

Refactor DocumentMarkerController::RemoveMarkers(const MarkerRemoverPredicate&)

MarkerRemoverPredicate seems to have been added with some amount of generality
in mind, but the way it's written it only serves a single purpose, so I'm
refactoring the RemoveMarkers() method that takes it to just take a vector of
words instead.

BUG=707867

Review-Url: https://codereview.chromium.org/2833753004
Cr-Commit-Position: refs/heads/master@{#466516}

[modify] https://crrev.com/ad6b1f378b7706541d97b227638ba93c446fc8f1/third_party/WebKit/Source/core/editing/markers/DocumentMarkerController.cpp
[modify] https://crrev.com/ad6b1f378b7706541d97b227638ba93c446fc8f1/third_party/WebKit/Source/core/editing/markers/DocumentMarkerController.h
[modify] https://crrev.com/ad6b1f378b7706541d97b227638ba93c446fc8f1/third_party/WebKit/Source/core/editing/spellcheck/SpellChecker.cpp

Project Member Comment 22 by bugdroid1@chromium.org, Apr 22 (5 days ago)
Project Member Comment 23 by bugdroid1@chromium.org, Apr 24 (3 days ago)
Project Member Comment 24 by bugdroid1@chromium.org, Apr 24 (3 days ago)
The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/9ad9f6860b4d6a4ec7f7f975b2c99672e02d5d49

commit 9ad9f6860b4d6a4ec7f7f975b2c99672e02d5d49
Author: rlanday <rlanday@chromium.org>
Date: Mon Apr 24 08:30:44 2017

Make DocumentMarkerController::AddMarker() take a pointer instead of a const ref

Right now this method can take a const reference despite DMC possibly later
needing to edit the marker because the current implementation always copies the
marker (to construct a RenderedDocumentMarker). However, this is not guaranteed
by the API, and will be changed in the future once we no longer construct
RenderedDocumentMarkers for all MarkerTypes. So this method needs to be changed
to take a non-const pointer (could use a ref but I think doing so is discouraged
by the style guide). Also, the methods that call AddMarker() need to pass
pointers to heap-allocated objects.

BUG=707867

Review-Url: https://codereview.chromium.org/2833043002
Cr-Commit-Position: refs/heads/master@{#466596}

[modify] https://crrev.com/9ad9f6860b4d6a4ec7f7f975b2c99672e02d5d49/third_party/WebKit/Source/core/editing/markers/DocumentMarkerController.cpp
[modify] https://crrev.com/9ad9f6860b4d6a4ec7f7f975b2c99672e02d5d49/third_party/WebKit/Source/core/editing/markers/DocumentMarkerController.h

Project Member Comment 25 by bugdroid1@chromium.org, Yesterday (40 hours ago)
The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/a487ccbcee73cf847f61aaf0c35b09447dfa3d73

commit a487ccbcee73cf847f61aaf0c35b09447dfa3d73
Author: rlanday <rlanday@chromium.org>
Date: Wed Apr 26 03:58:28 2017

Remove DocumentMarker::SpellCheckClientMarkers()

This is redundant with DocumentMarker::MisspellingMarkers().

Two arguments for keeping the MisspellingMarkers() name instead of this one:

- "MisspellingMarkers" is shorter than "SpellCheckClientMarkers"

- MisspellingMarkers() has nine callsites, SpellCheckClientMarkers() only has
  one

BUG=707867

Review-Url: https://codereview.chromium.org/2836393002
Cr-Commit-Position: refs/heads/master@{#467226}

[modify] https://crrev.com/a487ccbcee73cf847f61aaf0c35b09447dfa3d73/third_party/WebKit/Source/core/editing/markers/DocumentMarker.h
[modify] https://crrev.com/a487ccbcee73cf847f61aaf0c35b09447dfa3d73/third_party/WebKit/Source/core/editing/spellcheck/SpellCheckRequester.cpp

Project Member Comment 26 by bugdroid1@chromium.org, Today (18 hours ago)
The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/c84b714f7a9730eeae4a4ccd84ac1582a6074926

commit c84b714f7a9730eeae4a4ccd84ac1582a6074926
Author: rlanday <rlanday@chromium.org>
Date: Thu Apr 27 02:00:17 2017

Add DocumentMarkerList interface and GenericDocumentMarkerListImpl

Based on step 3 of yosin's plan:
https://codereview.chromium.org/2812423002#msg7

This CL does the following:

- Introduces a DocumentMarkerList interface

- Introduces GenericDocumentMarkerListImpl, an implementation
  of DocumentMarkerList that works for all marker types and is implemented on top
  of DocumentMarkerListEditor

- Refactors DocumentMarkerController to use GenericDocumentMarkerListImpl instead
  of using DocumentMarkerListEditor directly

BUG=707867

Review-Url: https://codereview.chromium.org/2820633002
Cr-Commit-Position: refs/heads/master@{#467549}

[modify] https://crrev.com/c84b714f7a9730eeae4a4ccd84ac1582a6074926/third_party/WebKit/Source/core/editing/BUILD.gn
[modify] https://crrev.com/c84b714f7a9730eeae4a4ccd84ac1582a6074926/third_party/WebKit/Source/core/editing/markers/DocumentMarkerController.cpp
[modify] https://crrev.com/c84b714f7a9730eeae4a4ccd84ac1582a6074926/third_party/WebKit/Source/core/editing/markers/DocumentMarkerController.h
[add] https://crrev.com/c84b714f7a9730eeae4a4ccd84ac1582a6074926/third_party/WebKit/Source/core/editing/markers/DocumentMarkerList.cpp
[add] https://crrev.com/c84b714f7a9730eeae4a4ccd84ac1582a6074926/third_party/WebKit/Source/core/editing/markers/DocumentMarkerList.h
[modify] https://crrev.com/c84b714f7a9730eeae4a4ccd84ac1582a6074926/third_party/WebKit/Source/core/editing/markers/DocumentMarkerListEditor.cpp
[modify] https://crrev.com/c84b714f7a9730eeae4a4ccd84ac1582a6074926/third_party/WebKit/Source/core/editing/markers/DocumentMarkerListEditor.h
[add] https://crrev.com/c84b714f7a9730eeae4a4ccd84ac1582a6074926/third_party/WebKit/Source/core/editing/markers/GenericDocumentMarkerListImpl.cpp
[add] https://crrev.com/c84b714f7a9730eeae4a4ccd84ac1582a6074926/third_party/WebKit/Source/core/editing/markers/GenericDocumentMarkerListImpl.h

Project Member Comment 27 by bugdroid1@chromium.org, Today (13 hours ago)
The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/4dbe8b7c5c9762619c4baf47e4650a9d8139d48f

commit 4dbe8b7c5c9762619c4baf47e4650a9d8139d48f
Author: rlanday <rlanday@chromium.org>
Date: Thu Apr 27 06:50:18 2017

Prepare DocumentMarkerController for using different DocumentMarkerList impls

I'm going to start writing different implementations of DocumentMarkerList for
the different MarkerTypes. This CL prepares us to be able to easily specify the
marker list class we want to use for each MarkerType.

BUG=707867

Review-Url: https://codereview.chromium.org/2826493003
Cr-Commit-Position: refs/heads/master@{#467609}

[modify] https://crrev.com/4dbe8b7c5c9762619c4baf47e4650a9d8139d48f/third_party/WebKit/Source/core/editing/markers/DocumentMarkerController.cpp

Sign in to add a comment