[DMC #24] Add SpellingMarker and GrammarMarker (subclasses of DocumentMarker)

Add SpellingMarker and GrammarMarker (subclasses of DocumentMarker)

In https://codereview.chromium.org/2904093002, I'm eliminating the
DocumentMarkerDetails subclass for TextMatchMarker in favor of storing the
TextMatchMarker-specific data directly on TextMatchMarker. In
https://codereview.chromium.org/2908643002, I'm creaing a similar subclass for
CompositionMarker. In this CL, I'm doing the same for SpellingMarker and
GrammarMarker, which means we can now eliminate DocumentMarkerDetails.

In a follow-up CL, I will make MarkerType() virtual so we won't have to store
the MarkerType as a field in DocumentMarker.

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

[rlanday, 2017-05-26 20:13:21]

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

[rlanday, 2017-05-26 20:13:46]

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

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

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-26 20:17:07]

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

[commit-bot: I haz the power, 2017-05-26 20:17:08]

Dry run: Try jobs failed on following builders:
  ios-device on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...)
  ios-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...)
  mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[rlanday, 2017-05-26 21:45:08]

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

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

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-26 22:47:44]

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

[commit-bot: I haz the power, 2017-05-26 22:47:45]

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

[rlanday, 2017-05-27 00:49:11]

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

[commit-bot: I haz the power, 2017-05-27 00:49:19]

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-27 00:58:50]

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

[commit-bot: I haz the power, 2017-05-27 00:58:51]

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

[yosin_UTC9, 2017-05-29 05:13:48]

https://codereview.chromium.org/2911723002/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/editing/markers/DocumentMarker.cpp (right):

https://codereview.chromium.org/2911723002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/editing/markers/DocumentMarker.cpp:37:
DocumentMarker::~DocumentMarker() {}
nit: s/{}/ = default;/

https://codereview.chromium.org/2911723002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/editing/markers/DocumentMarker.cpp:42: :
type_(type), start_offset_(start_offset), end_offset_(end_offset) {}
Let's add
DCHECK_LT(start_offset_, end_offset_)

https://codereview.chromium.org/2911723002/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/editing/markers/GrammarMarker.cpp (right):

https://codereview.chromium.org/2911723002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/editing/markers/GrammarMarker.cpp:15:
description) {}
Let's add DCHECK_LT(start_offset, end_offset)

https://codereview.chromium.org/2911723002/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/editing/markers/SpellCheckMarker.cpp
(right):

https://codereview.chromium.org/2911723002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/editing/markers/SpellCheckMarker.cpp:14:
description_(description) {}
Let's add DCHECK_LT(start_offset_, end_offset_)

https://codereview.chromium.org/2911723002/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/editing/markers/SpellCheckMarker.h (right):

https://codereview.chromium.org/2911723002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/editing/markers/SpellCheckMarker.h:22: String
description_;
nit: s/String/const String/

https://codereview.chromium.org/2911723002/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/editing/markers/SpellingMarker.cpp (right):

https://codereview.chromium.org/2911723002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/editing/markers/SpellingMarker.cpp:15:
description) {}
Let's add DCHECK_LT(start_offset_, end_offset_)

[rlanday, 2017-05-31 20:58:40]

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

[rlanday, 2017-05-31 20:59:09]

Updated

[commit-bot: I haz the power, 2017-05-31 20:59:58]

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-31 21:35:39]

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

[commit-bot: I haz the power, 2017-05-31 21:35:39]

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

[Xiaocheng, 2017-05-31 22:17:16]

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

https://codereview.chromium.org/2911723002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/editing/markers/DocumentMarker.cpp:43:
DCHECK_LT(start_offset_, end_offset_);
Redundant change.

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

