[SyncFS] Introduce CallbackTracker to handle callback abortion

[SyncFS] Introduce CallbackTracker to handle callback abortion

Adding a helper class for partial reconstruction of SyncFS system.

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

[tzik, 2014-06-04 06:56:10]

PTL

[peria, 2014-06-04 07:39:21]

https://codereview.chromium.org/310383002/diff/1/chrome/browser/sync_file_sys...
File chrome/browser/sync_file_system/drive_backend/callback_tracker.h (right):

https://codereview.chromium.org/310383002/diff/1/chrome/browser/sync_file_sys...
chrome/browser/sync_file_system/drive_backend/callback_tracker.h:94: #include
"chrome/browser/sync_file_system/drive_backend/callback_tracker_internal.h"
Putting a header file on the bottom line seems storage.

How about moving AbortHelper class into _internal.h(.pump) file?
Then you can put this #include on the head of this file.

[tzik, 2014-06-04 08:15:56]

https://codereview.chromium.org/310383002/diff/1/chrome/browser/sync_file_sys...
File chrome/browser/sync_file_system/drive_backend/callback_tracker.h (right):

https://codereview.chromium.org/310383002/diff/1/chrome/browser/sync_file_sys...
chrome/browser/sync_file_system/drive_backend/callback_tracker.h:94: #include
"chrome/browser/sync_file_system/drive_backend/callback_tracker_internal.h"
On 2014/06/04 07:39:21, peria wrote:
> Putting a header file on the bottom line seems storage.
> 
> How about moving AbortHelper class into _internal.h(.pump) file?
> Then you can put this #include on the head of this file.

Done.

[peria, 2014-06-04 08:24:41]

LGTM with a nit.

https://codereview.chromium.org/310383002/diff/20001/chrome/browser/sync_file...
File chrome/browser/sync_file_system/drive_backend/callback_tracker_internal.h
(right):

https://codereview.chromium.org/310383002/diff/20001/chrome/browser/sync_file...
chrome/browser/sync_file_system/drive_backend/callback_tracker_internal.h:44:
template <typename>
Duplicate with just above lines.
Remove either one.

https://codereview.chromium.org/310383002/diff/20001/chrome/browser/sync_file...
File
chrome/browser/sync_file_system/drive_backend/callback_tracker_internal.h.pump
(right):

https://codereview.chromium.org/310383002/diff/20001/chrome/browser/sync_file...
chrome/browser/sync_file_system/drive_backend/callback_tracker_internal.h.pump:48:
template <typename>
ditto.

[tzik, 2014-06-04 08:48:34]

https://codereview.chromium.org/310383002/diff/20001/chrome/browser/sync_file...
File chrome/browser/sync_file_system/drive_backend/callback_tracker_internal.h
(right):

https://codereview.chromium.org/310383002/diff/20001/chrome/browser/sync_file...
chrome/browser/sync_file_system/drive_backend/callback_tracker_internal.h:44:
template <typename>
On 2014/06/04 08:24:41, peria wrote:
> Duplicate with just above lines.
> Remove either one.

Done.

https://codereview.chromium.org/310383002/diff/20001/chrome/browser/sync_file...
File
chrome/browser/sync_file_system/drive_backend/callback_tracker_internal.h.pump
(right):

https://codereview.chromium.org/310383002/diff/20001/chrome/browser/sync_file...
chrome/browser/sync_file_system/drive_backend/callback_tracker_internal.h.pump:48:
template <typename>
On 2014/06/04 08:24:41, peria wrote:
> ditto.

Done.

[nhiroki, 2014-06-04 09:16:47]

lgtm

https://codereview.chromium.org/310383002/diff/20001/chrome/browser/sync_file...
File chrome/browser/sync_file_system/drive_backend/callback_tracker.cc (right):

https://codereview.chromium.org/310383002/diff/20001/chrome/browser/sync_file...
chrome/browser/sync_file_system/drive_backend/callback_tracker.cc:14:
AbortHelper::AbortHelper(CallbackTracker* tracker)
Why don't you move these definitions of AbortHelper into
"callback_tracker_internal.cc"?

https://codereview.chromium.org/310383002/diff/20001/chrome/browser/sync_file...
File chrome/browser/sync_file_system/drive_backend/callback_tracker.h (right):

https://codereview.chromium.org/310383002/diff/20001/chrome/browser/sync_file...
chrome/browser/sync_file_system/drive_backend/callback_tracker.h:64:
AbortClosureByHelper helpers_;  // Owns Wrapper.
What's "Wrapper"?

https://codereview.chromium.org/310383002/diff/20001/chrome/browser/sync_file...
File chrome/browser/sync_file_system/drive_backend/callback_tracker_unittest.cc
(right):

https://codereview.chromium.org/310383002/diff/20001/chrome/browser/sync_file...
chrome/browser/sync_file_system/drive_backend/callback_tracker_unittest.cc:56: }
nit: can you add an empty line?

[nhiroki, 2014-06-04 09:17:50]

Can you update BUG=?

[tzik, 2014-06-04 09:45:24]

https://codereview.chromium.org/310383002/diff/20001/chrome/browser/sync_file...
File chrome/browser/sync_file_system/drive_backend/callback_tracker.cc (right):

https://codereview.chromium.org/310383002/diff/20001/chrome/browser/sync_file...
chrome/browser/sync_file_system/drive_backend/callback_tracker.cc:14:
AbortHelper::AbortHelper(CallbackTracker* tracker)
On 2014/06/04 09:16:47, nhiroki wrote:
> Why don't you move these definitions of AbortHelper into
> "callback_tracker_internal.cc"?

Done.

https://codereview.chromium.org/310383002/diff/20001/chrome/browser/sync_file...
File chrome/browser/sync_file_system/drive_backend/callback_tracker.h (right):

https://codereview.chromium.org/310383002/diff/20001/chrome/browser/sync_file...
chrome/browser/sync_file_system/drive_backend/callback_tracker.h:64:
AbortClosureByHelper helpers_;  // Owns Wrapper.
On 2014/06/04 09:16:47, nhiroki wrote:
> What's "Wrapper"?

Ah, it's the old name of Helper.
Updated!

https://codereview.chromium.org/310383002/diff/20001/chrome/browser/sync_file...
File chrome/browser/sync_file_system/drive_backend/callback_tracker_unittest.cc
(right):

https://codereview.chromium.org/310383002/diff/20001/chrome/browser/sync_file...
chrome/browser/sync_file_system/drive_backend/callback_tracker_unittest.cc:56: }
On 2014/06/04 09:16:47, nhiroki wrote:
> nit: can you add an empty line?

Done.

[tzik, 2014-06-04 10:44:30]

The CQ bit was checked by tzik@chromium.org

[commit-bot: I haz the power, 2014-06-04 10:46:36]

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

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

FYI, CQ is re-trying this CL (attempt #1).
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-06-04 19:15:04]

Change committed as 274883