[fsp] Initial implementation of timeout logic for requests.

[fsp] Initial implementation of timeout logic for requests.

This patch introduces a basic mechanism for timing out stale requests.
If none of callbacks are called within a fixed amount of time, then the request
is considered as stale, and aborted with an FILE_ERROR_ABORT error code.

TEST=unit_test: FileSystemProviderRequestManagerTest.AbortOnTimeout
BUG=248427
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=264773

[mtomasz, 2014-04-18 04:50:07]

@hashimoto: PTAL. Thanks.

[hashimoto, 2014-04-18 05:28:08]

https://codereview.chromium.org/241673005/diff/1/chrome/browser/chromeos/file...
File chrome/browser/chromeos/file_system_provider/request_manager.cc (right):

https://codereview.chromium.org/241673005/diff/1/chrome/browser/chromeos/file...
chrome/browser/chromeos/file_system_provider/request_manager.cc:21:
RequestManager::RequestManager() : next_id_(1), timeout_(kTimeout) {}
Does this line compile?

https://codereview.chromium.org/241673005/diff/1/chrome/browser/chromeos/file...
chrome/browser/chromeos/file_system_provider/request_manager.cc:53:
base::Unretained(this),  // The timer is outlived by RequestManager.
Why don't you use WeakPtrFactory?

https://codereview.chromium.org/241673005/diff/1/chrome/browser/chromeos/file...
File chrome/browser/chromeos/file_system_provider/request_manager.h (right):

https://codereview.chromium.org/241673005/diff/1/chrome/browser/chromeos/file...
chrome/browser/chromeos/file_system_provider/request_manager.h:14: #include
"base/memory/scoped_ptr.h"
nit: This line is duplicated.

https://codereview.chromium.org/241673005/diff/1/chrome/browser/chromeos/file...
chrome/browser/chromeos/file_system_provider/request_manager.h:81: int timeout_;
Why don't you use base::TimeDelta which takes care about time units?
The same goes for SetTimeoutForTests.

https://codereview.chromium.org/241673005/diff/1/chrome/browser/chromeos/file...
File chrome/browser/chromeos/file_system_provider/request_manager_unittest.cc
(right):

https://codereview.chromium.org/241673005/diff/1/chrome/browser/chromeos/file...
chrome/browser/chromeos/file_system_provider/request_manager_unittest.cc:323:
request_manager_->SetTimeoutForTests(100);
Can't we use 0 as the timeout?
Then you can use RunUntilIdle() because the run loop never becomes idle.

[mtomasz, 2014-04-18 05:56:18]

https://codereview.chromium.org/241673005/diff/1/chrome/browser/chromeos/file...
File chrome/browser/chromeos/file_system_provider/request_manager.cc (right):

https://codereview.chromium.org/241673005/diff/1/chrome/browser/chromeos/file...
chrome/browser/chromeos/file_system_provider/request_manager.cc:21:
RequestManager::RequestManager() : next_id_(1), timeout_(kTimeout) {}
On 2014/04/18 05:28:09, hashimoto wrote:
> Does this line compile?

Oops. Done.

https://codereview.chromium.org/241673005/diff/1/chrome/browser/chromeos/file...
chrome/browser/chromeos/file_system_provider/request_manager.cc:53:
base::Unretained(this),  // The timer is outlived by RequestManager.
On 2014/04/18 05:28:09, hashimoto wrote:
> Why don't you use WeakPtrFactory?

That would be completely redundant. Timers are owned by Request, and Requests
are owned by RequestManager. So that timer is outlived by RequestManager,
therefore this is safe.

I had a similar discussion here:
https://codereview.chromium.org/223823004/diff/100001/chrome/browser/ui/ash/m...

And I was told, that using Unretained is OK for the same reason as I said above.

https://codereview.chromium.org/241673005/diff/1/chrome/browser/chromeos/file...
File chrome/browser/chromeos/file_system_provider/request_manager.h (right):

https://codereview.chromium.org/241673005/diff/1/chrome/browser/chromeos/file...
chrome/browser/chromeos/file_system_provider/request_manager.h:14: #include
"base/memory/scoped_ptr.h"
On 2014/04/18 05:28:09, hashimoto wrote:
> nit: This line is duplicated.

Done.

