Print Preview: Fix failure to save with long page title

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

+// Copyright 2017 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#include "chrome/browser/ui/webui/print_preview/print_preview_handler.h"
+
+#include <Windows.h>

[Lei Zhang, 2017/04/19 21:14:07]

lowercase

[rbpotter, 2017/04/19 23:30:12]

Done.

+#include <commdlg.h>
+
+#include "base/auto_reset.h"
+#include "base/run_loop.h"
+#include "base/timer/timer.h"
+#include "chrome/browser/platform_util.h"
+#include "chrome/browser/printing/print_preview_dialog_controller.h"
+#include "chrome/browser/printing/print_preview_test.h"
+#include "chrome/browser/printing/print_view_manager.h"
+#include "chrome/browser/ui/browser_commands.h"
+#include "chrome/browser/ui/browser_tabstrip.h"
+#include "chrome/browser/ui/tabs/tab_strip_model.h"
+#include "chrome/browser/ui/webui/print_preview/print_preview_ui.h"
+#include "chrome/test/base/browser_with_test_window_test.h"
+#include "components/web_modal/web_contents_modal_dialog_manager.h"
+#include "content/public/browser/plugin_service.h"
+#include "content/public/browser/site_instance.h"
+#include "content/public/browser/web_contents.h"
+#include "ui/shell_dialogs/select_file_dialog_win.h"
+
+using content::WebContents;
+using web_modal::WebContentsModalDialogManager;
+
+namespace {
+bool init_called = false;

[Lei Zhang, 2017/04/19 21:14:07]

Can we avoid these global variables by doing the following?

- Make them FakePrintPreviewHandler members.
- Bind() a pointer to the FakePrintPreviewHandler as the first argument to
GetSaveFileNameImpl()
- Then stick into the OPENFILENAME struct's lCustData field.
- For WM_INITDIALOG, |wParam| is the same struct.

[rbpotter, 2017/04/19 23:30:13]

Done.

+base::RunLoop* run_loop;
+
+}  // namespace
+
+// Defined here as Windows crashes if this is defined in a class or a
+// namespace.
+UINT_PTR CALLBACK PrintPreviewHandlerTestHookFunction(HWND hdlg,
+                                                      UINT uiMsg,

[Lei Zhang, 2017/04/19 21:14:07]

message, wparam, lparam

[rbpotter, 2017/04/19 23:30:13]

Done.

+                                                      WPARAM wParam,
+                                                      LPARAM lParam) {
+  if (uiMsg != WM_INITDIALOG)
+    return 0;
+  init_called = true;
+  PostMessage(hdlg, WM_COMMAND, IDABORT, 0);
+  if (run_loop)
+    run_loop->Quit();
+  return 1;
+}
+
+namespace {
+bool GetOpenFileNameImpl(OPENFILENAME* ofn) {
+  return ::GetOpenFileName(ofn);
+}
+
+bool GetSaveFileNameImpl(OPENFILENAME* ofn) {
+  // Modify ofn so that the hook function will be called.
+  ofn->Flags |= OFN_ENABLEHOOK;
+  ofn->lpfnHook = PrintPreviewHandlerTestHookFunction;
+  return ::GetSaveFileName(ofn);
+}
+
+class FakePrintPreviewHandler : public PrintPreviewHandler {
+ public:
+  explicit FakePrintPreviewHandler(content::WebUI* web_ui)
+      : save_failed_(false) {
+    set_web_ui(web_ui);
+  }
+
+  void FileSelected(const base::FilePath& path,
+                    int index,
+                    void* params) override {
+    save_failed_ = false;
+    if (run_loop)
+      run_loop->Quit();

[Lei Zhang, 2017/04/19 21:14:07]

As is, would this end up calling Quit() twice?

[rbpotter, 2017/04/19 23:30:13]

Only one of FileSelected or FileSelectionCanceled should ever be
called, and these are now the only places that call Quit().

+  }
+
+  void FileSelectionCanceled(void* params) override {
+    save_failed_ = true;
+    if (run_loop)
+      run_loop->Quit();
+  }
+
+  void StartPrintToPdf() {
+    PrintToPdf();
+    if (!run_loop && !init_called) {
+      base::RunLoop new_run_loop;

[Lei Zhang, 2017/04/19 21:14:08]

If the RunLoop becomes a member variable, can we get rid of this?

[rbpotter, 2017/04/19 23:30:12]

Done.

+      base::AutoReset<base::RunLoop*> auto_reset(&run_loop, &new_run_loop);
+      new_run_loop.Run();
+    }
+  }
+
+  bool save_failed() { return save_failed_; }
+
+ protected:

[Lei Zhang, 2017/04/19 21:14:08]

private?

[rbpotter, 2017/04/19 23:30:12]

Done.

+  // Simplified version of select file to avoid checking preferences and sticky
+  // settings in the test
+  void SelectFile(const base::FilePath& default_filename,
+                  bool prompt_user) override {
+    ui::SelectFileDialog::FileTypeInfo file_type_info;
+    file_type_info.extensions.resize(1);
+    file_type_info.extensions[0].push_back(FILE_PATH_LITERAL("pdf"));
+    select_file_dialog_ = ui::CreateWinSelectFileDialog(
+        this, nullptr /*policy already checked*/,
+        base::Bind(GetOpenFileNameImpl), base::Bind(GetSaveFileNameImpl));
+    select_file_dialog_->SelectFile(
+        ui::SelectFileDialog::SELECT_SAVEAS_FILE, base::string16(),
+        default_filename, &file_type_info, 0, base::FilePath::StringType(),
+        platform_util::GetTopLevel(web_ui()->GetWebContents()->GetNativeView()),
+        NULL);
+  }
+
+ private:
+  bool save_failed_;
+};
+}  // namespace
+
+class PrintPreviewHandlerTest : public PrintPreviewTest {
+ public:
+  PrintPreviewHandlerTest() {}

[Lei Zhang, 2017/04/19 21:14:08]

Use braces or default consistently between the ctor/dtor.

[rbpotter, 2017/04/19 23:30:13]

Done.

+  ~PrintPreviewHandlerTest() override = default;
+
+  void SetUp() override {
+    PrintPreviewTest::SetUp();
+
+    // Create a new tab

[Lei Zhang, 2017/04/19 21:14:08]

Not sure if you need to do this. I think the browser starts with 1 tab already.

[rbpotter, 2017/04/19 23:30:13]

Does not start with a tab - tried removing this and could not get the
initiator web contents.

[Lei Zhang, 2017/04/20 00:12:30]

OK, I remembered wrong then.

+    chrome::NewTab(browser());
+
+    // Initialize state
+    init_called = false;
+    run_loop = nullptr;
+  }
+
+ protected:
+  void CreateUiAndHandler() {
+    WebContents* initiator =
+        browser()->tab_strip_model()->GetActiveWebContents();
+    ASSERT_TRUE(initiator);
+
+    // Get print preview UI
+    printing::PrintPreviewDialogController* controller =
+        printing::PrintPreviewDialogController::GetInstance();
+    ASSERT_TRUE(controller);
+    printing::PrintViewManager* print_view_manager =
+        printing::PrintViewManager::FromWebContents(initiator);
+    print_view_manager->PrintPreviewNow(initiator->GetMainFrame(), false);
+    WebContents* preview_dialog =
+        controller->GetOrCreatePreviewDialog(initiator);
+    ASSERT_TRUE(preview_dialog);
+    preview_ui_ = static_cast<PrintPreviewUI*>(
+        preview_dialog->GetWebUI()->GetController());
+    ASSERT_TRUE(preview_ui_);
+
+    // Initialize |preview_handler_|

[Lei Zhang, 2017/04/19 21:14:07]

Probably not needed.

[rbpotter, 2017/04/19 23:30:13]

Done.

+    preview_handler_.reset(

[Lei Zhang, 2017/04/19 21:14:08]

Can we use foo = base::MakeUnique<Foo>() instead of foo.reset(new Foo()) ?

[rbpotter, 2017/04/19 23:30:13]

Done.

+        new FakePrintPreviewHandler(preview_dialog->GetWebUI()));
+  }
+
+  std::unique_ptr<FakePrintPreviewHandler> preview_handler_;
+  PrintPreviewUI* preview_ui_;

[Lei Zhang, 2017/04/19 21:14:07]

Initialize this variable, or maybe just drop it and have CreateUiAndHandler()
return it. It's not used for anything else.

[rbpotter, 2017/04/19 23:30:13]

Done.

+
+ private:
+  DISALLOW_COPY_AND_ASSIGN(PrintPreviewHandlerTest);
+};
+
+TEST_F(PrintPreviewHandlerTest, TestSaveAsPdf) {
+  CreateUiAndHandler();
+  preview_ui_->SetInitiatorTitle(L"111111111111111111111.html");
+  preview_handler_->StartPrintToPdf();
+  ASSERT_FALSE(!init_called && preview_handler_->save_failed());

[Lei Zhang, 2017/04/19 21:14:07]

So this is the same as the following, right?

ASSERT_TRUE(init_called || !preview_handler_->save_failed());

Should it actually check the following?

EXPECT_TRUE(init_called);
EXPECT_FALSE(preview_handler_->save_failed());

[rbpotter, 2017/04/19 23:30:13]

Done.

+}
+
+TEST_F(PrintPreviewHandlerTest, TestSaveAsPdfLongFileName) {
+  CreateUiAndHandler();
+  preview_ui_->SetInitiatorTitle(
+      L"11111111111111111111111111111111111111111111111111111111111111111111111"
+      L"11111111111111111111111111111111111111111111111111111111111111111111111"
+      L"11111111111111111111111111111111111111111111111111111111111111111111111"
+      L"11111111111111111111111111111111111111111111111111111111111111111111111"
+      L"1111111111111111111111111111111111111111111111111.html");
+  preview_handler_->StartPrintToPdf();
+  ASSERT_FALSE(!init_called && preview_handler_->save_failed());
+}