arc-app-launcher: Minimal support for ARC app launcher.

chrome/browser/ui/app_list/arc_app_icon.cc

+// Copyright 2015 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/ui/app_list/arc_app_icon.h"
+
+#include <algorithm>
+
+#include "base/bind.h"
+#include "base/files/file_path.h"
+#include "base/files/file_util.h"
+#include "chrome/browser/image_decoder.h"
+#include "chrome/browser/ui/app_list/arc_app_prefs.h"
+#include "content/public/browser/browser_thread.h"
+#include "extensions/grit/extensions_browser_resources.h"
+#include "ui/base/resource/resource_bundle.h"
+#include "ui/gfx/geometry/size.h"
+#include "ui/gfx/image/image_skia_source.h"
+
+////////////////////////////////////////////////////////////////////////////////
+// ArcAppIcon::Source
+
+class ArcAppIcon::Source : public gfx::ImageSkiaSource {
+ public:
+  Source(ArcAppIcon* host, const gfx::Size& size_in_dip);

[xiyuan, 2015/11/11 22:15:49]

|size_in_dip| seems not used?

[khmel1, 2015/11/12 08:05:29]

It is planned to use in future for new UI items but yes, at this moment we can
remove this.

+  ~Source() override;
+
+  void ResetHost();
+
+ private:
+  // gfx::ImageSkiaSource overrides:
+  gfx::ImageSkiaRep GetImageForScale(float scale) override;
+
+  // Used to load images asynchronously. NULLed out when the ArcAppIcon is
+  // destroyed.
+  ArcAppIcon* host_;

[xiyuan, 2015/11/11 22:15:49]

Do we want to use a WeakPtr<ArcAppIcon> here? This way, we don't need to
manually call ResetHost.

See
https://code.google.com/p/chromium/codesearch#chromium/src/base/memory/weak_p...

[khmel1, 2015/11/12 08:05:29]

Thanks for good advice! It is more elegant (I used extensions::IconImage::Source
for reference and it contains ResetHost)

+
+  DISALLOW_COPY_AND_ASSIGN(Source);
+};
+
+ArcAppIcon::Source::Source(ArcAppIcon* host, const gfx::Size& size_in_dip)
+  : host_(host) {
+}
+
+ArcAppIcon::Source::~Source() {
+}
+
+void ArcAppIcon::Source::ResetHost() {
+  host_ = NULL;

[xiyuan, 2015/11/11 22:15:49]

nit: NULL -> nullptr

[khmel1, 2015/11/12 08:05:29]

Removed once WeakPtr is used.

+}
+
+gfx::ImageSkiaRep ArcAppIcon::Source::GetImageForScale(float scale) {
+  if (host_) {
+    host_->LoadForScaleFactor(ui::GetSupportedScaleFactor(scale));
+  }
+
+  return gfx::ImageSkiaRep();
+}
+
+class ArcAppIcon::DecodeRequest : public ImageDecoder::ImageRequest {
+ public:
+  DecodeRequest(ArcAppIcon* host, int dimension, ui::ScaleFactor scale_factor);
+  ~DecodeRequest() override;
+
+  void ResetHost();
+
+  // ImageDecoder::ImageRequest
+  void OnImageDecoded(const SkBitmap& bitmap) override;
+  void OnDecodeImageFailed() override;
+ private:
+  ArcAppIcon* host_;
+  int dimension_;
+  ui::ScaleFactor scale_factor_;
+
+  DISALLOW_COPY_AND_ASSIGN(DecodeRequest);
+};
+
+////////////////////////////////////////////////////////////////////////////////
+// ArcAppIcon::DecodeRequest
+
+ArcAppIcon::DecodeRequest::DecodeRequest(ArcAppIcon* host,
+                                         int dimension,
+                                         ui::ScaleFactor scale_factor)
+  : host_(host),
+    dimension_(dimension),
+    scale_factor_(scale_factor) {
+}
+
+ArcAppIcon::DecodeRequest::~DecodeRequest() {
+}
+
+void ArcAppIcon::DecodeRequest::ResetHost() {
+  host_ = NULL;

[xiyuan, 2015/11/11 22:15:49]

nit: NULL -> nullptr

[khmel1, 2015/11/12 08:05:29]

Done.

+}
+
+void ArcAppIcon::DecodeRequest::OnImageDecoded(const SkBitmap& bitmap) {
+  DCHECK(!bitmap.isNull() && !bitmap.empty());
+
+  if (!host_) {
+    return;
+  }
+
+  int expected_dim = static_cast<int>(
+      ui::GetScaleForScaleFactor(scale_factor_) * dimension_ + 0.5f);
+  if (bitmap.width() != expected_dim || bitmap.height() != expected_dim) {
+    LOG(ERROR) << "Decoded ARC icon has unexpected dimension "
+               << bitmap.width() << "x" << bitmap.height() << ". Expected "
+               << expected_dim << "x" << ".";
+    return;
+  }
+
+  gfx::ImageSkia image_skia;
+  image_skia.AddRepresentation(gfx::ImageSkiaRep(
+      bitmap,
+      ui::GetScaleForScaleFactor(scale_factor_)));
+
+  host_->Update(&image_skia);
+  host_->DiscardDecodeRequest(this);
+
+  host_->observer_->OnIconUpdated();

[xiyuan, 2015/11/11 22:15:49]

Would this be a use-after-free? |this| should be deleted after
DiscardDecodeRequest above and its |host_| member should not be accessed
thereafter.

And think this should be part of host_'s Update() instead.

[khmel1, 2015/11/12 08:05:29]

Done.

+}
+
+void ArcAppIcon::DecodeRequest::OnDecodeImageFailed() {
+  LOG(ERROR) << "Failed to decode ARC icon.";
+
+  if (!host_) {
+    return;
+  }
+
+  host_->DiscardDecodeRequest(this);
+}
+
+////////////////////////////////////////////////////////////////////////////////
+// ArcAppIcon
+
+ArcAppIcon::ArcAppIcon(content::BrowserContext* context,
+                       const std::string& app_id,
+                       int resource_size_in_dip,
+                       Observer* observer)
+    : context_(context),
+      app_id_(app_id),
+      resource_size_in_dip_(resource_size_in_dip),
+      observer_(observer),
+      source_(NULL),

