Fix WindowdotPrintShouldBringUpPrintPreview

chrome/browser/apps/app_browsertest.cc

 // Copyright 2013 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 <stddef.h>
 
 #include <memory>
 
 #include "apps/launcher.h"
 #include "base/bind.h"
@@ skipping 24 matching lines @@
 #include "chrome/test/base/test_switches.h"
 #include "chrome/test/base/ui_test_utils.h"
 #include "components/pref_registry/pref_registry_syncable.h"
 #include "components/prefs/pref_service.h"
 #include "components/web_modal/web_contents_modal_dialog_manager.h"
 #include "content/public/browser/devtools_agent_host.h"
 #include "content/public/browser/host_zoom_map.h"
 #include "content/public/browser/render_process_host.h"
 #include "content/public/browser/render_widget_host_view.h"
 #include "content/public/test/browser_test_utils.h"
 #include "content/public/test/test_utils.h"

[tapted, 2017/02/09 01:10:02]

is one or both of these content..test_utils.h headers now unused with
content::MessageLoopRunner gone?

[rbpotter, 2017/02/09 01:56:40]

Done.

 #include "extensions/browser/app_window/app_window.h"
 #include "extensions/browser/app_window/app_window_registry.h"
 #include "extensions/browser/app_window/native_app_window.h"
 #include "extensions/browser/event_router.h"
 #include "extensions/browser/extension_prefs.h"
 #include "extensions/browser/extension_registry.h"
 #include "extensions/browser/extension_registry_observer.h"
 #include "extensions/browser/pref_names.h"
 #include "extensions/common/api/app_runtime.h"
 #include "extensions/common/constants.h"
@@ skipping 84 matching lines @@
   // PrintPreviewUI::TestingDelegate implementation.
   void DidGetPreviewPageCount(int page_count) override {
     total_page_count_ = page_count;
   }
 
   // PrintPreviewUI::TestingDelegate implementation.
   void DidRenderPreviewPage(content::WebContents* preview_dialog) override {
     dialog_size_ = preview_dialog->GetContainerBounds().size();
     ++rendered_page_count_;
     CHECK(rendered_page_count_ <= total_page_count_);
-    if (waiting_runner_.get() && rendered_page_count_ == total_page_count_) {
-      waiting_runner_->Quit();
+    if (rendered_page_count_ == total_page_count_ && run_loop_)  {
+      run_loop_->Quit();
     }
   }
 
   void WaitUntilPreviewIsReady() {
-    CHECK(!waiting_runner_.get());
+    CHECK(!run_loop_);

[tapted, 2017/02/09 01:10:02]

This is no longer being cleared after removing `waiting_runner_ = NULL;` - was
that intentional?. I think a neater way to do this is to declare run_loop_ as a
raw pointer.

base::RunLoop* run_loop_ = nullptr;

then this method is

void WaitUntilPreviewIsReady() {
  if (rendered_page_count_ >= total_page_count_)
    return;

  base::RunLoop run_loop;
  base::AutoReset<RunLoop> auto_reset(&run_loop_, &run_loop);
  run_loop.Run();
}


(this restructure is an optional nit though - I don't have strong feelings -
that's just how I'd write it ;)

[rbpotter, 2017/02/09 01:56:40]

Done. Thanks for the suggestion.

     if (rendered_page_count_ < total_page_count_) {
-      waiting_runner_ = new content::MessageLoopRunner;
-      waiting_runner_->Run();
-      waiting_runner_ = NULL;
+      run_loop_ = base::MakeUnique<base::RunLoop>();
+      run_loop_->Run();
     }
   }
 
   gfx::Size dialog_size() {
     return dialog_size_;
   }
 
  private:
   bool auto_cancel_;
   int total_page_count_;
   int rendered_page_count_;
-  scoped_refptr<content::MessageLoopRunner> waiting_runner_;
+  std::unique_ptr<base::RunLoop> run_loop_;
   gfx::Size dialog_size_;
 };
 
 #endif  // ENABLE_PRINT_PREVIEW
 
 #if !defined(OS_CHROMEOS) && !defined(OS_WIN)
 bool CopyTestDataAndGetTestFilePath(
     const base::FilePath& test_data_file,
     const base::FilePath& temp_dir,
     const char* filename,
@@ skipping 948 matching lines @@
 
   EXPECT_EQ(1LU, GetAppWindowCount());
   EXPECT_TRUE(GetFirstAppWindow()
                   ->web_contents()
                   ->GetRenderWidgetHostView()
                   ->HasFocus());
 }
 
 #if BUILDFLAG(ENABLE_PRINT_PREVIEW)
 
-#if defined(OS_WIN) || defined(OS_LINUX) || defined(OS_MACOSX)
-#define MAYBE_WindowDotPrintShouldBringUpPrintPreview \
-    DISABLED_WindowDotPrintShouldBringUpPrintPreview
-#else
-#define MAYBE_WindowDotPrintShouldBringUpPrintPreview \
-    WindowDotPrintShouldBringUpPrintPreview
-#endif
-
 IN_PROC_BROWSER_TEST_F(PlatformAppBrowserTest,
-                       MAYBE_WindowDotPrintShouldBringUpPrintPreview) {
-  ScopedPreviewTestingDelegate preview_delegate(true);
+                       WindowDotPrintShouldBringUpPrintPreview) {
+  ScopedPreviewTestingDelegate preview_delegate(false);

[tapted, 2017/02/09 01:10:02]

in the bug you say this was the only test passing |true|, so can the bool
argument just be removed?

[rbpotter, 2017/02/09 01:56:40]

Done.

   ASSERT_TRUE(RunPlatformAppTest("platform_apps/print_api")) << message_;
   preview_delegate.WaitUntilPreviewIsReady();
 }
 
 // This test verifies that http://crbug.com/297179 is fixed.
 IN_PROC_BROWSER_TEST_F(PlatformAppBrowserTest,
                        DISABLED_ClosingWindowWhilePrintingShouldNotCrash) {
   ScopedPreviewTestingDelegate preview_delegate(false);
   ASSERT_TRUE(RunPlatformAppTest("platform_apps/print_api")) << message_;
   preview_delegate.WaitUntilPreviewIsReady();
@@ skipping 217 matching lines @@
 // https://crbug.com/620194.
 #define MAYBE_AppWindowIframe DISABLED_AppWindowIframe
 // Sends chrome.test.sendMessage from chrome.app.window.create's callback.
 // The app window also adds an <iframe> to the page during window.onload.
 IN_PROC_BROWSER_TEST_F(PlatformAppBrowserTest, MAYBE_AppWindowIframe) {
   LoadAndLaunchPlatformApp("app_window_send_message",
                            "APP_WINDOW_CREATE_CALLBACK");
 }
 
 }  // namespace extensions