[DMC #18] Eliminate DocumentMarkerTextMatch (subclass of DocumentMarkerDetails)

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

Index: third_party/WebKit/Source/core/editing/markers/DocumentMarker.cpp
diff --git a/third_party/WebKit/Source/core/editing/markers/DocumentMarker.cpp b/third_party/WebKit/Source/core/editing/markers/DocumentMarker.cpp
index 1accf9afe0e92d19460b3694e0dc88531de4e071..20103426307e6313d9bd7da5a4feb98b9990ff59 100644
--- a/third_party/WebKit/Source/core/editing/markers/DocumentMarker.cpp
+++ b/third_party/WebKit/Source/core/editing/markers/DocumentMarker.cpp
@@ -30,6 +30,7 @@
 
 #include "core/editing/markers/DocumentMarker.h"
 
+#include "core/editing/markers/TextMatchMarker.h"
 #include "platform/wtf/StdLibExtras.h"
 
 namespace blink {
@@ -62,43 +63,6 @@ inline DocumentMarkerDescription* ToDocumentMarkerDescription(
   return 0;
 }
 
-class DocumentMarkerTextMatch final : public DocumentMarkerDetails {
- public:
-  static DocumentMarkerTextMatch* Create(DocumentMarker::MatchStatus);
-
-  bool IsActiveMatch() const {
-    return match_status_ == DocumentMarker::MatchStatus::kActive;
-  }
-
-  bool IsTextMatch() const override { return true; }
-
- private:
-  explicit DocumentMarkerTextMatch(DocumentMarker::MatchStatus match_status)
-      : match_status_(match_status) {}
-
-  DocumentMarker::MatchStatus match_status_;
-};
-
-DocumentMarkerTextMatch* DocumentMarkerTextMatch::Create(
-    DocumentMarker::MatchStatus match_status) {
-  DEFINE_STATIC_LOCAL(
-      DocumentMarkerTextMatch, active_instance,
-      (new DocumentMarkerTextMatch(DocumentMarker::MatchStatus::kActive)));
-  DEFINE_STATIC_LOCAL(
-      DocumentMarkerTextMatch, inactive_instance,
-      (new DocumentMarkerTextMatch(DocumentMarker::MatchStatus::kInactive)));
-  return match_status == DocumentMarker::MatchStatus::kActive
-             ? &active_instance
-             : &inactive_instance;
-}
-
-inline DocumentMarkerTextMatch* ToDocumentMarkerTextMatch(
-    DocumentMarkerDetails* details) {
-  if (details && details->IsTextMatch())
-    return static_cast<DocumentMarkerTextMatch*>(details);
-  return 0;
-}
-
 class TextCompositionMarkerDetails final : public DocumentMarkerDetails {
  public:
   static TextCompositionMarkerDetails* Create(Color underline_color,
@@ -140,6 +104,14 @@ inline TextCompositionMarkerDetails* ToTextCompositionMarkerDetails(
 
 DocumentMarker::DocumentMarker(MarkerType type,
                                unsigned start_offset,
+                               unsigned end_offset)
+    : type_(type),
+      start_offset_(start_offset),

[yosin_UTC9, 2017/05/29 05:05:33]

Just in case, let's add

DCHECK_LT(start_offset, end_offset)

[rlanday, 2017/05/30 21:28:11]

This causes a test case failure in
InputMethodControllerTest.InsertTextWithNewline():
https://cs.chromium.org/chromium/src/third_party/WebKit/Source/core/editing/I...

Apparently TextIterator can create zero-length ranges for certain nodes. What do
you think we should do (e.g., not add the DCHECK, filter the results from
TextIterator to not create these zero-length ranges)?

[Xiaocheng, 2017/05/30 21:31:36]

TextIterator shouldn't create zero-length ranges.

Let me check what happened since I'm working on TI these days...

[Xiaocheng, 2017/05/30 22:24:37]

DMC should filter results from TextIterator so that marker is created only when
both of the follow are true:
1. The anchor node is a text node; TI can emit text anchored at non-text nodes
(e.g., it emits a "\n" between two <div>s)
2. Start offset < end offset. There are some cases where TI emits non-empty the
same start and end offset. Since there's no spec/documentation for TI, I'm still
investigating what it means when TI emits non-empty text with a collapsed range.
Anyway, this should be an issue of TI, which can be tricky to handle...

In order not to block DMC, I think the best solution is to let DMC filter TI's
output, and add comments of:
1. Why the filtering is needed
2. TODO(editing-dev): investigate if it's a problem of TI

+      end_offset_(end_offset),
+      details_(nullptr) {}

[yosin_UTC9, 2017/05/29 05:05:33]

nit: s/details_(nullptr)//

Since |details_| is |Member<DocumetnMarkerDetaisl>>|, 
we don't need to initialize to |nullptr|.

+
+DocumentMarker::DocumentMarker(MarkerType type,
+                               unsigned start_offset,
                                unsigned end_offset,
                                const String& description)
     : type_(type),
@@ -151,14 +123,6 @@ DocumentMarker::DocumentMarker(MarkerType type,
 
 DocumentMarker::DocumentMarker(unsigned start_offset,
                                unsigned end_offset,
-                               DocumentMarker::MatchStatus match_status)
-    : type_(DocumentMarker::kTextMatch),
-      start_offset_(start_offset),
-      end_offset_(end_offset),
-      details_(DocumentMarkerTextMatch::Create(match_status)) {}
-
-DocumentMarker::DocumentMarker(unsigned start_offset,
-                               unsigned end_offset,
                                Color underline_color,
                                bool thick,
                                Color background_color)
@@ -219,11 +183,10 @@ void DocumentMarker::ShiftOffsets(int delta) {
   start_offset_ += delta;
   end_offset_ += delta;
 }
-
 void DocumentMarker::SetIsActiveMatch(bool active) {
-  details_ = DocumentMarkerTextMatch::Create(
-      active ? DocumentMarker::MatchStatus::kActive
-             : DocumentMarker::MatchStatus::kInactive);
+  if (GetType() != DocumentMarker::kTextMatch)
+    return;
+  ToTextMatchMarker(this)->SetIsActiveMatch(active);
 }
 
 const String& DocumentMarker::Description() const {
@@ -234,10 +197,9 @@ const String& DocumentMarker::Description() const {
 }
 
 bool DocumentMarker::IsActiveMatch() const {
-  if (DocumentMarkerTextMatch* details =
-          ToDocumentMarkerTextMatch(details_.Get()))
-    return details->IsActiveMatch();
-  return false;
+  if (GetType() != DocumentMarker::kTextMatch)
+    return false;
+  return ToTextMatchMarker(this)->IsActiveMatch();
 }
 
 Color DocumentMarker::UnderlineColor() const {