Domain Reliability: More security review.

components/domain_reliability/context.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_CONTEXT_H_
 #define COMPONENTS_DOMAIN_RELIABILITY_CONTEXT_H_
 
 #include <deque>
 #include <map>

[Ryan Sleevi, 2014/04/25 01:00:02]

IWYU: vector (line 48)
DIWYDU: delete map - you don't use it.

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

Done.

 
 #include "base/memory/ref_counted.h"

[Ryan Sleevi, 2014/04/25 01:00:02]

IWYU: I don't see any refcounting going on here.

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

Whoops. This used to take a URLRequestContextGetter, I think, which was
refcounted.

 #include "base/memory/scoped_ptr.h"
 #include "base/memory/weak_ptr.h"
 #include "base/time/time.h"
 #include "components/domain_reliability/beacon.h"
 #include "components/domain_reliability/config.h"

[Ryan Sleevi, 2014/04/25 01:00:02]

Normally you'd be able to forward declare this, if ::Resource wasn't an inner
class (see http://www.chromium.org/developers/coding-style/cpp-dos-and-donts )

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

Are you saying I should rearrange it so I can?

 #include "components/domain_reliability/domain_reliability_export.h"
 #include "components/domain_reliability/scheduler.h"
 #include "components/domain_reliability/uploader.h"
 #include "components/domain_reliability/util.h"
 
 class GURL;
 
 namespace domain_reliability {
 
 class DomainReliabilityDispatcher;
 class MockableTime;
 
 // The per-domain context for the Domain Reliability client; includes the
 // domain's config and per-resource beacon queues.
 class DOMAIN_RELIABILITY_EXPORT DomainReliabilityContext {
  public:
   DomainReliabilityContext(
       MockableTime* time,

[Ryan Sleevi, 2014/04/25 01:00:02]

nit: Should "MockableTime" be called what it is, rather than how it can be used
- eg: TimeSource (which is what we've used in things like cc/ and the
base/profiler code) or TimeProvider (used in base/ and extensions/)

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

Yeah, we might be able to come up with a nicer name for it. I'll add a TODO to
rename it.

       const DomainReliabilityScheduler::Params& scheduler_params,
       DomainReliabilityDispatcher* dispatcher,
       DomainReliabilityUploader* uploader,
       scoped_ptr<const DomainReliabilityConfig> config);
   virtual ~DomainReliabilityContext();

[Ryan Sleevi, 2014/04/25 01:00:02]

Why is this virtual, when you have no virtual methods and are not implementing
any interfaces?

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

Done.

 
+  // Adds a beacon; may or may not save the actual beacon to be uploaded,
+  // depending on the sample rates in the config, but will increment one of
+  // the request counters in any case.

[Ryan Sleevi, 2014/04/25 01:00:02]

Should this be called "RegisterBeacon" instead? Or something more event-notify'y
like "OnBeacon"?

The "add" in AddBeacon seems like it will take some concrete step with the
Beacon being added to something, but that's not entirely the case. Yes, you
increment the request counters, but you don't add the beacon to be uploaded,
which is the first thing I think of with "AddBeacon"

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

Yeah, I've never been happy with this name. I think OnBeacon is better enough.

   void AddBeacon(const DomainReliabilityBeacon& beacon, const GURL& url);
 
   void GetQueuedDataForTesting(
       int resource_index,
       std::vector<DomainReliabilityBeacon>* beacons_out,
       int* successful_requests_out,
       int* failed_requests_out) const;

[Ryan Sleevi, 2014/04/25 01:00:02]

For "counts of things" and indices, use unsigned integer types

See http://www.chromium.org/developers/coding-style#TOC-Types

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

Done.

 
   const DomainReliabilityConfig& config() { return *config_.get(); }
 
   // Maximum number of beacons queued per context; if more than this many are
   // queued; the oldest beacons will be removed.
   static const int kMaxQueuedBeacons;

[Ryan Sleevi, 2014/04/25 01:00:02]

For "counts of things", use unsigned integer types

See http://www.chromium.org/developers/coding-style#TOC-Types

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

Done.

 
  private:
   // Resource-specific state (queued beacons and request counts).
   class ResourceState {

[Ryan Sleevi, 2014/04/25 01:00:02]

Because you the ScopedVector<> isn't instantiated until the ctor/dtor (in the
.cc), and you have copy and assignment disabled, you should be able to forward
declare this entire class.

That would also allow you to forward declare DomainReliabilityBeacon, since
std::deque<> is the only thing preventing it from being so.

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

Done.

    public:
     ResourceState(DomainReliabilityContext* context,
                   const DomainReliabilityConfig::Resource* config);
     ~ResourceState();
 
     scoped_ptr<base::Value> ToValue(base::TimeTicks upload_time) const;
 
     // Remembers the current state of the resource data when an upload starts.
     void MarkUpload();
 
     // Uses the state remembered by |MarkUpload| to remove successfully uploaded
     // data but keep beacons and request counts added after the upload started.
     void CommitUpload();
 
     // Gets the start time of the oldest beacon, if there are any. Returns true
     // and sets |oldest_start_out| if so; otherwise, returns false.
     bool GetOldestBeaconStart(base::TimeTicks* oldest_start_out) const;
     // Removes the oldest beacon. DCHECKs if there isn't one.

[Ryan Sleevi, 2014/04/25 01:00:02]

nit: newline between 77/78.

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

Done.

     void RemoveOldestBeacon();
 
     DomainReliabilityContext* context;
     const DomainReliabilityConfig::Resource* config;
 
     std::deque<DomainReliabilityBeacon> beacons;
     int successful_requests;
     int failed_requests;
 
     // State saved during uploads; if an upload succeeds, these are used to
     // remove uploaded data from the beacon list and request counters.
     size_t uploading_beacons_size;
     int uploading_successful_requests;
     int uploading_failed_requests;

[Ryan Sleevi, 2014/04/25 01:00:02]

size_t for 85, 86, 91, 92

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

Done.

 
    private:
     DISALLOW_COPY_AND_ASSIGN(ResourceState);
   };
 
   typedef ScopedVector<ResourceState> ResourceStateVector;
   typedef ResourceStateVector::const_iterator ResourceStateIterator;
 
   void InitializeResourceStates();
   void ScheduleUpload(base::TimeDelta min_delay, base::TimeDelta max_delay);
   void StartUpload();
   void OnUploadComplete(bool success);
 
   scoped_ptr<const base::Value> CreateReport(base::TimeTicks upload_time) const;
 
   // Remembers the current state of the context when an upload starts.
   void MarkUpload();
 
   // Uses the state remembered by |MarkUpload| to remove successfully uploaded
   // data but keep beacons and request counts added after the upload started.
   void CommitUpload();
 
   // Finds and removes the oldest beacon. DCHECKs if there is none. (Called
   // when there are too many beacons queued.)
   void RemoveOldestBeacon();

[Ryan Sleevi, 2014/04/25 01:00:02]

I find it weird to see 108-117 duplicated with effectively 68-79, for what it's
worth.

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

If you want, I can just have Context::MarkUpload modify the states directly?

 
   scoped_ptr<const DomainReliabilityConfig> config_;
   MockableTime* time_;
   DomainReliabilityScheduler scheduler_;
   DomainReliabilityDispatcher* dispatcher_;
   DomainReliabilityUploader* uploader_;
 
   // Each ResourceState in |states_| corresponds to the Resource of the same
   // index in the config.
   ResourceStateVector states_;
   int beacon_count_;
   int uploading_beacon_count_;

[Ryan Sleevi, 2014/04/25 01:00:02]

size_t for counts

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

Done.

   base::TimeTicks upload_time_;
   base::TimeTicks last_upload_time_;
 
   base::WeakPtrFactory<DomainReliabilityContext> weak_factory_;
 
   DISALLOW_COPY_AND_ASSIGN(DomainReliabilityContext);
 };
 
 }  // namespace domain_reliability
 
 #endif  // COMPONENTS_DOMAIN_RELIABILITY_CONTEXT_H_