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

components/arc/intent_helper/arc_intent_helper_bridge_unittest.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 "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")));
   EXPECT_FALSE(ArcIntentHelperBridge::IsIntentHelperPackage(
       ArcIntentHelperBridge::kArcIntentHelperPackageName +
       std::string("/.ArcIntentHelperActivity")));
   EXPECT_TRUE(ArcIntentHelperBridge::IsIntentHelperPackage(
       ArcIntentHelperBridge::kArcIntentHelperPackageName));
 }
 
 // Tests if FilterOutIntentHelper removes handlers as expected.
-TEST(ArcIntentHelperTest, TestFilterOutIntentHelper) {
+TEST_F(ArcIntentHelperTest, TestFilterOutIntentHelper) {
   {
     std::vector<mojom::IntentHandlerInfoPtr> orig;
     std::vector<mojom::IntentHandlerInfoPtr> filtered =
         ArcIntentHelperBridge::FilterOutIntentHelper(std::move(orig));
     EXPECT_EQ(0U, filtered.size());
   }
 
   {
     std::vector<mojom::IntentHandlerInfoPtr> orig;
     orig.push_back(mojom::IntentHandlerInfo::New());
@@ skipping 55 matching lines @@
     orig[1]->name = "1";
     orig[1]->package_name = ArcIntentHelperBridge::kArcIntentHelperPackageName;
 
     // FilterOutIntentHelper should remove all elements.
     std::vector<mojom::IntentHandlerInfoPtr> filtered =
         ArcIntentHelperBridge::FilterOutIntentHelper(std::move(orig));
     EXPECT_EQ(0U, filtered.size());
   }
 }
 
+// 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