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

components/arc/intent_helper/arc_intent_helper_bridge.cc

 // Copyright 2016 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 "components/arc/intent_helper/arc_intent_helper_bridge.h"
 
 #include <utility>
 #include <vector>
 
 #include "ash/common/shell_delegate.h"
 #include "ash/common/wallpaper/wallpaper_controller.h"
 #include "ash/common/wm_shell.h"
 #include "ash/public/interfaces/new_window.mojom.h"
 #include "ash/shell.h"
 #include "base/command_line.h"
 #include "base/memory/weak_ptr.h"
 #include "components/arc/arc_bridge_service.h"
 #include "components/arc/intent_helper/activity_icon_loader.h"
 #include "components/arc/intent_helper/link_handler_model_impl.h"
 #include "components/arc/intent_helper/local_activity_resolver.h"
 #include "ui/base/layout.h"
 #include "url/gurl.h"
 
 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";
 
 ArcIntentHelperBridge::ArcIntentHelperBridge(
     ArcBridgeService* bridge_service,
     const scoped_refptr<ActivityIconLoader>& icon_loader,
     const scoped_refptr<LocalActivityResolver>& activity_resolver)
     : ArcService(bridge_service),
       binding_(this),
       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() {
   DCHECK(thread_checker_.CalledOnValidThread());
   ash::Shell::GetInstance()->set_link_handler_model_factory(this);
   auto* instance =
       arc_bridge_service()->intent_helper()->GetInstanceForMethod("Init");
   DCHECK(instance);
   instance->Init(binding_.CreateInterfacePtrAndBind());
 }
@@ skipping 28 matching lines @@
   DCHECK(thread_checker_.CalledOnValidThread());
   ash::WmShell::Get()->wallpaper_controller()->OpenSetWallpaperPage();
 }
 
 void ArcIntentHelperBridge::SetWallpaperDeprecated(
     mojo::Array<uint8_t> jpeg_data) {
   DCHECK(thread_checker_.CalledOnValidThread());
   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());
   std::unique_ptr<LinkHandlerModelImpl> impl(
       new LinkHandlerModelImpl(icon_loader_));
   if (!impl->Init(url))
     return nullptr;
   return std::move(impl);
 }
 
@@ skipping 58 matching lines @@
     const std::string& method_name_for_logging,
     uint32_t min_instance_version) {
   return GetIntentHelperInstanceWithErrorCode(method_name_for_logging,
                                               min_instance_version, nullptr);
 }
 
 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