Media Galleries: Prune uninteresting folders when scanning.

Media Galleries: Prune uninteresting folders when scanning.

Also remove sensitive directories from scan results.

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

[Lei Zhang, 2014-02-20 22:14:56]

WIP, let me know what you think.

[vandebo (ex-Chrome), 2014-02-20 22:15:48]

upload failed.

[vandebo (ex-Chrome), 2014-02-20 23:00:22]

https://codereview.chromium.org/173853007/diff/170001/chrome/browser/extensio...
File chrome/browser/extensions/api/file_system/file_system_api.cc (right):

https://codereview.chromium.org/173853007/diff/170001/chrome/browser/extensio...
chrome/browser/extensions/api/file_system/file_system_api.cc:249:
std::vector<base::FilePath> GetGrayListedDirectories() {
It looks like FileSystemApi doesn't use your new function, which just calls into
PathService, so why not put this function in media galleries code?

https://codereview.chromium.org/173853007/diff/170001/chrome/browser/media_ga...
File chrome/browser/media_galleries/media_folder_finder.cc (right):

https://codereview.chromium.org/173853007/diff/170001/chrome/browser/media_ga...
chrome/browser/media_galleries/media_folder_finder.cc:39: base::DIR_TEMP,
It's not clear that we want to prune the temp dir... probably, but not clear. 
What do you think?

https://codereview.chromium.org/173853007/diff/170001/chrome/browser/media_ga...
chrome/browser/media_galleries/media_folder_finder.cc:230: // * Enumerating a
directory.
nit: remove *

https://codereview.chromium.org/173853007/diff/170001/chrome/browser/media_ga...
chrome/browser/media_galleries/media_folder_finder.cc:245: // * Enumerating a
file.
ditto

https://codereview.chromium.org/173853007/diff/170001/chrome/browser/media_ga...
File chrome/browser/media_galleries/media_folder_finder.h (right):

https://codereview.chromium.org/173853007/diff/170001/chrome/browser/media_ga...
chrome/browser/media_galleries/media_folder_finder.h:32: const
std::vector<base::FilePath>& /*sensitive_locations*/)>
No reason to make this part of the result callback, just add a method to access
it.

https://codereview.chromium.org/173853007/diff/170001/chrome/browser/media_ga...
File chrome/browser/media_galleries/media_scan_manager_unittest.cc (right):

https://codereview.chromium.org/173853007/diff/170001/chrome/browser/media_ga...
chrome/browser/media_galleries/media_scan_manager_unittest.cc:185:
find_folders_sensitive_locations_ = sensitive_locations;
Clearly we should have a test that does remove a sensitive location.

[Lei Zhang, 2014-02-20 23:04:51]

https://codereview.chromium.org/173853007/diff/170001/chrome/browser/extensio...
File chrome/browser/extensions/api/file_system/file_system_api.cc (right):

https://codereview.chromium.org/173853007/diff/170001/chrome/browser/extensio...
chrome/browser/extensions/api/file_system/file_system_api.cc:249:
std::vector<base::FilePath> GetGrayListedDirectories() {
On 2014/02/20 23:00:23, vandebo wrote:
> It looks like FileSystemApi doesn't use your new function, which just calls
into
> PathService, so why not put this function in media galleries code?

|kGraylistedPaths| is in here. We may want to move this along with
Get/SetLastChooseEntryDirectory() in a future CL. They are all shared with media
galleries code.

https://codereview.chromium.org/173853007/diff/170001/chrome/browser/media_ga...
File chrome/browser/media_galleries/media_folder_finder.cc (right):

https://codereview.chromium.org/173853007/diff/170001/chrome/browser/media_ga...
chrome/browser/media_galleries/media_folder_finder.cc:39: base::DIR_TEMP,
On 2014/02/20 23:00:23, vandebo wrote:
> It's not clear that we want to prune the temp dir... probably, but not clear. 
> What do you think?

On Windows, it's buried and not easily accessible for the average user. I doubt
there's anything of value in there. Though it does tend to get crufty with lots
of old temp files.

[Lei Zhang, 2014-02-25 01:24:24]

https://codereview.chromium.org/173853007/diff/170001/chrome/browser/media_ga...
File chrome/browser/media_galleries/media_folder_finder.h (right):

https://codereview.chromium.org/173853007/diff/170001/chrome/browser/media_ga...
chrome/browser/media_galleries/media_folder_finder.h:32: const
std::vector<base::FilePath>& /*sensitive_locations*/)>
On 2014/02/20 23:00:23, vandebo wrote:
> No reason to make this part of the result callback, just add a method to
access
> it.

