If customization includes default wallpaper, download and apply it.

chrome/browser/chromeos/customization_document.cc

Index: chrome/browser/chromeos/customization_document.cc
diff --git a/chrome/browser/chromeos/customization_document.cc b/chrome/browser/chromeos/customization_document.cc
index 99d7d82bff3edf4999734377bcece14d89c73016..443c47aa7f2f0a4199668b3ac92ee398123dec17 100644
--- a/chrome/browser/chromeos/customization_document.cc
+++ b/chrome/browser/chromeos/customization_document.cc
@@ -12,6 +12,7 @@
 #include "base/logging.h"
 #include "base/memory/weak_ptr.h"
 #include "base/metrics/histogram.h"
+#include "base/path_service.h"
 #include "base/prefs/pref_registry_simple.h"
 #include "base/prefs/pref_service.h"
 #include "base/strings/string_split.h"
@@ -20,6 +21,8 @@
 #include "base/strings/utf_string_conversions.h"
 #include "base/time/time.h"
 #include "chrome/browser/browser_process.h"
+#include "chrome/browser/chromeos/customization_wallpaper_downloader.h"
+#include "chrome/browser/chromeos/login/wallpaper_manager.h"
 #include "chrome/browser/chromeos/login/wizard_controller.h"
 #include "chrome/browser/chromeos/net/delay_network_call.h"
 #include "chrome/browser/extensions/external_loader.h"
@@ -27,7 +30,9 @@
 #include "chrome/browser/profiles/profile.h"
 #include "chrome/browser/ui/app_list/app_list_syncable_service.h"
 #include "chrome/browser/ui/app_list/app_list_syncable_service_factory.h"
+#include "chrome/common/chrome_paths.h"
 #include "chrome/common/extensions/extension_constants.h"
+#include "chrome/common/pref_names.h"
 #include "chromeos/network/network_state.h"
 #include "chromeos/network/network_state_handler.h"
 #include "chromeos/system/statistics_provider.h"
@@ -64,6 +69,12 @@ const char kAcceptedManifestVersion[] = "1.0";
 const char kStartupCustomizationManifestPath[] =
     "/opt/oem/etc/startup_manifest.json";
 
+const char kCustomizationDefaultWallpaperDir[] = "customization/";

[Dmitry Polukhin, 2014/03/27 04:36:32]

Please add comment, also please avoid injecting '/', use  AppendRelativePath
instead.

[Alexander Alekseev, 2014/03/31 14:15:40]

Done.

+
+// The original downloaded image file is stored under this name.
+const char kCustomizationDefaultWallpaperDownloadedFile[] =
+    "default.downloaded";
+
 // Name of local state option that tracks if services customization has been
 // applied.
 const char kServicesCustomizationAppliedPref[] = "ServicesCustomizationApplied";
@@ -123,6 +134,12 @@ std::string GetLocaleSpecificStringImpl(
   return std::string();
 }
 
