sync: Refactor per-datatype throttling

sync: Refactor per-datatype throttling

This CL pulls the code to track throttled data types out of the sync
session context and into a class meant for that purpose.  This new
class, ThrottledDataTypeTracker, also implements code to notify the
AllStatus object whenever the set of throttled datatypes is changed.

The fact that ThrottledDataTypeTracker, which lives in sync/engine,
references AllStatus caused some problems with DEPS checks.  After a few
iterations during code review, this commit now includes the following
additional changes:
- Move all_status.{cc,h} from sync/internal_api to sync/engine.
- Move the SyncManager::Status inner class out of SyncManager and into
  sync/internal_api/public/engine/sync_status.{cc,h}.  The class has
been renamed to SyncStatus.

This CL does not include code to expose the throttled status on the
chrome://sync page, though it does contain functionality we will use to
implement it in a future commit.

BUG=125065
TEST=

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=141992

[rlarocque, 2012-06-01 01:20:23]

The goal of this CL is to make the list of throttled types (and maybe their
expiration times, too) available through AllStatus.

I've implemented it by creating a class to moving this state out of
SyncSessionContext and into a new DatatypeThrottlingManager class.  The lifetime
of this class is similar to other types available in the context but owned by
the SyncManager, ie. debug_info_event_listener_ or traffic_recorder_.  

The DatatypeThrottlineManager is somewhat unique in that it can manage its own
interaction with AllStatus.  Usually AllStatus is accessed only through the
SyncManager.  The DatatypeThrottlingManager has a constructor which takes a
pointer to a (not owned) AllStatus object.

Please skim over this CL and let me know if the approach looks reasonable.  If
it looks OK, I'll upload a better version and upgrade this to a real code
review.

[tim (not reviewing), 2012-06-11 18:53:45]

http://codereview.chromium.org/10454105/diff/3001/sync/engine/syncer_proto_ut...
File sync/engine/syncer_proto_util.cc (right):

http://codereview.chromium.org/10454105/diff/3001/sync/engine/syncer_proto_ut...
sync/engine/syncer_proto_util.cc:9: #include
"sync/engine/throttled_data_type_tracker.h"
include order

(tools/sort-headers.py is great!)

http://codereview.chromium.org/10454105/diff/3001/sync/engine/syncer_proto_ut...
sync/engine/syncer_proto_util.cc:217: ThrottledDataTypeTracker* tdtt,
We should try to use as meaningful a name as possible, maybe |tracker| or
|throttler|?

http://codereview.chromium.org/10454105/diff/3001/sync/engine/syncer_proto_ut...
File sync/engine/syncer_proto_util_unittest.cc (right):

http://codereview.chromium.org/10454105/diff/3001/sync/engine/syncer_proto_ut...
sync/engine/syncer_proto_util_unittest.cc:287:
SyncerProtoUtil::HandleThrottleError(error, ticks, NULL, &delegate);
I don't think we have to pass in NULL here?  Could we pass in a tracker and
check that there's no unthrottle time?

http://codereview.chromium.org/10454105/diff/3001/sync/engine/throttled_data_...
File sync/engine/throttled_data_type_tracker.cc (right):

http://codereview.chromium.org/10454105/diff/3001/sync/engine/throttled_data_...
sync/engine/throttled_data_type_tracker.cc:5: #include
"sync/engine/throttled_data_type_tracker.h"
nit - usually have a newline following the header include 
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Functi...

