Merge NetworkTimeNotifier to NetworkTimeTracker

Merge NetworkTimeNotifier to NetworkTimeTracker

To remove dependencies to BrowserProcess and IOThread from NetworkTimeTracker.

Remove NetworkTimeNotifier from IOThread, add NetworkTimeTracker to BrowserProcess instead.
Change all users to use the NetworkTimeTracker instance owned by BrowserProcess.
Since NetworkTimeTracker is living in the UI thread, there is no need to have multi-thread callback chain in NetowrkTimeTracker, the only exception is sync_backend_host_impl.cc who needs a callback which can be run on the IO thread.

BUG=371470
TEST=git cl try

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

[hashimoto, 2014-05-15 11:56:52]

haitol@, could you review this?

IIUC, it's simpler to have a singleton instance of NetworkTimeTracker on the UI
thread (owned by BrowserProcess), rather than having a singleton instance of
NetworkTimeNotifier on the IO thread and communicating it with multiple
NetworkTimeTracker instances living on the UI thread.

The only user code who needs to access NetworkTime related classes from a thread
other than the UI thread is syncer::HttpBridge who runs the callback passed by
SyncBackendHostImpl.
This use case is taken care by having a wrapper function in
sync_backend_host_impl.cc.

The only expected behavior change is the usage of
NetworkTimeTracker::InitFromSavedTime.
In the current code, InitFromSavedTime is called for each NetworkTimeTracker
instance owned by a NetworkTimeService separately.
In the new code, InitFromSavedTime is called for the shared instance of
NetworkTimeTracker by each NetworkTimeService, and InitFromSavedTime only takes
care about the first call and other calls are ignored.
I think this change is not so harmful as it restores usable network time at
least. WDYT?

[haitaol1, 2014-05-15 17:28:29]

On 2014/05/15 11:56:52, hashimoto wrote:
> haitol@, could you review this?
> 
> IIUC, it's simpler to have a singleton instance of NetworkTimeTracker on the
UI
> thread (owned by BrowserProcess), rather than having a singleton instance of
> NetworkTimeNotifier on the IO thread and communicating it with multiple
> NetworkTimeTracker instances living on the UI thread.
> 
> The only user code who needs to access NetworkTime related classes from a
thread
> other than the UI thread is syncer::HttpBridge who runs the callback passed by
> SyncBackendHostImpl.
> This use case is taken care by having a wrapper function in
> sync_backend_host_impl.cc.
> 
> The only expected behavior change is the usage of
> NetworkTimeTracker::InitFromSavedTime.
> In the current code, InitFromSavedTime is called for each NetworkTimeTracker
> instance owned by a NetworkTimeService separately.
> In the new code, InitFromSavedTime is called for the shared instance of
> NetworkTimeTracker by each NetworkTimeService, and InitFromSavedTime only
takes
> care about the first call and other calls are ignored.
> I think this change is not so harmful as it restores usable network time at
> least. WDYT?

Changes look fine to me. Please verify that tracker is only used on UI other
than sync.
And you need approval for changes to other directories.

[haitaol1, 2014-05-15 17:28:52]

lgtm

[hashimoto, 2014-05-16 07:12:39]

On 2014/05/15 17:28:29, haitaol1 wrote:
> On 2014/05/15 11:56:52, hashimoto wrote:
> > haitol@, could you review this?
> > 
> > IIUC, it's simpler to have a singleton instance of NetworkTimeTracker on the
> UI
> > thread (owned by BrowserProcess), rather than having a singleton instance of
> > NetworkTimeNotifier on the IO thread and communicating it with multiple
> > NetworkTimeTracker instances living on the UI thread.
> > 
> > The only user code who needs to access NetworkTime related classes from a
> thread
> > other than the UI thread is syncer::HttpBridge who runs the callback passed
by
> > SyncBackendHostImpl.
> > This use case is taken care by having a wrapper function in
> > sync_backend_host_impl.cc.
> > 
> > The only expected behavior change is the usage of
> > NetworkTimeTracker::InitFromSavedTime.
> > In the current code, InitFromSavedTime is called for each NetworkTimeTracker
> > instance owned by a NetworkTimeService separately.
> > In the new code, InitFromSavedTime is called for the shared instance of
> > NetworkTimeTracker by each NetworkTimeService, and InitFromSavedTime only
> takes
> > care about the first call and other calls are ignored.
> > I think this change is not so harmful as it restores usable network time at
> > least. WDYT?
> 
> Changes look fine to me. Please verify that tracker is only used on UI other
> than sync.
> And you need approval for changes to other directories.

In the new code, all code which accesses NetworkTimeTracker (listed below) is on
the UI thread.
- chrome/browser/metrics/variations/variations_service.cc
- chrome/browser/network_time/network_time_service.cc
- chrome/browser/sync/glue/sync_backend_host_impl.cc
- chrome/browser/upgrade_detector_impl.cc

[hashimoto, 2014-05-16 07:20:00]

sky@, could you review changes under chrome/browser/ and chrome/test/base?
(chrome/browser/network_time/ is already covered by haitaol@)
It's essentially moving NetworkTimeNotifier from IOThread to BrowserProcess.

mmenke@, could you review changes under net?
It's just removing NetworkTimeNotifier which is merged to NetworkTimeTracker.

[mmenke, 2014-05-16 13:09:22]

On 2014/05/16 07:20:00, hashimoto wrote:
> sky@, could you review changes under chrome/browser/ and chrome/test/base?
> (chrome/browser/network_time/ is already covered by haitaol@)
> It's essentially moving NetworkTimeNotifier from IOThread to BrowserProcess.
> 
> mmenke@, could you review changes under net?
> It's just removing NetworkTimeNotifier which is merged to NetworkTimeTracker.

LGTM

