Retry icon fetching after intent_helper is ready

chrome/browser/task_management/providers/arc/arc_process_task.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/task_management/providers/arc/arc_process_task.h"
 
 #include "base/bind.h"
 #include "base/i18n/rtl.h"
 #include "base/strings/utf_string_conversions.h"
 #include "chrome/grit/generated_resources.h"
-#include "components/arc/arc_bridge_service.h"

[Luis Héctor Chávez, 2016/06/02 16:59:47]

Keep it for IWYU?

[Yusuke Sato, 2016/06/02 18:03:19]

This has been moved to the header side, and as far as I know, it's allowed to
remove it from .cc:

https://google.github.io/styleguide/cppguide.html#Names_and_Order_of_Includes

"However, any includes present in the related header do not need to be included
again in the related cc (i.e., foo.cc can rely on foo.h's includes)."

 #include "components/arc/arc_service_manager.h"
 #include "components/arc/common/process.mojom.h"
 #include "content/public/browser/browser_thread.h"
 #include "content/public/common/child_process_host.h"
 #include "ui/base/l10n/l10n_util.h"
 #include "ui/gfx/image/image.h"
 
 namespace task_management {
 
 namespace {
@@ skipping 27 matching lines @@
                                const std::vector<std::string>& packages)
     : Task(MakeTitle(process_name), process_name, nullptr /* icon */, pid),
       nspid_(nspid),
       process_name_(process_name),
       process_state_(process_state),
       weak_ptr_factory_(this) {
   DCHECK_CURRENTLY_ON(content::BrowserThread::UI);
   if (packages.empty())
     return;
 
-  std::vector<arc::ActivityIconLoader::ActivityName> activity;
   // |packages| contains an alphabetically-sorted list of package names the
   // process has. Since the Task class can hold only one icon per process,
   // and there is no reliable way to pick the most important process from
   // the |packages| list, just use the first item in the list.
-  activity.emplace_back(packages.at(0), kEmptyActivityName);
+  package_name_ = packages.at(0);
+
+  if (!StartIconLoading()) {
+    // Need to retry loading the icon.
+    arc::ArcBridgeService::Get()->AddObserver(this);
+  }
+}
+
+bool ArcProcessTask::StartIconLoading() {
+  DCHECK_CURRENTLY_ON(content::BrowserThread::UI);
+  std::vector<arc::ActivityIconLoader::ActivityName> activity;

[Luis Héctor Chávez, 2016/06/02 16:59:47]

nit: maybe only do this in the if block?

[Yusuke Sato, 2016/06/02 18:03:19]

Good catch, done.

+  activity.emplace_back(package_name_, kEmptyActivityName);
 
   scoped_refptr<arc::ActivityIconLoader> icon_loader = GetIconLoader();
-  if (!icon_loader)
-    return;
+  arc::ActivityIconLoader::GetResult result =
+      arc::ActivityIconLoader::GetResult::FAILED_ARC_NOT_READY;
+  if (icon_loader) {
+    result = icon_loader->GetActivityIcons(
+        activity, base::Bind(&ArcProcessTask::OnIconLoaded,
+                             weak_ptr_factory_.GetWeakPtr()));
+  }
 
-  // TODO(yusukes): ArcProcessTask might be created before intent_helper
-  // instance becomes ready. Because of the race, OnIconLoaded() might
-  // be called synchronously with no icon. Fix the race to ensure that
-  // ArcProcessTask can always fetch icons.
-  icon_loader->GetActivityIcons(activity,
-                                base::Bind(&ArcProcessTask::OnIconLoaded,
-                                           weak_ptr_factory_.GetWeakPtr()));
+  // Returns true when there is no need to call GetActivityIcons() later.
+  return result != arc::ActivityIconLoader::GetResult::FAILED_ARC_NOT_READY;
 }
 
 ArcProcessTask::~ArcProcessTask() {
+  arc::ArcBridgeService::Get()->RemoveObserver(this);
 }
 
 Task::Type ArcProcessTask::GetType() const {
   return Task::ARC;
 }
 
 int ArcProcessTask::GetChildProcessUniqueID() const {
   // ARC process is not a child process of the browser.
   return content::ChildProcessHost::kInvalidUniqueID;
 }
@@ skipping 11 matching lines @@
     return;
   }
   if (arc::ArcBridgeService::Get()->process_version() < 1) {
     LOG(ERROR) << "ARC KillProcess IPC is unavailable.";
     return;
   }
   arc_process_instance->KillProcess(
       nspid_, "Killed manually from Task Manager");
 }
 
+void ArcProcessTask::OnIntentHelperInstanceReady() {
+  VLOG(2) << "intent_helper instance is ready. Fetching the icon for "
+          << package_name_;
+  arc::ArcBridgeService::Get()->RemoveObserver(this);
+
+  // Instead of calling into StartIconLoading() directly, return to the main
+  // loop first to make sure other ArcBridgeService observers are notified.
+  // Otherwise, arc::ActivityIconLoader::GetActivityIcon() may fail again.
+  content::BrowserThread::PostTask(
+      content::BrowserThread::UI, FROM_HERE,
+      base::Bind(base::IgnoreResult(&ArcProcessTask::StartIconLoading),

[Luis Héctor Chávez, 2016/06/02 16:59:47]

It might be the case that by the time you call StartIconLoading, the IconLoader
was destroyed and might be re-loaded at a later point in time.

So how about you make StartIconLoading not return anything and have it add the
observer to be notified in case it cannot get the IconLoader?

[Yusuke Sato, 2016/06/02 18:03:19]

Done. I'd keep this PostTask though (because directly calling into
StartIconLoading() which may call AddObserver() doesn't seem safe.)

[Luis Héctor Chávez, 2016/06/02 20:03:51]

Keeping the PostTask is the right way to do it.

+                 weak_ptr_factory_.GetWeakPtr()));
+}
+
 void ArcProcessTask::SetProcessState(arc::mojom::ProcessState process_state) {
   process_state_ = process_state;
 }
 
 void ArcProcessTask::OnIconLoaded(
     std::unique_ptr<arc::ActivityIconLoader::ActivityToIconsMap> icons) {
   DCHECK_CURRENTLY_ON(content::BrowserThread::UI);
   for (const auto& kv : *icons) {
     const gfx::Image& icon = kv.second.icon16;
     if (icon.IsEmpty())
       continue;
     set_icon(*icon.ToImageSkia());
     break;  // Since the parent class can hold only one icon, break here.
   }
 }
 
 }  // namespace task_management