[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),
+      end_offset_(end_offset),
+      details_(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 46 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)

[rlanday, 2017/05/25 21:00:48]

Note the behavioral change here: calling SetIsActiveMatch() on a
non-TextMatchMarker currently overwrites the DocumentMarkerDetails pointer with
one that holds TextMatchMarker-specific data. I'm changing it to be a no-op in
this case. In a follow-up CL, I'll force callers to cast to TextMatchMarker to
call this method, avoiding the issue entirely.

[Xiaocheng, 2017/05/26 00:46:25]

We should be fine here, since DM::SetIsActiveMatch is called only by
DMC::SetTextMatchMarkersActive.

There shouldn't be real behavioral change.

Anyway, cleaning it up is good.

+    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