Split up SyncEngineEventListener callbacks

Split up SyncEngineEventListener callbacks

Splits up the SyncEngineEventListener calls into one callback per event
type.  This allow us to trim the 'event' objects so they only contain
elements relevant to their event.

Also removes some dead code related to STOP_SYNCING_PERMANENTLY, which
is no longer in use.  It may have been used for birthday errors in the
past, but those are now handled with ActionableError.  It was used for
CLEAR_USER_DATA, but that feature is no longer supported.

BUG=339984
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=250384

[rlarocque, 2014-02-07 18:47:38]

This splits up the SyncEngineEvent callbacks so we can make it more clear which
ones do or do not require SyncSessionSnapshots.

Please review.

[rlarocque, 2014-02-10 22:55:12]

ping.

[Nicolas Zea, 2014-02-10 23:15:15]

https://codereview.chromium.org/152013003/diff/150001/sync/engine/all_status.cc
File sync/engine/all_status.cc (left):

https://codereview.chromium.org/152013003/diff/150001/sync/engine/all_status....
sync/engine/all_status.cc:55:
snapshot.model_neutral_state().sync_protocol_error;
Why is this removed?

https://codereview.chromium.org/152013003/diff/150001/sync/engine/sync_engine...
File sync/engine/sync_engine_event.h (left):

https://codereview.chromium.org/152013003/diff/150001/sync/engine/sync_engine...
sync/engine/sync_engine_event.h:39: STOP_SYNCING_PERMANENTLY,
How is STOP_SYNCING_PERMANENTLY being handled now?

https://codereview.chromium.org/152013003/diff/150001/sync/engine/sync_engine...
File sync/engine/sync_engine_event.h (right):

https://codereview.chromium.org/152013003/diff/150001/sync/engine/sync_engine...
sync/engine/sync_engine_event.h:12: #include
"sync/protocol/sync_protocol_error.h"
Can forward declare this

