Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
658013002:
Changed api to notify when watched directory is deleted. (Closed)
|
Created:
6 years, 2 months ago by yawano Modified:
6 years, 1 month ago CC:
chromium-reviews, extensions-reviews_chromium.org, nkostylev+watch_chromium.org, yoshiki+watch_chromium.org, rginda+watch_chromium.org, oshima+watch_chromium.org, chromium-apps-reviews_chromium.org, stevenjb+watch_chromium.org, davemoore+watch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Project:
chromium Visibility:
Public. |
DescriptionChanged api to notify when watched directory is deleted.
BUG=423257
TEST=out/Release/browser_tests --gtest_filter=FileManagerPrivateApiTest.OnFileChanged
Committed: https://crrev.com/962dea43ddd90e7e4224a03fa3c36a421281abb7
Cr-Commit-Position: refs/heads/master@{#301325}
Patch Set 1 #
Total comments: 16
Patch Set 2 : Changed to fix OnFileChanged rather than HandleFileWatchNotification. #
Total comments: 32
Patch Set 3 : Fixed commented things. #Patch Set 4 : Moved initialization/deletion of EventRouter to Setup/Teardown methods. #Patch Set 5 : Added DCHECK to testing_profile_. #Patch Set 6 : Added periods to the end of comments. #
Total comments: 8
Patch Set 7 : Changed FileChange.Update. #Patch Set 8 : Fixed nit. #
Total comments: 18
Patch Set 9 : Removed DispatchDirectoryChangeEventImpl class. #Patch Set 10 : Changed variable names of iterators. #
Total comments: 4
Patch Set 11 : Changed to use scoped_ptr. #
Total comments: 2
Patch Set 12 : Removed CreateDirectoryDeletionFileChange. #
Total comments: 4
Patch Set 13 : Changed to use auto and range-loop. #Patch Set 14 : Changed FileChange of deletion of a directory. #
Total comments: 4
Patch Set 15 : Fixed the error case /aaa. #
Total comments: 2
Patch Set 16 : Formatted arguments. #
Messages
Total messages: 39 (5 generated)
yawano@chromium.org changed reviewers: + mtomasz@chromium.org
Changed the API to notify when a watched directory itself is deleted. PTAL. Thanks.
https://codereview.chromium.org/658013002/diff/1/chrome/browser/chromeos/exte... File chrome/browser/chromeos/extensions/file_manager/event_router.cc (right): https://codereview.chromium.org/658013002/diff/1/chrome/browser/chromeos/exte... chrome/browser/chromeos/extensions/file_manager/event_router.cc:390: dispatch_dir_change_eve_impl_( ditto https://codereview.chromium.org/658013002/diff/1/chrome/browser/chromeos/exte... chrome/browser/chromeos/extensions/file_manager/event_router.cc:781: // Since a change in the parent directory of a watching directory causes What if we watch for /a/b only, and /a/b is removed? We won't get notifications for parent? Will it work then? https://codereview.chromium.org/658013002/diff/1/chrome/browser/chromeos/exte... chrome/browser/chromeos/extensions/file_manager/event_router.cc:791: base::PathExists(iter->first)) { PathExists can't be called on UI thread. You have to do that on IO or FILE thread or on a blocking pool. https://codereview.chromium.org/658013002/diff/1/chrome/browser/chromeos/exte... chrome/browser/chromeos/extensions/file_manager/event_router.cc:807: dispatch_dir_change_eve_impl_.Run(virtual_path, ditto https://codereview.chromium.org/658013002/diff/1/chrome/browser/chromeos/exte... chrome/browser/chromeos/extensions/file_manager/event_router.cc:994: dispatch_dir_change_eve_impl_ = callback; ditto https://codereview.chromium.org/658013002/diff/1/chrome/browser/chromeos/exte... File chrome/browser/chromeos/extensions/file_manager/event_router.h (right): https://codereview.chromium.org/658013002/diff/1/chrome/browser/chromeos/exte... chrome/browser/chromeos/extensions/file_manager/event_router.h:66: DispatchDirChangeEveImplCallback; typo: Event https://codereview.chromium.org/658013002/diff/1/chrome/browser/chromeos/exte... chrome/browser/chromeos/extensions/file_manager/event_router.h:144: void SetDispatchDirChangeEveImplForTesting( Please avoid abbreviations per our style guide. Dir -> Directory https://codereview.chromium.org/658013002/diff/1/chrome/browser/chromeos/exte... File chrome/browser/chromeos/extensions/file_manager/file_manager_private_apitest.cc (right): https://codereview.chromium.org/658013002/diff/1/chrome/browser/chromeos/exte... chrome/browser/chromeos/extensions/file_manager/file_manager_private_apitest.cc:114: struct DispatchDirChangeEve { ditto
Thank you for the review. I changed to fix OnFileChange rather than HandleFileWatchNotification to solve this issue. The notification comes differently depending on the deleted directory is on Drive or Local. PTAL. 1. Drive The deletion of directory comes via OnFileChanged. e.g. When /a/b is deleted, it comes as a fileChange of /a/b with a changeList of a deletion of a directory. 2. Local The deletion of directory directly comes to HandleFileWatchNotification. e.g. When watched directory /a/b is deleted, it comes to HandleFileWatchNotification directly as a notification of /a/b. https://codereview.chromium.org/658013002/diff/1/chrome/browser/chromeos/exte... File chrome/browser/chromeos/extensions/file_manager/event_router.cc (right): https://codereview.chromium.org/658013002/diff/1/chrome/browser/chromeos/exte... chrome/browser/chromeos/extensions/file_manager/event_router.cc:390: dispatch_dir_change_eve_impl_( On 2014/10/16 04:02:02, mtomasz wrote: > ditto Done. https://codereview.chromium.org/658013002/diff/1/chrome/browser/chromeos/exte... chrome/browser/chromeos/extensions/file_manager/event_router.cc:781: // Since a change in the parent directory of a watching directory causes On 2014/10/16 04:02:02, mtomasz wrote: > What if we watch for /a/b only, and /a/b is removed? We won't get notifications > for parent? Will it work then? While we changed the point to fix in the latest Cl, this logic works. If /a/b is deleted and it's in Drive, it is notified as a change of /a, and it matches. If /a/b is deleted and it's on Local, it is notified as a change of /a/b, and it also matches. https://codereview.chromium.org/658013002/diff/1/chrome/browser/chromeos/exte... chrome/browser/chromeos/extensions/file_manager/event_router.cc:791: base::PathExists(iter->first)) { On 2014/10/16 04:02:02, mtomasz wrote: > PathExists can't be called on UI thread. You have to do that on IO or FILE > thread or on a blocking pool. Acknowledged. https://codereview.chromium.org/658013002/diff/1/chrome/browser/chromeos/exte... chrome/browser/chromeos/extensions/file_manager/event_router.cc:807: dispatch_dir_change_eve_impl_.Run(virtual_path, On 2014/10/16 04:02:02, mtomasz wrote: > ditto Done. https://codereview.chromium.org/658013002/diff/1/chrome/browser/chromeos/exte... chrome/browser/chromeos/extensions/file_manager/event_router.cc:994: dispatch_dir_change_eve_impl_ = callback; On 2014/10/16 04:02:02, mtomasz wrote: > ditto Done. https://codereview.chromium.org/658013002/diff/1/chrome/browser/chromeos/exte... File chrome/browser/chromeos/extensions/file_manager/event_router.h (right): https://codereview.chromium.org/658013002/diff/1/chrome/browser/chromeos/exte... chrome/browser/chromeos/extensions/file_manager/event_router.h:66: DispatchDirChangeEveImplCallback; On 2014/10/16 04:02:02, mtomasz wrote: > typo: Event Done. https://codereview.chromium.org/658013002/diff/1/chrome/browser/chromeos/exte... chrome/browser/chromeos/extensions/file_manager/event_router.h:144: void SetDispatchDirChangeEveImplForTesting( On 2014/10/16 04:02:02, mtomasz wrote: > Please avoid abbreviations per our style guide. > Dir -> Directory Done. https://codereview.chromium.org/658013002/diff/1/chrome/browser/chromeos/exte... File chrome/browser/chromeos/extensions/file_manager/file_manager_private_apitest.cc (right): https://codereview.chromium.org/658013002/diff/1/chrome/browser/chromeos/exte... chrome/browser/chromeos/extensions/file_manager/file_manager_private_apitest.cc:114: struct DispatchDirChangeEve { On 2014/10/16 04:02:03, mtomasz wrote: > ditto Done.
Please add a CL description. https://codereview.chromium.org/658013002/diff/20001/chrome/browser/chromeos/... File chrome/browser/chromeos/extensions/file_manager/event_router.cc (right): https://codereview.chromium.org/658013002/diff/20001/chrome/browser/chromeos/... chrome/browser/chromeos/extensions/file_manager/event_router.cc:725: bool containsDirectoryDeletion = false; nit: Please mark const what can be const. https://codereview.chromium.org/658013002/diff/20001/chrome/browser/chromeos/... chrome/browser/chromeos/extensions/file_manager/event_router.cc:739: // Since if the change is a deletion of a directory, it may have deleted a Since if -> Since (or) If https://codereview.chromium.org/658013002/diff/20001/chrome/browser/chromeos/... chrome/browser/chromeos/extensions/file_manager/event_router.cc:739: // Since if the change is a deletion of a directory, it may have deleted a Does this mean, that if we have directories /a and /a/b, both watched, and they are removed on Drive, and /a is removed, then we were not getting a notification for /a/b? https://codereview.chromium.org/658013002/diff/20001/chrome/browser/chromeos/... chrome/browser/chromeos/extensions/file_manager/event_router.cc:743: // Expaned the deleted directory path with watched paths. typo: Expand https://codereview.chromium.org/658013002/diff/20001/chrome/browser/chromeos/... File chrome/browser/chromeos/extensions/file_manager/event_router.h (right): https://codereview.chromium.org/658013002/diff/20001/chrome/browser/chromeos/... chrome/browser/chromeos/extensions/file_manager/event_router.h:168: void DispatchDirectoryChangeEventImpl( nit: Comment is missing. https://codereview.chromium.org/658013002/diff/20001/chrome/browser/chromeos/... chrome/browser/chromeos/extensions/file_manager/event_router.h:233: base::WeakPtrFactory<EventRouter> weak_factory_; Please keep weak_factory_ as the last member. See the comment for it. https://codereview.chromium.org/658013002/diff/20001/chrome/browser/chromeos/... chrome/browser/chromeos/extensions/file_manager/event_router.h:235: DispatchDirectoryChangeEventImplCallback (No need for a comment for trivial members). https://codereview.chromium.org/658013002/diff/20001/chrome/browser/chromeos/... File chrome/browser/chromeos/extensions/file_manager/file_manager_private_apitest.cc (right): https://codereview.chromium.org/658013002/diff/20001/chrome/browser/chromeos/... chrome/browser/chromeos/extensions/file_manager/file_manager_private_apitest.cc:115: struct DispatchDirectoryChangeEvent { nit: This can be a nested struct in DispatchDirectoryChangeEventImpl, since only used there. https://codereview.chromium.org/658013002/diff/20001/chrome/browser/chromeos/... chrome/browser/chromeos/extensions/file_manager/file_manager_private_apitest.cc:132: } Please use DISALLOW_COPY_AND_ASSIGN macro on classes which are not supposed to be copied. https://codereview.chromium.org/658013002/diff/20001/chrome/browser/chromeos/... chrome/browser/chromeos/extensions/file_manager/file_manager_private_apitest.cc:145: void AddFileWatchCallback(bool b) { } Please avoid abbreviations (except common ones): http://google-styleguide.googlecode.com/svn/trunk/cppguide.html#General_Namin... https://codereview.chromium.org/658013002/diff/20001/chrome/browser/chromeos/... chrome/browser/chromeos/extensions/file_manager/file_manager_private_apitest.cc:324: // In this test case, we assume these pathes does not actually exist in the typo: paths https://codereview.chromium.org/658013002/diff/20001/chrome/browser/chromeos/... chrome/browser/chromeos/extensions/file_manager/file_manager_private_apitest.cc:324: // In this test case, we assume these pathes does not actually exist in the nit: does -> do [for plural] https://codereview.chromium.org/658013002/diff/20001/chrome/browser/chromeos/... chrome/browser/chromeos/extensions/file_manager/file_manager_private_apitest.cc:332: event_router.AddFileWatch(base::FilePath("no-existing-fs/root/a/d/e"), Please use FILE_PATH_LITERAL. https://codereview.chromium.org/658013002/diff/20001/chrome/browser/chromeos/... chrome/browser/chromeos/extensions/file_manager/file_manager_private_apitest.cc:340: ASSERT_EQ(2lu, dispatch_directory_change_event_impl.events.size()); Please either validate event members, or replace DispatchDirectoryChangeEvent with a simple counter. https://codereview.chromium.org/658013002/diff/20001/chrome/browser/chromeos/... chrome/browser/chromeos/extensions/file_manager/file_manager_private_apitest.cc:345: ASSERT_EQ(3lu, dispatch_directory_change_event_impl.events.size()); Isn't 3u enough? https://codereview.chromium.org/658013002/diff/20001/chrome/browser/chromeos/... chrome/browser/chromeos/extensions/file_manager/file_manager_private_apitest.cc:352: event_router.Shutdown(); If a test case fails on an assert, then EventRouter will be destroyed without calling Shutdown. In such case we'll end up in a crash, right? If so, then we may have to fix it, by eg. moving Shutdown to Teardown, and initialize event router in SetUp(). WDYT?
On 2014/10/17 05:26:57, mtomasz wrote: > Please add a CL description. > > https://codereview.chromium.org/658013002/diff/20001/chrome/browser/chromeos/... > File chrome/browser/chromeos/extensions/file_manager/event_router.cc (right): > > https://codereview.chromium.org/658013002/diff/20001/chrome/browser/chromeos/... > chrome/browser/chromeos/extensions/file_manager/event_router.cc:725: bool > containsDirectoryDeletion = false; > nit: Please mark const what can be const. > > https://codereview.chromium.org/658013002/diff/20001/chrome/browser/chromeos/... > chrome/browser/chromeos/extensions/file_manager/event_router.cc:739: // Since if > the change is a deletion of a directory, it may have deleted a > Since if -> Since (or) If > > https://codereview.chromium.org/658013002/diff/20001/chrome/browser/chromeos/... > chrome/browser/chromeos/extensions/file_manager/event_router.cc:739: // Since if > the change is a deletion of a directory, it may have deleted a > Does this mean, that if we have directories /a and /a/b, both watched, and they > are removed on Drive, and /a is removed, then we were not getting a notification > for /a/b? > > https://codereview.chromium.org/658013002/diff/20001/chrome/browser/chromeos/... > chrome/browser/chromeos/extensions/file_manager/event_router.cc:743: // Expaned > the deleted directory path with watched paths. > typo: Expand > > https://codereview.chromium.org/658013002/diff/20001/chrome/browser/chromeos/... > File chrome/browser/chromeos/extensions/file_manager/event_router.h (right): > > https://codereview.chromium.org/658013002/diff/20001/chrome/browser/chromeos/... > chrome/browser/chromeos/extensions/file_manager/event_router.h:168: void > DispatchDirectoryChangeEventImpl( > nit: Comment is missing. > > https://codereview.chromium.org/658013002/diff/20001/chrome/browser/chromeos/... > chrome/browser/chromeos/extensions/file_manager/event_router.h:233: > base::WeakPtrFactory<EventRouter> weak_factory_; > Please keep weak_factory_ as the last member. See the comment for it. > > https://codereview.chromium.org/658013002/diff/20001/chrome/browser/chromeos/... > chrome/browser/chromeos/extensions/file_manager/event_router.h:235: > DispatchDirectoryChangeEventImplCallback > (No need for a comment for trivial members). > > https://codereview.chromium.org/658013002/diff/20001/chrome/browser/chromeos/... > File > chrome/browser/chromeos/extensions/file_manager/file_manager_private_apitest.cc > (right): > > https://codereview.chromium.org/658013002/diff/20001/chrome/browser/chromeos/... > chrome/browser/chromeos/extensions/file_manager/file_manager_private_apitest.cc:115: > struct DispatchDirectoryChangeEvent { > nit: This can be a nested struct in DispatchDirectoryChangeEventImpl, since only > used there. > > https://codereview.chromium.org/658013002/diff/20001/chrome/browser/chromeos/... > chrome/browser/chromeos/extensions/file_manager/file_manager_private_apitest.cc:132: > } > Please use DISALLOW_COPY_AND_ASSIGN macro on classes which are not supposed to > be copied. > > https://codereview.chromium.org/658013002/diff/20001/chrome/browser/chromeos/... > chrome/browser/chromeos/extensions/file_manager/file_manager_private_apitest.cc:145: > void AddFileWatchCallback(bool b) { } > Please avoid abbreviations (except common ones): > http://google-styleguide.googlecode.com/svn/trunk/cppguide.html#General_Namin... > > https://codereview.chromium.org/658013002/diff/20001/chrome/browser/chromeos/... > chrome/browser/chromeos/extensions/file_manager/file_manager_private_apitest.cc:324: > // In this test case, we assume these pathes does not actually exist in the > typo: paths > > https://codereview.chromium.org/658013002/diff/20001/chrome/browser/chromeos/... > chrome/browser/chromeos/extensions/file_manager/file_manager_private_apitest.cc:324: > // In this test case, we assume these pathes does not actually exist in the > nit: does -> do [for plural] > > https://codereview.chromium.org/658013002/diff/20001/chrome/browser/chromeos/... > chrome/browser/chromeos/extensions/file_manager/file_manager_private_apitest.cc:332: > event_router.AddFileWatch(base::FilePath("no-existing-fs/root/a/d/e"), > Please use FILE_PATH_LITERAL. > > https://codereview.chromium.org/658013002/diff/20001/chrome/browser/chromeos/... > chrome/browser/chromeos/extensions/file_manager/file_manager_private_apitest.cc:340: > ASSERT_EQ(2lu, dispatch_directory_change_event_impl.events.size()); > Please either validate event members, or replace DispatchDirectoryChangeEvent > with a simple counter. > > https://codereview.chromium.org/658013002/diff/20001/chrome/browser/chromeos/... > chrome/browser/chromeos/extensions/file_manager/file_manager_private_apitest.cc:345: > ASSERT_EQ(3lu, dispatch_directory_change_event_impl.events.size()); > Isn't 3u enough? > > https://codereview.chromium.org/658013002/diff/20001/chrome/browser/chromeos/... > chrome/browser/chromeos/extensions/file_manager/file_manager_private_apitest.cc:352: > event_router.Shutdown(); > If a test case fails on an assert, then EventRouter will be destroyed without > calling Shutdown. In such case we'll end up in a crash, right? If so, then we > may have to fix it, by eg. moving Shutdown to Teardown, and initialize event > router in SetUp(). WDYT? BTW our code for file watchers is pretty bad, so fixing this bug may be tough. Take your time.
Moved initialization/shutdown of EventRouter to Setup/TearDown methods. PTAL. Thanks. https://codereview.chromium.org/658013002/diff/20001/chrome/browser/chromeos/... File chrome/browser/chromeos/extensions/file_manager/event_router.cc (right): https://codereview.chromium.org/658013002/diff/20001/chrome/browser/chromeos/... chrome/browser/chromeos/extensions/file_manager/event_router.cc:725: bool containsDirectoryDeletion = false; On 2014/10/17 05:26:56, mtomasz wrote: > nit: Please mark const what can be const. Done. https://codereview.chromium.org/658013002/diff/20001/chrome/browser/chromeos/... chrome/browser/chromeos/extensions/file_manager/event_router.cc:739: // Since if the change is a deletion of a directory, it may have deleted a On 2014/10/17 05:26:56, mtomasz wrote: > Since if -> Since (or) If Done. https://codereview.chromium.org/658013002/diff/20001/chrome/browser/chromeos/... chrome/browser/chromeos/extensions/file_manager/event_router.cc:739: // Since if the change is a deletion of a directory, it may have deleted a On 2014/10/17 05:26:56, mtomasz wrote: > Does this mean, that if we have directories /a and /a/b, both watched, and they > are removed on Drive, and /a is removed, then we were not getting a notification > for /a/b? Yes. When we are watching /a and /a/b, and /a is deleted, the drive only notifies /a is deleted. They don't notify that /a/b is also deleted. On the other hand, if it's on Local, in the same situation we get two notifications for /a and /a/b. https://codereview.chromium.org/658013002/diff/20001/chrome/browser/chromeos/... chrome/browser/chromeos/extensions/file_manager/event_router.cc:743: // Expaned the deleted directory path with watched paths. On 2014/10/17 05:26:56, mtomasz wrote: > typo: Expand Done. https://codereview.chromium.org/658013002/diff/20001/chrome/browser/chromeos/... File chrome/browser/chromeos/extensions/file_manager/event_router.h (right): https://codereview.chromium.org/658013002/diff/20001/chrome/browser/chromeos/... chrome/browser/chromeos/extensions/file_manager/event_router.h:168: void DispatchDirectoryChangeEventImpl( On 2014/10/17 05:26:56, mtomasz wrote: > nit: Comment is missing. Done. https://codereview.chromium.org/658013002/diff/20001/chrome/browser/chromeos/... chrome/browser/chromeos/extensions/file_manager/event_router.h:233: base::WeakPtrFactory<EventRouter> weak_factory_; On 2014/10/17 05:26:56, mtomasz wrote: > Please keep weak_factory_ as the last member. See the comment for it. Done. https://codereview.chromium.org/658013002/diff/20001/chrome/browser/chromeos/... chrome/browser/chromeos/extensions/file_manager/event_router.h:235: DispatchDirectoryChangeEventImplCallback On 2014/10/17 05:26:56, mtomasz wrote: > (No need for a comment for trivial members). Acknowledged. https://codereview.chromium.org/658013002/diff/20001/chrome/browser/chromeos/... File chrome/browser/chromeos/extensions/file_manager/file_manager_private_apitest.cc (right): https://codereview.chromium.org/658013002/diff/20001/chrome/browser/chromeos/... chrome/browser/chromeos/extensions/file_manager/file_manager_private_apitest.cc:115: struct DispatchDirectoryChangeEvent { On 2014/10/17 05:26:57, mtomasz wrote: > nit: This can be a nested struct in DispatchDirectoryChangeEventImpl, since only > used there. Acknowledged. https://codereview.chromium.org/658013002/diff/20001/chrome/browser/chromeos/... chrome/browser/chromeos/extensions/file_manager/file_manager_private_apitest.cc:132: } On 2014/10/17 05:26:57, mtomasz wrote: > Please use DISALLOW_COPY_AND_ASSIGN macro on classes which are not supposed to > be copied. Done. https://codereview.chromium.org/658013002/diff/20001/chrome/browser/chromeos/... chrome/browser/chromeos/extensions/file_manager/file_manager_private_apitest.cc:145: void AddFileWatchCallback(bool b) { } On 2014/10/17 05:26:57, mtomasz wrote: > Please avoid abbreviations (except common ones): > http://google-styleguide.googlecode.com/svn/trunk/cppguide.html#General_Namin... Done. https://codereview.chromium.org/658013002/diff/20001/chrome/browser/chromeos/... chrome/browser/chromeos/extensions/file_manager/file_manager_private_apitest.cc:324: // In this test case, we assume these pathes does not actually exist in the On 2014/10/17 05:26:57, mtomasz wrote: > nit: does -> do [for plural] Acknowledged. https://codereview.chromium.org/658013002/diff/20001/chrome/browser/chromeos/... chrome/browser/chromeos/extensions/file_manager/file_manager_private_apitest.cc:324: // In this test case, we assume these pathes does not actually exist in the On 2014/10/17 05:26:57, mtomasz wrote: > typo: paths Sorry, these comments are for old patches. I deleted this. https://codereview.chromium.org/658013002/diff/20001/chrome/browser/chromeos/... chrome/browser/chromeos/extensions/file_manager/file_manager_private_apitest.cc:332: event_router.AddFileWatch(base::FilePath("no-existing-fs/root/a/d/e"), On 2014/10/17 05:26:57, mtomasz wrote: > Please use FILE_PATH_LITERAL. Done. https://codereview.chromium.org/658013002/diff/20001/chrome/browser/chromeos/... chrome/browser/chromeos/extensions/file_manager/file_manager_private_apitest.cc:340: ASSERT_EQ(2lu, dispatch_directory_change_event_impl.events.size()); On 2014/10/17 05:26:57, mtomasz wrote: > Please either validate event members, or replace DispatchDirectoryChangeEvent > with a simple counter. Done. https://codereview.chromium.org/658013002/diff/20001/chrome/browser/chromeos/... chrome/browser/chromeos/extensions/file_manager/file_manager_private_apitest.cc:345: ASSERT_EQ(3lu, dispatch_directory_change_event_impl.events.size()); On 2014/10/17 05:26:57, mtomasz wrote: > Isn't 3u enough? Done. https://codereview.chromium.org/658013002/diff/20001/chrome/browser/chromeos/... chrome/browser/chromeos/extensions/file_manager/file_manager_private_apitest.cc:352: event_router.Shutdown(); On 2014/10/17 05:26:57, mtomasz wrote: > If a test case fails on an assert, then EventRouter will be destroyed without > calling Shutdown. In such case we'll end up in a crash, right? If so, then we > may have to fix it, by eg. moving Shutdown to Teardown, and initialize event > router in SetUp(). WDYT? Done. Yes. I also think it makes safer to move initialization/shutdown of EventRouter to Setup/Teardown.
Could you confirm how watchers behave if a file is deleted remotely on Drive? Eg. via drive.google.com? What sequence of notifications will we get? Is it the same as if we deleted it from Files app? https://codereview.chromium.org/658013002/diff/100001/chrome/browser/chromeos... File chrome/browser/chromeos/extensions/file_manager/file_manager_private_apitest.cc (right): https://codereview.chromium.org/658013002/diff/100001/chrome/browser/chromeos... chrome/browser/chromeos/extensions/file_manager/file_manager_private_apitest.cc:132: int counter_ = 0; In Chromium we use initializer lists to initialize members. https://codereview.chromium.org/658013002/diff/100001/chrome/browser/chromeos... chrome/browser/chromeos/extensions/file_manager/file_manager_private_apitest.cc:177: delete event_router_; Please use scoped_ptr instead of manually maintaining pointers. https://codereview.chromium.org/658013002/diff/100001/chrome/browser/chromeos... chrome/browser/chromeos/extensions/file_manager/file_manager_private_apitest.cc:340: DispatchDirectoryChangeEventImpl* dispatch_directory_change_event_impl = nit: This can be: "DispatchDirectoryChangeEventImpl* const", as we don't modify the pointer value. https://codereview.chromium.org/658013002/diff/100001/chrome/browser/chromeos... chrome/browser/chromeos/extensions/file_manager/file_manager_private_apitest.cc:361: event_router_->OnFileChanged(*CreateDirectoryDeletionFileChange( nit: It can be EXPECT_EQ.
Thank you for the comment. I checked the behavior, and file changes were different from the deletion on local. Deletions on Drive comes with unknown filetype. I quickly checked the code, and by looking at the unittest of remote_to_local_syncer, this behavior seems to be intended. I'm going to change this patch to handle these unknown file deletions. WDYT? Unittest of remote_to_local_syncer: https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/syn... 1. Watching /a/b on Files.app, and delete /a from Drive, we get following changes. FILE CHANGES ========================== { DELETE:UNKNOWN, } PATH: drive/root/New Folder/a ======================================= FILE CHANGES ========================== { DELETE:UNKNOWN, } PATH: drive/root/New Folder/a/b ======================================= 2. Watching /a/b on Files.app, and delete /a from Files.app, we get following changes. FILE CHANGES ========================== { DELETE:DIRECTORY, } PATH: drive/root/New Folder/a ======================================= 3. We are watching /a on Files.app, and delete /a/Foobar.gdoc from Drive, we get following changes. FILE CHANGES ========================== { DELETE:UNKNOWN, } PATH: drive/root/New Folder/a/Foobar.gdoc ======================================= 4. We are watching /a on Files.app, and delete /a/Foobar.gdoc from Files.app, we get following changes. FILE CHANGES ========================== { DELETE:FILE, } PATH: drive/root/New Folder/a/Foobar.gdoc ======================================= On 2014/10/20 01:02:51, mtomasz wrote: > Could you confirm how watchers behave if a file is deleted remotely on Drive? > Eg. via drive.google.com? What sequence of notifications will we get? Is it the > same as if we deleted it from Files app? > > https://codereview.chromium.org/658013002/diff/100001/chrome/browser/chromeos... > File > chrome/browser/chromeos/extensions/file_manager/file_manager_private_apitest.cc > (right): > > https://codereview.chromium.org/658013002/diff/100001/chrome/browser/chromeos... > chrome/browser/chromeos/extensions/file_manager/file_manager_private_apitest.cc:132: > int counter_ = 0; > In Chromium we use initializer lists to initialize members. > > https://codereview.chromium.org/658013002/diff/100001/chrome/browser/chromeos... > chrome/browser/chromeos/extensions/file_manager/file_manager_private_apitest.cc:177: > delete event_router_; > Please use scoped_ptr instead of manually maintaining pointers. > > https://codereview.chromium.org/658013002/diff/100001/chrome/browser/chromeos... > chrome/browser/chromeos/extensions/file_manager/file_manager_private_apitest.cc:340: > DispatchDirectoryChangeEventImpl* dispatch_directory_change_event_impl = > nit: This can be: "DispatchDirectoryChangeEventImpl* const", as we don't modify > the pointer value. > > https://codereview.chromium.org/658013002/diff/100001/chrome/browser/chromeos... > chrome/browser/chromeos/extensions/file_manager/file_manager_private_apitest.cc:361: > event_router_->OnFileChanged(*CreateDirectoryDeletionFileChange( > nit: It can be EXPECT_EQ.
On 2014/10/20 02:06:58, yawano wrote: > Thank you for the comment. I checked the behavior, and file changes were > different from the deletion on local. Deletions on Drive comes with unknown > filetype. I quickly checked the code, and by looking at the unittest of > remote_to_local_syncer, this behavior seems to be intended. > > I'm going to change this patch to handle these unknown file deletions. WDYT? > > Unittest of remote_to_local_syncer: > https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/syn... > > 1. Watching /a/b on Files.app, and delete /a from Drive, we get following > changes. > > FILE CHANGES ========================== > { DELETE:UNKNOWN, } > PATH: drive/root/New Folder/a > ======================================= > FILE CHANGES ========================== > { DELETE:UNKNOWN, } > PATH: drive/root/New Folder/a/b > ======================================= > > 2. Watching /a/b on Files.app, and delete /a from Files.app, we get following > changes. > > FILE CHANGES ========================== > { DELETE:DIRECTORY, } > PATH: drive/root/New Folder/a > ======================================= > > 3. We are watching /a on Files.app, and delete /a/Foobar.gdoc from Drive, we get > following changes. > > FILE CHANGES ========================== > { DELETE:UNKNOWN, } > PATH: drive/root/New Folder/a/Foobar.gdoc > ======================================= > > 4. We are watching /a on Files.app, and delete /a/Foobar.gdoc from Files.app, we > get following changes. > > FILE CHANGES ========================== > { DELETE:FILE, } > PATH: drive/root/New Folder/a/Foobar.gdoc > ======================================= > > > > On 2014/10/20 01:02:51, mtomasz wrote: > > Could you confirm how watchers behave if a file is deleted remotely on Drive? > > Eg. via drive.google.com? What sequence of notifications will we get? Is it > the > > same as if we deleted it from Files app? > > > > > https://codereview.chromium.org/658013002/diff/100001/chrome/browser/chromeos... > > File > > > chrome/browser/chromeos/extensions/file_manager/file_manager_private_apitest.cc > > (right): > > > > > https://codereview.chromium.org/658013002/diff/100001/chrome/browser/chromeos... > > > chrome/browser/chromeos/extensions/file_manager/file_manager_private_apitest.cc:132: > > int counter_ = 0; > > In Chromium we use initializer lists to initialize members. > > > > > https://codereview.chromium.org/658013002/diff/100001/chrome/browser/chromeos... > > > chrome/browser/chromeos/extensions/file_manager/file_manager_private_apitest.cc:177: > > delete event_router_; > > Please use scoped_ptr instead of manually maintaining pointers. > > > > > https://codereview.chromium.org/658013002/diff/100001/chrome/browser/chromeos... > > > chrome/browser/chromeos/extensions/file_manager/file_manager_private_apitest.cc:340: > > DispatchDirectoryChangeEventImpl* dispatch_directory_change_event_impl = > > nit: This can be: "DispatchDirectoryChangeEventImpl* const", as we don't > modify > > the pointer value. > > > > > https://codereview.chromium.org/658013002/diff/100001/chrome/browser/chromeos... > > > chrome/browser/chromeos/extensions/file_manager/file_manager_private_apitest.cc:361: > > event_router_->OnFileChanged(*CreateDirectoryDeletionFileChange( > > nit: It can be EXPECT_EQ. Thanks for the investigation. Could you please also add same one for Downloads? Maybe we should just fix Drive logic if it's different than all the rest.
On 2014/10/20 03:46:45, mtomasz wrote: > On 2014/10/20 02:06:58, yawano wrote: > > Thank you for the comment. I checked the behavior, and file changes were > > different from the deletion on local. Deletions on Drive comes with unknown > > filetype. I quickly checked the code, and by looking at the unittest of > > remote_to_local_syncer, this behavior seems to be intended. > > > > I'm going to change this patch to handle these unknown file deletions. WDYT? > > > > Unittest of remote_to_local_syncer: > > > https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/syn... > > > > 1. Watching /a/b on Files.app, and delete /a from Drive, we get following > > changes. > > > > FILE CHANGES ========================== > > { DELETE:UNKNOWN, } > > PATH: drive/root/New Folder/a > > ======================================= > > FILE CHANGES ========================== > > { DELETE:UNKNOWN, } > > PATH: drive/root/New Folder/a/b > > ======================================= > > > > 2. Watching /a/b on Files.app, and delete /a from Files.app, we get following > > changes. > > > > FILE CHANGES ========================== > > { DELETE:DIRECTORY, } > > PATH: drive/root/New Folder/a > > ======================================= > > > > 3. We are watching /a on Files.app, and delete /a/Foobar.gdoc from Drive, we > get > > following changes. > > > > FILE CHANGES ========================== > > { DELETE:UNKNOWN, } > > PATH: drive/root/New Folder/a/Foobar.gdoc > > ======================================= > > > > 4. We are watching /a on Files.app, and delete /a/Foobar.gdoc from Files.app, > we > > get following changes. > > > > FILE CHANGES ========================== > > { DELETE:FILE, } > > PATH: drive/root/New Folder/a/Foobar.gdoc > > ======================================= > > > > > > > > On 2014/10/20 01:02:51, mtomasz wrote: > > > Could you confirm how watchers behave if a file is deleted remotely on > Drive? > > > Eg. via drive.google.com? What sequence of notifications will we get? Is it > > the > > > same as if we deleted it from Files app? > > > > > > > > > https://codereview.chromium.org/658013002/diff/100001/chrome/browser/chromeos... > > > File > > > > > > chrome/browser/chromeos/extensions/file_manager/file_manager_private_apitest.cc > > > (right): > > > > > > > > > https://codereview.chromium.org/658013002/diff/100001/chrome/browser/chromeos... > > > > > > chrome/browser/chromeos/extensions/file_manager/file_manager_private_apitest.cc:132: > > > int counter_ = 0; > > > In Chromium we use initializer lists to initialize members. > > > > > > > > > https://codereview.chromium.org/658013002/diff/100001/chrome/browser/chromeos... > > > > > > chrome/browser/chromeos/extensions/file_manager/file_manager_private_apitest.cc:177: > > > delete event_router_; > > > Please use scoped_ptr instead of manually maintaining pointers. > > > > > > > > > https://codereview.chromium.org/658013002/diff/100001/chrome/browser/chromeos... > > > > > > chrome/browser/chromeos/extensions/file_manager/file_manager_private_apitest.cc:340: > > > DispatchDirectoryChangeEventImpl* dispatch_directory_change_event_impl = > > > nit: This can be: "DispatchDirectoryChangeEventImpl* const", as we don't > > modify > > > the pointer value. > > > > > > > > > https://codereview.chromium.org/658013002/diff/100001/chrome/browser/chromeos... > > > > > > chrome/browser/chromeos/extensions/file_manager/file_manager_private_apitest.cc:361: > > > event_router_->OnFileChanged(*CreateDirectoryDeletionFileChange( > > > nit: It can be EXPECT_EQ. > > Thanks for the investigation. Could you please also add same one for Downloads? > Maybe we should just fix Drive logic if it's different than all the rest. On Downloads, onFileChanged is not called. HandleFileWatchNotification is called directly. 5. When we are watching /a/b, and /a is deleted, the following are values of local_path. (the second argument of EventRouter::HandleFileWatchNotification) /usr/local/[....]/Downloads/a/b /usr/local/[....]/Downloads /usr/local/[....]/Downloads/a/b 6. When we are watching /a, and sample.png is deleted, the following are values of local_path. /usr/local/[....]/Downloads/a P.S. The code I pointed in the previous message was a wrong point. Since it uses file_change of src/chrome/browser/sync_file_system/file_change.h, and not src/chrome/browser/chromeos/drive/file_change.h which we are using in EventRouter. I'm still investigating why file_type of a deletion made on drive is unknown.
On 2014/10/20 04:02:57, yawano wrote: > On 2014/10/20 03:46:45, mtomasz wrote: > > On 2014/10/20 02:06:58, yawano wrote: > > > Thank you for the comment. I checked the behavior, and file changes were > > > different from the deletion on local. Deletions on Drive comes with unknown > > > filetype. I quickly checked the code, and by looking at the unittest of > > > remote_to_local_syncer, this behavior seems to be intended. > > > > > > I'm going to change this patch to handle these unknown file deletions. WDYT? > > > > > > Unittest of remote_to_local_syncer: > > > > > > https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/syn... > > > > > > 1. Watching /a/b on Files.app, and delete /a from Drive, we get following > > > changes. > > > > > > FILE CHANGES ========================== > > > { DELETE:UNKNOWN, } > > > PATH: drive/root/New Folder/a > > > ======================================= > > > FILE CHANGES ========================== > > > { DELETE:UNKNOWN, } > > > PATH: drive/root/New Folder/a/b > > > ======================================= > > > > > > 2. Watching /a/b on Files.app, and delete /a from Files.app, we get > following > > > changes. > > > > > > FILE CHANGES ========================== > > > { DELETE:DIRECTORY, } > > > PATH: drive/root/New Folder/a > > > ======================================= > > > > > > 3. We are watching /a on Files.app, and delete /a/Foobar.gdoc from Drive, we > > get > > > following changes. > > > > > > FILE CHANGES ========================== > > > { DELETE:UNKNOWN, } > > > PATH: drive/root/New Folder/a/Foobar.gdoc > > > ======================================= > > > > > > 4. We are watching /a on Files.app, and delete /a/Foobar.gdoc from > Files.app, > > we > > > get following changes. > > > > > > FILE CHANGES ========================== > > > { DELETE:FILE, } > > > PATH: drive/root/New Folder/a/Foobar.gdoc > > > ======================================= > > > > > > > > > > > > On 2014/10/20 01:02:51, mtomasz wrote: > > > > Could you confirm how watchers behave if a file is deleted remotely on > > Drive? > > > > Eg. via drive.google.com? What sequence of notifications will we get? Is > it > > > the > > > > same as if we deleted it from Files app? > > > > > > > > > > > > > > https://codereview.chromium.org/658013002/diff/100001/chrome/browser/chromeos... > > > > File > > > > > > > > > > chrome/browser/chromeos/extensions/file_manager/file_manager_private_apitest.cc > > > > (right): > > > > > > > > > > > > > > https://codereview.chromium.org/658013002/diff/100001/chrome/browser/chromeos... > > > > > > > > > > chrome/browser/chromeos/extensions/file_manager/file_manager_private_apitest.cc:132: > > > > int counter_ = 0; > > > > In Chromium we use initializer lists to initialize members. > > > > > > > > > > > > > > https://codereview.chromium.org/658013002/diff/100001/chrome/browser/chromeos... > > > > > > > > > > chrome/browser/chromeos/extensions/file_manager/file_manager_private_apitest.cc:177: > > > > delete event_router_; > > > > Please use scoped_ptr instead of manually maintaining pointers. > > > > > > > > > > > > > > https://codereview.chromium.org/658013002/diff/100001/chrome/browser/chromeos... > > > > > > > > > > chrome/browser/chromeos/extensions/file_manager/file_manager_private_apitest.cc:340: > > > > DispatchDirectoryChangeEventImpl* dispatch_directory_change_event_impl = > > > > nit: This can be: "DispatchDirectoryChangeEventImpl* const", as we don't > > > modify > > > > the pointer value. > > > > > > > > > > > > > > https://codereview.chromium.org/658013002/diff/100001/chrome/browser/chromeos... > > > > > > > > > > chrome/browser/chromeos/extensions/file_manager/file_manager_private_apitest.cc:361: > > > > event_router_->OnFileChanged(*CreateDirectoryDeletionFileChange( > > > > nit: It can be EXPECT_EQ. > > > > Thanks for the investigation. Could you please also add same one for > Downloads? > > Maybe we should just fix Drive logic if it's different than all the rest. > > On Downloads, onFileChanged is not called. HandleFileWatchNotification is called > directly. > > 5. When we are watching /a/b, and /a is deleted, the following are values of > local_path. (the second argument of EventRouter::HandleFileWatchNotification) > > /usr/local/[....]/Downloads/a/b > /usr/local/[....]/Downloads > /usr/local/[....]/Downloads/a/b > > 6. When we are watching /a, and sample.png is deleted, the following are values > of local_path. > > /usr/local/[....]/Downloads/a > > P.S. The code I pointed in the previous message was a wrong point. Since it uses > file_change of src/chrome/browser/sync_file_system/file_change.h, and not > src/chrome/browser/chromeos/drive/file_change.h which we are using in > EventRouter. I'm still investigating why file_type of a deletion made on drive > is unknown. I think the (2) case is broken. Maybe we could easily fix it instead of introducing a workaround. What do you think?
Added a patch to fix the case (1) to raise "directory" deleted events. PTAL. Thanks. https://codereview.chromium.org/658013002/diff/100001/chrome/browser/chromeos... File chrome/browser/chromeos/extensions/file_manager/file_manager_private_apitest.cc (right): https://codereview.chromium.org/658013002/diff/100001/chrome/browser/chromeos... chrome/browser/chromeos/extensions/file_manager/file_manager_private_apitest.cc:132: int counter_ = 0; On 2014/10/20 01:02:51, mtomasz wrote: > In Chromium we use initializer lists to initialize members. Done. https://codereview.chromium.org/658013002/diff/100001/chrome/browser/chromeos... chrome/browser/chromeos/extensions/file_manager/file_manager_private_apitest.cc:177: delete event_router_; On 2014/10/20 01:02:51, mtomasz wrote: > Please use scoped_ptr instead of manually maintaining pointers. Done. https://codereview.chromium.org/658013002/diff/100001/chrome/browser/chromeos... chrome/browser/chromeos/extensions/file_manager/file_manager_private_apitest.cc:340: DispatchDirectoryChangeEventImpl* dispatch_directory_change_event_impl = On 2014/10/20 01:02:51, mtomasz wrote: > nit: This can be: "DispatchDirectoryChangeEventImpl* const", as we don't modify > the pointer value. Done. https://codereview.chromium.org/658013002/diff/100001/chrome/browser/chromeos... chrome/browser/chromeos/extensions/file_manager/file_manager_private_apitest.cc:361: event_router_->OnFileChanged(*CreateDirectoryDeletionFileChange( On 2014/10/20 01:02:51, mtomasz wrote: > nit: It can be EXPECT_EQ. Done.
https://codereview.chromium.org/658013002/diff/140001/chrome/browser/chromeos... File chrome/browser/chromeos/extensions/file_manager/event_router.cc (right): https://codereview.chromium.org/658013002/diff/140001/chrome/browser/chromeos... chrome/browser/chromeos/extensions/file_manager/event_router.cc:717: // For deletion of a directory, onFileChanged gets different change_files. typo: changed_files https://codereview.chromium.org/658013002/diff/140001/chrome/browser/chromeos... chrome/browser/chromeos/extensions/file_manager/event_router.cc:720: // /a/b is watched, and delete /a from Drive(e.g. from Web). nit: Drive( -> Drive ( https://codereview.chromium.org/658013002/diff/140001/chrome/browser/chromeos... chrome/browser/chromeos/extensions/file_manager/event_router.cc:720: // /a/b is watched, and delete /a from Drive(e.g. from Web). nit: and delete /a -> and /a is deleted https://codereview.chromium.org/658013002/diff/140001/chrome/browser/chromeos... chrome/browser/chromeos/extensions/file_manager/event_router.cc:734: bool containsDirectoryDeletion = false; nit: please_use_underscores_in_variable_names https://codereview.chromium.org/658013002/diff/140001/chrome/browser/chromeos... chrome/browser/chromeos/extensions/file_manager/event_router.cc:753: for (WatcherMap::const_iterator i = file_watchers_.lower_bound(path); nit: We have it, iter and i. Hard to say which one is which one. How about a better naming? https://codereview.chromium.org/658013002/diff/140001/chrome/browser/chromeos... File chrome/browser/chromeos/extensions/file_manager/file_manager_private_apitest.cc (right): https://codereview.chromium.org/658013002/diff/140001/chrome/browser/chromeos... chrome/browser/chromeos/extensions/file_manager/file_manager_private_apitest.cc:120: void Callback(const base::FilePath& virtual_path, Note, that actually we don't need this entire class. We can simply do: void DispatchDirectoryChangeEventImpl( int* counter, const base::FilePath& virtual_path, const drive::FileChange* list, bool got_error, const std::vector<std::string>& extension_ids) { ++(*counter); } (...) int counter = 0; event_router_->SetDispatchDirectoryChangeEventImplForTesting( base::Bind(&DispatchDirectoryChangeEventImpl, &counter); https://codereview.chromium.org/658013002/diff/140001/chrome/browser/chromeos... chrome/browser/chromeos/extensions/file_manager/file_manager_private_apitest.cc:124: counter_++; nit: ++counter_ for consistency. https://codereview.chromium.org/658013002/diff/140001/chrome/browser/chromeos... chrome/browser/chromeos/extensions/file_manager/file_manager_private_apitest.cc:127: int Count() { We can write it as a getter: int counter() const { return counter_; } https://codereview.chromium.org/658013002/diff/140001/chrome/browser/chromeos... chrome/browser/chromeos/extensions/file_manager/file_manager_private_apitest.cc:147: void AddFileWatchCallback(bool success) { } nit: Please use "git cl format".
Thank you for the review. I removed the unnecessary callback class, and renamed variables of iterators. PTAL. Thanks. https://codereview.chromium.org/658013002/diff/140001/chrome/browser/chromeos... File chrome/browser/chromeos/extensions/file_manager/event_router.cc (right): https://codereview.chromium.org/658013002/diff/140001/chrome/browser/chromeos... chrome/browser/chromeos/extensions/file_manager/event_router.cc:717: // For deletion of a directory, onFileChanged gets different change_files. On 2014/10/20 07:03:13, mtomasz wrote: > typo: changed_files Done. https://codereview.chromium.org/658013002/diff/140001/chrome/browser/chromeos... chrome/browser/chromeos/extensions/file_manager/event_router.cc:720: // /a/b is watched, and delete /a from Drive(e.g. from Web). On 2014/10/20 07:03:13, mtomasz wrote: > nit: Drive( -> Drive ( Done. https://codereview.chromium.org/658013002/diff/140001/chrome/browser/chromeos... chrome/browser/chromeos/extensions/file_manager/event_router.cc:720: // /a/b is watched, and delete /a from Drive(e.g. from Web). On 2014/10/20 07:03:13, mtomasz wrote: > nit: and delete /a -> and /a is deleted Done. https://codereview.chromium.org/658013002/diff/140001/chrome/browser/chromeos... chrome/browser/chromeos/extensions/file_manager/event_router.cc:734: bool containsDirectoryDeletion = false; On 2014/10/20 07:03:13, mtomasz wrote: > nit: please_use_underscores_in_variable_names Done. https://codereview.chromium.org/658013002/diff/140001/chrome/browser/chromeos... chrome/browser/chromeos/extensions/file_manager/event_router.cc:753: for (WatcherMap::const_iterator i = file_watchers_.lower_bound(path); On 2014/10/20 07:03:13, mtomasz wrote: > nit: We have it, iter and i. Hard to say which one is which one. How about a > better naming? Done. https://codereview.chromium.org/658013002/diff/140001/chrome/browser/chromeos... File chrome/browser/chromeos/extensions/file_manager/file_manager_private_apitest.cc (right): https://codereview.chromium.org/658013002/diff/140001/chrome/browser/chromeos... chrome/browser/chromeos/extensions/file_manager/file_manager_private_apitest.cc:120: void Callback(const base::FilePath& virtual_path, On 2014/10/20 07:03:13, mtomasz wrote: > Note, that actually we don't need this entire class. We can simply do: > > void DispatchDirectoryChangeEventImpl( > int* counter, > const base::FilePath& virtual_path, > const drive::FileChange* list, > bool got_error, > const std::vector<std::string>& extension_ids) { > ++(*counter); > } > > (...) > > int counter = 0; > event_router_->SetDispatchDirectoryChangeEventImplForTesting( > base::Bind(&DispatchDirectoryChangeEventImpl, &counter); Done. Thank you for this suggestion. I haven't come up with this idea. This is much simpler than the current implementation. https://codereview.chromium.org/658013002/diff/140001/chrome/browser/chromeos... chrome/browser/chromeos/extensions/file_manager/file_manager_private_apitest.cc:124: counter_++; On 2014/10/20 07:03:13, mtomasz wrote: > nit: ++counter_ for consistency. Done. https://codereview.chromium.org/658013002/diff/140001/chrome/browser/chromeos... chrome/browser/chromeos/extensions/file_manager/file_manager_private_apitest.cc:127: int Count() { On 2014/10/20 07:03:13, mtomasz wrote: > We can write it as a getter: > > int counter() const { > return counter_; > } Acknowledged. https://codereview.chromium.org/658013002/diff/140001/chrome/browser/chromeos... chrome/browser/chromeos/extensions/file_manager/file_manager_private_apitest.cc:147: void AddFileWatchCallback(bool success) { } On 2014/10/20 07:03:13, mtomasz wrote: > nit: Please use "git cl format". Done.
https://codereview.chromium.org/658013002/diff/180001/chrome/browser/chromeos... File chrome/browser/chromeos/extensions/file_manager/file_manager_private_apitest.cc (right): https://codereview.chromium.org/658013002/diff/180001/chrome/browser/chromeos... chrome/browser/chromeos/extensions/file_manager/file_manager_private_apitest.cc:122: drive::FileChange* CreateDirectoryDeletionFileChange( Is the pointer ownership properly managed, and the pointer removed? If not, then we can simply return FileChange. Or, we can use scoped pointers. https://codereview.chromium.org/658013002/diff/180001/chrome/browser/chromeos... chrome/browser/chromeos/extensions/file_manager/file_manager_private_apitest.cc:124: drive::FileChange* file_change = new drive::FileChange; nit: This can be "drive::FileChange* const file_change"
Thanks for the reviews. PTAL. https://codereview.chromium.org/658013002/diff/180001/chrome/browser/chromeos... File chrome/browser/chromeos/extensions/file_manager/file_manager_private_apitest.cc (right): https://codereview.chromium.org/658013002/diff/180001/chrome/browser/chromeos... chrome/browser/chromeos/extensions/file_manager/file_manager_private_apitest.cc:122: drive::FileChange* CreateDirectoryDeletionFileChange( On 2014/10/20 08:05:23, mtomasz wrote: > Is the pointer ownership properly managed, and the pointer removed? If not, then > we can simply return FileChange. Or, we can use scoped pointers. Done. Changed to use scoped_ptr. https://codereview.chromium.org/658013002/diff/180001/chrome/browser/chromeos... chrome/browser/chromeos/extensions/file_manager/file_manager_private_apitest.cc:124: drive::FileChange* file_change = new drive::FileChange; On 2014/10/20 08:05:22, mtomasz wrote: > nit: This can be "drive::FileChange* const file_change" Acknowledged.
https://codereview.chromium.org/658013002/diff/200001/chrome/browser/chromeos... File chrome/browser/chromeos/extensions/file_manager/file_manager_private_apitest.cc (right): https://codereview.chromium.org/658013002/diff/200001/chrome/browser/chromeos... chrome/browser/chromeos/extensions/file_manager/file_manager_private_apitest.cc:343: base::FilePath(FILE_PATH_LITERAL( I'm not really sure if that is correct. scoped_ptr instances will be removed once they get out of scope, and here we have only temporary objects, so we may end up on "use after free". WDYT? How about creating local variables holding the scoped pointers? scoped_ptr<drive::FileChange> first_change = CreateDirectory... event_router_->OnFileChanged(*file_change.get(), ...). BTW. can't we just avoid pointers and have a normal object?
PTAL. Thanks. https://codereview.chromium.org/658013002/diff/200001/chrome/browser/chromeos... File chrome/browser/chromeos/extensions/file_manager/file_manager_private_apitest.cc (right): https://codereview.chromium.org/658013002/diff/200001/chrome/browser/chromeos... chrome/browser/chromeos/extensions/file_manager/file_manager_private_apitest.cc:343: base::FilePath(FILE_PATH_LITERAL( On 2014/10/20 08:56:53, mtomasz wrote: > I'm not really sure if that is correct. scoped_ptr instances will be removed > once they get out of scope, and here we have only temporary objects, so we may > end up on "use after free". WDYT? > > How about creating local variables holding the scoped pointers? > > scoped_ptr<drive::FileChange> first_change = CreateDirectory... > event_router_->OnFileChanged(*file_change.get(), ...). > > BTW. can't we just avoid pointers and have a normal object? I checked the object is usable (not released) in EventRouter::OnFileChanged, and I also checked these created objects are freed correctly by inserting a log to destructor of FileChange. However, as you commented, from readability point of view, I removed the helper method and introduced temporary local variables. I think this makes the code much simpler.
https://codereview.chromium.org/658013002/diff/220001/chrome/browser/chromeos... File chrome/browser/chromeos/extensions/file_manager/event_router.cc (right): https://codereview.chromium.org/658013002/diff/220001/chrome/browser/chromeos... chrome/browser/chromeos/extensions/file_manager/event_router.cc:731: for (drive::FileChange::Map::const_iterator file_it = nit: I was just told that we can use some cool C++11 features, as auto and in-range for loops. How about using it here for better readability? http://chromium-cpp.appspot.com/ https://codereview.chromium.org/658013002/diff/220001/chrome/browser/chromeos... chrome/browser/chromeos/extensions/file_manager/event_router.cc:745: map[path.DirName()].Update(path, file_it->second); I don't really understand this logic, including the original code. In the for loop we add new entries to the map. So, if we have: /c DELETE:DIRECTORY, then we will add / DELETE:DIRECTORY. Is it right? That seems wrong, since / is not deleted. Also, we add elements to the map without resetting the iterator, so we may or may not iterate them (depending if they are added before or after the current iterator). I may be wrong here. Could you double check if the code logic is correct since you work on it? This code seems to be introduced by this patch: https://codereview.chromium.org/343073003.
Thank you for the reivew. I changed the file change of a deleted directory.
When /c {DELETE:DIRECTORY} comes to OnFileChanged, we created the following map,
map[/] = /c {DELETE:DIRECTORY}
map[/c] = /c {... EMPTY ...}
However, while double checking the logic, I came to the following should be
better, and changed to it.
map[/] = /c {DELETE:DIRECTORY}
map[/c] = /c {DELETE:DIRECTORY}
PTAL. Thanks.
https://codereview.chromium.org/658013002/diff/220001/chrome/browser/chromeos...
File chrome/browser/chromeos/extensions/file_manager/event_router.cc (right):
https://codereview.chromium.org/658013002/diff/220001/chrome/browser/chromeos...
chrome/browser/chromeos/extensions/file_manager/event_router.cc:731: for
(drive::FileChange::Map::const_iterator file_it =
On 2014/10/21 02:13:52, mtomasz wrote:
> nit: I was just told that we can use some cool C++11 features, as auto and
> in-range for loops. How about using it here for better readability?
>
> http://chromium-cpp.appspot.com/
Done.
https://codereview.chromium.org/658013002/diff/220001/chrome/browser/chromeos...
chrome/browser/chromeos/extensions/file_manager/event_router.cc:745:
map[path.DirName()].Update(path, file_it->second);
On 2014/10/21 02:13:52, mtomasz wrote:
> I don't really understand this logic, including the original code. In the for
> loop we add new entries to the map. So, if we have: /c DELETE:DIRECTORY, then
we
> will add / DELETE:DIRECTORY. Is it right? That seems wrong, since / is not
> deleted. Also, we add elements to the map without resetting the iterator, so
we
> may or may not iterate them (depending if they are added before or after the
> current iterator).
>
> I may be wrong here. Could you double check if the code logic is correct since
> you work on it?
>
> This code seems to be introduced by this patch:
> https://codereview.chromium.org/343073003.
No, file change itself is not changed. HandleFileWatchNotification requres the
directory which file change had happend in order to matching the watched
directories.
For example, if /c {DELETE:DIRECTORY} comes we create the following "map".
map[/] = /c {DELETE:DIRECTORY}
and it is passed as "a change had happend in / with /c {DELETE:DIRECTORY}".
In this cl we extend this to create the following map.
map[/] = /c {DELETE:DIRECTORY}
map[/c] = /c {DELETE:DIRECTORY}
The second question about adding elements to the map is not the problem here,
since the iterator(file_it) is not a iterator of the "map". changed_file_map and
"map" are different map.
Thank you for the reivew. I changed the file change of a deleted directory.
When /c {DELETE:DIRECTORY} comes to OnFileChanged, we created the following map,
map[/] = /c {DELETE:DIRECTORY}
map[/c] = /c {... EMPTY ...}
However, while double checking the logic, I came to the following should be
better, and changed to it.
map[/] = /c {DELETE:DIRECTORY}
map[/c] = /c {DELETE:DIRECTORY}
PTAL. Thanks.
https://codereview.chromium.org/658013002/diff/260001/chrome/browser/chromeos... File chrome/browser/chromeos/extensions/file_manager/event_router.cc (right): https://codereview.chromium.org/658013002/diff/260001/chrome/browser/chromeos... chrome/browser/chromeos/extensions/file_manager/event_router.cc:733: bool contains_directory_deletion = false; nit: Please add a comment what actually 'contains_directory_deletion' mean. I was confused by it. Basically if contains_directory_deletion is true, then the directory under that path was removed. 'contains' suggests that a child is deleted, but it's not true. https://codereview.chromium.org/658013002/diff/260001/chrome/browser/chromeos... chrome/browser/chromeos/extensions/file_manager/event_router.cc:758: file_watchers_it != file_watchers_.end() && What if we have watchers on /a and /aaa. Will /aaa be notified if /a is removed?
https://codereview.chromium.org/658013002/diff/260001/chrome/browser/chromeos... File chrome/browser/chromeos/extensions/file_manager/event_router.cc (right): https://codereview.chromium.org/658013002/diff/260001/chrome/browser/chromeos... chrome/browser/chromeos/extensions/file_manager/event_router.cc:733: bool contains_directory_deletion = false; nit: Please add a comment what actually 'contains_directory_deletion' mean. I was confused by it. Basically if contains_directory_deletion is true, then the directory under that path was removed. 'contains' suggests that a child is deleted, but it's not true. https://codereview.chromium.org/658013002/diff/260001/chrome/browser/chromeos... chrome/browser/chromeos/extensions/file_manager/event_router.cc:758: file_watchers_it != file_watchers_.end() && What if we have watchers on /a and /aaa. Will /aaa be notified if /a is removed?
I fixed the error case (/aaa), and added test case for it. PTAL. Thanks. https://codereview.chromium.org/658013002/diff/260001/chrome/browser/chromeos... File chrome/browser/chromeos/extensions/file_manager/event_router.cc (right): https://codereview.chromium.org/658013002/diff/260001/chrome/browser/chromeos... chrome/browser/chromeos/extensions/file_manager/event_router.cc:733: bool contains_directory_deletion = false; On 2014/10/21 07:16:56, mtomasz wrote: > nit: Please add a comment what actually 'contains_directory_deletion' mean. I > was confused by it. Basically if contains_directory_deletion is true, then the > directory under that path was removed. 'contains' suggests that a child is > deleted, but it's not true. Done. https://codereview.chromium.org/658013002/diff/260001/chrome/browser/chromeos... chrome/browser/chromeos/extensions/file_manager/event_router.cc:758: file_watchers_it != file_watchers_.end() && On 2014/10/21 07:16:56, mtomasz wrote: > What if we have watchers on /a and /aaa. Will /aaa be notified if /a is removed? This was a error case in the previous CL. I added test case for this, and fixed it. IsParent of FilePath checks whether the path is ancestor or not. e.g. (/a).IsParent(/a/b) -> true (/a).IsParent(/a/b/c) -> true (/a).IsParent(/aaa) -> false Definition of IsParent. https://code.google.com/p/chromium/codesearch#chromium/src/base/files/file_pa...
LGTM! Nice!
The CQ bit was checked by yawano@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/658013002/280001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: chromium_presubmit on tryserver.chromium.linux (http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
yawano@chromium.org changed reviewers: + yoshiki@chromium.org
@yoshiki: Could you review this code? I also need a LGTM from the owner of chrome/browser/chromeos/drive/file_change.cc. Thanks.
lgtm with nits. Thanks! https://codereview.chromium.org/658013002/diff/280001/chrome/browser/chromeos... File chrome/browser/chromeos/extensions/file_manager/event_router.cc (right): https://codereview.chromium.org/658013002/diff/280001/chrome/browser/chromeos... chrome/browser/chromeos/extensions/file_manager/event_router.cc:846: const base::FilePath& virtual_path, const drive::FileChange* list, nit: please put 1 argument per 1 line.
@yoshiki: Thank you for the review. https://codereview.chromium.org/658013002/diff/280001/chrome/browser/chromeos... File chrome/browser/chromeos/extensions/file_manager/event_router.cc (right): https://codereview.chromium.org/658013002/diff/280001/chrome/browser/chromeos... chrome/browser/chromeos/extensions/file_manager/event_router.cc:846: const base::FilePath& virtual_path, const drive::FileChange* list, On 2014/10/27 02:31:10, yoshiki wrote: > nit: please put 1 argument per 1 line. Done.
The CQ bit was checked by yawano@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/658013002/300001
Message was sent while issue was closed.
Committed patchset #16 (id:300001)
Message was sent while issue was closed.
Patchset 16 (id:??) landed as https://crrev.com/962dea43ddd90e7e4224a03fa3c36a421281abb7 Cr-Commit-Position: refs/heads/master@{#301325}
Message was sent while issue was closed.
A revert of this CL (patchset #16 id:300001) has been created in https://codereview.chromium.org/676403003/ by avi@chromium.org. The reason for reverting is: Linux Chromium OS ASan LSan Tests (1) http://build.chromium.org/p/chromium.memory/builders/Linux%20Chromium%20OS%20... started burning when this landed: @@@STEP_LOG_LINE@OnFileChanged@Direct leak of 48 byte(s) in 3 object(s) allocated from:@@@ @@@STEP_LOG_LINE@OnFileChanged@ #0 0x579f3b in operator new(unsigned long) (/tmp/run_tha_testfLYqpC/out/Release/browser_tests+0x579f3b)@@@ @@@STEP_LOG_LINE@OnFileChanged@ #1 0x4a85629 in file_manager::(anonymous namespace)::CreateAndStartFilePathWatcher(base::FilePath const&, base::Callback\u003Cvoid (base::FilePath const&, bool)> const&) chrome/browser/chromeos/file_manager/file_watcher.cc:24:3@@@ . |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