https://codereview.chromium.org/2911723002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/editing/markers/SpellCheckMarker.h:14:
SpellCheckMarker(DocumentMarker::MarkerType,
The ctor should be protected.

[yosin_UTC9, 2017-06-01 03:29:12]

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

https://codereview.chromium.org/2911723002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/editing/markers/GrammarMarker.h:12: class
CORE_EXPORT GrammarMarker final : public SpellCheckMarker {
Please add a class comment.

https://codereview.chromium.org/2911723002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/editing/markers/GrammarMarker.h:17: };
nit: Could you add DISALLOW_COPY_AND_ASSING()?

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

https://codereview.chromium.org/2911723002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/editing/markers/SpellCheckMarker.h:12: class
CORE_EXPORT SpellCheckMarker : public DocumentMarker {
Please add a class comment.

https://codereview.chromium.org/2911723002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/editing/markers/SpellCheckMarker.h:19: const
String& Description() const;
nit: Since this simple getter, you can implement here.

https://codereview.chromium.org/2911723002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/editing/markers/SpellCheckMarker.h:23: };
nit: Could you add DISALLOW_COPY_AND_ASSING()?

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

https://codereview.chromium.org/2911723002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/editing/markers/SpellingMarker.h:12: class
CORE_EXPORT SpellingMarker final : public SpellCheckMarker {
Please add a class comment.

https://codereview.chromium.org/2911723002/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/testing/Internals.cpp (right):

https://codereview.chromium.org/2911723002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/testing/Internals.cpp:1014: if (!marker ||
(marker->GetType() != DocumentMarker::kSpelling &&
How about introduce global function IsSpellingMarker(const Marker&)?
Since, this conditional expression is used three times, DEFINE_TYPE_CAST() and
here,
it is over threshold.

[rlanday, 2017-06-01 05:00:33]

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

[rlanday, 2017-06-01 05:00:42]

Updated

[commit-bot: I haz the power, 2017-06-01 05:00:50]

Dry run: CQ is trying da patch.

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

[yosin_UTC9, 2017-06-01 05:45:39]

yosin@chromium.org changed reviewers:
+ tkent@chromium.org

[yosin_UTC9, 2017-06-01 05:45:40]

lgtm

+tkent@ for web/ContextMenuClientImpl.cpp

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

https://codereview.chromium.org/2911723002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/editing/markers/GrammarMarkerTest.cpp:15:
TEST_F(GrammarMarkerTest, MarkerType) {
Could you add a test case for IsSpellCheckerMarker() for GrammarMarker?

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

https://codereview.chromium.org/2911723002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/editing/markers/SpellingMarkerTest.cpp:15:
TEST_F(SpellingMarkerTest, MarkerType) {
Could you add a test case for IsSpellCheckerMarker() for SpellingMarker?

[tkent, 2017-06-01 05:50:24]

> +tkent@ for web/ContextMenuClientImpl.cpp

lgtm

[commit-bot: I haz the power, 2017-06-01 06:18:49]

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

[commit-bot: I haz the power, 2017-06-01 06:18:50]

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

[rlanday, 2017-06-01 18:11:14]

The CQ bit was checked by rlanday@chromium.org

[rlanday, 2017-06-01 18:11:14]

The patchset sent to the CQ was uploaded after l-g-t-m from tkent@chromium.org,
yosin@chromium.org
Link to the patchset: https://codereview.chromium.org/2911723002/#ps100001
(title: "Add IsSpellCheckMarker() tests")

[commit-bot: I haz the power, 2017-06-01 18:11: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-06-01 20:33:01]

CQ is committing da patch.

Bot data: {"patchset_id": 100001, "attempt_start_ts": 1496340674353300,
"parent_rev": "65dcbb2ab5805377de065aaa1dbc87215df460f7", "commit_rev":
"45ab0de82123f81a41e7af931ba30e180532066f"}

[commit-bot: I haz the power, 2017-06-01 20:33:09]

Description was changed from

==========
Add SpellingMarker and GrammarMarker (subclasses of DocumentMarker)

In https://codereview.chromium.org/2904093002, I'm eliminating the
DocumentMarkerDetails subclass for TextMatchMarker in favor of storing the
TextMatchMarker-specific data directly on TextMatchMarker. In
https://codereview.chromium.org/2908643002, I'm creaing a similar subclass for
CompositionMarker. In this CL, I'm doing the same for SpellingMarker and
GrammarMarker, which means we can now eliminate DocumentMarkerDetails.

In a follow-up CL, I will make MarkerType() virtual so we won't have to store
the MarkerType as a field in DocumentMarker.

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

to

==========
Add SpellingMarker and GrammarMarker (subclasses of DocumentMarker)

In https://codereview.chromium.org/2904093002, I'm eliminating the
DocumentMarkerDetails subclass for TextMatchMarker in favor of storing the
TextMatchMarker-specific data directly on TextMatchMarker. In
https://codereview.chromium.org/2908643002, I'm creaing a similar subclass for
CompositionMarker. In this CL, I'm doing the same for SpellingMarker and
GrammarMarker, which means we can now eliminate DocumentMarkerDetails.

In a follow-up CL, I will make MarkerType() virtual so we won't have to store
the MarkerType as a field in DocumentMarker.

BUG=707867

Review-Url: https://codereview.chromium.org/2911723002
Cr-Commit-Position: refs/heads/master@{#476419}
Committed:
https://chromium.googlesource.com/chromium/src/+/45ab0de82123f81a41e7af931ba3...
==========

[commit-bot: I haz the power, 2017-06-01 20:33:10]

Committed patchset #6 (id:100001) as
https://chromium.googlesource.com/chromium/src/+/45ab0de82123f81a41e7af931ba3...