Update ConnectionEventTracker to check if there is an in progress attempt.

Update ConnectionEventTracker to check if there is an in progress attempt.

It is possible to get EndConnectionAttempt when there is not an in progress
event, because EndConnectionAttempt is called from SignalConnectionReset,
which will be called any time the network changes state.

This CL modifies the code to only save the current_event_ if it has data.
This also adds a test to validate that multiple SignalConnectionReset
calls don't result in saving empty events.

BUG=662983
Review-Url: https://codereview.chromium.org/2628733004
Cr-Commit-Position: refs/heads/master@{#443885}
Committed: https://chromium.googlesource.com/chromium/src/+/9d9e71b1d98b8a2ae88d8661cbf15ce23e6c3513

[harkness, 2017-01-11 11:10:22]

harkness@chromium.org changed reviewers:
+ peter@chromium.org, zea@chromium.org

[Peter Beverloo, 2017-01-11 14:31:48]

It seems to me like it'd be cleaner for EndConnectionAttempt to not be called at
all if there is no in-progress connection attempt, instead of handling that case
in the method itself.

Could we make the call in the ConnectionFactoryImpl conditional?

https://codereview.chromium.org/2628733004/diff/1/google_apis/gcm/engine/conn...
File google_apis/gcm/engine/connection_event_tracker.cc (left):

https://codereview.chromium.org/2628733004/diff/1/google_apis/gcm/engine/conn...
google_apis/gcm/engine/connection_event_tracker.cc:35: // TODO(harkness): Modify
tests so that we can put a DCHECK here.
Why isn't the TODO relevant anymore?

https://codereview.chromium.org/2628733004/diff/1/google_apis/gcm/engine/conn...
File google_apis/gcm/engine/connection_event_tracker.cc (right):

https://codereview.chromium.org/2628733004/diff/1/google_apis/gcm/engine/conn...
google_apis/gcm/engine/connection_event_tracker.cc:39: if
(current_event_.has_time_connection_started_ms()) {
style nit: prefer early return over nesting.

  if (!current_event_.has_time_connection_started_ms())
    return;

[harkness, 2017-01-12 11:04:48]

There isn't a simple check for the ConnectionFactoryImpl to know whether an
attempt is in progress. I could use a combination of connecting_ and
IsEndpointReachable(), but I thought it was cleaner to just expose a method to
query the tracker for whether an event is currently being tracked.

https://codereview.chromium.org/2628733004/diff/1/google_apis/gcm/engine/conn...
File google_apis/gcm/engine/connection_event_tracker.cc (right):

https://codereview.chromium.org/2628733004/diff/1/google_apis/gcm/engine/conn...
google_apis/gcm/engine/connection_event_tracker.cc:39: if
(current_event_.has_time_connection_started_ms()) {
On 2017/01/11 14:31:48, Peter Beverloo wrote:
> style nit: prefer early return over nesting.
> 
>   if (!current_event_.has_time_connection_started_ms())
>     return;

No longer applicable.

[Peter Beverloo, 2017-01-12 16:03:27]

lgtm

https://codereview.chromium.org/2628733004/diff/20001/google_apis/gcm/engine/...
File google_apis/gcm/engine/connection_event_tracker.cc (right):

https://codereview.chromium.org/2628733004/diff/20001/google_apis/gcm/engine/...
google_apis/gcm/engine/connection_event_tracker.cc:40: //
StartConnectionAttempt.
optional nit: I'd drop the comment - that's obvious from the DCHECK

https://codereview.chromium.org/2628733004/diff/20001/google_apis/gcm/engine/...
File google_apis/gcm/engine/connection_event_tracker.h (right):

https://codereview.chromium.org/2628733004/diff/20001/google_apis/gcm/engine/...
google_apis/gcm/engine/connection_event_tracker.h:26: bool EventInProgress();
micro nits: (1) IsEventInProgress() and (2) make it a const method.

https://codereview.chromium.org/2628733004/diff/20001/google_apis/gcm/engine/...
File google_apis/gcm/engine/connection_factory_impl.cc (right):

https://codereview.chromium.org/2628733004/diff/20001/google_apis/gcm/engine/...
google_apis/gcm/engine/connection_factory_impl.cc:216: if (logging_in_)
note: this will need to merge with your reason == LOGIN_FAILURE change

[Nicolas Zea, 2017-01-12 18:02:16]

lgtm

[harkness, 2017-01-16 12:20:37]

https://codereview.chromium.org/2628733004/diff/20001/google_apis/gcm/engine/...
File google_apis/gcm/engine/connection_event_tracker.cc (right):

https://codereview.chromium.org/2628733004/diff/20001/google_apis/gcm/engine/...
google_apis/gcm/engine/connection_event_tracker.cc:40: //
StartConnectionAttempt.
On 2017/01/12 16:03:27, Peter Beverloo (OOO- Jan 19th) wrote:
> optional nit: I'd drop the comment - that's obvious from the DCHECK

Done.

https://codereview.chromium.org/2628733004/diff/20001/google_apis/gcm/engine/...
File google_apis/gcm/engine/connection_event_tracker.h (right):

https://codereview.chromium.org/2628733004/diff/20001/google_apis/gcm/engine/...
google_apis/gcm/engine/connection_event_tracker.h:26: bool EventInProgress();
On 2017/01/12 16:03:27, Peter Beverloo (OOO- Jan 19th) wrote:
> micro nits: (1) IsEventInProgress() and (2) make it a const method.

Done.

https://codereview.chromium.org/2628733004/diff/20001/google_apis/gcm/engine/...
File google_apis/gcm/engine/connection_factory_impl.cc (right):

https://codereview.chromium.org/2628733004/diff/20001/google_apis/gcm/engine/...
google_apis/gcm/engine/connection_factory_impl.cc:216: if (logging_in_)
On 2017/01/12 16:03:27, Peter Beverloo (OOO- Jan 19th) wrote:
> note: this will need to merge with your reason == LOGIN_FAILURE change

Acknowledged.

[harkness, 2017-01-16 12:20:48]

The CQ bit was checked by harkness@chromium.org

[harkness, 2017-01-16 12:20:48]

The patchset sent to the CQ was uploaded after l-g-t-m from peter@chromium.org,
zea@chromium.org
Link to the patchset: https://codereview.chromium.org/2628733004/#ps40001
(title: "Rebase and fix nits")

[commit-bot: I haz the power, 2017-01-16 12:20:57]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-01-16 12:20:58]

This CL has an open dependency (Issue 2620393002 Patch 120001). Please resolve
the dependency and try again.
If you are sure that there is no real dependency, please use one of the options
listed in https://goo.gl/9Es4OR to land the CL.

[harkness, 2017-01-16 12:55:23]

The CQ bit was checked by harkness@chromium.org

[commit-bot: I haz the power, 2017-01-16 12:55:34]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-01-16 13:39:57]

CQ is committing da patch.

Bot data: {"patchset_id": 40001, "attempt_start_ts": 1484571323316730,
"parent_rev": "24fec8948239a4b4491ed404085a55b801bc0e6e", "commit_rev":
"9d9e71b1d98b8a2ae88d8661cbf15ce23e6c3513"}

[commit-bot: I haz the power, 2017-01-16 13:40:25]

Description was changed from

==========
Update ConnectionEventTracker to check if there is an in progress attempt.

It is possible to get EndConnectionAttempt when there is not an in progress
event, because EndConnectionAttempt is called from SignalConnectionReset,
which will be called any time the network changes state.

This CL modifies the code to only save the current_event_ if it has data.
This also adds a test to validate that multiple SignalConnectionReset
calls don't result in saving empty events.

BUG=662983
==========

to

==========
Update ConnectionEventTracker to check if there is an in progress attempt.

It is possible to get EndConnectionAttempt when there is not an in progress
event, because EndConnectionAttempt is called from SignalConnectionReset,
which will be called any time the network changes state.

This CL modifies the code to only save the current_event_ if it has data.
This also adds a test to validate that multiple SignalConnectionReset
calls don't result in saving empty events.

BUG=662983

Review-Url: https://codereview.chromium.org/2628733004
Cr-Commit-Position: refs/heads/master@{#443885}
Committed:
https://chromium.googlesource.com/chromium/src/+/9d9e71b1d98b8a2ae88d8661cbf1...
==========

[commit-bot: I haz the power, 2017-01-16 13:40:26]

Committed patchset #3 (id:40001) as
https://chromium.googlesource.com/chromium/src/+/9d9e71b1d98b8a2ae88d8661cbf1...