Clean up PrintPreviewUI.

- Simplify ctors.
- Rewrite GetCurrentPrintPreviewStatus() as ShouldCancelRequest().

Change-Id: Ie4cbb4dc7a56c1310ec4768687e800f7f40fe61a
Reviewed-on: https://chromium-review.googlesource.com/1184507Reviewed-by: Rebekah Potter <rbpotter@chromium.org>
Commit-Queue: Lei Zhang <thestig@chromium.org>
Cr-Commit-Position: refs/heads/master@{#585274}

chrome/browser/printing/print_preview_message_handler.cc

@@ -252,9 +252,7 @@ void PrintPreviewMessageHandler::NotifyUIPreviewPageReady(
     return;
 
   // Don't bother notifying the UI if this request has been cancelled already.
-  bool cancel = false;
-  PrintPreviewUI::GetCurrentPrintPreviewStatus(ids, &cancel);
-  if (cancel)
+  if (PrintPreviewUI::ShouldCancelRequest(ids))
     return;
 
   print_preview_ui->SetPrintPreviewDataForIndex(page_number,

chrome/browser/printing/printing_message_filter.cc

@@ -364,7 +364,7 @@ void PrintingMessageFilter::OnUpdatePrintSettingsReply(
 #if BUILDFLAG(ENABLE_PRINT_PREVIEW)
 void PrintingMessageFilter::OnCheckForCancel(const PrintHostMsg_PreviewIds& ids,
                                              bool* cancel) {
-  PrintPreviewUI::GetCurrentPrintPreviewStatus(ids, cancel);
+  *cancel = PrintPreviewUI::ShouldCancelRequest(ids);
 }
 #endif
 

chrome/browser/ui/webui/print_preview/print_preview_handler.cc

@@ -762,8 +762,7 @@ void PrintPreviewHandler::HandleGetPreview(const base::ListValue* args) {
   preview_callbacks_[request_id] = callback_id;
   print_preview_ui()->OnPrintPreviewRequest(request_id);
   // Add an additional key in order to identify |print_preview_ui| later on
-  // when calling PrintPreviewUI::GetCurrentPrintPreviewStatus() on the IO
-  // thread.
+  // when calling PrintPreviewUI::ShouldCancelRequest() on the IO thread.
   settings->SetInteger(printing::kPreviewUIID,
                        print_preview_ui()->GetIDForPrintPreviewUI());
 

chrome/browser/ui/webui/print_preview/print_preview_ui.cc

@@ -48,7 +48,6 @@
 #include "content/public/browser/web_ui_data_source.h"
 #include "content/public/common/content_features.h"
 #include "extensions/common/constants.h"
-#include "printing/buildflags/buildflags.h"
 #include "printing/page_size_margins.h"
 #include "printing/print_job_constants.h"
 #include "ui/base/l10n/l10n_util.h"
@@ -558,6 +557,14 @@ content::WebUIDataSource* CreatePrintPreviewUISource(Profile* profile) {
   return source;
 }
 
+PrintPreviewHandler* CreatePrintPreviewHandlers(content::WebUI* web_ui) {
+  auto handler = std::make_unique<PrintPreviewHandler>();
+  PrintPreviewHandler* handler_ptr = handler.get();
+  web_ui->AddMessageHandler(std::move(handler));
+  web_ui->AddMessageHandler(std::make_unique<MetricsHandler>());
+  return handler_ptr;
+}
+
 }  // namespace
 
 PrintPreviewUI::PrintPreviewUI(content::WebUI* web_ui,
@@ -565,12 +572,7 @@ PrintPreviewUI::PrintPreviewUI(content::WebUI* web_ui,
     : ConstrainedWebDialogUI(web_ui),
       initial_preview_start_time_(base::TimeTicks::Now()),
       id_(g_print_preview_ui_id_map.Get().Add(this)),
-      handler_(nullptr),
-      source_is_modifiable_(true),
-      source_has_selection_(false),
-      print_selection_only_(false),
-      dialog_closed_(false) {
-  handler_ = handler.get();
+      handler_(handler.get()) {
   web_ui->AddMessageHandler(std::move(handler));
 
   g_print_preview_request_id_map.Get().Set(id_, -1);
@@ -580,11 +582,7 @@ PrintPreviewUI::PrintPreviewUI(content::WebUI* web_ui)
     : ConstrainedWebDialogUI(web_ui),
       initial_preview_start_time_(base::TimeTicks::Now()),
       id_(g_print_preview_ui_id_map.Get().Add(this)),
-      handler_(nullptr),
-      source_is_modifiable_(true),
-      source_has_selection_(false),
-      print_selection_only_(false),
-      dialog_closed_(false) {
+      handler_(CreatePrintPreviewHandlers(web_ui)) {
   // Set up the chrome://print/ data source.
   Profile* profile = Profile::FromWebUI(web_ui);
 
@@ -601,11 +599,6 @@ PrintPreviewUI::PrintPreviewUI(content::WebUI* web_ui)
   // Set up the chrome://theme/ source.
   content::URLDataSource::Add(profile, new ThemeSource(profile));
 
-  auto handler = std::make_unique<PrintPreviewHandler>();
-  handler_ = handler.get();
-  web_ui->AddMessageHandler(std::move(handler));
-  web_ui->AddMessageHandler(std::make_unique<MetricsHandler>());
-
   g_print_preview_request_id_map.Get().Set(id_, -1);
 }
 
@@ -651,15 +644,11 @@ void PrintPreviewUI::SetInitialParams(
 }
 
 // static
-void PrintPreviewUI::GetCurrentPrintPreviewStatus(
-    const PrintHostMsg_PreviewIds& ids,
-    bool* cancel) {
+bool PrintPreviewUI::ShouldCancelRequest(const PrintHostMsg_PreviewIds& ids) {
   int current_id = -1;
-  if (!g_print_preview_request_id_map.Get().Get(ids.ui_id, &current_id)) {
-    *cancel = true;
-    return;
-  }
-  *cancel = (ids.request_id != current_id);
+  if (!g_print_preview_request_id_map.Get().Get(ids.ui_id, &current_id))
+    return true;
+  return ids.request_id != current_id;
 }
 
 int32_t PrintPreviewUI::GetIDForPrintPreviewUI() const {
@@ -823,7 +812,7 @@ void PrintPreviewUI::SetDelegateForTesting(TestingDelegate* delegate) {
 }
 
 void PrintPreviewUI::SetSelectedFileForTesting(const base::FilePath& path) {
-  handler_->FileSelectedForTesting(path, 0, NULL);
+  handler_->FileSelectedForTesting(path, 0, nullptr);
 }
 
 void PrintPreviewUI::SetPdfSavedClosureForTesting(

chrome/browser/ui/webui/print_preview/print_preview_ui.h

@@ -15,7 +15,6 @@
 #include "base/memory/ref_counted.h"
 #include "base/time/time.h"
 #include "chrome/browser/ui/webui/constrained_web_dialog_ui.h"
-#include "printing/buildflags/buildflags.h"
 
 class PrintPreviewHandler;
 struct PrintHostMsg_DidGetPreviewPageCount_Params;
@@ -76,10 +75,9 @@ class PrintPreviewUI : public ConstrainedWebDialogUI {
       const PrintHostMsg_RequestPrintPreview_Params& params);
 
   // Determines whether to cancel a print preview request based on the request
-  // and ui ids in |ids|.
+  // and UI ids in |ids|.
   // Can be called from any thread.
-  static void GetCurrentPrintPreviewStatus(const PrintHostMsg_PreviewIds& ids,
-                                           bool* cancel);
+  static bool ShouldCancelRequest(const PrintHostMsg_PreviewIds& ids);
 
   // Returns an id to uniquely identify this PrintPreviewUI.
   int32_t GetIDForPrintPreviewUI() const;
@@ -184,24 +182,24 @@ class PrintPreviewUI : public ConstrainedWebDialogUI {
   const int32_t id_;
 
   // Weak pointer to the WebUI handler.
-  PrintPreviewHandler* handler_;
+  PrintPreviewHandler* const handler_;
 
   // Indicates whether the source document can be modified.
-  bool source_is_modifiable_;
+  bool source_is_modifiable_ = true;
 
   // Indicates whether the source document has selection.
-  bool source_has_selection_;
+  bool source_has_selection_ = false;
 
   // Indicates whether only the selection should be printed.
-  bool print_selection_only_;
+  bool print_selection_only_ = false;
+
+  // Keeps track of whether OnClosePrintPreviewDialog() has been called or not.
+  bool dialog_closed_ = false;
 
   // Store the initiator title, used for populating the print preview dialog
   // title.
   base::string16 initiator_title_;
 
-  // Keeps track of whether OnClosePrintPreviewDialog() has been called or not.
-  bool dialog_closed_;
-
   DISALLOW_COPY_AND_ASSIGN(PrintPreviewUI);
 };
 

chrome/browser/ui/webui/print_preview/print_preview_ui_unittest.cc

@@ -175,7 +175,7 @@ TEST_F(PrintPreviewUIUnitTest, PrintPreviewDraftPages) {
 }
 
 // Test the browser-side print preview cancellation functionality.
-TEST_F(PrintPreviewUIUnitTest, GetCurrentPrintPreviewStatus) {
+TEST_F(PrintPreviewUIUnitTest, ShouldCancelRequest) {
   WebContents* initiator = browser()->tab_strip_model()->GetActiveWebContents();
   ASSERT_TRUE(initiator);
 
@@ -196,38 +196,21 @@ TEST_F(PrintPreviewUIUnitTest, GetCurrentPrintPreviewStatus) {
       preview_dialog->GetWebUI()->GetController());
   ASSERT_TRUE(preview_ui);
 
-  // Test with invalid |preview_ui_addr|.
-  bool cancel = false;
+  // Test with invalid UI ID.
   const int32_t kInvalidId = -5;
-  preview_ui->GetCurrentPrintPreviewStatus(
-      PrintHostMsg_PreviewIds(0, kInvalidId), &cancel);
-  EXPECT_TRUE(cancel);
+  EXPECT_TRUE(preview_ui->ShouldCancelRequest({0, kInvalidId}));
 
   const int kFirstRequestId = 1000;
   const int kSecondRequestId = 1001;
-  const int32_t preview_ui_addr = preview_ui->GetIDForPrintPreviewUI();
+  const int32_t preview_id = preview_ui->GetIDForPrintPreviewUI();
 
   // Test with kFirstRequestId.
   preview_ui->OnPrintPreviewRequest(kFirstRequestId);
-  cancel = true;
-  preview_ui->GetCurrentPrintPreviewStatus(
-      PrintHostMsg_PreviewIds(kFirstRequestId, preview_ui_addr), &cancel);
-  EXPECT_FALSE(cancel);
-
-  cancel = false;
-  preview_ui->GetCurrentPrintPreviewStatus(
-      PrintHostMsg_PreviewIds(kSecondRequestId, preview_ui_addr), &cancel);
-  EXPECT_TRUE(cancel);
+  EXPECT_FALSE(preview_ui->ShouldCancelRequest({kFirstRequestId, preview_id}));
+  EXPECT_TRUE(preview_ui->ShouldCancelRequest({kSecondRequestId, preview_id}));
 
   // Test with kSecondRequestId.
   preview_ui->OnPrintPreviewRequest(kSecondRequestId);
-  cancel = false;
-  preview_ui->GetCurrentPrintPreviewStatus(
-      PrintHostMsg_PreviewIds(kFirstRequestId, preview_ui_addr), &cancel);
-  EXPECT_TRUE(cancel);
-
-  cancel = true;
-  preview_ui->GetCurrentPrintPreviewStatus(
-      PrintHostMsg_PreviewIds(kSecondRequestId, preview_ui_addr), &cancel);
-  EXPECT_FALSE(cancel);
+  EXPECT_TRUE(preview_ui->ShouldCancelRequest({kFirstRequestId, preview_id}));
+  EXPECT_FALSE(preview_ui->ShouldCancelRequest({kSecondRequestId, preview_id}));
 }