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

components/arc/intent_helper/arc_intent_helper_bridge_unittest.cc

Index: components/arc/intent_helper/arc_intent_helper_bridge_unittest.cc
diff --git a/components/arc/intent_helper/arc_intent_helper_bridge_unittest.cc b/components/arc/intent_helper/arc_intent_helper_bridge_unittest.cc
index 07ed13cbe76adc195375a16a6c65db3c5492aa26..3c6279a13dca61f49a96db69acd865b1f6b9cb3b 100644
--- a/components/arc/intent_helper/arc_intent_helper_bridge_unittest.cc
+++ b/components/arc/intent_helper/arc_intent_helper_bridge_unittest.cc
@@ -7,13 +7,45 @@
 #include <utility>
 
 #include "components/arc/common/intent_helper.mojom.h"
+#include "components/arc/intent_helper/activity_icon_loader.h"
+#include "components/arc/intent_helper/local_activity_resolver.h"
+#include "components/arc/test/fake_arc_bridge_service.h"
 #include "testing/gtest/include/gtest/gtest.h"
 
 namespace arc {
 
+namespace {
+
+class ArcIntentHelperTest : public testing::Test {
+ public:
+  ArcIntentHelperTest()
+      : icon_loader_(new ActivityIconLoader()),

[hidehiko, 2016/11/29 15:29:30]

Optional: How about put these into SetUp()/TearDown() so that initialization can
be done in one place?

[oka, 2016/11/30 07:22:16]

Done.

+        activity_resolver_(new LocalActivityResolver()) {}
+
+ protected:
+  std::unique_ptr<FakeArcBridgeService> fake_arc_bridge_service_;

[hidehiko, 2016/11/29 15:29:30]

Could you kindly sort the order in the initialization?

[oka, 2016/11/30 07:22:15]

Done.

+  scoped_refptr<ActivityIconLoader> icon_loader_;
+  scoped_refptr<LocalActivityResolver> activity_resolver_;
+  std::unique_ptr<ArcIntentHelperBridge> instance_;
+
+ private:
+  void SetUp() override {
+    fake_arc_bridge_service_.reset(new FakeArcBridgeService());
+    instance_.reset(new ArcIntentHelperBridge(
+        fake_arc_bridge_service_.get(), icon_loader_, activity_resolver_));
+  }
+
+  void TearDown() override {
+    instance_.reset();
+    fake_arc_bridge_service_.reset();
+  }
+};

[hidehiko, 2016/11/29 15:29:30]

Could you add:

DISALLOW_COPY_AND_ASSIGN(ArcIntentHelperTest);

?

[oka, 2016/11/30 07:22:15]

Done.

+
+}  // namespace
+
 // Tests if IsIntentHelperPackage works as expected. Probably too trivial
 // to test but just in case.
-TEST(ArcIntentHelperTest, TestIsIntentHelperPackage) {
+TEST_F(ArcIntentHelperTest, TestIsIntentHelperPackage) {
   EXPECT_FALSE(ArcIntentHelperBridge::IsIntentHelperPackage(""));
   EXPECT_FALSE(ArcIntentHelperBridge::IsIntentHelperPackage(
       ArcIntentHelperBridge::kArcIntentHelperPackageName + std::string("a")));
@@ -25,7 +57,7 @@ TEST(ArcIntentHelperTest, TestIsIntentHelperPackage) {
 }
 
 // Tests if FilterOutIntentHelper removes handlers as expected.
-TEST(ArcIntentHelperTest, TestFilterOutIntentHelper) {
+TEST_F(ArcIntentHelperTest, TestFilterOutIntentHelper) {
   {
     std::vector<mojom::IntentHandlerInfoPtr> orig;
     std::vector<mojom::IntentHandlerInfoPtr> filtered =
@@ -101,4 +133,29 @@ TEST(ArcIntentHelperTest, TestFilterOutIntentHelper) {
   }
 }
 
+// Tests if observer works as expected.
+TEST_F(ArcIntentHelperTest, TestObserver) {
+  struct FakeObserver : public ArcIntentHelperObserver {

[hidehiko, 2016/11/29 15:29:30]

Could you use class, instead?
https://google.github.io/styleguide/cppguide.html#Structs_vs._Classes

[oka, 2016/11/30 07:22:15]

Done.

+   public:
+    FakeObserver() : updated_(false) {}

[hidehiko, 2016/11/29 15:29:30]

You can initialize the member at declaration.

FakeObserver() = default;
 ...
bool updated_ = false;

[oka, 2016/11/30 07:22:16]

Done.

+    void OnAppsUpdated() override { updated_ = true; }
+    bool updated_;

[hidehiko, 2016/11/29 15:29:30]

the field should be private.
https://google.github.io/styleguide/cppguide.html#Access_Control

[oka, 2016/11/30 07:22:16]

Done.

+  };
+
+  // Observer should be called when intent filter is updated.

[hidehiko, 2016/11/29 15:29:30]

Nice description!

[oka, 2016/11/30 07:22:15]

Done.

+  std::unique_ptr<FakeObserver> observer(new FakeObserver());

[hidehiko, 2016/11/29 15:29:30]

Optional:

auto observer = base::MakeUnique<FakeObserver>();

so that FakeObserver name can be once.

[oka, 2016/11/30 07:22:15]

Done.

+  instance_->AddObserver(observer.get());
+  std::vector<mojom::IntentFilterPtr> v;

[hidehiko, 2016/11/29 15:29:30]

Optional:

I think you can inline the vector.

EXCEPT_FALSE(observer->updated());
instance_->OnInitFiltersUpdated(std::vector<mojom::IntentFilterPtr>());
EXPECT_TRUE(observer->updated());

Ditto for below.

[oka, 2016/11/30 07:22:15]

Done.

+  EXPECT_FALSE(observer->updated_);
+  instance_->OnIntentFiltersUpdated(std::move(v));
+  EXPECT_TRUE(observer->updated_);
+
+  // Observer should not be called after it's removed.
+  observer->updated_ = false;
+  instance_->RemoveObserver(observer.get());
+  std::vector<mojom::IntentFilterPtr> v2;
+  instance_->OnIntentFiltersUpdated(std::move(v2));
+  EXPECT_FALSE(observer->updated_);
+}
+
 }  // namespace arc