Change the type of file_type parameter to int, as the parameter actually takes or-ed bitmasks,

Change the type of file_type parameter to int, as the parameter actually takes or-ed bitmasks,
to remove static_cast<> in the callers.

BUG=139130
TEST=try bots

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=151192

[Haruki Sato, 2012-08-06 04:28:27]

Main changes are in file_util* files and
each of the caller files has a few lines of change removing
static_cast<file_util::FileEnumerator::FileType>.

[Haruki Sato, 2012-08-06 04:47:14]

Added several more reviewers.

Brett, could you take a look at this as an OWNER of base/ ?

Mattias, kalman, jar,
Could you review the changes in the caller files?

Thanks.

[not at google - send to devlin, 2012-08-06 05:04:56]

extensions lgtm

(those kFooBar changes were already like that, so I don't really mind if you
choose not to fix them up)

http://codereview.chromium.org/10855002/diff/32/chrome/browser/extensions/api...
File chrome/browser/extensions/api/serial/serial_port_enumerator_posix.cc
(right):

http://codereview.chromium.org/10855002/diff/32/chrome/browser/extensions/api...
chrome/browser/extensions/api/serial/serial_port_enumerator_posix.cc:69: const
int FILES_AND_SYM_LINKS =
should be kFilesAndSymLinks

http://codereview.chromium.org/10855002/diff/32/chrome/browser/extensions/san...
File chrome/browser/extensions/sandboxed_unpacker_unittest.cc (right):

http://codereview.chromium.org/10855002/diff/32/chrome/browser/extensions/san...
chrome/browser/extensions/sandboxed_unpacker_unittest.cc:138: int files_and_dirs
=
should be kFilesAndDirs

http://codereview.chromium.org/10855002/diff/32/chrome/common/extensions/exte...
File chrome/common/extensions/extension_file_util.cc (right):

http://codereview.chromium.org/10855002/diff/32/chrome/common/extensions/exte...
chrome/common/extensions/extension_file_util.cc:658:
file_util::FileEnumerator::FILES));
extra parens seem unnecessary

[kochi, 2012-08-06 05:35:25]

lgtm

[satorux1, 2012-08-06 06:09:01]

LGTM with a suggestion:

http://codereview.chromium.org/10855002/diff/32/base/file_util.h
File base/file_util.h (right):

http://codereview.chromium.org/10855002/diff/32/base/file_util.h#newcode520
base/file_util.h:520: // |file_type| specifies whether the enumerator should
match files,
What about:


|file_type|, a bit mask of |FileType|, specifies whether the enumerator should
match files,

[Mattias Nissler (ping if slow), 2012-08-06 07:29:33]

LGTM for chrome/browser/policy

[jar (doing other things), 2012-08-06 18:27:01]

Only provided readability nits.  Generally, you don't need extra parens around
an argument, and there are a lots of places where code wraps ahead of time, and
introduces a bunch of vertical space (that doesn't seem to help).

Some of my comments *might* not look as good... but I suspect most nits would
help.

http://codereview.chromium.org/10855002/diff/32/base/file_util.cc
File base/file_util.cc (right):

http://codereview.chromium.org/10855002/diff/32/base/file_util.cc#newcode173
base/file_util.cc:173: (FileEnumerator::FILES | FileEnumerator::DIRECTORIES));
nit: parens don't seem needed (or add to readability).

http://codereview.chromium.org/10855002/diff/32/base/file_util_posix.cc
File base/file_util_posix.cc (right):

http://codereview.chromium.org/10855002/diff/32/base/file_util_posix.cc#newco...
base/file_util_posix.cc:227: path,
nit: IMO, put lines 227 and 228 on line 226.

It is probably fine to start the third arg on a new line (indented 4 spaces, as
you did).

http://codereview.chromium.org/10855002/diff/32/base/file_util_unittest.cc
File base/file_util_unittest.cc (right):

http://codereview.chromium.org/10855002/diff/32/base/file_util_unittest.cc#ne...
base/file_util_unittest.cc:1865: (FILES_AND_DIRECTORIES |
file_util::FileEnumerator::INCLUDE_DOT_DOT));
nit: I don't see value in the paren around this or expression.

IMO, line 1864 should just be on the end of line 1863.

http://codereview.chromium.org/10855002/diff/32/chrome/browser/chromeos/gdata...
File chrome/browser/chromeos/gdata/gdata_cache.cc (right):

http://codereview.chromium.org/10855002/diff/32/chrome/browser/chromeos/gdata...
chrome/browser/chromeos/gdata/gdata_cache.cc:186:
file_util::FileEnumerator::SHOW_SYM_LINKS),
nit: IMO, no need for paren around arg.

http://codereview.chromium.org/10855002/diff/32/chrome/browser/chromeos/gdata...
File chrome/browser/chromeos/gdata/gdata_cache_metadata.cc (right):