Done.

https://codereview.chromium.org/173853007/diff/170001/chrome/browser/media_ga...
File chrome/browser/media_galleries/media_scan_manager_unittest.cc (right):

https://codereview.chromium.org/173853007/diff/170001/chrome/browser/media_ga...
chrome/browser/media_galleries/media_scan_manager_unittest.cc:185:
find_folders_sensitive_locations_ = sensitive_locations;
On 2014/02/20 23:00:23, vandebo wrote:
> Clearly we should have a test that does remove a sensitive location.

Done.

[vandebo (ex-Chrome), 2014-02-25 05:06:55]

LGTM

https://codereview.chromium.org/173853007/diff/290001/chrome/browser/media_ga...
File chrome/browser/media_galleries/media_scan_manager.cc (right):

https://codereview.chromium.org/173853007/diff/290001/chrome/browser/media_ga...
chrome/browser/media_galleries/media_scan_manager.cc:231: continue;
nit: shouldn't this be a break?

[Lei Zhang, 2014-02-25 05:46:19]

sammc: chrome/browser/extensions/api/file_system/file_system_api.*
mark: DIR_USER_LIBRARY ok in chrome/common/chrome_paths.h ?

[Lei Zhang, 2014-02-25 05:46:37]

+mark for reals

[Mark Mentovai, 2014-02-25 14:02:48]

Can you explain more about what you’re trying to do?

https://codereview.chromium.org/173853007/diff/290001/chrome/common/chrome_pa...
File chrome/common/chrome_paths.cc (right):

https://codereview.chromium.org/173853007/diff/290001/chrome/common/chrome_pa...
chrome/common/chrome_paths.cc:463: if (!GetLocalLibraryDirectory(&cur))
The comment in the .h said that you wanted the user’s ~/Library, but this gets
the root /Library.

[Lei Zhang, 2014-02-25 19:00:59]

On 2014/02/25 14:02:48, Mark Mentovai wrote:
> Can you explain more about what you’re trying to do?
> 
>
https://codereview.chromium.org/173853007/diff/290001/chrome/common/chrome_pa...
> File chrome/common/chrome_paths.cc (right):
> 
>
https://codereview.chromium.org/173853007/diff/290001/chrome/common/chrome_pa...
> chrome/common/chrome_paths.cc:463: if (!GetLocalLibraryDirectory(&cur))
> The comment in the .h said that you wanted the user’s ~/Library, but this gets
> the root /Library.

I'm trying to avoid scanning directories that are very unlikely to have user
media data. Perhaps I want both /Library and ~/Library ?

[Mark Mentovai, 2014-02-25 19:01:33]

I don’t think you want to scan anything outside of the user’s home directory at
all in that case.

[Lei Zhang, 2014-02-25 19:15:45]

On 2014/02/25 19:01:33, Mark Mentovai wrote:
> I don’t think you want to scan anything outside of the user’s home directory
at
> all in that case.

Right, we don't scan / actually. I'd still like to prune ~/Library though. I
don't see a NSSearchPathDirectory enum value for it though. How about just
appending "Library" to ~/ ?

[Mark Mentovai, 2014-02-25 19:50:53]

I thought you were scanning the entire disk. I got thrown off by seeing
DIR_TEMP in your prune list, in addition to /Library, I guess. Where do you
set the root of your scan?

You want to create a GetUserLibraryDirectory function much like
GetLocalLibraryDirectory. GetUserLibraryDirectory should call
GetUserDirectory with NSLibraryDirectory as its first argument. The
NSSearchPath constants all require a directory type (like
NSLibraryDirectory meaning "Library") and a domain (like NSUserDomainMask
meaning "home directory.")

I would also encourage you to not prune DIR_TEMP or DIR_CACHE on Mac
because they are under the user library directory. I would encourage you to
prune ~/Applications (NSApplicationDirectory in NSUserDomainMask) on Mac.
On Feb 25, 2014 2:15 PM, <thestig@chromium.org> wrote:

