[FileAPI] Add file open ID and close callback

[FileAPI] Add file open ID and close callback

This CL changes opened file tracking from URL based to sequence number based.
On close file sequence, current impl passes its URL only to FileSystemOperation to notify file close, and new version calls a callback registered on file open.
The callback can have more context than URL, such as, which file was opened.

BUG=220029, 235736
TEST='no functional change. should not break existing test.'

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

[tzik, 2013-04-25 03:22:24]

Hi, all.
I'd like to change file Open-Close sequence for better open file tracking.
Could you take a look?

Owners of the file which I'm changing are:

kinaba:
  chrome/browser/chromeos/drive/
  webkit/chromeos/fileapi/

yzshen:
  content/renderer/pepper/
  webkit/plugins/ppapi/

kinuko:
  webkit/fileapi/
  content/browser/fileapi/
  content/common/fileapi/

[kinuko, 2013-04-25 05:55:52]

(higher-level comments first)

https://codereview.chromium.org/14472008/diff/5001/content/browser/fileapi/fi...
File content/browser/fileapi/fileapi_message_filter.h (right):

https://codereview.chromium.org/14472008/diff/5001/content/browser/fileapi/fi...
content/browser/fileapi/fileapi_message_filter.h:202: std::map<int,
base::Closure> close_file_callbacks_;
If we don't have strong reason not to do so can we use base::IDMap?

https://codereview.chromium.org/14472008/diff/5001/webkit/fileapi/file_system...
File webkit/fileapi/file_system_operation.h (right):

https://codereview.chromium.org/14472008/diff/5001/webkit/fileapi/file_system...
webkit/fileapi/file_system_operation.h:73: const base::Closure& close_callback,
documentation?

Also close_callback makes me feel that the implementor needs to pass
ClosePlatformFile callback or something like that.

Maybe onclose_callback?

[tzik, 2013-04-25 06:43:46]

Thanks! Updated!

https://codereview.chromium.org/14472008/diff/5001/content/browser/fileapi/fi...
File content/browser/fileapi/fileapi_message_filter.h (right):

https://codereview.chromium.org/14472008/diff/5001/content/browser/fileapi/fi...
content/browser/fileapi/fileapi_message_filter.h:202: std::map<int,
base::Closure> close_file_callbacks_;
On 2013/04/25 05:55:52, kinuko wrote:
> If we don't have strong reason not to do so can we use base::IDMap?

Done.

https://codereview.chromium.org/14472008/diff/5001/webkit/fileapi/file_system...
File webkit/fileapi/file_system_operation.h (right):

https://codereview.chromium.org/14472008/diff/5001/webkit/fileapi/file_system...
webkit/fileapi/file_system_operation.h:73: const base::Closure& close_callback,
On 2013/04/25 05:55:52, kinuko wrote:
> documentation?
> 
> Also close_callback makes me feel that the implementor needs to pass
> ClosePlatformFile callback or something like that.
> 
> Maybe onclose_callback?

Done.

[kinuko, 2013-04-25 09:10:14]

lgtm

[kinaba, 2013-04-25 09:31:13]

https://codereview.chromium.org/14472008/diff/11001/content/browser/fileapi/f...
File content/browser/fileapi/fileapi_message_filter.cc (right):

https://codereview.chromium.org/14472008/diff/11001/content/browser/fileapi/f...
content/browser/fileapi/fileapi_message_filter.cc:470: on_close_callback->Run();
If I understand correctly, you need to
delete on_close_callback;
manually. Or you can use IDMap<Closure, IDMapOwnPointer>,
but then call ->Run() before Remove.

Same for OnChannelClosing().

[tzik, 2013-04-25 10:38:25]

Updated!

https://codereview.chromium.org/14472008/diff/11001/content/browser/fileapi/f...
File content/browser/fileapi/fileapi_message_filter.cc (right):

https://codereview.chromium.org/14472008/diff/11001/content/browser/fileapi/f...
content/browser/fileapi/fileapi_message_filter.cc:470: on_close_callback->Run();
On 2013/04/25 09:31:14, kinaba wrote:
> If I understand correctly, you need to
> delete on_close_callback;
> manually. Or you can use IDMap<Closure, IDMapOwnPointer>,
> but then call ->Run() before Remove.
> 
> Same for OnChannelClosing().

Done. Thanks!

[kinaba, 2013-04-25 14:12:14]

lgtm

[tzik, 2013-04-26 04:22:17]

Adding Cris as a content/common/fileapi/*_messages.h OWNERS.

Cris: Please take a look to
content/common/fileapi/file_system_messages.h
change.

I'm adding a sequential number of file open/close to match the pairs.

[Cris Neckar, 2013-04-29 16:49:19]

IPC security audit LGTM

[yzshen1, 2013-04-29 17:36:56]

On 2013/04/29 16:49:19, Cris Neckar wrote:
> IPC security audit LGTM

pepper LGTM

Thanks!

[commit-bot: I haz the power, 2013-04-30 03:26:47]

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

[commit-bot: I haz the power, 2013-04-30 05:57:45]

Change committed as 197290