http://codereview.chromium.org/10855002/diff/32/chrome/browser/chromeos/gdata...
chrome/browser/chromeos/gdata/gdata_cache_metadata.cc:115: false,  // not
recursive
nit: Just as you did in gdata_cache_unittest.cc, I don't think working to have
args 1 and 2 (lines 114, 115) on separate lines is helpful.  Either put them on
a single line, or push 114 onto the end of 113.

http://codereview.chromium.org/10855002/diff/32/chrome/browser/chromeos/login...
File chrome/browser/chromeos/login/camera_detector.cc (right):

http://codereview.chromium.org/10855002/diff/32/chrome/browser/chromeos/login...
chrome/browser/chromeos/login/camera_detector.cc:61: false,  // Don't recurse.
nit: suggest putting line 61 onto end of line 60.

http://codereview.chromium.org/10855002/diff/32/chrome/browser/chromeos/login...
chrome/browser/chromeos/login/camera_detector.cc:62: (FileEnumerator::FILES |
FileEnumerator::SHOW_SYM_LINKS));
nit: suggest romoving parens around this arg.

http://codereview.chromium.org/10855002/diff/32/chrome/browser/extensions/san...
File chrome/browser/extensions/sandboxed_unpacker_unittest.cc (right):

http://codereview.chromium.org/10855002/diff/32/chrome/browser/extensions/san...
chrome/browser/extensions/sandboxed_unpacker_unittest.cc:140:
file_util::FileEnumerator::FILES;
nit: push either line 140 or 139 onto the end of 138.

http://codereview.chromium.org/10855002/diff/32/chrome/browser/policy/app_pac...
File chrome/browser/policy/app_pack_updater.cc (right):

http://codereview.chromium.org/10855002/diff/32/chrome/browser/policy/app_pac...
chrome/browser/policy/app_pack_updater.cc:281: FileEnumerator::FILES |
FileEnumerator::DIRECTORIES |
nit: push one or more elements of line 281/282 onto line 280.

http://codereview.chromium.org/10855002/diff/32/chrome/browser/ui/webui/chrom...
File chrome/browser/ui/webui/chromeos/drive_internals_ui.cc (right):

http://codereview.chromium.org/10855002/diff/32/chrome/browser/ui/webui/chrom...
chrome/browser/ui/webui/chromeos/drive_internals_ui.cc:54: options);
nit: no reason to wrap these... and if you do wrap, wrap at the paren (rather
than at a 4 space indent), pushing line 52 onto teh end of line 51.

http://codereview.chromium.org/10855002/diff/32/chrome/common/zip.cc
File chrome/common/zip.cc (right):

http://codereview.chromium.org/10855002/diff/32/chrome/common/zip.cc#newcode134
chrome/common/zip.cc:134: src_dir, true,  // recursive
nit: consider pushing args back onto line 133.

http://codereview.chromium.org/10855002/diff/32/chrome/installer/setup/uninst...
File chrome/installer/setup/uninstall.cc (right):

http://codereview.chromium.org/10855002/diff/32/chrome/installer/setup/uninst...
chrome/installer/setup/uninstall.cc:472: (FileEnumerator::FILES |
FileEnumerator::DIRECTORIES));
nit: remove parens around arg 3;

Consider pushing 470 and 471 onto line 469.

http://codereview.chromium.org/10855002/diff/32/chrome/installer/util/duplica...
File chrome/installer/util/duplicate_tree_detector.cc (right):

http://codereview.chromium.org/10855002/diff/32/chrome/installer/util/duplica...
chrome/installer/util/duplicate_tree_detector.cc:34: (FileEnumerator::FILES |
FileEnumerator::DIRECTORIES));
nit: remove extra paren around arg 3, and push lines 32 and 33 onto line 31.

http://codereview.chromium.org/10855002/diff/32/chrome/tools/profiles/generat...
File chrome/tools/profiles/generate_profile.cc (right):

http://codereview.chromium.org/10855002/diff/32/chrome/tools/profiles/generat...
chrome/tools/profiles/generate_profile.cc:272: profile.GetPath(), false,
nit: push line 272 onto the end of line 271.

http://codereview.chromium.org/10855002/diff/32/content/browser/renderer_host...
File content/browser/renderer_host/pepper/pepper_file_message_filter.cc (right):

http://codereview.chromium.org/10855002/diff/32/content/browser/renderer_host...
content/browser/renderer_host/pepper/pepper_file_message_filter.cc:221:
full_path, false,
nit: push line 221 onto the end of 220.

http://codereview.chromium.org/10855002/diff/32/webkit/fileapi/file_system_da...
File webkit/fileapi/file_system_database_test_helper.cc (right):

http://codereview.chromium.org/10855002/diff/32/webkit/fileapi/file_system_da...
webkit/fileapi/file_system_database_test_helper.cc:23: db_path, false /*
recursive */,
nit: push line 23 onto the end of line 22.