[sky, 2014-05-16 17:17:42]

https://codereview.chromium.org/271853004/diff/130001/chrome/browser/sync/glu...
File chrome/browser/sync/glue/sync_backend_host_impl.cc (right):

https://codereview.chromium.org/271853004/diff/130001/chrome/browser/sync/glu...
chrome/browser/sync/glue/sync_backend_host_impl.cc:59:
content::BrowserThread::PostTask(
I'm not familiar with this code. But question. Is you're updating the network
time after posting to a separate thread won't that make the time drift?

[hashimoto, 2014-05-16 17:29:32]

https://codereview.chromium.org/271853004/diff/130001/chrome/browser/sync/glu...
File chrome/browser/sync/glue/sync_backend_host_impl.cc (right):

https://codereview.chromium.org/271853004/diff/130001/chrome/browser/sync/glu...
chrome/browser/sync/glue/sync_backend_host_impl.cc:59:
content::BrowserThread::PostTask(
On 2014/05/16 17:17:43, sky wrote:
> I'm not familiar with this code. But question. Is you're updating the network
> time after posting to a separate thread won't that make the time drift?

base::TimeTicks::Now() passed as |post_time| to UpdateNetworkTimeOnUIThread()
above is used to compensate the drift.
(In the existing code, UpdateNetworkNotifier() in network_time_tracker.cc is
doing the same thing.)

[sky, 2014-05-16 17:37:32]

LGTM

https://codereview.chromium.org/271853004/diff/130001/chrome/browser/sync/glu...
File chrome/browser/sync/glue/sync_backend_host_impl.cc (right):

https://codereview.chromium.org/271853004/diff/130001/chrome/browser/sync/glu...
chrome/browser/sync/glue/sync_backend_host_impl.cc:48: void
UpdateNetworkTimeOnUIThread(const base::Time& network_time,
nit: we generally don't use const references for times.

[hashimoto, 2014-05-19 03:42:39]

https://codereview.chromium.org/271853004/diff/130001/chrome/browser/sync/glu...
File chrome/browser/sync/glue/sync_backend_host_impl.cc (right):

https://codereview.chromium.org/271853004/diff/130001/chrome/browser/sync/glu...
chrome/browser/sync/glue/sync_backend_host_impl.cc:48: void
UpdateNetworkTimeOnUIThread(const base::Time& network_time,
On 2014/05/16 17:37:32, sky wrote:
> nit: we generally don't use const references for times.

Oh, I didn't know that base/time/time.h explicitly mentions about it.
Replaced "const base::Time&" with "base::Time" in this file and
network_time_tracker.*

UpdateNetworkTime() below is unchanged as it is used as
NetworkTimeUpdateCallback typedefed in
sync/internal_api/public/network_time_update_callback.h.
(Maybe fix later)

[hashimoto, 2014-05-19 03:43:48]

The CQ bit was checked by hashimoto@chromium.org

[commit-bot: I haz the power, 2014-05-19 03:43:58]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/hashimoto@chromium.org/271853004/150001

[commit-bot: I haz the power, 2014-05-19 05:40:50]

FYI, CQ is re-trying this CL (attempt #1).
Please consider checking whether the failures are real,
and report flakes to chrome-troopers@google.com.
The failing builders are:
  android_dbg_triggered_tests on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/android_dbg_triggered...)

[commit-bot: I haz the power, 2014-05-19 06:52:44]

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

[commit-bot: I haz the power, 2014-05-19 06:52:45]

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

[hashimoto, 2014-05-19 06:54:05]

The CQ bit was checked by hashimoto@chromium.org

[commit-bot: I haz the power, 2014-05-19 06:54:14]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/hashimoto@chromium.org/271853004/150001

[commit-bot: I haz the power, 2014-05-19 08:12:06]

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

[commit-bot: I haz the power, 2014-05-19 08:12:07]

Failed to commit the patch.
Sending        chrome/browser/browser_process.h
Sending        chrome/browser/browser_process_impl.cc
Sending        chrome/browser/browser_process_impl.h
Sending        chrome/browser/io_thread.cc
Sending        chrome/browser/io_thread.h
Sending        chrome/browser/metrics/variations/variations_service.cc
Sending        chrome/browser/network_time/network_time_service.cc
Sending        chrome/browser/network_time/network_time_service.h
Sending        chrome/browser/network_time/network_time_tracker.cc
Sending        chrome/browser/network_time/network_time_tracker.h
Sending        chrome/browser/network_time/network_time_tracker_unittest.cc
Sending        chrome/browser/sync/glue/sync_backend_host_impl.cc
Sending        chrome/browser/upgrade_detector_impl.cc
Sending        chrome/browser/upgrade_detector_impl.h
Sending        chrome/test/base/testing_browser_process.cc
Sending        chrome/test/base/testing_browser_process.h
Sending        chrome/test/base/testing_io_thread_state.cc
Sending        net/DEPS
Deleting       net/base/network_time_notifier.cc
Deleting       net/base/network_time_notifier.h
Sending        net/net.gypi
Sending        sync/internal_api/public/http_bridge.h
Transmitting file data ....................svn: Commit failed (details follow):
svn: File '/trunk/src/net/net.gypi' is out of date

[hashimoto, 2014-05-19 08:41:14]

The CQ bit was checked by hashimoto@chromium.org

[commit-bot: I haz the power, 2014-05-19 08:41:25]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/hashimoto@chromium.org/271853004/170001

[commit-bot: I haz the power, 2014-05-19 12:39:11]

FYI, CQ is re-trying this CL (attempt #1).
Please consider checking whether the failures are real,
and report flakes to chrome-troopers@google.com.
The failing builders are:
  win_chromium_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_rel/buil...)

[commit-bot: I haz the power, 2014-05-19 15:10:47]

Change committed as 271389