[DMC #5.55] Add DocumentMarkerList::MarkerType()

Add DocumentMarkerList::MarkerType()

We need a method on DocumentMarkerList that returns the marker type the list
supports so we know when we can safely cast a DocumentMarkerList* to an impl
type.

This CL adds two subclasses of SpellCheckMarkerListImpl, SpellingMarkerListImpl,
and GrammarMarkerListImpl, so we can implement this method.

Note that adding this method also requires modifying
GenericDocumentMarkerListImpl to take a MarkerType param (we're going to
eliminate GenericDocumentMarkerListImpl once we add TextMatchMarkerListImpl but
we need this for now).

BUG=707867
Review-Url: https://codereview.chromium.org/2868413002
Cr-Commit-Position: refs/heads/master@{#472206}
Committed: https://chromium.googlesource.com/chromium/src/+/d8b7adbe4b26fedd0a87ffd74aaac141a4694490

[rlanday, 2017-05-11 15:49:46]

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

[rlanday, 2017-05-11 15:50:44]

Note: I also have another version of this CL where I added a marker_type_ member
variable to SpellCheckMarkerListImpl instead of subclassing it:
https://codereview.chromium.org/2872353002

[Xiaocheng, 2017-05-11 18:40:17]

I prefer having a marker_type_ member in SpellCheckMarkerListImpl. Reasons:

1. One less class
2. No sub-classing
3. I guess we need to keep GenDMLImpl to ease introduction of new marker types.
GenDMLImpl has to store such a member.

[yosin_UTC9, 2017-05-12 04:59:03]

On 2017/05/11 at 18:40:17, xiaochengh wrote:
> I prefer having a marker_type_ member in SpellCheckMarkerListImpl. Reasons:
> 
> 1. One less class
> 2. No sub-classing
> 3. I guess we need to keep GenDMLImpl to ease introduction of new marker
types. GenDMLImpl has to store such a member.

I don't prefer to having SpllCheckMarkerListImpl::marker_type_ to reduce memory
usage.
I know this is small amount, 4K markers in different node take 4K *
sizeof(DocumentMarkery), assume 4, == 16K.

I think we'll get rid of GenDMLImpl once we have specific DMListImpl for each
type, as promised.
This is reason why we introduce DMListEditor.

[yosin_UTC9, 2017-05-12 05:00:04]

Good shape.
Wait for xiaochengh@'s reply about having
SpellCheckerMarkerListImpl::maker_type_

https://codereview.chromium.org/2868413002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/editing/markers/CompositionMarkerListImpl.h
(right):

https://codereview.chromium.org/2868413002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/editing/markers/CompositionMarkerListImpl.h:23:
DocumentMarker::MarkerType MarkerType() const;
nit: s/const/const final/

https://codereview.chromium.org/2868413002/diff/20001/third_party/WebKit/Sour...
File
third_party/WebKit/Source/core/editing/markers/GenericDocumentMarkerListImpl.h
(right):

https://codereview.chromium.org/2868413002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/editing/markers/GenericDocumentMarkerListImpl.h:22:
DocumentMarker::MarkerType MarkerType() const;
nit: s/const/const final/

https://codereview.chromium.org/2868413002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/editing/markers/GrammarMarkerListImpl.h
(right):

https://codereview.chromium.org/2868413002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/editing/markers/GrammarMarkerListImpl.h:12: class
GrammarMarkerListImpl : public SpellCheckMarkerListImpl {
Please add class documentation.
nit: s/:/final : /

https://codereview.chromium.org/2868413002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/editing/markers/GrammarMarkerListImpl.h:14:
DocumentMarker::MarkerType MarkerType() const;
nit: s/const/const final/

https://codereview.chromium.org/2868413002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/editing/markers/GrammarMarkerListImpl.h:15: };
We should have DISALLOW_COPY_AND_ASSIGN()

https://codereview.chromium.org/2868413002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/editing/markers/SpellCheckMarkerListImpl.h
(right):

https://codereview.chromium.org/2868413002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/editing/markers/SpellCheckMarkerListImpl.h:19:
SpellCheckMarkerListImpl() = default;
nit: We should get move ctor of SpellCheckMarkerListImpl to protect section.

https://codereview.chromium.org/2868413002/diff/20001/third_party/WebKit/Sour...
File
third_party/WebKit/Source/core/editing/markers/SpellCheckMarkerListImplTest.cpp
(right):

https://codereview.chromium.org/2868413002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/editing/markers/SpellCheckMarkerListImplTest.cpp:13:
class SpellCheckMarkerListImplForTesting : public SpellCheckMarkerListImpl {
We can use SpellingMarkerList and GrammarMakrerList here instead of introducing
test only class.

https://codereview.chromium.org/2868413002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/editing/markers/SpellingMarkerListImpl.h
(right):

https://codereview.chromium.org/2868413002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/editing/markers/SpellingMarkerListImpl.h:12:
class SpellingMarkerListImpl : public SpellCheckMarkerListImpl {
Please have class comment.
nit: s/:/ final :/

https://codereview.chromium.org/2868413002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/editing/markers/SpellingMarkerListImpl.h:14:
DocumentMarker::MarkerType MarkerType() const;
nit: s/const/const final/

[Xiaocheng, 2017-05-12 18:23:00]

Memory consumption is a strong reason not to introduce an extra field. Let's go
with this direction.

[rlanday, 2017-05-12 20:06:16]

Updated

[rlanday, 2017-05-12 21:07:02]

https://codereview.chromium.org/2868413002/diff/20001/third_party/WebKit/Sour...
File
third_party/WebKit/Source/core/editing/markers/SpellCheckMarkerListImplTest.cpp
(right):

https://codereview.chromium.org/2868413002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/editing/markers/SpellCheckMarkerListImplTest.cpp:13:
class SpellCheckMarkerListImplForTesting : public SpellCheckMarkerListImpl {
On 2017/05/12 at 05:00:04, yosin_UTC9 wrote:
> We can use SpellingMarkerList and GrammarMakrerList here instead of
introducing
> test only class.

Can you please clarify what you mean by "and"? Do you mean I should just use
SpellingMarkerListImpl, or that I should write a parameterized test that runs
all the test cases with both SpellingMarkerListImpl and GrammarMarkerListImpl,
or is there another option you want me to do?

[yosin_UTC9, 2017-05-15 04:03:04]

https://codereview.chromium.org/2868413002/diff/20001/third_party/WebKit/Sour...
File
third_party/WebKit/Source/core/editing/markers/SpellCheckMarkerListImplTest.cpp
(right):

https://codereview.chromium.org/2868413002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/editing/markers/SpellCheckMarkerListImplTest.cpp:13:
class SpellCheckMarkerListImplForTesting : public SpellCheckMarkerListImpl {
On 2017/05/12 at 21:07:02, rlanday wrote:
> On 2017/05/12 at 05:00:04, yosin_UTC9 wrote:
> > We can use SpellingMarkerList and GrammarMakrerList here instead of
introducing
> > test only class.
> 
> Can you please clarify what you mean by "and"? Do you mean I should just use
SpellingMarkerListImpl, or that I should write a parameterized test that runs
all the test cases with both SpellingMarkerListImpl and GrammarMarkerListImpl,
or is there another option you want me to do?

I propose having only "SpellingMarkerListImplTest.cpp" and
"GrammerMarkerListImpl.cpp":
 - SpellingMarkerListImpl: test all SpellingCheckerMakrerListImpl functions +
type checks
 - GrammarMakerListImpl: type checks + a comment mentions
"GrammerMarkerListImpl's member functions are implemented in
SpellCheckingMarkerListImpl and they are testing SpllingMarkerListImpl".

For simplicity and avoid test code duplication.

[rlanday, 2017-05-15 18:28:55]

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

[commit-bot: I haz the power, 2017-05-15 18:29: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-05-15 18:29:57]

On 2017/05/15 at 04:03:04, yosin wrote:
> I propose having only "SpellingMarkerListImplTest.cpp" and
"GrammerMarkerListImpl.cpp":
>  - SpellingMarkerListImpl: test all SpellingCheckerMakrerListImpl functions +
type checks
>  - GrammarMakerListImpl: type checks + a comment mentions
"GrammerMarkerListImpl's member functions are implemented in
SpellCheckingMarkerListImpl and they are testing SpllingMarkerListImpl".

Updated

[commit-bot: I haz the power, 2017-05-15 18:36:16]

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

[commit-bot: I haz the power, 2017-05-15 18:36:17]

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...)
  android_clang_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_clan...)
  android_compile_dbg on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_comp...)

[yosin_UTC9, 2017-05-16 01:20:49]

Please add "GrammarMarkerListImplTest.cpp" into this patch.

"analyze" step said:

ERROR at //third_party/WebKit/Source/core/editing/BUILD.gn:265:1: Source file
not found.
source_set("unit_tests") {
^-------------------------
The target:
  //third_party/WebKit/Source/core/editing:unit_tests
has a source file:
  //third_party/WebKit/Source/core/editing/markers/GrammarMarkerListImplTest.cpp
which was not found.

https://codereview.chromium.org/2868413002/diff/60001/third_party/WebKit/Sour...
File
third_party/WebKit/Source/core/editing/markers/GenericDocumentMarkerListImpl.h
(right):

https://codereview.chromium.org/2868413002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/editing/markers/GenericDocumentMarkerListImpl.h:42:
DocumentMarker::MarkerType marker_type_;
nit: s/DocumentMarker::MarkerType/const DocumentMarker::MarkerType/

Because of we can't change |GenericDMListImpl::marker_type_| once it is
constructed.

https://codereview.chromium.org/2868413002/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/editing/markers/GrammarMarkerListImpl.h
(right):

https://codereview.chromium.org/2868413002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/editing/markers/GrammarMarkerListImpl.h:20:
DISALLOW_COPY_AND_ASSIGN(GrammarMarkerListImpl);
nit: |DISALLOW_COPY_AND_ASSIGN()| should be in private section.

https://codereview.chromium.org/2868413002/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/editing/markers/SpellingMarkerListImpl.h
(right):

https://codereview.chromium.org/2868413002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/editing/markers/SpellingMarkerListImpl.h:20:
DISALLOW_COPY_AND_ASSIGN(SpellingMarkerListImpl);
nit: |DISALLOW_COPY_AND_ASSIGN()| should be in private section.

[rlanday, 2017-05-16 02:44:56]

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

[commit-bot: I haz the power, 2017-05-16 02:45: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-05-16 02:50:38]

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

[rlanday, 2017-05-16 02:50:55]

Updated. Sorry about the missing file :(

[commit-bot: I haz the power, 2017-05-16 02:50:59]

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-05-16 04:09:30]

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

[commit-bot: I haz the power, 2017-05-16 04:09:31]

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_...)

[yosin_UTC9, 2017-05-16 04:35:18]

lgtm % very small nit

https://codereview.chromium.org/2868413002/diff/100001/third_party/WebKit/Sou...
File
third_party/WebKit/Source/core/editing/markers/GrammarMarkerListImplTest.cpp
(right):

https://codereview.chromium.org/2868413002/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/editing/markers/GrammarMarkerListImplTest.cpp:14:
// GrammarMarkerListImplTest.cpp.
nit: s/GrammarMarkerListImplTest.cpp/SpellingMarkerListImplTest.cpp/

[rlanday, 2017-05-16 04:47:50]

The CQ bit was checked by rlanday@chromium.org

[rlanday, 2017-05-16 04:47:50]

The patchset sent to the CQ was uploaded after l-g-t-m from yosin@chromium.org
Link to the patchset: https://codereview.chromium.org/2868413002/#ps120001
(title: "Fix mistake in comment")

[commit-bot: I haz the power, 2017-05-16 04:48:17]

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-05-16 06:15:32]

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

[commit-bot: I haz the power, 2017-05-16 06:15:32]

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_...)

[rlanday, 2017-05-16 18:17:17]

The CQ bit was checked by rlanday@chromium.org

[commit-bot: I haz the power, 2017-05-16 18:18:05]

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-05-16 20:36:25]

CQ is committing da patch.

Bot data: {"patchset_id": 120001, "attempt_start_ts": 1494958637781140,
"parent_rev": "0c5ee3cba18800bf7664aaef43afd379e5c9a217", "commit_rev":
"d8b7adbe4b26fedd0a87ffd74aaac141a4694490"}

[commit-bot: I haz the power, 2017-05-16 20:36:37]

Description was changed from

==========
Add DocumentMarkerList::MarkerType()

We need a method on DocumentMarkerList that returns the marker type the list
supports so we know when we can safely cast a DocumentMarkerList* to an impl
type.

This CL adds two subclasses of SpellCheckMarkerListImpl, SpellingMarkerListImpl,
and GrammarMarkerListImpl, so we can implement this method.

Note that adding this method also requires modifying
GenericDocumentMarkerListImpl to take a MarkerType param (we're going to
eliminate GenericDocumentMarkerListImpl once we add TextMatchMarkerListImpl but
we need this for now).

BUG=707867
==========

to

==========
Add DocumentMarkerList::MarkerType()

We need a method on DocumentMarkerList that returns the marker type the list
supports so we know when we can safely cast a DocumentMarkerList* to an impl
type.

This CL adds two subclasses of SpellCheckMarkerListImpl, SpellingMarkerListImpl,
and GrammarMarkerListImpl, so we can implement this method.

Note that adding this method also requires modifying
GenericDocumentMarkerListImpl to take a MarkerType param (we're going to
eliminate GenericDocumentMarkerListImpl once we add TextMatchMarkerListImpl but
we need this for now).

BUG=707867

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

[commit-bot: I haz the power, 2017-05-16 20:36:39]

Committed patchset #7 (id:120001) as
https://chromium.googlesource.com/chromium/src/+/d8b7adbe4b26fedd0a87ffd74aaa...