[xiyuan, 2015/11/11 22:15:49]

nit: think in-class member initialization is preferred than this now.

i.e. do this in the header file
  Source* source_ = nullptr;

[khmel1, 2015/11/12 08:05:29]

Done.

+      weak_ptr_factory_(this) {
+  CHECK(observer_ != NULL);
+  gfx::Size resource_size(resource_size_in_dip, resource_size_in_dip);
+  source_ = new Source(this, resource_size);
+  image_skia_ = gfx::ImageSkia(source_, resource_size);
+  Update(ResourceBundle::GetSharedInstance().GetImageSkiaNamed(

[xiyuan, 2015/11/11 22:15:49]

This Update() call has some side effects.

ImageSkiaSource::GetImageForScale is only invoked when its hosting ImageSkia
does not have ImageRep for a scale factor. It looks like we are relying on this
behavior to trigger the ARC icon load. However, this Update() will fill in
default icon for certain scale factors and we will only see the default icon for
those scale factors as a result.

To show the default icon before real icon, think we should return default icon
in Source::GetImageForScale instead.

[khmel1, 2015/11/12 08:05:29]

Yes, I also noticed this issue when was implementing. You suggestion is good
fix.

+      IDR_APP_DEFAULT_ICON));
+}
+
+ArcAppIcon::~ArcAppIcon() {
+  source_->ResetHost();
+  for (size_t i = 0; i < decode_requests_.size(); ++i) {
+    decode_requests_[i]->ResetHost();
+    ImageDecoder::Cancel(decode_requests_[i]);

[xiyuan, 2015/11/11 22:15:49]

nit: Cancel() is not necessary here since ImageDecoder::ImageRequest's dtor
would call it anyway.

[khmel1, 2015/11/12 08:05:29]

Done.

+  }
+}
+
+void ArcAppIcon::LoadForScaleFactor(ui::ScaleFactor scale_factor) {
+  ArcAppPrefs* prefs = ArcAppPrefs::Get(context_);
+  if (!prefs) {
+    LOG(ERROR) << "ARC preferences service is not available.";
+    return;
+  }
+
+  base::FilePath path = prefs->GetIconPath(app_id_, scale_factor);
+  if (path.empty()) {
+    return;
+  }
+
+  base::Closure task = base::Bind(&ArcAppIcon::ReadOnFileThread,
+                                  base::Unretained(this),

[xiyuan, 2015/11/11 22:15:49]

Should this be weak_ptr_factory_.GetWeakPtr() instead of base::Unretained?

[khmel1, 2015/11/12 08:05:29]

Yes, it should

+                                  scale_factor,
+                                  path);
+  content::BrowserThread::PostTask(content::BrowserThread::FILE,

[xiyuan, 2015/11/11 22:15:49]

File thread is deprecated (because it is only one thread and easily got choked)
and we should use blocking pool in new code.

BrowserThread::GetBlockingPool()->PostTask(...)

[khmel1, 2015/11/12 08:05:29]

Good to know! Done

+                                   FROM_HERE,
+                                   task);
+}
+
+void ArcAppIcon::RequestIcon(ui::ScaleFactor scale_factor) {
+  DCHECK_CURRENTLY_ON(content::BrowserThread::UI);
+  ArcAppPrefs* prefs = ArcAppPrefs::Get(context_);
+  if (!prefs) {
+    LOG(ERROR) << "ARC preferences service is not available.";
+    return;
+  }
+  prefs->RequestIcon(app_id_, scale_factor);

[xiyuan, 2015/11/11 22:15:49]

How would we find out after ArcAppPrefs gets the icon?

[khmel1, 2015/11/12 08:05:29]

I added comment.

+}
+
+void ArcAppIcon::ReadOnFileThread(ui::ScaleFactor scale_factor,
+                                  const base::FilePath& path) {
+  DCHECK_CURRENTLY_ON(content::BrowserThread::FILE);
+  DCHECK(!path.empty());
+
+  if (!base::PathExists(path)) {
+    base::Closure task = base::Bind(&ArcAppIcon::RequestIcon,
+                                    base::Unretained(this),

[xiyuan, 2015/11/11 22:15:49]

weak_ptr_factory_.GetWeakPtr()?

[khmel1, 2015/11/12 08:05:29]

Done.

+                                    scale_factor);
+    content::BrowserThread::PostTask(content::BrowserThread::UI,
+                                     FROM_HERE,
+                                     task);
+    return;
+  }
+
+  std::string unsafe_icon_data;
+  // Read the file from disk.
+  if (!base::ReadFileToString(path, &unsafe_icon_data)) {
+    LOG(ERROR) << "Failed to read an ARC icon from file "
+               << path.MaybeAsASCII();
+    return;
+  }
+
+  base::Closure task = base::Bind(&ArcAppIcon::OnIconRead,
+                                  base::Unretained(this),

[xiyuan, 2015/11/11 22:15:49]

weak_ptr_factory_.GetWeakPtr()?

[khmel1, 2015/11/12 08:05:29]

Done.

+                                  scale_factor,
+                                  unsafe_icon_data);
+  content::BrowserThread::PostTask(content::BrowserThread::UI,
+                                   FROM_HERE,
+                                   task);
+}
+
+void ArcAppIcon::OnIconRead(ui::ScaleFactor scale_factor,
+                            const std::string& unsafe_image_data) {
+  DCHECK_CURRENTLY_ON(content::BrowserThread::UI);
+
+  decode_requests_.push_back(new DecodeRequest(this,
+                                               resource_size_in_dip_,
+                                              scale_factor));

[xiyuan, 2015/11/11 22:15:49]

nit: align with previous line.

[khmel1, 2015/11/12 08:05:29]

Done.

+  ImageDecoder::Start(decode_requests_.back(), unsafe_image_data);
+}
+
+void ArcAppIcon::Update(const gfx::ImageSkia* image) {
+  DCHECK_CURRENTLY_ON(content::BrowserThread::UI);
+  CHECK(image && !image->isNull());
+
+  std::vector<gfx::ImageSkiaRep> reps = image->image_reps();
+  for (const auto& image_rep : reps) {
+    if (ui::IsSupportedScale(image_rep.scale())) {
+      image_skia_.RemoveRepresentation(image_rep.scale());
+      image_skia_.AddRepresentation(image_rep);
+    }
+  }
+
+  image_ = gfx::Image(image_skia_);
+}
+
+void ArcAppIcon::DiscardDecodeRequest(DecodeRequest* request) {
+  DCHECK_CURRENTLY_ON(content::BrowserThread::UI);
+
+  ScopedVector<DecodeRequest>::iterator it = std::find(decode_requests_.begin(),
+                                                       decode_requests_.end(),
+                                                       request);
+  CHECK(it != decode_requests_.end());
+  decode_requests_.erase(it);
+}