http://codereview.chromium.org/10855002/diff/32/webkit/fileapi/isolated_file_...
File webkit/fileapi/isolated_file_util_unittest.cc (right):

http://codereview.chromium.org/10855002/diff/32/webkit/fileapi/isolated_file_...
webkit/fileapi/isolated_file_util_unittest.cc:328: (FileEnumerator::FILES |
FileEnumerator::DIRECTORIES));
nit: remove extra parents around arg 3.

http://codereview.chromium.org/10855002/diff/32/webkit/fileapi/local_file_uti...
File webkit/fileapi/local_file_util.cc (right):

http://codereview.chromium.org/10855002/diff/32/webkit/fileapi/local_file_uti...
webkit/fileapi/local_file_util.cc:146: file_path, root_url.path(), recursive,
nit:push 146 onto end of 145.

http://codereview.chromium.org/10855002/diff/32/webkit/fileapi/native_file_ut...
File webkit/fileapi/native_file_util.cc (right):

http://codereview.chromium.org/10855002/diff/32/webkit/fileapi/native_file_ut...
webkit/fileapi/native_file_util.cc:162: root_path, recursive,
nit: push line 162 onto end of line 161.

[Haruki Sato, 2012-08-06 23:22:17]

Thank you for the quick review!

jar, I tried to push as many args to the 
to reduce vertical spaces. Thank you for the advice!
I'm keeping some parentheses when the or-ed expression has multiple lines and is
a part of a complex expr/statement (e.g. an argument for the constructor),
as I think they help in that case.
Do you prefer removing all those parentheses?

http://codereview.chromium.org/10855002/diff/32/base/file_util.h
File base/file_util.h (right):

http://codereview.chromium.org/10855002/diff/32/base/file_util.h#newcode520
base/file_util.h:520: // |file_type| specifies whether the enumerator should
match files,
On 2012/08/06 06:09:01, satorux1 wrote:
> What about:
> 
> 
> |file_type|, a bit mask of |FileType|, specifies whether the enumerator should
> match files,

Done.
Thanks.

http://codereview.chromium.org/10855002/diff/32/base/file_util_posix.cc
File base/file_util_posix.cc (right):

http://codereview.chromium.org/10855002/diff/32/base/file_util_posix.cc#newco...
base/file_util_posix.cc:227: path,
On 2012/08/06 18:27:02, jar wrote:
> nit: IMO, put lines 227 and 228 on line 226.
> 
> It is probably fine to start the third arg on a new line (indented 4 spaces,
as
> you did).

Done.
Thank you for the thourough review.

http://codereview.chromium.org/10855002/diff/32/base/file_util_unittest.cc
File base/file_util_unittest.cc (right):

http://codereview.chromium.org/10855002/diff/32/base/file_util_unittest.cc#ne...
base/file_util_unittest.cc:1865: (FILES_AND_DIRECTORIES |
file_util::FileEnumerator::INCLUDE_DOT_DOT));
On 2012/08/06 18:27:02, jar wrote:
> nit: I don't see value in the paren around this or expression.
> 
> IMO, line 1864 should just be on the end of line 1863.

Done.

http://codereview.chromium.org/10855002/diff/32/chrome/browser/chromeos/gdata...
File chrome/browser/chromeos/gdata/gdata_cache.cc (right):

http://codereview.chromium.org/10855002/diff/32/chrome/browser/chromeos/gdata...
chrome/browser/chromeos/gdata/gdata_cache.cc:186:
file_util::FileEnumerator::SHOW_SYM_LINKS),
On 2012/08/06 18:27:02, jar wrote:
> nit: IMO, no need for paren around arg.

I think it helps when the expression occupies multiple lines.
Do you prefer to remove these parentheses?

http://codereview.chromium.org/10855002/diff/32/chrome/browser/chromeos/gdata...
File chrome/browser/chromeos/gdata/gdata_cache_metadata.cc (right):

http://codereview.chromium.org/10855002/diff/32/chrome/browser/chromeos/gdata...
chrome/browser/chromeos/gdata/gdata_cache_metadata.cc:115: false,  // not
recursive
On 2012/08/06 18:27:02, jar wrote:
> nit: Just as you did in gdata_cache_unittest.cc, I don't think working to have
> args 1 and 2 (lines 114, 115) on separate lines is helpful.  Either put them
on
> a single line, or push 114 onto the end of 113.

Done.
Thanks.

http://codereview.chromium.org/10855002/diff/32/chrome/browser/chromeos/login...
File chrome/browser/chromeos/login/camera_detector.cc (right):

http://codereview.chromium.org/10855002/diff/32/chrome/browser/chromeos/login...
chrome/browser/chromeos/login/camera_detector.cc:61: false,  // Don't recurse.
On 2012/08/06 18:27:02, jar wrote:
> nit: suggest putting line 61 onto end of line 60.