https://codereview.chromium.org/152013003/diff/150001/sync/engine/sync_engine...
sync/engine/sync_engine_event.h:16: struct SYNC_EXPORT_PRIVATE SyncCycleEvent {
Possibly move to its own file?

https://codereview.chromium.org/152013003/diff/150001/sync/engine/sync_engine...
sync/engine/sync_engine_event.h:38: class SYNC_EXPORT_PRIVATE
SyncEngineEventListener {
This file should probably be renamed sync_engine_event_listener.h (once
SyncCycleEvent is in its own file)

https://codereview.chromium.org/152013003/diff/150001/sync/internal_api/sync_...
File sync/internal_api/sync_manager_impl.cc (left):

https://codereview.chromium.org/152013003/diff/150001/sync/internal_api/sync_...
sync/internal_api/sync_manager_impl.cc:937: if (event.what_happened ==
SyncEngineEvent::STOP_SYNCING_PERMANENTLY) {
What about this case?

[rlarocque, 2014-02-11 00:19:42]

https://codereview.chromium.org/152013003/diff/150001/sync/engine/all_status.cc
File sync/engine/all_status.cc (left):

https://codereview.chromium.org/152013003/diff/150001/sync/engine/all_status....
sync/engine/all_status.cc:55:
snapshot.model_neutral_state().sync_protocol_error;
On 2014/02/10 23:15:16, Nicolas Zea wrote:
> Why is this removed?

It's handled in OnActionableError() now.

I suppose that won't help in the case where the error is not 'Actionable'. 
However, IsActionableError() defines an 'actionable error' as error.action !=
UNKNOWN_ACTION.  That case doesn't seem very important.

https://codereview.chromium.org/152013003/diff/150001/sync/engine/sync_engine...
File sync/engine/sync_engine_event.h (left):

https://codereview.chromium.org/152013003/diff/150001/sync/engine/sync_engine...
sync/engine/sync_engine_event.h:39: STOP_SYNCING_PERMANENTLY,
On 2014/02/10 23:15:16, Nicolas Zea wrote:
> How is STOP_SYNCING_PERMANENTLY being handled now?

As far as I can tell, it's not handled at all.

According to sync_enums.proto: "|STOP_SYNCING_PERMANENTLY| event should never be
seen by the server in the absence of bugs."

That may explain why I haven't been able to find any code that would create a
SyncEngineEvent with case == STOP_SYNCING_PERMANENTLY.  I think that feature is
deprecated, and this is dead code.

See also r235696.

https://codereview.chromium.org/152013003/diff/150001/sync/engine/sync_engine...
File sync/engine/sync_engine_event.h (right):

https://codereview.chromium.org/152013003/diff/150001/sync/engine/sync_engine...
sync/engine/sync_engine_event.h:12: #include
"sync/protocol/sync_protocol_error.h"
On 2014/02/10 23:15:16, Nicolas Zea wrote:
> Can forward declare this

Done.

https://codereview.chromium.org/152013003/diff/150001/sync/engine/sync_engine...
sync/engine/sync_engine_event.h:16: struct SYNC_EXPORT_PRIVATE SyncCycleEvent {
On 2014/02/10 23:15:16, Nicolas Zea wrote:
> Possibly move to its own file?

Done.

https://codereview.chromium.org/152013003/diff/150001/sync/engine/sync_engine...
sync/engine/sync_engine_event.h:38: class SYNC_EXPORT_PRIVATE
SyncEngineEventListener {
On 2014/02/10 23:15:16, Nicolas Zea wrote:
> This file should probably be renamed sync_engine_event_listener.h (once
> SyncCycleEvent is in its own file)

Done.

https://codereview.chromium.org/152013003/diff/150001/sync/internal_api/sync_...
File sync/internal_api/sync_manager_impl.cc (left):

https://codereview.chromium.org/152013003/diff/150001/sync/internal_api/sync_...
sync/internal_api/sync_manager_impl.cc:937: if (event.what_happened ==
SyncEngineEvent::STOP_SYNCING_PERMANENTLY) {
On 2014/02/10 23:15:16, Nicolas Zea wrote:
> What about this case?

I think this is dead code.

Come to think of it, SyncManagerObserver::OnStopSyncingPermanently() is probably
dead code, too.  I'll try to remove it in a separate CL.

[rlarocque, 2014-02-11 00:24:22]

On 2014/02/11 00:19:42, rlarocque wrote:
>
https://codereview.chromium.org/152013003/diff/150001/sync/engine/all_status.cc
> File sync/engine/all_status.cc (left):
> 
>
https://codereview.chromium.org/152013003/diff/150001/sync/engine/all_status....
> sync/engine/all_status.cc:55:
> snapshot.model_neutral_state().sync_protocol_error;
> On 2014/02/10 23:15:16, Nicolas Zea wrote:
> > Why is this removed?
> 
> It's handled in OnActionableError() now.
> 
> I suppose that won't help in the case where the error is not 'Actionable'. 
> However, IsActionableError() defines an 'actionable error' as error.action !=
> UNKNOWN_ACTION.  That case doesn't seem very important.
> 
>
https://codereview.chromium.org/152013003/diff/150001/sync/engine/sync_engine...
> File sync/engine/sync_engine_event.h (left):
> 
>
https://codereview.chromium.org/152013003/diff/150001/sync/engine/sync_engine...
> sync/engine/sync_engine_event.h:39: STOP_SYNCING_PERMANENTLY,
> On 2014/02/10 23:15:16, Nicolas Zea wrote:
> > How is STOP_SYNCING_PERMANENTLY being handled now?
> 
> As far as I can tell, it's not handled at all.
> 
> According to sync_enums.proto: "|STOP_SYNCING_PERMANENTLY| event should never
be
> seen by the server in the absence of bugs."
> 
> That may explain why I haven't been able to find any code that would create a
> SyncEngineEvent with case == STOP_SYNCING_PERMANENTLY.  I think that feature
is
> deprecated, and this is dead code.
> 
> See also r235696.
> 
>
https://codereview.chromium.org/152013003/diff/150001/sync/engine/sync_engine...
> File sync/engine/sync_engine_event.h (right):
> 
>
https://codereview.chromium.org/152013003/diff/150001/sync/engine/sync_engine...
> sync/engine/sync_engine_event.h:12: #include
> "sync/protocol/sync_protocol_error.h"
> On 2014/02/10 23:15:16, Nicolas Zea wrote:
> > Can forward declare this
> 
> Done.
> 
>
https://codereview.chromium.org/152013003/diff/150001/sync/engine/sync_engine...
> sync/engine/sync_engine_event.h:16: struct SYNC_EXPORT_PRIVATE SyncCycleEvent
{
> On 2014/02/10 23:15:16, Nicolas Zea wrote:
> > Possibly move to its own file?
> 
> Done.
> 
>
https://codereview.chromium.org/152013003/diff/150001/sync/engine/sync_engine...
> sync/engine/sync_engine_event.h:38: class SYNC_EXPORT_PRIVATE
> SyncEngineEventListener {
> On 2014/02/10 23:15:16, Nicolas Zea wrote:
> > This file should probably be renamed sync_engine_event_listener.h (once
> > SyncCycleEvent is in its own file)
> 
> Done.
> 
>
https://codereview.chromium.org/152013003/diff/150001/sync/internal_api/sync_...
> File sync/internal_api/sync_manager_impl.cc (left):
> 
>
https://codereview.chromium.org/152013003/diff/150001/sync/internal_api/sync_...
> sync/internal_api/sync_manager_impl.cc:937: if (event.what_happened ==
> SyncEngineEvent::STOP_SYNCING_PERMANENTLY) {
> On 2014/02/10 23:15:16, Nicolas Zea wrote:
> > What about this case?
> 
> I think this is dead code.
> 
> Come to think of it, SyncManagerObserver::OnStopSyncingPermanently() is
probably
> dead code, too.  I'll try to remove it in a separate CL.

Update: I think STOP_SYNCING_PERMANTENTLY has been deprecated since r143088.

[Nicolas Zea, 2014-02-11 00:35:05]

LGTM, with a sanity check that birthday errors still work. Also update commit
message to mention this cleans up dead code related to SyncProtocolErrors, which
are now actionable errors.

https://codereview.chromium.org/152013003/diff/220001/sync/engine/all_status.h
File sync/engine/all_status.h (right):

https://codereview.chromium.org/152013003/diff/220001/sync/engine/all_status....
sync/engine/all_status.h:16: #include "sync/engine/sync_cycle_event.h"
forward declare

[rlarocque, 2014-02-11 00:48:27]

https://codereview.chromium.org/152013003/diff/220001/sync/engine/all_status.h
File sync/engine/all_status.h (right):

https://codereview.chromium.org/152013003/diff/220001/sync/engine/all_status....
sync/engine/all_status.h:16: #include "sync/engine/sync_cycle_event.h"
On 2014/02/11 00:35:05, Nicolas Zea wrote:
> forward declare

Done.

[rlarocque, 2014-02-11 00:48:45]

The CQ bit was checked by rlarocque@chromium.org

[commit-bot: I haz the power, 2014-02-11 01:02:19]

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

[commit-bot: I haz the power, 2014-02-11 13:56:51]

Change committed as 250384