Remove policy watching from It2MeHost.

remoting/host/it2me/it2me_native_messaging_host_unittest.cc

 // Copyright 2013 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 "remoting/host/it2me/it2me_native_messaging_host.h"
 
 #include <cstdint>
 #include <memory>
 #include <string>
 #include <utility>
@@ skipping 107 matching lines @@
   return true;
 }
 
 void FakePolicyService::RefreshPolicies(const base::Closure& callback) {
   base::ThreadTaskRunnerHandle::Get()->PostTask(FROM_HERE, callback);
 }
 
 class MockIt2MeHost : public It2MeHost {
  public:
   MockIt2MeHost(std::unique_ptr<ChromotingHostContext> context,
-                std::unique_ptr<PolicyWatcher> policy_watcher,
                 base::WeakPtr<It2MeHost::Observer> observer,
                 std::unique_ptr<SignalStrategy> signal_strategy,
                 const std::string& username,
                 const std::string& directory_bot_jid)
       : It2MeHost(std::move(context),
-                  std::move(policy_watcher),
                   /*confirmation_dialog_factory=*/nullptr,
                   observer,
                   std::move(signal_strategy),
                   username,
                   directory_bot_jid) {}
 
   // It2MeHost overrides
   void Connect() override;
   void Disconnect() override;
   void RequestNatPolicy() override;
@@ skipping 77 matching lines @@
 
 MockIt2MeHostFactory::~MockIt2MeHostFactory() {}
 
 scoped_refptr<It2MeHost> MockIt2MeHostFactory::CreateIt2MeHost(
     std::unique_ptr<ChromotingHostContext> context,
     policy::PolicyService* policy_service,
     base::WeakPtr<It2MeHost::Observer> observer,
     std::unique_ptr<SignalStrategy> signal_strategy,
     const std::string& username,
     const std::string& directory_bot_jid) {
-  return new MockIt2MeHost(std::move(context),
-                           /*policy_watcher=*/nullptr, observer,
+  return new MockIt2MeHost(std::move(context), observer,
                            std::move(signal_strategy), username,
                            directory_bot_jid);
 }
 
 }  // namespace
 
 class It2MeNativeMessagingHostTest : public testing::Test {
  public:
   It2MeNativeMessagingHostTest() {}
   ~It2MeNativeMessagingHostTest() override {}
@@ skipping 148 matching lines @@
   VerifyCommonProperties(std::move(response), "helloResponse", request_id);
 }
 
 void It2MeNativeMessagingHostTest::VerifyErrorResponse() {
   std::unique_ptr<base::DictionaryValue> response = ReadMessageFromOutputPipe();
   VerifyStringProperty(std::move(response), "type", "error");
 }
 
 void It2MeNativeMessagingHostTest::VerifyConnectResponses(int request_id) {
   bool connect_response_received = false;
+  bool nat_policy_received = false;

[rkjnsn, 2017/05/03 21:31:47]

I see this was added to fix the unit test, but I can't figure out it falls out
from the other changes.

[Jamie, 2017/05/03 23:41:10]

The change to the native messaging host means that It2MeHost::OnPolicyUpdate is
now being called whereas previously it wasn't, which results in additional
message being sent. Does that answer your question?

[rkjnsn, 2017/05/04 20:45:45]

Ah, I see. Since MockIt2MeHost overrides Connect, which before this change would
have normally kicked off reading the policy, the policy was never read and
OnNatPolicyChanged was never getting called. Now that the code is calling
OnPolicyUpdate explicitly, which isn't overridden by the mock,
OnNatPolicyChanged is getting called as a result.

   bool starting_received = false;
   bool requestedAccessCode_received = false;
   bool receivedAccessCode_received = false;
   bool connecting_received = false;
   bool connected_received = false;
 
-  // We expect a total of 6 messages: 1 connectResponse and 5 hostStateChanged.
-  for (int i = 0; i < 6; ++i) {
+  // We expect a total of 7 messages: 1 connectResponse, 1 natPolicyChanged,
+  // and 5 hostStateChanged.
+  for (int i = 0; i < 7; ++i) {
     std::unique_ptr<base::DictionaryValue> response =
         ReadMessageFromOutputPipe();
     ASSERT_TRUE(response);
 
     std::string type;
     ASSERT_TRUE(response->GetString("type", &type));
 
     if (type == "connectResponse") {
       EXPECT_FALSE(connect_response_received);
       connect_response_received = true;
       VerifyId(std::move(response), request_id);
+    } else if (type == "natPolicyChanged") {
+      EXPECT_FALSE(nat_policy_received);
+      nat_policy_received = true;
     } else if (type == "hostStateChanged") {
       std::string state;
       ASSERT_TRUE(response->GetString("state", &state));
 
       std::string value;
       if (state == It2MeNativeMessagingHost::HostStateToString(kStarting)) {
         EXPECT_FALSE(starting_received);
         starting_received = true;
       } else if (state == It2MeNativeMessagingHost::HostStateToString(
                               kRequestedAccessCode)) {
@@ skipping 171 matching lines @@
   EXPECT_TRUE(response);
   std::string value;
   EXPECT_FALSE(response->GetString("id", &value));
 
   response = ReadMessageFromOutputPipe();
   EXPECT_TRUE(response);
   EXPECT_TRUE(response->GetString("id", &value));
   EXPECT_EQ("42", value);
 }
 
-TEST_F(It2MeNativeMessagingHostTest, Connect) {
+TEST_F(It2MeNativeMessagingHostTest, ConnectMultiple) {
   // A new It2MeHost instance is created for every it2me session. The native
   // messaging host, on the other hand, is long lived. This test verifies
   // multiple It2Me host startup and shutdowns.
   for (int i = 0; i < 3; ++i) {
     TestConnect();
   }
 }
 
 // Verify non-Dictionary requests are rejected.
 TEST_F(It2MeNativeMessagingHostTest, WrongFormat) {
   base::ListValue message;
   // No "error" response will be sent for non-Dictionary messages.
   TestBadRequest(message, false);
 }
 
 // Verify requests with no type are rejected.
 TEST_F(It2MeNativeMessagingHostTest, MissingType) {
   base::DictionaryValue message;
   TestBadRequest(message, true);
 }
 
 // Verify rejection if type is unrecognized.
 TEST_F(It2MeNativeMessagingHostTest, InvalidType) {
   base::DictionaryValue message;
   message.SetString("type", "xxx");
   TestBadRequest(message, true);
 }
 
 }  // namespace remoting