Screenshot effect non-obvious

chrome/browser/ui/ash/screenshot_taker.h

Index: chrome/browser/ui/ash/screenshot_taker.h
diff --git a/chrome/browser/ui/ash/screenshot_taker.h b/chrome/browser/ui/ash/screenshot_taker.h
index 6ba0416277b03db6dd6c6429a6725b2c03b38091..db9bfd75c74da69b0468688d082da30f6f098e84 100644
--- a/chrome/browser/ui/ash/screenshot_taker.h
+++ b/chrome/browser/ui/ash/screenshot_taker.h
@@ -8,13 +8,19 @@
 #include "ash/screenshot_delegate.h"
 #include "base/basictypes.h"
 #include "base/compiler_specific.h"
+#include "base/memory/weak_ptr.h"
+#include "base/string16.h"
 #include "base/time.h"
-#include "ui/compositor/layer.h"
+#include "ui/gfx/image/image.h"
 
 namespace aura {
 class Window;
 }  // namespace aura
 
+namespace base {
+class FilePath;
+} // namespace base
+
 class ScreenshotTaker : public ash::ScreenshotDelegate {
  public:
   ScreenshotTaker();
@@ -26,20 +32,33 @@ class ScreenshotTaker : public ash::ScreenshotDelegate {
                                            const gfx::Rect& rect) OVERRIDE;
   virtual bool CanTakeScreenshot() OVERRIDE;
 
- private:
-  // Flashes the screen to provide visual feedback that a screenshot has
-  // been taken.
-  void DisplayVisualFeedback(const gfx::Rect& rect);
+  // Show notification that screenshot was taken or failed.
+  void ShowNotification(bool success, const base::FilePath& screenshot_path);
 
-  // Closes the visual feedback layer.
-  void CloseVisualFeedbackLayer();
+ private:
+  // WeakPtr factory.

[James Cook, 2013/03/26 23:54:25]

Better to explain why you need weak pointers at all.

[sschmitz, 2013/03/27 14:58:49]

Removed.

+  base::WeakPtrFactory<ScreenshotTaker> factory_;
 
   // 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_;
+  // Notification icon.

[James Cook, 2013/03/26 23:54:25]

This comment doesn't add anything - the variable name is enough.

[sschmitz, 2013/03/27 14:58:49]

Removed

+  gfx::Image notification_icon_;
+
+  // Notificaton identifier.
+  int notification_id_;
+
+  // Notification title text when successful.

[James Cook, 2013/03/26 23:54:25]

Likewise, none of these comments add anything. I might just use a single comment
like "// Text appearing in notifications." for all 4 members.

[sschmitz, 2013/03/27 14:58:49]

Removed

+  string16 notification_title_success_;
+
+  // Notification title text when a failure occurred.
+  string16 notification_title_fail_;
+
+  // Notification body text when successful.
+  string16 notification_text_success_;
+
+  // Notification body text when a failure occurred.
+  string16 notification_text_fail_;
 
   DISALLOW_COPY_AND_ASSIGN(ScreenshotTaker);
 };