[DMC #5.6] Add type cast for SpellCheckMarkerListImpl

Add type cast for SpellCheckMarkerListImpl

This will be used to cast a DocumentMarkerList* to SpellCheckMarkerListImpl* in
DocumentMarkerController once RemoveMarkersUnderWords() is a method defined on
SpellCheckMarkerListImpl instead of DocumentMarkerList.

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

[rlanday, 2017-05-08 21:20:24]

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

[rlanday, 2017-05-08 21:21:39]

Note currently there's no method on SpellCheckMarkerListImpl for the cast to
DCHECK
that it's a safe conversion. Should I add a method (e.g.
IsSpellCheckMarkerListImpl()
on DocumentMarkerList) so we can verify this, or is an unchecked cast fine here?

[yosin_UTC9, 2017-05-10 07:43:11]

On 2017/05/08 at 21:21:39, rlanday wrote:
> Note currently there's no method on SpellCheckMarkerListImpl for the cast to
DCHECK
> that it's a safe conversion. Should I add a method (e.g.
IsSpellCheckMarkerListImpl()
> on DocumentMarkerList) so we can verify this, or is an unchecked cast fine
here?

We need to have a test for this change. 
EXPECT_TRUE(IsSpellingMarkerList(new SpellCheckMarkerListImpl());
EXPECT_FALSE(IsSpellingMarkerList(new CompositionMarkerListImpl());


So, we should have some methods in DocumentMarkerList interface.

Options:
1. bool IsSpellCheckerMarkerList()
  PROS: What we need at this time.
  CONS: Each ListImpl has { return false; }
  CONS: When we need to have another downcast, we need to introduce another
function in DMList and having { return false; } for them on each impl
2. DocumentMarkerType MarkerType()
  PROS: Seems each ListImpl return meaningful value.
  PROS: We can have DCHECK_EQ(MarkrType(), marker->MakerType()) for ListImpl's
functions.
  PROS: When we need to have another downcast, just add DEFINE_TYPE_CAST() for
that ListImpl.
  CONS: We need to have both SpellingListImpl and GrammarListImpl, or introduce
SpellCheckingMarkerListImpl::marker_type_

It seems option 2 with SpellingListImpl and GrammarListImpl, is choice.
WDYT?

[rlanday, 2017-05-10 17:05:45]

I agree that option 1 gets kind of messy as we have to keep adding different
methods (although I think we can clean it up slightly by having a default
implementation of each method on DocumentMarkerList that returns false).

For option 2, if we split up the list impl into two versions, we have to deal
with the duplication of logic somehow. Even if we have all the actual method
implementations somewhere else and we're just calling them, I think it's messy
just to have all the calls duplicated in two files. Can we introduce
SpellingListImpl and GrammarListImpl as subclasses of SpellCheckMarkerListImpl
and deal with the duplication that way? Or do you not want to have any
subclassing?

[rlanday, 2017-05-10 17:23:27]

Note that we can't actually have a ToSpellCheckMarkerListImpl() cast that works
for both SpellingListImpl and GrammarListImpl unless we have a
SpellCheckMarkerListImpl :) So I think we probably want to subclass?

[Xiaocheng, 2017-05-10 18:30:04]

I prefer 2 with SpellCheckingMarkerListImpl::marker_type_, which leads to the
least amount of work

[rlanday, 2017-05-10 19:07:47]

On 2017/05/10 at 18:30:04, xiaochengh wrote:
> I prefer 2 with SpellCheckingMarkerListImpl::marker_type_, which leads to the
least amount of work

This is what adding SpellingMarkerListImpl and GrammarMarkerListImpl looks like:
https://codereview.chromium.org/2868413002

This is what adding SpellCheckingMarkerListImpl::marker_type_ looks like:
https://codereview.chromium.org/2872353002

[rlanday, 2017-05-12 20:26:32]

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

[commit-bot: I haz the power, 2017-05-12 20:27:20]

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-12 22:54:07]

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

[commit-bot: I haz the power, 2017-05-12 22:54:08]

Dry run: 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_...)

[yosin_UTC9, 2017-05-15 02:17:28]

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

https://codereview.chromium.org/2873483002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/editing/markers/CompositionMarkerListImpl.h:23:
DocumentMarker::MarkerType MarkerType() const final;
Could you introduce |DocumentMarkerList::MarkerType()| in different patch?

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

https://codereview.chromium.org/2873483002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/editing/markers/SpellCheckMarkerListImpl.h:49:
true,
MarkerType() == kSpellingMarker and MarkerType() == kGrammarMarker?

[rlanday, 2017-05-15 02:27:11]

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

https://codereview.chromium.org/2873483002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/editing/markers/SpellCheckMarkerListImpl.h:49:
true,
On 2017/05/15 at 02:17:28, yosin_UTC9 wrote:
> MarkerType() == kSpellingMarker and MarkerType() == kGrammarMarker?

Oh wow, I think I was missing the forest for the trees a bit here :)

[rlanday, 2017-05-15 03:00:41]

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

[commit-bot: I haz the power, 2017-05-15 03:00:53]

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 03:02:20]

Ok, I'm sorry, I was accidentally including the changes from
https://codereview.chromium.org/2868413002 before and had forgotten to actually
add the MarkerType() check we decided we wanted; the CL should be fixed now.

[yosin_UTC9, 2017-05-15 03:54:55]

lgtm

[commit-bot: I haz the power, 2017-05-15 05:03:19]

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

[commit-bot: I haz the power, 2017-05-15 05:03:21]

Dry run: This issue passed the CQ dry run.

[rlanday, 2017-05-16 20:55:13]

The CQ bit was checked by rlanday@chromium.org

[rlanday, 2017-05-16 20:55:13]

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/2873483002/#ps60001
(title: "Rebase")

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

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 22:49:35]

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

[commit-bot: I haz the power, 2017-05-16 22:49:35]

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

[rlanday, 2017-05-17 00:01:34]

The CQ bit was checked by rlanday@chromium.org

[commit-bot: I haz the power, 2017-05-17 00:03:53]

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-17 04:27:21]

CQ is committing da patch.

Bot data: {"patchset_id": 60001, "attempt_start_ts": 1494979294072540,
"parent_rev": "59bb4d543e23a248553affa3dc9de1e01a1ce228", "commit_rev":
"afa32648e767d5288c0ea85ca18526ac8810fe0d"}

[commit-bot: I haz the power, 2017-05-17 04:27:35]

Description was changed from

==========
Add type cast for SpellCheckMarkerListImpl

This will be used to cast a DocumentMarkerList* to SpellCheckMarkerListImpl* in
DocumentMarkerController once RemoveMarkersUnderWords() is a method defined on
SpellCheckMarkerListImpl instead of DocumentMarkerList.

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

to

==========
Add type cast for SpellCheckMarkerListImpl

This will be used to cast a DocumentMarkerList* to SpellCheckMarkerListImpl* in
DocumentMarkerController once RemoveMarkersUnderWords() is a method defined on
SpellCheckMarkerListImpl instead of DocumentMarkerList.

BUG=707867

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

[commit-bot: I haz the power, 2017-05-17 04:27:36]

Committed patchset #4 (id:60001) as
https://chromium.googlesource.com/chromium/src/+/afa32648e767d5288c0ea85ca185...