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"
 #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 21 matching lines @@
 }  // namespace
 
 ArcProcessTask::ArcProcessTask(base::ProcessId pid,
                                base::ProcessId nspid,
                                const std::string& process_name,
                                arc::mojom::ProcessState process_state,
                                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),

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

Nit: I would add here instead:

package_name_(!packages.empty() ? packages.at(0) : ""),

and then inside the ctor body, call StartIconLoading() anyway.

Inside StartIconLoading(), first line should be:
  if (package_name_.empty())
    return;

WDYT?

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

Done.

       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);
+
+  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 {
   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() {
+  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;
 }
 
 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