Done.

http://codereview.chromium.org/10855002/diff/32/chrome/browser/chromeos/login...
chrome/browser/chromeos/login/camera_detector.cc:62: (FileEnumerator::FILES |
FileEnumerator::SHOW_SYM_LINKS));
On 2012/08/06 18:27:02, jar wrote:
> nit: suggest romoving parens around this arg.

Done.

http://codereview.chromium.org/10855002/diff/32/chrome/browser/extensions/api...
File chrome/browser/extensions/api/serial/serial_port_enumerator_posix.cc
(right):

http://codereview.chromium.org/10855002/diff/32/chrome/browser/extensions/api...
chrome/browser/extensions/api/serial/serial_port_enumerator_posix.cc:69: const
int FILES_AND_SYM_LINKS =
On 2012/08/06 05:04:56, kalman wrote:
> should be kFilesAndSymLinks

I agree. Can I do that in another CL?

http://codereview.chromium.org/10855002/diff/32/chrome/browser/extensions/san...
File chrome/browser/extensions/sandboxed_unpacker_unittest.cc (right):

http://codereview.chromium.org/10855002/diff/32/chrome/browser/extensions/san...
chrome/browser/extensions/sandboxed_unpacker_unittest.cc:138: int files_and_dirs
=
On 2012/08/06 05:04:56, kalman wrote:
> should be kFilesAndDirs
Good catch. I will follow up with another CL.

http://codereview.chromium.org/10855002/diff/32/chrome/browser/extensions/san...
chrome/browser/extensions/sandboxed_unpacker_unittest.cc:140:
file_util::FileEnumerator::FILES;
On 2012/08/06 18:27:02, jar wrote:
> nit: push either line 140 or 139 onto the end of 138.

Done.

http://codereview.chromium.org/10855002/diff/32/chrome/browser/policy/app_pac...
File chrome/browser/policy/app_pack_updater.cc (right):

http://codereview.chromium.org/10855002/diff/32/chrome/browser/policy/app_pac...
chrome/browser/policy/app_pack_updater.cc:281: FileEnumerator::FILES |
FileEnumerator::DIRECTORIES |
On 2012/08/06 18:27:02, jar wrote:
> nit: push one or more elements of line 281/282 onto line 280.

Done.

http://codereview.chromium.org/10855002/diff/32/chrome/browser/ui/webui/chrom...
File chrome/browser/ui/webui/chromeos/drive_internals_ui.cc (right):

http://codereview.chromium.org/10855002/diff/32/chrome/browser/ui/webui/chrom...
chrome/browser/ui/webui/chromeos/drive_internals_ui.cc:54: options);
On 2012/08/06 18:27:02, jar wrote:
> nit: no reason to wrap these... and if you do wrap, wrap at the paren (rather
> than at a 4 space indent), pushing line 52 onto teh end of line 51.

Done.
Thanks!

http://codereview.chromium.org/10855002/diff/32/chrome/common/extensions/exte...
File chrome/common/extensions/extension_file_util.cc (right):

http://codereview.chromium.org/10855002/diff/32/chrome/common/extensions/exte...
chrome/common/extensions/extension_file_util.cc:658:
file_util::FileEnumerator::FILES));
On 2012/08/06 05:04:56, kalman wrote:
> extra parens seem unnecessary
Ack. replied to another comment.

http://codereview.chromium.org/10855002/diff/32/chrome/common/zip.cc
File chrome/common/zip.cc (right):

http://codereview.chromium.org/10855002/diff/32/chrome/common/zip.cc#newcode134
chrome/common/zip.cc:134: src_dir, true,  // recursive
On 2012/08/06 18:27:02, jar wrote:
> nit: consider pushing args back onto line 133.

Done.

http://codereview.chromium.org/10855002/diff/32/chrome/installer/setup/uninst...
File chrome/installer/setup/uninstall.cc (right):

http://codereview.chromium.org/10855002/diff/32/chrome/installer/setup/uninst...
chrome/installer/setup/uninstall.cc:472: (FileEnumerator::FILES |
FileEnumerator::DIRECTORIES));
On 2012/08/06 18:27:02, jar wrote:
> nit: remove parens around arg 3;
> 
> Consider pushing 470 and 471 onto line 469.

Done.
Thanks.

http://codereview.chromium.org/10855002/diff/32/chrome/installer/util/duplica...
File chrome/installer/util/duplicate_tree_detector.cc (right):

http://codereview.chromium.org/10855002/diff/32/chrome/installer/util/duplica...
chrome/installer/util/duplicate_tree_detector.cc:34: (FileEnumerator::FILES |
FileEnumerator::DIRECTORIES));
On 2012/08/06 18:27:02, jar wrote:
> nit: remove extra paren around arg 3, and push lines 32 and 33 onto line 31.

