Media Galleries: Initial media scanner implementation.

Media Galleries: Initial media scanner implementation.

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

[vandebo (ex-Chrome), 2014-01-29 18:41:45]

Generally looks good.

https://codereview.chromium.org/149363004/diff/20001/chrome/browser/media_gal...
File chrome/browser/media_galleries/fileapi/media_path_filter.cc (right):

https://codereview.chromium.org/149363004/diff/20001/chrome/browser/media_gal...
chrome/browser/media_galleries/fileapi/media_path_filter.cc:114: for (size_t i =
0; i < arraysize(kExtraSupportedImageExtensions); ++i)
nit: seems silly to add it to the list, just to iterate the list later.  Maybe
have both vector and array versions of AddExtensionosToMediaFileExtensionMap ?
or use vector_as_array from stl_util...

https://codereview.chromium.org/149363004/diff/20001/chrome/browser/media_gal...
File chrome/browser/media_galleries/fileapi/media_scanner.cc (right):

https://codereview.chromium.org/149363004/diff/20001/chrome/browser/media_gal...
chrome/browser/media_galleries/fileapi/media_scanner.cc:10: #include
"base/threading/sequenced_worker_pool.h"
nit: you may omit this - in header

https://codereview.chromium.org/149363004/diff/20001/chrome/browser/media_gal...
chrome/browser/media_galleries/fileapi/media_scanner.cc:48:
base::FileEnumerator::FileInfo file_info = enumerator.GetInfo();
Feel free to do in a follow up CL, but we want to make sure that there's at
least one file above a size threshold: images >= 200KB, videos >= 1MB, music >=
500KB.

All files should count, but we want at least one above the size cutoff.

https://codereview.chromium.org/149363004/diff/20001/chrome/browser/media_gal...
chrome/browser/media_galleries/fileapi/media_scanner.cc:54:
MediaGalleryScanFileType type = filter_callback_.Run(full_path);
Some types might be audio or video (.ogg ?)  Count those as both - probably
easiest with another enum value

https://codereview.chromium.org/149363004/diff/20001/chrome/browser/media_gal...
chrome/browser/media_galleries/fileapi/media_scanner.cc:108: 
Looks like you also need to set a bool scan_cancelled and check it in
GotScanResults()

