Notify Files App when ARC++ app is installed/removed

components/arc/intent_helper/arc_intent_helper_bridge.cc

Index: components/arc/intent_helper/arc_intent_helper_bridge.cc
diff --git a/components/arc/intent_helper/arc_intent_helper_bridge.cc b/components/arc/intent_helper/arc_intent_helper_bridge.cc
index b691412c6e00863612c455b2230e235936bf3f83..e12cc2d8e6623621ee099977d50b68ad7aaf7fb0 100644
--- a/components/arc/intent_helper/arc_intent_helper_bridge.cc
+++ b/components/arc/intent_helper/arc_intent_helper_bridge.cc
@@ -23,6 +23,9 @@
 
 namespace arc {
 
+// TODO(oka): Avoid to assume the class is singleton.

[Yusuke Sato, 2016/11/17 04:22:37]

Adding more global pointers to components/arc/ seems sad. We already have
enough. Instead of this, what about the following:

* Remove g_arc_intent_helper_bridge and ArcIntentHelperBridge::Get()
* Add [set_]arc_intent_helper_bridge() setter/getter to ArcServiceManager.
* Add arc_intent_helper_bridge_ raw pointer (member variable) to
ArcServiceManager;
* Call ArcServiceManager::Get()->set_arc_intent_helper_bridge(this) in
ArcIntentHelperBridge's ctor, and
ArcServiceManager::Get()->set_arc_intent_helper_bridge(nullptr) in its dtor.
* In chrome/browser/, get an ArcIntentHelperBridge instance with
ArcServiceManager::Get()->arc_intent_helper_bridge().

ArcServiceManager is already a singleton and has Get() method. The manager also
has getters like icon_loader() for allowing chrome/browser/ code to use
components/arc/'s objects. The proposal above follows the current way to some
extent.

[hidehiko, 2016/11/17 07:34:19]

ArcIntentHelperBridge's ctor, and
ArcServiceManager::Get()->set_arc_intent_helper_bridge(nullptr) in its dtor.

I do not want this. If we'll get rid of global ptr, I prefer calling the setter
in ArcServiceLauncher::Initialize() instead.
We have some getters, but those are not for ArcService classes, so this looks
something new to me, too. (And that's why I recommended to oka@ san to use
global ptr.)

[Yusuke Sato, 2016/11/17 09:15:27]

Well, I wanted to make the code *look* slightly better (although it does not
resolve anything and the member variable is still effectively like a global
pointer) but if it's not for your taste, that's fine :)

BTW, I have another idea to remove g_arc_intent_helper_bridge (this time,
really) and confirmed that hidehiko@ is okay with it. The idea is:

* Continue to define the Observer class, but probably in a dedicated file in
components/arc/intent_helper/.
* Add AddObserver and RemoveObserver to ArcIntentHelperBridge.
* Let ArcServiceManager observe ArcIntentHelperBridge. You can call
ArcIntentHelperBridge::AddObserver(arc_service_manager); in
ArcServiceLauncher::Initialize() where ArcIntentHelperBridge is created and
added to ArcServiceManager.
* Since ArcServiceManager always outlives the ArcIntentHelperBridge object,
ArcServiceManager doesn't have to call
ArcIntentHelperBridge::RemoveObserver(this); (we need some code comments
though.)
* Let your EventRouter code observe ArcServiceManager, instead of
ArcIntentHelperBridge. You can call ArcServiceManager::AddObserver() in
EventRouter::ObserveEvents().
* You can also call ArcServiceManager::RemoveObserver() in
EventRouter::Shutdown(). When EventRouter::Shutdown() is called,
ArcIntentHelperBridge has already been gone, but ArcServiceManager is still
there.
* Obviously, you also have to chain the OnAppsUpdated() calls.
ArcIntentHelperBridge calls its observers' OnAppsUpdated(), which is ASM's
OnAppsUpdated(). ASM's OnAppsUpdated() calls its observers' OnAppsUpdated(),
which is EventRouter::OnAppsUpdated().

WDYT?
 

[oka, 2016/11/21 07:02:15]

Thank you for the suggestion. I will try the construct in the next patch.

[oka, 2016/11/21 07:02:15]

Abe-san suggested this solution. @hidehiko what do you think?

[Yusuke Sato, 2016/11/21 17:21:05]

What do you mean by "the next patch"? Next patch set in this CL, or different
CL? Since this is not a small TODO but more like a structural change, I'd prefer
the former unless this is urgent.

[oka, 2016/11/24 15:14:17]

Updated the construct as you suggested.

+ArcIntentHelperBridge* g_arc_intent_helper_bridge = nullptr;
+
 // static
 const char ArcIntentHelperBridge::kArcIntentHelperPackageName[] =
     "org.chromium.arc.intent_helper";
@@ -36,12 +39,25 @@ ArcIntentHelperBridge::ArcIntentHelperBridge(
       icon_loader_(icon_loader),
       activity_resolver_(activity_resolver) {
   DCHECK(thread_checker_.CalledOnValidThread());
+  DCHECK(!g_arc_intent_helper_bridge);
+
   arc_bridge_service()->intent_helper()->AddObserver(this);
+
+  g_arc_intent_helper_bridge = this;
 }
 
 ArcIntentHelperBridge::~ArcIntentHelperBridge() {
   DCHECK(thread_checker_.CalledOnValidThread());
+  DCHECK_EQ(this, g_arc_intent_helper_bridge);
+
   arc_bridge_service()->intent_helper()->RemoveObserver(this);
+
+  g_arc_intent_helper_bridge = nullptr;
+}
+
+// static
+ArcIntentHelperBridge* ArcIntentHelperBridge::Get() {
+  return g_arc_intent_helper_bridge;
 }
 
 void ArcIntentHelperBridge::OnInstanceReady() {
@@ -90,6 +106,10 @@ void ArcIntentHelperBridge::SetWallpaperDeprecated(
   LOG(ERROR) << "IntentHelper.SetWallpaper is deprecated";
 }
 
+void ArcIntentHelperBridge::AddObserver(Observer* observer) {
+  observer_list_.AddObserver(observer);
+}
+
 std::unique_ptr<ash::LinkHandlerModel> ArcIntentHelperBridge::CreateModel(
     const GURL& url) {
   DCHECK(thread_checker_.CalledOnValidThread());
@@ -168,6 +188,10 @@ void ArcIntentHelperBridge::OnIntentFiltersUpdated(
     mojo::Array<mojom::IntentFilterPtr> filters) {
   DCHECK(thread_checker_.CalledOnValidThread());
   activity_resolver_->UpdateIntentFilters(std::move(filters));
+
+  for (auto& observer : observer_list_) {

[Yusuke Sato, 2016/11/17 02:29:54]

nit: no {}

[oka, 2016/11/21 07:02:15]

Done.

+    observer.OnAppsUpdated();
+  }
 }
 
 }  // namespace arc