Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
334263017:
[fsp] Remember mounted file systems as soon as possible. (Closed)
|
Created:
6 years, 6 months ago by mtomasz Modified:
6 years, 5 months ago Reviewers:
hashimoto CC:
chromium-reviews, stevenjb+watch_chromium.org, davemoore+watch_chromium.org, oshima+watch_chromium.org, nkostylev+watch_chromium.org Base URL:
svn://svn.chromium.org/chrome/trunk/src Project:
chromium Visibility:
Public. |
Description[fsp] Remember mounted file systems as soon as possible.
Before, mounted file systems were remembered at shutdown. However, if
a Chromebook crashes, or goes out of battery, then Chrome will fail to remount
previously mounted file systems after a reboot.
This patch remembers mounted file systems in preferences, as soon as they are
mounted, or unmounted.
TEST=unit_tests: *FileSystemProvider*Service*
BUG=389012
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=282589
Patch Set 1 #Patch Set 2 : Rewritten. #Patch Set 3 : Fixed when dots in keys. #Patch Set 4 : Updated tests with a dot. #Patch Set 5 : Cleaned up. #Patch Set 6 : Fixed forgetting on shutting down. #Patch Set 7 : Cleaned up. #
Total comments: 45
Patch Set 8 : Fixed. #
Total comments: 10
Patch Set 9 : Fixed. #Patch Set 10 : Fixed. #
Total comments: 12
Patch Set 11 : Addressed comments. #Patch Set 12 : Rebased. #
Total comments: 4
Patch Set 13 : Updated a comment. #
Messages
Total messages: 14 (0 generated)
@hashimoto: PTAL. Thanks.
https://codereview.chromium.org/334263017/diff/120001/chrome/browser/chromeos... File chrome/browser/chromeos/file_system_provider/service.cc (right): https://codereview.chromium.org/334263017/diff/120001/chrome/browser/chromeos... chrome/browser/chromeos/file_system_provider/service.cc:67: pref_service->ClearPref(prefs::kFileSystemProviderMounted); Is it OK to clear the pref at this point? Extensions may not be loaded if a crash happens, or the batter becomes empty. https://codereview.chromium.org/334263017/diff/120001/chrome/browser/chromeos... chrome/browser/chromeos/file_system_provider/service.cc:289: reason == extensions::UnloadedExtensionInfo::REASON_PROFILE_SHUTDOWN If this method gets called during shutdown, is it needed to call UnmountFileSystem() in the dtor? https://codereview.chromium.org/334263017/diff/120001/chrome/browser/chromeos... chrome/browser/chromeos/file_system_provider/service.cc:335: file_system->SetString(kPrefKeyFileSystemId, Please use one of "FooWithoutPathExpansion" or "Foo" variants exclusively. ("WithoutPathExpansion" is better) https://codereview.chromium.org/334263017/diff/120001/chrome/browser/chromeos... chrome/browser/chromeos/file_system_provider/service.cc:382: if (!file_systems_to_restore_->GetDictionary(extension_id, Is it OK to be unable to read prefs saved by the older version code? https://codereview.chromium.org/334263017/diff/120001/chrome/browser/chromeos... chrome/browser/chromeos/file_system_provider/service.cc:388: while (!it.IsAtEnd()) { nit: How about "for (base::DicationaryValue::Iterator it(...); !it.IsAtEnd(); it.Advance())"? https://codereview.chromium.org/334263017/diff/120001/chrome/browser/chromeos... File chrome/browser/chromeos/file_system_provider/service.h (right): https://codereview.chromium.org/334263017/diff/120001/chrome/browser/chromeos... chrome/browser/chromeos/file_system_provider/service.h:61: // Mode of unmounting. In case of UNMOUNT_MODE_SHUTDOWN, the file system will nit: How about using the term "reason" instead of "mode" to be consistent with extensions::UnloadedExtensionInfo? https://codereview.chromium.org/334263017/diff/120001/chrome/browser/chromeos... chrome/browser/chromeos/file_system_provider/service.h:64: enum UnmountMode { UNMOUNT_MODE_USER, UNMOUNT_MODE_SHUTDOWN }; Is there any reason to make this enum public? Seems UNMOUNT_MODE_SHUTDOWN is only used by Service itself and there is no reason to allow external code to call UnmountFileSystem() with SHUTDOWN. https://codereview.chromium.org/334263017/diff/120001/chrome/browser/chromeos... File chrome/browser/chromeos/file_system_provider/service_unittest.cc (right): https://codereview.chromium.org/334263017/diff/120001/chrome/browser/chromeos... chrome/browser/chromeos/file_system_provider/service_unittest.cc:34: const char kFileSystemId[] = "camera/pictures/id .!@#$%^&*()_+"; What is the point of this change? https://codereview.chromium.org/334263017/diff/120001/chrome/browser/chromeos... chrome/browser/chromeos/file_system_provider/service_unittest.cc:309: { Why do we need a scope here? https://codereview.chromium.org/334263017/diff/120001/chrome/browser/chromeos... chrome/browser/chromeos/file_system_provider/service_unittest.cc:310: scoped_ptr<Service> service( Why can't you call reset() against |service_| here? https://codereview.chromium.org/334263017/diff/120001/chrome/browser/chromeos... chrome/browser/chromeos/file_system_provider/service_unittest.cc:311: new Service(profile_.get(), extension_registry_.get())); Please add a comment to describe that you want to load the saved file system by creating a new Service. https://codereview.chromium.org/334263017/diff/120001/chrome/browser/chromeos... chrome/browser/chromeos/file_system_provider/service_unittest.cc:377: TEST_F(FileSystemProviderServiceTest, RememberFileSystem_OnUnmountByUser) { What is the relationship between these two new test cases and the removed test cases, "ForgetFileSystem_OnExtensionUnload" and "RememberFileSystem_OnShutdown"?
https://codereview.chromium.org/334263017/diff/120001/chrome/browser/chromeos... File chrome/browser/chromeos/file_system_provider/service.cc (right): https://codereview.chromium.org/334263017/diff/120001/chrome/browser/chromeos... chrome/browser/chromeos/file_system_provider/service.cc:67: pref_service->ClearPref(prefs::kFileSystemProviderMounted); On 2014/07/02 08:59:37, hashimoto wrote: > Is it OK to clear the pref at this point? > Extensions may not be loaded if a crash happens, or the batter becomes empty. Done. https://codereview.chromium.org/334263017/diff/120001/chrome/browser/chromeos... chrome/browser/chromeos/file_system_provider/service.cc:289: reason == extensions::UnloadedExtensionInfo::REASON_PROFILE_SHUTDOWN On 2014/07/02 08:59:37, hashimoto wrote: > If this method gets called during shutdown, is it needed to call > UnmountFileSystem() in the dtor? That's correct but but not in case of unit tests. I'd like to clean it up in a separate patch, since there is no quick fix for it. https://codereview.chromium.org/334263017/diff/120001/chrome/browser/chromeos... chrome/browser/chromeos/file_system_provider/service.cc:335: file_system->SetString(kPrefKeyFileSystemId, On 2014/07/02 08:59:37, hashimoto wrote: > Please use one of "FooWithoutPathExpansion" or "Foo" variants exclusively. > ("WithoutPathExpansion" is better) Currently I'm using WithoutPathExpansion only when the key can contain a dot. Using it everywhere would make the code bloated. https://codereview.chromium.org/334263017/diff/120001/chrome/browser/chromeos... chrome/browser/chromeos/file_system_provider/service.cc:382: if (!file_systems_to_restore_->GetDictionary(extension_id, On 2014/07/02 08:59:37, hashimoto wrote: > Is it OK to be unable to read prefs saved by the older version code? Yes. The API is not released yet, and it is available when built from sources only. https://codereview.chromium.org/334263017/diff/120001/chrome/browser/chromeos... chrome/browser/chromeos/file_system_provider/service.cc:388: while (!it.IsAtEnd()) { On 2014/07/02 08:59:37, hashimoto wrote: > nit: How about "for (base::DicationaryValue::Iterator it(...); !it.IsAtEnd(); > it.Advance())"? Done. https://codereview.chromium.org/334263017/diff/120001/chrome/browser/chromeos... File chrome/browser/chromeos/file_system_provider/service.h (right): https://codereview.chromium.org/334263017/diff/120001/chrome/browser/chromeos... chrome/browser/chromeos/file_system_provider/service.h:61: // Mode of unmounting. In case of UNMOUNT_MODE_SHUTDOWN, the file system will On 2014/07/02 08:59:37, hashimoto wrote: > nit: How about using the term "reason" instead of "mode" to be consistent with > extensions::UnloadedExtensionInfo? Good idea. Done. https://codereview.chromium.org/334263017/diff/120001/chrome/browser/chromeos... chrome/browser/chromeos/file_system_provider/service.h:64: enum UnmountMode { UNMOUNT_MODE_USER, UNMOUNT_MODE_SHUTDOWN }; On 2014/07/02 08:59:37, hashimoto wrote: > Is there any reason to make this enum public? > Seems UNMOUNT_MODE_SHUTDOWN is only used by Service itself and there is no > reason to allow external code to call UnmountFileSystem() with SHUTDOWN. This enum is used from file_system_provider_api.cc. Alternatively, we could have an extra method and make the enum private. However, that method would be called only from one place. Public enum and one method is just simpler. WDYT? https://codereview.chromium.org/334263017/diff/120001/chrome/browser/chromeos... File chrome/browser/chromeos/file_system_provider/service_unittest.cc (right): https://codereview.chromium.org/334263017/diff/120001/chrome/browser/chromeos... chrome/browser/chromeos/file_system_provider/service_unittest.cc:34: const char kFileSystemId[] = "camera/pictures/id .!@#$%^&*()_+"; On 2014/07/02 08:59:37, hashimoto wrote: > What is the point of this change? File system IDs are used as keys in preferences. This dot ensures, that we correctly save preferences. Note, that Value::Set(kFileSystemId, ...) would expand the key, which is unexpected. SetWithoutPathExpansion has to be used. https://codereview.chromium.org/334263017/diff/120001/chrome/browser/chromeos... chrome/browser/chromeos/file_system_provider/service_unittest.cc:309: { On 2014/07/02 08:59:37, hashimoto wrote: > Why do we need a scope here? Not needed. Done. https://codereview.chromium.org/334263017/diff/120001/chrome/browser/chromeos... chrome/browser/chromeos/file_system_provider/service_unittest.cc:310: scoped_ptr<Service> service( On 2014/07/02 08:59:37, hashimoto wrote: > Why can't you call reset() against |service_| here? Sure I can, but is it better? The new instance is needed only in this scope, so is there a reason to save it as a member? https://codereview.chromium.org/334263017/diff/120001/chrome/browser/chromeos... chrome/browser/chromeos/file_system_provider/service_unittest.cc:311: new Service(profile_.get(), extension_registry_.get())); On 2014/07/02 08:59:37, hashimoto wrote: > Please add a comment to describe that you want to load the saved file system by > creating a new Service. Done. https://codereview.chromium.org/334263017/diff/120001/chrome/browser/chromeos... chrome/browser/chromeos/file_system_provider/service_unittest.cc:377: TEST_F(FileSystemProviderServiceTest, RememberFileSystem_OnUnmountByUser) { On 2014/07/02 08:59:38, hashimoto wrote: > What is the relationship between these two new test cases and the removed test > cases, "ForgetFileSystem_OnExtensionUnload" and "RememberFileSystem_OnShutdown"? ForgetFileSystem_OnExtensionUnload -> RememberFileSystem_OnUnmountByUser RememberFileSystem_OnShutdown -> RememberFileSystem_OnUnmountOnShutdown
https://codereview.chromium.org/334263017/diff/120001/chrome/browser/chromeos... File chrome/browser/chromeos/file_system_provider/service.cc (right): https://codereview.chromium.org/334263017/diff/120001/chrome/browser/chromeos... chrome/browser/chromeos/file_system_provider/service.cc:289: reason == extensions::UnloadedExtensionInfo::REASON_PROFILE_SHUTDOWN On 2014/07/03 05:24:07, mtomasz wrote: > On 2014/07/02 08:59:37, hashimoto wrote: > > If this method gets called during shutdown, is it needed to call > > UnmountFileSystem() in the dtor? > > That's correct but but not in case of unit tests. I'd like to clean it up in a > separate patch, since there is no quick fix for it. Please add a TODO comment to remove the redundant code. https://codereview.chromium.org/334263017/diff/120001/chrome/browser/chromeos... chrome/browser/chromeos/file_system_provider/service.cc:335: file_system->SetString(kPrefKeyFileSystemId, On 2014/07/03 05:24:07, mtomasz wrote: > On 2014/07/02 08:59:37, hashimoto wrote: > > Please use one of "FooWithoutPathExpansion" or "Foo" variants exclusively. > > ("WithoutPathExpansion" is better) > > Currently I'm using WithoutPathExpansion only when the key can contain a dot. > Using it everywhere would make the code bloated. The problem is that nothing can prevent you and other developers from using a wrong variant when adding a new pref member. I don't think we should make the product code bug-prone just to save a handful of key strokes. Please use WithoutPathExpansion variants everywhere. https://codereview.chromium.org/334263017/diff/120001/chrome/browser/chromeos... File chrome/browser/chromeos/file_system_provider/service.h (right): https://codereview.chromium.org/334263017/diff/120001/chrome/browser/chromeos... chrome/browser/chromeos/file_system_provider/service.h:64: enum UnmountMode { UNMOUNT_MODE_USER, UNMOUNT_MODE_SHUTDOWN }; On 2014/07/03 05:24:07, mtomasz wrote: > On 2014/07/02 08:59:37, hashimoto wrote: > > Is there any reason to make this enum public? > > Seems UNMOUNT_MODE_SHUTDOWN is only used by Service itself and there is no > > reason to allow external code to call UnmountFileSystem() with SHUTDOWN. > > This enum is used from file_system_provider_api.cc. > Alternatively, we could have an extra method and make the enum private. However, > that method would be called only from one place. Public enum and one method is > just simpler. WDYT? Public interface should be minimal to encapsulte things in each class and to limit where to look when things get broken. Seems in the new patch set UnmountFileSystem(SHUTDOWN) can be replaced with UnmountFileSystem()+ForgetFileSystem(). Please remove this enum. https://codereview.chromium.org/334263017/diff/120001/chrome/browser/chromeos... File chrome/browser/chromeos/file_system_provider/service_unittest.cc (right): https://codereview.chromium.org/334263017/diff/120001/chrome/browser/chromeos... chrome/browser/chromeos/file_system_provider/service_unittest.cc:34: const char kFileSystemId[] = "camera/pictures/id .!@#$%^&*()_+"; On 2014/07/03 05:24:07, mtomasz wrote: > On 2014/07/02 08:59:37, hashimoto wrote: > > What is the point of this change? > > File system IDs are used as keys in preferences. This dot ensures, that we > correctly save preferences. Note, that Value::Set(kFileSystemId, ...) would > expand the key, which is unexpected. SetWithoutPathExpansion has to be used. Then "." here has a special role unlike other random symbols. Please add a comment about it. https://codereview.chromium.org/334263017/diff/120001/chrome/browser/chromeos... chrome/browser/chromeos/file_system_provider/service_unittest.cc:310: scoped_ptr<Service> service( On 2014/07/03 05:24:07, mtomasz wrote: > On 2014/07/02 08:59:37, hashimoto wrote: > > Why can't you call reset() against |service_| here? > > Sure I can, but is it better? The new instance is needed only in this scope, so > is there a reason to save it as a member? Having |service| and |service_| in the same scope looks ambiguous. Please remove this variable or rename it to a more distinguishable name. https://codereview.chromium.org/334263017/diff/120001/chrome/browser/chromeos... chrome/browser/chromeos/file_system_provider/service_unittest.cc:377: TEST_F(FileSystemProviderServiceTest, RememberFileSystem_OnUnmountByUser) { On 2014/07/03 05:24:07, mtomasz wrote: > On 2014/07/02 08:59:38, hashimoto wrote: > > What is the relationship between these two new test cases and the removed test > > cases, "ForgetFileSystem_OnExtensionUnload" and > "RememberFileSystem_OnShutdown"? > > ForgetFileSystem_OnExtensionUnload -> RememberFileSystem_OnUnmountByUser > RememberFileSystem_OnShutdown -> RememberFileSystem_OnUnmountOnShutdown Why did you change the order? Even if you need to do that, could you do that in a separate change? It's hard to review all of file_system_provider_service_ renaming, pref structure change and test case reordering at the same time. https://codereview.chromium.org/334263017/diff/140001/chrome/browser/chromeos... File chrome/browser/chromeos/file_system_provider/service.cc (right): https://codereview.chromium.org/334263017/diff/140001/chrome/browser/chromeos... chrome/browser/chromeos/file_system_provider/service.cc:66: file_systems_to_restore_.reset(file_systems->DeepCopy()); Do we need this code now? You can read the dictionary from the prefs directly without file_systems_to_restore_. https://codereview.chromium.org/334263017/diff/140001/chrome/browser/chromeos... chrome/browser/chromeos/file_system_provider/service.cc:226: if (reason == UNMOUNT_REASON_USER) multiple lines should be accommodated with {}. https://codereview.chromium.org/334263017/diff/140001/chrome/browser/chromeos... chrome/browser/chromeos/file_system_provider/service.cc:360: pref_service->CommitPendingWrite(); pref_service.h says CommitPendingWrite() should be used only during shutdown. Do we need this now? https://codereview.chromium.org/334263017/diff/140001/chrome/browser/chromeos... chrome/browser/chromeos/file_system_provider/service.cc:380: pref_service->CommitPendingWrite(); ditto.
https://codereview.chromium.org/334263017/diff/120001/chrome/browser/chromeos... File chrome/browser/chromeos/file_system_provider/service.cc (right): https://codereview.chromium.org/334263017/diff/120001/chrome/browser/chromeos... chrome/browser/chromeos/file_system_provider/service.cc:289: reason == extensions::UnloadedExtensionInfo::REASON_PROFILE_SHUTDOWN On 2014/07/04 06:38:41, hashimoto wrote: > On 2014/07/03 05:24:07, mtomasz wrote: > > On 2014/07/02 08:59:37, hashimoto wrote: > > > If this method gets called during shutdown, is it needed to call > > > UnmountFileSystem() in the dtor? > > > > That's correct but but not in case of unit tests. I'd like to clean it up in a > > separate patch, since there is no quick fix for it. > > Please add a TODO comment to remove the redundant code. Done. https://codereview.chromium.org/334263017/diff/120001/chrome/browser/chromeos... chrome/browser/chromeos/file_system_provider/service.cc:335: file_system->SetString(kPrefKeyFileSystemId, On 2014/07/04 06:38:41, hashimoto wrote: > On 2014/07/03 05:24:07, mtomasz wrote: > > On 2014/07/02 08:59:37, hashimoto wrote: > > > Please use one of "FooWithoutPathExpansion" or "Foo" variants exclusively. > > > ("WithoutPathExpansion" is better) > > > > Currently I'm using WithoutPathExpansion only when the key can contain a dot. > > Using it everywhere would make the code bloated. > The problem is that nothing can prevent you and other developers from using a > wrong variant when adding a new pref member. > I don't think we should make the product code bug-prone just to save a handful > of key strokes. > Please use WithoutPathExpansion variants everywhere. Do you want me to use WithoutPathExpansion variants in the entire File System Provider API code for every base::DictionaryValue? There are two variants of methods, one with expansion and one without. I don't see reason why not using both of them depending on the situation. https://codereview.chromium.org/334263017/diff/120001/chrome/browser/chromeos... File chrome/browser/chromeos/file_system_provider/service.h (right): https://codereview.chromium.org/334263017/diff/120001/chrome/browser/chromeos... chrome/browser/chromeos/file_system_provider/service.h:64: enum UnmountMode { UNMOUNT_MODE_USER, UNMOUNT_MODE_SHUTDOWN }; On 2014/07/04 06:38:41, hashimoto wrote: > On 2014/07/03 05:24:07, mtomasz wrote: > > On 2014/07/02 08:59:37, hashimoto wrote: > > > Is there any reason to make this enum public? > > > Seems UNMOUNT_MODE_SHUTDOWN is only used by Service itself and there is no > > > reason to allow external code to call UnmountFileSystem() with SHUTDOWN. > > > > This enum is used from file_system_provider_api.cc. > > Alternatively, we could have an extra method and make the enum private. > However, > > that method would be called only from one place. Public enum and one method is > > just simpler. WDYT? > > Public interface should be minimal to encapsulte things in each class and to > limit where to look when things get broken. > Seems in the new patch set UnmountFileSystem(SHUTDOWN) can be replaced with > UnmountFileSystem()+ForgetFileSystem(). > Please remove this enum. We don't want to forget on SHUTDOWN. We need to forget on USER. Hence, I don't see an easy way to remove this enum without creating extra methods. https://codereview.chromium.org/334263017/diff/120001/chrome/browser/chromeos... File chrome/browser/chromeos/file_system_provider/service_unittest.cc (right): https://codereview.chromium.org/334263017/diff/120001/chrome/browser/chromeos... chrome/browser/chromeos/file_system_provider/service_unittest.cc:34: const char kFileSystemId[] = "camera/pictures/id .!@#$%^&*()_+"; On 2014/07/04 06:38:41, hashimoto wrote: > On 2014/07/03 05:24:07, mtomasz wrote: > > On 2014/07/02 08:59:37, hashimoto wrote: > > > What is the point of this change? > > > > File system IDs are used as keys in preferences. This dot ensures, that we > > correctly save preferences. Note, that Value::Set(kFileSystemId, ...) would > > expand the key, which is unexpected. SetWithoutPathExpansion has to be used. > > Then "." here has a special role unlike other random symbols. > Please add a comment about it. Done. https://codereview.chromium.org/334263017/diff/120001/chrome/browser/chromeos... chrome/browser/chromeos/file_system_provider/service_unittest.cc:310: scoped_ptr<Service> service( On 2014/07/04 06:38:41, hashimoto wrote: > On 2014/07/03 05:24:07, mtomasz wrote: > > On 2014/07/02 08:59:37, hashimoto wrote: > > > Why can't you call reset() against |service_| here? > > > > Sure I can, but is it better? The new instance is needed only in this scope, > so > > is there a reason to save it as a member? > > Having |service| and |service_| in the same scope looks ambiguous. > Please remove this variable or rename it to a more distinguishable name. Done. https://codereview.chromium.org/334263017/diff/120001/chrome/browser/chromeos... chrome/browser/chromeos/file_system_provider/service_unittest.cc:377: TEST_F(FileSystemProviderServiceTest, RememberFileSystem_OnUnmountByUser) { On 2014/07/04 06:38:41, hashimoto wrote: > On 2014/07/03 05:24:07, mtomasz wrote: > > On 2014/07/02 08:59:38, hashimoto wrote: > > > What is the relationship between these two new test cases and the removed > test > > > cases, "ForgetFileSystem_OnExtensionUnload" and > > "RememberFileSystem_OnShutdown"? > > > > ForgetFileSystem_OnExtensionUnload -> RememberFileSystem_OnUnmountByUser > > RememberFileSystem_OnShutdown -> RememberFileSystem_OnUnmountOnShutdown > > Why did you change the order? > Even if you need to do that, could you do that in a separate change? > It's hard to review all of file_system_provider_service_ renaming, pref > structure change and test case reordering at the same time. I flipped the order, but the diff doesn't look much better. https://codereview.chromium.org/334263017/diff/140001/chrome/browser/chromeos... File chrome/browser/chromeos/file_system_provider/service.cc (right): https://codereview.chromium.org/334263017/diff/140001/chrome/browser/chromeos... chrome/browser/chromeos/file_system_provider/service.cc:66: file_systems_to_restore_.reset(file_systems->DeepCopy()); On 2014/07/04 06:38:42, hashimoto wrote: > Do we need this code now? > You can read the dictionary from the prefs directly without > file_systems_to_restore_. It makes things easier. Note, that In #420 we want to remove the file system from preferences, while we are iterating. If we didn't have this |file_systems_to_restore_| member, then we could end up on a segmentation fault error. https://codereview.chromium.org/334263017/diff/140001/chrome/browser/chromeos... chrome/browser/chromeos/file_system_provider/service.cc:226: if (reason == UNMOUNT_REASON_USER) On 2014/07/04 06:38:42, hashimoto wrote: > multiple lines should be accommodated with {}. Done. https://codereview.chromium.org/334263017/diff/140001/chrome/browser/chromeos... chrome/browser/chromeos/file_system_provider/service.cc:360: pref_service->CommitPendingWrite(); On 2014/07/04 06:38:42, hashimoto wrote: > pref_service.h says CommitPendingWrite() should be used only during shutdown. > Do we need this now? If a crash happens, then we don't want to lose mounted file systems. However, it seems that the current pref service implementation flushes preferences every 10 seconds after a change. I think it is fast enough. Removed. Done. https://codereview.chromium.org/334263017/diff/140001/chrome/browser/chromeos... chrome/browser/chromeos/file_system_provider/service.cc:380: pref_service->CommitPendingWrite(); On 2014/07/04 06:38:41, hashimoto wrote: > ditto. Done.
https://codereview.chromium.org/334263017/diff/120001/chrome/browser/chromeos... File chrome/browser/chromeos/file_system_provider/service.cc (right): https://codereview.chromium.org/334263017/diff/120001/chrome/browser/chromeos... chrome/browser/chromeos/file_system_provider/service.cc:335: file_system->SetString(kPrefKeyFileSystemId, On 2014/07/04 07:50:42, mtomasz wrote: > On 2014/07/04 06:38:41, hashimoto wrote: > > On 2014/07/03 05:24:07, mtomasz wrote: > > > On 2014/07/02 08:59:37, hashimoto wrote: > > > > Please use one of "FooWithoutPathExpansion" or "Foo" variants exclusively. > > > > ("WithoutPathExpansion" is better) > > > > > > Currently I'm using WithoutPathExpansion only when the key can contain a > dot. > > > Using it everywhere would make the code bloated. > > The problem is that nothing can prevent you and other developers from using a > > wrong variant when adding a new pref member. > > I don't think we should make the product code bug-prone just to save a handful > > of key strokes. > > Please use WithoutPathExpansion variants everywhere. > > Do you want me to use WithoutPathExpansion variants in the entire File System > Provider API code for every base::DictionaryValue? > > There are two variants of methods, one with expansion and one without. I don't > see reason why not using both of them depending on the situation. What Set/Get does is Set/GetWithoutPathExpansion + path expansion. When you don't need the path expansion functionality, you should use the WithoutPathExpansion variant. https://codereview.chromium.org/334263017/diff/120001/chrome/browser/chromeos... File chrome/browser/chromeos/file_system_provider/service.h (right): https://codereview.chromium.org/334263017/diff/120001/chrome/browser/chromeos... chrome/browser/chromeos/file_system_provider/service.h:64: enum UnmountMode { UNMOUNT_MODE_USER, UNMOUNT_MODE_SHUTDOWN }; On 2014/07/04 07:50:42, mtomasz wrote: > On 2014/07/04 06:38:41, hashimoto wrote: > > On 2014/07/03 05:24:07, mtomasz wrote: > > > On 2014/07/02 08:59:37, hashimoto wrote: > > > > Is there any reason to make this enum public? > > > > Seems UNMOUNT_MODE_SHUTDOWN is only used by Service itself and there is no > > > > reason to allow external code to call UnmountFileSystem() with SHUTDOWN. > > > > > > This enum is used from file_system_provider_api.cc. > > > Alternatively, we could have an extra method and make the enum private. > > However, > > > that method would be called only from one place. Public enum and one method > is > > > just simpler. WDYT? > > > > Public interface should be minimal to encapsulte things in each class and to > > limit where to look when things get broken. > > Seems in the new patch set UnmountFileSystem(SHUTDOWN) can be replaced with > > UnmountFileSystem()+ForgetFileSystem(). > > Please remove this enum. > > We don't want to forget on SHUTDOWN. We need to forget on USER. Hence, I don't > see an easy way to remove this enum without creating extra methods. > Ah, I see. Then exposing this enum may be acceptable. https://codereview.chromium.org/334263017/diff/120001/chrome/browser/chromeos... File chrome/browser/chromeos/file_system_provider/service_unittest.cc (right): https://codereview.chromium.org/334263017/diff/120001/chrome/browser/chromeos... chrome/browser/chromeos/file_system_provider/service_unittest.cc:310: scoped_ptr<Service> service( On 2014/07/04 07:50:42, mtomasz wrote: > On 2014/07/04 06:38:41, hashimoto wrote: > > On 2014/07/03 05:24:07, mtomasz wrote: > > > On 2014/07/02 08:59:37, hashimoto wrote: > > > > Why can't you call reset() against |service_| here? > > > > > > Sure I can, but is it better? The new instance is needed only in this scope, > > so > > > is there a reason to save it as a member? > > > > Having |service| and |service_| in the same scope looks ambiguous. > > Please remove this variable or rename it to a more distinguishable name. > > Done. |file_system_provider_service| does not tell how it's different from |service_|. Please use a more meaningful name like |new_service| or |service_after_reboot|. https://codereview.chromium.org/334263017/diff/140001/chrome/browser/chromeos... File chrome/browser/chromeos/file_system_provider/service.cc (right): https://codereview.chromium.org/334263017/diff/140001/chrome/browser/chromeos... chrome/browser/chromeos/file_system_provider/service.cc:66: file_systems_to_restore_.reset(file_systems->DeepCopy()); On 2014/07/04 07:50:42, mtomasz wrote: > On 2014/07/04 06:38:42, hashimoto wrote: > > Do we need this code now? > > You can read the dictionary from the prefs directly without > > file_systems_to_restore_. > > It makes things easier. Note, that In #420 we want to remove the file system > from preferences, while we are iterating. If we didn't have this > |file_systems_to_restore_| member, then we could end up on a segmentation fault > error. You can save a list of file systems to be removed while iterating and remove the file systems after that. Or you can make a copy of the dictionary just before starting the iteration. https://codereview.chromium.org/334263017/diff/180001/chrome/browser/chromeos... File chrome/browser/chromeos/file_system_provider/service_unittest.cc (right): https://codereview.chromium.org/334263017/diff/180001/chrome/browser/chromeos... chrome/browser/chromeos/file_system_provider/service_unittest.cc:349: EXPECT_TRUE(result); nit: EXPECT_TRUE(service_->MountFileSystem())? https://codereview.chromium.org/334263017/diff/180001/chrome/browser/chromeos... chrome/browser/chromeos/file_system_provider/service_unittest.cc:352: TestingPrefServiceSyncable* const pref_service = nit: No need to have const here. We don't usually care about the const correctness of the pointer itself. https://codereview.chromium.org/334263017/diff/180001/chrome/browser/chromeos... chrome/browser/chromeos/file_system_provider/service_unittest.cc:360: const base::DictionaryValue* file_systems; nit: Initialize with NULL. https://codereview.chromium.org/334263017/diff/180001/chrome/browser/chromeos... chrome/browser/chromeos/file_system_provider/service_unittest.cc:362: ASSERT_EQ(1u, file_systems->size()); nit: Can be EXPECT. https://codereview.chromium.org/334263017/diff/180001/chrome/browser/chromeos... chrome/browser/chromeos/file_system_provider/service_unittest.cc:389: { No need to have this scope. https://codereview.chromium.org/334263017/diff/180001/chrome/browser/chromeos... chrome/browser/chromeos/file_system_provider/service_unittest.cc:399: const base::DictionaryValue* file_systems; nit: Initialize with NULL.
https://codereview.chromium.org/334263017/diff/120001/chrome/browser/chromeos... File chrome/browser/chromeos/file_system_provider/service.cc (right): https://codereview.chromium.org/334263017/diff/120001/chrome/browser/chromeos... chrome/browser/chromeos/file_system_provider/service.cc:335: file_system->SetString(kPrefKeyFileSystemId, On 2014/07/07 11:07:46, hashimoto wrote: > On 2014/07/04 07:50:42, mtomasz wrote: > > On 2014/07/04 06:38:41, hashimoto wrote: > > > On 2014/07/03 05:24:07, mtomasz wrote: > > > > On 2014/07/02 08:59:37, hashimoto wrote: > > > > > Please use one of "FooWithoutPathExpansion" or "Foo" variants > exclusively. > > > > > ("WithoutPathExpansion" is better) > > > > > > > > Currently I'm using WithoutPathExpansion only when the key can contain a > > dot. > > > > Using it everywhere would make the code bloated. > > > The problem is that nothing can prevent you and other developers from using > a > > > wrong variant when adding a new pref member. > > > I don't think we should make the product code bug-prone just to save a > handful > > > of key strokes. > > > Please use WithoutPathExpansion variants everywhere. > > > > Do you want me to use WithoutPathExpansion variants in the entire File System > > Provider API code for every base::DictionaryValue? > > > > There are two variants of methods, one with expansion and one without. I don't > > see reason why not using both of them depending on the situation. > > What Set/Get does is Set/GetWithoutPathExpansion + path expansion. > When you don't need the path expansion functionality, you should use the > WithoutPathExpansion variant. According to code search's xrefs: DictionaryValue::SetStringWithoutPathExpansion() 176 calls DictionaryValue::SetString() 2852 calls But since you prefer the longer version, fixed. Done. https://codereview.chromium.org/334263017/diff/120001/chrome/browser/chromeos... File chrome/browser/chromeos/file_system_provider/service_unittest.cc (right): https://codereview.chromium.org/334263017/diff/120001/chrome/browser/chromeos... chrome/browser/chromeos/file_system_provider/service_unittest.cc:310: scoped_ptr<Service> service( On 2014/07/07 11:07:46, hashimoto wrote: > On 2014/07/04 07:50:42, mtomasz wrote: > > On 2014/07/04 06:38:41, hashimoto wrote: > > > On 2014/07/03 05:24:07, mtomasz wrote: > > > > On 2014/07/02 08:59:37, hashimoto wrote: > > > > > Why can't you call reset() against |service_| here? > > > > > > > > Sure I can, but is it better? The new instance is needed only in this > scope, > > > so > > > > is there a reason to save it as a member? > > > > > > Having |service| and |service_| in the same scope looks ambiguous. > > > Please remove this variable or rename it to a more distinguishable name. > > > > Done. > > |file_system_provider_service| does not tell how it's different from |service_|. > Please use a more meaningful name like |new_service| or |service_after_reboot|. Done. https://codereview.chromium.org/334263017/diff/140001/chrome/browser/chromeos... File chrome/browser/chromeos/file_system_provider/service.cc (right): https://codereview.chromium.org/334263017/diff/140001/chrome/browser/chromeos... chrome/browser/chromeos/file_system_provider/service.cc:66: file_systems_to_restore_.reset(file_systems->DeepCopy()); On 2014/07/07 11:07:46, hashimoto wrote: > On 2014/07/04 07:50:42, mtomasz wrote: > > On 2014/07/04 06:38:42, hashimoto wrote: > > > Do we need this code now? > > > You can read the dictionary from the prefs directly without > > > file_systems_to_restore_. > > > > It makes things easier. Note, that In #420 we want to remove the file system > > from preferences, while we are iterating. If we didn't have this > > |file_systems_to_restore_| member, then we could end up on a segmentation > fault > > error. > > You can save a list of file systems to be removed while iterating and remove the > file systems after that. > Or you can make a copy of the dictionary just before starting the iteration. Done. https://codereview.chromium.org/334263017/diff/180001/chrome/browser/chromeos... File chrome/browser/chromeos/file_system_provider/service_unittest.cc (right): https://codereview.chromium.org/334263017/diff/180001/chrome/browser/chromeos... chrome/browser/chromeos/file_system_provider/service_unittest.cc:349: EXPECT_TRUE(result); On 2014/07/07 11:07:46, hashimoto wrote: > nit: EXPECT_TRUE(service_->MountFileSystem())? Done. https://codereview.chromium.org/334263017/diff/180001/chrome/browser/chromeos... chrome/browser/chromeos/file_system_provider/service_unittest.cc:352: TestingPrefServiceSyncable* const pref_service = On 2014/07/07 11:07:46, hashimoto wrote: > nit: No need to have const here. We don't usually care about the const > correctness of the pointer itself. I try to keep File System Provider API code const correct wherever possible. Keeping const gives us additional compile time bug protection, so let me keep it. https://codereview.chromium.org/334263017/diff/180001/chrome/browser/chromeos... chrome/browser/chromeos/file_system_provider/service_unittest.cc:360: const base::DictionaryValue* file_systems; On 2014/07/07 11:07:46, hashimoto wrote: > nit: Initialize with NULL. Done. https://codereview.chromium.org/334263017/diff/180001/chrome/browser/chromeos... chrome/browser/chromeos/file_system_provider/service_unittest.cc:362: ASSERT_EQ(1u, file_systems->size()); On 2014/07/07 11:07:46, hashimoto wrote: > nit: Can be EXPECT. Done. https://codereview.chromium.org/334263017/diff/180001/chrome/browser/chromeos... chrome/browser/chromeos/file_system_provider/service_unittest.cc:389: { On 2014/07/07 11:07:46, hashimoto wrote: > No need to have this scope. Having a scope here allows to have the same variable names, hence simple code. https://codereview.chromium.org/334263017/diff/180001/chrome/browser/chromeos... chrome/browser/chromeos/file_system_provider/service_unittest.cc:399: const base::DictionaryValue* file_systems; On 2014/07/07 11:07:46, hashimoto wrote: > nit: Initialize with NULL. Done.
https://codereview.chromium.org/334263017/diff/120001/chrome/browser/chromeos... File chrome/browser/chromeos/file_system_provider/service.cc (right): https://codereview.chromium.org/334263017/diff/120001/chrome/browser/chromeos... chrome/browser/chromeos/file_system_provider/service.cc:335: file_system->SetString(kPrefKeyFileSystemId, On 2014/07/08 01:52:08, mtomasz wrote: > On 2014/07/07 11:07:46, hashimoto wrote: > > On 2014/07/04 07:50:42, mtomasz wrote: > > > On 2014/07/04 06:38:41, hashimoto wrote: > > > > On 2014/07/03 05:24:07, mtomasz wrote: > > > > > On 2014/07/02 08:59:37, hashimoto wrote: > > > > > > Please use one of "FooWithoutPathExpansion" or "Foo" variants > > exclusively. > > > > > > ("WithoutPathExpansion" is better) > > > > > > > > > > Currently I'm using WithoutPathExpansion only when the key can contain a > > > dot. > > > > > Using it everywhere would make the code bloated. > > > > The problem is that nothing can prevent you and other developers from > using > > a > > > > wrong variant when adding a new pref member. > > > > I don't think we should make the product code bug-prone just to save a > > handful > > > > of key strokes. > > > > Please use WithoutPathExpansion variants everywhere. > > > > > > Do you want me to use WithoutPathExpansion variants in the entire File > System > > > Provider API code for every base::DictionaryValue? > > > > > > There are two variants of methods, one with expansion and one without. I > don't > > > see reason why not using both of them depending on the situation. > > > > What Set/Get does is Set/GetWithoutPathExpansion + path expansion. > > When you don't need the path expansion functionality, you should use the > > WithoutPathExpansion variant. > > According to code search's xrefs: > DictionaryValue::SetStringWithoutPathExpansion() 176 calls > DictionaryValue::SetString() 2852 calls So what? Generally speaking, WithoutPathExpansion() variants should be used when the path expansion behavior is not desired. Here, the path expansion behavior is more dangerous than usual because you are dealing with both types of strings which may contain dots and which may not. Following one simple rule to use WithoutPathExpansion() variants exclusively is much safer than trusting you and future developers who touch this file. > > But since you prefer the longer version, fixed. Done. https://codereview.chromium.org/334263017/diff/240001/chrome/browser/chromeos... File chrome/browser/chromeos/file_system_provider/service.cc (right): https://codereview.chromium.org/334263017/diff/240001/chrome/browser/chromeos... chrome/browser/chromeos/file_system_provider/service.cc:419: // In case of remounting during startup, forget the file system from What does this comment mean? https://codereview.chromium.org/334263017/diff/240001/chrome/browser/chromeos... chrome/browser/chromeos/file_system_provider/service.cc:421: ForgetFileSystem(extension_id, file_system_id); Is it OK to forget the file system which is actually still mounted?
https://codereview.chromium.org/334263017/diff/120001/chrome/browser/chromeos... File chrome/browser/chromeos/file_system_provider/service.cc (right): https://codereview.chromium.org/334263017/diff/120001/chrome/browser/chromeos... chrome/browser/chromeos/file_system_provider/service.cc:335: file_system->SetString(kPrefKeyFileSystemId, On 2014/07/09 08:36:39, hashimoto wrote: > On 2014/07/08 01:52:08, mtomasz wrote: > > On 2014/07/07 11:07:46, hashimoto wrote: > > > On 2014/07/04 07:50:42, mtomasz wrote: > > > > On 2014/07/04 06:38:41, hashimoto wrote: > > > > > On 2014/07/03 05:24:07, mtomasz wrote: > > > > > > On 2014/07/02 08:59:37, hashimoto wrote: > > > > > > > Please use one of "FooWithoutPathExpansion" or "Foo" variants > > > exclusively. > > > > > > > ("WithoutPathExpansion" is better) > > > > > > > > > > > > Currently I'm using WithoutPathExpansion only when the key can contain > a > > > > dot. > > > > > > Using it everywhere would make the code bloated. > > > > > The problem is that nothing can prevent you and other developers from > > using > > > a > > > > > wrong variant when adding a new pref member. > > > > > I don't think we should make the product code bug-prone just to save a > > > handful > > > > > of key strokes. > > > > > Please use WithoutPathExpansion variants everywhere. > > > > > > > > Do you want me to use WithoutPathExpansion variants in the entire File > > System > > > > Provider API code for every base::DictionaryValue? > > > > > > > > There are two variants of methods, one with expansion and one without. I > > don't > > > > see reason why not using both of them depending on the situation. > > > > > > What Set/Get does is Set/GetWithoutPathExpansion + path expansion. > > > When you don't need the path expansion functionality, you should use the > > > WithoutPathExpansion variant. > > > > According to code search's xrefs: > > DictionaryValue::SetStringWithoutPathExpansion() 176 calls > > DictionaryValue::SetString() 2852 calls > So what? > Generally speaking, WithoutPathExpansion() variants should be used when the path > expansion behavior is not desired. > Here, the path expansion behavior is more dangerous than usual because you are > dealing with both types of strings which may contain dots and which may not. > Following one simple rule to use WithoutPathExpansion() variants exclusively is > much safer than trusting you and future developers who touch this file. > > > > But since you prefer the longer version, fixed. Done. > DictionaryValue::SetString() sounds like a default method for setting strings, while DictionaryValue::SetStringWithoutPathExpansion() sounds like a version for special cases (when path expansion is *not allowed* rather than *not needed*). Hence, I was using SetStringWithoutPathExpansion() for that special cases, where dot may happen. I'm not saying I disagree with you. I just don't think it's such a big deal. As I showed in stats, seems that almost everyone use Set() unless a dot may appear there. Done. https://codereview.chromium.org/334263017/diff/240001/chrome/browser/chromeos... File chrome/browser/chromeos/file_system_provider/service.cc (right): https://codereview.chromium.org/334263017/diff/240001/chrome/browser/chromeos... chrome/browser/chromeos/file_system_provider/service.cc:419: // In case of remounting during startup, forget the file system from On 2014/07/09 08:36:39, hashimoto wrote: > What does this comment mean? RestoreFileSystems() is called during startup. If remounting fails, then we want to remove the file system from preferences. https://codereview.chromium.org/334263017/diff/240001/chrome/browser/chromeos... chrome/browser/chromeos/file_system_provider/service.cc:421: ForgetFileSystem(extension_id, file_system_id); On 2014/07/09 08:36:39, hashimoto wrote: > Is it OK to forget the file system which is actually still mounted? This case can't happen. RestoreFileSystems() is called as soon as the extension is enabled, so there is no way there are some file systems mounted by it already. It is also impossible to have two same file systems in preferences, since they are keyed in preferences by file system id. So, mounting in #414 can return false only if mounting failed because either file system id/name are incorrect, or if there are too many file systems mounted already, or it failed to register the mount point. We need to forget the file system from preferences to avoid keeping it in preferences forever, and failing every reboot.
lgtm https://codereview.chromium.org/334263017/diff/120001/chrome/browser/chromeos... File chrome/browser/chromeos/file_system_provider/service.cc (right): https://codereview.chromium.org/334263017/diff/120001/chrome/browser/chromeos... chrome/browser/chromeos/file_system_provider/service.cc:335: file_system->SetString(kPrefKeyFileSystemId, On 2014/07/09 09:18:17, mtomasz wrote: > On 2014/07/09 08:36:39, hashimoto wrote: > > On 2014/07/08 01:52:08, mtomasz wrote: > > > On 2014/07/07 11:07:46, hashimoto wrote: > > > > On 2014/07/04 07:50:42, mtomasz wrote: > > > > > On 2014/07/04 06:38:41, hashimoto wrote: > > > > > > On 2014/07/03 05:24:07, mtomasz wrote: > > > > > > > On 2014/07/02 08:59:37, hashimoto wrote: > > > > > > > > Please use one of "FooWithoutPathExpansion" or "Foo" variants > > > > exclusively. > > > > > > > > ("WithoutPathExpansion" is better) > > > > > > > > > > > > > > Currently I'm using WithoutPathExpansion only when the key can > contain > > a > > > > > dot. > > > > > > > Using it everywhere would make the code bloated. > > > > > > The problem is that nothing can prevent you and other developers from > > > using > > > > a > > > > > > wrong variant when adding a new pref member. > > > > > > I don't think we should make the product code bug-prone just to save a > > > > handful > > > > > > of key strokes. > > > > > > Please use WithoutPathExpansion variants everywhere. > > > > > > > > > > Do you want me to use WithoutPathExpansion variants in the entire File > > > System > > > > > Provider API code for every base::DictionaryValue? > > > > > > > > > > There are two variants of methods, one with expansion and one without. I > > > don't > > > > > see reason why not using both of them depending on the situation. > > > > > > > > What Set/Get does is Set/GetWithoutPathExpansion + path expansion. > > > > When you don't need the path expansion functionality, you should use the > > > > WithoutPathExpansion variant. > > > > > > According to code search's xrefs: > > > DictionaryValue::SetStringWithoutPathExpansion() 176 calls > > > DictionaryValue::SetString() 2852 calls > > So what? > > Generally speaking, WithoutPathExpansion() variants should be used when the > path > > expansion behavior is not desired. > > Here, the path expansion behavior is more dangerous than usual because you are > > dealing with both types of strings which may contain dots and which may not. > > Following one simple rule to use WithoutPathExpansion() variants exclusively > is > > much safer than trusting you and future developers who touch this file. > > > > > > But since you prefer the longer version, fixed. Done. > > > > DictionaryValue::SetString() sounds like a default method for setting strings, > while DictionaryValue::SetStringWithoutPathExpansion() sounds like a version for > special cases (when path expansion is *not allowed* rather than *not needed*). > > Hence, I was using SetStringWithoutPathExpansion() for that special cases, where > dot may happen. > > I'm not saying I disagree with you. I just don't think it's such a big deal. As > I showed in stats, seems that almost everyone use Set() unless a dot may appear > there. > > Done. Set() has a shorter name and is popular only because SetWithoutPathExpansion() was not there until recently (http://crrev.com/156716). There is no reason to avoid being fool proof when writing new code.
https://codereview.chromium.org/334263017/diff/120001/chrome/browser/chromeos... File chrome/browser/chromeos/file_system_provider/service.cc (right): https://codereview.chromium.org/334263017/diff/120001/chrome/browser/chromeos... chrome/browser/chromeos/file_system_provider/service.cc:335: file_system->SetString(kPrefKeyFileSystemId, On 2014/07/11 06:09:05, hashimoto wrote: > On 2014/07/09 09:18:17, mtomasz wrote: > > On 2014/07/09 08:36:39, hashimoto wrote: > > > On 2014/07/08 01:52:08, mtomasz wrote: > > > > On 2014/07/07 11:07:46, hashimoto wrote: > > > > > On 2014/07/04 07:50:42, mtomasz wrote: > > > > > > On 2014/07/04 06:38:41, hashimoto wrote: > > > > > > > On 2014/07/03 05:24:07, mtomasz wrote: > > > > > > > > On 2014/07/02 08:59:37, hashimoto wrote: > > > > > > > > > Please use one of "FooWithoutPathExpansion" or "Foo" variants > > > > > exclusively. > > > > > > > > > ("WithoutPathExpansion" is better) > > > > > > > > > > > > > > > > Currently I'm using WithoutPathExpansion only when the key can > > contain > > > a > > > > > > dot. > > > > > > > > Using it everywhere would make the code bloated. > > > > > > > The problem is that nothing can prevent you and other developers > from > > > > using > > > > > a > > > > > > > wrong variant when adding a new pref member. > > > > > > > I don't think we should make the product code bug-prone just to save > a > > > > > handful > > > > > > > of key strokes. > > > > > > > Please use WithoutPathExpansion variants everywhere. > > > > > > > > > > > > Do you want me to use WithoutPathExpansion variants in the entire File > > > > System > > > > > > Provider API code for every base::DictionaryValue? > > > > > > > > > > > > There are two variants of methods, one with expansion and one without. > I > > > > don't > > > > > > see reason why not using both of them depending on the situation. > > > > > > > > > > What Set/Get does is Set/GetWithoutPathExpansion + path expansion. > > > > > When you don't need the path expansion functionality, you should use the > > > > > WithoutPathExpansion variant. > > > > > > > > According to code search's xrefs: > > > > DictionaryValue::SetStringWithoutPathExpansion() 176 calls > > > > DictionaryValue::SetString() 2852 calls > > > So what? > > > Generally speaking, WithoutPathExpansion() variants should be used when the > > path > > > expansion behavior is not desired. > > > Here, the path expansion behavior is more dangerous than usual because you > are > > > dealing with both types of strings which may contain dots and which may not. > > > Following one simple rule to use WithoutPathExpansion() variants exclusively > > is > > > much safer than trusting you and future developers who touch this file. > > > > > > > > But since you prefer the longer version, fixed. Done. > > > > > > > DictionaryValue::SetString() sounds like a default method for setting strings, > > while DictionaryValue::SetStringWithoutPathExpansion() sounds like a version > for > > special cases (when path expansion is *not allowed* rather than *not needed*). > > > > Hence, I was using SetStringWithoutPathExpansion() for that special cases, > where > > dot may happen. > > > > I'm not saying I disagree with you. I just don't think it's such a big deal. > As > > I showed in stats, seems that almost everyone use Set() unless a dot may > appear > > there. > > > > Done. > > Set() has a shorter name and is popular only because SetWithoutPathExpansion() > was not there until recently (http://crrev.com/156716). > There is no reason to avoid being fool proof when writing new code. Got it. That makes sense.
The CQ bit was checked by mtomasz@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/mtomasz@chromium.org/334263017/260001
Message was sent while issue was closed.
Change committed as 282589 |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
