Reland: Add GCMChannelStatusSyncer to schedule requests and enable/disable GCM

Reland: Add GCMChannelStatusSyncer to schedule requests and enable/disable GCM

BUG=384041
TEST=new tests added

Committed: https://crrev.com/3c23f4a188e171998f3042ad62f4aa5717e66d63
Cr-Commit-Position: refs/heads/master@{#295524}

Committed: https://crrev.com/2dc910b0b9082050e5be6c81b00dddf97ffa3e6e
Cr-Commit-Position: refs/heads/master@{#295645}

[jianli, 2014-09-10 21:52:59]

jianli@chromium.org changed reviewers:
+ fgorski@chromium.org, zea@chromium.org

[jianli, 2014-09-10 21:57:17]

jianli@chromium.org changed reviewers:
+ thestig@chromium.org

[jianli, 2014-09-10 21:57:18]

thestig for changes to chrome/browser/browser_process_impl.cc and
chrome/browser/prefs/browser_prefs.cc.

zea and fgorski for all GCM changes.

[Lei Zhang, 2014-09-10 22:53:54]

On 2014/09/10 21:57:18, jianli wrote:
> thestig for changes to chrome/browser/browser_process_impl.cc and
> chrome/browser/prefs/browser_prefs.cc.

Non-GCM bits lgtm assuming GCMChannelStatusSyncer is used on all platforms.

[jianli, 2014-09-10 23:07:37]

On 2014/09/10 22:53:54, Lei Zhang wrote:
> On 2014/09/10 21:57:18, jianli wrote:
> > thestig for changes to chrome/browser/browser_process_impl.cc and
> > chrome/browser/prefs/browser_prefs.cc.
> 
> Non-GCM bits lgtm assuming GCMChannelStatusSyncer is used on all platforms.

Thanks for reminding. GCM is not used on Android. So I moved the registration
code inside ifdef.

[fgorski, 2014-09-11 18:24:36]

Mostly looks good.

https://codereview.chromium.org/561943002/diff/60001/components/gcm_driver/gc...
File components/gcm_driver/gcm_channel_status_syncer.cc (right):

https://codereview.chromium.org/561943002/diff/60001/components/gcm_driver/gc...
components/gcm_driver/gcm_channel_status_syncer.cc:115: last_check_time_ =
base::Time::Now();
injected clock is typically more useful for testing, but I see what you are
doing below.

https://codereview.chromium.org/561943002/diff/60001/components/gcm_driver/gc...
components/gcm_driver/gcm_channel_status_syncer.cc:129: poll_interval_seconds_ =
poll_interval_seconds;
this code should check for min poll interval.

try to extract method from 83-88 and call it from here as well.

https://codereview.chromium.org/561943002/diff/60001/components/gcm_driver/gc...
File components/gcm_driver/gcm_channel_status_syncer.h (right):

https://codereview.chromium.org/561943002/diff/60001/components/gcm_driver/gc...
components/gcm_driver/gcm_channel_status_syncer.h:14: #include
"components/pref_registry/pref_registry_syncable.h"
you are also forward declaring it before. I think you should get rid of one.

https://codereview.chromium.org/561943002/diff/60001/components/gcm_driver/gc...
components/gcm_driver/gcm_channel_status_syncer.h:59: // Schedules to start next
request.
nit: remove "to start"

https://codereview.chromium.org/561943002/diff/60001/components/gcm_driver/gc...
components/gcm_driver/gcm_channel_status_syncer.h:69: GCMDriver* driver_;  //
GCMDriver owns GCMChannelStatusSyncer instance.
nit: why not putting comment above?

https://codereview.chromium.org/561943002/diff/60001/components/gcm_driver/gc...
File components/gcm_driver/gcm_driver_desktop_unittest.cc (right):

https://codereview.chromium.org/561943002/diff/60001/components/gcm_driver/gc...
components/gcm_driver/gcm_driver_desktop_unittest.cc:1094:
TEST_F(GCMChannelStatusSyncerTest, FirstPolling) {
nit: FirstTimePolling

[jianli, 2014-09-11 21:04:31]

https://codereview.chromium.org/561943002/diff/60001/components/gcm_driver/gc...
File components/gcm_driver/gcm_channel_status_syncer.cc (right):

https://codereview.chromium.org/561943002/diff/60001/components/gcm_driver/gc...
components/gcm_driver/gcm_channel_status_syncer.cc:115: last_check_time_ =
base::Time::Now();
On 2014/09/11 18:24:35, fgorski wrote:
> injected clock is typically more useful for testing, but I see what you are
> doing below.

Agreed. For now, testing with real clock seems to work fine. If in the future,
there is something that can't be covered with real clock, I will do the switch.

https://codereview.chromium.org/561943002/diff/60001/components/gcm_driver/gc...
components/gcm_driver/gcm_channel_status_syncer.cc:129: poll_interval_seconds_ =
poll_interval_seconds;
On 2014/09/11 18:24:35, fgorski wrote:
> this code should check for min poll interval.
> 
> try to extract method from 83-88 and call it from here as well.

OnRequestCompleted is called with min value enforced for poll_interval_seconds.
I added a DCHECK for this.

https://codereview.chromium.org/561943002/diff/60001/components/gcm_driver/gc...
File components/gcm_driver/gcm_channel_status_syncer.h (right):

https://codereview.chromium.org/561943002/diff/60001/components/gcm_driver/gc...
components/gcm_driver/gcm_channel_status_syncer.h:14: #include
"components/pref_registry/pref_registry_syncable.h"
On 2014/09/11 18:24:35, fgorski wrote:
> you are also forward declaring it before. I think you should get rid of one.

Done.

https://codereview.chromium.org/561943002/diff/60001/components/gcm_driver/gc...
components/gcm_driver/gcm_channel_status_syncer.h:59: // Schedules to start next
request.
On 2014/09/11 18:24:35, fgorski wrote:
> nit: remove "to start"

Done.

https://codereview.chromium.org/561943002/diff/60001/components/gcm_driver/gc...
components/gcm_driver/gcm_channel_status_syncer.h:69: GCMDriver* driver_;  //
GCMDriver owns GCMChannelStatusSyncer instance.
On 2014/09/11 18:24:35, fgorski wrote:
> nit: why not putting comment above?

Done.

https://codereview.chromium.org/561943002/diff/60001/components/gcm_driver/gc...
File components/gcm_driver/gcm_driver_desktop_unittest.cc (right):

https://codereview.chromium.org/561943002/diff/60001/components/gcm_driver/gc...
components/gcm_driver/gcm_driver_desktop_unittest.cc:1094:
TEST_F(GCMChannelStatusSyncerTest, FirstPolling) {
On 2014/09/11 18:24:35, fgorski wrote:
> nit: FirstTimePolling

Done.

[jianli, 2014-09-11 21:05:29]

jianli@chromium.org changed reviewers:
+ bauerb@chromium.org

[jianli, 2014-09-11 21:05:29]

Adding bauerb for approval on adding dependency on components/pref_registry.

[fgorski, 2014-09-11 21:21:24]

lgtm

[jianli, 2014-09-11 22:26:59]

Patchset #5 (id:80001) has been deleted

[Bernhard Bauer, 2014-09-12 08:56:41]

LGTM w/ a nit in pref registration (I don't think you need approval from an
OWNER of a component to depend on it though, just to make it depend on something
else).

https://codereview.chromium.org/561943002/diff/90017/chrome/browser/prefs/bro...
File chrome/browser/prefs/browser_prefs.cc (right):

https://codereview.chromium.org/561943002/diff/90017/chrome/browser/prefs/bro...
chrome/browser/prefs/browser_prefs.cc:294:
BackgroundModeManager::RegisterPrefs(registry);
Can you add this here instead?

[jianli, 2014-09-13 18:44:46]

https://codereview.chromium.org/561943002/diff/90017/chrome/browser/prefs/bro...
File chrome/browser/prefs/browser_prefs.cc (right):

https://codereview.chromium.org/561943002/diff/90017/chrome/browser/prefs/bro...
chrome/browser/prefs/browser_prefs.cc:294:
BackgroundModeManager::RegisterPrefs(registry);
On 2014/09/12 08:56:40, Bernhard Bauer wrote:
> Can you add this here instead?

Done.

[Nicolas Zea, 2014-09-15 22:35:59]

Mostly nits, with a couple questions

https://codereview.chromium.org/561943002/diff/110001/components/gcm_driver/g...
File components/gcm_driver/gcm_channel_status_syncer.cc (right):

https://codereview.chromium.org/561943002/diff/110001/components/gcm_driver/g...
components/gcm_driver/gcm_channel_status_syncer.cc:99: if (request_)
Note this isn't going to prevent ScheduleRequest from being called multiple
times (as long as the posted task hasn't run yet). Is that ok?

https://codereview.chromium.org/561943002/diff/110001/components/gcm_driver/g...
components/gcm_driver/gcm_channel_status_syncer.cc:165: int delay_seconds =
static_cast<int>(poll_interval_seconds_ -
would it be better to do int64 math here rather than normal ints? (and remove
the static cast)

https://codereview.chromium.org/561943002/diff/110001/components/gcm_driver/g...
components/gcm_driver/gcm_channel_status_syncer.cc:174: delay_seconds =
kFirstTimeDelaySeconds;
Why do we want a first time delay?

https://codereview.chromium.org/561943002/diff/110001/components/gcm_driver/g...
File components/gcm_driver/gcm_channel_status_syncer.h (right):

https://codereview.chromium.org/561943002/diff/110001/components/gcm_driver/g...
components/gcm_driver/gcm_channel_status_syncer.h:17: namespace net {
nit: newline before and after namespace

https://codereview.chromium.org/561943002/diff/110001/components/gcm_driver/g...
components/gcm_driver/gcm_channel_status_syncer.h:29: class
GCMChannelStatusSyncer {
nit: description

https://codereview.chromium.org/561943002/diff/110001/components/gcm_driver/g...
components/gcm_driver/gcm_channel_status_syncer.h:31: static void
RegisterPrefs(PrefRegistrySimple* registry);
Why are both of these needed?

https://codereview.chromium.org/561943002/diff/110001/components/gcm_driver/g...
components/gcm_driver/gcm_channel_status_syncer.h:37:
scoped_refptr<net::URLRequestContextGetter> request_context);
nit: pass scoped_refptr as const ref (i.e. const scoped_refptr<..>& ...)

[jianli, 2014-09-16 01:07:45]

https://codereview.chromium.org/561943002/diff/110001/components/gcm_driver/g...
File components/gcm_driver/gcm_channel_status_syncer.cc (right):

https://codereview.chromium.org/561943002/diff/110001/components/gcm_driver/g...
components/gcm_driver/gcm_channel_status_syncer.cc:99: if (request_)
On 2014/09/15 22:35:58, Nicolas Zea wrote:
> Note this isn't going to prevent ScheduleRequest from being called multiple
> times (as long as the posted task hasn't run yet). Is that ok?

Good catch. Fixed.

https://codereview.chromium.org/561943002/diff/110001/components/gcm_driver/g...
components/gcm_driver/gcm_channel_status_syncer.cc:165: int delay_seconds =
static_cast<int>(poll_interval_seconds_ -
On 2014/09/15 22:35:58, Nicolas Zea wrote:
> would it be better to do int64 math here rather than normal ints? (and remove
> the static cast)

Done.

https://codereview.chromium.org/561943002/diff/110001/components/gcm_driver/g...
components/gcm_driver/gcm_channel_status_syncer.cc:174: delay_seconds =
kFirstTimeDelaySeconds;
On 2014/09/15 22:35:58, Nicolas Zea wrote:
> Why do we want a first time delay?

It might be possible that GCM is triggered at browser startup time and the first
time poll request is not completed successfully last time. We do not want to
send a network request immediately when browser is in the process of starting
up.

If the GCM is first used after the browser starts, I think it is fine to delay
just 1 minute for first time poll request.

https://codereview.chromium.org/561943002/diff/110001/components/gcm_driver/g...
File components/gcm_driver/gcm_channel_status_syncer.h (right):

https://codereview.chromium.org/561943002/diff/110001/components/gcm_driver/g...
components/gcm_driver/gcm_channel_status_syncer.h:17: namespace net {
On 2014/09/15 22:35:58, Nicolas Zea wrote:
> nit: newline before and after namespace

Done.

https://codereview.chromium.org/561943002/diff/110001/components/gcm_driver/g...
components/gcm_driver/gcm_channel_status_syncer.h:29: class
GCMChannelStatusSyncer {
On 2014/09/15 22:35:58, Nicolas Zea wrote:
> nit: description

Done.

https://codereview.chromium.org/561943002/diff/110001/components/gcm_driver/g...
components/gcm_driver/gcm_channel_status_syncer.h:31: static void
RegisterPrefs(PrefRegistrySimple* registry);
On 2014/09/15 22:35:58, Nicolas Zea wrote:
> Why are both of these needed?

GCMDriver can now run under 2 modes: profile and non-profile (device/instance
level). For profile, we persist data to prefs store and for non-profile, we
persist to local states store. The way to register for these 2 stores are a bit
different.

https://codereview.chromium.org/561943002/diff/110001/components/gcm_driver/g...
components/gcm_driver/gcm_channel_status_syncer.h:37:
scoped_refptr<net::URLRequestContextGetter> request_context);
On 2014/09/15 22:35:58, Nicolas Zea wrote:
> nit: pass scoped_refptr as const ref (i.e. const scoped_refptr<..>& ...)

Done.

[Nicolas Zea, 2014-09-18 16:47:10]

lgtm

[jianli, 2014-09-18 17:08:18]

The CQ bit was checked by jianli@chromium.org

[commit-bot: I haz the power, 2014-09-18 17:08:57]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/561943002/130001

[commit-bot: I haz the power, 2014-09-18 17:13:09]

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

[commit-bot: I haz the power, 2014-09-18 17:13:11]

Try jobs failed on following builders:
  chromium_presubmit on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
  ios_rel_device_ninja on tryserver.chromium.mac
(http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_rel_device_ni...)
  win8_chromium_rel on tryserver.chromium.win
(http://build.chromium.org/p/tryserver.chromium.win/builders/win8_chromium_rel...)

[jianli, 2014-09-18 18:36:03]

The CQ bit was checked by jianli@chromium.org

[commit-bot: I haz the power, 2014-09-18 18:36:47]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/561943002/150001

[commit-bot: I haz the power, 2014-09-18 19:34:06]

Committed patchset #8 (id:150001) as 2c1958e619e9eb20b03bbee350f95e9d91add0cb

[commit-bot: I haz the power, 2014-09-18 19:34:49]

Patchset 8 (id:??) landed as
https://crrev.com/3c23f4a188e171998f3042ad62f4aa5717e66d63
Cr-Commit-Position: refs/heads/master@{#295524}

[Dan Beam, 2014-09-18 21:04:32]

A revert of this CL (patchset #8 id:150001) has been created in
https://codereview.chromium.org/582913003/ by dbeam@chromium.org.

The reason for reverting is: this CL might be breaking browser_tests and
interactive_ui_tests on ChromeOS because it's starting a request on startup. 
here's an example:

[15345:15369:0918/132152:ERROR:leak_tracker.h(97)] Leaked 0x11cd70da7cc0 which
was allocated by:
[15345:15369:0918/132152:ERROR:leak_tracker.h(98)] #0 0x7f44b584025e
base::debug::StackTrace::StackTrace()
#1 0x000000dfc481 base::debug::LeakTracker<>::LeakTracker()
#2 0x000000df3e8d SystemURLRequestContextGetter::SystemURLRequestContextGetter()
#3 0x000000df57f1 IOThread::InitSystemRequestContext()
#4 0x000000df570b IOThread::system_url_request_context_getter()
#5 0x000001443d9e BrowserProcessImpl::system_request_context()
#6 0x000001028a8e SafeBrowsingService::Initialize()
#7 0x0000014462c9 BrowserProcessImpl::CreateSafeBrowsingService()
#8 0x000001446196 BrowserProcessImpl::safe_browsing_service()
#9 0x0000014b9a06
ChromeResourceDispatcherHostDelegate::ChromeResourceDispatcherHostDelegate()
#10 0x0000014466dc BrowserProcessImpl::ResourceDispatcherHostCreated()
#11 0x0000008a88e5
chrome::ChromeContentBrowserClient::ResourceDispatcherHostCreated()
#12 0x7f44be489a2f
content::ResourceDispatcherHostImpl::ResourceDispatcherHostImpl()
#13 0x7f44be060f44 content::BrowserMainLoop::BrowserThreadsStarted()
#14 0x7f44be068302 base::internal::RunnableAdapter<>::Run()
#15 0x7f44be06826c base::internal::InvokeHelper<>::MakeItSo()
#16 0x7f44be06821a base::internal::Invoker<>::Run()
#17 0x7f44be54e46e base::Callback<>::Run()
#18 0x7f44be8d223b content::StartupTaskRunner::RunAllTasksNow()
#19 0x7f44be05ff97 content::BrowserMainLoop::CreateStartupTasks()
#20 0x7f44be06b824 content::BrowserMainRunnerImpl::Initialize()
#21 0x7f44be05c7df content::BrowserMain()
#22 0x7f44bdee5f6f content::RunNamedProcessTypeMain()
#23 0x7f44bdee9268 content::ContentMainRunnerImpl::Run()
#24 0x7f44bdee5505 content::ContentMain()
#25 0x0000041aa8cb content::BrowserTestBase::SetUp()
#26 0x00000173e117 InProcessBrowserTest::SetUp()
#27 0x000000671180 ExtensionBrowserTest::SetUp()
#28 0x0000006711b2 ExtensionBrowserTest::SetUp()
#29 0x0000017fb993 testing::internal::HandleSehExceptionsInMethodIfSupported<>()
#30 0x0000017e8bfe testing::internal::HandleExceptionsInMethodIfSupported<>()
#31 0x0000017dd1e3 testing::Test::Run()
#32 0x0000017dd92b testing::TestInfo::Run()
#33 0x0000017ddf1a testing::TestCase::Run()
#34 0x0000017e3413 testing::internal::UnitTestImpl::RunAllTests()
#35 0x0000017f47d3 testing::internal::HandleSehExceptionsInMethodIfSupported<>()
#36 0x0000017ea96e testing::internal::HandleExceptionsInMethodIfSupported<>()
#37 0x0000017e30b1 testing::UnitTest::Run()
#38 0x000004187e41 RUN_ALL_TESTS()
#39 0x000004186e67 base::TestSuite::Run()
#40 0x0000007fcd62 InteractiveUITestSuiteRunner::RunTestSuite()
#41 0x00000173c7b8 (anonymous
namespace)::ChromeTestLauncherDelegate::RunTestSuite()
#42 0x0000041bc2fb content::LaunchTests()
#43 0x00000173c6e9 LaunchChromeTests()
#44 0x0000007fccbf main
#45 0x7f44ad8ed76d __libc_start_main
#46 0x000000601389 <unknown>

[15345:15369:0918/132152:FATAL:leak_tracker.h(102)] Check failed: 0u == count (0
vs. 1)
#0 0x7f44b584025e base::debug::StackTrace::StackTrace()
#1 0x7f44b58d7a62 logging::LogMessage::~LogMessage()
#2 0x000000dfdd01 base::debug::LeakTracker<>::CheckForLeaks()
#3 0x000000df8e13 IOThread::CleanUp()
#4 0x7f44be0810b6 content::BrowserThreadImpl::CleanUp()
#5 0x7f44be07e852 content::BrowserProcessSubThread::CleanUp()
#6 0x7f44b59d0601 base::Thread::ThreadMain()
#7 0x7f44b59bba3c base::(anonymous namespace)::ThreadFunc()
#8 0x7f44b1aa9e9a start_thread
#9 0x7f44ad9c03fd clone.

[jianli, 2014-09-19 01:42:49]

The CQ bit was checked by jianli@chromium.org

[commit-bot: I haz the power, 2014-09-19 01:44:16]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/561943002/170001

[commit-bot: I haz the power, 2014-09-19 02:43:07]

Committed patchset #9 (id:170001) as 6c0ef6fee7a28913faf5b34c06bc6a559eec9bf7

[commit-bot: I haz the power, 2014-09-19 02:44:12]

Patchset 9 (id:??) landed as
https://crrev.com/2dc910b0b9082050e5be6c81b00dddf97ffa3e6e
Cr-Commit-Position: refs/heads/master@{#295645}