Fetch ARC Kiosk app name and icon from Android side.

chrome/browser/chromeos/app_mode/kiosk_app_icon_loader.cc

+// Copyright 2017 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/app_mode/kiosk_app_icon_loader.h"
+
+#include <vector>
+
+#include "base/bind.h"
+#include "base/files/file_path.h"
+#include "base/files/file_util.h"
+#include "base/threading/sequenced_worker_pool.h"
+#include "chrome/browser/image_decoder.h"
+#include "content/public/browser/browser_thread.h"
+
+using content::BrowserThread;
+
+namespace chromeos {
+
+class KioskAppIconLoader::IconImageRequest : public ImageDecoder::ImageRequest {
+ public:
+  IconImageRequest(const scoped_refptr<base::SequencedTaskRunner>& task_runner,
+                   KioskAppIconLoader* icon_loader)
+      : ImageRequest(task_runner), icon_loader_(icon_loader) {}
+
+  void OnImageDecoded(const SkBitmap& decoded_image) override {
+    icon_loader_->icon_ = gfx::ImageSkia::CreateFrom1xBitmap(decoded_image);
+    icon_loader_->icon_.MakeThreadSafe();
+    icon_loader_->ReportResultOnBlockingPool(SUCCESS);
+    delete this;
+  }
+
+  void OnDecodeImageFailed() override {
+    icon_loader_->ReportResultOnBlockingPool(FAILED_TO_DECODE);
+    delete this;
+  }
+
+ private:
+  ~IconImageRequest() override = default;
+  KioskAppIconLoader* icon_loader_;

[Luis Héctor Chávez, 2017/03/30 16:49:54]

KioskAppIconLoader* const icon_loader_;

[Sergey Poromov, 2017/03/30 18:22:08]

Done.

+};
+
+KioskAppIconLoader::KioskAppIconLoader(const base::WeakPtr<Delegate>& delegate)
+    : delegate_(delegate) {}
+
+KioskAppIconLoader::~KioskAppIconLoader() = default;
+
+void KioskAppIconLoader::Start(const base::FilePath& icon_path) {
+  base::SequencedWorkerPool* pool = BrowserThread::GetBlockingPool();
+  base::SequencedWorkerPool::SequenceToken token = pool->GetSequenceToken();
+  task_runner_ = pool->GetSequencedTaskRunnerWithShutdownBehavior(
+      token, base::SequencedWorkerPool::SKIP_ON_SHUTDOWN);
+  task_runner_->PostTask(FROM_HERE,
+                         base::Bind(&KioskAppIconLoader::LoadOnBlockingPool,
+                                    base::Unretained(this), icon_path));

[Luis Héctor Chávez, 2017/03/30 16:49:54]

base::Unretained(this) is unsafe in all this class: deleting |task_runner_| does
not guarantee that it will be deleted since something else might have a
reference to it, and so accessing it might fail. Use a weakptr for |this|.

[Sergey Poromov, 2017/03/30 18:22:08]

KioskAppIconLoader is self-owned and as I can see nothing else can have a
reference to it.

[Luis Héctor Chávez, 2017/03/30 19:52:27]

Looked at the impl of this particular flavor of task runner. Even if you do
delete the task runner itself, by the time this returns, the task has already
been handed over to the persistent pool and you no longer have any way to notify
it of cancellation.

Given the recommendation I made in the other file to avoid having this class be
self-owned, please move the weakptr to this class instead.

[xiyuan, 2017/03/30 22:23:56]

Not sure why deleting |task_runner_| is relevant here. The KioskAppIconLoader
manages its own lifetime. No one holds an reference to it. It runs until
finished then update its Delegate with loaded and decoded image data.

Making it owned by its Delegate (KioskAppData) works. But IMHO, it is not the
most optimized. The image loading is a one time thing while KioskAppData would
be held in memory indefinitely. Making loader a member of the Delegate seems a
bit wasteful.

This fire-and-forget pattern is used in a few places, e.g. SafeJsonParserImpl
[1], IconLoader [2].

[1]:
https://cs.chromium.org/chromium/src/components/safe_json/safe_json_parser_im...
[2]:
https://cs.chromium.org/chromium/src/chrome/browser/icon_loader_chromeos.cc?r...

[Sergey Poromov, 2017/03/31 12:56:24]

I agree that this pattern is suitable here, as KioskAppIconLoader should be
alive only during the real load, no need to keep pointer to it after callbacks
are called.
On the other hand I understand Luis that it's safer to use weak_ptr here to not
rely on what happens in task_runner. So, I implemented the suggested change in
the latest Patch Set 7 - all other comments were resolved before in previous
Patch Set 6. Also, KioskAppIconLoader can still be released in
OnIconLoadSuccess()/Failure() callbacks - it will be the same behavior as before
in terms of lifetime.

+}
+
+void KioskAppIconLoader::LoadOnBlockingPool(const base::FilePath& icon_path) {
+  DCHECK(task_runner_->RunsTasksOnCurrentThread());
+
+  std::string data;
+  if (!base::ReadFileToString(base::FilePath(icon_path), &data)) {
+    ReportResultOnBlockingPool(FAILED_TO_LOAD);
+    return;
+  }
+  raw_icon_ = base::RefCountedString::TakeString(&data);
+
+  IconImageRequest* image_request = new IconImageRequest(task_runner_, this);

[Luis Héctor Chávez, 2017/03/30 16:49:54]

Please note that you are transferring ownership of |image_request| to
ImageDecoder, and will delete itself on completion.

[Sergey Poromov, 2017/03/30 18:22:08]

Done.

+  ImageDecoder::Start(image_request, raw_icon_->data());

[Luis Héctor Chávez, 2017/03/30 16:49:54]

Do you need the refcounted string? Why not transfer ownership of the data?

ImageDecoder::Start(image_request, std::vector<uint8_t>(image_data.begin(),
image_data.end()))


Also, this class is going away:
https://cs.chromium.org/chromium/src/chrome/browser/image_decoder.h?type=cs&q...

Can you use its replacement instead?

[Sergey Poromov, 2017/03/30 18:22:07]

Done for first comment.

I think refactoring to use the replacement is out of scope of the CL. Most of
this class was just extracted from KioskAppData::IconLoader, so I prefer to not
do complex refactoring, rather then nits or bug fixes.

+}
+
+void KioskAppIconLoader::ReportResultOnBlockingPool(LoadResult result) {
+  DCHECK(task_runner_->RunsTasksOnCurrentThread());
+
+  BrowserThread::PostTask(
+      BrowserThread::UI, FROM_HERE,
+      base::Bind(&KioskAppIconLoader::ReportResultOnUIThread,
+                 base::Unretained(this), result));
+}
+
+void KioskAppIconLoader::NotifyDelegate(LoadResult result) {
+  if (!delegate_)
+    return;
+
+  if (result == SUCCESS)
+    delegate_->OnIconLoadSuccess(icon_);
+  else
+    delegate_->OnIconLoadFailure();
+}
+
+void KioskAppIconLoader::ReportResultOnUIThread(LoadResult result) {
+  DCHECK_CURRENTLY_ON(BrowserThread::UI);
+
+  NotifyDelegate(result);
+  delete this;
+}
+
+}  // namespace chromeos