https://codereview.chromium.org/149363004/diff/20001/chrome/browser/media_gal...
chrome/browser/media_galleries/fileapi/media_scanner.cc:113: void
MediaScanner::DoScan() {
DoScan -> ScanDirectory ?

https://codereview.chromium.org/149363004/diff/20001/chrome/browser/media_gal...
chrome/browser/media_galleries/fileapi/media_scanner.cc:139:
folders_to_scan_.pop();
nit: easier to reason about if you pop it off before scanning it.

https://codereview.chromium.org/149363004/diff/20001/chrome/browser/media_gal...
File chrome/browser/media_galleries/fileapi/media_scanner.h (right):

https://codereview.chromium.org/149363004/diff/20001/chrome/browser/media_gal...
chrome/browser/media_galleries/fileapi/media_scanner.h:5: #ifndef
CHROME_BROWSER_MEDIA_GALLERIES_FILEAPI_MEDIA_SCANNER_H_
I wouldn't put this in fileapi - it's not really part of the fileapi

https://codereview.chromium.org/149363004/diff/20001/chrome/browser/media_gal...
chrome/browser/media_galleries/fileapi/media_scanner.h:20: class MediaScanner {
MediaDirectoryFinder ?

https://codereview.chromium.org/149363004/diff/20001/chrome/browser/media_gal...
chrome/browser/media_galleries/fileapi/media_scanner.h:26: typedef
base::Callback<MediaGalleryScanFileType(const base::FilePath&)>
private ?

https://codereview.chromium.org/149363004/diff/20001/chrome/browser/media_gal...
chrome/browser/media_galleries/fileapi/media_scanner.h:41: // Cancel the scan.
The MediaScanner should be deleted afterwards.
afterwards -> after the callback is called?

https://codereview.chromium.org/149363004/diff/20001/chrome/browser/media_gal...
chrome/browser/media_galleries/fileapi/media_scanner.h:47: const
std::vector<base::FilePath>* new_folders,
nit: arg order, keep path and scan_result together

[Lei Zhang, 2014-01-30 00:10:48]

I'll do the name and location change in a bit.

https://codereview.chromium.org/149363004/diff/20001/chrome/browser/media_gal...
File chrome/browser/media_galleries/fileapi/media_path_filter.cc (right):

https://codereview.chromium.org/149363004/diff/20001/chrome/browser/media_gal...
chrome/browser/media_galleries/fileapi/media_path_filter.cc:114: for (size_t i =
0; i < arraysize(kExtraSupportedImageExtensions); ++i)
On 2014/01/29 18:41:45, vandebo wrote:
> nit: seems silly to add it to the list, just to iterate the list later.  Maybe
> have both vector and array versions of AddExtensionosToMediaFileExtensionMap ?
> or use vector_as_array from stl_util...

Done.

https://codereview.chromium.org/149363004/diff/20001/chrome/browser/media_gal...
File chrome/browser/media_galleries/fileapi/media_scanner.cc (right):

https://codereview.chromium.org/149363004/diff/20001/chrome/browser/media_gal...
chrome/browser/media_galleries/fileapi/media_scanner.cc:10: #include
"base/threading/sequenced_worker_pool.h"
On 2014/01/29 18:41:45, vandebo wrote:
> nit: you may omit this - in header

Done.

https://codereview.chromium.org/149363004/diff/20001/chrome/browser/media_gal...
chrome/browser/media_galleries/fileapi/media_scanner.cc:48:
base::FileEnumerator::FileInfo file_info = enumerator.GetInfo();
On 2014/01/29 18:41:45, vandebo wrote:
> Feel free to do in a follow up CL, but we want to make sure that there's at
> least one file above a size threshold: images >= 200KB, videos >= 1MB, music
>=
> 500KB.
> 
> All files should count, but we want at least one above the size cutoff.

Heh, I knew we had more requirements. I'll follow up in a later CL.

https://codereview.chromium.org/149363004/diff/20001/chrome/browser/media_gal...
chrome/browser/media_galleries/fileapi/media_scanner.cc:54:
MediaGalleryScanFileType type = filter_callback_.Run(full_path);
On 2014/01/29 18:41:45, vandebo wrote:
> Some types might be audio or video (.ogg ?)  Count those as both - probably
> easiest with another enum value

Done.

https://codereview.chromium.org/149363004/diff/20001/chrome/browser/media_gal...
chrome/browser/media_galleries/fileapi/media_scanner.cc:108: 
On 2014/01/29 18:41:45, vandebo wrote:
> Looks like you also need to set a bool scan_cancelled and check it in
> GotScanResults()

I removed CancelScan() and added |scan_state_|.

https://codereview.chromium.org/149363004/diff/20001/chrome/browser/media_gal...
chrome/browser/media_galleries/fileapi/media_scanner.cc:113: void
MediaScanner::DoScan() {
On 2014/01/29 18:41:45, vandebo wrote:
> DoScan -> ScanDirectory ?

Done.

https://codereview.chromium.org/149363004/diff/20001/chrome/browser/media_gal...
chrome/browser/media_galleries/fileapi/media_scanner.cc:139:
folders_to_scan_.pop();
On 2014/01/29 18:41:45, vandebo wrote:
> nit: easier to reason about if you pop it off before scanning it.

Done.

https://codereview.chromium.org/149363004/diff/20001/chrome/browser/media_gal...
File chrome/browser/media_galleries/fileapi/media_scanner.h (right):

https://codereview.chromium.org/149363004/diff/20001/chrome/browser/media_gal...
chrome/browser/media_galleries/fileapi/media_scanner.h:26: typedef
base::Callback<MediaGalleryScanFileType(const base::FilePath&)>
On 2014/01/29 18:41:45, vandebo wrote:
> private ?

Used in (anonymous)::DoScanOnBlockingPool()

https://codereview.chromium.org/149363004/diff/20001/chrome/browser/media_gal...
chrome/browser/media_galleries/fileapi/media_scanner.h:41: // Cancel the scan.
The MediaScanner should be deleted afterwards.
On 2014/01/29 18:41:45, vandebo wrote:
> afterwards -> after the callback is called?

CancelScan() does not return until the callback finishes.

Actually, I'll just get rid of CancelScan() altogether. Just delete the object
instead.

https://codereview.chromium.org/149363004/diff/20001/chrome/browser/media_gal...
chrome/browser/media_galleries/fileapi/media_scanner.h:47: const
std::vector<base::FilePath>* new_folders,
On 2014/01/29 18:41:45, vandebo wrote:
> nit: arg order, keep path and scan_result together

I don't see why it even matters, but sure.

[Lei Zhang, 2014-01-30 00:29:50]

https://codereview.chromium.org/149363004/diff/20001/chrome/browser/media_gal...
File chrome/browser/media_galleries/fileapi/media_scanner.h (right):

https://codereview.chromium.org/149363004/diff/20001/chrome/browser/media_gal...
chrome/browser/media_galleries/fileapi/media_scanner.h:5: #ifndef
CHROME_BROWSER_MEDIA_GALLERIES_FILEAPI_MEDIA_SCANNER_H_
On 2014/01/29 18:41:45, vandebo wrote:
> I wouldn't put this in fileapi - it's not really part of the fileapi

Done.

https://codereview.chromium.org/149363004/diff/20001/chrome/browser/media_gal...
chrome/browser/media_galleries/fileapi/media_scanner.h:20: class MediaScanner {
On 2014/01/29 18:41:45, vandebo wrote:
> MediaDirectoryFinder ?

Done.

[vandebo (ex-Chrome), 2014-01-30 01:03:11]

LGTM with nits

https://codereview.chromium.org/149363004/diff/80001/chrome/browser/media_gal...
File chrome/browser/media_galleries/fileapi/media_path_filter.cc (right):

https://codereview.chromium.org/149363004/diff/80001/chrome/browser/media_gal...
chrome/browser/media_galleries/fileapi/media_path_filter.cc:81: // static
remove

https://codereview.chromium.org/149363004/diff/80001/chrome/browser/media_gal...
chrome/browser/media_galleries/fileapi/media_path_filter.cc:112: return (it ==
media_file_extensions_map_.end()) ?
nit: no need for trinary here.

https://codereview.chromium.org/149363004/diff/80001/chrome/browser/media_gal...
chrome/browser/media_galleries/fileapi/media_path_filter.cc:157: for (size_t i =
0; i < additional_extensions_size; ++i) {
nit: You basically have the same loop here twice.  If this function just takes
an array, you can call it twice as many times.

https://codereview.chromium.org/149363004/diff/80001/chrome/browser/media_gal...
File chrome/browser/media_galleries/media_folder_finder.cc (right):

https://codereview.chromium.org/149363004/diff/80001/chrome/browser/media_gal...
chrome/browser/media_galleries/media_folder_finder.cc:154: if
(!new_folders->empty()) {
if isn't needed

https://codereview.chromium.org/149363004/diff/80001/chrome/browser/media_gal...
chrome/browser/media_galleries/media_folder_finder.cc:157:
CHECK(!path_to_add.empty());
Check where you use it, i.e. in ScanFolderOnBlockingPool

https://codereview.chromium.org/149363004/diff/80001/chrome/browser/media_gal...
File chrome/browser/media_galleries/media_folder_finder.h (right):

https://codereview.chromium.org/149363004/diff/80001/chrome/browser/media_gal...
chrome/browser/media_galleries/media_folder_finder.h:32: // |callback| will get
called when the scan finishes, or if this object gets
nit: .. finishes.  If the object is deleted before it finishes, the scan will
stop and |callback| will be called with success = false.

[Lei Zhang, 2014-01-30 01:35:41]

Still writing unit tests...

https://codereview.chromium.org/149363004/diff/80001/chrome/browser/media_gal...
File chrome/browser/media_galleries/fileapi/media_path_filter.cc (right):

https://codereview.chromium.org/149363004/diff/80001/chrome/browser/media_gal...
chrome/browser/media_galleries/fileapi/media_path_filter.cc:81: // static
On 2014/01/30 01:03:11, vandebo wrote:
> remove

Done.

https://codereview.chromium.org/149363004/diff/80001/chrome/browser/media_gal...
chrome/browser/media_galleries/fileapi/media_path_filter.cc:112: return (it ==
media_file_extensions_map_.end()) ?
On 2014/01/30 01:03:11, vandebo wrote:
> nit: no need for trinary here.

Done.

https://codereview.chromium.org/149363004/diff/80001/chrome/browser/media_gal...
chrome/browser/media_galleries/fileapi/media_path_filter.cc:157: for (size_t i =
0; i < additional_extensions_size; ++i) {
On 2014/01/30 01:03:11, vandebo wrote:
> nit: You basically have the same loop here twice.  If this function just takes
> an array, you can call it twice as many times.

const char* array != std::string() array. I still need two different versions of
the function.

https://codereview.chromium.org/149363004/diff/80001/chrome/browser/media_gal...
File chrome/browser/media_galleries/media_folder_finder.cc (right):

https://codereview.chromium.org/149363004/diff/80001/chrome/browser/media_gal...
chrome/browser/media_galleries/media_folder_finder.cc:154: if
(!new_folders->empty()) {
On 2014/01/30 01:03:11, vandebo wrote:
> if isn't needed

Done.

https://codereview.chromium.org/149363004/diff/80001/chrome/browser/media_gal...
chrome/browser/media_galleries/media_folder_finder.cc:157:
CHECK(!path_to_add.empty());
On 2014/01/30 01:03:11, vandebo wrote:
> Check where you use it, i.e. in ScanFolderOnBlockingPool

Done.

https://codereview.chromium.org/149363004/diff/80001/chrome/browser/media_gal...
File chrome/browser/media_galleries/media_folder_finder.h (right):

https://codereview.chromium.org/149363004/diff/80001/chrome/browser/media_gal...
chrome/browser/media_galleries/media_folder_finder.h:32: // |callback| will get
called when the scan finishes, or if this object gets
On 2014/01/30 01:03:11, vandebo wrote:
> nit: .. finishes.  If the object is deleted before it finishes, the scan will
> stop and |callback| will be called with success = false.

Done.

[Lei Zhang, 2014-01-30 12:13:23]

TBR unit test.

[commit-bot: I haz the power, 2014-01-30 12:13:36]

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

[vandebo (ex-Chrome), 2014-01-30 17:29:04]

LGTM

[commit-bot: I haz the power, 2014-01-30 18:03:37]

Change committed as 247993

[commit-bot: I haz the power, 2014-01-30 18:03:42]

CQ bit was unchecked on CL. Ignoring.