[fsp] Make the reading operation abortable.

[fsp] Make the reading operation abortable.

Reading may be slow. We should be able to abort it to avoid unnecessary
consumption of resources such as network, cpu and memory.

TEST=browser_test: *FileSystemProvider*ReadFile*
BUG=400574
Committed: https://crrev.com/73506a4a2982ee9017636e5fa607f88b918e226e
Cr-Commit-Position: refs/heads/master@{#291645}

[mtomasz, 2014-08-22 04:50:49]

@hirono: PTAL. Thanks.

[hirono, 2014-08-22 05:43:47]

https://codereview.chromium.org/496953003/diff/20001/chrome/browser/chromeos/...
File chrome/browser/chromeos/file_system_provider/fileapi/file_stream_reader.cc
(right):

https://codereview.chromium.org/496953003/diff/20001/chrome/browser/chromeos/...
chrome/browser/chromeos/file_system_provider/fileapi/file_stream_reader.cc:68:
DCHECK_CURRENTLY_ON(BrowserThread::UI);
The indent is wrong from here?

[mtomasz, 2014-08-22 06:09:50]

https://codereview.chromium.org/496953003/diff/20001/chrome/browser/chromeos/...
File chrome/browser/chromeos/file_system_provider/fileapi/file_stream_reader.cc
(right):

https://codereview.chromium.org/496953003/diff/20001/chrome/browser/chromeos/...
chrome/browser/chromeos/file_system_provider/fileapi/file_stream_reader.cc:68:
DCHECK_CURRENTLY_ON(BrowserThread::UI);
On 2014/08/22 05:43:47, hirono wrote:
> The indent is wrong from here?

Done.

[hirono, 2014-08-22 06:44:56]

https://codereview.chromium.org/496953003/diff/40001/chrome/browser/chromeos/...
File chrome/browser/chromeos/file_system_provider/fileapi/file_stream_reader.cc
(right):

https://codereview.chromium.org/496953003/diff/40001/chrome/browser/chromeos/...
chrome/browser/chromeos/file_system_provider/fileapi/file_stream_reader.cc:74:
abort_callback_ = ProvidedFileSystemInterface::AbortCallback();
If file_system_.get() == NULL, does not it update (clear) abort_callback_?

https://codereview.chromium.org/496953003/diff/40001/chrome/browser/chromeos/...
chrome/browser/chromeos/file_system_provider/fileapi/file_stream_reader.cc:165:
file_handle_ = file_handle;
The complete callback does not clear abort_callback_.
Is it OK?

[mtomasz, 2014-08-22 08:25:19]

https://codereview.chromium.org/496953003/diff/40001/chrome/browser/chromeos/...
File chrome/browser/chromeos/file_system_provider/fileapi/file_stream_reader.cc
(right):

https://codereview.chromium.org/496953003/diff/40001/chrome/browser/chromeos/...
chrome/browser/chromeos/file_system_provider/fileapi/file_stream_reader.cc:74:
abort_callback_ = ProvidedFileSystemInterface::AbortCallback();
On 2014/08/22 06:44:55, hirono wrote:
> If file_system_.get() == NULL, does not it update (clear) abort_callback_?
> 

Hm. The aborting logic has serious flaws. Let me rework it before continuing
with this patch.

[mtomasz, 2014-08-25 00:46:32]

https://codereview.chromium.org/496953003/diff/40001/chrome/browser/chromeos/...
File chrome/browser/chromeos/file_system_provider/fileapi/file_stream_reader.cc
(right):

https://codereview.chromium.org/496953003/diff/40001/chrome/browser/chromeos/...
chrome/browser/chromeos/file_system_provider/fileapi/file_stream_reader.cc:74:
abort_callback_ = ProvidedFileSystemInterface::AbortCallback();
On 2014/08/22 08:25:19, mtomasz wrote:
> On 2014/08/22 06:44:55, hirono wrote:
> > If file_system_.get() == NULL, does not it update (clear) abort_callback_?
> > 

The abort_callback_ should store the callback to the latest abortable operation.
I removed the #74 line.

> 
> Hm. The aborting logic has serious flaws. Let me rework it before continuing
> with this patch.

I spent some time on thinking about the aborting logic, and I decided to keep it
as it is. crbug.com/406265.

[hirono, 2014-08-25 03:33:16]

lgtm with nits

https://codereview.chromium.org/496953003/diff/40001/chrome/test/data/extensi...
File chrome/test/data/extensions/api_test/file_system_provider/read_file/test.js
(right):

https://codereview.chromium.org/496953003/diff/40001/chrome/test/data/extensi...
chrome/test/data/extensions/api_test/file_system_provider/read_file/test.js:177:
fileReader.onload = function(e) {
nit: e -> event ? It looks an error.

https://codereview.chromium.org/496953003/diff/40001/chrome/test/data/extensi...
chrome/test/data/extensions/api_test/file_system_provider/read_file/test.js:180:
fileReader.onerror = function(e) {
ditto

https://codereview.chromium.org/496953003/diff/40001/chrome/test/data/extensi...
chrome/test/data/extensions/api_test/file_system_provider/read_file/test.js:233:
}, 0);
nit: fix this indent.

[mtomasz, 2014-08-25 03:58:17]

https://codereview.chromium.org/496953003/diff/40001/chrome/test/data/extensi...
File chrome/test/data/extensions/api_test/file_system_provider/read_file/test.js
(right):

https://codereview.chromium.org/496953003/diff/40001/chrome/test/data/extensi...
chrome/test/data/extensions/api_test/file_system_provider/read_file/test.js:177:
fileReader.onload = function(e) {
On 2014/08/25 03:33:15, hirono wrote:
> nit: e -> event ? It looks an error.

The 'e' naming is quite often used for events in Chromium.

/ui/file_manager
$ git grep function\(e\) | wc -l
72
$ git grep function\(event\) | wc -l
155

/chrome/browser/resources
git grep function\(e\) | wc -l
466
git grep function\(event\) | wc -l
353

If we choose event, over e, then I'd like to do it in a separate patch, since
all of my tests for File System Provider API use "e" as events. WDYT?

https://codereview.chromium.org/496953003/diff/40001/chrome/test/data/extensi...
chrome/test/data/extensions/api_test/file_system_provider/read_file/test.js:233:
}, 0);
On 2014/08/25 03:33:15, hirono wrote:
> nit: fix this indent.

