Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
14556015:
[Media Galleries] Lazily initialize the storage monitor. (Closed)
|
Created:
7 years, 7 months ago by Greg Billock Modified:
7 years, 6 months ago CC:
chromium-reviews, oshima+watch_chromium.org, stevenjb+watch_chromium.org, davemoore+watch_chromium.org, sail+watch_chromium.org, nkostylev+watch_chromium.org Base URL:
http://git.chromium.org/chromium/src.git@master Visibility:
Public. |
Description[Media Galleries] Lazily initialize the storage monitor.
The storage monitor is no longer initialized upon startup. Instead
the registry and dialog controller which need it will initialize it
on first use.
BUG=234409
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=203801
Patch Set 1 #Patch Set 2 : Fix-ups for tests #Patch Set 3 : Windows compile tweaks #
Total comments: 14
Patch Set 4 : Privatize #
Total comments: 12
Patch Set 5 : Rebase #Patch Set 6 : Get rid of GetPreferences, update callers to async method #Patch Set 7 : Weak ptr for registry #
Total comments: 14
Patch Set 8 : path check #
Total comments: 3
Patch Set 9 : Test repair #
Total comments: 9
Patch Set 10 : Don't reset prefs in tracker #Patch Set 11 : Rebase #
Total comments: 27
Patch Set 12 : Improve tracker code #
Total comments: 10
Patch Set 13 : Change signature, add RVH transaction obj #Patch Set 14 : Touch-ups #Patch Set 15 : Rebase #Patch Set 16 : Mark override #Patch Set 17 : Switch approaches #
Total comments: 14
Patch Set 18 : Get everything moved over #Patch Set 19 : Adjustments #
Total comments: 22
Patch Set 20 : Rebase on prefs #
Total comments: 14
Patch Set 21 : Rebase #Patch Set 22 : Rebase onto master post-dep-commit #Patch Set 23 : Move storage monitor init into the API call layer #Patch Set 24 : Fix some comments ++ #Patch Set 25 : Add StorageMonitor comment #
Total comments: 29
Patch Set 26 : Harden async boundary in WebUI #
Total comments: 6
Patch Set 27 : Use thread checker #Patch Set 28 : Rebase to head #Patch Set 29 : Fix merge #
Total comments: 2
Messages
Total messages: 50 (0 generated)
This is ready for a look. I don't think I'll submit until after the branch, but have a look and let me know if you think the approach is right. Also if I've forgotten any call sites or ignored edge cases let me know. I also need to write a test that the registry correctly dances with the callback -- the current test kind of just makes it all OK.
I don't see a place where you call StorageMonitor::Init().
On 2013/05/09 02:42:35, Lei Zhang wrote: > I don't see a place where you call StorageMonitor::Init(). registry line 503
Can we move the storage monitor code out of chrome_browser_main* altogether? https://codereview.chromium.org/14556015/diff/15002/chrome/browser/media_gall... File chrome/browser/media_galleries/media_galleries_preferences.cc (right): https://codereview.chromium.org/14556015/diff/15002/chrome/browser/media_gall... chrome/browser/media_galleries/media_galleries_preferences.cc:405: LOG(INFO) << "Add gallery intenral " << device_id; remove debug statement here and elsewhere.
https://codereview.chromium.org/14556015/diff/15002/chrome/browser/media_gall... File chrome/browser/media_galleries/media_file_system_registry.cc (right): https://codereview.chromium.org/14556015/diff/15002/chrome/browser/media_gall... chrome/browser/media_galleries/media_file_system_registry.cc:493: if (!monitor) { Do we want to just make tests that use MediaFSRegistry have a storage monitor, instead of writing handlers for things that should never happen in production? https://codereview.chromium.org/14556015/diff/15002/chrome/browser/media_gall... File chrome/browser/media_galleries/media_file_system_registry.h (right): https://codereview.chromium.org/14556015/diff/15002/chrome/browser/media_gall... chrome/browser/media_galleries/media_file_system_registry.h:84: // Bottom half of |GetMediaFileSystemsForExtensions|, called after we nit: drop the we. https://codereview.chromium.org/14556015/diff/15002/chrome/browser/media_gall... chrome/browser/media_galleries/media_file_system_registry.h:86: void GetMediaFileSystemsPostStorageMonitorInit( Also private. https://codereview.chromium.org/14556015/diff/15002/chrome/browser/media_gall... chrome/browser/media_galleries/media_file_system_registry.h:100: void OnStorageMonitorInitialized( also private. https://codereview.chromium.org/14556015/diff/15002/chrome/browser/media_gall... File chrome/browser/media_galleries/media_galleries_dialog_controller.h (right): https://codereview.chromium.org/14556015/diff/15002/chrome/browser/media_gall... chrome/browser/media_galleries/media_galleries_dialog_controller.h:77: void Init(MediaGalleriesPreferences* preferences); This can be private since nobody outside references it.
https://codereview.chromium.org/14556015/diff/15002/chrome/browser/media_gall... File chrome/browser/media_galleries/media_file_system_registry.cc (right): https://codereview.chromium.org/14556015/diff/15002/chrome/browser/media_gall... chrome/browser/media_galleries/media_file_system_registry.cc:493: if (!monitor) { On 2013/05/10 05:24:43, Lei Zhang wrote: > Do we want to just make tests that use MediaFSRegistry have a storage monitor, > instead of writing handlers for things that should never happen in production? Yes, I think that'd be better. I kind of suspect this may even be obsolete, and all our tests really do have a storage monitor already. :-/ Let's do that separately though. https://codereview.chromium.org/14556015/diff/15002/chrome/browser/media_gall... File chrome/browser/media_galleries/media_file_system_registry.h (right): https://codereview.chromium.org/14556015/diff/15002/chrome/browser/media_gall... chrome/browser/media_galleries/media_file_system_registry.h:84: // Bottom half of |GetMediaFileSystemsForExtensions|, called after we On 2013/05/10 05:24:43, Lei Zhang wrote: > nit: drop the we. Done. https://codereview.chromium.org/14556015/diff/15002/chrome/browser/media_gall... chrome/browser/media_galleries/media_file_system_registry.h:86: void GetMediaFileSystemsPostStorageMonitorInit( On 2013/05/10 05:24:43, Lei Zhang wrote: > Also private. Done. https://codereview.chromium.org/14556015/diff/15002/chrome/browser/media_gall... chrome/browser/media_galleries/media_file_system_registry.h:100: void OnStorageMonitorInitialized( On 2013/05/10 05:24:43, Lei Zhang wrote: > also private. Done. https://codereview.chromium.org/14556015/diff/15002/chrome/browser/media_gall... File chrome/browser/media_galleries/media_galleries_dialog_controller.h (right): https://codereview.chromium.org/14556015/diff/15002/chrome/browser/media_gall... chrome/browser/media_galleries/media_galleries_dialog_controller.h:77: void Init(MediaGalleriesPreferences* preferences); On 2013/05/10 05:24:43, Lei Zhang wrote: > This can be private since nobody outside references it. Done. https://codereview.chromium.org/14556015/diff/15002/chrome/browser/media_gall... File chrome/browser/media_galleries/media_galleries_preferences.cc (right): https://codereview.chromium.org/14556015/diff/15002/chrome/browser/media_gall... chrome/browser/media_galleries/media_galleries_preferences.cc:405: LOG(INFO) << "Add gallery intenral " << device_id; On 2013/05/10 02:40:46, Lei Zhang wrote: > remove debug statement here and elsewhere. Done.
On 2013/05/10 02:40:45, Lei Zhang wrote: > Can we move the storage monitor code out of chrome_browser_main* altogether? Yes, I think we want to do this (put it in BrowserProcess). Then we can move towards eliminating the global altogether, which will be really nice. Let's do that separately though. > > https://codereview.chromium.org/14556015/diff/15002/chrome/browser/media_gall... > File chrome/browser/media_galleries/media_galleries_preferences.cc (right): > > https://codereview.chromium.org/14556015/diff/15002/chrome/browser/media_gall... > chrome/browser/media_galleries/media_galleries_preferences.cc:405: LOG(INFO) << > "Add gallery intenral " << device_id; > remove debug statement here and elsewhere.
lgtm https://codereview.chromium.org/14556015/diff/41001/chrome/browser/media_gall... File chrome/browser/media_galleries/media_file_system_registry.cc (right): https://codereview.chromium.org/14556015/diff/41001/chrome/browser/media_gall... chrome/browser/media_galleries/media_file_system_registry.cc:489: StorageMonitor* monitor = StorageMonitor::GetInstance(); I see that StorageMonitor::GetInstance() is called in many other places, but this is the only place Init is called. Does this mean that this piece of code is guaranteed to run before all other calls to GetInstance? If so, I think GetInstance should be DCHECKed to make sure it's initialized. The first initialization here could be done with a static function.
https://codereview.chromium.org/14556015/diff/41001/chrome/browser/media_gall... File chrome/browser/media_galleries/media_file_system_registry.cc (right): https://codereview.chromium.org/14556015/diff/41001/chrome/browser/media_gall... chrome/browser/media_galleries/media_file_system_registry.cc:489: StorageMonitor* monitor = StorageMonitor::GetInstance(); On 2013/05/10 17:41:24, tommycli wrote: > I see that StorageMonitor::GetInstance() is called in many other places, but > this is the only place Init is called. Yes. I have a long-term goal to reduce/eliminate that global, but that'll take more time. > Does this mean that this piece of code is guaranteed to run before all other > calls to GetInstance? If so, I think GetInstance should be DCHECKed to make sure > it's initialized. Yes, that's the expectation. It's kind of risky, so please let me know if there are gaps. I'm not planning to check this in until I have more DCHECKs in place to validate this assumption. > The first initialization here could be done with a static function. It's legal to do some things (register for notifications, for instance), but not others. So that's a problem we don't have a great solution for. :-/
On 2013/05/10 17:58:26, Greg Billock wrote: > https://codereview.chromium.org/14556015/diff/41001/chrome/browser/media_gall... > File chrome/browser/media_galleries/media_file_system_registry.cc (right): > > https://codereview.chromium.org/14556015/diff/41001/chrome/browser/media_gall... > chrome/browser/media_galleries/media_file_system_registry.cc:489: > StorageMonitor* monitor = StorageMonitor::GetInstance(); > On 2013/05/10 17:41:24, tommycli wrote: > > I see that StorageMonitor::GetInstance() is called in many other places, but > > this is the only place Init is called. > > Yes. I have a long-term goal to reduce/eliminate that global, but that'll take > more time. > > > > Does this mean that this piece of code is guaranteed to run before all other > > calls to GetInstance? If so, I think GetInstance should be DCHECKed to make > sure > > it's initialized. > > Yes, that's the expectation. It's kind of risky, so please let me know if there > are gaps. I'm not planning to check this in until I have more DCHECKs in place > to validate this assumption. > > > The first initialization here could be done with a static function. > > It's legal to do some things (register for notifications, for instance), but not > others. So that's a problem we don't have a great solution for. :-/ Haha that is an unenviable situation.
https://codereview.chromium.org/14556015/diff/41001/chrome/browser/media_gall... File chrome/browser/media_galleries/media_file_system_registry.h (right): https://codereview.chromium.org/14556015/diff/41001/chrome/browser/media_gall... chrome/browser/media_galleries/media_file_system_registry.h:88: // Called on the UI thread. Do not call from outside this class. If it should only be called from within this class, it can be private, right?
https://codereview.chromium.org/14556015/diff/15002/chrome/browser/media_gall... File chrome/browser/media_galleries/media_file_system_registry.cc (right): https://codereview.chromium.org/14556015/diff/15002/chrome/browser/media_gall... chrome/browser/media_galleries/media_file_system_registry.cc:493: if (!monitor) { On 2013/05/10 16:20:27, Greg Billock wrote: > On 2013/05/10 05:24:43, Lei Zhang wrote: > > Do we want to just make tests that use MediaFSRegistry have a storage monitor, > > instead of writing handlers for things that should never happen in production? > > Yes, I think that'd be better. I kind of suspect this may even be obsolete, and > all our tests really do have a storage monitor already. :-/ Let's do that > separately though. Can you add a TODO so we don't forget? https://codereview.chromium.org/14556015/diff/41001/chrome/browser/media_gall... File chrome/browser/media_galleries/media_file_system_registry.cc (right): https://codereview.chromium.org/14556015/diff/41001/chrome/browser/media_gall... chrome/browser/media_galleries/media_file_system_registry.cc:502: base::Unretained(this), I would probably feel better about this Unretained() if we can guarantee MediaFileSystemRegistry outlives StorageMonitor. I feel StorageMonitor is a more low level object, so there's no guarantee MediaFileSystemRegistry outlives StorageMonitor. https://codereview.chromium.org/14556015/diff/41001/chrome/browser/media_gall... File chrome/browser/media_galleries/media_file_system_registry.h (right): https://codereview.chromium.org/14556015/diff/41001/chrome/browser/media_gall... chrome/browser/media_galleries/media_file_system_registry.h:91: // Call the given callback with preferences. This is asynchronous so that If GetPreferences() is going to be private, you may want to retain some of its comments here.
https://codereview.chromium.org/14556015/diff/15002/chrome/browser/media_gall... File chrome/browser/media_galleries/media_file_system_registry.cc (right): https://codereview.chromium.org/14556015/diff/15002/chrome/browser/media_gall... chrome/browser/media_galleries/media_file_system_registry.cc:493: if (!monitor) { On 2013/05/11 02:12:22, Lei Zhang wrote: > On 2013/05/10 16:20:27, Greg Billock wrote: > > On 2013/05/10 05:24:43, Lei Zhang wrote: > > > Do we want to just make tests that use MediaFSRegistry have a storage > monitor, > > > instead of writing handlers for things that should never happen in > production? > > > > Yes, I think that'd be better. I kind of suspect this may even be obsolete, > and > > all our tests really do have a storage monitor already. :-/ Let's do that > > separately though. > > Can you add a TODO so we don't forget? Done. https://codereview.chromium.org/14556015/diff/41001/chrome/browser/media_gall... File chrome/browser/media_galleries/media_file_system_registry.cc (right): https://codereview.chromium.org/14556015/diff/41001/chrome/browser/media_gall... chrome/browser/media_galleries/media_file_system_registry.cc:502: base::Unretained(this), On 2013/05/11 02:12:23, Lei Zhang wrote: > I would probably feel better about this Unretained() if we can guarantee > MediaFileSystemRegistry outlives StorageMonitor. I feel StorageMonitor is a more > low level object, so there's no guarantee MediaFileSystemRegistry outlives > StorageMonitor. Shall we add a weak ptr factory? https://codereview.chromium.org/14556015/diff/41001/chrome/browser/media_gall... File chrome/browser/media_galleries/media_file_system_registry.h (right): https://codereview.chromium.org/14556015/diff/41001/chrome/browser/media_gall... chrome/browser/media_galleries/media_file_system_registry.h:88: // Called on the UI thread. Do not call from outside this class. On 2013/05/10 21:20:39, vandebo wrote: > If it should only be called from within this class, it can be private, right? It should be. I was going to wait, but I'll just try and move it in this CL... https://codereview.chromium.org/14556015/diff/41001/chrome/browser/media_gall... chrome/browser/media_galleries/media_file_system_registry.h:91: // Call the given callback with preferences. This is asynchronous so that On 2013/05/11 02:12:23, Lei Zhang wrote: > If GetPreferences() is going to be private, you may want to retain some of its > comments here. Done.
https://codereview.chromium.org/14556015/diff/41001/chrome/browser/media_gall... File chrome/browser/media_galleries/media_file_system_registry_unittest.cc (right): https://codereview.chromium.org/14556015/diff/41001/chrome/browser/media_gall... chrome/browser/media_galleries/media_file_system_registry_unittest.cc:871: GetMediaFileSystemRegistry()->GetPreferences(profile_state->profile()); Any chance we convert this test? The changes here defeats the purpose of this regression test. https://codereview.chromium.org/14556015/diff/56001/chrome/browser/extensions... File chrome/browser/extensions/api/media_galleries_private/gallery_watch_state_tracker.cc (right): https://codereview.chromium.org/14556015/diff/56001/chrome/browser/extensions... chrome/browser/extensions/api/media_galleries_private/gallery_watch_state_tracker.cc:259: SetPreferences(preferences); Do we need both this and the SetPreferences() call from media_galleries_private_api.cc? https://codereview.chromium.org/14556015/diff/56001/chrome/browser/extensions... File chrome/browser/extensions/api/media_galleries_private/media_galleries_private_api.cc (right): https://codereview.chromium.org/14556015/diff/56001/chrome/browser/extensions... chrome/browser/extensions/api/media_galleries_private/media_galleries_private_api.cc:285: chrome::MediaGalleryPrefId pref_id = 0; Use kInvalidMediaGalleryPrefId instead of 0? https://codereview.chromium.org/14556015/diff/56001/chrome/browser/extensions... chrome/browser/extensions/api/media_galleries_private/media_galleries_private_api.cc:296: base::Unretained(this), pref_id)); MediaGalleriesPrivateAddGalleryWatchFunction is a AsyncExtensionFunction, which is a UIThreadExtensionFunction, which is a ExtensionFunction, which is RefCountedThreadSafe. Safer without Unretained()? https://codereview.chromium.org/14556015/diff/56001/chrome/browser/extensions... chrome/browser/extensions/api/media_galleries_private/media_galleries_private_api.cc:303: base::FilePath file_path(preferences->LookUpGalleryPathForExtension( Handle the case of an empty |file_path| like before this change? https://codereview.chromium.org/14556015/diff/56001/chrome/browser/extensions... chrome/browser/extensions/api/media_galleries_private/media_galleries_private_api.cc:389: base::FilePath file_path(preferences->LookUpGalleryPathForExtension( ditto https://codereview.chromium.org/14556015/diff/56001/chrome/browser/extensions... File chrome/browser/extensions/api/media_galleries_private/media_galleries_private_api.h (right): https://codereview.chromium.org/14556015/diff/56001/chrome/browser/extensions... chrome/browser/extensions/api/media_galleries_private/media_galleries_private_api.h:155: void OnPreferences( private? https://codereview.chromium.org/14556015/diff/56001/chrome/browser/ui/webui/o... File chrome/browser/ui/webui/options/media_galleries_handler.h (right): https://codereview.chromium.org/14556015/diff/56001/chrome/browser/ui/webui/o... chrome/browser/ui/webui/options/media_galleries_handler.h:44: void HandleForgetGalleryWithPreferences( Can you comment FooWithPreferences() as the second half of Foo?
https://codereview.chromium.org/14556015/diff/41001/chrome/browser/media_gall... File chrome/browser/media_galleries/media_file_system_registry_unittest.cc (right): https://codereview.chromium.org/14556015/diff/41001/chrome/browser/media_gall... chrome/browser/media_galleries/media_file_system_registry_unittest.cc:871: GetMediaFileSystemRegistry()->GetPreferences(profile_state->profile()); On 2013/05/14 04:12:22, Lei Zhang wrote: > Any chance we convert this test? The changes here defeats the purpose of this > regression test. Yeah, I noticed this while converting these classes. Please take a look at the patch now -- I'm not sure the conversion worked totally. https://codereview.chromium.org/14556015/diff/56001/chrome/browser/extensions... File chrome/browser/extensions/api/media_galleries_private/gallery_watch_state_tracker.cc (right): https://codereview.chromium.org/14556015/diff/56001/chrome/browser/extensions... chrome/browser/extensions/api/media_galleries_private/gallery_watch_state_tracker.cc:259: SetPreferences(preferences); On 2013/05/14 04:12:23, Lei Zhang wrote: > Do we need both this and the SetPreferences() call from > media_galleries_private_api.cc? I think they get touched in different paths. So there's redundancy, but I think we need it. https://codereview.chromium.org/14556015/diff/56001/chrome/browser/extensions... File chrome/browser/extensions/api/media_galleries_private/media_galleries_private_api.cc (right): https://codereview.chromium.org/14556015/diff/56001/chrome/browser/extensions... chrome/browser/extensions/api/media_galleries_private/media_galleries_private_api.cc:285: chrome::MediaGalleryPrefId pref_id = 0; On 2013/05/14 04:12:23, Lei Zhang wrote: > Use kInvalidMediaGalleryPrefId instead of 0? Done. https://codereview.chromium.org/14556015/diff/56001/chrome/browser/extensions... chrome/browser/extensions/api/media_galleries_private/media_galleries_private_api.cc:296: base::Unretained(this), pref_id)); On 2013/05/14 04:12:23, Lei Zhang wrote: > MediaGalleriesPrivateAddGalleryWatchFunction is a AsyncExtensionFunction, which > is a UIThreadExtensionFunction, which is a ExtensionFunction, which is > RefCountedThreadSafe. Safer without Unretained()? Good point. Done. https://codereview.chromium.org/14556015/diff/56001/chrome/browser/extensions... chrome/browser/extensions/api/media_galleries_private/media_galleries_private_api.cc:303: base::FilePath file_path(preferences->LookUpGalleryPathForExtension( On 2013/05/14 04:12:23, Lei Zhang wrote: > Handle the case of an empty |file_path| like before this change? Oops! I thought I had maintained that, but it didn't make it. https://codereview.chromium.org/14556015/diff/56001/chrome/browser/extensions... chrome/browser/extensions/api/media_galleries_private/media_galleries_private_api.cc:389: base::FilePath file_path(preferences->LookUpGalleryPathForExtension( On 2013/05/14 04:12:23, Lei Zhang wrote: > ditto Done. https://codereview.chromium.org/14556015/diff/56001/chrome/browser/extensions... File chrome/browser/extensions/api/media_galleries_private/media_galleries_private_api.h (right): https://codereview.chromium.org/14556015/diff/56001/chrome/browser/extensions... chrome/browser/extensions/api/media_galleries_private/media_galleries_private_api.h:155: void OnPreferences( On 2013/05/14 04:12:23, Lei Zhang wrote: > private? Done. https://codereview.chromium.org/14556015/diff/56001/chrome/browser/ui/webui/o... File chrome/browser/ui/webui/options/media_galleries_handler.h (right): https://codereview.chromium.org/14556015/diff/56001/chrome/browser/ui/webui/o... chrome/browser/ui/webui/options/media_galleries_handler.h:44: void HandleForgetGalleryWithPreferences( On 2013/05/14 04:12:23, Lei Zhang wrote: > Can you comment FooWithPreferences() as the second half of Foo? Done.
https://codereview.chromium.org/14556015/diff/66001/chrome/browser/extensions... File chrome/browser/extensions/api/media_galleries_private/gallery_watch_state_tracker.cc (right): https://codereview.chromium.org/14556015/diff/66001/chrome/browser/extensions... chrome/browser/extensions/api/media_galleries_private/gallery_watch_state_tracker.cc:79: preferences_(NULL) { The ReadFromStorage() code will never be called if we don't do the notification registration right below here. How about just making one GetPreferencesAsync() call here, have registering for notifications as the callback, save the returned value as |preferences_|, and get rid of SetPreferences? https://codereview.chromium.org/14556015/diff/66001/chrome/browser/media_gall... File chrome/browser/media_galleries/media_file_system_registry_unittest.cc (right): https://codereview.chromium.org/14556015/diff/66001/chrome/browser/media_gall... chrome/browser/media_galleries/media_file_system_registry_unittest.cc:735: DCHECK(wait_for_init.IsSignaled()); EXPECT_EQ in tests?
https://codereview.chromium.org/14556015/diff/41001/chrome/browser/media_gall... File chrome/browser/media_galleries/media_file_system_registry_unittest.cc (right): https://codereview.chromium.org/14556015/diff/41001/chrome/browser/media_gall... chrome/browser/media_galleries/media_file_system_registry_unittest.cc:871: GetMediaFileSystemRegistry()->GetPreferences(profile_state->profile()); On 2013/05/14 21:27:13, Greg Billock wrote: > On 2013/05/14 04:12:22, Lei Zhang wrote: > > Any chance we convert this test? The changes here defeats the purpose of this > > regression test. > > Yeah, I noticed this while converting these classes. Please take a look at the > patch now -- I'm not sure the conversion worked totally. You gotta keep the GetPreferences() call for this test to exercise the code it's meant to exercise. So we got to do GetPreferences() -> GetPreferencesAsync() here, probably with some WaitableEvent like with the other conversion in this file.
https://codereview.chromium.org/14556015/diff/41001/chrome/browser/media_gall... File chrome/browser/media_galleries/media_file_system_registry_unittest.cc (right): https://codereview.chromium.org/14556015/diff/41001/chrome/browser/media_gall... chrome/browser/media_galleries/media_file_system_registry_unittest.cc:871: GetMediaFileSystemRegistry()->GetPreferences(profile_state->profile()); On 2013/05/14 23:45:24, Lei Zhang wrote: > On 2013/05/14 21:27:13, Greg Billock wrote: > > On 2013/05/14 04:12:22, Lei Zhang wrote: > > > Any chance we convert this test? The changes here defeats the purpose of > this > > > regression test. > > > > Yeah, I noticed this while converting these classes. Please take a look at the > > patch now -- I'm not sure the conversion worked totally. > > You gotta keep the GetPreferences() call for this test to exercise the code it's > meant to exercise. So we got to do GetPreferences() -> GetPreferencesAsync() > here, probably with some WaitableEvent like with the other conversion in this > file. Done.
https://codereview.chromium.org/14556015/diff/66001/chrome/browser/extensions... File chrome/browser/extensions/api/media_galleries_private/gallery_watch_state_tracker.cc (right): https://codereview.chromium.org/14556015/diff/66001/chrome/browser/extensions... chrome/browser/extensions/api/media_galleries_private/gallery_watch_state_tracker.cc:79: preferences_(NULL) { On 2013/05/14 22:32:00, Lei Zhang wrote: > The ReadFromStorage() code will never be called if we don't do the notification > registration right below here. How about just making one GetPreferencesAsync() > call here, have registering for notifications as the callback, save the returned > value as |preferences_|, and get rid of SetPreferences? I'm not sure I understand. That call is being made externally, right? Won't it happen as a result of these notifications? If we call GetPreferencesAsync here, won't it just always load the storage monitor first thing, even if there's no registered watchers?
On 2013/05/15 01:42:56, Greg Billock wrote: > https://codereview.chromium.org/14556015/diff/66001/chrome/browser/extensions... > File > chrome/browser/extensions/api/media_galleries_private/gallery_watch_state_tracker.cc > (right): > > https://codereview.chromium.org/14556015/diff/66001/chrome/browser/extensions... > chrome/browser/extensions/api/media_galleries_private/gallery_watch_state_tracker.cc:79: > preferences_(NULL) { > On 2013/05/14 22:32:00, Lei Zhang wrote: > > The ReadFromStorage() code will never be called if we don't do the > notification > > registration right below here. How about just making one GetPreferencesAsync() > > call here, have registering for notifications as the callback, save the > returned > > value as |preferences_|, and get rid of SetPreferences? > > I'm not sure I understand. That call is being made externally, right? Won't it > happen as a result of these notifications? If we call GetPreferencesAsync here, > won't it just always load the storage monitor first thing, even if there's no > registered watchers? Ya, you are right, the state tracker object gets created pretty early on. Basically I don't like having multiple attempts to set the tracker's |preferences_| pointer. It really should be set once and be done with.
https://codereview.chromium.org/14556015/diff/75001/chrome/browser/extensions... File chrome/browser/extensions/api/media_galleries_private/gallery_watch_state_tracker.cc (right): https://codereview.chromium.org/14556015/diff/75001/chrome/browser/extensions... chrome/browser/extensions/api/media_galleries_private/gallery_watch_state_tracker.cc:158: preferences_ = preferences; Make this a no-op if |preferences_| is already set? https://codereview.chromium.org/14556015/diff/75001/chrome/browser/extensions... chrome/browser/extensions/api/media_galleries_private/gallery_watch_state_tracker.cc:247: if (gallery_ids.size() > 0) { You can use set::empty() instead. https://codereview.chromium.org/14556015/diff/75001/chrome/browser/extensions... chrome/browser/extensions/api/media_galleries_private/gallery_watch_state_tracker.cc:248: g_browser_process->media_file_system_registry()->GetPreferencesAsync( How about we check |preferences_|, and if it is already set, then just call ReadFromStorageWithPreferences() directly? https://codereview.chromium.org/14556015/diff/75001/chrome/browser/media_gall... File chrome/browser/media_galleries/media_file_system_registry_unittest.cc (right): https://codereview.chromium.org/14556015/diff/75001/chrome/browser/media_gall... chrome/browser/media_galleries/media_file_system_registry_unittest.cc:904: prefs = GetPreferences(profile_state->profile()); Can you retain the old comment?
https://codereview.chromium.org/14556015/diff/75001/chrome/browser/extensions... File chrome/browser/extensions/api/media_galleries_private/gallery_watch_state_tracker.cc (right): https://codereview.chromium.org/14556015/diff/75001/chrome/browser/extensions... chrome/browser/extensions/api/media_galleries_private/gallery_watch_state_tracker.cc:158: preferences_ = preferences; On 2013/05/15 22:44:31, Lei Zhang wrote: > Make this a no-op if |preferences_| is already set? Actually, this is important. Will the tracker be used across profiles? If so, we need a different strategy than I have here. If there's one per profile, it'll work OK. The constructor takes a profile, so that makes it seem like the answer is no, it's per-profile. I'll set the code to assume this. https://codereview.chromium.org/14556015/diff/75001/chrome/browser/extensions... chrome/browser/extensions/api/media_galleries_private/gallery_watch_state_tracker.cc:247: if (gallery_ids.size() > 0) { On 2013/05/15 22:44:31, Lei Zhang wrote: > You can use set::empty() instead. Done. https://codereview.chromium.org/14556015/diff/75001/chrome/browser/extensions... chrome/browser/extensions/api/media_galleries_private/gallery_watch_state_tracker.cc:248: g_browser_process->media_file_system_registry()->GetPreferencesAsync( On 2013/05/15 22:44:31, Lei Zhang wrote: > How about we check |preferences_|, and if it is already set, then just call > ReadFromStorageWithPreferences() directly? I'd rather always use the same access path -- this will call the callback in the same stack if it is already set. https://codereview.chromium.org/14556015/diff/75001/chrome/browser/media_gall... File chrome/browser/media_galleries/media_file_system_registry_unittest.cc (right): https://codereview.chromium.org/14556015/diff/75001/chrome/browser/media_gall... chrome/browser/media_galleries/media_file_system_registry_unittest.cc:904: prefs = GetPreferences(profile_state->profile()); On 2013/05/15 22:44:31, Lei Zhang wrote: > Can you retain the old comment? Done.
lgtm https://codereview.chromium.org/14556015/diff/75001/chrome/browser/extensions... File chrome/browser/extensions/api/media_galleries_private/gallery_watch_state_tracker.cc (right): https://codereview.chromium.org/14556015/diff/75001/chrome/browser/extensions... chrome/browser/extensions/api/media_galleries_private/gallery_watch_state_tracker.cc:158: preferences_ = preferences; On 2013/05/16 01:59:58, Greg Billock wrote: > On 2013/05/15 22:44:31, Lei Zhang wrote: > > Make this a no-op if |preferences_| is already set? > > Actually, this is important. Will the tracker be used across profiles? If so, we > need a different strategy than I have here. If there's one per profile, it'll > work OK. The constructor takes a profile, so that makes it seem like the answer > is no, it's per-profile. > > I'll set the code to assume this. The tracker object is only used by the media gallery api object, which is per profile.
https://codereview.chromium.org/14556015/diff/106001/chrome/browser/extension... File chrome/browser/extensions/api/media_galleries_private/gallery_watch_state_tracker.cc (right): https://codereview.chromium.org/14556015/diff/106001/chrome/browser/extension... chrome/browser/extensions/api/media_galleries_private/gallery_watch_state_tracker.cc:280: base::FilePath gallery_file_path(preferences_->LookUpGalleryPathForExtension( Why are we sure preferences will be set by this point? https://codereview.chromium.org/14556015/diff/106001/chrome/browser/extension... chrome/browser/extensions/api/media_galleries_private/gallery_watch_state_tracker.cc:309: base::FilePath gallery_file_path(preferences_->LookUpGalleryPathForExtension( ditto https://codereview.chromium.org/14556015/diff/106001/chrome/browser/extension... File chrome/browser/extensions/api/media_galleries_private/gallery_watch_state_tracker.h (right): https://codereview.chromium.org/14556015/diff/106001/chrome/browser/extension... chrome/browser/extensions/api/media_galleries_private/gallery_watch_state_tracker.h:136: // The preferences object from which we get gallery information. I assume the lifetime of this class is such that we don't have to worry about storing a raw pointer? If so, please add a comment to that effect. https://codereview.chromium.org/14556015/diff/106001/chrome/browser/media_gal... File chrome/browser/media_galleries/media_file_system_registry.cc (right): https://codereview.chromium.org/14556015/diff/106001/chrome/browser/media_gal... chrome/browser/media_galleries/media_file_system_registry.cc:497: if (monitor->IsInitialized()) { You don't need this check - StorageMonitor::Initialize does this internally. https://codereview.chromium.org/14556015/diff/106001/chrome/browser/media_gal... chrome/browser/media_galleries/media_file_system_registry.cc:513: callback.Run(GetPreferences(profile)); I don't think this is safe. If it is, a comment why should be added. What happens if profile goes away why StorageMonitor is initializing. https://codereview.chromium.org/14556015/diff/106001/chrome/browser/media_gal... File chrome/browser/media_galleries/media_file_system_registry_unittest.cc (right): https://codereview.chromium.org/14556015/diff/106001/chrome/browser/media_gal... chrome/browser/media_galleries/media_file_system_registry_unittest.cc:603: void GetPreferencesForwarder(base::WaitableEvent* event, As far as I can tell, you don't need a waitable event in any of the cases you have it in this file (Wait() is never used). https://codereview.chromium.org/14556015/diff/106001/chrome/browser/media_gal... File chrome/browser/media_galleries/media_galleries_dialog_controller.h (right): https://codereview.chromium.org/14556015/diff/106001/chrome/browser/media_gal... chrome/browser/media_galleries/media_galleries_dialog_controller.h:130: void Init(MediaGalleriesPreferences* preferences); Should this be called OnPreferences() ? https://codereview.chromium.org/14556015/diff/106001/chrome/browser/ui/webui/... File chrome/browser/ui/webui/options/media_galleries_handler.cc (right): https://codereview.chromium.org/14556015/diff/106001/chrome/browser/ui/webui/... chrome/browser/ui/webui/options/media_galleries_handler.cc:104: web_ui()->CallJavascriptFunction( Is it ok to do this asynchronously ?
https://codereview.chromium.org/14556015/diff/106001/chrome/browser/extension... File chrome/browser/extensions/api/media_galleries_private/gallery_watch_state_tracker.cc (right): https://codereview.chromium.org/14556015/diff/106001/chrome/browser/extension... chrome/browser/extensions/api/media_galleries_private/gallery_watch_state_tracker.cc:280: base::FilePath gallery_file_path(preferences_->LookUpGalleryPathForExtension( On 2013/05/16 18:56:27, vandebo wrote: > Why are we sure preferences will be set by this point? I looked at the caller chain and someone must have called SetPreferences() before getting here. Let me look that up... https://codereview.chromium.org/14556015/diff/106001/chrome/browser/ui/webui/... File chrome/browser/ui/webui/options/media_galleries_handler.cc (right): https://codereview.chromium.org/14556015/diff/106001/chrome/browser/ui/webui/... chrome/browser/ui/webui/options/media_galleries_handler.cc:104: web_ui()->CallJavascriptFunction( On 2013/05/16 18:56:27, vandebo wrote: > Is it ok to do this asynchronously ? Most browser <-> renderer communication is async.
https://codereview.chromium.org/14556015/diff/106001/chrome/browser/extension... File chrome/browser/extensions/api/media_galleries_private/gallery_watch_state_tracker.cc (right): https://codereview.chromium.org/14556015/diff/106001/chrome/browser/extension... chrome/browser/extensions/api/media_galleries_private/gallery_watch_state_tracker.cc:280: base::FilePath gallery_file_path(preferences_->LookUpGalleryPathForExtension( On 2013/05/16 21:09:12, Lei Zhang wrote: > On 2013/05/16 18:56:27, vandebo wrote: > > Why are we sure preferences will be set by this point? > > I looked at the caller chain and someone must have called SetPreferences() > before getting here. Let me look that up... Oh, SetupGalleryWatch() has two callers: 1) ReadFromStorageWithPreferences() above which calls SetPreferences(). 2) OnGalleryPermissionChanged(), and MediaGalleriesPreferences calls OnGalleryPermissionChanged(). For this to happen, the extension must exist, and if the extension exists, then (1) already occurred.
OK, I improved the object flow through the watch tracker. There were a couple other spots we found talking that are fixed up -- the WebUI is doing the right thing, and the tracker remove-all call is fixed up with a bottom half. https://codereview.chromium.org/14556015/diff/106001/chrome/browser/extension... File chrome/browser/extensions/api/media_galleries_private/gallery_watch_state_tracker.cc (right): https://codereview.chromium.org/14556015/diff/106001/chrome/browser/extension... chrome/browser/extensions/api/media_galleries_private/gallery_watch_state_tracker.cc:280: base::FilePath gallery_file_path(preferences_->LookUpGalleryPathForExtension( On 2013/05/16 21:09:12, Lei Zhang wrote: > On 2013/05/16 18:56:27, vandebo wrote: > > Why are we sure preferences will be set by this point? > > I looked at the caller chain and someone must have called SetPreferences() > before getting here. Let me look that up... One call is from ReadFromStorageWithPreferences above, the other is from OnGalleryPermissionChanged, which is called from the permissions object itself. We may need to pass the prefs through that call to make sure it gets set, I'm not sure yet. Lei, do you know? https://codereview.chromium.org/14556015/diff/106001/chrome/browser/extension... chrome/browser/extensions/api/media_galleries_private/gallery_watch_state_tracker.cc:280: base::FilePath gallery_file_path(preferences_->LookUpGalleryPathForExtension( On 2013/05/16 22:17:52, Lei Zhang wrote: > On 2013/05/16 21:09:12, Lei Zhang wrote: > > On 2013/05/16 18:56:27, vandebo wrote: > > > Why are we sure preferences will be set by this point? > > > > I looked at the caller chain and someone must have called SetPreferences() > > before getting here. Let me look that up... > > Oh, SetupGalleryWatch() has two callers: > 1) ReadFromStorageWithPreferences() above which calls SetPreferences(). > > 2) OnGalleryPermissionChanged(), and MediaGalleriesPreferences calls > OnGalleryPermissionChanged(). For this to happen, the extension must exist, and > if the extension exists, then (1) already occurred. Is there a race where the prefs can get initialized and call this before the callback will run SetPreferences? If so, we can pass (this) through that OnGalleryPermissionChanged call. That's easy enough and makes the point clear. https://codereview.chromium.org/14556015/diff/106001/chrome/browser/extension... chrome/browser/extensions/api/media_galleries_private/gallery_watch_state_tracker.cc:309: base::FilePath gallery_file_path(preferences_->LookUpGalleryPathForExtension( On 2013/05/16 18:56:27, vandebo wrote: > ditto Call situation here is only from OnGalleryPermissionChanged. So we may need to push the prefs pointer through that call. https://codereview.chromium.org/14556015/diff/106001/chrome/browser/extension... File chrome/browser/extensions/api/media_galleries_private/gallery_watch_state_tracker.h (right): https://codereview.chromium.org/14556015/diff/106001/chrome/browser/extension... chrome/browser/extensions/api/media_galleries_private/gallery_watch_state_tracker.h:136: // The preferences object from which we get gallery information. On 2013/05/16 18:56:27, vandebo wrote: > I assume the lifetime of this class is such that we don't have to worry about > storing a raw pointer? If so, please add a comment to that effect. Both are effectively profile scoped, but there may be shutdown issues we need to control for. https://codereview.chromium.org/14556015/diff/106001/chrome/browser/media_gal... File chrome/browser/media_galleries/media_file_system_registry.cc (right): https://codereview.chromium.org/14556015/diff/106001/chrome/browser/media_gal... chrome/browser/media_galleries/media_file_system_registry.cc:497: if (monitor->IsInitialized()) { On 2013/05/16 18:56:27, vandebo wrote: > You don't need this check - StorageMonitor::Initialize does this internally. This is the fast path -- if its initialized we just immediately run the callback in the same stack. If it isn't initialized, we call initialize() below. https://codereview.chromium.org/14556015/diff/106001/chrome/browser/media_gal... chrome/browser/media_galleries/media_file_system_registry.cc:513: callback.Run(GetPreferences(profile)); On 2013/05/16 18:56:27, vandebo wrote: > I don't think this is safe. If it is, a comment why should be added. What > happens if profile goes away why StorageMonitor is initializing. Makes sense. It turns out all we do with this is call the factory GetForProfile method. That'll return NULL if the profile is dead, but this happening at runtime is considered a boo-boo. We need to coordinate with the factory, which gets lifecycle calls per-profile. https://codereview.chromium.org/14556015/diff/106001/chrome/browser/media_gal... File chrome/browser/media_galleries/media_file_system_registry_unittest.cc (right): https://codereview.chromium.org/14556015/diff/106001/chrome/browser/media_gal... chrome/browser/media_galleries/media_file_system_registry_unittest.cc:603: void GetPreferencesForwarder(base::WaitableEvent* event, On 2013/05/16 18:56:27, vandebo wrote: > As far as I can tell, you don't need a waitable event in any of the cases you > have it in this file (Wait() is never used). I was thinking I'd have to run this in another thread, but this test actually just uses the one message loop. I could take it out -- it's just getting used as a bit storage currently. https://codereview.chromium.org/14556015/diff/106001/chrome/browser/media_gal... File chrome/browser/media_galleries/media_galleries_dialog_controller.h (right): https://codereview.chromium.org/14556015/diff/106001/chrome/browser/media_gal... chrome/browser/media_galleries/media_galleries_dialog_controller.h:130: void Init(MediaGalleriesPreferences* preferences); On 2013/05/16 18:56:27, vandebo wrote: > Should this be called OnPreferences() ? Done.
https://codereview.chromium.org/14556015/diff/106001/chrome/browser/media_gal... File chrome/browser/media_galleries/media_file_system_registry.cc (right): https://codereview.chromium.org/14556015/diff/106001/chrome/browser/media_gal... chrome/browser/media_galleries/media_file_system_registry.cc:497: if (monitor->IsInitialized()) { On 2013/05/16 23:27:55, Greg Billock wrote: > On 2013/05/16 18:56:27, vandebo wrote: > > You don't need this check - StorageMonitor::Initialize does this internally. > > This is the fast path -- if its initialized we just immediately run the callback > in the same stack. If it isn't initialized, we call initialize() below. But it's not substantially faster than just calling into monitor->Initialize. If it's already initialized, it does almost exactly this line. https://codereview.chromium.org/14556015/diff/106001/chrome/browser/media_gal... chrome/browser/media_galleries/media_file_system_registry.cc:513: callback.Run(GetPreferences(profile)); On 2013/05/16 23:27:55, Greg Billock wrote: > On 2013/05/16 18:56:27, vandebo wrote: > > I don't think this is safe. If it is, a comment why should be added. What > > happens if profile goes away why StorageMonitor is initializing. > > Makes sense. It turns out all we do with this is call the factory GetForProfile > method. That'll return NULL if the profile is dead, but this happening at > runtime is considered a boo-boo. We need to coordinate with the factory, which > gets lifecycle calls per-profile. I think we need to handle this before committing anything. https://codereview.chromium.org/14556015/diff/106001/chrome/browser/media_gal... File chrome/browser/media_galleries/media_file_system_registry_unittest.cc (right): https://codereview.chromium.org/14556015/diff/106001/chrome/browser/media_gal... chrome/browser/media_galleries/media_file_system_registry_unittest.cc:603: void GetPreferencesForwarder(base::WaitableEvent* event, On 2013/05/16 23:27:55, Greg Billock wrote: > On 2013/05/16 18:56:27, vandebo wrote: > > As far as I can tell, you don't need a waitable event in any of the cases you > > have it in this file (Wait() is never used). > > I was thinking I'd have to run this in another thread, but this test actually > just uses the one message loop. I could take it out -- it's just getting used as > a bit storage currently. Yes, please remove the waitable events, it makes things more complicated to understand. https://codereview.chromium.org/14556015/diff/106001/chrome/browser/ui/webui/... File chrome/browser/ui/webui/options/media_galleries_handler.cc (right): https://codereview.chromium.org/14556015/diff/106001/chrome/browser/ui/webui/... chrome/browser/ui/webui/options/media_galleries_handler.cc:104: web_ui()->CallJavascriptFunction( On 2013/05/16 21:09:12, Lei Zhang wrote: > On 2013/05/16 18:56:27, vandebo wrote: > > Is it ok to do this asynchronously ? > > Most browser <-> renderer communication is async. 'doh - right. https://codereview.chromium.org/14556015/diff/116001/chrome/browser/extension... File chrome/browser/extensions/api/media_galleries_private/gallery_watch_state_tracker.cc (right): https://codereview.chromium.org/14556015/diff/116001/chrome/browser/extension... chrome/browser/extensions/api/media_galleries_private/gallery_watch_state_tracker.cc:146: &GalleryWatchStateTracker::RemoveAllGalleryWatchersWithPreferences, Because of this, I think MediaGalleriesPrivateRemoveAllGalleryWatchFunction should inherit from AsyncExtensionFunction https://codereview.chromium.org/14556015/diff/116001/chrome/browser/media_gal... File chrome/browser/media_galleries/media_file_system_registry.cc (right): https://codereview.chromium.org/14556015/diff/116001/chrome/browser/media_gal... chrome/browser/media_galleries/media_file_system_registry.cc:459: Profile::FromBrowserContext(rvh->GetProcess()->GetBrowserContext()); Hmm, could the rvh have gone away while we were getting the preferences? https://codereview.chromium.org/14556015/diff/116001/chrome/browser/media_gal... File chrome/browser/media_galleries/media_file_system_registry.h (right): https://codereview.chromium.org/14556015/diff/116001/chrome/browser/media_gal... chrome/browser/media_galleries/media_file_system_registry.h:92: void GetPreferencesAsync( nit picky: maybe leave this name the same and rename the private one GetPreferencesInternal? Since it takes a callback it's kind of obvious that it is async... https://codereview.chromium.org/14556015/diff/116001/chrome/browser/media_gal... chrome/browser/media_galleries/media_file_system_registry.h:112: void GetMediaFileSystemsPostStorageMonitorInit( GetMediaFileSystemsWithPreferences ? https://codereview.chromium.org/14556015/diff/116001/chrome/browser/media_gal... File chrome/browser/media_galleries/media_galleries_preferences.cc (right): https://codereview.chromium.org/14556015/diff/116001/chrome/browser/media_gal... chrome/browser/media_galleries/media_galleries_preferences.cc:622: extensions::GalleryWatchStateTracker* state_tracker = Grr, this shouldn't be here... filed bug 241903
https://codereview.chromium.org/14556015/diff/106001/chrome/browser/media_gal... File chrome/browser/media_galleries/media_file_system_registry.cc (right): https://codereview.chromium.org/14556015/diff/106001/chrome/browser/media_gal... chrome/browser/media_galleries/media_file_system_registry.cc:497: if (monitor->IsInitialized()) { On 2013/05/17 22:19:58, vandebo wrote: > On 2013/05/16 23:27:55, Greg Billock wrote: > > On 2013/05/16 18:56:27, vandebo wrote: > > > You don't need this check - StorageMonitor::Initialize does this internally. > > > > This is the fast path -- if its initialized we just immediately run the > callback > > in the same stack. If it isn't initialized, we call initialize() below. > > But it's not substantially faster than just calling into monitor->Initialize. > If it's already initialized, it does almost exactly this line. It goes through a pair of callbacks. That's not crazy overhead, but it's worth smoothing over. Or are you worried about code coming to depend on being called in the same stack when it should not? https://codereview.chromium.org/14556015/diff/106001/chrome/browser/media_gal... chrome/browser/media_galleries/media_file_system_registry.cc:513: callback.Run(GetPreferences(profile)); On 2013/05/17 22:19:58, vandebo wrote: > On 2013/05/16 23:27:55, Greg Billock wrote: > > On 2013/05/16 18:56:27, vandebo wrote: > > > I don't think this is safe. If it is, a comment why should be added. What > > > happens if profile goes away why StorageMonitor is initializing. > > > > Makes sense. It turns out all we do with this is call the factory > GetForProfile > > method. That'll return NULL if the profile is dead, but this happening at > > runtime is considered a boo-boo. We need to coordinate with the factory, which > > gets lifecycle calls per-profile. > > I think we need to handle this before committing anything. I'm pondering this, but coming up with nothing except to change the structure of the async step pretty dramatically, and have callers get the preferences themselves after the call. Managing profile life within this operation is the kind of chore that's going to be a big headache and gum up the works. https://codereview.chromium.org/14556015/diff/106001/chrome/browser/media_gal... File chrome/browser/media_galleries/media_file_system_registry_unittest.cc (right): https://codereview.chromium.org/14556015/diff/106001/chrome/browser/media_gal... chrome/browser/media_galleries/media_file_system_registry_unittest.cc:603: void GetPreferencesForwarder(base::WaitableEvent* event, On 2013/05/17 22:19:58, vandebo wrote: > On 2013/05/16 23:27:55, Greg Billock wrote: > > On 2013/05/16 18:56:27, vandebo wrote: > > > As far as I can tell, you don't need a waitable event in any of the cases > you > > > have it in this file (Wait() is never used). > > > > I was thinking I'd have to run this in another thread, but this test actually > > just uses the one message loop. I could take it out -- it's just getting used > as > > a bit storage currently. > > Yes, please remove the waitable events, it makes things more complicated to > understand. Done.
https://codereview.chromium.org/14556015/diff/116001/chrome/browser/media_gal... File chrome/browser/media_galleries/media_file_system_registry.cc (right): https://codereview.chromium.org/14556015/diff/116001/chrome/browser/media_gal... chrome/browser/media_galleries/media_file_system_registry.cc:459: Profile::FromBrowserContext(rvh->GetProcess()->GetBrowserContext()); On 2013/05/17 22:19:58, vandebo wrote: > Hmm, could the rvh have gone away while we were getting the preferences? I think so. Here's my current thinking. We need to package this transaction in an object which is a RenderViewHostObserver. We also need to make GetPreferencesAsync return a pointer to a class MediaGalleriesPreferencesVendor { GetPreferences(Profile*) = 0; } which the registry can then implement. I think that'll all work. It's a step more indirect and complex than this, but encapsulating the transaction objects may make it tractable. :-/
https://codereview.chromium.org/14556015/diff/116001/chrome/browser/media_gal... File chrome/browser/media_galleries/media_file_system_registry.h (right): https://codereview.chromium.org/14556015/diff/116001/chrome/browser/media_gal... chrome/browser/media_galleries/media_file_system_registry.h:92: void GetPreferencesAsync( On 2013/05/17 22:19:58, vandebo wrote: > nit picky: maybe leave this name the same and rename the private one > GetPreferencesInternal? Since it takes a callback it's kind of obvious that it > is async... Fiddled this with the new interface. https://codereview.chromium.org/14556015/diff/116001/chrome/browser/media_gal... chrome/browser/media_galleries/media_file_system_registry.h:112: void GetMediaFileSystemsPostStorageMonitorInit( On 2013/05/17 22:19:58, vandebo wrote: > GetMediaFileSystemsWithPreferences ? Done. https://codereview.chromium.org/14556015/diff/116001/chrome/browser/media_gal... File chrome/browser/media_galleries/media_galleries_preferences.cc (right): https://codereview.chromium.org/14556015/diff/116001/chrome/browser/media_gal... chrome/browser/media_galleries/media_galleries_preferences.cc:622: extensions::GalleryWatchStateTracker* state_tracker = On 2013/05/17 22:19:58, vandebo wrote: > Grr, this shouldn't be here... filed bug 241903 We're using at least two mechanisms: the registry is using the prefs watcher to look for changes, we have this direct plumbing. Perhaps we need an observer on this class. :-/
I'm still digesting the Vendor concept. I'll take another look after we talk about the following ideas. A different approach we could take would be to ensure that storage monitor get initialized to all code entry points that could use it. I think that is in any call to media galleries api / media galleries private api, or the web ui. Though I'm not sure if that would be a lot better than what you already have. After seeing the complications lazily initialize adds, I think we should consider a different approach. It's async because we need to wait for storage monitor to load info about the currently attached devices. What if, we instead made initialization synchronous, but added a flag to the device attach callback indicating if the device is new or if it was already attached? There are probably still some places where the code assumes it can query about attached devices, but if we return an negative response until they are attached, it would probably be ok in a lot of places. What do you think? https://codereview.chromium.org/14556015/diff/106001/chrome/browser/media_gal... File chrome/browser/media_galleries/media_file_system_registry.cc (right): https://codereview.chromium.org/14556015/diff/106001/chrome/browser/media_gal... chrome/browser/media_galleries/media_file_system_registry.cc:497: if (monitor->IsInitialized()) { On 2013/05/18 00:02:26, Greg Billock wrote: > On 2013/05/17 22:19:58, vandebo wrote: > > On 2013/05/16 23:27:55, Greg Billock wrote: > > > On 2013/05/16 18:56:27, vandebo wrote: > > > > You don't need this check - StorageMonitor::Initialize does this > internally. > > > > > > This is the fast path -- if its initialized we just immediately run the > > callback > > > in the same stack. If it isn't initialized, we call initialize() below. > > > > But it's not substantially faster than just calling into monitor->Initialize. > > If it's already initialized, it does almost exactly this line. > > It goes through a pair of callbacks. That's not crazy overhead, but it's worth > smoothing over. Or are you worried about code coming to depend on being called > in the same stack when it should not? There's one (not two) extra layers of callback, yes. But If that simplifies the code and lets us get rid of IsInitialized, then I think it's worthwhile. https://codereview.chromium.org/14556015/diff/116001/chrome/browser/media_gal... File chrome/browser/media_galleries/media_galleries_preferences.cc (right): https://codereview.chromium.org/14556015/diff/116001/chrome/browser/media_gal... chrome/browser/media_galleries/media_galleries_preferences.cc:622: extensions::GalleryWatchStateTracker* state_tracker = On 2013/05/18 06:28:40, Greg Billock wrote: > On 2013/05/17 22:19:58, vandebo wrote: > > Grr, this shouldn't be here... filed bug 241903 > > We're using at least two mechanisms: the registry is using the prefs watcher to > look for changes, we have this direct plumbing. Perhaps we need an observer on > this class. :-/ Actually three - This class already implements an observer, see NotifyChangeObservers.
On 2013/05/20 15:45:46, vandebo wrote: > I'm still digesting the Vendor concept. I'll take another look after we talk > about the following ideas. > > A different approach we could take would be to ensure that storage monitor get > initialized to all code entry points that could use it. I think that is in any > call to media galleries api / media galleries private api, or the web ui. > Though I'm not sure if that would be a lot better than what you already have. > > After seeing the complications lazily initialize adds, I think we should > consider a different approach. It's async because we need to wait for storage > monitor to load info about the currently attached devices. What if, we instead > made initialization synchronous, but added a flag to the device attach callback > indicating if the device is new or if it was already attached? > > There are probably still some places where the code assumes it can query about > attached devices, but if we return an negative response until they are attached, > it would probably be ok in a lot of places. What do you think? > > https://codereview.chromium.org/14556015/diff/106001/chrome/browser/media_gal... > File chrome/browser/media_galleries/media_file_system_registry.cc (right): > > https://codereview.chromium.org/14556015/diff/106001/chrome/browser/media_gal... > chrome/browser/media_galleries/media_file_system_registry.cc:497: if > (monitor->IsInitialized()) { > On 2013/05/18 00:02:26, Greg Billock wrote: > > On 2013/05/17 22:19:58, vandebo wrote: > > > On 2013/05/16 23:27:55, Greg Billock wrote: > > > > On 2013/05/16 18:56:27, vandebo wrote: > > > > > You don't need this check - StorageMonitor::Initialize does this > > internally. > > > > > > > > This is the fast path -- if its initialized we just immediately run the > > > callback > > > > in the same stack. If it isn't initialized, we call initialize() below. > > > > > > But it's not substantially faster than just calling into > monitor->Initialize. > > > If it's already initialized, it does almost exactly this line. > > > > It goes through a pair of callbacks. That's not crazy overhead, but it's worth > > smoothing over. Or are you worried about code coming to depend on being called > > in the same stack when it should not? > > There's one (not two) extra layers of callback, yes. But If that simplifies the > code and lets us get rid of IsInitialized, then I think it's worthwhile. > > https://codereview.chromium.org/14556015/diff/116001/chrome/browser/media_gal... > File chrome/browser/media_galleries/media_galleries_preferences.cc (right): > > https://codereview.chromium.org/14556015/diff/116001/chrome/browser/media_gal... > chrome/browser/media_galleries/media_galleries_preferences.cc:622: > extensions::GalleryWatchStateTracker* state_tracker = > On 2013/05/18 06:28:40, Greg Billock wrote: > > On 2013/05/17 22:19:58, vandebo wrote: > > > Grr, this shouldn't be here... filed bug 241903 > > > > We're using at least two mechanisms: the registry is using the prefs watcher > to > > look for changes, we have this direct plumbing. Perhaps we need an observer on > > this class. :-/ > > Actually three - This class already implements an observer, see > NotifyChangeObservers. Switched approaches. Now I've decoupled getting the storage monitor initialized and getting the preferences. Still a bit blocky, but better I think.
https://codereview.chromium.org/14556015/diff/169001/chrome/browser/extension... File chrome/browser/extensions/api/media_galleries_private/gallery_watch_state_tracker.cc (right): https://codereview.chromium.org/14556015/diff/169001/chrome/browser/extension... chrome/browser/extensions/api/media_galleries_private/gallery_watch_state_tracker.cc:88: registrar_.Add(this, chrome::NOTIFICATION_EXTENSION_LOADED, Why do we need to wait for storage monitor to be initialized before doing these registrations? Is it correct for GalleryWatchStateTracker to exist in a half initialized state? https://codereview.chromium.org/14556015/diff/169001/chrome/browser/extension... chrome/browser/extensions/api/media_galleries_private/gallery_watch_state_tracker.cc:247: chrome::MediaGalleriesPreferences* preferences = DCHECK that StorageMonitor is initialized? https://codereview.chromium.org/14556015/diff/169001/chrome/browser/extension... File chrome/browser/extensions/api/media_galleries_private/media_galleries_private_api.cc (right): https://codereview.chromium.org/14556015/diff/169001/chrome/browser/extension... chrome/browser/extensions/api/media_galleries_private/media_galleries_private_api.cc:309: HandleResponse(pref_id, false); error_ = kInvalidGalleryIDError; https://codereview.chromium.org/14556015/diff/169001/chrome/browser/extension... chrome/browser/extensions/api/media_galleries_private/media_galleries_private_api.cc:397: HandleResponse(pref_id, false); error_ = kInvalidGalleryIDError; https://codereview.chromium.org/14556015/diff/169001/chrome/browser/extension... chrome/browser/extensions/api/media_galleries_private/media_galleries_private_api.cc:403: state_tracker->SetPreferences(preferences); I thought we didn't have SetPreferences() any more.
https://codereview.chromium.org/14556015/diff/169001/chrome/browser/extension... File chrome/browser/extensions/api/media_galleries_private/gallery_watch_state_tracker.cc (right): https://codereview.chromium.org/14556015/diff/169001/chrome/browser/extension... chrome/browser/extensions/api/media_galleries_private/gallery_watch_state_tracker.cc:88: registrar_.Add(this, chrome::NOTIFICATION_EXTENSION_LOADED, On 2013/05/22 16:27:52, vandebo wrote: > Why do we need to wait for storage monitor to be initialized before doing these > registrations? Is it correct for GalleryWatchStateTracker to exist in a half > initialized state? Observe() calls ReadFromStorage(), which requires initialization. I wondered about the half-initialized state, and investigated that. The only other calls into the class carry the initialized permissions, so we're OK. https://codereview.chromium.org/14556015/diff/169001/chrome/browser/extension... chrome/browser/extensions/api/media_galleries_private/gallery_watch_state_tracker.cc:247: chrome::MediaGalleriesPreferences* preferences = On 2013/05/22 16:27:52, vandebo wrote: > DCHECK that StorageMonitor is initialized? This is done in GetPreferences(). https://codereview.chromium.org/14556015/diff/169001/chrome/browser/extension... File chrome/browser/extensions/api/media_galleries_private/media_galleries_private_api.cc (right): https://codereview.chromium.org/14556015/diff/169001/chrome/browser/extension... chrome/browser/extensions/api/media_galleries_private/media_galleries_private_api.cc:309: HandleResponse(pref_id, false); On 2013/05/22 16:27:52, vandebo wrote: > error_ = kInvalidGalleryIDError; Done. https://codereview.chromium.org/14556015/diff/169001/chrome/browser/extension... chrome/browser/extensions/api/media_galleries_private/media_galleries_private_api.cc:397: HandleResponse(pref_id, false); On 2013/05/22 16:27:52, vandebo wrote: > error_ = kInvalidGalleryIDError; Done. https://codereview.chromium.org/14556015/diff/169001/chrome/browser/extension... chrome/browser/extensions/api/media_galleries_private/media_galleries_private_api.cc:403: state_tracker->SetPreferences(preferences); On 2013/05/22 16:27:52, vandebo wrote: > I thought we didn't have SetPreferences() any more. We don't. Fixing now.
https://codereview.chromium.org/14556015/diff/177001/chrome/browser/storage_m... File chrome/browser/storage_monitor/storage_monitor_unittest.cc (right): https://codereview.chromium.org/14556015/diff/177001/chrome/browser/storage_m... chrome/browser/storage_monitor/storage_monitor_unittest.cc:18: content::TestBrowserThread(content::BrowserThread::UI, &message_loop_); Since you no longer DCHECK that you're on UI thread, is this still necessary?
https://codereview.chromium.org/14556015/diff/169001/chrome/browser/extension... File chrome/browser/extensions/api/media_galleries_private/gallery_watch_state_tracker.cc (right): https://codereview.chromium.org/14556015/diff/169001/chrome/browser/extension... chrome/browser/extensions/api/media_galleries_private/gallery_watch_state_tracker.cc:88: registrar_.Add(this, chrome::NOTIFICATION_EXTENSION_LOADED, On 2013/05/22 18:42:52, Greg Billock wrote: > On 2013/05/22 16:27:52, vandebo wrote: > > Why do we need to wait for storage monitor to be initialized before doing > these > > registrations? Is it correct for GalleryWatchStateTracker to exist in a half > > initialized state? > > Observe() calls ReadFromStorage(), which requires initialization. > > I wondered about the half-initialized state, and investigated that. The only > other calls into the class carry the initialized permissions, so we're OK. I dug into this a bit and I think it can be untangled. Registration for the events that Observe() receives is done in the GalleryWatchStateTracker constructor. But the tracker doesn't care about the events until someone calls the API, so we could delay creating the state tracker until it's needed. In fact, it seems that's what's done with MediaGalleriesPrivateEventRouter. MediaGalleriesPrivateApi (which, by the way is a profile keyed service) owns both MediaGalleriesPrivateEventRouter and GalleryWatchStateTracker. If we just move GalleryWatchStateTracker to be owned by MediaGalleriesPrivateEventRouter, I think we can be assured that the StorageMonitor is initialized before any notification arrives. https://codereview.chromium.org/14556015/diff/177001/chrome/browser/extension... File chrome/browser/extensions/api/media_galleries_private/gallery_watch_state_tracker.h (right): https://codereview.chromium.org/14556015/diff/177001/chrome/browser/extension... chrome/browser/extensions/api/media_galleries_private/gallery_watch_state_tracker.h:56: void OnGalleryPermissionChanged( Can you base this change on the other, so we don't see the other changes here. https://codereview.chromium.org/14556015/diff/177001/chrome/browser/media_gal... File chrome/browser/media_galleries/media_file_system_registry.cc (right): https://codereview.chromium.org/14556015/diff/177001/chrome/browser/media_gal... chrome/browser/media_galleries/media_file_system_registry.cc:419: class GetPreferencesTransaction : public content::RenderViewHostObserver { You probably don't need this. But if you do the StorageMonitor init at the API level, it'll be clear that you don't need it. https://codereview.chromium.org/14556015/diff/177001/chrome/browser/media_gal... File chrome/browser/media_galleries/media_file_system_registry.h (left): https://codereview.chromium.org/14556015/diff/177001/chrome/browser/media_gal... chrome/browser/media_galleries/media_file_system_registry.h:76: const content::RenderViewHost* rvh, why? https://codereview.chromium.org/14556015/diff/177001/chrome/browser/media_gal... File chrome/browser/media_galleries/media_file_system_registry.h (right): https://codereview.chromium.org/14556015/diff/177001/chrome/browser/media_gal... chrome/browser/media_galleries/media_file_system_registry.h:95: virtual MediaGalleriesPreferences* GetPreferences(Profile* profile) OVERRIDE; I think you want to undo the changes here. https://codereview.chromium.org/14556015/diff/177001/chrome/browser/media_gal... chrome/browser/media_galleries/media_file_system_registry.h:103: void GetMediaFileSystemsPostInit(const content::RenderViewHost* rvh, I guess this plus the dialog controller is a narrower choke point, but maybe it would be better to ensure the storage monitor is initialized in all the media galleries API calls. It would make things more regular across all the APIs. https://codereview.chromium.org/14556015/diff/177001/chrome/browser/media_gal... File chrome/browser/media_galleries/media_file_system_registry_unittest.cc (right): https://codereview.chromium.org/14556015/diff/177001/chrome/browser/media_gal... chrome/browser/media_galleries/media_file_system_registry_unittest.cc:601: return GetMediaFileSystemRegistry()->GetPreferences(profile); Isn't this a recursive call? https://codereview.chromium.org/14556015/diff/177001/chrome/browser/media_gal... chrome/browser/media_galleries/media_file_system_registry_unittest.cc:732: monitor_.reset(new test::TestStorageMonitor()); Looks like the waitable event found it's way back. https://codereview.chromium.org/14556015/diff/177001/chrome/browser/media_gal... File chrome/browser/media_galleries/media_galleries_dialog_controller.cc (right): https://codereview.chromium.org/14556015/diff/177001/chrome/browser/media_gal... chrome/browser/media_galleries/media_galleries_dialog_controller.cc:128: weak_ptr_factory_(this) {} Controller is self owned (see media_galleries/media_galleries_api.cc:215) so we don't need this. https://codereview.chromium.org/14556015/diff/177001/chrome/browser/media_gal... File chrome/browser/media_galleries/media_galleries_preferences.h (right): https://codereview.chromium.org/14556015/diff/177001/chrome/browser/media_gal... chrome/browser/media_galleries/media_galleries_preferences.h:128: class Vendor { Left over?
https://codereview.chromium.org/14556015/diff/169001/chrome/browser/extension... File chrome/browser/extensions/api/media_galleries_private/gallery_watch_state_tracker.cc (right): https://codereview.chromium.org/14556015/diff/169001/chrome/browser/extension... chrome/browser/extensions/api/media_galleries_private/gallery_watch_state_tracker.cc:88: registrar_.Add(this, chrome::NOTIFICATION_EXTENSION_LOADED, On 2013/05/22 21:40:34, vandebo wrote: > On 2013/05/22 18:42:52, Greg Billock wrote: > > On 2013/05/22 16:27:52, vandebo wrote: > > > Why do we need to wait for storage monitor to be initialized before doing > > these > > > registrations? Is it correct for GalleryWatchStateTracker to exist in a > half > > > initialized state? > > > > Observe() calls ReadFromStorage(), which requires initialization. > > > > I wondered about the half-initialized state, and investigated that. The only > > other calls into the class carry the initialized permissions, so we're OK. > > I dug into this a bit and I think it can be untangled. Registration for the > events that Observe() receives is done in the GalleryWatchStateTracker > constructor. But the tracker doesn't care about the events until someone calls > the API, so we could delay creating the state tracker until it's needed. In > fact, it seems that's what's done with MediaGalleriesPrivateEventRouter. > MediaGalleriesPrivateApi (which, by the way is a profile keyed service) owns > both MediaGalleriesPrivateEventRouter and GalleryWatchStateTracker. If we just > move GalleryWatchStateTracker to be owned by MediaGalleriesPrivateEventRouter, I > think we can be assured that the StorageMonitor is initialized before any > notification arrives. Shall I do that here? That's part of the refactor we talked about. I could split it out separately, as well... https://codereview.chromium.org/14556015/diff/177001/chrome/browser/extension... File chrome/browser/extensions/api/media_galleries_private/gallery_watch_state_tracker.h (right): https://codereview.chromium.org/14556015/diff/177001/chrome/browser/extension... chrome/browser/extensions/api/media_galleries_private/gallery_watch_state_tracker.h:56: void OnGalleryPermissionChanged( On 2013/05/22 21:40:34, vandebo wrote: > Can you base this change on the other, so we don't see the other changes here. Yeah, I'll do that. https://codereview.chromium.org/14556015/diff/177001/chrome/browser/media_gal... File chrome/browser/media_galleries/media_file_system_registry.cc (right): https://codereview.chromium.org/14556015/diff/177001/chrome/browser/media_gal... chrome/browser/media_galleries/media_file_system_registry.cc:419: class GetPreferencesTransaction : public content::RenderViewHostObserver { On 2013/05/22 21:40:34, vandebo wrote: > You probably don't need this. But if you do the StorageMonitor init at the API > level, it'll be clear that you don't need it. We need it since the RVH can disappear while we're initializing the storage monitor. GetMediaFileSystemsForExtension is the entry point of the API, so that's our trigger to initialize. Doing that async means the RVH could be closed. https://codereview.chromium.org/14556015/diff/177001/chrome/browser/media_gal... File chrome/browser/media_galleries/media_file_system_registry.h (left): https://codereview.chromium.org/14556015/diff/177001/chrome/browser/media_gal... chrome/browser/media_galleries/media_file_system_registry.h:76: const content::RenderViewHost* rvh, On 2013/05/22 21:40:34, vandebo wrote: > why? Needed to make a RenderViewHostObserver https://codereview.chromium.org/14556015/diff/177001/chrome/browser/media_gal... File chrome/browser/media_galleries/media_file_system_registry.h (right): https://codereview.chromium.org/14556015/diff/177001/chrome/browser/media_gal... chrome/browser/media_galleries/media_file_system_registry.h:95: virtual MediaGalleriesPreferences* GetPreferences(Profile* profile) OVERRIDE; On 2013/05/22 21:40:34, vandebo wrote: > I think you want to undo the changes here. Done, but the comment is edited somewhat. Put it back in the right spot. https://codereview.chromium.org/14556015/diff/177001/chrome/browser/media_gal... chrome/browser/media_galleries/media_file_system_registry.h:103: void GetMediaFileSystemsPostInit(const content::RenderViewHost* rvh, On 2013/05/22 21:40:34, vandebo wrote: > I guess this plus the dialog controller is a narrower choke point, but maybe it > would be better to ensure the storage monitor is initialized in all the media > galleries API calls. It would make things more regular across all the APIs. Oh, I see what you're saying. Hmmm. Let me think about that. We have to do the private APIs anyway, and it may be that sticking it in the other API call is less invasive. https://codereview.chromium.org/14556015/diff/177001/chrome/browser/media_gal... File chrome/browser/media_galleries/media_file_system_registry_unittest.cc (right): https://codereview.chromium.org/14556015/diff/177001/chrome/browser/media_gal... chrome/browser/media_galleries/media_file_system_registry_unittest.cc:601: return GetMediaFileSystemRegistry()->GetPreferences(profile); On 2013/05/22 21:40:34, vandebo wrote: > Isn't this a recursive call? No, this is in the test. ;-) https://codereview.chromium.org/14556015/diff/177001/chrome/browser/media_gal... chrome/browser/media_galleries/media_file_system_registry_unittest.cc:732: monitor_.reset(new test::TestStorageMonitor()); On 2013/05/22 21:40:34, vandebo wrote: > Looks like the waitable event found it's way back. Grrr. I must have branched without checking in some stuff or something stupid. https://codereview.chromium.org/14556015/diff/177001/chrome/browser/media_gal... File chrome/browser/media_galleries/media_galleries_dialog_controller.cc (right): https://codereview.chromium.org/14556015/diff/177001/chrome/browser/media_gal... chrome/browser/media_galleries/media_galleries_dialog_controller.cc:128: weak_ptr_factory_(this) {} On 2013/05/22 21:40:34, vandebo wrote: > Controller is self owned (see media_galleries/media_galleries_api.cc:215) so we > don't need this. Done. https://codereview.chromium.org/14556015/diff/177001/chrome/browser/media_gal... File chrome/browser/media_galleries/media_galleries_preferences.h (right): https://codereview.chromium.org/14556015/diff/177001/chrome/browser/media_gal... chrome/browser/media_galleries/media_galleries_preferences.h:128: class Vendor { On 2013/05/22 21:40:34, vandebo wrote: > Left over? Yes. Removed. https://codereview.chromium.org/14556015/diff/177001/chrome/browser/storage_m... File chrome/browser/storage_monitor/storage_monitor_unittest.cc (right): https://codereview.chromium.org/14556015/diff/177001/chrome/browser/storage_m... chrome/browser/storage_monitor/storage_monitor_unittest.cc:18: content::TestBrowserThread(content::BrowserThread::UI, &message_loop_); On 2013/05/22 21:13:12, tommycli wrote: > Since you no longer DCHECK that you're on UI thread, is this still necessary? No. It doesn't work anyway. :-/
https://codereview.chromium.org/14556015/diff/169001/chrome/browser/extension... File chrome/browser/extensions/api/media_galleries_private/gallery_watch_state_tracker.cc (right): https://codereview.chromium.org/14556015/diff/169001/chrome/browser/extension... chrome/browser/extensions/api/media_galleries_private/gallery_watch_state_tracker.cc:88: registrar_.Add(this, chrome::NOTIFICATION_EXTENSION_LOADED, On 2013/05/23 00:55:59, Greg Billock wrote: > On 2013/05/22 21:40:34, vandebo wrote: > > On 2013/05/22 18:42:52, Greg Billock wrote: > > > On 2013/05/22 16:27:52, vandebo wrote: > > > > Why do we need to wait for storage monitor to be initialized before doing > > > these > > > > registrations? Is it correct for GalleryWatchStateTracker to exist in a > > half > > > > initialized state? > > > > > > Observe() calls ReadFromStorage(), which requires initialization. > > > > > > I wondered about the half-initialized state, and investigated that. The only > > > other calls into the class carry the initialized permissions, so we're OK. > > > > I dug into this a bit and I think it can be untangled. Registration for the > > events that Observe() receives is done in the GalleryWatchStateTracker > > constructor. But the tracker doesn't care about the events until someone calls > > the API, so we could delay creating the state tracker until it's needed. In > > fact, it seems that's what's done with MediaGalleriesPrivateEventRouter. > > MediaGalleriesPrivateApi (which, by the way is a profile keyed service) owns > > both MediaGalleriesPrivateEventRouter and GalleryWatchStateTracker. If we > just > > move GalleryWatchStateTracker to be owned by MediaGalleriesPrivateEventRouter, > I > > think we can be assured that the StorageMonitor is initialized before any > > notification arrives. > > Shall I do that here? That's part of the refactor we talked about. I could split > it out separately, as well... Seems like that too would benefit from its own CL. https://codereview.chromium.org/14556015/diff/177001/chrome/browser/media_gal... File chrome/browser/media_galleries/media_file_system_registry.cc (right): https://codereview.chromium.org/14556015/diff/177001/chrome/browser/media_gal... chrome/browser/media_galleries/media_file_system_registry.cc:419: class GetPreferencesTransaction : public content::RenderViewHostObserver { On 2013/05/23 00:55:59, Greg Billock wrote: > On 2013/05/22 21:40:34, vandebo wrote: > > You probably don't need this. But if you do the StorageMonitor init at the > API > > level, it'll be clear that you don't need it. > > We need it since the RVH can disappear while we're initializing the storage > monitor. GetMediaFileSystemsForExtension is the entry point of the API, so > that's our trigger to initialize. Doing that async means the RVH could be > closed. We talked about the extension system probably having a rvh listener, did you check for that? https://codereview.chromium.org/14556015/diff/188001/chrome/browser/extension... File chrome/browser/extensions/api/media_galleries_private/media_galleries_private_api.h (right): https://codereview.chromium.org/14556015/diff/188001/chrome/browser/extension... chrome/browser/extensions/api/media_galleries_private/media_galleries_private_api.h:111: void OnStorageMonitorInit(chrome::MediaGalleryPrefId pref_id); Is the implementation of these missing? https://codereview.chromium.org/14556015/diff/188001/chrome/browser/media_gal... File chrome/browser/media_galleries/media_file_system_registry.h (right): https://codereview.chromium.org/14556015/diff/188001/chrome/browser/media_gal... chrome/browser/media_galleries/media_file_system_registry.h:87: virtual MediaGalleriesPreferences* GetPreferences(Profile* profile) OVERRIDE; Not an override. Does it need to be virtual? https://codereview.chromium.org/14556015/diff/188001/chrome/browser/media_gal... File chrome/browser/media_galleries/media_file_system_registry_unittest.cc (right): https://codereview.chromium.org/14556015/diff/188001/chrome/browser/media_gal... chrome/browser/media_galleries/media_file_system_registry_unittest.cc:601: return GetMediaFileSystemRegistry()->GetPreferences(profile); Am I being dense? How is this different than just not overriding it? https://codereview.chromium.org/14556015/diff/188001/chrome/browser/media_gal... chrome/browser/media_galleries/media_file_system_registry_unittest.cc:732: monitor_.reset(new test::TestStorageMonitor()); Put this in the #else clause. Can use TestStoageMonitor on all platforms? https://codereview.chromium.org/14556015/diff/188001/chrome/browser/media_gal... File chrome/browser/media_galleries/media_galleries_dialog_controller.cc (right): https://codereview.chromium.org/14556015/diff/188001/chrome/browser/media_gal... chrome/browser/media_galleries/media_galleries_dialog_controller.cc:102: StorageMonitor::GetInstance()->Initialize(base::Bind( nit: add a comment about why unretained is safe. https://codereview.chromium.org/14556015/diff/188001/chrome/browser/media_gal... chrome/browser/media_galleries/media_galleries_dialog_controller.cc:117: if (monitor) aside: looks like you can remove this if(monitor) https://codereview.chromium.org/14556015/diff/188001/chrome/browser/ui/webui/... File chrome/browser/ui/webui/options/media_galleries_handler.cc (right): https://codereview.chromium.org/14556015/diff/188001/chrome/browser/ui/webui/... chrome/browser/ui/webui/options/media_galleries_handler.cc:67: // GetPreferencesAsync. That is needed not only to populate the comment out of date.
OK, approaches switched again. https://codereview.chromium.org/14556015/diff/169001/chrome/browser/extension... File chrome/browser/extensions/api/media_galleries_private/gallery_watch_state_tracker.cc (right): https://codereview.chromium.org/14556015/diff/169001/chrome/browser/extension... chrome/browser/extensions/api/media_galleries_private/gallery_watch_state_tracker.cc:88: registrar_.Add(this, chrome::NOTIFICATION_EXTENSION_LOADED, On 2013/05/23 15:04:17, vandebo wrote: > On 2013/05/23 00:55:59, Greg Billock wrote: > > On 2013/05/22 21:40:34, vandebo wrote: > > > On 2013/05/22 18:42:52, Greg Billock wrote: > > > > On 2013/05/22 16:27:52, vandebo wrote: > > > > > Why do we need to wait for storage monitor to be initialized before > doing > > > > these > > > > > registrations? Is it correct for GalleryWatchStateTracker to exist in a > > > half > > > > > initialized state? > > > > > > > > Observe() calls ReadFromStorage(), which requires initialization. > > > > > > > > I wondered about the half-initialized state, and investigated that. The > only > > > > other calls into the class carry the initialized permissions, so we're OK. > > > > > > I dug into this a bit and I think it can be untangled. Registration for the > > > events that Observe() receives is done in the GalleryWatchStateTracker > > > constructor. But the tracker doesn't care about the events until someone > calls > > > the API, so we could delay creating the state tracker until it's needed. In > > > fact, it seems that's what's done with MediaGalleriesPrivateEventRouter. > > > MediaGalleriesPrivateApi (which, by the way is a profile keyed service) owns > > > both MediaGalleriesPrivateEventRouter and GalleryWatchStateTracker. If we > > just > > > move GalleryWatchStateTracker to be owned by > MediaGalleriesPrivateEventRouter, > > I > > > think we can be assured that the StorageMonitor is initialized before any > > > notification arrives. > > > > Shall I do that here? That's part of the refactor we talked about. I could > split > > it out separately, as well... > > Seems like that too would benefit from its own CL. It was in the other one https://codereview.chromium.org/14556015/diff/177001/chrome/browser/media_gal... File chrome/browser/media_galleries/media_file_system_registry.cc (right): https://codereview.chromium.org/14556015/diff/177001/chrome/browser/media_gal... chrome/browser/media_galleries/media_file_system_registry.cc:419: class GetPreferencesTransaction : public content::RenderViewHostObserver { On 2013/05/23 15:04:17, vandebo wrote: > On 2013/05/23 00:55:59, Greg Billock wrote: > > On 2013/05/22 21:40:34, vandebo wrote: > > > You probably don't need this. But if you do the StorageMonitor init at the > > API > > > level, it'll be clear that you don't need it. > > > > We need it since the RVH can disappear while we're initializing the storage > > monitor. GetMediaFileSystemsForExtension is the entry point of the API, so > > that's our trigger to initialize. Doing that async means the RVH could be > > closed. > > We talked about the extension system probably having a rvh listener, did you > check for that? This is moved to the API now. You'll see a couple classes needed weak ptrs, which I've added. https://codereview.chromium.org/14556015/diff/188001/chrome/browser/extension... File chrome/browser/extensions/api/media_galleries_private/media_galleries_private_api.h (right): https://codereview.chromium.org/14556015/diff/188001/chrome/browser/extension... chrome/browser/extensions/api/media_galleries_private/media_galleries_private_api.h:111: void OnStorageMonitorInit(chrome::MediaGalleryPrefId pref_id); On 2013/05/23 15:04:17, vandebo wrote: > Is the implementation of these missing? Added https://codereview.chromium.org/14556015/diff/188001/chrome/browser/media_gal... File chrome/browser/media_galleries/media_file_system_registry.h (right): https://codereview.chromium.org/14556015/diff/188001/chrome/browser/media_gal... chrome/browser/media_galleries/media_file_system_registry.h:87: virtual MediaGalleriesPreferences* GetPreferences(Profile* profile) OVERRIDE; On 2013/05/23 15:04:17, vandebo wrote: > Not an override. Does it need to be virtual? Reverted this. (Was from the ::Vendor class I had before). https://codereview.chromium.org/14556015/diff/188001/chrome/browser/media_gal... File chrome/browser/media_galleries/media_file_system_registry_unittest.cc (right): https://codereview.chromium.org/14556015/diff/188001/chrome/browser/media_gal... chrome/browser/media_galleries/media_file_system_registry_unittest.cc:601: return GetMediaFileSystemRegistry()->GetPreferences(profile); On 2013/05/23 15:04:17, vandebo wrote: > Am I being dense? How is this different than just not overriding it? The test isn't a subclass -- it's just a convenience method to shorten some test lines. https://codereview.chromium.org/14556015/diff/188001/chrome/browser/media_gal... chrome/browser/media_galleries/media_file_system_registry_unittest.cc:732: monitor_.reset(new test::TestStorageMonitor()); On 2013/05/23 15:04:17, vandebo wrote: > Put this in the #else clause. > > Can use TestStoageMonitor on all platforms? The Win mock is for the MTP thing, which really should be in another test file, not here. https://codereview.chromium.org/14556015/diff/188001/chrome/browser/media_gal... File chrome/browser/media_galleries/media_galleries_dialog_controller.cc (right): https://codereview.chromium.org/14556015/diff/188001/chrome/browser/media_gal... chrome/browser/media_galleries/media_galleries_dialog_controller.cc:102: StorageMonitor::GetInstance()->Initialize(base::Bind( On 2013/05/23 15:04:17, vandebo wrote: > nit: add a comment about why unretained is safe. Done. https://codereview.chromium.org/14556015/diff/188001/chrome/browser/media_gal... chrome/browser/media_galleries/media_galleries_dialog_controller.cc:117: if (monitor) On 2013/05/23 15:04:17, vandebo wrote: > aside: looks like you can remove this if(monitor) Done in separate cl along with many others. https://codereview.chromium.org/14556015/diff/188001/chrome/browser/ui/webui/... File chrome/browser/ui/webui/options/media_galleries_handler.cc (right): https://codereview.chromium.org/14556015/diff/188001/chrome/browser/ui/webui/... chrome/browser/ui/webui/options/media_galleries_handler.cc:67: // GetPreferencesAsync. That is needed not only to populate the On 2013/05/23 15:04:17, vandebo wrote: > comment out of date. Done.
I wanted to sign off on this, but the issue in MediaGalleriesHandler stopped me. Everything else is nits. https://codereview.chromium.org/14556015/diff/213003/chrome/browser/extension... File chrome/browser/extensions/api/media_galleries/media_galleries_api.h (right): https://codereview.chromium.org/14556015/diff/213003/chrome/browser/extension... chrome/browser/extensions/api/media_galleries/media_galleries_api.h:32: // Bottom half for RunImpl, invoked after the storage monitor is initialized. nit: Since RunImpl was already async and posts tasks with callbacks, to call this the bottom half is not quite right. Not sure what the right comment is though. https://codereview.chromium.org/14556015/diff/213003/chrome/browser/extension... File chrome/browser/extensions/api/media_galleries_private/media_galleries_eject_apitest.cc (right): https://codereview.chromium.org/14556015/diff/213003/chrome/browser/extension... chrome/browser/extensions/api/media_galleries_private/media_galleries_eject_apitest.cc:8: #include "base/run_loop.h" not needed? https://codereview.chromium.org/14556015/diff/213003/chrome/browser/extension... File chrome/browser/extensions/api/media_galleries_private/media_galleries_private_api.cc (right): https://codereview.chromium.org/14556015/diff/213003/chrome/browser/extension... chrome/browser/extensions/api/media_galleries_private/media_galleries_private_api.cc:338: DCHECK(chrome::StorageMonitor::GetInstance()->IsInitialized()); GetGalleryFilePathAndId uses preferences, so move the DCHECK to the top of the method. https://codereview.chromium.org/14556015/diff/213003/chrome/browser/extension... chrome/browser/extensions/api/media_galleries_private/media_galleries_private_api.cc:424: DCHECK(chrome::StorageMonitor::GetInstance()->IsInitialized()); and here https://codereview.chromium.org/14556015/diff/213003/chrome/browser/extension... File chrome/browser/extensions/extension_test_message_listener.cc (right): https://codereview.chromium.org/14556015/diff/213003/chrome/browser/extension... chrome/browser/extensions/extension_test_message_listener.cc:52: VLOG(1) << "Message from JS: " << content; Remove https://codereview.chromium.org/14556015/diff/213003/chrome/browser/media_gal... File chrome/browser/media_galleries/media_file_system_registry.cc (left): https://codereview.chromium.org/14556015/diff/213003/chrome/browser/media_gal... chrome/browser/media_galleries/media_file_system_registry.cc:471: // TODO(gbillock): Move this stanza to MediaGalleriesPreferences init code. I think we still want to do this, leave in todo? https://codereview.chromium.org/14556015/diff/213003/chrome/browser/media_gal... File chrome/browser/media_galleries/media_galleries_dialog_controller.cc (right): https://codereview.chromium.org/14556015/diff/213003/chrome/browser/media_gal... chrome/browser/media_galleries/media_galleries_dialog_controller.cc:102: // Note: Passing unretained pointer is safe, since the dialog controller nit: Remove "Note:" a comment is already a Note. The comment doesn't exactly explain why it is true. Probably should say that the controller owns itself. https://codereview.chromium.org/14556015/diff/213003/chrome/browser/storage_m... File chrome/browser/storage_monitor/storage_monitor.cc (right): https://codereview.chromium.org/14556015/diff/213003/chrome/browser/storage_m... chrome/browser/storage_monitor/storage_monitor.cc:65: if (initialized_) { Add a check that this is only called from a single thread. Otherwise we need a lock here. https://codereview.chromium.org/14556015/diff/213003/chrome/browser/storage_m... File chrome/browser/storage_monitor/storage_monitor.h (right): https://codereview.chromium.org/14556015/diff/213003/chrome/browser/storage_m... chrome/browser/storage_monitor/storage_monitor.h:42: // attached volumes, can be done lazily at first use through the async nit: can be done => are done https://codereview.chromium.org/14556015/diff/213003/chrome/browser/storage_m... chrome/browser/storage_monitor/storage_monitor.h:46: // For platform implementations, the intention is that notifications be sent This implies that we try to suppress duplicates, but may not get all of them. But we don't try. Since the current consumers aren't in this situation, just update the comment to say that duplicates would be sent. https://codereview.chromium.org/14556015/diff/213003/chrome/browser/storage_m... chrome/browser/storage_monitor/storage_monitor.h:164: bool initializing_; nit: instead of two bools, an enum might be better. Feel free to do in a follow up CL. https://codereview.chromium.org/14556015/diff/213003/chrome/browser/ui/webui/... File chrome/browser/ui/webui/options/media_galleries_handler.cc (right): https://codereview.chromium.org/14556015/diff/213003/chrome/browser/ui/webui/... chrome/browser/ui/webui/options/media_galleries_handler.cc:62: base::Bind(&MediaGalleriesHandler::OnGalleriesChanged, This registration effectively assumes that StorageMonitor is already up and running.... I think we need to init StorageMonitor just a bit earlier for MediaGalleriesHandler. https://codereview.chromium.org/14556015/diff/213003/chrome/browser/ui/webui/... File chrome/browser/ui/webui/options/media_galleries_handler.h (right): https://codereview.chromium.org/14556015/diff/213003/chrome/browser/ui/webui/... chrome/browser/ui/webui/options/media_galleries_handler.h:11: #include "chrome/browser/media_galleries/media_galleries_preferences.h" remove
https://codereview.chromium.org/14556015/diff/213003/chrome/browser/extension... File chrome/browser/extensions/api/media_galleries/media_galleries_api.h (right): https://codereview.chromium.org/14556015/diff/213003/chrome/browser/extension... chrome/browser/extensions/api/media_galleries/media_galleries_api.h:32: // Bottom half for RunImpl, invoked after the storage monitor is initialized. On 2013/05/31 18:05:03, vandebo wrote: > nit: Since RunImpl was already async and posts tasks with callbacks, to call > this the bottom half is not quite right. Not sure what the right comment is > though. Hmm. I don't think of bottom half being that specific. And besides, RunImpl is getting called synchronously. https://codereview.chromium.org/14556015/diff/213003/chrome/browser/extension... File chrome/browser/extensions/api/media_galleries_private/media_galleries_eject_apitest.cc (right): https://codereview.chromium.org/14556015/diff/213003/chrome/browser/extension... chrome/browser/extensions/api/media_galleries_private/media_galleries_eject_apitest.cc:8: #include "base/run_loop.h" removed On 2013/05/31 18:05:03, vandebo wrote: > not needed? https://codereview.chromium.org/14556015/diff/213003/chrome/browser/extension... File chrome/browser/extensions/api/media_galleries_private/media_galleries_private_api.cc (right): https://codereview.chromium.org/14556015/diff/213003/chrome/browser/extension... chrome/browser/extensions/api/media_galleries_private/media_galleries_private_api.cc:338: DCHECK(chrome::StorageMonitor::GetInstance()->IsInitialized()); On 2013/05/31 18:05:03, vandebo wrote: > GetGalleryFilePathAndId uses preferences, so move the DCHECK to the top of the > method. Done. https://codereview.chromium.org/14556015/diff/213003/chrome/browser/extension... chrome/browser/extensions/api/media_galleries_private/media_galleries_private_api.cc:424: DCHECK(chrome::StorageMonitor::GetInstance()->IsInitialized()); On 2013/05/31 18:05:03, vandebo wrote: > and here Done. https://codereview.chromium.org/14556015/diff/213003/chrome/browser/extension... File chrome/browser/extensions/extension_test_message_listener.cc (right): https://codereview.chromium.org/14556015/diff/213003/chrome/browser/extension... chrome/browser/extensions/extension_test_message_listener.cc:52: VLOG(1) << "Message from JS: " << content; On 2013/05/31 18:05:03, vandebo wrote: > Remove Yeah. I might send this in separately, though. It is kind of convenient. :-) https://codereview.chromium.org/14556015/diff/213003/chrome/browser/media_gal... File chrome/browser/media_galleries/media_file_system_registry.cc (left): https://codereview.chromium.org/14556015/diff/213003/chrome/browser/media_gal... chrome/browser/media_galleries/media_file_system_registry.cc:471: // TODO(gbillock): Move this stanza to MediaGalleriesPreferences init code. On 2013/05/31 18:05:03, vandebo wrote: > I think we still want to do this, leave in todo? Yeah, I think this was impossible with the getting preferences async, but may still be doable now. https://codereview.chromium.org/14556015/diff/213003/chrome/browser/media_gal... File chrome/browser/media_galleries/media_galleries_dialog_controller.cc (right): https://codereview.chromium.org/14556015/diff/213003/chrome/browser/media_gal... chrome/browser/media_galleries/media_galleries_dialog_controller.cc:102: // Note: Passing unretained pointer is safe, since the dialog controller On 2013/05/31 18:05:03, vandebo wrote: > nit: Remove "Note:" a comment is already a Note. The comment doesn't exactly > explain why it is true. Probably should say that the controller owns itself. Done. https://codereview.chromium.org/14556015/diff/213003/chrome/browser/storage_m... File chrome/browser/storage_monitor/storage_monitor.cc (right): https://codereview.chromium.org/14556015/diff/213003/chrome/browser/storage_m... chrome/browser/storage_monitor/storage_monitor.cc:65: if (initialized_) { On 2013/05/31 18:05:03, vandebo wrote: > Add a check that this is only called from a single thread. Otherwise we need a > lock here. There's a note in the header. I think I tried a check here in the make-async-init-possible change, but it makes the tests a lot more complicated -- the unit test thread doesn't identify itself as the UI thread, so we'd have to pull in some machinery. Let's do that separately if we need to. https://codereview.chromium.org/14556015/diff/213003/chrome/browser/storage_m... File chrome/browser/storage_monitor/storage_monitor.h (right): https://codereview.chromium.org/14556015/diff/213003/chrome/browser/storage_m... chrome/browser/storage_monitor/storage_monitor.h:46: // For platform implementations, the intention is that notifications be sent On 2013/05/31 18:05:03, vandebo wrote: > This implies that we try to suppress duplicates, but may not get all of them. > But we don't try. Since the current consumers aren't in this situation, just > update the comment to say that duplicates would be sent. OK, this was a bit premature, as we haven't done that yet. We have to remember to update the comment on that change, although I think the gist is going to be the same -- "call GetAttachedStorage and register a listener to get everything, but you may get duplicates". https://codereview.chromium.org/14556015/diff/213003/chrome/browser/storage_m... chrome/browser/storage_monitor/storage_monitor.h:164: bool initializing_; On 2013/05/31 18:05:03, vandebo wrote: > nit: instead of two bools, an enum might be better. Feel free to do in a follow > up CL. I think two bools are easier to understand -- one governs internally-observable state, the other is the externally-observable state. https://codereview.chromium.org/14556015/diff/213003/chrome/browser/ui/webui/... File chrome/browser/ui/webui/options/media_galleries_handler.cc (right): https://codereview.chromium.org/14556015/diff/213003/chrome/browser/ui/webui/... chrome/browser/ui/webui/options/media_galleries_handler.cc:62: base::Bind(&MediaGalleriesHandler::OnGalleriesChanged, On 2013/05/31 18:05:03, vandebo wrote: > This registration effectively assumes that StorageMonitor is already up and > running.... I think we need to init StorageMonitor just a bit earlier for > MediaGalleriesHandler. Yeah, I think you're right. This isn't specifically bound to the prefs currently, so it just won't get notifications until the thing is initialized later, but we're going to move to directly observing it, in which case we'll want the initialization before that. Also, the RegisterMessages needs similar treatment. The calls to InitializePage and RegisterMessages are coming in through JS-initiated calls, both async, so while I suspect we are unlikely ever to get one before InitializePage, I think it's best to be defensive here while we're thinking carefully about it. https://codereview.chromium.org/14556015/diff/213003/chrome/browser/ui/webui/... File chrome/browser/ui/webui/options/media_galleries_handler.h (right): https://codereview.chromium.org/14556015/diff/213003/chrome/browser/ui/webui/... chrome/browser/ui/webui/options/media_galleries_handler.h:11: #include "chrome/browser/media_galleries/media_galleries_preferences.h" On 2013/05/31 18:05:03, vandebo wrote: > remove Done.
LGTM https://codereview.chromium.org/14556015/diff/213003/chrome/browser/storage_m... File chrome/browser/storage_monitor/storage_monitor.cc (right): https://codereview.chromium.org/14556015/diff/213003/chrome/browser/storage_m... chrome/browser/storage_monitor/storage_monitor.cc:65: if (initialized_) { On 2013/06/01 01:48:46, Greg Billock wrote: > On 2013/05/31 18:05:03, vandebo wrote: > > Add a check that this is only called from a single thread. Otherwise we need > a > > lock here. > > There's a note in the header. I think I tried a check here in the > make-async-init-possible change, but it makes the tests a lot more complicated > -- the unit test thread doesn't identify itself as the UI thread, so we'd have > to pull in some machinery. Let's do that separately if we need to. I think you can use base/threading/thread_checker.h Didn't mention it last time because I couldn't remember the name at the time. https://codereview.chromium.org/14556015/diff/213003/chrome/browser/storage_m... File chrome/browser/storage_monitor/storage_monitor.h (right): https://codereview.chromium.org/14556015/diff/213003/chrome/browser/storage_m... chrome/browser/storage_monitor/storage_monitor.h:164: bool initializing_; On 2013/06/01 01:48:46, Greg Billock wrote: > On 2013/05/31 18:05:03, vandebo wrote: > > nit: instead of two bools, an enum might be better. Feel free to do in a > follow > > up CL. > > I think two bools are easier to understand -- one governs internally-observable > state, the other is the externally-observable state. Together they specify the state of the object. If you want a subset of that state to be publicly visible, that's easy enough to do. Further, some combinations don't make sense, which argues for an enum. i.e. initialized_ == true && initializing_ == true
https://codereview.chromium.org/14556015/diff/242001/chrome/browser/extension... File chrome/browser/extensions/api/media_galleries_private/media_galleries_private_api.h (right): https://codereview.chromium.org/14556015/diff/242001/chrome/browser/extension... chrome/browser/extensions/api/media_galleries_private/media_galleries_private_api.h:157: : public SyncExtensionFunction { async https://codereview.chromium.org/14556015/diff/242001/chrome/browser/media_gal... File chrome/browser/media_galleries/media_file_system_registry_unittest.cc (right): https://codereview.chromium.org/14556015/diff/242001/chrome/browser/media_gal... chrome/browser/media_galleries/media_file_system_registry_unittest.cc:731: #if !defined(OS_WIN) style nit: #if defined(OS_WIN) ... #else ... #endif https://codereview.chromium.org/14556015/diff/242001/chrome/browser/ui/webui/... File chrome/browser/ui/webui/options/media_galleries_handler.h (right): https://codereview.chromium.org/14556015/diff/242001/chrome/browser/ui/webui/... chrome/browser/ui/webui/options/media_galleries_handler.h:44: // Bottom half of |InitializePage| after async call to initialize nit: InitializePage(), same below.
https://codereview.chromium.org/14556015/diff/213003/chrome/browser/storage_m... File chrome/browser/storage_monitor/storage_monitor.cc (right): https://codereview.chromium.org/14556015/diff/213003/chrome/browser/storage_m... chrome/browser/storage_monitor/storage_monitor.cc:65: if (initialized_) { On 2013/06/01 15:47:24, vandebo wrote: > On 2013/06/01 01:48:46, Greg Billock wrote: > > On 2013/05/31 18:05:03, vandebo wrote: > > > Add a check that this is only called from a single thread. Otherwise we > need > > a > > > lock here. > > > > There's a note in the header. I think I tried a check here in the > > make-async-init-possible change, but it makes the tests a lot more complicated > > -- the unit test thread doesn't identify itself as the UI thread, so we'd have > > to pull in some machinery. Let's do that separately if we need to. > > I think you can use base/threading/thread_checker.h Didn't mention it last time > because I couldn't remember the name at the time. Added. We'll see if the bots complain. https://codereview.chromium.org/14556015/diff/213003/chrome/browser/storage_m... File chrome/browser/storage_monitor/storage_monitor.h (right): https://codereview.chromium.org/14556015/diff/213003/chrome/browser/storage_m... chrome/browser/storage_monitor/storage_monitor.h:164: bool initializing_; On 2013/06/01 15:47:24, vandebo wrote: > On 2013/06/01 01:48:46, Greg Billock wrote: > > On 2013/05/31 18:05:03, vandebo wrote: > > > nit: instead of two bools, an enum might be better. Feel free to do in a > > follow > > > up CL. > > > > I think two bools are easier to understand -- one governs > internally-observable > > state, the other is the externally-observable state. > > Together they specify the state of the object. If you want a subset of that > state to be publicly visible, that's easy enough to do. Further, some > combinations don't make sense, which argues for an enum. i.e. initialized_ == > true && initializing_ == true True, but still I think this is more clear. https://codereview.chromium.org/14556015/diff/242001/chrome/browser/extension... File chrome/browser/extensions/api/media_galleries_private/media_galleries_private_api.h (right): https://codereview.chromium.org/14556015/diff/242001/chrome/browser/extension... chrome/browser/extensions/api/media_galleries_private/media_galleries_private_api.h:157: : public SyncExtensionFunction { On 2013/06/03 06:01:35, Lei Zhang wrote: > async Done. https://codereview.chromium.org/14556015/diff/242001/chrome/browser/media_gal... File chrome/browser/media_galleries/media_file_system_registry_unittest.cc (right): https://codereview.chromium.org/14556015/diff/242001/chrome/browser/media_gal... chrome/browser/media_galleries/media_file_system_registry_unittest.cc:731: #if !defined(OS_WIN) On 2013/06/03 06:01:35, Lei Zhang wrote: > style nit: > #if defined(OS_WIN) > ... > #else > ... > #endif Done. https://codereview.chromium.org/14556015/diff/242001/chrome/browser/ui/webui/... File chrome/browser/ui/webui/options/media_galleries_handler.h (right): https://codereview.chromium.org/14556015/diff/242001/chrome/browser/ui/webui/... chrome/browser/ui/webui/options/media_galleries_handler.h:44: // Bottom half of |InitializePage| after async call to initialize On 2013/06/03 06:01:35, Lei Zhang wrote: > nit: InitializePage(), same below. Done.
https://codereview.chromium.org/14556015/diff/261034/chrome/browser/storage_m... File chrome/browser/storage_monitor/storage_monitor.cc (right): https://codereview.chromium.org/14556015/diff/261034/chrome/browser/storage_m... chrome/browser/storage_monitor/storage_monitor.cc:65: DCHECK(thread_checker_.CalledOnValidThread()); Isn't this always the UI thread?
https://codereview.chromium.org/14556015/diff/261034/chrome/browser/storage_m... File chrome/browser/storage_monitor/storage_monitor.cc (right): https://codereview.chromium.org/14556015/diff/261034/chrome/browser/storage_m... chrome/browser/storage_monitor/storage_monitor.cc:65: DCHECK(thread_checker_.CalledOnValidThread()); On 2013/06/03 20:17:13, Lei Zhang wrote: > Isn't this always the UI thread? Not in tests.
On 2013/06/03 22:33:01, Greg Billock wrote: > https://codereview.chromium.org/14556015/diff/261034/chrome/browser/storage_m... > File chrome/browser/storage_monitor/storage_monitor.cc (right): > > https://codereview.chromium.org/14556015/diff/261034/chrome/browser/storage_m... > chrome/browser/storage_monitor/storage_monitor.cc:65: > DCHECK(thread_checker_.CalledOnValidThread()); > On 2013/06/03 20:17:13, Lei Zhang wrote: > > Isn't this always the UI thread? > > Not in tests. Got it, lgtm.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/gbillock@chromium.org/14556015/261034
Message was sent while issue was closed.
Change committed as 203801 |
