Add DocumentMarker::clone()

third_party/WebKit/Source/core/editing/markers/DocumentMarker.h

Index: third_party/WebKit/Source/core/editing/markers/DocumentMarker.h
diff --git a/third_party/WebKit/Source/core/editing/markers/DocumentMarker.h b/third_party/WebKit/Source/core/editing/markers/DocumentMarker.h
index dbcfdf2964555be13aa50b7551bf2305f37a6fd8..72840834c21b4c5fbbfb2a55acae527541cec303 100644
--- a/third_party/WebKit/Source/core/editing/markers/DocumentMarker.h
+++ b/third_party/WebKit/Source/core/editing/markers/DocumentMarker.h
@@ -134,6 +134,8 @@ class CORE_EXPORT DocumentMarker : public GarbageCollected<DocumentMarker> {
     SpellCheckClientMarkers() : MarkerTypes(Spelling | Grammar) {}
   };
 
+  virtual DocumentMarker* clone() const;

[yosin_UTC9, 2017/04/04 01:37:51]

I don't understand why we need to have |clone()| member function, and copy ctor.
We don't have any marker having same properties.

The usage of copy-ctor in |DMC::removeMarkers()| is creating
|RenderedDocumentMarker()|.
So, we should change |DMC::removeMarkers()| not to use copy-ctor.

[rlanday, 2017/04/04 02:07:46]

In https://codereview.chromium.org/2780313002 I'm converting DocumentMarker to a
polymorphic class hierarchy. RenderedDocumentMarker is being removed in
https://codereview.chromium.org/2723663002 so it's not super relevant. The clone
method is so we can properly copy a DocumentMarker. The reason we need to copy
markers in removeMarkers() is because if we pass the
DoNotRemovePartiallyOverlappingMarker flag and remove the middle of a marker, we
actually end up with two markers, which necessitates copying the marker.

[rlanday, 2017/04/04 02:07:46]

In https://codereview.chromium.org/2780313002 I'm converting DocumentMarker to a
polymorphic class hierarchy. RenderedDocumentMarker is being removed in
https://codereview.chromium.org/2723663002 so it's not super relevant. The clone
method is so we can properly copy a DocumentMarker. The reason we need to copy
markers in removeMarkers() is because if we pass the
DoNotRemovePartiallyOverlappingMarker flag and remove the middle of a marker, we
actually end up with two markers, which necessitates copying the marker.

[yosin_UTC9, 2017/04/04 04:07:36]

I'm not a fan of clone or copy since we should not have identical DocumentMarker
objects.
How about introducing createMarker(const DocumentMarker& marker, unsigned
startOffset, unsigned endOffset);

+
   DocumentMarker(MarkerType,
                  unsigned startOffset,
                  unsigned endOffset,
@@ -145,8 +147,6 @@ class CORE_EXPORT DocumentMarker : public GarbageCollected<DocumentMarker> {
                  bool thick,
                  Color backgroundColor);
 
-  DocumentMarker(const DocumentMarker&);
-
   MarkerType type() const { return m_type; }
   unsigned startOffset() const { return m_startOffset; }
   unsigned endOffset() const { return m_endOffset; }
@@ -176,6 +176,9 @@ class CORE_EXPORT DocumentMarker : public GarbageCollected<DocumentMarker> {
 
   DECLARE_TRACE();
 
+ protected:
+  DocumentMarker(const DocumentMarker&);

[yosin_UTC9, 2017/04/04 01:37:51]

Could you add TODO to remove copy-ctor from DocumentMarker?
Usage of ctor is questionable and error prone due by subclassing.

Also, we want to make |m_type| to mark |const|.

[rlanday, 2017/04/04 02:07:46]

I need this to implement clone().

+
  private:
   MarkerType m_type;
   unsigned m_startOffset;