Done.

http://codereview.chromium.org/10855002/diff/32/chrome/tools/profiles/generat...
File chrome/tools/profiles/generate_profile.cc (right):

http://codereview.chromium.org/10855002/diff/32/chrome/tools/profiles/generat...
chrome/tools/profiles/generate_profile.cc:272: profile.GetPath(), false,
On 2012/08/06 18:27:02, jar wrote:
> nit: push line 272 onto the end of line 271.

Done.

http://codereview.chromium.org/10855002/diff/32/content/browser/renderer_host...
File content/browser/renderer_host/pepper/pepper_file_message_filter.cc (right):

http://codereview.chromium.org/10855002/diff/32/content/browser/renderer_host...
content/browser/renderer_host/pepper/pepper_file_message_filter.cc:221:
full_path, false,
On 2012/08/06 18:27:02, jar wrote:
> nit: push line 221 onto the end of 220.

Done.

http://codereview.chromium.org/10855002/diff/32/webkit/fileapi/file_system_da...
File webkit/fileapi/file_system_database_test_helper.cc (right):

http://codereview.chromium.org/10855002/diff/32/webkit/fileapi/file_system_da...
webkit/fileapi/file_system_database_test_helper.cc:23: db_path, false /*
recursive */,
On 2012/08/06 18:27:02, jar wrote:
> nit: push line 23 onto the end of line 22.

Done.

http://codereview.chromium.org/10855002/diff/32/webkit/fileapi/isolated_file_...
File webkit/fileapi/isolated_file_util_unittest.cc (right):

http://codereview.chromium.org/10855002/diff/32/webkit/fileapi/isolated_file_...
webkit/fileapi/isolated_file_util_unittest.cc:328: (FileEnumerator::FILES |
FileEnumerator::DIRECTORIES));
On 2012/08/06 18:27:02, jar wrote:
> nit: remove extra parents around arg 3.

Done.

http://codereview.chromium.org/10855002/diff/32/webkit/fileapi/local_file_uti...
File webkit/fileapi/local_file_util.cc (right):

http://codereview.chromium.org/10855002/diff/32/webkit/fileapi/local_file_uti...
webkit/fileapi/local_file_util.cc:146: file_path, root_url.path(), recursive,
On 2012/08/06 18:27:02, jar wrote:
> nit:push 146 onto end of 145.

Done.

http://codereview.chromium.org/10855002/diff/32/webkit/fileapi/native_file_ut...
File webkit/fileapi/native_file_util.cc (right):

http://codereview.chromium.org/10855002/diff/32/webkit/fileapi/native_file_ut...
webkit/fileapi/native_file_util.cc:162: root_path, recursive,
On 2012/08/06 18:27:02, jar wrote:
> nit: push line 162 onto end of line 161.

Done.

[not at google - send to devlin, 2012-08-06 23:28:18]

http://codereview.chromium.org/10855002/diff/32/chrome/browser/extensions/api...
File chrome/browser/extensions/api/serial/serial_port_enumerator_posix.cc
(right):

http://codereview.chromium.org/10855002/diff/32/chrome/browser/extensions/api...
chrome/browser/extensions/api/serial/serial_port_enumerator_posix.cc:69: const
int FILES_AND_SYM_LINKS =
On 2012/08/06 23:22:18, Haruki Sato wrote:
> On 2012/08/06 05:04:56, kalman wrote:
> > should be kFilesAndSymLinks
> 
> I agree. Can I do that in another CL?

I guess, though, easy enough to do now if at all.

http://codereview.chromium.org/10855002/diff/32/chrome/common/extensions/exte...
File chrome/common/extensions/extension_file_util.cc (right):

http://codereview.chromium.org/10855002/diff/32/chrome/common/extensions/exte...
chrome/common/extensions/extension_file_util.cc:658:
file_util::FileEnumerator::FILES));
On 2012/08/06 23:22:18, Haruki Sato wrote:
> On 2012/08/06 05:04:56, kalman wrote:
> > extra parens seem unnecessary
> Ack. replied to another comment.

You could assign to a kFilesAndDirectories constant like the other examples?

[jar (doing other things), 2012-08-07 01:47:43]

LGTM for base.  See nit below.

http://codereview.chromium.org/10855002/diff/32/chrome/browser/chromeos/gdata...
File chrome/browser/chromeos/gdata/gdata_cache.cc (right):

http://codereview.chromium.org/10855002/diff/32/chrome/browser/chromeos/gdata...
chrome/browser/chromeos/gdata/gdata_cache.cc:186:
file_util::FileEnumerator::SHOW_SYM_LINKS),
These examples are IMO borderline... but they provide a nice wrapping point....
so you can keep 'em IMO.