> On 2014/02/25 19:01:33, Mark Mentovai wrote:
>
>> I don't think you want to scan anything outside of the user's home
>> directory
>>
> at
>
>> all in that case.
>>
>
> Right, we don't scan / actually. I'd still like to prune ~/Library though.
> I
> don't see a NSSearchPathDirectory enum value for it though. How about just
> appending "Library" to ~/ ?
>
> https://codereview.chromium.org/173853007/
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[Lei Zhang, 2014-02-25 22:03:56]

On 2014/02/25 19:50:53, Mark Mentovai wrote:
> I thought you were scanning the entire disk. I got thrown off by seeing
> DIR_TEMP in your prune list, in addition to /Library, I guess. Where do you
> set the root of your scan?

I'm scanning DIR_HOME and any other mounted volumes except for the volume
mounted on /.

> I would also encourage you to not prune DIR_TEMP or DIR_CACHE on Mac
> because they are under the user library directory. I would encourage you to
> prune ~/Applications (NSApplicationDirectory in NSUserDomainMask) on Mac.
> On Feb 25, 2014 2:15 PM, <mailto:thestig@chromium.org> wrote:

Are you discouraging pruning DIR_TEMP and DIR_CACHE because they are under
~/Library, so if I prune ~/Library, they would be redundant?

[Sam McNally, 2014-02-25 22:31:50]

chrome/browser/extensions/api/file_system/file_system_api.* lgtm

[Mark Mentovai, 2014-02-26 00:58:46]

thestig@chromium.org wrote:

> I'm scanning DIR_HOME and any other mounted volumes except for the volume
> mounted on /.
>
>  I would also encourage you to not prune DIR_TEMP or DIR_CACHE on Mac
>> because they are under the user library directory. I would encourage you
>> to
>> prune ~/Applications (NSApplicationDirectory in NSUserDomainMask) on Mac.
>> On Feb 25, 2014 2:15 PM, <mailto:thestig@chromium.org> wrote:
>>
>
> Are you discouraging pruning DIR_TEMP and DIR_CACHE because they are under
> ~/Library, so if I prune ~/Library, they would be redundant?
>

Yes. DIR_CACHE is inside ~/Library, so it’s redundant if you’re already
excluding ~/Library. DIR_TEMP isn’t even in the home directory, it’s on the
root volume, so I don’t think you need to exclude it either.

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[Lei Zhang, 2014-02-26 00:58:53]

Mac changes / test fixes in patch set 4.

https://codereview.chromium.org/173853007/diff/290001/chrome/browser/media_ga...
File chrome/browser/media_galleries/media_scan_manager.cc (right):

https://codereview.chromium.org/173853007/diff/290001/chrome/browser/media_ga...
chrome/browser/media_galleries/media_scan_manager.cc:231: continue;
On 2014/02/25 05:06:55, vandebo wrote:
> nit: shouldn't this be a break?

Done.

[Mark Mentovai, 2014-02-26 01:00:50]

Mac LGTM as of three minutes ago (although I’d still take out DIR_CACHE and
DIR_TEMP, they’re just noise now).

[Lei Zhang, 2014-02-26 01:24:36]

On 2014/02/26 01:00:50, Mark Mentovai wrote:
> Mac LGTM as of three minutes ago (although I’d still take out DIR_CACHE and
> DIR_TEMP, they’re just noise now).

Done.

[Mark Mentovai, 2014-02-26 03:40:37]

LGTM

[Mark Mentovai, 2014-02-26 03:40:47]

(Have a nice vacation!)

[Lei Zhang, 2014-02-26 07:04:25]

The CQ bit was checked by thestig@chromium.org

[commit-bot: I haz the power, 2014-02-26 07:06:30]

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

[commit-bot: I haz the power, 2014-02-26 08:12:03]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-02-26 08:12:05]

Retried try job too often on win_rel for step(s) base_unittests, browser_tests,
interactive_ui_tests, net_unittests, unit_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...

[Lei Zhang, 2014-02-26 09:02:23]

The CQ bit was checked by thestig@chromium.org

[commit-bot: I haz the power, 2014-02-26 09:02:36]

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

[commit-bot: I haz the power, 2014-02-26 09:59:00]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-02-26 09:59:02]

Retried try job too often on win_rel for step(s) base_unittests, browser_tests,
interactive_ui_tests, net_unittests, unit_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...

[Lei Zhang, 2014-02-26 18:54:42]

The CQ bit was checked by thestig@chromium.org

[commit-bot: I haz the power, 2014-02-26 18:55:19]

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

[commit-bot: I haz the power, 2014-02-26 23:11:51]

Change committed as 253618