Sync 3rd party wallpaper app name

chrome/browser/chromeos/extensions/wallpaper_private_api.cc

 // Copyright (c) 2013 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.
 
 #include "chrome/browser/chromeos/extensions/wallpaper_private_api.h"
 
 #include <map>
 #include <set>
 #include <string>
 #include <utility>
@@ skipping 50 matching lines @@
 namespace get_offline_wallpaper_list =
     wallpaper_private::GetOfflineWallpaperList;
 
 namespace {
 
 #if defined(GOOGLE_CHROME_BUILD)
 const char kWallpaperManifestBaseURL[] =
     "https://storage.googleapis.com/chromeos-wallpaper-public/manifest_";
 #endif
 
+// Keep this value the same with the corresponding constant defined in
+// chrome/browser/resources/chromeos/wallpaper_manager/js/constants.js.
+const char kThirdPartyWallpaperPrefix[] = "third_party_";
+
 bool IsOEMDefaultWallpaper() {
   return base::CommandLine::ForCurrentProcess()->HasSwitch(
       chromeos::switches::kDefaultWallpaperIsOem);
 }
 
 // Saves |data| as |file_name| to directory with |key|. Return false if the
 // directory can not be found/created or failed to write file.
 bool SaveData(int key,
               const std::string& file_name,
               const std::vector<char>& data) {
@@ skipping 546 matching lines @@
 
   bool update_wallpaper =
       account_id_ ==
       user_manager::UserManager::Get()->GetActiveUser()->GetAccountId();
   wallpaper_manager->SetCustomWallpaper(
       account_id_, user_id_hash_, params->file_name, layout,
       user_manager::User::CUSTOMIZED, image, update_wallpaper);
   unsafe_wallpaper_decoder_ = NULL;
 
   Profile* profile = Profile::FromBrowserContext(browser_context());
-  // This API is only available to the component wallpaper picker. We do not
-  // need to show the app's name if it is the component wallpaper picker. So set
-  // the pref to empty string.
-  profile->GetPrefs()->SetString(prefs::kCurrentWallpaperAppName,
-                                 std::string());
+  // This API is only available to the component wallpaper picker. It can be
+  // called to set a custom wallpaper or sync a third party wallpaper. We need
+  // to set the pref according to different types of wallpapers.

[tbarzic, 2016/01/26 01:20:44]

I assume this prefs should be set by third-party app using wallpaper API. Can
you clarify that in the comment (set perf according to type does not give too
much information :) )

[xdai1, 2016/01/26 17:57:48]

Currently the 3rd party wallpaper syncing scenario works like this: suppose we
have two device A and B. On device A, a third party wallpaper was set by a third
party wallpaper app by calling wallpaper API, and yes during this call this pref
was set. The corresponding third party wallpaper was then saved into the
built-in wallpaper picker app's sandboxed sync filesystem.
Then on device B, the built-in wallpaper picker app listened to the filesystem
change. If there was any change, chrome.storage.onChanged was triggered to set
device B's wallpaper from sync filesystem by calling wallpaper PRIVATE API (more
specific, chrome.wallpaperPRIVATE.setCustomWallpaper() API, as seen in this
file).

chrome.wallpaperPRIVATE.setCustomWallpaper() API can also be called when a user
manually set up a custom wallpaper. The naming for custom wallpapers and third
party wallpapers are different, and that's the way we tell these two different
types of wallpapers apart.

[tbarzic, 2016/01/26 18:57:26]

Yep, I assumed as much. What I'd like is for the comment here to say that app
name should not be overwritten here because it already has the correct value,
which was set using public wallpaper API (and synced using preferences).

Though, I wonder if syncing wallpaper information using two mechanisms is the
way to go here (sync storage and preferences). Can we add app name to params and
update profile pref based on that (and leave the preference type as it was)?
Otherwise, there will be a window in which info stored in the preferences won't
be consistent with the actual state of the wallpaper manager.

+  if (params->file_name.find(kThirdPartyWallpaperPrefix) == std::string::npos) {

[tbarzic, 2016/01/26 01:20:44]

Can this logic be moved to wallpaper app?
E.g. add an optional bool parameter to params (like clearAppName, or
isThirdPartyWallpaper), or even appName to set?

[xdai1, 2016/01/26 17:57:48]

Yes, it can be moved to wallpaper app by introducing another parameter like
isThirdPartyWallpaper, but it seems a little redundant since we still use the
file name to check if the current wallpaper is a custom wallpaper or a third
party wallpaper... Since the file name has all the information we need, the
additional parameter seems a little unnecessary? WDYT?

[tbarzic, 2016/01/26 18:57:26]

Yes, but I would prefer not to leak file name format from wallpaper manager app
into the API. Adding a flag for clearing app name, or app name to parameters
(see the other comment) makes the API cleaner. The fact that the app name can be
derived from the file name is implementation detail of the app, and the API
should not depend on that.

+    profile->GetPrefs()->SetString(prefs::kCurrentWallpaperAppName,
+                                   std::string());
+  }
 
   if (params->generate_thumbnail) {
     image.EnsureRepsForSupportedScales();
     scoped_ptr<gfx::ImageSkia> deep_copy(image.DeepCopy());
     // Generates thumbnail before call api function callback. We can then
     // request thumbnail in the javascript callback.
     task_runner->PostTask(FROM_HERE,
         base::Bind(
             &WallpaperPrivateSetCustomWallpaperFunction::GenerateThumbnail,
             this, thumbnail_path, base::Passed(&deep_copy)));
@@ skipping 275 matching lines @@
                  this, file_list));
 }
 
 void WallpaperPrivateGetOfflineWallpaperListFunction::OnComplete(
     const std::vector<std::string>& file_list) {
   base::ListValue* results = new base::ListValue();
   results->AppendStrings(file_list);
   SetResult(results);
   SendResponse(true);
 }