On 2012/08/06 23:22:18, Haruki Sato wrote:
> On 2012/08/06 18:27:02, jar wrote:
> > nit: IMO, no need for paren around arg.
> 
> I think it helps when the expression occupies multiple lines.
> Do you prefer to remove these parentheses?

http://codereview.chromium.org/10855002/diff/32/chrome/browser/extensions/api...
File chrome/browser/extensions/api/serial/serial_port_enumerator_posix.cc
(right):

http://codereview.chromium.org/10855002/diff/32/chrome/browser/extensions/api...
chrome/browser/extensions/api/serial/serial_port_enumerator_posix.cc:69: const
int FILES_AND_SYM_LINKS =
+1  The name is all in this file... so it should be straight forward.  Probably
change DEV_ROOT while your'e at it.

On 2012/08/06 23:28:18, kalman wrote:
> On 2012/08/06 23:22:18, Haruki Sato wrote:
> > On 2012/08/06 05:04:56, kalman wrote:
> > > should be kFilesAndSymLinks
> > 
> > I agree. Can I do that in another CL?
> 
> I guess, though, easy enough to do now if at all.

[Haruki Sato, 2012-08-07 02:29:17]

http://codereview.chromium.org/10855002/diff/32/chrome/browser/extensions/api...
File chrome/browser/extensions/api/serial/serial_port_enumerator_posix.cc
(right):

http://codereview.chromium.org/10855002/diff/32/chrome/browser/extensions/api...
chrome/browser/extensions/api/serial/serial_port_enumerator_posix.cc:69: const
int FILES_AND_SYM_LINKS =
On 2012/08/07 01:47:43, jar wrote:
> +1  The name is all in this file... so it should be straight forward. 
Probably
> change DEV_ROOT while your'e at it.
> 
> On 2012/08/06 23:28:18, kalman wrote:
> > On 2012/08/06 23:22:18, Haruki Sato wrote:
> > > On 2012/08/06 05:04:56, kalman wrote:
> > > > should be kFilesAndSymLinks
> > > 
> > > I agree. Can I do that in another CL?
> > 
> > I guess, though, easy enough to do now if at all.
> 

Done.
Could you double check?

http://codereview.chromium.org/10855002/diff/32/chrome/common/extensions/exte...
File chrome/common/extensions/extension_file_util.cc (right):

http://codereview.chromium.org/10855002/diff/32/chrome/common/extensions/exte...
chrome/common/extensions/extension_file_util.cc:658:
file_util::FileEnumerator::FILES));
On 2012/08/06 23:28:18, kalman wrote:
> On 2012/08/06 23:22:18, Haruki Sato wrote:
> > On 2012/08/06 05:04:56, kalman wrote:
> > > extra parens seem unnecessary
> > Ack. replied to another comment.
> 
> You could assign to a kFilesAndDirectories constant like the other examples?

Done.

[not at google - send to devlin, 2012-08-07 02:43:00]

lgtm

[not at google - send to devlin, 2012-08-07 02:43:26]

(for extensions, in response to your comment about checking it again)

[commit-bot: I haz the power, 2012-08-07 03:42:52]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/haruki@chromium.org/10855002/3008

[commit-bot: I haz the power, 2012-08-07 03:43:02]

Presubmit check for 10855002-3008 failed and returned exit status 1.

Running presubmit commit checks ...

** Presubmit Messages **
You might be calling functions intended only for testing from
production code.  It is OK to ignore this warning if you know what
you are doing, as the heuristics used to detect the situation are
not perfect.  The commit queue will not block on this warning.
Email joi@chromium.org if you have questions.
  chrome/browser/chromeos/gdata/gdata_cache.cc:750
    metadata_->ForceRescanForTesting(cache_paths_); \
  chrome/browser/chromeos/gdata/gdata_cache_metadata.cc:307
    virtual void ForceRescanForTesting( \
  chrome/browser/chromeos/gdata/gdata_cache_metadata.cc:429
    virtual void ForceRescanForTesting(

** Presubmit ERRORS **
Missing LGTM from an OWNER for files in these directories:
    content/browser/renderer_host
    chrome/test/webdriver
    chrome/installer
    chrome
    webkit/fileapi

Presubmit checks took 2.7s to calculate.

[Haruki Sato, 2012-08-07 04:38:21]

Adding several more reviewers for OWNERS reviews for the directories.
ben@ for content/browser/renderer_host
kkania@ for chrome/test/webdriver
grt@ for chrome/installer
ericu@ for webkit/fileapi

Everyone, could you review the changes in those directories?


Sorry for spamming you and missing these reviewers in the first place.

Thanks.

[kinuko, 2012-08-07 05:22:46]

webkit/fileapi lgtm

On 2012/08/07 04:38:21, Haruki Sato wrote:
> Adding several more reviewers for OWNERS reviews for the directories.
> ben@ for content/browser/renderer_host
> kkania@ for chrome/test/webdriver
> grt@ for chrome/installer
> ericu@ for webkit/fileapi
> 
> Everyone, could you review the changes in those directories?
> 
> 
> Sorry for spamming you and missing these reviewers in the first place.
> 
> Thanks.

[kinuko, 2012-08-07 05:23:11]

some nits.  (still lgtm)

http://codereview.chromium.org/10855002/diff/3008/base/file_util.h
File base/file_util.h (right):

http://codereview.chromium.org/10855002/diff/3008/base/file_util.h#newcode520
base/file_util.h:520: // |file_type|, a bit mask of |FileType|, specifies
whether the enumerator
nit: you won't need '|' for FileType

http://codereview.chromium.org/10855002/diff/3008/webkit/fileapi/file_system_...
File webkit/fileapi/file_system_database_test_helper.cc (right):

http://codereview.chromium.org/10855002/diff/3008/webkit/fileapi/file_system_...
webkit/fileapi/file_system_database_test_helper.cc:24:
file_util::FileEnumerator::FILES));
nit: parens not needed?  (here and elsewhere.  I only marked fileapi ones)

