[GCM] Add ConnectionListener support, and hook up to debug page

[GCM] Add ConnectionListener support, and hook up to debug page

The ConnectionFactory::ConnectionListener gets informed of connection events,
including the actual ip endpoint with which a connection was established. This
will eventually be hooked up to app handlers to allow services to act on
connection changes. For now it just allows the debug page to more accurately
reflect the connection state.

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

[Nicolas Zea, 2014-06-04 23:23:03]

PTAL

[fgorski, 2014-06-05 00:03:11]

I understand that the change is pretty basic, but perhaps you could add a few
simple tests for making sure the updates are triggered?

https://codereview.chromium.org/317723004/diff/1/google_apis/gcm/engine/conne...
File google_apis/gcm/engine/connection_factory_impl.cc (right):

https://codereview.chromium.org/317723004/diff/1/google_apis/gcm/engine/conne...
google_apis/gcm/engine/connection_factory_impl.cc:146: return "CONNECTED";
the list of bool variables begs for a state enum... perhaps a TODO?

https://codereview.chromium.org/317723004/diff/1/google_apis/gcm/engine/conne...
File google_apis/gcm/engine/connection_factory_impl.h (right):

https://codereview.chromium.org/317723004/diff/1/google_apis/gcm/engine/conne...
google_apis/gcm/engine/connection_factory_impl.h:68: net::IPEndPoint
GetCurrentIP();
Could we change the name to reflect that we mean the Peer endpoint and not ours?

GetCurrentPeerIP // I would go as far as drop the Current.
GetPeerIP
GetPeerIPEndpoint

[Nicolas Zea, 2014-06-05 20:17:27]

added some tests and addressed comments, PTAL

https://codereview.chromium.org/317723004/diff/1/google_apis/gcm/engine/conne...
File google_apis/gcm/engine/connection_factory_impl.cc (right):

https://codereview.chromium.org/317723004/diff/1/google_apis/gcm/engine/conne...
google_apis/gcm/engine/connection_factory_impl.cc:146: return "CONNECTED";
On 2014/06/05 00:03:11, fgorski wrote:
> the list of bool variables begs for a state enum... perhaps a TODO?

The problem is that they are not mutually exclusive variables, so the state enum
would have to be even more complex. I'd rather just leave the variables
independent. For debug purposes it's okay to look at the "important" variable,
but not as far as actually making connection decisions.

https://codereview.chromium.org/317723004/diff/1/google_apis/gcm/engine/conne...
File google_apis/gcm/engine/connection_factory_impl.h (right):

https://codereview.chromium.org/317723004/diff/1/google_apis/gcm/engine/conne...
google_apis/gcm/engine/connection_factory_impl.h:68: net::IPEndPoint
GetCurrentIP();
On 2014/06/05 00:03:11, fgorski wrote:
> Could we change the name to reflect that we mean the Peer endpoint and not
ours?
> 
> GetCurrentPeerIP // I would go as far as drop the Current.
> GetPeerIP
> GetPeerIPEndpoint
> 

Done.

[fgorski, 2014-06-05 20:31:15]

lgtm

[Nicolas Zea, 2014-06-05 20:35:27]

The CQ bit was checked by zea@chromium.org

[commit-bot: I haz the power, 2014-06-05 20:36:27]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/zea@chromium.org/317723004/40001

[commit-bot: I haz the power, 2014-06-06 00:57:32]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  linux_chromium_chromeos_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_chrome...)
  linux_chromium_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_rel/bu...)
  mac_chromium_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/mac_chromium_rel/buil...)
  win_chromium_compile_dbg on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_compile_...)
  win_chromium_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_rel/buil...)
  win_chromium_x64_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_x64_rel/...)
  linux_gpu on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/linux_gpu/builds/...)
  win_gpu on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/win_gpu/builds/16696)

[commit-bot: I haz the power, 2014-06-06 01:15:11]

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

[commit-bot: I haz the power, 2014-06-06 01:15:12]

Try jobs failed on following builders:
  linux_chromium_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_rel/bu...)

[Nicolas Zea, 2014-06-09 19:00:11]

