Refactor screenshot directory source

chrome/browser/ui/webui/screenshot_source.cc

Index: chrome/browser/ui/webui/screenshot_source.cc
diff --git a/chrome/browser/ui/webui/screenshot_source.cc b/chrome/browser/ui/webui/screenshot_source.cc
index f4ba7ada5c43e78f74972f5fdf347a0f77a99ea9..e5876d0b7ea49cea2f0f80df28f17b98e482af85 100644
--- a/chrome/browser/ui/webui/screenshot_source.cc
+++ b/chrome/browser/ui/webui/screenshot_source.cc
@@ -6,31 +6,60 @@
 
 #include "base/bind.h"
 #include "base/callback.h"
+#include "base/file_path.h"
 #include "base/file_util.h"
+#include "base/i18n/time_formatting.h"
 #include "base/memory/ref_counted_memory.h"
 #include "base/message_loop.h"
 #include "base/path_service.h"
 #include "base/string16.h"
+#include "base/stringprintf.h"
 #include "base/string_util.h"
+#include "chrome/browser/browser_process.h"
 #include "chrome/browser/download/download_prefs.h"
+#include "chrome/browser/prefs/pref_service.h"
+#include "chrome/browser/profiles/profile.h"
+#include "chrome/browser/profiles/profile_manager.h"
 #include "chrome/common/chrome_paths.h"
+#include "chrome/common/pref_names.h"
 #include "chrome/common/url_constants.h"
 #include "googleurl/src/url_canon.h"
 #include "googleurl/src/url_util.h"
 
-#if defined(OS_CHROMEOS)
+#if defined(USE_ASH)
 #include "ash/shell.h"
 #include "ash/shell_delegate.h"
+#endif
+
+#if defined(OS_CHROMEOS)
 #include "chrome/browser/chromeos/gdata/drive_file_system_interface.h"
 #include "chrome/browser/chromeos/gdata/drive_file_system_util.h"
 #include "chrome/browser/chromeos/gdata/drive_system_service.h"
+#include "chrome/browser/chromeos/gdata/gdata_util.h"
+#include "chrome/browser/chromeos/login/user_manager.h"
 #include "content/public/browser/browser_thread.h"
 #endif
 
-static const char kCurrentScreenshotFilename[] = "current";
+const char ScreenshotSource::kScreenshotUrlRoot[] = "chrome://screenshots/";

[sky, 2012/09/12 04:32:01]

For statics like this we typically put:
// static
on the previous line.

[Harry McCleave, 2012/09/13 03:05:39]

Done.

+const char ScreenshotSource::kScreenshotCurrent[] = "current";
+const char ScreenshotSource::kScreenshotSaved[] = "saved/";
 #if defined(OS_CHROMEOS)
