Retry icon fetching after intent_helper is ready

chrome/browser/task_management/providers/arc/arc_process_task.cc

Index: chrome/browser/task_management/providers/arc/arc_process_task.cc
diff --git a/chrome/browser/task_management/providers/arc/arc_process_task.cc b/chrome/browser/task_management/providers/arc/arc_process_task.cc
index 3770482bf3f424a8e12bc78fcfda3b5ff701df35..097dd7a06b8c67725424b588103221237f148f1e 100644
--- a/chrome/browser/task_management/providers/arc/arc_process_task.cc
+++ b/chrome/browser/task_management/providers/arc/arc_process_task.cc
@@ -8,7 +8,6 @@
 #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"
@@ -56,27 +55,38 @@ ArcProcessTask::ArcProcessTask(base::ProcessId pid,
   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 {
@@ -108,6 +118,20 @@ void ArcProcessTask::Kill() {
       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;
 }