The CQ bit was checked by zea@chromium.org

[commit-bot: I haz the power, 2014-06-09 19:01:58]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/zea@chromium.org/317723004/60001

[Nicolas Zea, 2014-06-09 22:11:07]

The CQ bit was checked by zea@chromium.org

[commit-bot: I haz the power, 2014-06-09 22:11:24]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/zea@chromium.org/317723004/80001

[semenzato, 2014-06-10 00:16:20]

On 2014/06/09 22:11:24, I haz the power (commit-bot) wrote:
> CQ is trying da patch. Follow status at
>  https://chromium-status.appspot.com/cq/zea@chromium.org/317723004/80001

Sorry for being a little late on this, but does the connection state also
include the address type (ipv6 vs. ipv4)?  We will need it to program the NIC.

Thanks!

[Nicolas Zea, 2014-06-10 00:22:05]

On 2014/06/10 00:16:20, semenzato wrote:
> On 2014/06/09 22:11:24, I haz the power (commit-bot) wrote:
> > CQ is trying da patch. Follow status at
> >  https://chromium-status.appspot.com/cq/zea@chromium.org/317723004/80001
> 
> Sorry for being a little late on this, but does the connection state also
> include the address type (ipv6 vs. ipv4)?  We will need it to program the NIC.
> 
> Thanks!

Yep, it should, take a look at net/base/ip_endpoint.h. AddressFamily specifies
ipv6 vs ipv4

[commit-bot: I haz the power, 2014-06-10 15:27:57]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  win_chromium_compile_dbg on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_compile_...)

[commit-bot: I haz the power, 2014-06-10 17:54:54]

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

[commit-bot: I haz the power, 2014-06-10 17:54:55]

Try jobs failed on following builders:
  win_chromium_compile_dbg on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_compile_...)

[Nicolas Zea, 2014-06-10 17:57:31]

The CQ bit was checked by zea@chromium.org

[commit-bot: I haz the power, 2014-06-10 18:02:45]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/zea@chromium.org/317723004/80001

[Nicolas Zea, 2014-06-10 18:33:01]

The CQ bit was unchecked by zea@chromium.org

[Nicolas Zea, 2014-06-10 19:00:00]

The CQ bit was checked by zea@chromium.org

[commit-bot: I haz the power, 2014-06-10 19:02:28]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/zea@chromium.org/317723004/100001

[commit-bot: I haz the power, 2014-06-11 01:08:23]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  android_dbg_triggered_tests on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/android_dbg_triggered...)
  win_chromium_compile_dbg on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_compile_...)

[commit-bot: I haz the power, 2014-06-11 04:11:52]

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

[commit-bot: I haz the power, 2014-06-11 04:11:53]

Try jobs failed on following builders:
  win_chromium_compile_dbg on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_compile_...)

[Nicolas Zea, 2014-06-11 19:06:36]

The CQ bit was checked by zea@chromium.org

[commit-bot: I haz the power, 2014-06-11 19:07:46]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/zea@chromium.org/317723004/120001

[commit-bot: I haz the power, 2014-06-11 21:33:18]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  mac_chromium_compile_dbg on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/mac_chromium_compile_...)
  mac_chromium_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/mac_chromium_rel/buil...)
  win_chromium_compile_dbg on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_compile_...)
  win_chromium_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_rel/buil...)
  win_chromium_x64_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_x64_rel/...)

[commit-bot: I haz the power, 2014-06-11 21:36:44]

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

[commit-bot: I haz the power, 2014-06-11 21:36:45]

Try jobs failed on following builders:
  mac_chromium_compile_dbg on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/mac_chromium_compile_...)
  mac_chromium_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/mac_chromium_rel/buil...)
  win_chromium_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_rel/buil...)

[Nicolas Zea, 2014-06-12 17:08:06]

The CQ bit was checked by zea@chromium.org

[commit-bot: I haz the power, 2014-06-12 17:14:48]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/zea@chromium.org/317723004/120001

[commit-bot: I haz the power, 2014-06-12 21:00:50]

Change committed as 276800