cheets: add wallpaper observer to arc_wallpaper_service.

chrome/browser/chromeos/arc/arc_wallpaper_service.cc

 // Copyright 2016 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/arc/arc_wallpaper_service.h"
 
 #include <string>
 #include <utility>
 #include <vector>
 
@@ skipping 13 matching lines @@
 #include "ui/gfx/image/image.h"
 #include "ui/gfx/image/image_skia.h"
 #include "ui/gfx/image/image_util.h"
 
 using user_manager::UserManager;
 
 namespace arc {
 
 namespace {
 
+constexpr uint32_t kMinOnWallpaperChangedVersion = 1;
 constexpr char kAndroidWallpaperFilename[] = "android.jpg";
 
 // Sets a decoded bitmap as the wallpaper.
 void SetBitmapAsWallpaper(const SkBitmap& bitmap) {
   // Make the SkBitmap immutable as we won't modify it. This is important
   // because otherwise it gets duplicated during painting, wasting memory.
   SkBitmap immutable_bitmap(bitmap);
   immutable_bitmap.setImmutable();
   gfx::ImageSkia image = gfx::ImageSkia::CreateFrom1xBitmap(immutable_bitmap);
   image.MakeThreadSafe();
@@ skipping 30 matching lines @@
 ArcWallpaperService::ArcWallpaperService(ArcBridgeService* bridge_service)
     : ArcService(bridge_service), binding_(this) {
   arc_bridge_service()->wallpaper()->AddObserver(this);
 }
 
 ArcWallpaperService::~ArcWallpaperService() {
   DCHECK_CURRENTLY_ON(content::BrowserThread::UI);
   // 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);
 }
 
 void ArcWallpaperService::OnInstanceReady() {
   DCHECK_CURRENTLY_ON(content::BrowserThread::UI);
   mojom::WallpaperInstance* wallpaper_instance =
       arc_bridge_service()->wallpaper()->instance();
   if (!wallpaper_instance) {
     LOG(DFATAL) << "OnWallpaperInstanceReady called, "
                 << "but no wallpaper instance found";
     return;
   }
   wallpaper_instance->Init(binding_.CreateInterfacePtrAndBind());
+  ash::WmShell::Get()->wallpaper_controller()->AddObserver(this);
 }
 
 void ArcWallpaperService::SetWallpaper(mojo::Array<uint8_t> png_data) {
   DCHECK_CURRENTLY_ON(content::BrowserThread::UI);
   ImageDecoder::Cancel(this);
   ImageDecoder::StartWithOptions(this, png_data.PassStorage(),
                                  ImageDecoder::ROBUST_PNG_CODEC, true);
 }
 
 void ArcWallpaperService::GetWallpaper(const GetWallpaperCallback& callback) {
   DCHECK_CURRENTLY_ON(content::BrowserThread::UI);
   ash::WallpaperController* wc = ash::WmShell::Get()->wallpaper_controller();
   gfx::ImageSkia wallpaper = wc->GetWallpaper();
   base::PostTaskAndReplyWithResult(
       content::BrowserThread::GetBlockingPool(), FROM_HERE,
       base::Bind(&EncodeImagePng, wallpaper), callback);
 }
 
 void ArcWallpaperService::OnImageDecoded(const SkBitmap& bitmap) {
   DCHECK_CURRENTLY_ON(content::BrowserThread::UI);
   SetBitmapAsWallpaper(bitmap);
 }
 
 void ArcWallpaperService::OnDecodeImageFailed() {
   DCHECK_CURRENTLY_ON(content::BrowserThread::UI);
   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