Sync 3rd party wallpaper app name

chrome/browser/chromeos/extensions/wallpaper_private_api.cc

Index: chrome/browser/chromeos/extensions/wallpaper_private_api.cc
diff --git a/chrome/browser/chromeos/extensions/wallpaper_private_api.cc b/chrome/browser/chromeos/extensions/wallpaper_private_api.cc
index 9da0e4275e618a1a9d88f71a9d5c53e8f3d727d7..6350a7f746afe9ae546da7a1e1263294071ec384 100644
--- a/chrome/browser/chromeos/extensions/wallpaper_private_api.cc
+++ b/chrome/browser/chromeos/extensions/wallpaper_private_api.cc
@@ -68,6 +68,10 @@ 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);
@@ -634,11 +638,13 @@ void WallpaperPrivateSetCustomWallpaperFunction::OnWallpaperDecoded(
   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();