http://codereview.chromium.org/10855002/diff/3008/webkit/fileapi/file_system_...
File webkit/fileapi/file_system_directory_database.cc (right):

http://codereview.chromium.org/10855002/diff/3008/webkit/fileapi/file_system_...
webkit/fileapi/file_system_directory_database.cc:278:
file_util::FileEnumerator::FILES));
extra parens

http://codereview.chromium.org/10855002/diff/3008/webkit/fileapi/local_file_u...
File webkit/fileapi/local_file_util.cc (right):

http://codereview.chromium.org/10855002/diff/3008/webkit/fileapi/local_file_u...
webkit/fileapi/local_file_util.cc:146: (file_util::FileEnumerator::FILES |
ditto

http://codereview.chromium.org/10855002/diff/3008/webkit/fileapi/native_file_...
File webkit/fileapi/native_file_util.cc (right):

http://codereview.chromium.org/10855002/diff/3008/webkit/fileapi/native_file_...
webkit/fileapi/native_file_util.cc:162: (file_util::FileEnumerator::FILES |
ditto

http://codereview.chromium.org/10855002/diff/3008/webkit/fileapi/obfuscated_f...
File webkit/fileapi/obfuscated_file_util.cc (right):

http://codereview.chromium.org/10855002/diff/3008/webkit/fileapi/obfuscated_f...
webkit/fileapi/obfuscated_file_util.cc:999: (file_util::FileEnumerator::FILES |
ditto

[Haruki Sato, 2012-08-07 13:34:25]

I now understand no-parentheses is our style here.
Revised those styles.
Thank you jar, kalman, kinuko for your thorough review!

http://codereview.chromium.org/10855002/diff/3008/base/file_util.h
File base/file_util.h (right):

http://codereview.chromium.org/10855002/diff/3008/base/file_util.h#newcode520
base/file_util.h:520: // |file_type|, a bit mask of |FileType|, specifies
whether the enumerator
On 2012/08/07 05:23:11, kinuko wrote:
> nit: you won't need '|' for FileType

Done.
Thanks!

http://codereview.chromium.org/10855002/diff/3008/webkit/fileapi/file_system_...
File webkit/fileapi/file_system_database_test_helper.cc (right):

http://codereview.chromium.org/10855002/diff/3008/webkit/fileapi/file_system_...
webkit/fileapi/file_system_database_test_helper.cc:24:
file_util::FileEnumerator::FILES));
On 2012/08/07 05:23:11, kinuko wrote:
> nit: parens not needed?  (here and elsewhere.  I only marked fileapi ones)

Done.

http://codereview.chromium.org/10855002/diff/3008/webkit/fileapi/file_system_...
File webkit/fileapi/file_system_directory_database.cc (right):

http://codereview.chromium.org/10855002/diff/3008/webkit/fileapi/file_system_...
webkit/fileapi/file_system_directory_database.cc:278:
file_util::FileEnumerator::FILES));
On 2012/08/07 05:23:11, kinuko wrote:
> extra parens

Done.

http://codereview.chromium.org/10855002/diff/3008/webkit/fileapi/local_file_u...
File webkit/fileapi/local_file_util.cc (right):

