[WIP] Refactor DocumentMarker

Refactor DocumentMarker

Eliminate DocumentMarkerDetails in favor of making DocumentMarker a polymorphic
class hierarchy (since we're already subclassing it for TextMarker anyway)

Depends on
https://codereview.chromium.org/2763893002/
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2

BUG=707867

[rlanday, 2017-03-29 22:46:24]

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

[rlanday, 2017-03-29 22:47:09]

Description was changed from

==========
Refactor DocumentMarker

Eliminate DocumentMarkerDetails in favor of making DocumentMarker a polymorphic
class hierarchy (since we're already subclassing it for TextMarker anyway)
==========

to

==========
Refactor DocumentMarker

Eliminate DocumentMarkerDetails in favor of making DocumentMarker a polymorphic
class hierarchy (since we're already subclassing it for TextMarker anyway)

Depends on
https://codereview.chromium.org/2763893002/
==========

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

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 23:13:29]

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

[rlanday, 2017-03-29 23:13:39]

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

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

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 23:19:33]

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

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

Dry run: Try jobs failed on following builders:
  chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
  linux_chromium_tsan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  ios-device-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-xcode-...)
  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_...)

[Xiaocheng, 2017-03-29 23:40:11]

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

https://codereview.chromium.org/2780313002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/editing/markers/DocumentMarker.h:144: virtual
const String& description() const;
These functions only make sense in subclasses. Can we move them down there?

More down-castings are expected after this change, but I think it's worthy.

https://codereview.chromium.org/2780313002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/editing/markers/DocumentMarker.h:166: bool
operator==(const DocumentMarker& other) const;
These comparison methods seem not used at all.

Could you remove them with another patch?

https://codereview.chromium.org/2780313002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/editing/markers/DocumentMarkerController.cpp
(left):

https://codereview.chromium.org/2780313002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/editing/markers/DocumentMarkerController.cpp:67:
DCHECK(type != DocumentMarker::TextMatch);
nit: no need to remove this check.

[rlanday, 2017-03-30 21:00:25]

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

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

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 21:10:07]

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

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

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

[rlanday, 2017-03-31 01:34:53]

https://codereview.chromium.org/2780313002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/editing/markers/DocumentMarkerController.cpp
(left):

https://codereview.chromium.org/2780313002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/editing/markers/DocumentMarkerController.cpp:67:
DCHECK(type != DocumentMarker::TextMatch);
On 2017/03/29 at 23:40:11, Xiaocheng wrote:
> nit: no need to remove this check.

SpellCheckMarker already DCHECKs that the type being passed in is either
Spelling or Grammar, so I think it's redundant

[rlanday, 2017-03-31 01:37:49]

Description was changed from

==========
Refactor DocumentMarker

Eliminate DocumentMarkerDetails in favor of making DocumentMarker a polymorphic
class hierarchy (since we're already subclassing it for TextMarker anyway)

Depends on
https://codereview.chromium.org/2763893002/
==========

to

==========
Refactor DocumentMarker

Eliminate DocumentMarkerDetails in favor of making DocumentMarker a polymorphic
class hierarchy (since we're already subclassing it for TextMarker anyway)

Depends on
https://codereview.chromium.org/2763893002/
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2
==========

[Xiaocheng, 2017-03-31 02:01:01]

Almost good.

https://codereview.chromium.org/2780313002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/editing/markers/DocumentMarkerController.cpp
(left):

https://codereview.chromium.org/2780313002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/editing/markers/DocumentMarkerController.cpp:67:
DCHECK(type != DocumentMarker::TextMatch);
On 2017/03/31 at 01:34:53, rlanday wrote:
> On 2017/03/29 at 23:40:11, Xiaocheng wrote:
> > nit: no need to remove this check.
> 
> SpellCheckMarker already DCHECKs that the type being passed in is either
Spelling or Grammar, so I think it's redundant

Makes sense.

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

https://codereview.chromium.org/2780313002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/editing/markers/CompositionMarkerList.h:19:
CompositionMarker* at(size_t index) final;
virtual |at()| should be in some previous patch.

https://codereview.chromium.org/2780313002/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/editing/markers/DocumentMarker.h (left):

https://codereview.chromium.org/2780313002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/editing/markers/DocumentMarker.h:154: const
String& description() const;
Hmm... I didn't expect these accessors to be used so widely.

So once again, please split :)

1. This patch keeps these accessors, and leaves a TODO to move then down to
subclasses
2. A dependent patch moves them down, and modify the call sites

[rlanday, 2017-03-31 17:40:41]

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

https://codereview.chromium.org/2780313002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/editing/markers/CompositionMarkerList.h:19:
CompositionMarker* at(size_t index) final;
On 2017/03/31 at 02:01:01, Xiaocheng wrote:
> virtual |at()| should be in some previous patch.

I don't think this makes sense, there is no CompositionMarker class until this
patch, so this method would be exactly the same as its parent...

[rlanday, 2017-03-31 18:03:12]

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

[rlanday, 2017-03-31 18:03:34]

On 2017/03/31 at 02:01:01, xiaochengh wrote:
> Hmm... I didn't expect these accessors to be used so widely.
> 
> So once again, please split :)
> 
> 1. This patch keeps these accessors, and leaves a TODO to move then down to
subclasses
> 2. A dependent patch moves them down, and modify the call sites

Ok, I've removed the accessor changes from this CL, I will upload a second CL
with those changes

[commit-bot: I haz the power, 2017-03-31 18:04:08]

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-31 18:18:23]

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

[commit-bot: I haz the power, 2017-03-31 18:19:13]

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-31 18:22:51]

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

[commit-bot: I haz the power, 2017-03-31 18:22:52]

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-device-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-xcode-...)
  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_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[Xiaocheng, 2017-03-31 19:59:18]

lgtm

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

https://codereview.chromium.org/2780313002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/editing/markers/CompositionMarkerList.h:19:
CompositionMarker* at(size_t index) final;
On 2017/03/31 at 17:40:41, rlanday wrote:
> On 2017/03/31 at 02:01:01, Xiaocheng wrote:
> > virtual |at()| should be in some previous patch.
> 
> I don't think this makes sense, there is no CompositionMarker class until this
patch, so this method would be exactly the same as its parent...

Yeah, you're right!

[rlanday, 2017-04-03 17:51:12]

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

[commit-bot: I haz the power, 2017-04-03 17:52:09]

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-04-03 17:58:48]

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

[commit-bot: I haz the power, 2017-04-03 17:58:49]

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...)
  android_cronet on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_cron...)
  cast_shell_android on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/cast_shell_a...)
  linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)

[rlanday, 2017-04-03 18:55:22]

Description was changed from

==========
Refactor DocumentMarker

Eliminate DocumentMarkerDetails in favor of making DocumentMarker a polymorphic
class hierarchy (since we're already subclassing it for TextMarker anyway)

Depends on
https://codereview.chromium.org/2763893002/
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2
==========

to

==========
Refactor DocumentMarker

Eliminate DocumentMarkerDetails in favor of making DocumentMarker a polymorphic
class hierarchy (since we're already subclassing it for TextMarker anyway)

Depends on
https://codereview.chromium.org/2763893002/
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2

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