Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(49)

Issues Search
    My Issues | Starred     Open | Closed | All

Issue 116533006: Control invalidations network channel from TiclInvalidationService (Closed)

Created:
7 years ago by pavely
Modified:
6 years, 11 months ago
Reviewers:
rlarocque
CC:
chromium-reviews, tim+watch_chromium.org, rsimha+watch_chromium.org, haitaol+watch_chromium.org, maniscalco+watch_chromium.org
Base URL:
https://chromium.googlesource.com/chromium/src.git@master
Visibility:
Public.

Description

Control invalidations network channel from TiclInvalidationService This change introduces boilerplate GCMNetworkChannel implementation and switches sync/notifier classes to refer network channel through base class SyncNetworkChannel. TiclInvalidationService can initialize invalidator with either PushClient or GCM channel. On one hand goal was to not expose network channel implementation to TiclInvalidationService, on the other hand to avoid passing channel specific parameters across sync/notifier classes during initialization. The solution is to introduce NetworkChannelCreator callback that takes no parameters and returns scoped_ptr to SyncNetworkChannel. There are helper functions in SyncNetworkChannel to create implementations for Puch/GCM clients and helper functions in NonBlockingInvalidator to createNetworkChannelCreator. The alternative was to create NetworkChannel factory interface and write implementations of it for each channel type. This would provide more expressive names, but requires interface and two implementations for simple purpose. Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=242991

Patch Set 1 #

Patch Set 2 : Rebase #

Total comments: 7

Patch Set 3 : Apply feedback #

Patch Set 4 : Rebase #

Unified diffs Side-by-side diffs Delta from patch set Stats (+297 lines, -70 lines) Patch
M chrome/browser/invalidation/ticl_invalidation_service.h View 1 chunk +6 lines, -1 line 0 comments Download
M chrome/browser/invalidation/ticl_invalidation_service.cc View 1 2 5 chunks +31 lines, -10 lines 0 comments Download
A sync/notifier/gcm_network_channel.h View 1 2 1 chunk +38 lines, -0 lines 0 comments Download
A sync/notifier/gcm_network_channel.cc View 1 chunk +22 lines, -0 lines 0 comments Download
A sync/notifier/gcm_network_channel_unittest.cc View 1 2 1 chunk +40 lines, -0 lines 0 comments Download
M sync/notifier/invalidation_notifier.h View 1 chunk +1 line, -1 line 0 comments Download
M sync/notifier/invalidation_notifier.cc View 2 chunks +2 lines, -2 lines 0 comments Download
M sync/notifier/invalidation_notifier_unittest.cc View 2 chunks +6 lines, -1 line 0 comments Download
M sync/notifier/non_blocking_invalidator.h View 1 2 4 chunks +19 lines, -2 lines 0 comments Download
M sync/notifier/non_blocking_invalidator.cc View 5 chunks +69 lines, -33 lines 0 comments Download
M sync/notifier/non_blocking_invalidator_unittest.cc View 1 chunk +7 lines, -4 lines 0 comments Download
M sync/notifier/push_client_channel.h View 1 chunk +2 lines, -1 line 0 comments Download
M sync/notifier/push_client_channel.cc View 1 chunk +0 lines, -1 line 0 comments Download
M sync/notifier/sync_invalidation_listener.h View 3 chunks +2 lines, -3 lines 0 comments Download
M sync/notifier/sync_invalidation_listener.cc View 2 chunks +6 lines, -6 lines 0 comments Download
M sync/notifier/sync_invalidation_listener_unittest.cc View 2 chunks +3 lines, -1 line 0 comments Download
M sync/notifier/sync_system_resources.h View 2 chunks +9 lines, -0 lines 0 comments Download
M sync/notifier/sync_system_resources.cc View 2 chunks +15 lines, -0 lines 0 comments Download
M sync/notifier/sync_system_resources_unittest.cc View 1 chunk +4 lines, -0 lines 0 comments Download
M sync/sync_notifier.gypi View 1 chunk +2 lines, -0 lines 0 comments Download
M sync/sync_tests.gypi View 1 2 3 1 chunk +1 line, -0 lines 0 comments Download
M sync/tools/sync_client.cc View 1 2 1 chunk +6 lines, -2 lines 0 comments Download
M sync/tools/sync_listen_notifications.cc View 1 chunk +6 lines, -2 lines 0 comments Download

Messages

Total messages: 14 (0 generated)
pavely
https://codereview.chromium.org/116533006/diff/20001/sync/notifier/non_blocking_invalidator.cc File sync/notifier/non_blocking_invalidator.cc (right): https://codereview.chromium.org/116533006/diff/20001/sync/notifier/non_blocking_invalidator.cc#newcode22 sync/notifier/non_blocking_invalidator.cc:22: struct NonBlockingInvalidator::InitializeOptions { Number of parameters to NonBlockingInvalidator::Core is ...
7 years ago (2013-12-20 00:04:15 UTC) #1
rlarocque
Sorry for the late review. Most of this looks good. https://codereview.chromium.org/116533006/diff/20001/sync/notifier/gcm_network_channel.h File sync/notifier/gcm_network_channel.h (right): https://codereview.chromium.org/116533006/diff/20001/sync/notifier/gcm_network_channel.h#newcode33 ...
6 years, 11 months ago (2014-01-02 18:56:11 UTC) #2
pavely
https://codereview.chromium.org/116533006/diff/20001/sync/notifier/gcm_network_channel.h File sync/notifier/gcm_network_channel.h (right): https://codereview.chromium.org/116533006/diff/20001/sync/notifier/gcm_network_channel.h#newcode33 sync/notifier/gcm_network_channel.h:33: On 2014/01/02 18:56:11, rlarocque wrote: > nit: remove unnecessary ...
6 years, 11 months ago (2014-01-03 00:33:24 UTC) #3
rlarocque
lgtm
6 years, 11 months ago (2014-01-03 00:37:02 UTC) #4
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/pavely@chromium.org/116533006/70001
6 years, 11 months ago (2014-01-03 00:37:54 UTC) #5
commit-bot: I haz the power
Retried try job too often on mac_rel for step(s) chromedriver2_unittests http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=mac_rel&number=207252
6 years, 11 months ago (2014-01-03 01:29:24 UTC) #6
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/pavely@chromium.org/116533006/70001
6 years, 11 months ago (2014-01-03 18:01:19 UTC) #7
commit-bot: I haz the power
Retried try job too often on mac_rel for step(s) chromedriver2_unittests http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=mac_rel&number=207539
6 years, 11 months ago (2014-01-03 18:41:14 UTC) #8
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/pavely@chromium.org/116533006/390002
6 years, 11 months ago (2014-01-03 19:08:07 UTC) #9
commit-bot: I haz the power
Retried try job too often on android_aosp for step(s) compile http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_aosp&number=37487
6 years, 11 months ago (2014-01-03 20:05:44 UTC) #10
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/pavely@chromium.org/116533006/390002
6 years, 11 months ago (2014-01-03 20:11:15 UTC) #11
commit-bot: I haz the power
Retried try job too often on win for step(s) compile http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win&number=137785
6 years, 11 months ago (2014-01-03 21:37:32 UTC) #12
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/pavely@chromium.org/116533006/390002
6 years, 11 months ago (2014-01-03 21:38:42 UTC) #13
commit-bot: I haz the power
6 years, 11 months ago (2014-01-04 02:48:53 UTC) #14
Message was sent while issue was closed. 
Change committed as 242991

Powered by Google App Engine
This is Rietveld 408576698