http://codereview.chromium.org/10454105/diff/3001/sync/engine/throttled_data_...
sync/engine/throttled_data_type_tracker.cc:22: const base::TimeTicks& time) {
nit - indent

http://codereview.chromium.org/10454105/diff/3001/sync/engine/throttled_data_...
File sync/engine/throttled_data_type_tracker.h (right):

http://codereview.chromium.org/10454105/diff/3001/sync/engine/throttled_data_...
sync/engine/throttled_data_type_tracker.h:20:
ThrottledDataTypeTracker(AllStatus* allstatus);
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.

http://codereview.chromium.org/10454105/diff/3001/sync/engine/throttled_data_...
sync/engine/throttled_data_type_tracker.h:24: void
SetUnthrottleTime(syncable::ModelTypeSet types,
Comments on these functions would be nice.

http://codereview.chromium.org/10454105/diff/3001/sync/engine/throttled_data_...
sync/engine/throttled_data_type_tracker.h:41: AllStatus* allstatus_;
DISALLOW_COPY_AND_ASSIGN

http://codereview.chromium.org/10454105/diff/3001/sync/engine/throttled_data_...
File sync/engine/throttled_data_type_tracker_unittest.cc (right):

http://codereview.chromium.org/10454105/diff/3001/sync/engine/throttled_data_...
sync/engine/throttled_data_type_tracker_unittest.cc:34:
EXPECT_TRUE(throttler.GetThrottledTypes().Empty());
It'd be nice to have a test that doesn't wind up pruning all types.

http://codereview.chromium.org/10454105/diff/3001/sync/internal_api/all_status.h
File sync/internal_api/all_status.h (right):

http://codereview.chromium.org/10454105/diff/3001/sync/internal_api/all_statu...
sync/internal_api/all_status.h:54: void SetThrottledTypes(syncable::ModelTypeSet
types);
I'm not sure why SetEncryptedTypes doesn't use const& already, but we should
here (and there, most likely).

http://codereview.chromium.org/10454105/diff/3001/sync/sessions/sync_session_...
File sync/sessions/sync_session_context.h (left):

http://codereview.chromium.org/10454105/diff/3001/sync/sessions/sync_session_...
sync/sessions/sync_session_context.h:64: virtual ~SyncSessionContext();
Was MockSyncSessionContext in syncer_proto_util_unittest the only subclass?

http://codereview.chromium.org/10454105/diff/3001/sync/sync.gyp
File sync/sync.gyp (right):

http://codereview.chromium.org/10454105/diff/3001/sync/sync.gyp#newcode48
sync/sync.gyp:48: 'engine/throttled_data_type_tracker.cc',
abc order

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

Fixed several issues.  PTAL.

http://codereview.chromium.org/10454105/diff/3001/sync/engine/syncer_proto_ut...
File sync/engine/syncer_proto_util.cc (right):

http://codereview.chromium.org/10454105/diff/3001/sync/engine/syncer_proto_ut...
sync/engine/syncer_proto_util.cc:9: #include
"sync/engine/throttled_data_type_tracker.h"
On 2012/06/11 18:53:46, timsteele wrote:
> include order
> 
> (tools/sort-headers.py is great!)

Done.

http://codereview.chromium.org/10454105/diff/3001/sync/engine/syncer_proto_ut...
sync/engine/syncer_proto_util.cc:217: ThrottledDataTypeTracker* tdtt,
On 2012/06/11 18:53:46, timsteele wrote:
> We should try to use as meaningful a name as possible, maybe |tracker| or
> |throttler|?

Done.

http://codereview.chromium.org/10454105/diff/3001/sync/engine/syncer_proto_ut...
File sync/engine/syncer_proto_util_unittest.cc (right):

http://codereview.chromium.org/10454105/diff/3001/sync/engine/syncer_proto_ut...
sync/engine/syncer_proto_util_unittest.cc:287:
SyncerProtoUtil::HandleThrottleError(error, ticks, NULL, &delegate);
On 2012/06/11 18:53:46, timsteele wrote:
> I don't think we have to pass in NULL here?  Could we pass in a tracker and
> check that there's no unthrottle time?

Will fix.  

I was reluctant at first, since the fact that the per-datatype throttling set
should be empty when general throttling is applied is a bit unintuitive, and an
implementation detail.

On the other hand, if we ever do decide to change that behaviour, we probably
should update this test, so it's not so bad if it would start failing.

http://codereview.chromium.org/10454105/diff/3001/sync/engine/throttled_data_...
File sync/engine/throttled_data_type_tracker.cc (right):

http://codereview.chromium.org/10454105/diff/3001/sync/engine/throttled_data_...
sync/engine/throttled_data_type_tracker.cc:5: #include
"sync/engine/throttled_data_type_tracker.h"
On 2012/06/11 18:53:46, timsteele wrote:
> nit - usually have a newline following the header include 
>
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Functi...

Done.

http://codereview.chromium.org/10454105/diff/3001/sync/engine/throttled_data_...
sync/engine/throttled_data_type_tracker.cc:22: const base::TimeTicks& time) {
On 2012/06/11 18:53:46, timsteele wrote:
> nit - indent

Done.

http://codereview.chromium.org/10454105/diff/3001/sync/engine/throttled_data_...
File sync/engine/throttled_data_type_tracker.h (right):

http://codereview.chromium.org/10454105/diff/3001/sync/engine/throttled_data_...
sync/engine/throttled_data_type_tracker.h:20:
ThrottledDataTypeTracker(AllStatus* allstatus);
On 2012/06/11 18:53:46, timsteele wrote:
> 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.

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.

http://codereview.chromium.org/10454105/diff/3001/sync/engine/throttled_data_...
sync/engine/throttled_data_type_tracker.h:24: void
SetUnthrottleTime(syncable::ModelTypeSet types,
On 2012/06/11 18:53:46, timsteele wrote:
> Comments on these functions would be nice.

Done.

http://codereview.chromium.org/10454105/diff/3001/sync/engine/throttled_data_...
sync/engine/throttled_data_type_tracker.h:41: AllStatus* allstatus_;
On 2012/06/11 18:53:46, timsteele wrote:
> DISALLOW_COPY_AND_ASSIGN

Done.

http://codereview.chromium.org/10454105/diff/3001/sync/engine/throttled_data_...
File sync/engine/throttled_data_type_tracker_unittest.cc (right):

http://codereview.chromium.org/10454105/diff/3001/sync/engine/throttled_data_...
sync/engine/throttled_data_type_tracker_unittest.cc:34:
EXPECT_TRUE(throttler.GetThrottledTypes().Empty());
On 2012/06/11 18:53:46, timsteele wrote:
> It'd be nice to have a test that doesn't wind up pruning all types.

Good point.  Done.

http://codereview.chromium.org/10454105/diff/3001/sync/internal_api/all_status.h
File sync/internal_api/all_status.h (right):

http://codereview.chromium.org/10454105/diff/3001/sync/internal_api/all_statu...
sync/internal_api/all_status.h:54: void SetThrottledTypes(syncable::ModelTypeSet
types);
On 2012/06/11 18:53:46, timsteele wrote:
> I'm not sure why SetEncryptedTypes doesn't use const& already, but we should
> here (and there, most likely).

Done.  

I fixed SetEncryptedTypes, too.  I feel bad about merging an unrelated change
into this CL, but I doubt I would ever bother to fix it otherwise.

http://codereview.chromium.org/10454105/diff/3001/sync/sessions/sync_session_...
File sync/sessions/sync_session_context.h (left):

http://codereview.chromium.org/10454105/diff/3001/sync/sessions/sync_session_...
sync/sessions/sync_session_context.h:64: virtual ~SyncSessionContext();
On 2012/06/11 18:53:46, timsteele wrote:
> Was MockSyncSessionContext in syncer_proto_util_unittest the only subclass?

I assumed so, since a grep for 'public' and 'SyncSessionContext' turned up
nothing.

http://codereview.chromium.org/10454105/diff/12002/sync/engine/throttled_data...
File sync/engine/throttled_data_type_tracker_unittest.cc (right):

http://codereview.chromium.org/10454105/diff/12002/sync/engine/throttled_data...
sync/engine/throttled_data_type_tracker_unittest.cc:48:
TEST(ThrottledDataTypeTrackerTest, DISABLED_UnthrottleSomeTypesTest) {
This test had to be disabled because it was failing.  It turns out that the code
to throttle data types is willing to override a long throttle interval with a
shorter one.  Do you know if that is the intended behaviour?

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

http://codereview.chromium.org/10454105/diff/3001/sync/engine/throttled_data_...
File sync/engine/throttled_data_type_tracker.h (right):

http://codereview.chromium.org/10454105/diff/3001/sync/engine/throttled_data_...
sync/engine/throttled_data_type_tracker.h:20:
ThrottledDataTypeTracker(AllStatus* allstatus);
On 2012/06/11 23:28:51, rlarocque wrote:
> On 2012/06/11 18:53:46, timsteele wrote:
> > 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.
> 
> 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.  

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.

http://codereview.chromium.org/10454105/diff/12002/sync/engine/throttled_data...
File sync/engine/throttled_data_type_tracker_unittest.cc (right):

http://codereview.chromium.org/10454105/diff/12002/sync/engine/throttled_data...
sync/engine/throttled_data_type_tracker_unittest.cc:48:
TEST(ThrottledDataTypeTrackerTest, DISABLED_UnthrottleSomeTypesTest) {
On 2012/06/11 23:28:51, rlarocque wrote:
> This test had to be disabled because it was failing.  It turns out that the
code
> to throttle data types is willing to override a long throttle interval with a
> shorter one.  Do you know if that is the intended behaviour?

Hmm. On one hand, I could see the logic being "server gets full control / say",
but if we're throttled for a long period we shouldn't ever hear back from the
server for that datatype anyway, so I don't see how we'd hit the short-override
case in practice.  Maybe it's a good idea to explicitly code against it,
defensively?

[rlarocque, 2012-06-12 18:11:43]

I've resolved the issue with the unit test.  PTAL.

http://codereview.chromium.org/10454105/diff/12002/sync/engine/throttled_data...
File sync/engine/throttled_data_type_tracker_unittest.cc (right):

http://codereview.chromium.org/10454105/diff/12002/sync/engine/throttled_data...
sync/engine/throttled_data_type_tracker_unittest.cc:48:
TEST(ThrottledDataTypeTrackerTest, DISABLED_UnthrottleSomeTypesTest) {
On 2012/06/12 00:12:20, timsteele wrote:
> On 2012/06/11 23:28:51, rlarocque wrote:
> > This test had to be disabled because it was failing.  It turns out that the
> code
> > to throttle data types is willing to override a long throttle interval with
a
> > shorter one.  Do you know if that is the intended behaviour?
> 
> Hmm. On one hand, I could see the logic being "server gets full control /
say",
> but if we're throttled for a long period we shouldn't ever hear back from the
> server for that datatype anyway, so I don't see how we'd hit the
short-override
> case in practice.  Maybe it's a good idea to explicitly code against it,
> defensively?

The client code was written in a way that suggests server-specified throttle
timeouts were the norm.  Some grepping through the server code indicates that
this is not the case.  You're right, we're very unlikely to hit this in
practice.  

I'll remove BOOKMARKS from the short_throttled_types so this test can pass.

[tim (not reviewing), 2012-06-12 20:56:43]

LGTM
On Jun 11, 2012 8:12 PM, <tim@chromium.org> wrote:

>
> http://codereview.chromium.**org/10454105/diff/3001/sync/**
>
engine/throttled_data_type_**tracker.h<http://codereview.chromium.org/10454105/diff/3001/sync/engine/throttled_data_type_tracker.h>
> File sync/engine/throttled_data_**type_tracker.h (right):
>
> http://codereview.chromium.**org/10454105/diff/3001/sync/**
>
engine/throttled_data_type_**tracker.h#newcode20<http://codereview.chromium.org/10454105/diff/3001/sync/engine/throttled_data_type_tracker.h#newcode20>
> sync/engine/throttled_data_**type_tracker.h:20:
> ThrottledDataTypeTracker(**AllStatus* allstatus);
> On 2012/06/11 23:28:51, rlarocque wrote:
>
>> On 2012/06/11 18:53:46, timsteele wrote:
>> > 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=Function_**Overloading#Function_**Overloading<http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Function_Overloading#Function_Overloading>
> )
>
>> >
>> >
>> > 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.
>>
>
>  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.
>>
>
> 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.
>
> http://codereview.chromium.**org/10454105/diff/12002/sync/**
>
engine/throttled_data_type_**tracker_unittest.cc<http://codereview.chromium.org/10454105/diff/12002/sync/engine/throttled_data_type_tracker_unittest.cc>
> File sync/engine/throttled_data_**type_tracker_unittest.cc (right):
>
> http://codereview.chromium.**org/10454105/diff/12002/sync/**
>
engine/throttled_data_type_**tracker_unittest.cc#newcode48<http://codereview.chromium.org/10454105/diff/12002/sync/engine/throttled_data_type_tracker_unittest.cc#newcode48>
> sync/engine/throttled_data_**type_tracker_unittest.cc:48:
> TEST(**ThrottledDataTypeTrackerTest, DISABLED_**UnthrottleSomeTypesTest) {
> On 2012/06/11 23:28:51, rlarocque wrote:
>
>> This test had to be disabled because it was failing.  It turns out
>>
> that the code
>
>> to throttle data types is willing to override a long throttle interval
>>
> with a
>
>> shorter one.  Do you know if that is the intended behaviour?
>>
>
> Hmm. On one hand, I could see the logic being "server gets full control
> / say", but if we're throttled for a long period we shouldn't ever hear
> back from the server for that datatype anyway, so I don't see how we'd
> hit the short-override case in practice.  Maybe it's a good idea to
> explicitly code against it, defensively?
>
>
http://codereview.chromium.**org/10454105/<http://codereview.chromium.org/104...
>

[commit-bot: I haz the power, 2012-06-12 21:00:27]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/rlarocque@chromium.org/10454105/5003

[commit-bot: I haz the power, 2012-06-12 21:41:26]

Try job failure for 10454105-5003 (retry) on mac_rel for step "check_deps".
It's a second try, previously, step "check_deps" failed.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=mac_rel&nu...

[rlarocque, 2012-06-12 23:35:54]

On 2012/06/12 21:41:26, I haz the power (commit-bot) wrote:
> Try job failure for 10454105-5003 (retry) on mac_rel for step "check_deps".
> It's a second try, previously, step "check_deps" failed.
>
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=mac_rel&nu...

The dependency checker doesn't like that
sync/engine/throttled_data_type_tracker.cc is including
sync/internal_api/all_status.h.  

Tim, would it be OK to make an exception to the rule specifically for
all_status.h?  If not, is there some interal_api/public folder to which we could
move all_status.h?  

AllStatus is small and relatively simple.  I wouldn't see any harm in allowing
it up to be included from more locations.  

The alternative would be to find some way to re-route the throttled data type
updates back to SyncManager (probably via OnSyncEngineEvent) then have it send
the update to AllStatus.  I'd prefer to not do that.

[tim (not reviewing), 2012-06-13 01:25:47]

I think what you want here is to move all_status out of internal_api and
into something like engine (or another suitable sync/ core folder). Not
internal_api/public though - public is for components other than sync to
include, and that isn't the problem here (as far as I can tell). We
shouldn't hoist things there unnecessarily.

On Tue, Jun 12, 2012 at 4:35 PM, <rlarocque@chromium.org> wrote:

> On 2012/06/12 21:41:26, I haz the power (commit-bot) wrote:
>
>> Try job failure for 10454105-5003 (retry) on mac_rel for step
>> "check_deps".
>> It's a second try, previously, step "check_deps" failed.
>>
>
> http://build.chromium.org/p/**tryserver.chromium/**
>
buildstatus?builder=mac_rel&**number=31957<http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=mac_rel&number=31957>
>
> The dependency checker doesn't like that
> sync/engine/throttled_data_**type_tracker.cc is including
> sync/internal_api/all_status.**h.
>
> Tim, would it be OK to make an exception to the rule specifically for
> all_status.h?  If not, is there some interal_api/public folder to which we
> could
> move all_status.h?
>
> AllStatus is small and relatively simple.  I wouldn't see any harm in
> allowing
> it up to be included from more locations.
>
> The alternative would be to find some way to re-route the throttled data
> type
> updates back to SyncManager (probably via OnSyncEngineEvent) then have it
> send
> the update to AllStatus.  I'd prefer to not do that.
>
>
https://chromiumcodereview.**appspot.com/10454105/<https://chromiumcodereview...
>

[tim (not reviewing), 2012-06-13 01:27:12]

Note that sync_manager.cc is allowed to include things from sync/engine and
friends.  Only sync_manager.h will wind up in /public (if I ever land
http://codereview.chromium.org/10534080/).

On Tue, Jun 12, 2012 at 6:25 PM, Tim Steele <tim@chromium.org> wrote:

> I think what you want here is to move all_status out of internal_api and
> into something like engine (or another suitable sync/ core folder). Not
> internal_api/public though - public is for components other than sync to
> include, and that isn't the problem here (as far as I can tell). We
> shouldn't hoist things there unnecessarily.
>
> On Tue, Jun 12, 2012 at 4:35 PM, <rlarocque@chromium.org> wrote:
>
>> On 2012/06/12 21:41:26, I haz the power (commit-bot) wrote:
>>
>>> Try job failure for 10454105-5003 (retry) on mac_rel for step
>>> "check_deps".
>>> It's a second try, previously, step "check_deps" failed.
>>>
>>
>> http://build.chromium.org/p/**tryserver.chromium/**
>>
buildstatus?builder=mac_rel&**number=31957<http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=mac_rel&number=31957>
>>
>> The dependency checker doesn't like that
>> sync/engine/throttled_data_**type_tracker.cc is including
>> sync/internal_api/all_status.**h.
>>
>> Tim, would it be OK to make an exception to the rule specifically for
>> all_status.h?  If not, is there some interal_api/public folder to which
>> we could
>> move all_status.h?
>>
>> AllStatus is small and relatively simple.  I wouldn't see any harm in
>> allowing
>> it up to be included from more locations.
>>
>> The alternative would be to find some way to re-route the throttled data
>> type
>> updates back to SyncManager (probably via OnSyncEngineEvent) then have it
>> send
>> the update to AllStatus.  I'd prefer to not do that.
>>
>>
https://chromiumcodereview.**appspot.com/10454105/<https://chromiumcodereview...
>>
>
>

[rlarocque, 2012-06-13 18:33:21]

Here is one possible solution to the checkdeps issues.

1. Move all_status.{cc,h} from internal_api to engine.  
2. Extract the nested class SyncManager::Status out of SyncManager and put it
into internal_api/public/engine/sync_status.h under the name SyncStatus.  I'm
not sure 'public/engine' was the right choice; it seemed the least bad place to
put it.

[tim (not reviewing), 2012-06-13 19:06:27]

That seems reasonable. This kind of hits an old annoyance of mine in the
face, having multiple "status" concepts that all kind of sound the same.
The purpose of this one has historically been for instantaneous UI
consumption and debugging, while we have SyncerStatus and
SyncSessionSnapshot under public/sessions for programmatic use in response
to a particular Session that took place.

On one hand it might make sense to keep it alongside the others
(/public/sessions) to force us to differentiate / group accordingly (now or
later). Engine/ is not bad though - You should revamp the comments in your
new file to reflect the intended audience / how it differs from the others.
Maybe Sync[Engine]StatusReport[ForUI] are better (less generic sounding)
names?

On Wed, Jun 13, 2012 at 11:33 AM, <rlarocque@chromium.org> wrote:

> Here is one possible solution to the checkdeps issues.
>
> 1. Move all_status.{cc,h} from internal_api to engine.
> 2. Extract the nested class SyncManager::Status out of SyncManager and put
> it
> into internal_api/public/engine/**sync_status.h under the name
> SyncStatus.  I'm
> not sure 'public/engine' was the right choice; it seemed the least bad
> place to
> put it.
>
>
http://codereview.chromium.**org/10454105/<http://codereview.chromium.org/104...
>

[rlarocque, 2012-06-13 19:55:35]

I don't think SyncStatus is such a bad name for this class.  I suppose we could
rename it to SyncEngineStatus, but I don't think that does much do disambiguate
it from SyncerStatus.

I think the right course of action would be to rename SyncerStatus, or maybe
merge it into SyncSessionSnapshot.  Could we deal with this in a future commit? 


On 2012/06/13 19:06:27, timsteele wrote:
> That seems reasonable. This kind of hits an old annoyance of mine in the
> face, having multiple "status" concepts that all kind of sound the same.
> The purpose of this one has historically been for instantaneous UI
> consumption and debugging, while we have SyncerStatus and
> SyncSessionSnapshot under public/sessions for programmatic use in response
> to a particular Session that took place.
> 
> On one hand it might make sense to keep it alongside the others
> (/public/sessions) to force us to differentiate / group accordingly (now or
> later). Engine/ is not bad though - You should revamp the comments in your
> new file to reflect the intended audience / how it differs from the others.
> Maybe Sync[Engine]StatusReport[ForUI] are better (less generic sounding)
> names?
> 
> On Wed, Jun 13, 2012 at 11:33 AM, <mailto:rlarocque@chromium.org> wrote:
> 
> > Here is one possible solution to the checkdeps issues.
> >
> > 1. Move all_status.{cc,h} from internal_api to engine.
> > 2. Extract the nested class SyncManager::Status out of SyncManager and put
> > it
> > into internal_api/public/engine/**sync_status.h under the name
> > SyncStatus.  I'm
> > not sure 'public/engine' was the right choice; it seemed the least bad
> > place to
> > put it.
> >
> >
>
http://codereview.chromium.**org/10454105/%3Chttp://codereview.chromium.org/1...>
> >

[rlarocque, 2012-06-13 19:56:16]

(Also, updated some comments.  PTAL)

[tim (not reviewing), 2012-06-13 20:05:18]

Yeah, enough for now :) Sorry for the back-and-forth.
LGTM!

[commit-bot: I haz the power, 2012-06-13 20:20:48]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/rlarocque@chromium.org/10454105/9027

[commit-bot: I haz the power, 2012-06-13 22:14:06]

Change committed as 141992