Done.

[hirono, 2014-08-25 04:05:26]

https://codereview.chromium.org/496953003/diff/40001/chrome/test/data/extensi...
File chrome/test/data/extensions/api_test/file_system_provider/read_file/test.js
(right):

https://codereview.chromium.org/496953003/diff/40001/chrome/test/data/extensi...
chrome/test/data/extensions/api_test/file_system_provider/read_file/test.js:177:
fileReader.onload = function(e) {
On 2014/08/25 03:58:17, mtomasz wrote:
> On 2014/08/25 03:33:15, hirono wrote:
> > nit: e -> event ? It looks an error.
> 
> The 'e' naming is quite often used for events in Chromium.
> 
> /ui/file_manager
> $ git grep function\(e\) | wc -l
> 72
> $ git grep function\(event\) | wc -l
> 155
> 
> /chrome/browser/resources
> git grep function\(e\) | wc -l
> 466
> git grep function\(event\) | wc -l
> 353
> 
> If we choose event, over e, then I'd like to do it in a separate patch, since
> all of my tests for File System Provider API use "e" as events. WDYT?

Thanks for investigation. This is nit. Keeping 'e' is also OK. Up to you.

[mtomasz, 2014-08-25 05:24:47]

The CQ bit was checked by mtomasz@chromium.org

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

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

[commit-bot: I haz the power, 2014-08-25 06:17:33]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  win_chromium_rel_swarming on tryserver.chromium.win
(http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

[commit-bot: I haz the power, 2014-08-25 07:29:56]

Committed patchset #6 (100001) as 3eec8dfe752173a255839365ca1a116642794345

[commit-bot: I haz the power, 2014-09-10 02:33:12]

Patchset 6 (id:??) landed as
https://crrev.com/73506a4a2982ee9017636e5fa607f88b918e226e
Cr-Commit-Position: refs/heads/master@{#291645}