Don't drop debug_info when posting to sync server fails

Don't clear debug info until after it has been sent to the server.

Replaced DebugInfoGetter::GetAndClearDebugInfo with GetDebugInfo and ClearDebugInfo so we can clear the debug info only after we have successfully sent it to the server.

Moved MockDebugInfoGetter into its own file so it can be used by other tests.

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

[maniscalco, 2013-11-19 22:41:03]

Hi Nicolas, would you please review this change?

There were some comments in the code about sending debug info only once per
session. I'm not sure I understood the context of those comments. Can you fill
me in?

[Nicolas Zea, 2013-11-19 23:43:03]

LGTM with nits.

Re: the once per session comments, I'm not sure those made sense in the first
place, given that we were clearing when we got the events. It may have just been
for the sake of being careful. Either way, I think at this point the appropriate
approach is to track whether we should send debug info by whether there is any
to send.

https://codereview.chromium.org/61213009/diff/100001/sync/engine/download.cc
File sync/engine/download.cc (right):

https://codereview.chromium.org/61213009/diff/100001/sync/engine/download.cc#...
sync/engine/download.cc:405: if (debug_info_getter) {
nit: you're already checking debug_info_getter before calling this. One of these
checks is unnecessary.

https://codereview.chromium.org/61213009/diff/100001/sync/test/engine/mock_co...
File sync/test/engine/mock_connection_manager.cc (right):

https://codereview.chromium.org/61213009/diff/100001/sync/test/engine/mock_co...
sync/test/engine/mock_connection_manager.cc:87:
requests_.push_back(sync_pb::ClientToServerMessage::default_instance());
push_back(post)?

https://codereview.chromium.org/61213009/diff/100001/sync/test/sessions/mock_...
File sync/test/sessions/mock_debug_info_getter.h (right):

https://codereview.chromium.org/61213009/diff/100001/sync/test/sessions/mock_...
sync/test/sessions/mock_debug_info_getter.h:16: class SYNC_EXPORT_PRIVATE
MockDebugInfoGetter
nit: Brief comment about what the class is.

https://codereview.chromium.org/61213009/diff/100001/sync/test/sessions/mock_...
sync/test/sessions/mock_debug_info_getter.h:21: virtual void
GetDebugInfo(sync_pb::DebugInfo* debug_info) OVERRIDE;
nit: typically we'll comment "DebugInfoGetter implementation" when implementing
an interface.

https://codereview.chromium.org/61213009/diff/100001/sync/test/sessions/mock_...
sync/test/sessions/mock_debug_info_getter.h:27: sync_pb::DebugInfo debug_info_;
nit: DISALLOW_COPY_AND_ASSIGN(MockDebugInfoGetter);

[maniscalco, 2013-11-20 01:03:32]

Thanks for the feedback. All addressed. RFAL.

https://codereview.chromium.org/61213009/diff/100001/sync/engine/download.cc
File sync/engine/download.cc (right):

https://codereview.chromium.org/61213009/diff/100001/sync/engine/download.cc#...
sync/engine/download.cc:405: if (debug_info_getter) {
On 2013/11/19 23:43:04, Nicolas Zea wrote:
> nit: you're already checking debug_info_getter before calling this. One of
these
> checks is unnecessary.

Done.  Now relying on caller's check.

https://codereview.chromium.org/61213009/diff/100001/sync/test/engine/mock_co...
File sync/test/engine/mock_connection_manager.cc (right):

https://codereview.chromium.org/61213009/diff/100001/sync/test/engine/mock_co...
sync/test/engine/mock_connection_manager.cc:87:
requests_.push_back(sync_pb::ClientToServerMessage::default_instance());
On 2013/11/19 23:43:04, Nicolas Zea wrote:
> push_back(post)?

Good catch. Done.

https://codereview.chromium.org/61213009/diff/100001/sync/test/sessions/mock_...
File sync/test/sessions/mock_debug_info_getter.h (right):

https://codereview.chromium.org/61213009/diff/100001/sync/test/sessions/mock_...
sync/test/sessions/mock_debug_info_getter.h:16: class SYNC_EXPORT_PRIVATE
MockDebugInfoGetter
On 2013/11/19 23:43:04, Nicolas Zea wrote:
> nit: Brief comment about what the class is.

Done.

https://codereview.chromium.org/61213009/diff/100001/sync/test/sessions/mock_...
sync/test/sessions/mock_debug_info_getter.h:21: virtual void
GetDebugInfo(sync_pb::DebugInfo* debug_info) OVERRIDE;
On 2013/11/19 23:43:04, Nicolas Zea wrote:
> nit: typically we'll comment "DebugInfoGetter implementation" when
implementing
> an interface.

Done.

https://codereview.chromium.org/61213009/diff/100001/sync/test/sessions/mock_...
sync/test/sessions/mock_debug_info_getter.h:27: sync_pb::DebugInfo debug_info_;
On 2013/11/19 23:43:04, Nicolas Zea wrote:
> nit: DISALLOW_COPY_AND_ASSIGN(MockDebugInfoGetter);

Done.

[rlarocque, 2013-11-20 01:25:18]

Two small nits.  Otherwise LGTM.

https://codereview.chromium.org/61213009/diff/200001/sync/internal_api/debug_...
File sync/internal_api/debug_info_event_listener.cc (right):

https://codereview.chromium.org/61213009/diff/200001/sync/internal_api/debug_...
sync/internal_api/debug_info_event_listener.cc:147: void
DebugInfoEventListener::GetDebugInfo(
Does this not fit on one line?

https://codereview.chromium.org/61213009/diff/200001/sync/test/sessions/mock_...
File sync/test/sessions/mock_debug_info_getter.cc (right):

https://codereview.chromium.org/61213009/diff/200001/sync/test/sessions/mock_...
sync/test/sessions/mock_debug_info_getter.cc:27: 
Remove unnecessary whitespace.

[maniscalco, 2013-11-20 01:50:01]

Thanks for the feedback.

https://codereview.chromium.org/61213009/diff/200001/sync/internal_api/debug_...
File sync/internal_api/debug_info_event_listener.cc (right):

https://codereview.chromium.org/61213009/diff/200001/sync/internal_api/debug_...
sync/internal_api/debug_info_event_listener.cc:147: void
DebugInfoEventListener::GetDebugInfo(
On 2013/11/20 01:25:18, rlarocque wrote:
> Does this not fit on one line?

Oops, it does fit. Done.

https://codereview.chromium.org/61213009/diff/200001/sync/test/sessions/mock_...
File sync/test/sessions/mock_debug_info_getter.cc (right):

https://codereview.chromium.org/61213009/diff/200001/sync/test/sessions/mock_...
sync/test/sessions/mock_debug_info_getter.cc:27: 
On 2013/11/20 01:25:18, rlarocque wrote:
> Remove unnecessary whitespace.

Done.

[Nicolas Zea, 2013-11-20 20:59:40]

LGTM

[commit-bot: I haz the power, 2013-11-21 18:57:03]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/maniscalco@chromium.org/61213009/360001

[commit-bot: I haz the power, 2013-11-21 20:55:47]

Change committed as 236591