https://codereview.chromium.org/241673005/diff/1/chrome/browser/chromeos/file...
chrome/browser/chromeos/file_system_provider/request_manager.h:81: int timeout_;
On 2014/04/18 05:28:09, hashimoto wrote:
> Why don't you use base::TimeDelta which takes care about time units?
> The same goes for SetTimeoutForTests.

Done.

https://codereview.chromium.org/241673005/diff/1/chrome/browser/chromeos/file...
File chrome/browser/chromeos/file_system_provider/request_manager_unittest.cc
(right):

https://codereview.chromium.org/241673005/diff/1/chrome/browser/chromeos/file...
chrome/browser/chromeos/file_system_provider/request_manager_unittest.cc:323:
request_manager_->SetTimeoutForTests(100);
On 2014/04/18 05:28:09, hashimoto wrote:
> Can't we use 0 as the timeout?
> Then you can use RunUntilIdle() because the run loop never becomes idle.

Good idea. Done.

[hashimoto, 2014-04-18 06:08:00]

lgtm

https://codereview.chromium.org/241673005/diff/1/chrome/browser/chromeos/file...
File chrome/browser/chromeos/file_system_provider/request_manager.cc (right):

https://codereview.chromium.org/241673005/diff/1/chrome/browser/chromeos/file...
chrome/browser/chromeos/file_system_provider/request_manager.cc:53:
base::Unretained(this),  // The timer is outlived by RequestManager.
On 2014/04/18 05:56:18, mtomasz wrote:
> On 2014/04/18 05:28:09, hashimoto wrote:
> > Why don't you use WeakPtrFactory?
> 
> That would be completely redundant. Timers are owned by Request, and Requests
> are owned by RequestManager. So that timer is outlived by RequestManager,
> therefore this is safe.
> 
> I had a similar discussion here:
>
https://codereview.chromium.org/223823004/diff/100001/chrome/browser/ui/ash/m...
> 
> And I was told, that using Unretained is OK for the same reason as I said
above.

I'd use WeakPtr because there is a period during the destruction of
RequestManager where the timer is alive, but RequestManager is partially
destructed.
As you said, however, it's also true that it shouldn't cause problems in this
current code.
I'm OK with use of Unretained.

[mtomasz, 2014-04-18 06:19:51]

https://codereview.chromium.org/241673005/diff/1/chrome/browser/chromeos/file...
File chrome/browser/chromeos/file_system_provider/request_manager.cc (right):

https://codereview.chromium.org/241673005/diff/1/chrome/browser/chromeos/file...
chrome/browser/chromeos/file_system_provider/request_manager.cc:53:
base::Unretained(this),  // The timer is outlived by RequestManager.
On 2014/04/18 06:08:00, hashimoto wrote:
> On 2014/04/18 05:56:18, mtomasz wrote:
> > On 2014/04/18 05:28:09, hashimoto wrote:
> > > Why don't you use WeakPtrFactory?
> > 
> > That would be completely redundant. Timers are owned by Request, and
Requests
> > are owned by RequestManager. So that timer is outlived by RequestManager,
> > therefore this is safe.
> > 
> > I had a similar discussion here:
> >
>
https://codereview.chromium.org/223823004/diff/100001/chrome/browser/ui/ash/m...
> > 
> > And I was told, that using Unretained is OK for the same reason as I said
> above.
> 
> I'd use WeakPtr because there is a period during the destruction of
> RequestManager where the timer is alive, but RequestManager is partially
> destructed.
> As you said, however, it's also true that it shouldn't cause problems in this
> current code.
> I'm OK with use of Unretained.

Makes sense to me. Let's go with weak ptrs for simplicity. Done.

[mtomasz, 2014-04-18 06:19:56]

The CQ bit was checked by mtomasz@chromium.org

[mtomasz, 2014-04-18 06:20:18]

The CQ bit was unchecked by mtomasz@chromium.org

[commit-bot: I haz the power, 2014-04-18 06:20:18]

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

[mtomasz, 2014-04-18 06:20:31]

The CQ bit was unchecked by mtomasz@chromium.org

[mtomasz, 2014-04-18 08:36:26]

The CQ bit was checked by mtomasz@chromium.org

[commit-bot: I haz the power, 2014-04-18 08:36:37]

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

[commit-bot: I haz the power, 2014-04-18 11:45:45]

Change committed as 264773