Add DocumentMarker::clone()

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

 /*
  * This file is part of the DOM implementation for WebCore.
  *
  * Copyright (C) 2006 Apple Computer, Inc.
  *
  * This library is free software; you can redistribute it and/or
  * modify it under the terms of the GNU Library General Public
  * License as published by the Free Software Foundation; either
  * version 2 of the License, or (at your option) any later version.
  *
@@ skipping 116 matching lines @@
   class MisspellingMarkers : public MarkerTypes {
    public:
     MisspellingMarkers() : MarkerTypes(Spelling | Grammar) {}
   };
 
   class SpellCheckClientMarkers : public MarkerTypes {
    public:
     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,
                  const String& description);
   DocumentMarker(unsigned startOffset, unsigned endOffset, bool activeMatch);
   DocumentMarker(unsigned startOffset,
                  unsigned endOffset,
                  Color underlineColor,
                  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; }
 
   const String& description() const;
   bool activeMatch() const;
   Color underlineColor() const;
   bool thick() const;
   Color backgroundColor() const;
   DocumentMarkerDetails* details() const;
 
   void setActiveMatch(bool);
   void clearDetails() { m_details.clear(); }
 
   // Offset modifications are done by DocumentMarkerController.
   // Other classes should not call following setters.
   void setStartOffset(unsigned offset) { m_startOffset = offset; }
   void setEndOffset(unsigned offset) { m_endOffset = offset; }
   void shiftOffsets(int delta);
 
   bool operator==(const DocumentMarker& o) const {
     return type() == o.type() && startOffset() == o.startOffset() &&
            endOffset() == o.endOffset();
   }
 
   bool operator!=(const DocumentMarker& o) const { return !(*this == o); }
 
   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;
   unsigned m_endOffset;
   Member<DocumentMarkerDetails> m_details;
 };
 
 using DocumentMarkerVector = HeapVector<Member<DocumentMarker>>;
 
 inline DocumentMarkerDetails* DocumentMarker::details() const {
   return m_details.get();
 }
 
 class DocumentMarkerDetails
     : public GarbageCollectedFinalized<DocumentMarkerDetails> {
  public:
   DocumentMarkerDetails() {}
   virtual ~DocumentMarkerDetails();
   virtual bool isDescription() const { return false; }
   virtual bool isTextMatch() const { return false; }
   virtual bool isComposition() const { return false; }
 
   DEFINE_INLINE_VIRTUAL_TRACE() {}
 };
 
 }  // namespace blink
 
 #endif  // DocumentMarker_h