Screenshot effect non-obvious

chrome/browser/ui/ash/screenshot_taker.h

 // Copyright (c) 2012 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.
 
 #ifndef CHROME_BROWSER_UI_ASH_SCREENSHOT_TAKER_H_
 #define CHROME_BROWSER_UI_ASH_SCREENSHOT_TAKER_H_
 
 #include "ash/screenshot_delegate.h"
 #include "base/basictypes.h"
 #include "base/compiler_specific.h"
+#include "base/memory/scoped_ptr.h"
 #include "base/time.h"
-#include "ui/compositor/layer.h"
 
 namespace aura {
 class Window;
 }  // namespace aura
 
+namespace base {
+class FilePath;
+}  // namespace base
+
+class ScreenshotTakerObserver {
+ public:
+  enum Result {
+    SCREENSHOT_SUCCESS = 0,
+    SCREENSHOT_GRABWINDOW_PARTIAL_FAILED,
+    SCREENSHOT_GRABWINDOW_FULL_FAILED,
+    SCREENSHOT_CREATE_DIR_FAILED,
+    SCREENSHOT_GET_DIR_FAILED,
+    SCREENSHOT_CHECK_DIR_FAILED,
+    SCREENSHOT_CREATE_FILE_FAILED,
+    SCREENSHOT_WRITE_FILE_FAILED,
+    SCREENSHOT_RESULT_COUNT
+  };
+
+  virtual ~ScreenshotTakerObserver() {}
+
+  virtual void OnScreenshotCompleted(
+      Result screenshot_result, const base::FilePath& screenshot_path) = 0;
+};
+
 class ScreenshotTaker : public ash::ScreenshotDelegate {
  public:
   ScreenshotTaker();
   virtual ~ScreenshotTaker();
 
   // Overridden from ash::ScreenshotDelegate:
   virtual void HandleTakeScreenshotForAllRootWindows() OVERRIDE;
   virtual void HandleTakePartialScreenshot(aura::Window* window,
                                            const gfx::Rect& rect) OVERRIDE;
   virtual bool CanTakeScreenshot() OVERRIDE;
 
+  // For test we override the screenshot directory for cleanup.
+  void SetScreenshotDirectoryForTest(const base::FilePath& directory);
+
+  // For test we observe the completion of a screenshot.
+  static void SetObserverForTest(ScreenshotTakerObserver* observer);
+  static ScreenshotTakerObserver* GetObserverForTest();

[Jun Mukai, 2013/03/29 23:01:37]

move them to private methods and declare friend class the test class, for
safety?

[sschmitz, 2013/03/29 23:40:44]

I think there would be a problem, because friendship is not inherited and the
actual test is derived from ScreenshotTakerTest in the IN_PROC_BROWSER_TEST_F
macro. And I'd rather not declare friendship to
class ScreenshotTakerTest_TakeScreenshot_Test as well.

[Jun Mukai, 2013/03/30 00:37:17]

You can call the private methods from SetUp() of your friend class
ScreenshotTakerTest.  Or if you want to allow it in each IN_PROC_BROWSER_TEST_F,
you can declare some utility methods in ScreenshotTakerTest to do so, and make
them protected.

[sschmitz, 2013/04/02 21:23:25]

I did what you suggested. Made the override functions (renamed) private and the
test a friend. I only access these functions in the test base class as you
suggested.
Done. 

+
  private:
-  // Flashes the screen to provide visual feedback that a screenshot has
-  // been taken.
-  void DisplayVisualFeedback(const gfx::Rect& rect);
-
-  // Closes the visual feedback layer.
-  void CloseVisualFeedbackLayer();
-
   // The timestamp when the screenshot task was issued last time.
   base::Time last_screenshot_timestamp_;
 
-  // The flashing effect of the screen for the visual feedback when taking a
-  // screenshot.
-  scoped_ptr<ui::Layer> visual_feedback_layer_;
+  scoped_ptr<base::FilePath> screenshot_directory_for_test_;

[James Cook, 2013/03/29 22:49:34]

I don't think this needs to be a pointer - you should just be able to use
base::FilePath and check for empty.

[sschmitz, 2013/03/29 23:40:44]

Done.

+
+  static ScreenshotTakerObserver* observer_for_test_;

[Jun Mukai, 2013/03/29 23:01:37]

why static?  I don't think your test needs to create multiple screenshot taker
instance, so just a simple member would be fine.

[sschmitz, 2013/03/29 23:40:44]

The reason for static is to avoid passing a weak pointer to the ScreenshotTaker
through ~5 namespace (non-member) functions. (I had implemented that earlier but
reverted to a simpler solution).

[Jun Mukai, 2013/03/30 00:37:17]

I still personally prefer non-static solution but it's up to you.

[sschmitz, 2013/04/02 21:23:25]

Yes. Good point. I removed all statics in favor of members. The namespace
non-member fcts pass a weak ptr to the "final" function can be a member fct.
Done.

 
   DISALLOW_COPY_AND_ASSIGN(ScreenshotTaker);
 };
 
 #endif  // CHROME_BROWSER_UI_ASH_SCREENSHOT_TAKER_H_