cheets: add wallpaper observer to arc_wallpaper_service.

chrome/browser/chromeos/arc/arc_wallpaper_service.cc

Index: chrome/browser/chromeos/arc/arc_wallpaper_service.cc
diff --git a/chrome/browser/chromeos/arc/arc_wallpaper_service.cc b/chrome/browser/chromeos/arc/arc_wallpaper_service.cc
index f6a12cdb5a1eaba07ac09ec039de27a4aba85917..df8deb2a8fca73267ce9425637b89d6a8eed438e 100644
--- a/chrome/browser/chromeos/arc/arc_wallpaper_service.cc
+++ b/chrome/browser/chromeos/arc/arc_wallpaper_service.cc
@@ -31,6 +31,7 @@ namespace arc {
 
 namespace {
 
+constexpr uint32_t kMinOnWallpaperChangedVersion = 1;
 constexpr char kAndroidWallpaperFilename[] = "android.jpg";
 
 // Sets a decoded bitmap as the wallpaper.
@@ -81,6 +82,7 @@ ArcWallpaperService::~ArcWallpaperService() {
   // Make sure the callback is never called after destruction. It is safe to
   // call Cancel() even when there is no in-flight request.
   ImageDecoder::Cancel(this);
+  ash::WmShell::Get()->wallpaper_controller()->RemoveObserver(this);

[Yusuke Sato, 2016/09/16 00:45:19]

You need null check(s). See the trybot (browser_test) failure.

[Muyuan, 2016/09/16 17:35:16]

Done.

   arc_bridge_service()->wallpaper()->RemoveObserver(this);
 }
 
@@ -94,6 +96,7 @@ void ArcWallpaperService::OnInstanceReady() {
     return;
   }
   wallpaper_instance->Init(binding_.CreateInterfacePtrAndBind());
+  ash::WmShell::Get()->wallpaper_controller()->AddObserver(this);
 }
 
 void ArcWallpaperService::SetWallpaper(mojo::Array<uint8_t> png_data) {
@@ -122,4 +125,22 @@ void ArcWallpaperService::OnDecodeImageFailed() {
   LOG(ERROR) << "Failed to decode wallpaper image.";
 }
 
+void ArcWallpaperService::OnWallpaperDataChanged() {
+  DCHECK_CURRENTLY_ON(content::BrowserThread::UI);
+  if (!CheckWallpaperInstanceVersion(kMinOnWallpaperChangedVersion)) {
+    return;
+  }
+  arc_bridge_service()->wallpaper()->instance()->OnWallpaperChanged();

[Luis Héctor Chávez, 2016/09/16 00:01:58]

what if instance() is null? It's probably better to follow the pattern found
elsewhere:

https://cs.chromium.org/chromium/src/components/arc/intent_helper/arc_intent_...

(although you probably don't need the error code part, nor the part where you
check for ArcBridgeService's existence, since you are guaranteed that it will
exist by virtue of being an ArcService).

[Muyuan, 2016/09/16 16:30:05]

The observer is registered in OnInstanceReady, so can I assume that when
OnWallpaperChanged is called, the instance will never be null?

[Luis Héctor Chávez, 2016/09/16 16:34:02]

You can't assume that: What happens if the other side closes the connection
afterwards?

However, if you also add the OnInstanceClosed() override and unregister the
wallpaper observer there, you _can_ assume that.

Still, the point about following the pattern found elsewhere stands.

[Muyuan, 2016/09/16 17:35:16]

Done.

[Muyuan, 2016/09/16 17:37:22]

Should I remove the wallpaper observer when the instance is null and hope it
will be re-registered in OnInstanceReady?

[Luis Héctor Chávez, 2016/09/16 17:49:14]

So what I'm saying is:

a) Change CheckWallpaperInstanceVersion to be mojom::WallpaperInstance*
GetWallpaperInstance(uint32_t min_version). This is for consistency with other
services that do the same. I'll probably make this a feature of the
InstanceHolder given that everybody seems to need it. I'll try to post that
change for review later, so you can wait for me to land it if you want.
b) Additionally you should remove the observer in the OnInstanceClosed(). This
is to avoid getting notifications when you can't do anything about it and avoid
spurious logs.

[Muyuan, 2016/09/16 19:52:07]

Done.

+}
+
+bool ArcWallpaperService::CheckWallpaperInstanceVersion(uint32_t version_need) {
+  uint32_t version = arc_bridge_service()->wallpaper()->version();
+  if (version >= version_need) {
+    return true;
+  }
+  LOG(WARNING) << "Wallpaper instance is too old (version " << version
+               << ") need version " << version_need;
+  return false;
+}
+
 }  // namespace arc