Media Galleries: Make DeviceMediaAsyncFileUtil filter directory contents.

Media Galleries: Make DeviceMediaAsyncFileUtil filter directory contents.

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

[Lei Zhang, 2014-05-21 23:15:57]

Requires https://codereview.chromium.org/298843002/

[vandebo (ex-Chrome), 2014-05-22 17:17:42]

LGTM but consider removing the refcounted class.

https://codereview.chromium.org/293033009/diff/40001/chrome/browser/media_gal...
File chrome/browser/media_galleries/fileapi/device_media_async_file_util.cc
(right):

https://codereview.chromium.org/293033009/diff/40001/chrome/browser/media_gal...
chrome/browser/media_galleries/fileapi/device_media_async_file_util.cc:192: :
public base::RefCountedThreadSafe<MediaPathFilterWrapper> {
DeviceMediaAsyncFileUtil already has a weak factory, there's no need to
introduce this thread safe ref counted object.

https://codereview.chromium.org/293033009/diff/40001/chrome/browser/media_gal...
chrome/browser/media_galleries/fileapi/device_media_async_file_util.cc:482:
callback.Run(base::File::FILE_OK, file_info);
nit: call OnDidCheckMediaForGetFileInfo() ?

https://codereview.chromium.org/293033009/diff/40001/chrome/browser/media_gal...
chrome/browser/media_galleries/fileapi/device_media_async_file_util.cc:486:
scoped_refptr<base::SequencedTaskRunner> task_runner = context->task_runner();
inline?

https://codereview.chromium.org/293033009/diff/40001/chrome/browser/media_gal...
chrome/browser/media_galleries/fileapi/device_media_async_file_util.cc:507:
scoped_refptr<base::SequencedTaskRunner> task_runner = context->task_runner();
ditto?

[Lei Zhang, 2014-05-22 18:11:11]

https://codereview.chromium.org/293033009/diff/40001/chrome/browser/media_gal...
File chrome/browser/media_galleries/fileapi/device_media_async_file_util.cc
(right):

https://codereview.chromium.org/293033009/diff/40001/chrome/browser/media_gal...
chrome/browser/media_galleries/fileapi/device_media_async_file_util.cc:192: :
public base::RefCountedThreadSafe<MediaPathFilterWrapper> {
On 2014/05/22 17:17:42, vandebo wrote:
> DeviceMediaAsyncFileUtil already has a weak factory, there's no need to
> introduce this thread safe ref counted object.

That WeakPtr factor is used on the IO thread. You can't reuse it on the media
task runner.

https://codereview.chromium.org/293033009/diff/40001/chrome/browser/media_gal...
chrome/browser/media_galleries/fileapi/device_media_async_file_util.cc:486:
scoped_refptr<base::SequencedTaskRunner> task_runner = context->task_runner();
On 2014/05/22 17:17:42, vandebo wrote:
> inline?

We have to take a reference or else |task_runner| can go away at the wrong time.
Same pattern is used all over.

[vandebo (ex-Chrome), 2014-05-22 18:22:24]

still LGTM

https://codereview.chromium.org/293033009/diff/40001/chrome/browser/media_gal...
File chrome/browser/media_galleries/fileapi/device_media_async_file_util.cc
(right):

https://codereview.chromium.org/293033009/diff/40001/chrome/browser/media_gal...
chrome/browser/media_galleries/fileapi/device_media_async_file_util.cc:486:
scoped_refptr<base::SequencedTaskRunner> task_runner = context->task_runner();
On 2014/05/22 18:11:12, Lei Zhang wrote:
> On 2014/05/22 17:17:42, vandebo wrote:
> > inline?
> 
> We have to take a reference or else |task_runner| can go away at the wrong
time.
> Same pattern is used all over.

Doesn't PostTask take a ref if it's a refcounted type?

[Lei Zhang, 2014-05-22 19:51:16]

https://codereview.chromium.org/293033009/diff/40001/chrome/browser/media_gal...
File chrome/browser/media_galleries/fileapi/device_media_async_file_util.cc
(right):

https://codereview.chromium.org/293033009/diff/40001/chrome/browser/media_gal...
chrome/browser/media_galleries/fileapi/device_media_async_file_util.cc:486:
scoped_refptr<base::SequencedTaskRunner> task_runner = context->task_runner();
On 2014/05/22 18:22:24, vandebo wrote:
> On 2014/05/22 18:11:12, Lei Zhang wrote:
> > On 2014/05/22 17:17:42, vandebo wrote:
> > > inline?
> > 
> > We have to take a reference or else |task_runner| can go away at the wrong
> time.
> > Same pattern is used all over.
> 
> Doesn't PostTask take a ref if it's a refcounted type?

PostTaskAndReplyWithResult() takes a TaskRunner*, but actually I think you are
right. The reason we had to do it in other places is because we Bind() with
base::Passed(&context).

[Lei Zhang, 2014-05-22 20:38:15]

https://codereview.chromium.org/293033009/diff/40001/chrome/browser/media_gal...
File chrome/browser/media_galleries/fileapi/device_media_async_file_util.cc
(right):

https://codereview.chromium.org/293033009/diff/40001/chrome/browser/media_gal...
chrome/browser/media_galleries/fileapi/device_media_async_file_util.cc:482:
callback.Run(base::File::FILE_OK, file_info);
On 2014/05/22 17:17:42, vandebo wrote:
> nit: call OnDidCheckMediaForGetFileInfo() ?

Done.

https://codereview.chromium.org/293033009/diff/40001/chrome/browser/media_gal...
chrome/browser/media_galleries/fileapi/device_media_async_file_util.cc:486:
scoped_refptr<base::SequencedTaskRunner> task_runner = context->task_runner();
On 2014/05/22 17:17:42, vandebo wrote:
> inline?

Done.

https://codereview.chromium.org/293033009/diff/40001/chrome/browser/media_gal...
chrome/browser/media_galleries/fileapi/device_media_async_file_util.cc:507:
scoped_refptr<base::SequencedTaskRunner> task_runner = context->task_runner();
On 2014/05/22 17:17:42, vandebo wrote:
> ditto?

Done.

[Lei Zhang, 2014-05-22 20:38:18]

The CQ bit was checked by thestig@chromium.org

[commit-bot: I haz the power, 2014-05-22 20:39:50]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/thestig@chromium.org/293033009/80001

[commit-bot: I haz the power, 2014-05-23 07:54:18]

Change committed as 272439