[LayoutNG] Improvements in using DrawingRecorder in PaintTextChild

This patch improves the logic in PaintTextChild so that:
1. Move DrawingRecorder code after the check of
   ShouldPaintTextFragment and have_selection. This matches
   to InlineTextBoxPainter logic order, avoiding
   instantiation of DrawingRecorder if nothing are painted.
2. Check LayoutObject::IsSelected before
   ComputeLayoutSelectionStatus. This matches to legacy
   InlineTextBoxPainter, avoiding a few unnecessary function
   calls when there is no selection on the LayoutObject.

These are not the primary goal of issue 936024, but found
during the review and has positive impacts on blink_perf:
https://pinpoint-dot-chromeperf.appspot.com/job/1184c765340000

Bug: 936024
Change-Id: Idda53c9f41476d6024c6f5855594b3bd7dd35418
Reviewed-on: https://chromium-review.googlesource.com/c/1487432Reviewed-by: Xianzhu Wang <wangxianzhu@chromium.org>
Commit-Queue: Koji Ishii <kojii@chromium.org>
Cr-Commit-Position: refs/heads/master@{#635626}

third_party/blink/renderer/core/paint/ng/ng_box_fragment_painter.cc

@@ -11,7 +11,6 @@
 #include "third_party/blink/renderer/core/layout/hit_test_location.h"
 #include "third_party/blink/renderer/core/layout/hit_test_result.h"
 #include "third_party/blink/renderer/core/layout/layout_inline.h"
-#include "third_party/blink/renderer/core/layout/layout_list_marker.h"
 #include "third_party/blink/renderer/core/layout/layout_table.h"
 #include "third_party/blink/renderer/core/layout/layout_table_cell.h"
 #include "third_party/blink/renderer/core/layout/ng/geometry/ng_border_edges.h"
@@ -24,7 +23,6 @@
 #include "third_party/blink/renderer/core/paint/background_image_geometry.h"
 #include "third_party/blink/renderer/core/paint/box_decoration_data.h"
 #include "third_party/blink/renderer/core/paint/compositing/composited_layer_mapping.h"
-#include "third_party/blink/renderer/core/paint/list_marker_painter.h"
 #include "third_party/blink/renderer/core/paint/ng/ng_fieldset_painter.h"
 #include "third_party/blink/renderer/core/paint/ng/ng_fragment_painter.h"
 #include "third_party/blink/renderer/core/paint/ng/ng_inline_box_fragment_painter.h"
@@ -808,43 +806,8 @@ void NGBoxFragmentPainter::PaintTextChild(const NGPaintFragment& text_fragment,
       return;
   }
 
-  // The text clip phase already has a DrawingRecorder. Text clips are initiated
-  // only in BoxPainterBase::PaintFillLayer, which is already within a
-  // DrawingRecorder.
-  base::Optional<DrawingRecorder> recorder;
-  if (paint_info.phase != PaintPhase::kTextClip) {
-    if (DrawingRecorder::UseCachedDrawingIfPossible(
-            paint_info.context, text_fragment, paint_info.phase))
-      return;
-    recorder.emplace(paint_info.context, text_fragment, paint_info.phase);
-  }
-
-  const NGPhysicalTextFragment& physical_text_fragment =
-      ToNGPhysicalTextFragment(text_fragment.PhysicalFragment());
-  if (physical_text_fragment.TextType() ==
-      NGPhysicalTextFragment::kSymbolMarker) {
-    // The NGInlineItem of marker might be Split(). So PaintSymbol only if the
-    // StartOffset is 0, or it might be painted several times.
-    if (!physical_text_fragment.StartOffset())
-      PaintSymbol(text_fragment, paint_info, paint_offset);
-  } else {
-    NGTextFragmentPainter text_painter(text_fragment);
-    text_painter.Paint(paint_info, paint_offset);
-  }
-}
-
-void NGBoxFragmentPainter::PaintSymbol(const NGPaintFragment& fragment,
-                                       const PaintInfo& paint_info,
-                                       const LayoutPoint& paint_offset) {
-  const ComputedStyle& style = fragment.Style();
-  LayoutRect marker_rect =
-      LayoutListMarker::RelativeSymbolMarkerRect(style, fragment.Size().width);
-  marker_rect.MoveBy(fragment.Offset().ToLayoutPoint());
-  marker_rect.MoveBy(paint_offset);
-  IntRect rect = PixelSnappedIntRect(marker_rect);
-
-  ListMarkerPainter::PaintSymbol(paint_info, fragment.GetLayoutObject(), style,
-                                 rect);
+  NGTextFragmentPainter text_painter(text_fragment);
+  text_painter.Paint(paint_info, paint_offset);
 }
 
 void NGBoxFragmentPainter::PaintAtomicInline(const PaintInfo& paint_info) {

third_party/blink/renderer/core/paint/ng/ng_box_fragment_painter.h

@@ -106,9 +106,6 @@ class NGBoxFragmentPainter : public BoxPainterBase {
                        const LayoutRect&,
                        const Color& background_color,
                        BackgroundBleedAvoidance = kBackgroundBleedNone);
-  void PaintSymbol(const NGPaintFragment&,
-                   const PaintInfo&,
-                   const LayoutPoint& paint_offset);
   void PaintCarets(const PaintInfo&, const LayoutPoint& paint_offset);
 
   void RecordHitTestData(const PaintInfo& paint_info,

third_party/blink/renderer/core/paint/ng/ng_text_fragment_painter.cc

@@ -10,6 +10,7 @@
 #include "third_party/blink/renderer/core/editing/markers/document_marker_controller.h"
 #include "third_party/blink/renderer/core/editing/markers/text_match_marker.h"
 #include "third_party/blink/renderer/core/frame/local_frame.h"
+#include "third_party/blink/renderer/core/layout/layout_list_marker.h"
 #include "third_party/blink/renderer/core/layout/ng/geometry/ng_logical_rect.h"
 #include "third_party/blink/renderer/core/layout/ng/inline/ng_offset_mapping.h"
 #include "third_party/blink/renderer/core/layout/ng/inline/ng_physical_text_fragment.h"
@@ -18,6 +19,7 @@
 #include "third_party/blink/renderer/core/layout/ng/ng_text_decoration_offset.h"
 #include "third_party/blink/renderer/core/paint/document_marker_painter.h"
 #include "third_party/blink/renderer/core/paint/inline_text_box_painter.h"
+#include "third_party/blink/renderer/core/paint/list_marker_painter.h"
 #include "third_party/blink/renderer/core/paint/ng/ng_paint_fragment.h"
 #include "third_party/blink/renderer/core/paint/ng/ng_text_painter.h"
 #include "third_party/blink/renderer/core/paint/paint_info.h"
@@ -27,6 +29,7 @@
 #include "third_party/blink/renderer/core/style/computed_style.h"
 #include "third_party/blink/renderer/platform/fonts/character_range.h"
 #include "third_party/blink/renderer/platform/graphics/graphics_context_state_saver.h"
+#include "third_party/blink/renderer/platform/graphics/paint/drawing_recorder.h"
 
 namespace blink {
 
@@ -263,6 +266,19 @@ static void PaintSelection(GraphicsContext& context,
             color);
 }
 
+void NGTextFragmentPainter::PaintSymbol(const PaintInfo& paint_info,
+                                        const LayoutPoint& paint_offset) {
+  const ComputedStyle& style = fragment_.Style();
+  LayoutRect marker_rect =
+      LayoutListMarker::RelativeSymbolMarkerRect(style, fragment_.Size().width);
+  marker_rect.MoveBy(fragment_.Offset().ToLayoutPoint());
+  marker_rect.MoveBy(paint_offset);
+  IntRect rect = PixelSnappedIntRect(marker_rect);
+
+  ListMarkerPainter::PaintSymbol(paint_info, fragment_.GetLayoutObject(), style,
+                                 rect);
+}
+
 // This is copied from InlineTextBoxPainter::PaintSelection() but lacks of
 // ltr, expanding new line wrap or so which uses InlineTextBox functions.
 void NGTextFragmentPainter::Paint(const PaintInfo& paint_info,
@@ -275,6 +291,45 @@ void NGTextFragmentPainter::Paint(const PaintInfo& paint_info,
   if (!ShouldPaintTextFragment(text_fragment, style))
     return;
 
+  bool is_printing = paint_info.IsPrinting();
+
+  // Determine whether or not we're selected.
+  bool have_selection = !is_printing &&
+                        paint_info.phase != PaintPhase::kTextClip &&
+                        text_fragment.GetLayoutObject()->IsSelected();
+  base::Optional<LayoutSelectionStatus> selection_status;
+  if (have_selection) {
+    selection_status =
+        document.GetFrame()->Selection().ComputeLayoutSelectionStatus(
+            fragment_);
+    DCHECK_LE(selection_status->start, selection_status->end);
+    have_selection = selection_status->start < selection_status->end;
+  }
+  if (!have_selection && paint_info.phase == PaintPhase::kSelection) {
+    // When only painting the selection, don't bother to paint if there is none.
+    return;
+  }
+
+  // The text clip phase already has a DrawingRecorder. Text clips are initiated
+  // only in BoxPainterBase::PaintFillLayer, which is already within a
+  // DrawingRecorder.
+  base::Optional<DrawingRecorder> recorder;
+  if (paint_info.phase != PaintPhase::kTextClip) {
+    if (DrawingRecorder::UseCachedDrawingIfPossible(
+            paint_info.context, fragment_, paint_info.phase))
+      return;
+    recorder.emplace(paint_info.context, fragment_, paint_info.phase);
+  }
+
+  if (UNLIKELY(text_fragment.TextType() ==
+               NGPhysicalTextFragment::kSymbolMarker)) {
+    // The NGInlineItem of marker might be Split(). So PaintSymbol only if the
+    // StartOffset is 0, or it might be painted several times.
+    if (!text_fragment.StartOffset())
+      PaintSymbol(paint_info, paint_offset);
+    return;
+  }
+
   // We round the y-axis to ensure consistent line heights.
   LayoutPoint adjusted_paint_offset =
       LayoutPoint(paint_offset.X(), LayoutUnit(paint_offset.Y().Round()));
@@ -285,18 +340,6 @@ void NGTextFragmentPainter::Paint(const PaintInfo& paint_info,
 
   GraphicsContext& context = paint_info.context;
 
-  bool is_printing = paint_info.IsPrinting();
-
-  // Determine whether or not we're selected.
-  const LayoutSelectionStatus& selection_status =
-      document.GetFrame()->Selection().ComputeLayoutSelectionStatus(fragment_);
-  DCHECK_LE(selection_status.start, selection_status.end);
-  const bool have_selection = selection_status.start < selection_status.end;
-  if (!have_selection && paint_info.phase == PaintPhase::kSelection) {
-    // When only painting the selection, don't bother to paint if there is none.
-    return;
-  }
-
   // Determine text colors.
   TextPaintStyle text_style =
       TextPainterBase::TextPaintingStyle(document, style, paint_info);
@@ -332,7 +375,7 @@ void NGTextFragmentPainter::Paint(const PaintInfo& paint_info,
 
     if (have_selection) {
       PaintSelection(context, fragment_, document, style,
-                     selection_style.fill_color, box_rect, selection_status);
+                     selection_style.fill_color, box_rect, *selection_status);
     }
   }
 
@@ -393,12 +436,12 @@ void NGTextFragmentPainter::Paint(const PaintInfo& paint_info,
 
     if (have_selection && paint_selected_text_separately) {
       // Paint only the text that is not selected.
-      if (start_offset < selection_status.start) {
-        text_painter.Paint(start_offset, selection_status.start, length,
+      if (start_offset < selection_status->start) {
+        text_painter.Paint(start_offset, selection_status->start, length,
                            text_style);
       }
-      if (selection_status.end < end_offset) {
-        text_painter.Paint(selection_status.end, end_offset, length,
+      if (selection_status->end < end_offset) {
+        text_painter.Paint(selection_status->end, end_offset, length,
                            text_style);
       }
     } else {
@@ -416,7 +459,7 @@ void NGTextFragmentPainter::Paint(const PaintInfo& paint_info,
   if (have_selection &&
       (paint_selected_text_only || paint_selected_text_separately)) {
     // Paint only the text that is selected.
-    text_painter.Paint(selection_status.start, selection_status.end, length,
+    text_painter.Paint(selection_status->start, selection_status->end, length,
                        selection_style);
   }
 

third_party/blink/renderer/core/paint/ng/ng_text_fragment_painter.h

@@ -27,6 +27,9 @@ class NGTextFragmentPainter {
   void Paint(const PaintInfo&, const LayoutPoint& paint_offset);
 
  private:
+  void PaintSymbol(const PaintInfo& paint_info,
+                   const LayoutPoint& paint_offset);
+
   const NGPaintFragment& fragment_;
 };