Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
253343003:
Stop appending slash for directory's URL in util.isDescendantEntry() (Closed)
DescriptionStop appending slash for directory's URL in util.isDescendantEntry()
If the ancestorEntry is root directory, the URL for it ends with slash.
Appending one more slash cause failure to comparison.
I think appending slash is not necessary because we have already checked
that the ascestorEntry is directory.
BUG=361047
TEST=ran browser_tests gtest_filter=*FileManager*
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=266506
Patch Set 1 #
Total comments: 4
Patch Set 2 : Check that entries are in same file system. #
Total comments: 8
Patch Set 3 : Append traling slash for ancestor directory when it misses. #Messages
Total messages: 14 (0 generated)
Hi Tomasz, could you take a look?
https://codereview.chromium.org/253343003/diff/1/ui/file_manager/file_manager... File ui/file_manager/file_manager/common/js/util.js (left): https://codereview.chromium.org/253343003/diff/1/ui/file_manager/file_manager... ui/file_manager/file_manager/common/js/util.js:1121: if (childEntry.toURL().indexOf(ancestorEntry.toURL() + '/') !== 0) Are you sure it is impossible to get a URL without a trailing slash? Some time ago it was quite common. Please check several scenarios. Eg. for a root directory, subdirectory, etc. Thanks!
https://codereview.chromium.org/253343003/diff/1/ui/file_manager/file_manager... File ui/file_manager/file_manager/common/js/util.js (left): https://codereview.chromium.org/253343003/diff/1/ui/file_manager/file_manager... ui/file_manager/file_manager/common/js/util.js:1121: if (childEntry.toURL().indexOf(ancestorEntry.toURL() + '/') !== 0) On 2014/04/24 07:09:39, mtomasz wrote: > Are you sure it is impossible to get a URL without a trailing slash? Some time > ago it was quite common. Please check several scenarios. Eg. for a root > directory, subdirectory, etc. Thanks! According to the current implementation of toURL(), if it is not the root directory, URL is without trailing slash. But I think, if ancestorEntry is directory and anchestorEntry's URL is a prefix of childEntry's URL, we can conclude that the childEntry is under ancestorEntry. What do you think?
https://codereview.chromium.org/253343003/diff/1/ui/file_manager/file_manager... File ui/file_manager/file_manager/common/js/util.js (left): https://codereview.chromium.org/253343003/diff/1/ui/file_manager/file_manager... ui/file_manager/file_manager/common/js/util.js:1121: if (childEntry.toURL().indexOf(ancestorEntry.toURL() + '/') !== 0) On 2014/04/24 07:15:32, fukino wrote: > On 2014/04/24 07:09:39, mtomasz wrote: > > Are you sure it is impossible to get a URL without a trailing slash? Some time > > ago it was quite common. Please check several scenarios. Eg. for a root > > directory, subdirectory, etc. Thanks! > > According to the current implementation of toURL(), if it is not the root > directory, URL is without trailing slash. > But I think, if ancestorEntry is directory and anchestorEntry's URL is a prefix > of childEntry's URL, we can conclude that the childEntry is under ancestorEntry. > What do you think? This would work but only within the same file system. For different ones it would fail on: (1) filesystem:chrome-extension://extension-id/external/drive/very_long_file_name.txt (2) filesystem:chrome-extension://extension-id/external/d (1) is a file on drive (2) is a root directory on a file system with 'd' mount point name. So, we can safely remove adding the slash but we have to check earlier if both entries are on the same file system: if (!util.isSameEntry(ancestorEntry.filesystem.root, childEntry.filesystem.root)) return false; WDYT?
https://codereview.chromium.org/253343003/diff/1/ui/file_manager/file_manager... File ui/file_manager/file_manager/common/js/util.js (left): https://codereview.chromium.org/253343003/diff/1/ui/file_manager/file_manager... ui/file_manager/file_manager/common/js/util.js:1121: if (childEntry.toURL().indexOf(ancestorEntry.toURL() + '/') !== 0) On 2014/04/24 07:46:53, mtomasz wrote: > On 2014/04/24 07:15:32, fukino wrote: > > On 2014/04/24 07:09:39, mtomasz wrote: > > > Are you sure it is impossible to get a URL without a trailing slash? Some > time > > > ago it was quite common. Please check several scenarios. Eg. for a root > > > directory, subdirectory, etc. Thanks! > > > > According to the current implementation of toURL(), if it is not the root > > directory, URL is without trailing slash. > > But I think, if ancestorEntry is directory and anchestorEntry's URL is a > prefix > > of childEntry's URL, we can conclude that the childEntry is under > ancestorEntry. > > What do you think? > > This would work but only within the same file system. For different ones it > would fail on: > (1) > filesystem:chrome-extension://extension-id/external/drive/very_long_file_name.txt > (2) filesystem:chrome-extension://extension-id/external/d > > (1) is a file on drive > (2) is a root directory on a file system with 'd' mount point name. > > So, we can safely remove adding the slash but we have to check earlier if both > entries are on the same file system: > > if (!util.isSameEntry(ancestorEntry.filesystem.root, > childEntry.filesystem.root)) > return false; > > WDYT? > Thank you for explanation! I agree with it. As discussed, I'll use util.isSameFileSystem() and remove toURL() by using entry.filePath. https://codereview.chromium.org/253343003/diff/20001/ui/file_manager/file_man... File ui/file_manager/file_manager/common/js/util.js (right): https://codereview.chromium.org/253343003/diff/20001/ui/file_manager/file_man... ui/file_manager/file_manager/common/js/util.js:1121: typeof childEntry.fullPath !== 'string') Make sure that we can call indexOf() safely. It is because the argument may be fake entry or simplely Object.
https://codereview.chromium.org/253343003/diff/20001/ui/file_manager/file_man... File ui/file_manager/file_manager/common/js/util.js (right): https://codereview.chromium.org/253343003/diff/20001/ui/file_manager/file_man... ui/file_manager/file_manager/common/js/util.js:1121: typeof childEntry.fullPath !== 'string') On 2014/04/24 08:40:26, fukino wrote: > Make sure that we can call indexOf() safely. > It is because the argument may be fake entry or simplely Object. nit: Please use util.isFakeEntry(). The argument can be either an Entry or a fake entry. https://codereview.chromium.org/253343003/diff/20001/ui/file_manager/file_man... ui/file_manager/file_manager/common/js/util.js:1123: if (childEntry.fullPath.indexOf(ancestorEntry.fullPath) !== 0) Please confirm that fullPath is always finished with '/' for non-roots directories.
https://codereview.chromium.org/253343003/diff/20001/ui/file_manager/file_man... File ui/file_manager/file_manager/common/js/util.js (right): https://codereview.chromium.org/253343003/diff/20001/ui/file_manager/file_man... ui/file_manager/file_manager/common/js/util.js:1121: typeof childEntry.fullPath !== 'string') On 2014/04/24 11:59:59, mtomasz wrote: > On 2014/04/24 08:40:26, fukino wrote: > > Make sure that we can call indexOf() safely. > > It is because the argument may be fake entry or simplely Object. > > nit: Please use util.isFakeEntry(). The argument can be either an Entry or a > fake entry. Thanks! I'll use isFakeEntry(), but let me confirm just in case. FakeEntries never have ancestor-descendant relationship. Am I correct? https://codereview.chromium.org/253343003/diff/20001/ui/file_manager/file_man... ui/file_manager/file_manager/common/js/util.js:1123: if (childEntry.fullPath.indexOf(ancestorEntry.fullPath) !== 0) On 2014/04/24 11:59:59, mtomasz wrote: > Please confirm that fullPath is always finished with '/' for non-roots > directories. Maybe I'm lost... Let me clarify my current understanding. A is descendant of B if and only if: * A is a Entry, and * B is a DirectoryEntry, and * A and B is different Entry, and * Both A and B are within the same filesystem, and * B.fullPath is a prefix of A.fullPath. Now that we verified A and B is within the same filesystem, I think we don't have to check trailing slash. Is my understanding correct?
Sorry for late response! https://codereview.chromium.org/253343003/diff/20001/ui/file_manager/file_man... File ui/file_manager/file_manager/common/js/util.js (right): https://codereview.chromium.org/253343003/diff/20001/ui/file_manager/file_man... ui/file_manager/file_manager/common/js/util.js:1123: if (childEntry.fullPath.indexOf(ancestorEntry.fullPath) !== 0) On 2014/04/25 00:40:03, fukino wrote: > On 2014/04/24 11:59:59, mtomasz wrote: > > Please confirm that fullPath is always finished with '/' for non-roots > > directories. > > Maybe I'm lost... > Let me clarify my current understanding. > > A is descendant of B if and only if: > * A is a Entry, and > * B is a DirectoryEntry, and > * A and B is different Entry, and > * Both A and B are within the same filesystem, and > * B.fullPath is a prefix of A.fullPath. > > Now that we verified A and B is within the same filesystem, I think we don't > have to check trailing slash. > Is my understanding correct? If we had paths: ancestorEntry.fullPath: /hello/w childEntry.fullPath: /hello/world/file.txt Then this function would return true, but it is wrong. The 'childEntry' is not a child entry, but its path is a prefix. If we had a trailing slash for directories: ancestorEntry.fullPath: /hello/w/ childEntry.fullPath: /hello/world/file.txt Then this function would return false, which is correct.
https://codereview.chromium.org/253343003/diff/20001/ui/file_manager/file_man... File ui/file_manager/file_manager/common/js/util.js (right): https://codereview.chromium.org/253343003/diff/20001/ui/file_manager/file_man... ui/file_manager/file_manager/common/js/util.js:1123: if (childEntry.fullPath.indexOf(ancestorEntry.fullPath) !== 0) On 2014/04/28 00:41:54, mtomasz wrote: > On 2014/04/25 00:40:03, fukino wrote: > > On 2014/04/24 11:59:59, mtomasz wrote: > > > Please confirm that fullPath is always finished with '/' for non-roots > > > directories. > > > > Maybe I'm lost... > > Let me clarify my current understanding. > > > > A is descendant of B if and only if: > > * A is a Entry, and > > * B is a DirectoryEntry, and > > * A and B is different Entry, and > > * Both A and B are within the same filesystem, and > > * B.fullPath is a prefix of A.fullPath. > > > > Now that we verified A and B is within the same filesystem, I think we don't > > have to check trailing slash. > > Is my understanding correct? > > If we had paths: > ancestorEntry.fullPath: /hello/w > childEntry.fullPath: /hello/world/file.txt > > Then this function would return true, but it is wrong. The 'childEntry' is not a > child entry, but its path is a prefix. > > If we had a trailing slash for directories: > ancestorEntry.fullPath: /hello/w/ > childEntry.fullPath: /hello/world/file.txt > > Then this function would return false, which is correct. Oops, I overlooked the case... Thank you for gentle explanation! As a fix, I appended trailing slash for ancestor directory if it didn't finish by slash. After that, I checked if the ancestor's path with trailing slash is a prefix of child's path.
LGTM! https://codereview.chromium.org/253343003/diff/20001/ui/file_manager/file_man... File ui/file_manager/file_manager/common/js/util.js (right): https://codereview.chromium.org/253343003/diff/20001/ui/file_manager/file_man... ui/file_manager/file_manager/common/js/util.js:1121: typeof childEntry.fullPath !== 'string') On 2014/04/25 00:40:03, fukino wrote: > On 2014/04/24 11:59:59, mtomasz wrote: > > On 2014/04/24 08:40:26, fukino wrote: > > > Make sure that we can call indexOf() safely. > > > It is because the argument may be fake entry or simplely Object. > > > > nit: Please use util.isFakeEntry(). The argument can be either an Entry or a > > fake entry. > > Thanks! I'll use isFakeEntry(), but let me confirm just in case. > FakeEntries never have ancestor-descendant relationship. Am I correct? I think we can ignore fake entries.
> > Thanks! I'll use isFakeEntry(), but let me confirm just in case. > > FakeEntries never have ancestor-descendant relationship. Am I correct? > > I think we can ignore fake entries. I think so, too. Thank you for confirmation!
The CQ bit was checked by fukino@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/fukino@chromium.org/253343003/40001
Message was sent while issue was closed.
Change committed as 266506 |
