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..24a82205ea56618fcadfca7ac597f276b6bdf1c9 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"
 #include "components/arc/arc_service_manager.h"
 #include "components/arc/common/process.mojom.h"
 #include "content/public/browser/browser_thread.h"
@@ -56,27 +55,37 @@ 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);
+
+  StartIconLoading();
+}
+
+void ArcProcessTask::StartIconLoading() {
+  DCHECK_CURRENTLY_ON(content::BrowserThread::UI);
 
   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) {
+    std::vector<arc::ActivityIconLoader::ActivityName> activity = {

[afakhry, 2016/06/02 22:23:20]

Nit: I would call this |activities| (plural) since it's a vector, even though it
contains a single element.

[Yusuke Sato, 2016/06/02 23:02:34]

Done.

+        {package_name_, kEmptyActivityName}};
+    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()));
+  if (result == arc::ActivityIconLoader::GetResult::FAILED_ARC_NOT_READY) {
+    // Need to retry loading the icon.
+    arc::ArcBridgeService::Get()->AddObserver(this);
+  }
 }
 
 ArcProcessTask::~ArcProcessTask() {
+  arc::ArcBridgeService::Get()->RemoveObserver(this);

[afakhry, 2016/06/02 21:47:12]

What happens if you're not observing?

[Yusuke Sato, 2016/06/02 21:56:06]

Nothing happens. It's implemented with base::ObserverList and its
RemoveObserver() simply ignores unknown pointers.

I'll update ArcBridgeService's function comment and its unittest in a separate
CL.

[afakhry, 2016/06/02 22:23:20]

Acknowledged.

[Yusuke Sato, 2016/06/02 23:02:34]

Done. CL in CQ: https://codereview.chromium.org/2031143002/

 }
 
 Task::Type ArcProcessTask::GetType() const {
@@ -108,6 +117,21 @@ void ArcProcessTask::Kill() {
       nspid_, "Killed manually from Task Manager");
 }
 
+void ArcProcessTask::OnIntentHelperInstanceReady() {
+  DCHECK_CURRENTLY_ON(content::BrowserThread::UI);
+
+  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(&ArcProcessTask::StartIconLoading,
+                                              weak_ptr_factory_.GetWeakPtr()));
+}
+
 void ArcProcessTask::SetProcessState(arc::mojom::ProcessState process_state) {
   process_state_ = process_state;
 }