Brushes up the code of screenshot filename.

Currently it just checks the filename duplication and adding numbers (1),(2),... in
case of duplication.  But this doesn't work well if the file will be stored into GoogleDrive,
since file operations are not atomic there.

This CL does the followings:
- the screenshot taker accepts "taking a screenshot for each root window" requests
  rather than taking a screenshot for the specified window
- adding the number in case of multiple root-windows, in screenshot taker side
- change the screenshot interval from 500msec -> 1sec, to avoid the duplication
- remove the existing filename duplication checking code, which is no longer necessary

BUG=140749

Review URL: https://chromiumcodereview.appspot.com/10827193

git-svn-id: svn://svn.chromium.org/chrome/trunk/src@150758 0039d316-1c4b-4281-b951-d872f2087c98

ash/accelerators/accelerator_controller.cc

@@ -441,9 +441,7 @@ bool AcceleratorController::PerformAction(int action,
     case TAKE_SCREENSHOT_BY_PRTSCN_KEY:
       if (screenshot_delegate_.get() &&
           screenshot_delegate_->CanTakeScreenshot()) {
-        Shell::RootWindowList root_windows = Shell::GetAllRootWindows();
-        for (size_t i = 0; i < root_windows.size(); ++i)
-          screenshot_delegate_->HandleTakeScreenshot(root_windows[i]);
+        screenshot_delegate_->HandleTakeScreenshotForAllRootWindows();
       }
       // Return true to prevent propagation of the key event.
       return true;

ash/accelerators/accelerator_controller_unittest.cc

@@ -70,9 +70,8 @@ class DummyScreenshotDelegate : public ScreenshotDelegate {
   virtual ~DummyScreenshotDelegate() {}
 
   // Overridden from ScreenshotDelegate:
-  virtual void HandleTakeScreenshot(aura::Window* window) OVERRIDE {
-    if (window != NULL)
-      ++handle_take_screenshot_count_;
+  virtual void HandleTakeScreenshotForAllRootWindows() OVERRIDE {
+    ++handle_take_screenshot_count_;
   }
 
   virtual void HandleTakePartialScreenshot(

ash/accelerators/accelerator_filter_unittest.cc

@@ -30,9 +30,8 @@ class DummyScreenshotDelegate : public ScreenshotDelegate {
   virtual ~DummyScreenshotDelegate() {}
 
   // Overridden from ScreenshotDelegate:
-  virtual void HandleTakeScreenshot(aura::Window* window) OVERRIDE {
-    if (window != NULL)
-      ++handle_take_screenshot_count_;
+  virtual void HandleTakeScreenshotForAllRootWindows() OVERRIDE {
+    ++handle_take_screenshot_count_;
   }
 
   virtual void HandleTakePartialScreenshot(

ash/screenshot_delegate.h

@@ -20,10 +20,9 @@ class ScreenshotDelegate {
  public:
   virtual ~ScreenshotDelegate() {}
 
-  // The actual task of taking a screenshot for the given window.
-  // This method is called when the user wants to take a screenshot
-  // manually.
-  virtual void HandleTakeScreenshot(aura::Window* window) = 0;
+  // The actual task of taking a screenshot for each root window.
+  // This method is called when the user wants to take a screenshot manually.
+  virtual void HandleTakeScreenshotForAllRootWindows() = 0;
 
   // The actual task of taking a partial screenshot for the given
   // window.

chrome/browser/ui/ash/screenshot_taker.cc

@@ -35,7 +35,16 @@
 #endif
 
 namespace {
-const int kScreenshotMinimumIntervalInMS = 500;
+// How opaque should the layer that we flash onscreen to provide visual
+// feedback after the screenshot is taken be?
+const float kVisualFeedbackLayerOpacity = 0.25f;
+
+// How long should the visual feedback layer be displayed?
+const int64 kVisualFeedbackLayerDisplayTimeMs = 100;
+
+// The minimum interval between two screenshot commands.  It has to be
+// more than 1000 to prevent the conflict of filenames.
+const int kScreenshotMinimumIntervalInMS = 1000;
 
 bool ShouldUse24HourClock() {
 #if defined(OS_CHROMEOS)
@@ -50,7 +59,12 @@ bool ShouldUse24HourClock() {
   return base::GetHourClockType() == base::k24HourClock;
 }
 
-std::string GetScreenShotBaseFilename(bool use_24hour_clock) {
+bool AreScreenshotsDisabled() {
+  return g_browser_process->local_state()->GetBoolean(
+      prefs::kDisableScreenshots);
+}
+
+std::string GetScreenshotBaseFilename() {
   base::Time::Exploded now;
   base::Time::Now().LocalExplode(&now);
 
@@ -61,7 +75,7 @@ std::string GetScreenShotBaseFilename(bool use_24hour_clock) {
   std::string file_name = base::StringPrintf(
       "Screenshot %d-%02d-%02d at ", now.year, now.month, now.day_of_month);
 
-  if (use_24hour_clock) {
+  if (ShouldUse24HourClock()) {
     file_name.append(base::StringPrintf(
         "%02d.%02d.%02d", now.hour, now.minute, now.second));
   } else {
@@ -79,25 +93,32 @@ std::string GetScreenShotBaseFilename(bool use_24hour_clock) {
   return file_name;
 }
 
-FilePath GetScreenshotPath(const FilePath& base_directory,
-                           const std::string& base_name) {
-  DCHECK(content::BrowserThread::CurrentlyOn(content::BrowserThread::FILE));
-  for (int retry = 0; retry < INT_MAX; retry++) {
-    std::string retry_suffix;
-    if (retry > 0)
-      retry_suffix = base::StringPrintf(" (%d)", retry + 1);
+bool GetScreenshotDirectory(FilePath* directory) {
+  if (AreScreenshotsDisabled())
+    return false;
 
-    FilePath file_path = base_directory.AppendASCII(
-        base_name + retry_suffix + ".png");
-    if (!file_util::PathExists(file_path))
-      return file_path;
+  bool is_logged_in = true;
+#if defined(OS_CHROMEOS)
+  is_logged_in = chromeos::UserManager::Get()->IsUserLoggedIn();
+#endif
+
+  if (is_logged_in) {
+    DownloadPrefs* download_prefs = DownloadPrefs::FromBrowserContext(
+        ash::Shell::GetInstance()->delegate()->GetCurrentBrowserContext());
+    *directory = download_prefs->DownloadPath();
+  } else {
+    if (!file_util::GetTempDir(directory)) {
+      LOG(ERROR) << "Failed to find temporary directory.";
+      return false;
+    }
   }
-  return FilePath();
+  return true;
 }
 
-void SaveScreenshotToLocalFile(scoped_refptr<base::RefCountedBytes> png_data,
-                               const FilePath& screenshot_path) {
+void SaveScreenshot(const FilePath& screenshot_path,
+                    scoped_refptr<base::RefCountedBytes> png_data) {
   DCHECK(content::BrowserThread::CurrentlyOn(content::BrowserThread::FILE));
+  DCHECK(!screenshot_path.empty());
   if (static_cast<size_t>(file_util::WriteFile(
           screenshot_path,
           reinterpret_cast<char*>(&(png_data->data()[0])),
@@ -106,17 +127,6 @@ void SaveScreenshotToLocalFile(scoped_refptr<base::RefCountedBytes> png_data,
   }
 }
 
-void SaveScreenshot(const FilePath& screenshot_directory,
-                    const std::string& base_name,
-                    scoped_refptr<base::RefCountedBytes> png_data) {
-  FilePath screenshot_path = GetScreenshotPath(screenshot_directory, base_name);
-  if (screenshot_path.empty()) {
-    LOG(ERROR) << "Failed to find a screenshot file name.";
-    return;
-  }
-  SaveScreenshotToLocalFile(png_data, screenshot_path);
-}
-
 // TODO(kinaba): crbug.com/140425, remove this ungly #ifdef dispatch.
 #ifdef OS_CHROMEOS
 void SaveScreenshotToGData(scoped_refptr<base::RefCountedBytes> png_data,
@@ -126,43 +136,34 @@ void SaveScreenshotToGData(scoped_refptr<base::RefCountedBytes> png_data,
     LOG(ERROR) << "Failed to write screenshot image to Google Drive: " << error;
     return;
   }
-  SaveScreenshotToLocalFile(png_data, local_path);
+  SaveScreenshot(local_path, png_data);
 }
 
-void PostSaveScreenshotTask(const FilePath& screenshot_directory,
-                            const std::string& base_name,
+void PostSaveScreenshotTask(const FilePath& screenshot_path,
                             scoped_refptr<base::RefCountedBytes> png_data) {
-  if (gdata::util::IsUnderGDataMountPoint(screenshot_directory)) {
+  if (gdata::util::IsUnderGDataMountPoint(screenshot_path)) {
     Profile* profile = ProfileManager::GetDefaultProfileOrOffTheRecord();
     if (profile) {
-      // TODO(kinaba,mukai): crbug.com/140749. Take care of the case
-      // "base_name.png" already exists.
       gdata::util::PrepareWritableFileAndRun(
           profile,
-          screenshot_directory.Append(base_name + ".png"),
+          screenshot_path,
           base::Bind(&SaveScreenshotToGData, png_data));
     }
   } else {
     content::BrowserThread::PostTask(
         content::BrowserThread::FILE, FROM_HERE,
-        base::Bind(&SaveScreenshot, screenshot_directory, base_name, png_data));
+        base::Bind(&SaveScreenshot, screenshot_path, png_data));
   }
 }
 #else
-void PostSaveScreenshotTask(const FilePath& screenshot_directory,
-                            const std::string& base_name,
+void PostSaveScreenshotTask(const FilePath& screenshot_path,
                             scoped_refptr<base::RefCountedBytes> png_data) {
   content::BrowserThread::PostTask(
       content::BrowserThread::FILE, FROM_HERE,
-      base::Bind(&SaveScreenshot, screenshot_directory, base_name, png_data));
+      base::Bind(&SaveScreenshot, screenshot_path, png_data));
 }
 #endif
 
-bool AreScreenshotsDisabled() {
-  return g_browser_process->local_state()->GetBoolean(
-      prefs::kDisableScreenshots);
-}
-
 bool GrabWindowSnapshot(aura::Window* window,
                         const gfx::Rect& snapshot_bounds,
                         std::vector<unsigned char>* png_data) {
@@ -182,13 +183,6 @@ bool GrabWindowSnapshot(aura::Window* window,
   return chrome::GrabWindowSnapshotForUser(window, png_data, snapshot_bounds);
 }
 
-// How opaque should the layer that we flash onscreen to provide visual
-// feedback after the screenshot is taken be?
-const float kVisualFeedbackLayerOpacity = 0.25f;
-
-// How long should the visual feedback layer be displayed?
-const int64 kVisualFeedbackLayerDisplayTimeMs = 100;
-
 }  // namespace
 
 ScreenshotTaker::ScreenshotTaker() {
@@ -197,42 +191,47 @@ ScreenshotTaker::ScreenshotTaker() {
 ScreenshotTaker::~ScreenshotTaker() {
 }
 
-void ScreenshotTaker::HandleTakeScreenshot(aura::Window* window) {
-  HandleTakePartialScreenshot(window, window->bounds());
+void ScreenshotTaker::HandleTakeScreenshotForAllRootWindows() {
+  FilePath screenshot_directory;
+  if (!GetScreenshotDirectory(&screenshot_directory))
+    return;
+
+  std::string screenshot_basename = GetScreenshotBaseFilename();
+  ash::Shell::RootWindowList root_windows = ash::Shell::GetAllRootWindows();
+  for (size_t i = 0; i < root_windows.size(); ++i) {
+    aura::RootWindow* root_window = root_windows[i];
+    scoped_refptr<base::RefCountedBytes> png_data(new base::RefCountedBytes);
+    std::string basename = screenshot_basename;
+    gfx::Rect rect = root_window->bounds();
+    if (root_windows.size() > 1)
+      basename += base::StringPrintf(" - Display %d", static_cast<int>(i + 1));
+    if (GrabWindowSnapshot(root_window, rect, &png_data->data())) {
+      DisplayVisualFeedback(rect);
+      PostSaveScreenshotTask(
+          screenshot_directory.AppendASCII(basename + ".png"), png_data);
+    } else {
+      LOG(ERROR) << "Failed to grab the window screenshot for " << i;
+    }
+  }
+  last_screenshot_timestamp_ = base::Time::Now();
 }
 
 void ScreenshotTaker::HandleTakePartialScreenshot(
     aura::Window* window, const gfx::Rect& rect) {
   DCHECK(content::BrowserThread::CurrentlyOn(content::BrowserThread::UI));
 
-  if (AreScreenshotsDisabled())
+  FilePath screenshot_directory;
+  if (!GetScreenshotDirectory(&screenshot_directory))
     return;
 
   scoped_refptr<base::RefCountedBytes> png_data(new base::RefCountedBytes);
 
-  bool is_logged_in = true;
-#if defined(OS_CHROMEOS)
-  is_logged_in = chromeos::UserManager::Get()->IsUserLoggedIn();
-#endif
-
-  FilePath screenshot_directory;
-  if (is_logged_in) {
-    DownloadPrefs* download_prefs = DownloadPrefs::FromBrowserContext(
-        ash::Shell::GetInstance()->delegate()->GetCurrentBrowserContext());
-    screenshot_directory = download_prefs->DownloadPath();
-  } else {
-    if (!file_util::GetTempDir(&screenshot_directory)) {
-      LOG(ERROR) << "Failed to find temporary directory.";
-      return;
-    }
-  }
-
   if (GrabWindowSnapshot(window, rect, &png_data->data())) {
     last_screenshot_timestamp_ = base::Time::Now();
     DisplayVisualFeedback(rect);
-    PostSaveScreenshotTask(screenshot_directory,
-                           GetScreenShotBaseFilename(ShouldUse24HourClock()),
-                           png_data);
+    PostSaveScreenshotTask(
+        screenshot_directory.AppendASCII(GetScreenshotBaseFilename() + ".png"),
+        png_data);
   } else {
     LOG(ERROR) << "Failed to grab the window screenshot";
   }

chrome/browser/ui/ash/screenshot_taker.h

@@ -21,7 +21,7 @@ class ScreenshotTaker : public ash::ScreenshotDelegate {
   virtual ~ScreenshotTaker();
 
   // Overridden from ash::ScreenshotDelegate:
-  virtual void HandleTakeScreenshot(aura::Window* window) OVERRIDE;
+  virtual void HandleTakeScreenshotForAllRootWindows() OVERRIDE;
   virtual void HandleTakePartialScreenshot(aura::Window* window,
                                            const gfx::Rect& rect) OVERRIDE;
   virtual bool CanTakeScreenshot() OVERRIDE;