diff --git a/pdf/pdf_ink_metrics_handler.cc b/pdf/pdf_ink_metrics_handler.cc index bcdaf5399696b..e1c1363aae430 100644 --- a/pdf/pdf_ink_metrics_handler.cc +++ b/pdf/pdf_ink_metrics_handler.cc @@ -4,27 +4,70 @@ #include "pdf/pdf_ink_metrics_handler.h" +#include "base/containers/fixed_flat_map.h" #include "base/metrics/histogram_functions.h" +#include "base/notreached.h" #include "pdf/pdf_ink_brush.h" namespace chrome_pdf { namespace { -void ReportStrokeType(StrokeMetricBrushType type) { +// Pens and erasers share the same sizes. +constexpr auto kPenAndEraserSizes = + base::MakeFixedFlatMap<float, StrokeMetricBrushSize>({ + {1.0f, StrokeMetricBrushSize::kExtraThin}, + {2.0f, StrokeMetricBrushSize::kThin}, + {3.0f, StrokeMetricBrushSize::kMedium}, + {6.0f, StrokeMetricBrushSize::kThick}, + {8.0f, StrokeMetricBrushSize::kExtraThick}, + }); + +constexpr auto kHighlighterSizes = + base::MakeFixedFlatMap<float, StrokeMetricBrushSize>({ + {4.0f, StrokeMetricBrushSize::kExtraThin}, + {6.0f, StrokeMetricBrushSize::kThin}, + {8.0f, StrokeMetricBrushSize::kMedium}, + {12.0f, StrokeMetricBrushSize::kThick}, + {16.0f, StrokeMetricBrushSize::kExtraThick}, + }); + +void ReportStrokeTypeAndSize(StrokeMetricBrushType type, + StrokeMetricBrushSize size) { base::UmaHistogramEnumeration("PDF.Ink2StrokeBrushType", type); + const char* size_metric = nullptr; + switch (type) { + case StrokeMetricBrushType::kPen: + size_metric = "PDF.Ink2StrokePenSize"; + break; + case StrokeMetricBrushType::kHighlighter: + size_metric = "PDF.Ink2StrokeHighlighterSize"; + break; + case StrokeMetricBrushType::kEraser: + size_metric = "PDF.Ink2StrokeEraserSize"; + break; + }; + CHECK(size_metric); + base::UmaHistogramEnumeration(size_metric, size); } } // namespace -void ReportDrawStroke(PdfInkBrush::Type type) { +void ReportDrawStroke(PdfInkBrush::Type type, const ink::Brush& brush) { bool is_pen = type == PdfInkBrush::Type::kPen; - ReportStrokeType(is_pen ? StrokeMetricBrushType::kPen - : StrokeMetricBrushType::kHighlighter); + const base::fixed_flat_map<float, StrokeMetricBrushSize, 5>& sizes = + is_pen ? kPenAndEraserSizes : kHighlighterSizes; + auto size_iter = sizes.find(brush.GetSize()); + CHECK(size_iter != sizes.end()); + ReportStrokeTypeAndSize(is_pen ? StrokeMetricBrushType::kPen + : StrokeMetricBrushType::kHighlighter, + size_iter->second); } -void ReportEraseStroke() { - ReportStrokeType(StrokeMetricBrushType::kEraser); +void ReportEraseStroke(float size) { + auto iter = kPenAndEraserSizes.find(size); + CHECK(iter != kPenAndEraserSizes.end()); + ReportStrokeTypeAndSize(StrokeMetricBrushType::kEraser, iter->second); } } // namespace chrome_pdf diff --git a/pdf/pdf_ink_metrics_handler.h b/pdf/pdf_ink_metrics_handler.h index 293f5228cd2de..92aeb49960710 100644 --- a/pdf/pdf_ink_metrics_handler.h +++ b/pdf/pdf_ink_metrics_handler.h @@ -12,6 +12,20 @@ static_assert(BUILDFLAG(ENABLE_PDF_INK2), "ENABLE_PDF_INK2 not set to true"); namespace chrome_pdf { +// These values are persisted to logs. Entries should not be renumbered and +// numeric values should never be reused. +// +// LINT.IfChange(PDFInk2StrokeBrushSize) +enum class StrokeMetricBrushSize { + kExtraThin = 0, + kThin = 1, + kMedium = 2, + kThick = 3, + kExtraThick = 4, + kMaxValue = 4, +}; +// LINT.ThenChange(//tools/metrics/histograms/metadata/pdf/enums.xml:PDFInk2StrokeBrushSize) + // These values are persisted to logs. Entries should not be renumbered and // numeric values should never be reused. // @@ -24,9 +38,9 @@ enum class StrokeMetricBrushType { }; // LINT.ThenChange(//tools/metrics/histograms/metadata/pdf/enums.xml:PDFInk2StrokeBrushType) -void ReportDrawStroke(PdfInkBrush::Type type); +void ReportDrawStroke(PdfInkBrush::Type type, const ink::Brush& brush); -void ReportEraseStroke(); +void ReportEraseStroke(float size); } // namespace chrome_pdf diff --git a/pdf/pdf_ink_module.cc b/pdf/pdf_ink_module.cc index 49fa6a71b1fea..64957d92bf385 100644 --- a/pdf/pdf_ink_module.cc +++ b/pdf/pdf_ink_module.cc @@ -536,7 +536,7 @@ bool PdfInkModule::FinishStroke(const gfx::PointF& position, bool undo_redo_success = undo_redo_model_.FinishDraw(); CHECK(undo_redo_success); - ReportDrawStroke(state.brush_type); + ReportDrawStroke(state.brush_type, GetDrawingBrush().ink_brush()); // Reset `state` now that the stroke operation is done. state.inputs.clear(); @@ -608,7 +608,7 @@ bool PdfInkModule::FinishEraseStroke(const gfx::PointF& position) { client_->UpdateThumbnail(page_index); } - ReportEraseStroke(); + ReportEraseStroke(eraser_size_); } // Reset `state` now that the erase operation is done. diff --git a/pdf/pdf_ink_module_unittest.cc b/pdf/pdf_ink_module_unittest.cc index 26bdd5784ae4a..dec50c56db2ed 100644 --- a/pdf/pdf_ink_module_unittest.cc +++ b/pdf/pdf_ink_module_unittest.cc @@ -1986,6 +1986,10 @@ TEST_F(PdfInkModuleGetVisibleStrokesTest, MultiplePageStrokes) { class PdfInkModuleMetricsTest : public PdfInkModuleUndoRedoTest { protected: static constexpr char kTypeMetric[] = "PDF.Ink2StrokeBrushType"; + static constexpr char kPenSizeMetric[] = "PDF.Ink2StrokePenSize"; + static constexpr char kHighlighterSizeMetric[] = + "PDF.Ink2StrokeHighlighterSize"; + static constexpr char kEraserSizeMetric[] = "PDF.Ink2StrokeEraserSize"; }; TEST_F(PdfInkModuleMetricsTest, StrokeUndoRedoDoesNotAffectMetrics) { @@ -1996,6 +2000,8 @@ TEST_F(PdfInkModuleMetricsTest, StrokeUndoRedoDoesNotAffectMetrics) { RunStrokeCheckTest(/*annotation_mode_enabled=*/true); histograms.ExpectUniqueSample(kTypeMetric, StrokeMetricBrushType::kPen, 1); + histograms.ExpectUniqueSample(kPenSizeMetric, StrokeMetricBrushSize::kMedium, + 1); // Undo and redo. PerformUndo(); @@ -2003,6 +2009,119 @@ TEST_F(PdfInkModuleMetricsTest, StrokeUndoRedoDoesNotAffectMetrics) { // The metrics should stay the same. histograms.ExpectUniqueSample(kTypeMetric, StrokeMetricBrushType::kPen, 1); + histograms.ExpectUniqueSample(kPenSizeMetric, StrokeMetricBrushSize::kMedium, + 1); +} + +TEST_F(PdfInkModuleMetricsTest, StrokeBrushSizePen) { + InitializeSimpleSinglePageBasicLayout(); + base::HistogramTester histograms; + + // Draw a stroke. + RunStrokeCheckTest(/*annotation_mode_enabled=*/true); + + histograms.ExpectUniqueSample(kPenSizeMetric, StrokeMetricBrushSize::kMedium, + 1); + + TestAnnotationBrushMessageParams params = {/*color_r=*/242, /*color_g=*/139, + /*color_b=*/130}; + SelectBrushTool(PdfInkBrush::Type::kPen, 1.0f, params); + ApplyStrokeWithMouseAtMouseDownPoint(); + + histograms.ExpectBucketCount(kPenSizeMetric, + StrokeMetricBrushSize::kExtraThin, 1); + histograms.ExpectTotalCount(kPenSizeMetric, 2); + + SelectBrushTool(PdfInkBrush::Type::kPen, 8.0f, params); + ApplyStrokeWithMouseAtMouseDownPoint(); + + histograms.ExpectBucketCount(kPenSizeMetric, + StrokeMetricBrushSize::kExtraThick, 1); + histograms.ExpectTotalCount(kPenSizeMetric, 3); + histograms.ExpectTotalCount(kHighlighterSizeMetric, 0); + histograms.ExpectTotalCount(kEraserSizeMetric, 0); +} + +TEST_F(PdfInkModuleMetricsTest, StrokeBrushSizeHighlighter) { + EnableAnnotationMode(); + InitializeSimpleSinglePageBasicLayout(); + base::HistogramTester histograms; + + // Draw a stroke with medium size. + TestAnnotationBrushMessageParams params = {/*color_r=*/242, /*color_g=*/139, + /*color_b=*/130}; + SelectBrushTool(PdfInkBrush::Type::kHighlighter, 8.0f, params); + ApplyStrokeWithMouseAtMouseDownPoint(); + + histograms.ExpectUniqueSample(kHighlighterSizeMetric, + StrokeMetricBrushSize::kMedium, 1); + + // Draw a stroke with extra thin size. + SelectBrushTool(PdfInkBrush::Type::kHighlighter, 4.0f, params); + ApplyStrokeWithMouseAtMouseDownPoint(); + + histograms.ExpectBucketCount(kHighlighterSizeMetric, + StrokeMetricBrushSize::kExtraThin, 1); + histograms.ExpectTotalCount(kHighlighterSizeMetric, 2); + + // Draw a stroke with extra thick size. + SelectBrushTool(PdfInkBrush::Type::kHighlighter, 16.0f, params); + ApplyStrokeWithMouseAtMouseDownPoint(); + + histograms.ExpectBucketCount(kHighlighterSizeMetric, + StrokeMetricBrushSize::kExtraThick, 1); + histograms.ExpectTotalCount(kPenSizeMetric, 0); + histograms.ExpectTotalCount(kHighlighterSizeMetric, 3); + histograms.ExpectTotalCount(kEraserSizeMetric, 0); +} + +TEST_F(PdfInkModuleMetricsTest, StrokeBrushSizeEraser) { + EnableAnnotationMode(); + InitializeSimpleSinglePageBasicLayout(); + base::HistogramTester histograms; + + // Draw a pen stroke. Draw an eraser stroke that erases it with medium size. + TestAnnotationBrushMessageParams params = {/*color_r=*/242, /*color_g=*/139, + /*color_b=*/130}; + SelectBrushTool(PdfInkBrush::Type::kPen, 3.0f, params); + ApplyStrokeWithMouseAtMouseDownPoint(); + SelectEraserToolOfSize(3.0f); + ApplyStrokeWithMouseAtMouseDownPoint(); + + histograms.ExpectUniqueSample(kEraserSizeMetric, + StrokeMetricBrushSize::kMedium, 1); + + // Draw a pen stroke. Draw an eraser stroke that erases it with extra thin + // size. + SelectBrushTool(PdfInkBrush::Type::kPen, 3.0f, params); + ApplyStrokeWithMouseAtMouseDownPoint(); + SelectEraserToolOfSize(1.0f); + ApplyStrokeWithMouseAtMouseDownPoint(); + + histograms.ExpectBucketCount(kEraserSizeMetric, + StrokeMetricBrushSize::kExtraThin, 1); + histograms.ExpectTotalCount(kEraserSizeMetric, 2); + + // Draw a pen stroke. Draw an eraser stroke that erases it with extra thick + // size. + SelectBrushTool(PdfInkBrush::Type::kPen, 3.0f, params); + ApplyStrokeWithMouseAtMouseDownPoint(); + SelectEraserToolOfSize(8.0f); + ApplyStrokeWithMouseAtMouseDownPoint(); + + histograms.ExpectBucketCount(kEraserSizeMetric, + StrokeMetricBrushSize::kExtraThick, 1); + + // There should be no visible strokes on the page. Draw an eraser stroke that + // does not erase any other strokes. The metric should stay the same. + ApplyStrokeWithMouseAtMouseDownPoint(); + + histograms.ExpectBucketCount(kEraserSizeMetric, + StrokeMetricBrushSize::kExtraThick, 1); + + histograms.ExpectTotalCount(kPenSizeMetric, 3); + histograms.ExpectTotalCount(kHighlighterSizeMetric, 0); + histograms.ExpectTotalCount(kEraserSizeMetric, 3); } TEST_F(PdfInkModuleMetricsTest, StrokeBrushType) { diff --git a/tools/metrics/histograms/metadata/pdf/enums.xml b/tools/metrics/histograms/metadata/pdf/enums.xml index 96aa97a8041cc..86090cc8e3448 100644 --- a/tools/metrics/histograms/metadata/pdf/enums.xml +++ b/tools/metrics/histograms/metadata/pdf/enums.xml @@ -144,6 +144,18 @@ chromium-metrics-reviews@google.com. <int value="3" label="Foreground XFA (XFAF)"/> </enum> +<!-- LINT.IfChange(PDFInk2StrokeBrushSize) --> + +<enum name="PDFInk2StrokeBrushSize"> + <int value="0" label="ExtraThin"/> + <int value="1" label="Thin"/> + <int value="2" label="Medium"/> + <int value="3" label="Thick"/> + <int value="4" label="ExtraThick"/> +</enum> + +<!-- LINT.ThenChange(//pdf/pdf_ink_metrics_handler.h:PDFInk2StrokeBrushSize) --> + <!-- LINT.IfChange(PDFInk2StrokeBrushType) --> <enum name="PDFInk2StrokeBrushType"> diff --git a/tools/metrics/histograms/metadata/pdf/histograms.xml b/tools/metrics/histograms/metadata/pdf/histograms.xml index 6e647c3e51485..fa2e5e706e39a 100644 --- a/tools/metrics/histograms/metadata/pdf/histograms.xml +++ b/tools/metrics/histograms/metadata/pdf/histograms.xml @@ -91,6 +91,25 @@ chromium-metrics-reviews@google.com. </summary> </histogram> +<histogram name="PDF.Ink2Stroke{Brush}Size" enum="PDFInk2StrokeBrushSize" + expires_after="2025-12-01"> + <owner>andyphan@chromium.org</owner> + <owner>thestig@chromium.org</owner> + <summary> + Tracks the brush size used for an Ink2 {Brush} stroke modification in the + PDF viewer. This includes new drawing strokes as well as erasing strokes. + Erase strokes that erase pre-existing strokes in a PDF are also included. + This is only recorded when drawing or erasing actions are performed by the + user, but not if they occur as part of undo or redo operations. Eraser + strokes that do not erase any other strokes are ignored. + </summary> + <token key="Brush"> + <variant name="Eraser"/> + <variant name="Highlighter"/> + <variant name="Pen"/> + </token> +</histogram> + <histogram name="PDF.LoadStatus2" enum="ChromePDFViewerLoadStatus" expires_after="2025-03-30"> <owner>kmoon@chromium.org</owner>