http://codereview.chromium.org/10855002/diff/3008/webkit/fileapi/local_file_u...
webkit/fileapi/local_file_util.cc:146: (file_util::FileEnumerator::FILES |
On 2012/08/07 05:23:11, kinuko wrote:
> ditto

Done.

http://codereview.chromium.org/10855002/diff/3008/webkit/fileapi/native_file_...
File webkit/fileapi/native_file_util.cc (right):

http://codereview.chromium.org/10855002/diff/3008/webkit/fileapi/native_file_...
webkit/fileapi/native_file_util.cc:162: (file_util::FileEnumerator::FILES |
On 2012/08/07 05:23:11, kinuko wrote:
> ditto

Done.

http://codereview.chromium.org/10855002/diff/3008/webkit/fileapi/obfuscated_f...
File webkit/fileapi/obfuscated_file_util.cc (right):

http://codereview.chromium.org/10855002/diff/3008/webkit/fileapi/obfuscated_f...
webkit/fileapi/obfuscated_file_util.cc:999: (file_util::FileEnumerator::FILES |
On 2012/08/07 05:23:11, kinuko wrote:
> ditto

Done.

[ericu, 2012-08-07 16:34:19]

webkit/fileapi LGTM

[Ben Goodger (Google), 2012-08-07 18:07:05]

Pepper-related, so replacing myself with Brett who can review the
content/browser/renderer_host or delegate.

[Haruki Sato, 2012-08-08 08:16:45]

Thank you all for the review!

brettw@, grt@,
could you take a look?

Thanks.

[Haruki Sato, 2012-08-09 09:00:23]

Seems to me I've already got OWNERS's lgtm covering whole my change. Posting to
CQ.

Please let me know if you find anything we should fix/work on.

Thanks!

[commit-bot: I haz the power, 2012-08-09 09:00:41]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/haruki@chromium.org/10855002/6010

[commit-bot: I haz the power, 2012-08-09 09:00:57]

Presubmit check for 10855002-6010 failed and returned exit status 1.


Running presubmit commit checks ...

** Presubmit Messages **
You might be calling functions intended only for testing from
production code.  It is OK to ignore this warning if you know what
you are doing, as the heuristics used to detect the situation are
not perfect.  The commit queue will not block on this warning.
Email joi@chromium.org if you have questions.
  chrome/browser/chromeos/gdata/gdata_cache.cc:750
    metadata_->ForceRescanForTesting(cache_paths_); \
  chrome/browser/chromeos/gdata/gdata_cache_metadata.cc:307
    virtual void ForceRescanForTesting( \
  chrome/browser/chromeos/gdata/gdata_cache_metadata.cc:429
    virtual void ForceRescanForTesting(

** Presubmit ERRORS **
Missing LGTM from an OWNER for files in these directories:
    content/browser/renderer_host
    chrome/test/webdriver
    chrome/installer
    chrome

Presubmit checks took 3.0s to calculate.

[Haruki Sato, 2012-08-09 13:25:41]

oops, we still miss some OWNERS lgtm.

brettw@, grt@,
could you take a look?

[brettw, 2012-08-10 00:02:20]

base lgtm

[Haruki Sato (Google.com), 2012-08-10 14:29:11]

Thank you all for the review!
Adding to CQ.
Please let me know if you have any other concern.

Thanks!

[commit-bot: I haz the power, 2012-08-10 14:29:25]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/haruki@chromium.org/10855002/6010

[commit-bot: I haz the power, 2012-08-10 15:14:10]

Try job failure for 10855002-6010 (retry) on win_rel for step "compile" (clobber
build).
It's a second try, previously, step "compile" failed.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...

[satorux1, 2012-08-10 17:18:39]

haruki, the failure seems to be real:

E:\b\build\slave\win\build\src\chrome\installer\util\duplicate_tree_detector.cc(32)
: error C2143: syntax error : missing ')' before 'FILES'
E:\b\build\slave\win\build\src\chrome\installer\util\duplicate_tree_detector.cc(32)
: error C2661: 'file_util::FileEnumerator::FileEnumerator' : no overloaded
function takes 2 arguments
E:\b\build\slave\win\build\src\chrome\installer\util\duplicate_tree_detector.cc(32)
: error C2059: syntax error : ')'


Please fix them.

[Haruki Sato (Google.com), 2012-08-11 03:45:06]

On 2012/08/10 17:18:39, satorux1 wrote:
> haruki, the failure seems to be real:
> 
>
E:\b\build\slave\win\build\src\chrome\installer\util\duplicate_tree_detector.cc(32)
> : error C2143: syntax error : missing ')' before 'FILES'
>
E:\b\build\slave\win\build\src\chrome\installer\util\duplicate_tree_detector.cc(32)
> : error C2661: 'file_util::FileEnumerator::FileEnumerator' : no overloaded
> function takes 2 arguments
>
E:\b\build\slave\win\build\src\chrome\installer\util\duplicate_tree_detector.cc(32)
> : error C2059: syntax error : ')'
> 
> 
> Please fix them.

my bad...fixed and trying again.
Thank you for the help.

[commit-bot: I haz the power, 2012-08-11 03:45:40]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/haruki@chromium.org/10855002/11011

[commit-bot: I haz the power, 2012-08-11 04:33:29]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/haruki@chromium.org/10855002/11011

[commit-bot: I haz the power, 2012-08-12 01:57:26]

Change committed as 151192