Sync the browser test host resolver with the network process before a test runs.

net/dns/mock_host_resolver.h

 // Copyright (c) 2011 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.
 
 #ifndef NET_DNS_MOCK_HOST_RESOLVER_H_
 #define NET_DNS_MOCK_HOST_RESOLVER_H_
 
 #include <stddef.h>
 
 #include <list>
@@ skipping 194 matching lines @@
   // Make sure that |host| will not be re-mapped or even processed by underlying
   // host resolver procedures. It can also be a pattern.
   void AllowDirectLookup(const std::string& host);
 
   // Simulate a lookup failure for |host| (it also can be a pattern).
   void AddSimulatedFailure(const std::string& host);
 
   // Deletes all the rules that have been added.
   void ClearRules();
 
+  // Asserts if any changes are made to the rules after this call.

[mmenke, 2017/04/25 16:37:53]

Could you clarify that this makes calls that add/remove/modify rules assert?  As
opposed to this method itself asserting.

[jam, 2017/04/25 17:04:40]

"after this call" isn't clear enough?

[mmenke, 2017/04/25 17:44:44]

Oops, forgot to respond to this.  My feeling is that for clarity, methods should
have comments along the lines what they do when called, not what they will do in
the future (Or worse, what they cause other, unspecified methods to do in the
future).

So I think it's best to prefer something like "Causes/tells/makes class to
assert if <....>", for instance, to more clearly say what the method does when
called.  This also more closely matches format of other method descriptions.

[mmenke, 2017/04/25 18:10:08]

A better explanation:  This method does not "Assert if any changes are made to
the rules after this call".  It makes other methods assert if changes are made
after it is called.

[jam, 2017/04/25 18:19:37]

Done.

+  void Lock();

[mmenke, 2017/04/25 16:37:55]

Add a TODO to remove this, once it's no longer used, and to make Rule private
again when that happens?  I assume we won't need either of those, once we're
using a Mojo interface for this.

[mmenke, 2017/04/25 16:37:55]

Can we not call this lock?  Commit() or Initialize(), maybe?  (I don't like lock
because this doesn't have anything to do with a base::Lock objects)

[jam, 2017/04/25 17:04:40]

Done.

[jam, 2017/04/25 17:04:40]

Done.

+
   // HostResolverProc methods:
   int Resolve(const std::string& host,
               AddressFamily address_family,
               HostResolverFlags host_resolver_flags,
               AddressList* addrlist,
               int* os_error) override;
 
- private:
-  struct Rule;
+  struct Rule {
+    enum ResolverType {
+      kResolverTypeFail,
+      kResolverTypeSystem,
+      kResolverTypeIPLiteral,
+    };
+
+    ResolverType resolver_type;
+    std::string host_pattern;
+    AddressFamily address_family;
+    HostResolverFlags host_resolver_flags;
+    std::string replacement;
+    std::string canonical_name;

[mmenke, 2017/04/25 16:37:54]

include <string> (Should have been included already)

[jam, 2017/04/25 17:04:41]

Done.

+    int latency_ms;  // In milliseconds.
+
+    Rule(ResolverType resolver_type,
+         const std::string& host_pattern,
+         AddressFamily address_family,
+         HostResolverFlags host_resolver_flags,
+         const std::string& replacement,
+         const std::string& canonical_name,
+         int latency_ms);
+    Rule(const Rule& other);

[mmenke, 2017/04/25 16:37:56]

Per style guide, constructor/destructor should go before member variables.

[jam, 2017/04/25 17:04:40]

Done.

+  };
+
   typedef std::list<Rule> RuleList;
 
+  RuleList GetRules();
+
+ private:
   ~RuleBasedHostResolverProc() override;
 
   void AddRuleInternal(const Rule& rule);
 
   RuleList rules_;
 
   // Must be obtained before writing to or reading from |rules_|.
   base::Lock rule_lock_;
+
+  // Whether changes are allowed.
+  bool locked_;

[mmenke, 2017/04/25 16:37:55]

Hrm...Should this be protected by the lock as well?  Currently, it's actually
safe to add a rule on any thread.  This CL changes that.  That's probably not an
expected use-case of this class, but I'm not sure whether or not any test is
actually doing it.

[jam, 2017/04/25 17:04:40]

this will be modified before any threading happens by definition (since if there
are races with locking and new rules being added, then the network service would
miss some rules)

[mmenke, 2017/04/25 17:29:46]

SGTM (Particularly with the above TODO)

 };
 
 // Create rules that map all requests to localhost.
 RuleBasedHostResolverProc* CreateCatchAllHostResolverProc();
 
 // HangingHostResolver never completes its |Resolve| request.
 class HangingHostResolver : public HostResolver {
  public:
   int Resolve(const RequestInfo& info,
               RequestPriority priority,
@@ skipping 25 matching lines @@
   void Init(HostResolverProc* proc);
 
  private:
   scoped_refptr<HostResolverProc> current_proc_;
   scoped_refptr<HostResolverProc> previous_proc_;
 };
 
 }  // namespace net
 
 #endif  // NET_DNS_MOCK_HOST_RESOLVER_H_