-static const char kSavedScreenshotsBasePath[] = "saved/";
+const char ScreenshotSource::kScreenshotPrefix[] = "Screenshot ";
+const char ScreenshotSource::kScreenshotSuffix[] = ".png";
+#endif
+
+bool ShouldUse24HourClock() {
+#if defined(OS_CHROMEOS)
+  Profile* profile = ProfileManager::GetDefaultProfileOrOffTheRecord();
+  if (profile) {
+    PrefService* pref_service = profile->GetPrefs();

[sky, 2012/09/12 04:32:01]

Do you really need the NULL check here?

[Harry McCleave, 2012/09/13 03:05:39]

I am not sure, the profile interface does not specify that the return must not
be null, though the only implementation of it does return a non-null value for
all code paths, the safe/conservative side of me says to make no assumptions
based on the implementation, what is the general practice for chromium on such
things?

[sky, 2012/09/13 15:57:31]

Only have null checks if it can be null. Look at other code. I believe you
shouldn't have the null check here.

+    if (pref_service) {

[sky, 2012/09/12 04:32:01]

remove {}

[Harry McCleave, 2012/09/13 03:05:39]

Done.

+      return pref_service->GetBoolean(prefs::kUse24HourClock);
+    }
+  }
 #endif
+  return base::GetHourClockType() == base::k24HourClock;
+}
 
 ScreenshotSource::ScreenshotSource(
     std::vector<unsigned char>* current_screenshot,
@@ -46,6 +75,67 @@ ScreenshotSource::ScreenshotSource(
 
 ScreenshotSource::~ScreenshotSource() {}
 
+std::string ScreenshotSource::GetScreenshotBaseFilename() {

[sky, 2012/09/12 04:32:01]

static methods also get:
// static
on the previous line

[Harry McCleave, 2012/09/13 03:05:39]

Done.

+  base::Time::Exploded now;
+  base::Time::Now().LocalExplode(&now);
+
+  // We don't use base/i18n/time_formatting.h here because it doesn't
+  // support our format.  Don't use ICU either to avoid i18n file names
+  // for non-English locales.
+  // TODO(mukai): integrate this logic somewhere time_formatting.h
+  std::string file_name = base::StringPrintf(
+      "Screenshot %d-%02d-%02d at ", now.year, now.month, now.day_of_month);
+
+  if (ShouldUse24HourClock()) {
+    file_name.append(base::StringPrintf(
+        "%02d.%02d.%02d", now.hour, now.minute, now.second));
+  } else {
+    int hour = now.hour;
+    if (hour > 12) {
+      hour -= 12;
+    } else if (hour == 0) {
+      hour = 12;
+    }
+    file_name.append(base::StringPrintf(
+        "%d.%02d.%02d ", hour, now.minute, now.second));
+    file_name.append((now.hour >= 12) ? "PM" : "AM");
+  }
+
+  return file_name;
+}
+
+#if defined(USE_ASH)
+
+bool ScreenshotSource::AreScreenshotsDisabled() {
+  return g_browser_process->local_state()->GetBoolean(
+      prefs::kDisableScreenshots);
+}
+
+bool ScreenshotSource::GetScreenshotDirectory(FilePath* directory) {
+  if (ScreenshotSource::AreScreenshotsDisabled())
+    return false;
+
+  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 true;
+}
+
+#endif
+
 void ScreenshotSource::StartDataRequest(const std::string& path, bool,
                                         int request_id) {
   SendScreenshot(path, request_id);
@@ -75,14 +165,15 @@ void ScreenshotSource::SendScreenshot(const std::string& screenshot_path,
   // image gets reloaded instead of being pulled from the browser cache
   std::string path = screenshot_path.substr(
       0, screenshot_path.find_first_of("?"));
-  if (path == kCurrentScreenshotFilename) {
+  if (path == ScreenshotSource::kScreenshotCurrent) {
     CacheAndSendScreenshot(path, request_id, current_screenshot_);
 #if defined(OS_CHROMEOS)
-  } else if (path.compare(0, strlen(kSavedScreenshotsBasePath),
-                          kSavedScreenshotsBasePath) == 0) {
+  } else if (path.compare(0, strlen(ScreenshotSource::kScreenshotSaved),
+             ScreenshotSource::kScreenshotSaved) == 0) {
     using content::BrowserThread;
 
-    std::string filename = path.substr(strlen(kSavedScreenshotsBasePath));
+    std::string filename =
+                       path.substr(strlen(ScreenshotSource::kScreenshotSaved));
 
     url_canon::RawCanonOutputT<char16> decoded;
     url_util::DecodeURLEscapeSequences(
@@ -91,9 +182,8 @@ void ScreenshotSource::SendScreenshot(const std::string& screenshot_path,
     std::string decoded_filename = UTF16ToASCII(string16(
         decoded.data(), decoded.length()));
 
-    DownloadPrefs* download_prefs = DownloadPrefs::FromBrowserContext(
-        ash::Shell::GetInstance()->delegate()->GetCurrentBrowserContext());
-    FilePath download_path = download_prefs->DownloadPath();
+    FilePath download_path;
+    GetScreenshotDirectory(&download_path);
     if (gdata::util::IsUnderDriveMountPoint(download_path)) {
       gdata::DriveFileSystemInterface* file_system =
           gdata::DriveSystemServiceFactory::GetForProfile(