Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2892173003:
Add recursive pattern matching for subfolders in file_enumerator. (Closed)
DescriptionAdd recursive pattern matching for subfolders in file_enumerator.
FolderSearchPolicy parameter is added to FileEnumerator class.
MATCH_ONLY policy is default and refers to the current behaviour where
the pattern only mathces the contents of root_path, not files in
recursive subdirectories. ALL policy is the new fixed behaviour. It
matches pattern to all subdirecroties.
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.win:win10_chromium_x64_rel_ng
Review-Url: https://codereview.chromium.org/2892173003
Cr-Commit-Position: refs/heads/master@{#483920}
Committed: https://chromium.googlesource.com/chromium/src/+/829f2e0b37d0175ac65aee0cbc9a603fcc994db7
Patch Set 1 #
Total comments: 20
Patch Set 2 : Add recursive pattern matching for subfolders in file_enumerator #
Total comments: 16
Patch Set 3 : Add recursive pattern matching for subfolders in file_enumerator #
Total comments: 20
Patch Set 4 : Add recursive pattern matching for subfolders in file_enumerator #
Total comments: 2
Patch Set 5 : Add recursive pattern matching for subfolders in file_enumerator #
Messages
Total messages: 64 (36 generated)
ivafanas@gmail.com changed reviewers: + ivafanas@gmail.com
Patch fixes TODO message in base::FileEnumerator: // NOTE: the pattern only matches the contents of root_path, not files in // recursive subdirectories. // TODO(erikkay): Fix the pattern matching to work at all levels. Previous behaviour is saved and used as default. The corrected one is enabled by FolderSearchPolicy::ALL parameter.
https://codereview.chromium.org/2892173003/diff/1/base/BUILD.gn File base/BUILD.gn (right): https://codereview.chromium.org/2892173003/diff/1/base/BUILD.gn#newcode1 base/BUILD.gn:1: # Copyright (c) 2013 The Chromium Authors. All rights reserved. CL description: you shouldn’t say “fix” because it’s not clear that there’s anything wrong with the existing behavior. In fact, you retain the existing behavior as one of the possible modes. You are, however, introducing a new mode. CL description: “matches” https://codereview.chromium.org/2892173003/diff/1/base/files/file_enumerator.cc File base/files/file_enumerator.cc (right): https://codereview.chromium.org/2892173003/diff/1/base/files/file_enumerator.... base/files/file_enumerator.cc:22: return (is_dir && (file_type_ & FileEnumerator::DIRECTORIES)) || file_type_ & (is_dir ? FileEnumerator::DIRECTORIES : FileEnumerator::FILES); https://codereview.chromium.org/2892173003/diff/1/base/files/file_enumerator.h File base/files/file_enumerator.h (right): https://codereview.chromium.org/2892173003/diff/1/base/files/file_enumerator.... base/files/file_enumerator.h:89: // Recursive search will pass through folder which names matches pattern. folders whose names match the pattern https://codereview.chromium.org/2892173003/diff/1/base/files/file_enumerator.... base/files/file_enumerator.h:91: // matches pattern will be ignored within their interior. Folders with names that do not match the pattern https://codereview.chromium.org/2892173003/diff/1/base/files/file_enumerator.... base/files/file_enumerator.h:93: // Recursive search will pass through any folder and perform pattern every folder https://codereview.chromium.org/2892173003/diff/1/base/files/file_enumerator.... base/files/file_enumerator.h:126: FolderSearchPolicy folder_search_policy); I don’t see this being used anywhere yet, and there’s no BUG= giving more information about where it’s desirable. What’s the plan? https://codereview.chromium.org/2892173003/diff/1/base/files/file_enumerator_... File base/files/file_enumerator_posix.cc (right): https://codereview.chromium.org/2892173003/diff/1/base/files/file_enumerator_... base/files/file_enumerator_posix.cc:29: memset(st, 0, sizeof(*st)); Why this instead of a failure return value? https://codereview.chromium.org/2892173003/diff/1/base/files/file_enumerator_... File base/files/file_enumerator_win.cc (right): https://codereview.chromium.org/2892173003/diff/1/base/files/file_enumerator_... base/files/file_enumerator_win.cc:7: #include <Shlwapi.h> We always use lowercase names for these. https://codereview.chromium.org/2892173003/diff/1/base/files/file_enumerator_... base/files/file_enumerator_win.cc:8: Why the blank line? https://codereview.chromium.org/2892173003/diff/1/base/files/file_enumerator_... base/files/file_enumerator_win.cc:21: if (base::win::GetVersion() >= base::win::VERSION_WIN7) { Isn’t this our minimum anyway? https://codereview.chromium.org/2892173003/diff/1/base/files/file_enumerator_... base/files/file_enumerator_win.cc:24: find_data, FindExSearchNameMatch, NULL, nullptr instead of NULL
Fix code review notes https://codereview.chromium.org/2892173003/diff/1/base/files/file_enumerator.cc File base/files/file_enumerator.cc (right): https://codereview.chromium.org/2892173003/diff/1/base/files/file_enumerator.... base/files/file_enumerator.cc:22: return (is_dir && (file_type_ & FileEnumerator::DIRECTORIES)) || On 2017/05/19 17:16:11, Mark Mentovai wrote: > file_type_ & (is_dir ? FileEnumerator::DIRECTORIES : FileEnumerator::FILES); Done. It was needed to add comparison with zero. Otherwise compiler emits warning "C4800: 'int': forcing value to bool 'true' or 'false' (performance warning)". https://codereview.chromium.org/2892173003/diff/1/base/files/file_enumerator.h File base/files/file_enumerator.h (right): https://codereview.chromium.org/2892173003/diff/1/base/files/file_enumerator.... base/files/file_enumerator.h:89: // Recursive search will pass through folder which names matches pattern. On 2017/05/19 17:16:11, Mark Mentovai wrote: > folders whose names match the pattern Done. https://codereview.chromium.org/2892173003/diff/1/base/files/file_enumerator.... base/files/file_enumerator.h:91: // matches pattern will be ignored within their interior. On 2017/05/19 17:16:11, Mark Mentovai wrote: > Folders with names that do not match the pattern Done. https://codereview.chromium.org/2892173003/diff/1/base/files/file_enumerator.... base/files/file_enumerator.h:93: // Recursive search will pass through any folder and perform pattern On 2017/05/19 17:16:11, Mark Mentovai wrote: > every folder Done. https://codereview.chromium.org/2892173003/diff/1/base/files/file_enumerator.... base/files/file_enumerator.h:126: FolderSearchPolicy folder_search_policy); On 2017/05/19 17:16:11, Mark Mentovai wrote: > I don’t see this being used anywhere yet, and there’s no BUG= giving more > information about where it’s desirable. What’s the plan? It's a feature enhancement proposed in TODO message. The current behaviour looks inconsistent. Recursive search by "*.exe" pattern in a tree: foo.exe bar.exe subfolder1/foo.exe subfolder2/bar.exe .exe/file.txt returns: foo.exe bar.exe .exe/file.txt I've saved the previous behaviour and not changed uses just not to introduce possible new bugs. We are going to use this feature in our chromium fork. Looks like the feature is useful for chromium project also (at least, feature author assumed this enhancement in TODO message). https://codereview.chromium.org/2892173003/diff/1/base/files/file_enumerator_... File base/files/file_enumerator_posix.cc (right): https://codereview.chromium.org/2892173003/diff/1/base/files/file_enumerator_... base/files/file_enumerator_posix.cc:29: memset(st, 0, sizeof(*st)); On 2017/05/19 17:16:11, Mark Mentovai wrote: > Why this instead of a failure return value? I've just saved the previous behaviour: https://cs.chromium.org/chromium/src/base/files/file_enumerator_posix.cc?q=fi... https://codereview.chromium.org/2892173003/diff/1/base/files/file_enumerator_... File base/files/file_enumerator_win.cc (right): https://codereview.chromium.org/2892173003/diff/1/base/files/file_enumerator_... base/files/file_enumerator_win.cc:7: #include <Shlwapi.h> On 2017/05/19 17:16:12, Mark Mentovai wrote: > We always use lowercase names for these. Done. https://codereview.chromium.org/2892173003/diff/1/base/files/file_enumerator_... base/files/file_enumerator_win.cc:8: On 2017/05/19 17:16:12, Mark Mentovai wrote: > Why the blank line? Done. https://codereview.chromium.org/2892173003/diff/1/base/files/file_enumerator_... base/files/file_enumerator_win.cc:21: if (base::win::GetVersion() >= base::win::VERSION_WIN7) { On 2017/05/19 17:16:12, Mark Mentovai wrote: > Isn’t this our minimum anyway? Done.
The CQ bit was checked by ivafanas@yandex-team.ru to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: No L-G-T-M from a valid reviewer yet. CQ run can only be started once the patch has received an L-G-T-M from a full committer. Even if an L-G-T-M may have been provided, it was from a non-committer,_not_ a full super star committer. Committers are members of the group "project-chromium-committers". Note that this has nothing to do with OWNERS files.
Description was changed from ========== Fix subdirs pattern in file_enumerator. FolderSearchPolicy parameter is added to FileEnumerator class. MATCH_ONLY policy is default and refers to the current behaviour where the pattern only mathces the contents of root_path, not files in recursive subdirectories. ALL policy is the new fixed behaviour. It matches pattern to all subdirecroties. R=mark@chromium.org ========== to ========== FolderSearchPolicy parameter is added to FileEnumerator class. MATCH_ONLY policy is default and refers to the current behaviour where the pattern only mathces the contents of root_path, not files in recursive subdirectories. ALL policy is the new fixed behaviour. It matches pattern to all subdirecroties. R=mark@chromium.org ==========
ivafanas@yandex-team.ru changed reviewers: - ivafanas@gmail.com
Hi, Mark, Do you have any comments?
ivafanas@yandex-team.ru changed reviewers: + thestig@chromium.org
On 2017/05/25 03:13:17, ivafanas wrote: > Hi, Mark, > > Do you have any comments? Hi, Lei Zhang, Could you please review the merge request? Mark is not available for a week.
Question - where are we going with this? Do you have plans to use the new functionality somewhere?
To answer my own question - Nevermind, I didn't read the previous comments carefully enough. It may be better to write file_enumerator_unittest.cc first in a separate CL to test the existing code, before modifying the implementation in this CL. https://codereview.chromium.org/2892173003/diff/20001/base/files/file_enumera... File base/files/file_enumerator.h (right): https://codereview.chromium.org/2892173003/diff/20001/base/files/file_enumera... base/files/file_enumerator.h:142: bool IsTypeMatched(bool is_dir); const method. https://codereview.chromium.org/2892173003/diff/20001/base/files/file_enumera... base/files/file_enumerator.h:142: bool IsTypeMatched(bool is_dir); nit: This doesn't look related to ShouldSkip(). Separate with a new line. https://codereview.chromium.org/2892173003/diff/20001/base/files/file_enumera... File base/files/file_enumerator_posix.cc (left): https://codereview.chromium.org/2892173003/diff/20001/base/files/file_enumera... base/files/file_enumerator_posix.cc:65: // The Windows version of this code appends the pattern to the root_path, Weren't we trying to emulate the Windows implementation here? https://codereview.chromium.org/2892173003/diff/20001/base/files/file_enumera... File base/files/file_enumerator_posix.cc (right): https://codereview.chromium.org/2892173003/diff/20001/base/files/file_enumera... base/files/file_enumerator_posix.cc:20: void GetStat(const FilePath& path, struct stat* st, bool show_links) { Put |st|, the in-out param, after the in-params. https://codereview.chromium.org/2892173003/diff/20001/base/files/file_enumera... base/files/file_enumerator_posix.cc:143: // recursive (possible directory will not be added to penfing paths) - typo: pending https://codereview.chromium.org/2892173003/diff/20001/base/files/file_enumera... File base/files/file_enumerator_unittest.cc (right): https://codereview.chromium.org/2892173003/diff/20001/base/files/file_enumera... base/files/file_enumerator_unittest.cc:51: FilePath path; Just initialize this to some dummy value and assert that PathExists() returns false. https://codereview.chromium.org/2892173003/diff/20001/base/files/file_enumera... base/files/file_enumerator_unittest.cc:70: for (auto policy : {FileEnumerator::FolderSearchPolicy::MATCH_ONLY, Isn't this kFolderSearchPolicies? https://codereview.chromium.org/2892173003/diff/20001/base/files/file_enumera... File base/files/file_enumerator_win.cc (right): https://codereview.chromium.org/2892173003/diff/20001/base/files/file_enumera... base/files/file_enumerator_win.cc:124: const auto src = I think you should just write out FilePath here instead of auto. It's not obvious what |src| is, unlike in other cases like base::MakeUnique.
The CQ bit was checked by thestig@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)
Codereview notes were fixed. Thanks. https://codereview.chromium.org/2892173003/diff/20001/base/files/file_enumera... File base/files/file_enumerator.h (right): https://codereview.chromium.org/2892173003/diff/20001/base/files/file_enumera... base/files/file_enumerator.h:142: bool IsTypeMatched(bool is_dir); On 2017/06/01 01:37:28, Lei Zhang wrote: > const method. Done. https://codereview.chromium.org/2892173003/diff/20001/base/files/file_enumera... base/files/file_enumerator.h:142: bool IsTypeMatched(bool is_dir); On 2017/06/01 01:37:28, Lei Zhang wrote: > nit: This doesn't look related to ShouldSkip(). Separate with a new line. Done. https://codereview.chromium.org/2892173003/diff/20001/base/files/file_enumera... File base/files/file_enumerator_posix.cc (left): https://codereview.chromium.org/2892173003/diff/20001/base/files/file_enumera... base/files/file_enumerator_posix.cc:65: // The Windows version of this code appends the pattern to the root_path, On 2017/06/01 01:37:28, Lei Zhang wrote: > Weren't we trying to emulate the Windows implementation here? Yes, we are. I've implemented several unit tests which are common for win and posix. Therefor tests check that win and posix implementations have the same behaviour. This comment is not relevant more, so as posix implementation was optimized and does the same thing in a different way. https://codereview.chromium.org/2892173003/diff/20001/base/files/file_enumera... File base/files/file_enumerator_posix.cc (right): https://codereview.chromium.org/2892173003/diff/20001/base/files/file_enumera... base/files/file_enumerator_posix.cc:20: void GetStat(const FilePath& path, struct stat* st, bool show_links) { On 2017/06/01 01:37:28, Lei Zhang wrote: > Put |st|, the in-out param, after the in-params. Done. https://codereview.chromium.org/2892173003/diff/20001/base/files/file_enumera... base/files/file_enumerator_posix.cc:143: // recursive (possible directory will not be added to penfing paths) - On 2017/06/01 01:37:28, Lei Zhang wrote: > typo: pending Done. https://codereview.chromium.org/2892173003/diff/20001/base/files/file_enumera... File base/files/file_enumerator_unittest.cc (right): https://codereview.chromium.org/2892173003/diff/20001/base/files/file_enumera... base/files/file_enumerator_unittest.cc:51: FilePath path; On 2017/06/01 01:37:28, Lei Zhang wrote: > Just initialize this to some dummy value and assert that PathExists() returns > false. Done. https://codereview.chromium.org/2892173003/diff/20001/base/files/file_enumera... base/files/file_enumerator_unittest.cc:70: for (auto policy : {FileEnumerator::FolderSearchPolicy::MATCH_ONLY, On 2017/06/01 01:37:29, Lei Zhang wrote: > Isn't this kFolderSearchPolicies? Done. https://codereview.chromium.org/2892173003/diff/20001/base/files/file_enumera... File base/files/file_enumerator_win.cc (right): https://codereview.chromium.org/2892173003/diff/20001/base/files/file_enumera... base/files/file_enumerator_win.cc:124: const auto src = On 2017/06/01 01:37:29, Lei Zhang wrote: > I think you should just write out FilePath here instead of auto. It's not > obvious what |src| is, unlike in other cases like base::MakeUnique. Done.
On 2017/06/01 08:29:19, ivafanas wrote: > Codereview notes were fixed. > > .... Hi, Lei Zhang, Do you have any comments?
On 2017/06/07 06:42:43, ivafanas wrote: > On 2017/06/01 08:29:19, ivafanas wrote: > > Codereview notes were fixed. > > > > .... > > Hi, Lei Zhang, > > Do you have any comments? If you look at the trybot results from patch set 2, you can see ELFImportsTest.ChromeElfLoadSanityTest fails on Windows in release + non-component builds. This is due to the PathMatchSpec() call added by this CL. Can you take a look at that please?
On 2017/06/08 07:28:59, Lei Zhang wrote: > On 2017/06/07 06:42:43, ivafanas wrote: > > On 2017/06/01 08:29:19, ivafanas wrote: > > > Codereview notes were fixed. > > > > > > .... > > > > Hi, Lei Zhang, > > > > Do you have any comments? > > If you look at the trybot results from patch set 2, you can see > ELFImportsTest.ChromeElfLoadSanityTest fails on Windows in release + > non-component builds. This is due to the PathMatchSpec() call added by this CL. > Can you take a look at that please? Probably need to add /DELAYLOAD:shlwapi.dll in chrome_elf/BUILD.gn.
On 2017/06/08 07:35:43, Lei Zhang wrote: > On 2017/06/08 07:28:59, Lei Zhang wrote: > > On 2017/06/07 06:42:43, ivafanas wrote: > > > On 2017/06/01 08:29:19, ivafanas wrote: > > > > Codereview notes were fixed. > > > > > > > > .... > > > > > > Hi, Lei Zhang, > > > > > > Do you have any comments? > > > > If you look at the trybot results from patch set 2, you can see > > ELFImportsTest.ChromeElfLoadSanityTest fails on Windows in release + > > non-component builds. This is due to the PathMatchSpec() call added by this > CL. > > Can you take a look at that please? > > Probably need to add /DELAYLOAD:shlwapi.dll in chrome_elf/BUILD.gn. Thanks a lot! I will try.
Someone else already fixed the Win7 check in base/files/file_enumerator_win.cc because I'm too slow at reviewing. So you also need to sync your git tree. https://codereview.chromium.org/2892173003/diff/40001/base/files/file_enumera... File base/files/file_enumerator.h (right): https://codereview.chromium.org/2892173003/diff/40001/base/files/file_enumera... base/files/file_enumerator.h:163: FolderSearchPolicy folder_search_policy_; Make this const? Probably |recursive_| and |file_type_| above too. https://codereview.chromium.org/2892173003/diff/40001/base/files/file_enumera... File base/files/file_enumerator_posix.cc (right): https://codereview.chromium.org/2892173003/diff/40001/base/files/file_enumera... base/files/file_enumerator_posix.cc:101: #if !defined(OS_LINUX) && !defined(OS_MACOSX) && !defined(OS_BSD) && \ Did this list get shorter? https://codereview.chromium.org/2892173003/diff/40001/base/files/file_enumera... base/files/file_enumerator_posix.cc:156: if (IsTypeMatched(is_dir) && is_pattern_matched) You can check |is_pattern_matched| first, and if that evaluates to false, then the check can fail earlier without doing the more complicated IsTypeMatched() check. https://codereview.chromium.org/2892173003/diff/40001/base/files/file_enumera... File base/files/file_enumerator_unittest.cc (right): https://codereview.chromium.org/2892173003/diff/40001/base/files/file_enumera... base/files/file_enumerator_unittest.cc:44: bool CreateFile(const FilePath& path) { Can we call this CreateDummyFile() or CreateTestFile()? Doesn't the win32 API already have a CreateFile() function? https://codereview.chromium.org/2892173003/diff/40001/base/files/file_enumera... base/files/file_enumerator_unittest.cc:128: const auto filepath_foo = path.AppendASCII("foo.txt"); My comments about auto use carries over here too. Is "const auto filepath_foo" more readable than "const FilePath foo_txt" ? https://codereview.chromium.org/2892173003/diff/40001/base/files/file_enumera... File base/files/file_enumerator_win.cc (left): https://codereview.chromium.org/2892173003/diff/40001/base/files/file_enumera... base/files/file_enumerator_win.cc:101: src = src.Append(L"*"); // No pattern = match everything. Is this special case no longer needed in BuildSearchFilter() in the new code, because the pattern is never empty? https://codereview.chromium.org/2892173003/diff/40001/base/files/file_enumera... File base/files/file_enumerator_win.cc (right): https://codereview.chromium.org/2892173003/diff/40001/base/files/file_enumera... base/files/file_enumerator_win.cc:28: return root_path.AppendASCII("*"); Can you do Append(L"*") here to avoid an ASCIIToUTF16() call at run time in production code? Whereas using AppendASCII() for convenience in file_enumerator_unittest.cc is fine. https://codereview.chromium.org/2892173003/diff/40001/base/files/file_enumera... base/files/file_enumerator_win.cc:150: pattern_ = FILE_PATH_LITERAL("*"); You don't need FILE_PATH_LITERAL in Windows-only files. Just write L"*" here. https://codereview.chromium.org/2892173003/diff/40001/base/files/file_enumera... base/files/file_enumerator_win.cc:162: const auto abs_path = root_path_.Append(filename); Just write out FilePath. Using auto is not saving much here. https://codereview.chromium.org/2892173003/diff/40001/base/files/file_enumera... base/files/file_enumerator_win.cc:190: return pattern_.empty() || Can |pattern_| be empty?
Description was changed from ========== FolderSearchPolicy parameter is added to FileEnumerator class. MATCH_ONLY policy is default and refers to the current behaviour where the pattern only mathces the contents of root_path, not files in recursive subdirectories. ALL policy is the new fixed behaviour. It matches pattern to all subdirecroties. R=mark@chromium.org ========== to ========== FolderSearchPolicy parameter is added to FileEnumerator class. MATCH_ONLY policy is default and refers to the current behaviour where the pattern only mathces the contents of root_path, not files in recursive subdirectories. ALL policy is the new fixed behaviour. It matches pattern to all subdirecroties. R=mark@chromium.org CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.win:win10_chromium_x64_rel_ng ==========
Post-review fixes. Thanks a lot for the notes. https://codereview.chromium.org/2892173003/diff/40001/base/files/file_enumera... File base/files/file_enumerator.h (right): https://codereview.chromium.org/2892173003/diff/40001/base/files/file_enumera... base/files/file_enumerator.h:163: FolderSearchPolicy folder_search_policy_; On 2017/06/08 08:21:17, Lei Zhang wrote: > Make this const? Probably |recursive_| and |file_type_| above too. Done. https://codereview.chromium.org/2892173003/diff/40001/base/files/file_enumera... File base/files/file_enumerator_posix.cc (right): https://codereview.chromium.org/2892173003/diff/40001/base/files/file_enumera... base/files/file_enumerator_posix.cc:101: #if !defined(OS_LINUX) && !defined(OS_MACOSX) && !defined(OS_BSD) && \ On 2017/06/08 08:21:18, Lei Zhang wrote: > Did this list get shorter? Done. https://codereview.chromium.org/2892173003/diff/40001/base/files/file_enumera... base/files/file_enumerator_posix.cc:156: if (IsTypeMatched(is_dir) && is_pattern_matched) On 2017/06/08 08:21:18, Lei Zhang wrote: > You can check |is_pattern_matched| first, and if that evaluates to false, then > the check can fail earlier without doing the more complicated IsTypeMatched() > check. Done. https://codereview.chromium.org/2892173003/diff/40001/base/files/file_enumera... File base/files/file_enumerator_unittest.cc (right): https://codereview.chromium.org/2892173003/diff/40001/base/files/file_enumera... base/files/file_enumerator_unittest.cc:44: bool CreateFile(const FilePath& path) { On 2017/06/08 08:21:18, Lei Zhang wrote: > Can we call this CreateDummyFile() or CreateTestFile()? Doesn't the win32 API > already have a CreateFile() function? Done. https://codereview.chromium.org/2892173003/diff/40001/base/files/file_enumera... base/files/file_enumerator_unittest.cc:128: const auto filepath_foo = path.AppendASCII("foo.txt"); On 2017/06/08 08:21:18, Lei Zhang wrote: > My comments about auto use carries over here too. > > Is "const auto filepath_foo" more readable than "const FilePath foo_txt" ? Done. https://codereview.chromium.org/2892173003/diff/40001/base/files/file_enumera... File base/files/file_enumerator_win.cc (left): https://codereview.chromium.org/2892173003/diff/40001/base/files/file_enumera... base/files/file_enumerator_win.cc:101: src = src.Append(L"*"); // No pattern = match everything. On 2017/06/08 08:21:19, Lei Zhang wrote: > Is this special case no longer needed in BuildSearchFilter() in the new code, > because the pattern is never empty? The pattern is never empty. Yes, there is always-false check below in IsPatternMatched method, I'll fix that. https://codereview.chromium.org/2892173003/diff/40001/base/files/file_enumera... File base/files/file_enumerator_win.cc (right): https://codereview.chromium.org/2892173003/diff/40001/base/files/file_enumera... base/files/file_enumerator_win.cc:28: return root_path.AppendASCII("*"); On 2017/06/08 08:21:19, Lei Zhang wrote: > Can you do Append(L"*") here to avoid an ASCIIToUTF16() call at run time in > production code? > > Whereas using AppendASCII() for convenience in file_enumerator_unittest.cc is > fine. Done. https://codereview.chromium.org/2892173003/diff/40001/base/files/file_enumera... base/files/file_enumerator_win.cc:150: pattern_ = FILE_PATH_LITERAL("*"); On 2017/06/08 08:21:18, Lei Zhang wrote: > You don't need FILE_PATH_LITERAL in Windows-only files. Just write L"*" here. Done. https://codereview.chromium.org/2892173003/diff/40001/base/files/file_enumera... base/files/file_enumerator_win.cc:162: const auto abs_path = root_path_.Append(filename); On 2017/06/08 08:21:19, Lei Zhang wrote: > Just write out FilePath. Using auto is not saving much here. Done. https://codereview.chromium.org/2892173003/diff/40001/base/files/file_enumera... base/files/file_enumerator_win.cc:190: return pattern_.empty() || On 2017/06/08 08:21:19, Lei Zhang wrote: > Can |pattern_| be empty? You are right, it can not. Fixed.
The CQ bit was checked by ivafanas@yandex-team.ru to run a CQ dry run
The CQ bit was unchecked by ivafanas@yandex-team.ru
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: No L-G-T-M from a valid reviewer yet. CQ run can only be started once the patch has received an L-G-T-M from a full committer. Even if an L-G-T-M may have been provided, it was from a non-committer,_not_ a full super star committer. Committers are members of the group "project-chromium-committers". Note that this has nothing to do with OWNERS files.
The CQ bit was checked by thestig@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Description was changed from ========== FolderSearchPolicy parameter is added to FileEnumerator class. MATCH_ONLY policy is default and refers to the current behaviour where the pattern only mathces the contents of root_path, not files in recursive subdirectories. ALL policy is the new fixed behaviour. It matches pattern to all subdirecroties. R=mark@chromium.org CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.win:win10_chromium_x64_rel_ng ========== to ========== Add recursive pattern matching for subfolders in file_enumerator. FolderSearchPolicy parameter is added to FileEnumerator class. MATCH_ONLY policy is default and refers to the current behaviour where the pattern only mathces the contents of root_path, not files in recursive subdirectories. ALL policy is the new fixed behaviour. It matches pattern to all subdirecroties. CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.win:win10_chromium_x64_rel_ng ==========
thestig@chromium.org changed reviewers: + robertshield@chromium.org
lgtm +robertshield for chrome_elf/ I also did some light editing on the CL description.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: ios-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...) mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...) mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
Can you check that ChromeElfSanityCheck runs correctly in a Release build on Windows? A quick look at shlwapi.dll's imports suggests it imports user32.dll, if the test fails then this CL would break chrome_elf which depends on maintaining a restricted set of imports.
scottmg@chromium.org changed reviewers: + scottmg@chromium.org
(I don't really know why chrome_elf needs FileEnumerator off the top of my head, any chance we could remove that instead of adding shlwapi?) https://codereview.chromium.org/2892173003/diff/60001/base/files/file_enumera... File base/files/file_enumerator_posix.cc (right): https://codereview.chromium.org/2892173003/diff/60001/base/files/file_enumera... base/files/file_enumerator_posix.cc:127: while (readdir_r(dir, &dent_buf, &dent) == 0 && dent) { Please make sure the readdir_r->readdir, and the ".." case for OS_FUCHSIA is maintained when this is rebased.
On 2017/06/29 05:33:24, robertshield wrote: > Can you check that ChromeElfSanityCheck runs correctly in a Release build on > Windows? > > A quick look at shlwapi.dll's imports suggests it imports user32.dll, if the > test fails then this CL would break chrome_elf which depends on maintaining a > restricted set of imports. I've built chrome_elf_unittests locally on windows 10 with "is_debug = false" in gn args and "ELFImportsTest.ChromeElfSanityCheck" test passed. May be there is another option in gn args or I'm doing something wrong?
readdir_r->readdir and the ".." case for OS_FUCHSIA is maintained after rebase. https://codereview.chromium.org/2892173003/diff/60001/base/files/file_enumera... File base/files/file_enumerator_posix.cc (right): https://codereview.chromium.org/2892173003/diff/60001/base/files/file_enumera... base/files/file_enumerator_posix.cc:127: while (readdir_r(dir, &dent_buf, &dent) == 0 && dent) { On 2017/06/29 23:44:21, scottmg (ooo Jul1-Jul24) wrote: > Please make sure the readdir_r->readdir, and the ".." case for OS_FUCHSIA is > maintained when this is rebased. Done.
The CQ bit was checked by ivafanas@yandex-team.ru to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by ivafanas@yandex-team.ru
The patchset sent to the CQ was uploaded after l-g-t-m from thestig@chromium.org Link to the patchset: https://codereview.chromium.org/2892173003/#ps80001 (title: "Add recursive pattern matching for subfolders in file_enumerator")
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
ivafanas@yandex-team.ru changed reviewers: + brettw@google.com
On 2017/06/30 07:56:18, commit-bot: I haz the power wrote: > Try jobs failed on following builders: > chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED, > http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...) brettw, Could you please review the issue?
On 2017/06/30 04:55:54, ivafanas wrote: > On 2017/06/29 05:33:24, robertshield wrote: > > Can you check that ChromeElfSanityCheck runs correctly in a Release build on > > Windows? > > > > A quick look at shlwapi.dll's imports suggests it imports user32.dll, if the > > test fails then this CL would break chrome_elf which depends on maintaining a > > restricted set of imports. > > I've built chrome_elf_unittests locally on windows 10 with "is_debug = false" in > gn args and "ELFImportsTest.ChromeElfSanityCheck" test passed. May be there is > another option in gn args or I'm doing something wrong? If the unit test passes then this is probably ok, but I strongly second Scott's request to figure out why chrome_elf is pulling in FileEnumerator. When building the chrome_elf target, which obj file(s) refer(s) to the missing symbol from shlwapi if it is _not_ linked?
LGTM in any case since the test passing means that release-mode chrome_elf.dll isn't affected. Also I just noticed the dependency is being added only to the test target, so I suspect it's not code in chrome_elf.dll that is pulling in the shlwapi dependency :)
The CQ bit was checked by ivafanas@yandex-team.ru
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
On 2017/06/30 08:06:57, ivafanas wrote: > On 2017/06/30 07:56:18, commit-bot: I haz the power wrote: > > Try jobs failed on following builders: > > chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED, > > > http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...) > > brettw, > > Could you please review the issue? brettw, Sorry, I did not notice that robertshield is an owner of chrome_elf.
CQ is committing da patch.
Bot data: {"patchset_id": 80001, "attempt_start_ts": 1498981662081190,
"parent_rev": "853276ff285e4e76711476144d387048c37e7e77", "commit_rev":
"829f2e0b37d0175ac65aee0cbc9a603fcc994db7"}
Message was sent while issue was closed.
Description was changed from ========== Add recursive pattern matching for subfolders in file_enumerator. FolderSearchPolicy parameter is added to FileEnumerator class. MATCH_ONLY policy is default and refers to the current behaviour where the pattern only mathces the contents of root_path, not files in recursive subdirectories. ALL policy is the new fixed behaviour. It matches pattern to all subdirecroties. CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.win:win10_chromium_x64_rel_ng ========== to ========== Add recursive pattern matching for subfolders in file_enumerator. FolderSearchPolicy parameter is added to FileEnumerator class. MATCH_ONLY policy is default and refers to the current behaviour where the pattern only mathces the contents of root_path, not files in recursive subdirectories. ALL policy is the new fixed behaviour. It matches pattern to all subdirecroties. CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.win:win10_chromium_x64_rel_ng Review-Url: https://codereview.chromium.org/2892173003 Cr-Commit-Position: refs/heads/master@{#483920} Committed: https://chromium.googlesource.com/chromium/src/+/829f2e0b37d0175ac65aee0cbc9a... ==========
Message was sent while issue was closed.
Committed patchset #5 (id:80001) as https://chromium.googlesource.com/chromium/src/+/829f2e0b37d0175ac65aee0cbc9a... |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
