[headless] Streamline HeadlessPrintManager implementation.

This CL removes unnecessary state held while the PrintToPdf() method
is running as well as the rfh->PrintingDone() call at the end of
the run.

Drive by: got rid of the print_to_pdf::PageRangeError and used
the generic print_to_pdf::PdfPrintResult for TextPageRangesToPageRanges
error reporting thus eliminating error code conversion.

Change-Id: Ib9ce5ce0718787a93b565f0f0ce91f7e2093d492
Cq-Include-Trybots: luci.chromium.try:linux-headless-shell-rel
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3780384
Reviewed-by: Andrey Kosyakov <caseq@chromium.org>
Commit-Queue: Peter Kvitek <kvitekp@chromium.org>
Reviewed-by: Lei Zhang <thestig@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1028534}

chrome/browser/devtools/protocol/page_handler.cc

@@ -278,7 +278,7 @@ void PageHandler::OnPDFCreated(bool return_as_stream,
                                std::unique_ptr<PrintToPDFCallback> callback,
                                print_to_pdf::PdfPrintResult print_result,
                                scoped_refptr<base::RefCountedMemory> data) {
-  if (print_result != print_to_pdf::PdfPrintResult::PRINT_SUCCESS) {
+  if (print_result != print_to_pdf::PdfPrintResult::kPrintSuccess) {
     callback->sendFailure(protocol::Response::ServerError(
         print_to_pdf::PdfPrintResultToString(print_result)));
     return;

components/printing/browser/headless/headless_print_manager.cc

@@ -17,7 +17,6 @@
 #include "mojo/public/cpp/bindings/message.h"
 #endif
 
-using print_to_pdf::PageRangeError;
 using print_to_pdf::PdfPrintResult;
 
 namespace headless {
@@ -58,39 +57,31 @@ void HeadlessPrintManager::PrintToPdf(
     PrintToPdfCallback callback) {
   DCHECK(callback);
 
-  if (callback_) {
-    std::move(callback).Run(PdfPrintResult::SIMULTANEOUS_PRINT_ACTIVE,
+  if (print_to_pdf_callback_) {
+    std::move(callback).Run(PdfPrintResult::kSimultaneousPrintActive,
                             base::MakeRefCounted<base::RefCountedString>());
     return;
   }
 
   if (!rfh->IsRenderFrameLive()) {
-    std::move(callback).Run(PdfPrintResult::PRINTING_FAILED,
+    std::move(callback).Run(PdfPrintResult::kPrintFailure,
                             base::MakeRefCounted<base::RefCountedString>());
     return;
   }
 
-  absl::variant<printing::PageRanges, PageRangeError> parsed_ranges =
+  absl::variant<printing::PageRanges, PdfPrintResult> parsed_ranges =
       print_to_pdf::TextPageRangesToPageRanges(page_ranges);
-  if (absl::holds_alternative<PageRangeError>(parsed_ranges)) {
-    PdfPrintResult print_result;
-    switch (absl::get<PageRangeError>(parsed_ranges)) {
-      case PageRangeError::kSyntaxError:
-        print_result = PdfPrintResult::PAGE_RANGE_SYNTAX_ERROR;
-        break;
-      case PageRangeError::kInvalidRange:
-        print_result = PdfPrintResult::PAGE_RANGE_INVALID_RANGE;
-        break;
-    }
-    std::move(callback).Run(print_result,
+  if (absl::holds_alternative<PdfPrintResult>(parsed_ranges)) {
+    DCHECK_NE(absl::get<PdfPrintResult>(parsed_ranges),
+              PdfPrintResult::kPrintSuccess);
+    std::move(callback).Run(absl::get<PdfPrintResult>(parsed_ranges),
                             base::MakeRefCounted<base::RefCountedString>());
     return;
   }
 
   printing_rfh_ = rfh;
   print_pages_params->pages = absl::get<printing::PageRanges>(parsed_ranges);
-  set_cookie(print_pages_params->params->document_cookie);
-  callback_ = std::move(callback);
+  print_to_pdf_callback_ = std::move(callback);
 
   // There is no need for a weak pointer here since the mojo proxy is held
   // in the base class. If we're gone, mojo will discard the callback.
@@ -100,31 +91,14 @@ void HeadlessPrintManager::PrintToPdf(
                      base::Unretained(this)));
 }
 
-void HeadlessPrintManager::OnDidPrintWithParams(
-    printing::mojom::PrintWithParamsResultPtr result) {
-  if (result->is_failure_reason()) {
-    switch (result->get_failure_reason()) {
-      case printing::mojom::PrintFailureReason::kGeneralFailure:
-        ReleaseJob(PdfPrintResult::PRINTING_FAILED);
-        return;
-      case printing::mojom::PrintFailureReason::kInvalidPageRange:
-        ReleaseJob(PdfPrintResult::PAGE_COUNT_EXCEEDED);
-        return;
-    }
-  }
+void HeadlessPrintManager::RenderFrameDeleted(
+    content::RenderFrameHost* render_frame_host) {
+  PrintManager::RenderFrameDeleted(render_frame_host);
 
-  auto& content = *result->get_params()->content;
-  if (!content.metafile_data_region.IsValid()) {
-    ReleaseJob(PdfPrintResult::INVALID_MEMORY_HANDLE);
-    return;
-  }
-  base::ReadOnlySharedMemoryMapping map = content.metafile_data_region.Map();
-  if (!map.IsValid()) {
-    ReleaseJob(PdfPrintResult::METAFILE_MAP_ERROR);
+  if (printing_rfh_ != render_frame_host)
     return;
-  }
-  data_ = std::string(static_cast<const char*>(map.memory()), map.size());
-  ReleaseJob(PdfPrintResult::PRINT_SUCCESS);
+
+  FailJob(PdfPrintResult::kPrintFailure);
 }
 
 void HeadlessPrintManager::GetDefaultPrintSettings(
@@ -143,7 +117,7 @@ void HeadlessPrintManager::ScriptedPrint(
 }
 
 void HeadlessPrintManager::ShowInvalidPrinterSettingsError() {
-  ReleaseJob(PdfPrintResult::INVALID_PRINTER_SETTINGS);
+  FailJob(PdfPrintResult::kInvalidPrinterSettings);
 }
 
 #if BUILDFLAG(ENABLE_PRINT_PREVIEW)
@@ -187,50 +161,59 @@ void HeadlessPrintManager::SetAccessibilityTree(
 void HeadlessPrintManager::PdfWritingDone(int page_count) {}
 #endif
 
-void HeadlessPrintManager::RenderFrameDeleted(
-    content::RenderFrameHost* render_frame_host) {
-  PrintManager::RenderFrameDeleted(render_frame_host);
+void HeadlessPrintManager::OnDidPrintWithParams(
+    printing::mojom::PrintWithParamsResultPtr result) {
+  if (result->is_failure_reason()) {
+    switch (result->get_failure_reason()) {
+      case printing::mojom::PrintFailureReason::kGeneralFailure:
+        FailJob(PdfPrintResult::kPrintFailure);
+        return;
+      case printing::mojom::PrintFailureReason::kInvalidPageRange:
+        FailJob(PdfPrintResult::kPageCountExceeded);
+        return;
+    }
+  }
 
-  if (printing_rfh_ != render_frame_host) {
+  auto& content = *result->get_params()->content;
+  if (!content.metafile_data_region.IsValid()) {
+    FailJob(PdfPrintResult::kInvalidMemoryHandle);
     return;
   }
 
-  if (callback_) {
-    std::move(callback_).Run(PdfPrintResult::PRINTING_FAILED,
-                             base::MakeRefCounted<base::RefCountedString>());
+  base::ReadOnlySharedMemoryMapping map = content.metafile_data_region.Map();
+  if (!map.IsValid()) {
+    FailJob(PdfPrintResult::kMetafileMapError);
+    return;
   }
 
+  std::string data =
+      std::string(static_cast<const char*>(map.memory()), map.size());
+  std::move(print_to_pdf_callback_)
+      .Run(PdfPrintResult::kPrintSuccess,
+           base::RefCountedString::TakeString(&data));
+
   Reset();
 }
 
-void HeadlessPrintManager::Reset() {
-  printing_rfh_ = nullptr;
-  callback_.Reset();
-  data_.clear();
-}
+void HeadlessPrintManager::FailJob(PdfPrintResult result) {
+  DCHECK_NE(result, PdfPrintResult::kPrintSuccess);
 
-void HeadlessPrintManager::ReleaseJob(PdfPrintResult result) {
-  if (!callback_) {
-    DLOG(ERROR) << "ReleaseJob is called when callback_ is null. Check whether "
-                   "ReleaseJob is called more than once.";
-    return;
+  if (print_to_pdf_callback_) {
+    std::move(print_to_pdf_callback_)
+        .Run(result, base::MakeRefCounted<base::RefCountedString>());
   }
 
-  DCHECK(result == PdfPrintResult::PRINT_SUCCESS || data_.empty());
-  std::move(callback_).Run(result, base::RefCountedString::TakeString(&data_));
-  // TODO(https://crbug.com/1286556): In theory, this should not be needed. In
-  // practice, nothing seems to restrict receiving incoming Mojo method calls
-  // for reporting the printing state to `printing_rfh_`.
-  //
-  // This should probably be changed so that the browser pushes endpoints to the
-  // renderer rather than the renderer connecting on-demand to the browser...
-  if (printing_rfh_ && printing_rfh_->IsRenderFrameLive()) {
-    GetPrintRenderFrame(printing_rfh_)
-        ->PrintingDone(result == PdfPrintResult::PRINT_SUCCESS);
-  }
   Reset();
 }
 
+void HeadlessPrintManager::Reset() {
+  printing_rfh_ = nullptr;
+
+  // The callback is supposed to be consumed at this point meaning we
+  // reported results to the PrintToPdf() caller.
+  CHECK(!print_to_pdf_callback_);
+}
+
 WEB_CONTENTS_USER_DATA_KEY_IMPL(HeadlessPrintManager);
 
 }  // namespace headless

components/printing/browser/headless/headless_print_manager.h

@@ -53,8 +53,6 @@ class HeadlessPrintManager
 
   explicit HeadlessPrintManager(content::WebContents* web_contents);
 
-  void OnDidPrintWithParams(printing::mojom::PrintWithParamsResultPtr result);
-
   // WebContentsObserver overrides (via PrintManager):
   void RenderFrameDeleted(content::RenderFrameHost* render_frame_host) override;
 
@@ -86,12 +84,13 @@ class HeadlessPrintManager
   void PdfWritingDone(int page_count) override;
 #endif
 
+  void OnDidPrintWithParams(printing::mojom::PrintWithParamsResultPtr result);
+
+  void FailJob(print_to_pdf::PdfPrintResult result);
   void Reset();
-  void ReleaseJob(print_to_pdf::PdfPrintResult result);
 
   raw_ptr<content::RenderFrameHost> printing_rfh_ = nullptr;
-  PrintToPdfCallback callback_;
-  std::string data_;
+  PrintToPdfCallback print_to_pdf_callback_;
 
   WEB_CONTENTS_USER_DATA_KEY_DECL();
 };

components/printing/browser/print_to_pdf/pdf_print_result.cc

@@ -8,23 +8,23 @@ namespace print_to_pdf {
 
 std::string PdfPrintResultToString(PdfPrintResult result) {
   switch (result) {
-    case PdfPrintResult::PRINT_SUCCESS:
+    case PdfPrintResult::kPrintSuccess:
       return std::string();  // no error message
-    case PdfPrintResult::PRINTING_FAILED:
+    case PdfPrintResult::kPrintFailure:
       return "Printing failed";
-    case PdfPrintResult::INVALID_PRINTER_SETTINGS:
+    case PdfPrintResult::kInvalidPrinterSettings:
       return "Show invalid printer settings error";
-    case PdfPrintResult::INVALID_MEMORY_HANDLE:
+    case PdfPrintResult::kInvalidMemoryHandle:
       return "Invalid memory handle";
-    case PdfPrintResult::METAFILE_MAP_ERROR:
+    case PdfPrintResult::kMetafileMapError:
       return "Map to shared memory error";
-    case PdfPrintResult::SIMULTANEOUS_PRINT_ACTIVE:
+    case PdfPrintResult::kSimultaneousPrintActive:
       return "The previous printing job hasn't finished";
-    case PdfPrintResult::PAGE_RANGE_SYNTAX_ERROR:
+    case PdfPrintResult::kPageRangeSyntaxError:
       return "Page range syntax error";
-    case PdfPrintResult::PAGE_RANGE_INVALID_RANGE:
+    case PdfPrintResult::kPageRangeInvalidRange:
       return "Page range is invalid (start > end)";
-    case PdfPrintResult::PAGE_COUNT_EXCEEDED:
+    case PdfPrintResult::kPageCountExceeded:
       return "Page range exceeds page count";
   }
 }

components/printing/browser/print_to_pdf/pdf_print_result.h

@@ -10,16 +10,15 @@
 namespace print_to_pdf {
 
 enum class PdfPrintResult {
-  PRINT_SUCCESS,
-  PRINTING_FAILED,
-  INVALID_PRINTER_SETTINGS,
-  INVALID_MEMORY_HANDLE,
-  METAFILE_MAP_ERROR,
-  SIMULTANEOUS_PRINT_ACTIVE,
-  PAGE_RANGE_SYNTAX_ERROR,
-  PAGE_RANGE_INVALID_RANGE,
-  PAGE_COUNT_EXCEEDED,
-
+  kPrintSuccess,
+  kPrintFailure,
+  kInvalidPrinterSettings,
+  kInvalidMemoryHandle,
+  kMetafileMapError,
+  kSimultaneousPrintActive,
+  kPageRangeSyntaxError,
+  kPageRangeInvalidRange,
+  kPageCountExceeded,
 };
 
 std::string PdfPrintResultToString(PdfPrintResult result);

components/printing/browser/print_to_pdf/pdf_print_utils.cc

@@ -33,7 +33,7 @@ static constexpr double kDefaultMarginInInches =
 
 }  // namespace
 
-absl::variant<printing::PageRanges, PageRangeError> TextPageRangesToPageRanges(
+absl::variant<printing::PageRanges, PdfPrintResult> TextPageRangesToPageRanges(
     base::StringPiece page_range_text) {
   printing::PageRanges page_ranges;
   for (const auto& range_string :
@@ -42,7 +42,7 @@ absl::variant<printing::PageRanges, PageRangeError> TextPageRangesToPageRanges(
     printing::PageRange range;
     if (range_string.find("-") == base::StringPiece::npos) {
       if (!base::StringToUint(range_string, &range.from))
-        return PageRangeError::kSyntaxError;
+        return PdfPrintResult::kPageRangeSyntaxError;
       range.to = range.from;
     } else if (range_string == "-") {
       range.from = 1;
@@ -54,25 +54,25 @@ absl::variant<printing::PageRanges, PageRangeError> TextPageRangesToPageRanges(
     } else if (base::StartsWith(range_string, "-")) {
       range.from = 1;
       if (!base::StringToUint(range_string.substr(1), &range.to))
-        return PageRangeError::kSyntaxError;
+        return PdfPrintResult::kPageRangeSyntaxError;
     } else if (base::EndsWith(range_string, "-")) {
       // See comment regarding kMaxPage above.
       range.to = printing::PageRange::kMaxPage + 1;
       if (!base::StringToUint(range_string.substr(0, range_string.length() - 1),
                               &range.from)) {
-        return PageRangeError::kSyntaxError;
+        return PdfPrintResult::kPageRangeSyntaxError;
       }
     } else {
       auto tokens = base::SplitStringPiece(
           range_string, "-", base::TRIM_WHITESPACE, base::SPLIT_WANT_NONEMPTY);
       if (tokens.size() != 2 || !base::StringToUint(tokens[0], &range.from) ||
           !base::StringToUint(tokens[1], &range.to)) {
-        return PageRangeError::kSyntaxError;
+        return PdfPrintResult::kPageRangeSyntaxError;
       }
     }
 
     if (range.from < 1 || range.from > range.to)
-      return PageRangeError::kInvalidRange;
+      return PdfPrintResult::kPageRangeInvalidRange;
 
     // Page numbers are 1-based in the dictionary.
     // Page numbers are 0-based for the print settings.

components/printing/browser/print_to_pdf/pdf_print_utils.h

@@ -8,6 +8,7 @@
 #include <string>
 
 #include "base/strings/string_piece.h"
+#include "components/printing/browser/print_to_pdf/pdf_print_result.h"
 #include "components/printing/common/print.mojom.h"
 #include "printing/page_range.h"
 #include "third_party/abseil-cpp/absl/types/optional.h"
@@ -16,11 +17,9 @@
 
 namespace print_to_pdf {
 
-enum class PageRangeError { kSyntaxError, kInvalidRange };
-
 // Converts textual representation of the page range to printing::PageRanges,
-// page range error is returned as the PageRangeError variant case.
-absl::variant<printing::PageRanges, PageRangeError> TextPageRangesToPageRanges(
+// page range error is returned as the PdfPrintResult variant case.
+absl::variant<printing::PageRanges, PdfPrintResult> TextPageRangesToPageRanges(
     base::StringPiece page_range_text);
 
 // Converts print settings to printing::mojom::PrintPagesParamsPtr,

components/printing/browser/print_to_pdf/pdf_print_utils_unittest.cc

@@ -4,6 +4,7 @@
 
 #include "components/printing/browser/print_to_pdf/pdf_print_utils.h"
 
+#include "components/printing/browser/print_to_pdf/pdf_print_result.h"
 #include "printing/page_number.h"
 #include "testing/gmock/include/gmock/gmock.h"
 #include "testing/gtest/include/gtest/gtest.h"
@@ -43,16 +44,19 @@ TEST(PageRangeTextToPagesTest, General) {
                   ElementsAreArray<PageRange>({{0, 2}, {8, 9}, {3, 5}})));
 
   // Range with a start page greater than the end page results in an error.
-  EXPECT_THAT(TextPageRangesToPageRanges("6-4"),
-              VariantWith<PageRangeError>(PageRangeError::kInvalidRange));
+  EXPECT_THAT(
+      TextPageRangesToPageRanges("6-4"),
+      VariantWith<PdfPrintResult>(PdfPrintResult::kPageRangeInvalidRange));
 
   // Invalid input results in an error.
-  EXPECT_THAT(TextPageRangesToPageRanges("abcd"),
-              VariantWith<PageRangeError>(PageRangeError::kSyntaxError));
+  EXPECT_THAT(
+      TextPageRangesToPageRanges("abcd"),
+      VariantWith<PdfPrintResult>(PdfPrintResult::kPageRangeSyntaxError));
 
   // Invalid input results in an error.
-  EXPECT_THAT(TextPageRangesToPageRanges("1-3,9-a10,4-6"),
-              VariantWith<PageRangeError>(PageRangeError::kSyntaxError));
+  EXPECT_THAT(
+      TextPageRangesToPageRanges("1-3,9-a10,4-6"),
+      VariantWith<PdfPrintResult>(PdfPrintResult::kPageRangeSyntaxError));
 }
 
 }  // namespace