+void CheckWallpaperCacheExists(const base::FilePath& path, bool* exists) {
+  DCHECK(!content::BrowserThread::CurrentlyOn(content::BrowserThread::UI));
+  DCHECK(exists);
+  *exists = base::PathExists(path);
+}
+
 }  // anonymous namespace
 
 namespace chromeos {
@@ -356,6 +373,7 @@ ServicesCustomizationDocument* ServicesCustomizationDocument::GetInstance() {
 void ServicesCustomizationDocument::RegisterPrefs(
     PrefRegistrySimple* registry) {
   registry->RegisterBooleanPref(kServicesCustomizationAppliedPref, false);
+  registry->RegisterStringPref(prefs::kCustomizationDefaultWallpaperURL, "");

[Dmitry Polukhin, 2014/03/27 04:36:32]

I'm bit worry about keeping URL to download file. It could be attack vector to
replace this URL with some malicious URL in local settings (potential cross user
infection path). I think we should re-download customization manifest if fail
was not download and saved to disk at once. We download customization manifest
using HTPS from Google server so it is more reliable sources than URL in local
settings.

[Alexander Alekseev, 2014/03/31 14:15:40]

This URL is never fetched. It's used as a flag that image by this URL has beed
cached. It's only compared to the URL from manifest.

 }
 
 // static
@@ -468,6 +486,8 @@ void ServicesCustomizationDocument::OnManifestLoaded() {
   if (!ServicesCustomizationDocument::WasOOBECustomizationApplied())
     ApplyOOBECustomization();
 
+  CheckAndApplyWallpaper();

[Dmitry Polukhin, 2014/03/27 04:36:32]

Why do you need this??? Wallpaper should be applied only once per machine. But
this code path will be called for every user on the machine.

[Alexander Alekseev, 2014/03/31 14:15:40]

Done.

+
   scoped_ptr<base::DictionaryValue> prefs =
       GetDefaultAppsInProviderFormat(*root_);
   for (ExternalLoaders::iterator it = external_loaders_.begin();
@@ -516,8 +536,134 @@ void ServicesCustomizationDocument::OnURLFetchComplete(
   fetch_started_ = false;
 }
 
+void ServicesCustomizationDocument::StartOEMWallpaperDownload(

[Daniel Erat, 2014/03/27 01:45:50]

please make the order of the methods match between .h and .cc files; the
implementation is difficult to navigate otherwise

[Alexander Alekseev, 2014/03/31 14:15:40]

Done.

+    const GURL& wallpaper_url) {
+  DCHECK(wallpaper_url.is_valid());
+
+  const base::FilePath dir = GetCustomizedWallpaperCacheDir();
+  const base::FilePath file = GetCustomizedWallpaperDownloadedFileName();
+  if (!dir.empty() && !file.empty()) {
+    wallpaper_downloader_.reset(new CustomizationWallpaperDownloader(
+        g_browser_process->system_request_context(), wallpaper_url, dir, file));
+
+    wallpaper_downloader_->Start();
+  }
+}
+
+// static
+base::FilePath ServicesCustomizationDocument::GetCustomizedWallpaperCacheDir() {
+  base::FilePath custom_wallpaper_dir;
+  const bool success = PathService::Get(chrome::DIR_CHROMEOS_CUSTOM_WALLPAPERS,
+                                        &custom_wallpaper_dir);
+  DCHECK(success);
+
+  if (success)
+    return custom_wallpaper_dir.Append(kCustomizationDefaultWallpaperDir);
+
+  return custom_wallpaper_dir;
+}
+
+// static
+base::FilePath
+    ServicesCustomizationDocument::GetCustomizedWallpaperDownloadedFileName() {
+  const base::FilePath dir = GetCustomizedWallpaperCacheDir();
+  if (dir.empty())
+    return dir;
+  return dir.Append(kCustomizationDefaultWallpaperDownloadedFile);
+}
+
+// static
+void ServicesCustomizationDocument::SetDefaultWallpaperPath(
+    const GURL& wallpaper_url) {

[Daniel Erat, 2014/03/27 01:45:50]

why does this need to be a static method?

[Alexander Alekseev, 2014/03/31 14:15:40]

It just doesn't require access to object. It doesn't need to be static.
Do you think I should still make it "normal" method?

[Daniel Erat, 2014/03/31 14:22:15]

if it doesn't need to be accessed outside of the class, just make it into a
function in the anonymous namespace. this makes the header file smaller and is
easier to maintain (since you don't need to maintain a separate method signature
in the header).

[Alexander Alekseev, 2014/03/31 15:39:15]

It's called from both this class and CustomizationWallpaperDownloader.

+  DCHECK(wallpaper_url.is_valid());
+
+  VLOG(1) << "Setting default wallpaper to '"
+          << GetCustomizedWallpaperDownloadedFileName().value() << "' ('"
+          << wallpaper_url.spec() << "')";
+  WallpaperManager::Get()->SetCustomizedDefaultWallpaper(
+      wallpaper_url,
+      GetCustomizedWallpaperDownloadedFileName(),
+      GetCustomizedWallpaperCacheDir());
+}
+
+// static
+void ServicesCustomizationDocument::DestroyWallpaperDownloader() {
+  ServicesCustomizationDocument* self =
+      ServicesCustomizationDocument::GetInstance();

[Daniel Erat, 2014/03/27 01:45:50]

why does this need to be a static method? can't the caller just call
GetInstance() itself?

[Dmitry Polukhin, 2014/03/27 04:36:32]

I think we need to use weak pointers that comes from parameter here. Otherwise
we might face crashes on browser shutdown when ServicesCustomizationDocument is
destroyed.

[Alexander Alekseev, 2014/03/31 14:15:40]

Done.

[Alexander Alekseev, 2014/03/31 14:15:40]

I don't see the way to crash here, as Singleton will be reinitialized on Get().
Also, CustomizedWallpaperDownloader is owned by ServicedCustomizationDocument,
so it should be destroyed before owner.

+  DCHECK(self);
+  self->wallpaper_downloader_.reset();
+}
+
+void ServicesCustomizationDocument::ApplyWallpaper(
+    bool default_wallpaper_file_exists) {
+  GURL wallpaper_url = GetDefaultWallpaperUrl();
+  DCHECK(wallpaper_url.is_valid());

[Dmitry Polukhin, 2014/03/27 04:36:32]

Could you please add checks for expect thread for each method that is called
with PostTask?

[Alexander Alekseev, 2014/03/31 14:15:40]

All of them already have these checks. ApplyWallpaper is never called with
PostTask.

+
+  PrefService* pref_service = g_browser_process->local_state();
+
+  std::string current_url =
+      pref_service->GetString(prefs::kCustomizationDefaultWallpaperURL);
+  if (current_url != wallpaper_url.spec()) {
+    VLOG(1) << "ServicesCustomizationDocument::ApplyWallpaper() : "
+            << "Wallpaper URL in customization document '"
+            << wallpaper_url.spec() << "' differs from current '" << current_url
+            << "'."
+            << (GURL(current_url).is_valid() && default_wallpaper_file_exists
+                    ? " Ignored."
+                    : " Will refetch.");
+  }
+  // Never update system-wide wallpaper (i.e. do not check
+  // current_url == wallpaper_url.spec() )
+  if (GURL(current_url).is_valid() && default_wallpaper_file_exists) {
+    VLOG(1)
+        << "ServicesCustomizationDocument::ApplyWallpaper() : reuse existing";
+    SetDefaultWallpaperPath(GURL(current_url));
+  } else {
+    VLOG(1)
+        << "ServicesCustomizationDocument::ApplyWallpaper() : start download";
+    StartOEMWallpaperDownload(wallpaper_url);
+  }
+}
+
+void ServicesCustomizationDocument::OnCheckedWallpaperCacheExists(
+    scoped_ptr<bool> exists) {
+  DCHECK(content::BrowserThread::CurrentlyOn(content::BrowserThread::UI));
+  DCHECK(exists.get());
+
+  ApplyWallpaper(*exists);
+}
+
+void ServicesCustomizationDocument::CheckAndApplyWallpaper() {
+  if (wallpaper_downloader_.get()) {
+    VLOG(1) << "CheckAndApplyWallpaper(): download has already started.";
+    return;
+  }
+  GURL wallpaper_url = GetDefaultWallpaperUrl();
+  // Should fail if this ever happens in tests.
+  DCHECK(wallpaper_url.is_valid() || wallpaper_url.is_empty());
+  if (!wallpaper_url.is_valid()) {
+    if (!wallpaper_url.is_empty()) {
+      LOG(WARNING) << "Invalid Customized Wallpaper URL.";
+    }
+    return;
+  }
+  scoped_ptr<bool> exists(new bool(false));

[Dmitry Polukhin, 2014/03/27 04:36:32]

I don't like this approach to me it would be cleaner to pass callback to
callback that will be called if file exists. scoped_ptr<bool> looks very
suspicious.

[Alexander Alekseev, 2014/03/31 14:15:40]

This approach guarantees that WeakPointer never leaves UI thread, so it will be
destroyed on UI thread. Passing callback with WeakPointer to another thread
means it would be once destroyed there.

I can do what you want, but would you accept current approach instead? 

+
+  base::Closure check_file_exists =
+      base::Bind(&CheckWallpaperCacheExists,
+                 GetCustomizedWallpaperDownloadedFileName(),
+                 base::Unretained(exists.get()));
+  base::Closure on_checked_closure =
+      base::Bind(&ServicesCustomizationDocument::OnCheckedWallpaperCacheExists,
+                 weak_ptr_factory_.GetWeakPtr(),
+                 base::Passed(exists.Pass()));
+  if (!content::BrowserThread::PostBlockingPoolTaskAndReply(
+          FROM_HERE, check_file_exists, on_checked_closure)) {
+    LOG(WARNING) << "Failed to start check Wallpaper cache exists.";
+  }
+}
+
 bool ServicesCustomizationDocument::ApplyOOBECustomization() {
-  // TODO(dpolukhin): apply default wallpaper, crbug.com/348136.
   SetApplied(true);
   return true;
 }