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

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

 /*
  * Copyright (C) 2011 Google Inc. All rights reserved.
  *
  * Redistribution and use in source and binary forms, with or without
  * modification, are permitted provided that the following conditions are
  * met:
  *
  *     * Redistributions of source code must retain the above copyright
  * notice, this list of conditions and the following disclaimer.
  *     * Redistributions in binary form must reproduce the above
@@ skipping 12 matching lines @@
  * SPECIAL, EXEMPLARY, OR CONSEQUENTIAL DAMAGES (INCLUDING, BUT NOT
  * LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES; LOSS OF USE,
  * DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER CAUSED AND ON ANY
  * THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, OR TORT
  * (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE
  * OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
  */
 
 #include "core/editing/markers/DocumentMarker.h"
 
+#include "core/editing/markers/TextMatchMarker.h"
 #include "platform/wtf/StdLibExtras.h"
 
 namespace blink {
 
 DocumentMarkerDetails::~DocumentMarkerDetails() {}
 
 class DocumentMarkerDescription final : public DocumentMarkerDetails {
  public:
   static DocumentMarkerDescription* Create(const String&);
 
@@ skipping 12 matching lines @@
   return new DocumentMarkerDescription(description);
 }
 
 inline DocumentMarkerDescription* ToDocumentMarkerDescription(
     DocumentMarkerDetails* details) {
   if (details && details->IsDescription())
     return static_cast<DocumentMarkerDescription*>(details);
   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,
                                               bool thick,
                                               Color background_color);
 
   bool IsComposition() const override { return true; }
   Color UnderlineColor() const { return underline_color_; }
   bool Thick() const { return thick_; }
   Color BackgroundColor() const { return background_color_; }
@@ skipping 21 matching lines @@
 
 inline TextCompositionMarkerDetails* ToTextCompositionMarkerDetails(
     DocumentMarkerDetails* details) {
   if (details && details->IsComposition())
     return static_cast<TextCompositionMarkerDetails*>(details);
   return nullptr;
 }
 
 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),
       start_offset_(start_offset),
       end_offset_(end_offset),
       details_(description.IsEmpty()
                    ? nullptr
                    : DocumentMarkerDescription::Create(description)) {}
 
 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)
     : type_(DocumentMarker::kComposition),
       start_offset_(start_offset),
       end_offset_(end_offset),
       details_(TextCompositionMarkerDetails::Create(underline_color,
                                                     thick,
                                                     background_color)) {}
 
@@ skipping 40 matching lines @@
   if (result.start_offset >= result.end_offset)
     return WTF::nullopt;
 
   return result;
 }
 
 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 {
   if (DocumentMarkerDescription* details =
           ToDocumentMarkerDescription(details_.Get()))
     return details->Description();
   return g_empty_string;
 }
 
 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 {
   if (TextCompositionMarkerDetails* details =
           ToTextCompositionMarkerDetails(details_.Get()))
     return details->UnderlineColor();
   return Color::kTransparent;
 }
 
 bool DocumentMarker::Thick() const {
   if (TextCompositionMarkerDetails* details =
           ToTextCompositionMarkerDetails(details_.Get()))
     return details->Thick();
   return false;
 }
 
 Color DocumentMarker::BackgroundColor() const {
   if (TextCompositionMarkerDetails* details =
           ToTextCompositionMarkerDetails(details_.Get()))
     return details->BackgroundColor();
   return Color::kTransparent;
 }
 
 DEFINE_TRACE(DocumentMarker) {
   visitor->Trace(details_);
 }
 
 }  // namespace blink