Domain Reliability: More security review.

components/domain_reliability/monitor.h

 // Copyright 2014 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 COMPONENTS_DOMAIN_RELIABILITY_MONITOR_H_
 #define COMPONENTS_DOMAIN_RELIABILITY_MONITOR_H_
 
 #include <map>
 
 #include "base/memory/ref_counted.h"
@@ skipping 13 matching lines @@
 namespace net {
 class URLRequest;
 class URLRequestContext;
 class URLRequestContextGetter;
 }
 
 namespace domain_reliability {
 
 // The top-level per-profile object that measures requests and hands off the
 // measurements to the proper |DomainReliabilityContext|.  Referenced by the
 // |ChromeNetworkDelegate|, which calls the On* methods.

[Ryan Sleevi, 2014/04/26 02:37:39]

You're a component now, you don't get to talk about ChromeNetworkDelegate
(that's a layering violation).

Instead, describe this class, it's requirements (eg: that it live on the same
thread as the net::URLRequestContext, which must outlive it), and how to use it
(eg: implement a net::NetworkDelegate that calls the On* methods as appropriate)

[Deprecated (see juliatuttle), 2014/04/28 22:01:13]

Done.

 class DOMAIN_RELIABILITY_EXPORT DomainReliabilityMonitor {
  public:
   // NB: We don't take a URLRequestContextGetter because we already live on the
   // I/O thread.

[Ryan Sleevi, 2014/04/26 02:37:39]

NB: Pronouns in comments considered harmful :)

https://groups.google.com/a/chromium.org/d/topic/chromium-dev/NH-S6KCkr2M/dis...

Rather than discuss this in the constructor, document it in the class design
itself

[Deprecated (see juliatuttle), 2014/04/28 22:01:13]

Done.

   explicit DomainReliabilityMonitor(
       net::URLRequestContext* url_request_context);
-  DomainReliabilityMonitor(
-      net::URLRequestContext* url_request_context,
-      scoped_ptr<MockableTime> time);
+  DomainReliabilityMonitor(net::URLRequestContext* url_request_context,
+                           scoped_ptr<MockableTime> time);
   ~DomainReliabilityMonitor();
 
   // Adds the "baked-in" configuration(s) for Google sites.

[Ryan Sleevi, 2014/04/26 02:37:39]

// Add in configurations that have been configured at compile-time.

Yes, as the reviewer who stamped this, I'm a horrible person :)

[Deprecated (see juliatuttle), 2014/04/28 22:01:13]

Done.

   void AddBakedInConfigs();
 
   // Should be called from the profile's NetworkDelegate on the corresponding
   // events:

[Ryan Sleevi, 2014/04/26 02:37:39]

Components don't get to talk about profiles either (AIUI)

Ideally, however, you also wouldn't want to talk about NetworkDelegate - it's
not an API contract of yours that users implement in terms of NetworkDelegate -
it just happens to be the most convenient way, and that has a 1:1 mapping.

Instead, describe *your* contract and expectations for these methods to be
called. What are your exact pre-conditions/post-conditions?

[Deprecated (see juliatuttle), 2014/04/28 22:01:13]

Done.

   void OnBeforeRedirect(net::URLRequest* request);
   void OnCompleted(net::URLRequest* request, bool started);
 
   DomainReliabilityContext* AddContextForTesting(
       scoped_ptr<const DomainReliabilityConfig> config);
 
   size_t contexts_size_for_testing() const { return contexts_.size(); }
 
  private:
   friend class DomainReliabilityMonitorTest;
 
   typedef std::map<std::string, DomainReliabilityContext*> ContextMap;
 
   struct DOMAIN_RELIABILITY_EXPORT RequestInfo {
     RequestInfo();
     RequestInfo(const net::URLRequest& request);

[Ryan Sleevi, 2014/04/26 02:37:39]

style: explicit

[Deprecated (see juliatuttle), 2014/04/28 22:01:13]

Done.

     ~RequestInfo();
 
     bool DefinitelyReachedNetwork() const;
 
     GURL url;
     net::URLRequestStatus status;
     int response_code;
     net::HostPortPair socket_address;
     net::LoadTimingInfo load_timing_info;
     bool was_cached;
     int load_flags;
   };
 
   // Creates a context, adds it to the monitor, and returns a pointer to it.
   // (The pointer is only valid until the Monitor is destroyed.)
   DomainReliabilityContext* AddContext(
       scoped_ptr<const DomainReliabilityConfig> config);
   void OnRequestLegComplete(const RequestInfo& info);
 
   scoped_ptr<MockableTime> time_;
   scoped_refptr<net::URLRequestContextGetter> url_request_context_getter_;

[Ryan Sleevi, 2014/04/26 02:37:39]

You don't take one (because of threading), but you live on one? What's up with
that?

[Deprecated (see juliatuttle), 2014/04/28 22:01:13]

URLFetcher requires a URLRequestContextGetter, since (IIRC) it can be managed on
any thread. I don't want to burden *my* callers with making one, so I take a
context directly, but I do need to create a trivial one (see .cc for details) to
pass to the URLFetcher.

   DomainReliabilityScheduler::Params scheduler_params_;
   DomainReliabilityDispatcher dispatcher_;
   scoped_ptr<DomainReliabilityUploader> uploader_;
   ContextMap contexts_;
 
   DISALLOW_COPY_AND_ASSIGN(DomainReliabilityMonitor);
 };
 
 }  // namespace domain_reliability
 
 #endif  // COMPONENTS_DOMAIN_RELIABILITY_MONITOR_H_