sync: Refactor per-datatype throttling

sync/engine/throttled_data_type_tracker.h

Index: sync/engine/throttled_data_type_tracker.h
diff --git a/sync/engine/throttled_data_type_tracker.h b/sync/engine/throttled_data_type_tracker.h
new file mode 100644
index 0000000000000000000000000000000000000000..8f96d026993a39a64fc11aadb6f15fee73c54bf0
--- /dev/null
+++ b/sync/engine/throttled_data_type_tracker.h
@@ -0,0 +1,46 @@
+// Copyright (c) 2012 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 SYNC_ENGINE_THROTTLED_DATA_TYPE_TRACKER_H_
+#define SYNC_ENGINE_THROTTLED_DATA_TYPE_TRACKER_H_
+#pragma once
+
+#include <map>
+
+#include "base/gtest_prod_util.h"
+#include "sync/syncable/model_type.h"
+
+namespace browser_sync {
+
+class AllStatus;
+
+class ThrottledDataTypeTracker {
+ public:
+  ThrottledDataTypeTracker(AllStatus* allstatus);

[tim (not reviewing), 2012/06/11 18:53:46]

Single param ctor should be 'explicit'.  

Also overloading constructors is generally discouraged (there are some
exceptions,
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Functi...)


If this is just to allow allstatus_ to be NULL, it's clearer to the caller if
NULL is explicitly passed in (rather than having to deduce if TDTT(NULL) is the
same as TDTT().

If there's more reason behind the split (and purpose for multiple modes of
construction), I'd opt for a default ctor + a descriptively named factory
function.

[rlarocque, 2012/06/11 23:28:51]

It's just for the NULL case.  

By the way, I remember not too long ago arguing that you should implement two
different classes rather than deal with a possible NULL pointer in one of your
reviews.  

I think this case is different, since there is significant functionality shared
between the NULL and non-NULL cases.  However, I probably could be convinced to
break this into two separate classes, especially if you can think of good names
for both of them.  

[tim (not reviewing), 2012/06/12 00:12:20]

I don't honestly feel like two classes are justified here since as you say
there's a fair bit of commonality; it's just a question of whether we forward /
persist state changes to the AllStatus or not.  I think passing in NULL meaning
"don't *additionally* send changes to an allstatus" is reasonable.  

+  ThrottledDataTypeTracker();
+  ~ThrottledDataTypeTracker();
+
+  void SetUnthrottleTime(syncable::ModelTypeSet types,

[tim (not reviewing), 2012/06/11 18:53:46]

Comments on these functions would be nice.

[rlarocque, 2012/06/11 23:28:51]

Done.

+                         const base::TimeTicks& time);
+  void PruneUnthrottledTypes(const base::TimeTicks& time);
+  syncable::ModelTypeSet GetThrottledTypes() const;
+
+ private:
+  FRIEND_TEST_ALL_PREFIXES(ThrottledDataTypeTrackerTest,
+                           AddUnthrottleTimeTest);
+  FRIEND_TEST_ALL_PREFIXES(ThrottledDataTypeTrackerTest,
+                           GetCurrentlyThrottledTypesTest);
+
+  typedef std::map<syncable::ModelType, base::TimeTicks> UnthrottleTimes;
+
+  // This is a map from throttled data types to the time at which they can be
+  // unthrottled.
+  UnthrottleTimes unthrottle_times_;
+
+  AllStatus* allstatus_;

[tim (not reviewing), 2012/06/11 18:53:46]

DISALLOW_COPY_AND_ASSIGN

[rlarocque, 2012/06/11 23:28:51]

Done.

+};
+
+}  // namespace browser_sync
+
+#endif  // SYNC_ENGINE_THROTTLED_DATA_TYPE_TRACKER_H_