Media Galleries API: Fix MediaGalleriesPreferences finders race.

Media Galleries API: Fix MediaGalleriesPreferences finders race.

Unfortunately, MediaGalleriesPreferences now relies on two async finders during initialization, and currently has a race, where the finders don't return their devices until sometimes unspecified later. This adds an EnsureInitialized mechanic similar to StorageMonitor to solve that. Kind of ugly.

BUG=151701, 295880
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=226127

[tommycli, 2013-09-19 19:02:53]

gbillock/vandebo: This fixes the race, but it also bloats the SLOC count and
makes things a bit more complicated. I'm not super-happy about it, especially
how I'm doing the finder_callbacks_waiting thing...

Suggestions?

[tommycli, 2013-09-20 22:53:24]

gbillock: Since this is based on your StorageMonitor work, you have the
privilege of reviewing these changes.

thestig: Need your approval for chrome/browser/ui

Since this is kind-of a bug, should we aim to get it committed by branch point
EOD monday?

[Lei Zhang, 2013-09-20 23:55:28]

https://codereview.chromium.org/24269007/diff/22002/chrome/browser/media_gall...
File chrome/browser/media_galleries/fileapi/picasa_finder.cc (right):

https://codereview.chromium.org/24269007/diff/22002/chrome/browser/media_gall...
chrome/browser/media_galleries/fileapi/picasa_finder.cc:54:
callback.Run(device_id);
You need to update the FindPicasaDatabase() comment to reflect this new
behavior. FWIW, ITunesFinder does this too.

https://codereview.chromium.org/24269007/diff/22002/chrome/browser/media_gall...
File chrome/browser/media_galleries/media_galleries_test_util.h (right):

https://codereview.chromium.org/24269007/diff/22002/chrome/browser/media_gall...
chrome/browser/media_galleries/media_galleries_test_util.h:25: class
TestMediaDirectoriesOverrides {
Is this needed in this CL?

[Lei Zhang, 2013-09-21 00:12:34]

https://codereview.chromium.org/24269007/diff/22002/chrome/browser/extensions...
File
chrome/browser/extensions/api/media_galleries_private/media_galleries_private_api.cc
(right):

https://codereview.chromium.org/24269007/diff/22002/chrome/browser/extensions...
chrome/browser/extensions/api/media_galleries_private/media_galleries_private_api.cc:125:
// Make sure MediaGalleriesPreferences and StorageMonitor are initialized.
Whether StorageMonitor is initialized or not is an impl detail at this point?

https://codereview.chromium.org/24269007/diff/22002/chrome/browser/extensions...
chrome/browser/extensions/api/media_galleries_private/media_galleries_private_api.cc:131:
->GetPreferences(profile_)
I hate it when things get so long that you have to do this, but if you must,
know that files with foo->\nbar outnumbers files with foo\n->bar 10:1.

https://codereview.chromium.org/24269007/diff/22002/chrome/browser/extensions...
chrome/browser/extensions/api/media_galleries_private/media_galleries_private_api.cc:262:
void MediaGalleriesPrivateRemoveGalleryWatchFunction::OnStorageMonitorInit(
Doesn't this also need to change?

https://codereview.chromium.org/24269007/diff/22002/chrome/browser/media_gall...
File chrome/browser/media_galleries/media_galleries_permissions_unittest.cc
(right):

https://codereview.chromium.org/24269007/diff/22002/chrome/browser/media_gall...
chrome/browser/media_galleries/media_galleries_permissions_unittest.cc:151:
content::TestBrowserThread file_thread_;
ajwong said to "Use TestBrowserThreadBundle" instead.

https://codereview.chromium.org/24269007/diff/22002/chrome/browser/ui/webui/o...
File chrome/browser/ui/webui/options/media_galleries_handler.cc (right):

https://codereview.chromium.org/24269007/diff/22002/chrome/browser/ui/webui/o...
chrome/browser/ui/webui/options/media_galleries_handler.cc:76:
base::Bind(&MediaGalleriesHandler::RegisterOnStorageMonitorInit,
Also change this to just wait for pref initialization?

[Lei Zhang, 2013-09-21 00:36:05]

https://codereview.chromium.org/24269007/diff/26001/chrome/browser/media_gall...
File chrome/browser/media_galleries/media_galleries_preferences.h (right):

https://codereview.chromium.org/24269007/diff/26001/chrome/browser/media_gall...
chrome/browser/media_galleries/media_galleries_preferences.h:306:
base::ThreadChecker thread_checker_;
If we know MediaGalleriesPreferences must run on the UI thread, just check for
that instead.

[vandebo (ex-Chrome), 2013-09-21 01:20:59]

https://codereview.chromium.org/24269007/diff/26001/chrome/browser/extensions...
File
chrome/browser/extensions/api/media_galleries_private/media_galleries_private_api.cc
(right):

https://codereview.chromium.org/24269007/diff/26001/chrome/browser/extensions...
chrome/browser/extensions/api/media_galleries_private/media_galleries_private_api.cc:130:
g_browser_process->media_file_system_registry()
nit: storing either registry or preferences into a temporary would alleviate the
need to wrap at method invocation.

https://codereview.chromium.org/24269007/diff/26001/chrome/browser/media_gall...
File chrome/browser/media_galleries/media_file_system_registry.cc (right):

https://codereview.chromium.org/24269007/diff/26001/chrome/browser/media_gall...
chrome/browser/media_galleries/media_file_system_registry.cc:441:
preferences->EnsureInitialized(base::Bind(
Are we not already sure that preferences have been initialized?  IIRC, the only
way to get to this code is through the API or the dialog...

https://codereview.chromium.org/24269007/diff/26001/chrome/browser/media_gall...
File chrome/browser/media_galleries/media_galleries_preferences.cc (right):

https://codereview.chromium.org/24269007/diff/26001/chrome/browser/media_gall...
chrome/browser/media_galleries/media_galleries_preferences.cc:356: if
(!callback.is_null()) {
nit: always push the callback and then you can remove the initializing_
variable.

https://codereview.chromium.org/24269007/diff/26001/chrome/browser/media_gall...
chrome/browser/media_galleries/media_galleries_preferences.cc:424:
StorageMonitor::GetInstance()->AddObserver(this);
Add the observer before iterating the available storages to preserve existing
behavior and to prevent a race where a device comes in between the two.

https://codereview.chromium.org/24269007/diff/26001/chrome/browser/media_gall...
chrome/browser/media_galleries/media_galleries_preferences.cc:943:
ExtensionPrefs::ScopedListUpdate update(
Style guide seems to prefer this the way it was.
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml#Function_Decla...

[vandebo (ex-Chrome), 2013-09-23 18:35:08]

https://codereview.chromium.org/24269007/diff/68001/chrome/browser/extensions...
File
chrome/browser/extensions/api/media_galleries_private/gallery_watch_state_tracker.cc
(right):

https://codereview.chromium.org/24269007/diff/68001/chrome/browser/extensions...
chrome/browser/extensions/api/media_galleries_private/gallery_watch_state_tracker.cc:94:
DCHECK(preferences->IsInitialized());
nit: this isn't really needed; aside from being in a place where we know it must
already be initialized, the remaining code in this method doesn't need it to be
initialized.

https://codereview.chromium.org/24269007/diff/68001/chrome/browser/extensions...
chrome/browser/extensions/api/media_galleries_private/gallery_watch_state_tracker.cc:263:
DCHECK(preferences->IsInitialized());
Since this is the only place we actually use preferences, this is probably the
only place that it needs to be changed.

https://codereview.chromium.org/24269007/diff/68001/chrome/browser/extensions...
File
chrome/browser/extensions/api/media_galleries_private/media_galleries_private_api.cc
(right):

https://codereview.chromium.org/24269007/diff/68001/chrome/browser/extensions...
chrome/browser/extensions/api/media_galleries_private/media_galleries_private_api.cc:68:
DCHECK(preferences->IsInitialized());
Actually... since preferences checks that its initialized in all of these calls,
no reason to do it twice, just let prefs do it.

https://codereview.chromium.org/24269007/diff/68001/chrome/browser/media_gall...
File chrome/browser/media_galleries/media_file_system_registry.cc (right):

https://codereview.chromium.org/24269007/diff/68001/chrome/browser/media_gall...
chrome/browser/media_galleries/media_file_system_registry.cc:469:
extension_host->GetMediaFileSystems(
nit: style guide says previous layout was fine.

https://codereview.chromium.org/24269007/diff/68001/chrome/browser/media_gall...
File chrome/browser/media_galleries/media_galleries_preferences.cc (right):

https://codereview.chromium.org/24269007/diff/68001/chrome/browser/media_gall...
chrome/browser/media_galleries/media_galleries_preferences.cc:350: if
(initialized_) {
nit: use IsInitialized()

https://codereview.chromium.org/24269007/diff/68001/chrome/browser/media_gall...
chrome/browser/media_galleries/media_galleries_preferences.cc:359:
on_initialize_callbacks_.push_back(callback);
Move above 356

https://codereview.chromium.org/24269007/diff/68001/chrome/browser/media_gall...
chrome/browser/media_galleries/media_galleries_preferences.cc:515: if
(!IsInitialized())
nit: remove if

https://codereview.chromium.org/24269007/diff/68001/chrome/browser/media_gall...
chrome/browser/media_galleries/media_galleries_preferences.cc:977:
ExtensionPrefs::ScopedListUpdate update(
nit: formatting

[tommycli, 2013-09-23 20:39:07]

https://codereview.chromium.org/24269007/diff/22002/chrome/browser/extensions...
File
chrome/browser/extensions/api/media_galleries_private/media_galleries_private_api.cc
(right):

https://codereview.chromium.org/24269007/diff/22002/chrome/browser/extensions...
chrome/browser/extensions/api/media_galleries_private/media_galleries_private_api.cc:125:
// Make sure MediaGalleriesPreferences and StorageMonitor are initialized.
On 2013/09/21 00:12:35, Lei Zhang wrote:
> Whether StorageMonitor is initialized or not is an impl detail at this point?

Done.

https://codereview.chromium.org/24269007/diff/22002/chrome/browser/extensions...
chrome/browser/extensions/api/media_galleries_private/media_galleries_private_api.cc:131:
->GetPreferences(profile_)
On 2013/09/21 00:12:35, Lei Zhang wrote:
> I hate it when things get so long that you have to do this, but if you must,
> know that files with foo->\nbar outnumbers files with foo\n->bar 10:1.

Done. Either way is fine with me. I just did git cl format.

https://codereview.chromium.org/24269007/diff/22002/chrome/browser/extensions...
chrome/browser/extensions/api/media_galleries_private/media_galleries_private_api.cc:262:
void MediaGalleriesPrivateRemoveGalleryWatchFunction::OnStorageMonitorInit(
On 2013/09/21 00:12:35, Lei Zhang wrote:
> Doesn't this also need to change?

Done. Yes it does. Thanks.

https://codereview.chromium.org/24269007/diff/22002/chrome/browser/media_gall...
File chrome/browser/media_galleries/fileapi/picasa_finder.cc (right):

https://codereview.chromium.org/24269007/diff/22002/chrome/browser/media_gall...
chrome/browser/media_galleries/fileapi/picasa_finder.cc:54:
callback.Run(device_id);
On 2013/09/20 23:55:28, Lei Zhang wrote:
> You need to update the FindPicasaDatabase() comment to reflect this new
> behavior. FWIW, ITunesFinder does this too.

Done.

https://codereview.chromium.org/24269007/diff/22002/chrome/browser/media_gall...
File chrome/browser/media_galleries/media_galleries_permissions_unittest.cc
(right):

https://codereview.chromium.org/24269007/diff/22002/chrome/browser/media_gall...
chrome/browser/media_galleries/media_galleries_permissions_unittest.cc:151:
content::TestBrowserThread file_thread_;
On 2013/09/21 00:12:35, Lei Zhang wrote:
> ajwong said to "Use TestBrowserThreadBundle" instead.

I tried it. Because the base class already spawns a UI thread, the bundle
doesn't (easily) work.

https://codereview.chromium.org/24269007/diff/22002/chrome/browser/media_gall...
File chrome/browser/media_galleries/media_galleries_test_util.h (right):

https://codereview.chromium.org/24269007/diff/22002/chrome/browser/media_gall...
chrome/browser/media_galleries/media_galleries_test_util.h:25: class
TestMediaDirectoriesOverrides {
On 2013/09/20 23:55:28, Lei Zhang wrote:
> Is this needed in this CL?

Done. I took it out.

https://codereview.chromium.org/24269007/diff/22002/chrome/browser/ui/webui/o...
File chrome/browser/ui/webui/options/media_galleries_handler.cc (right):

https://codereview.chromium.org/24269007/diff/22002/chrome/browser/ui/webui/o...
chrome/browser/ui/webui/options/media_galleries_handler.cc:76:
base::Bind(&MediaGalleriesHandler::RegisterOnStorageMonitorInit,
On 2013/09/21 00:12:35, Lei Zhang wrote:
> Also change this to just wait for pref initialization?

Done.

https://codereview.chromium.org/24269007/diff/26001/chrome/browser/extensions...
File
chrome/browser/extensions/api/media_galleries_private/media_galleries_private_api.cc
(right):

https://codereview.chromium.org/24269007/diff/26001/chrome/browser/extensions...
chrome/browser/extensions/api/media_galleries_private/media_galleries_private_api.cc:130:
g_browser_process->media_file_system_registry()
On 2013/09/21 01:20:59, vandebo wrote:
> nit: storing either registry or preferences into a temporary would alleviate
the
> need to wrap at method invocation.

Done.

https://codereview.chromium.org/24269007/diff/26001/chrome/browser/media_gall...
File chrome/browser/media_galleries/media_file_system_registry.cc (right):

https://codereview.chromium.org/24269007/diff/26001/chrome/browser/media_gall...
chrome/browser/media_galleries/media_file_system_registry.cc:441:
preferences->EnsureInitialized(base::Bind(
On 2013/09/21 01:20:59, vandebo wrote:
> Are we not already sure that preferences have been initialized?  IIRC, the
only
> way to get to this code is through the API or the dialog...

I made this assume it was already initialized, and made all the API callsites
initialize the media preferences beforehand.

https://codereview.chromium.org/24269007/diff/26001/chrome/browser/media_gall...
File chrome/browser/media_galleries/media_galleries_preferences.cc (right):

https://codereview.chromium.org/24269007/diff/26001/chrome/browser/media_gall...
chrome/browser/media_galleries/media_galleries_preferences.cc:356: if
(!callback.is_null()) {
On 2013/09/21 01:20:59, vandebo wrote:
> nit: always push the callback and then you can remove the initializing_
> variable.

Done.

https://codereview.chromium.org/24269007/diff/26001/chrome/browser/media_gall...
chrome/browser/media_galleries/media_galleries_preferences.cc:424:
StorageMonitor::GetInstance()->AddObserver(this);
On 2013/09/21 01:20:59, vandebo wrote:
> Add the observer before iterating the available storages to preserve existing
> behavior and to prevent a race where a device comes in between the two.

Done.

https://codereview.chromium.org/24269007/diff/26001/chrome/browser/media_gall...
chrome/browser/media_galleries/media_galleries_preferences.cc:943:
ExtensionPrefs::ScopedListUpdate update(
On 2013/09/21 01:20:59, vandebo wrote:
> Style guide seems to prefer this the way it was.
>
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml#Function_Decla...

Done.

https://codereview.chromium.org/24269007/diff/26001/chrome/browser/media_gall...
File chrome/browser/media_galleries/media_galleries_preferences.h (right):

https://codereview.chromium.org/24269007/diff/26001/chrome/browser/media_gall...
chrome/browser/media_galleries/media_galleries_preferences.h:306:
base::ThreadChecker thread_checker_;
On 2013/09/21 00:36:06, Lei Zhang wrote:
> If we know MediaGalleriesPreferences must run on the UI thread, just check for
> that instead.

Done.

https://codereview.chromium.org/24269007/diff/68001/chrome/browser/extensions...
File
chrome/browser/extensions/api/media_galleries_private/gallery_watch_state_tracker.cc
(right):

https://codereview.chromium.org/24269007/diff/68001/chrome/browser/extensions...
chrome/browser/extensions/api/media_galleries_private/gallery_watch_state_tracker.cc:94:
DCHECK(preferences->IsInitialized());
On 2013/09/23 18:35:13, vandebo wrote:
> nit: this isn't really needed; aside from being in a place where we know it
must
> already be initialized, the remaining code in this method doesn't need it to
be
> initialized.

Done.

https://codereview.chromium.org/24269007/diff/68001/chrome/browser/extensions...
chrome/browser/extensions/api/media_galleries_private/gallery_watch_state_tracker.cc:263:
DCHECK(preferences->IsInitialized());
On 2013/09/23 18:35:13, vandebo wrote:
> Since this is the only place we actually use preferences, this is probably the
> only place that it needs to be changed.

Done.

https://codereview.chromium.org/24269007/diff/68001/chrome/browser/extensions...
File
chrome/browser/extensions/api/media_galleries_private/media_galleries_private_api.cc
(right):

https://codereview.chromium.org/24269007/diff/68001/chrome/browser/extensions...
chrome/browser/extensions/api/media_galleries_private/media_galleries_private_api.cc:68:
DCHECK(preferences->IsInitialized());
On 2013/09/23 18:35:13, vandebo wrote:
> Actually... since preferences checks that its initialized in all of these
calls,
> no reason to do it twice, just let prefs do it.

Done.

https://codereview.chromium.org/24269007/diff/68001/chrome/browser/media_gall...
File chrome/browser/media_galleries/media_file_system_registry.cc (right):

https://codereview.chromium.org/24269007/diff/68001/chrome/browser/media_gall...
chrome/browser/media_galleries/media_file_system_registry.cc:469:
extension_host->GetMediaFileSystems(
On 2013/09/23 18:35:13, vandebo wrote:
> nit: style guide says previous layout was fine.

Done.

https://codereview.chromium.org/24269007/diff/68001/chrome/browser/media_gall...
File chrome/browser/media_galleries/media_galleries_preferences.cc (right):

https://codereview.chromium.org/24269007/diff/68001/chrome/browser/media_gall...
chrome/browser/media_galleries/media_galleries_preferences.cc:350: if
(initialized_) {
On 2013/09/23 18:35:13, vandebo wrote:
> nit: use IsInitialized()

Done.

https://codereview.chromium.org/24269007/diff/68001/chrome/browser/media_gall...
chrome/browser/media_galleries/media_galleries_preferences.cc:359:
on_initialize_callbacks_.push_back(callback);
On 2013/09/23 18:35:13, vandebo wrote:
> Move above 356

Done.

https://codereview.chromium.org/24269007/diff/68001/chrome/browser/media_gall...
chrome/browser/media_galleries/media_galleries_preferences.cc:515: if
(!IsInitialized())
On 2013/09/23 18:35:13, vandebo wrote:
> nit: remove if

Done.

https://codereview.chromium.org/24269007/diff/68001/chrome/browser/media_gall...
chrome/browser/media_galleries/media_galleries_preferences.cc:977:
ExtensionPrefs::ScopedListUpdate update(
On 2013/09/23 18:35:13, vandebo wrote:
> nit: formatting

Done.

[tommycli, 2013-09-26 16:20:00]

ping OWNER/s for review

[vandebo (ex-Chrome), 2013-09-26 22:42:36]

LGTM with some nits.

https://codereview.chromium.org/24269007/diff/122001/chrome/browser/extension...
File
chrome/browser/extensions/api/media_galleries_private/gallery_watch_state_tracker.cc
(right):

https://codereview.chromium.org/24269007/diff/122001/chrome/browser/extension...
chrome/browser/extensions/api/media_galleries_private/gallery_watch_state_tracker.cc:261:
DCHECK(preferences->IsInitialized());
nit: remove

https://codereview.chromium.org/24269007/diff/122001/chrome/browser/media_gal...
File chrome/browser/media_galleries/media_file_system_registry.cc (right):

https://codereview.chromium.org/24269007/diff/122001/chrome/browser/media_gal...
chrome/browser/media_galleries/media_file_system_registry.cc:441:
DCHECK(preferences->IsInitialized());
nit: remove

https://codereview.chromium.org/24269007/diff/122001/chrome/browser/media_gal...
chrome/browser/media_galleries/media_file_system_registry.cc:722:
DCHECK(preferences->IsInitialized());
nit: remove

https://codereview.chromium.org/24269007/diff/122001/chrome/browser/media_gal...
File chrome/browser/media_galleries/media_galleries_preferences.cc (right):

https://codereview.chromium.org/24269007/diff/122001/chrome/browser/media_gal...
chrome/browser/media_galleries/media_galleries_preferences.cc:362:
pre_initialization_callbacks_waiting_ = 2;
nit: I think it would be better to increment this on the line just before the
callback is passed in, i.e. 364, 369.  It would make it clearer where the number
is coming from and harder for it to get out of sync.

https://codereview.chromium.org/24269007/diff/122001/chrome/browser/ui/webui/...
File chrome/browser/ui/webui/options/media_galleries_handler.cc (right):

https://codereview.chromium.org/24269007/diff/122001/chrome/browser/ui/webui/...
chrome/browser/ui/webui/options/media_galleries_handler.cc:99:
DCHECK(preferences->IsInitialized());
nit: remove

https://codereview.chromium.org/24269007/diff/122001/chrome/browser/ui/webui/...
chrome/browser/ui/webui/options/media_galleries_handler.cc:148:
DCHECK(preferences->IsInitialized());
nit: remove

https://codereview.chromium.org/24269007/diff/122001/chrome/browser/ui/webui/...
chrome/browser/ui/webui/options/media_galleries_handler.cc:159:
DCHECK(preferences->IsInitialized());
nit: remove

[tommycli, 2013-09-26 23:53:32]

One of the below is not a 'Done'. Ima give you a chance to object before I check
CQ.

https://codereview.chromium.org/24269007/diff/122001/chrome/browser/extension...
File
chrome/browser/extensions/api/media_galleries_private/gallery_watch_state_tracker.cc
(right):

https://codereview.chromium.org/24269007/diff/122001/chrome/browser/extension...
chrome/browser/extensions/api/media_galleries_private/gallery_watch_state_tracker.cc:261:
DCHECK(preferences->IsInitialized());
On 2013/09/26 22:42:36, vandebo wrote:
> nit: remove

Done.

https://codereview.chromium.org/24269007/diff/122001/chrome/browser/media_gal...
File chrome/browser/media_galleries/media_file_system_registry.cc (right):

https://codereview.chromium.org/24269007/diff/122001/chrome/browser/media_gal...
chrome/browser/media_galleries/media_file_system_registry.cc:441:
DCHECK(preferences->IsInitialized());
On 2013/09/26 22:42:36, vandebo wrote:
> nit: remove

Done.

https://codereview.chromium.org/24269007/diff/122001/chrome/browser/media_gal...
chrome/browser/media_galleries/media_file_system_registry.cc:722:
DCHECK(preferences->IsInitialized());
On 2013/09/26 22:42:36, vandebo wrote:
> nit: remove

Done.

https://codereview.chromium.org/24269007/diff/122001/chrome/browser/media_gal...
File chrome/browser/media_galleries/media_galleries_preferences.cc (right):

https://codereview.chromium.org/24269007/diff/122001/chrome/browser/media_gal...
chrome/browser/media_galleries/media_galleries_preferences.cc:362:
pre_initialization_callbacks_waiting_ = 2;
On 2013/09/26 22:42:36, vandebo wrote:
> nit: I think it would be better to increment this on the line just before the
> callback is passed in, i.e. 364, 369.  It would make it clearer where the
number
> is coming from and harder for it to get out of sync.

So this is ... haha...

I originally incremented it.

Unfortunately, StorageMonitor was already ready and called the callback
immediately, decrementing it to zero and calling FinishInitialization
immediately...

The 'real' solution is probably not not use a simple integer like this and use
Scheduler. (https://codereview.chromium.org/24298002/)

https://codereview.chromium.org/24269007/diff/122001/chrome/browser/ui/webui/...
File chrome/browser/ui/webui/options/media_galleries_handler.cc (right):

https://codereview.chromium.org/24269007/diff/122001/chrome/browser/ui/webui/...
chrome/browser/ui/webui/options/media_galleries_handler.cc:99:
DCHECK(preferences->IsInitialized());
On 2013/09/26 22:42:36, vandebo wrote:
> nit: remove

Done.

https://codereview.chromium.org/24269007/diff/122001/chrome/browser/ui/webui/...
chrome/browser/ui/webui/options/media_galleries_handler.cc:148:
DCHECK(preferences->IsInitialized());
On 2013/09/26 22:42:36, vandebo wrote:
> nit: remove

Done.

https://codereview.chromium.org/24269007/diff/122001/chrome/browser/ui/webui/...
chrome/browser/ui/webui/options/media_galleries_handler.cc:159:
DCHECK(preferences->IsInitialized());
On 2013/09/26 22:42:36, vandebo wrote:
> nit: remove

Done.

[Lei Zhang, 2013-09-27 00:24:25]

https://codereview.chromium.org/24269007/diff/136001/chrome/browser/extension...
File chrome/browser/extensions/api/media_galleries/media_galleries_api.cc
(right):

https://codereview.chromium.org/24269007/diff/136001/chrome/browser/extension...
chrome/browser/extensions/api/media_galleries/media_galleries_api.cc:247:
DCHECK(StorageMonitor::GetInstance()->IsInitialized());
This file probably shouldn't reference StorageMonitor anymore. Remove / change?

https://codereview.chromium.org/24269007/diff/136001/chrome/browser/extension...
File
chrome/browser/extensions/api/media_galleries_private/media_galleries_private_api.cc
(right):

https://codereview.chromium.org/24269007/diff/136001/chrome/browser/extension...
chrome/browser/extensions/api/media_galleries_private/media_galleries_private_api.cc:267:
DCHECK(StorageMonitor::GetInstance()->IsInitialized());
Ditto in this file, does this dcheck make sense now?

[vandebo (ex-Chrome), 2013-09-27 17:07:27]

https://codereview.chromium.org/24269007/diff/122001/chrome/browser/media_gal...
File chrome/browser/media_galleries/media_galleries_preferences.cc (right):

https://codereview.chromium.org/24269007/diff/122001/chrome/browser/media_gal...
chrome/browser/media_galleries/media_galleries_preferences.cc:362:
pre_initialization_callbacks_waiting_ = 2;
On 2013/09/26 23:53:33, tommycli wrote:
> On 2013/09/26 22:42:36, vandebo wrote:
> > nit: I think it would be better to increment this on the line just before
the
> > callback is passed in, i.e. 364, 369.  It would make it clearer where the
> number
> > is coming from and harder for it to get out of sync.
> 
> So this is ... haha...
> 
> I originally incremented it.
> 
> Unfortunately, StorageMonitor was already ready and called the callback
> immediately, decrementing it to zero and calling FinishInitialization
> immediately...
> 
> The 'real' solution is probably not not use a simple integer like this and use
> Scheduler. (https://codereview.chromium.org/24298002/)

I see.  Please add a comment explaining that this has to match the number of
things dispatched below and why you can't do it inline like I suggested.

[tommycli, 2013-09-27 17:54:46]

thestig: I removed all the DCHECKS in OnPreferencesInit methods since it's
pretty obvious and they usually call into a DCHECKed Preferences method anyways.
I changed the StorageMonitor DCHECKs to Preferences DCHECKS in other places.

vandebo: Added a comment.

https://codereview.chromium.org/24269007/diff/122001/chrome/browser/media_gal...
File chrome/browser/media_galleries/media_galleries_preferences.cc (right):

https://codereview.chromium.org/24269007/diff/122001/chrome/browser/media_gal...
chrome/browser/media_galleries/media_galleries_preferences.cc:362:
pre_initialization_callbacks_waiting_ = 2;
On 2013/09/27 17:07:27, vandebo wrote:
> On 2013/09/26 23:53:33, tommycli wrote:
> > On 2013/09/26 22:42:36, vandebo wrote:
> > > nit: I think it would be better to increment this on the line just before
> the
> > > callback is passed in, i.e. 364, 369.  It would make it clearer where the
> > number
> > > is coming from and harder for it to get out of sync.
> > 
> > So this is ... haha...
> > 
> > I originally incremented it.
> > 
> > Unfortunately, StorageMonitor was already ready and called the callback
> > immediately, decrementing it to zero and calling FinishInitialization
> > immediately...
> > 
> > The 'real' solution is probably not not use a simple integer like this and
use
> > Scheduler. (https://codereview.chromium.org/24298002/)
> 
> I see.  Please add a comment explaining that this has to match the number of
> things dispatched below and why you can't do it inline like I suggested.

Done.

https://codereview.chromium.org/24269007/diff/136001/chrome/browser/extension...
File chrome/browser/extensions/api/media_galleries/media_galleries_api.cc
(right):

https://codereview.chromium.org/24269007/diff/136001/chrome/browser/extension...
chrome/browser/extensions/api/media_galleries/media_galleries_api.cc:247:
DCHECK(StorageMonitor::GetInstance()->IsInitialized());
On 2013/09/27 00:24:26, Lei Zhang wrote:
> This file probably shouldn't reference StorageMonitor anymore. Remove /
change?

Done.

https://codereview.chromium.org/24269007/diff/136001/chrome/browser/extension...
File
chrome/browser/extensions/api/media_galleries_private/media_galleries_private_api.cc
(right):

https://codereview.chromium.org/24269007/diff/136001/chrome/browser/extension...
chrome/browser/extensions/api/media_galleries_private/media_galleries_private_api.cc:267:
DCHECK(StorageMonitor::GetInstance()->IsInitialized());
On 2013/09/27 00:24:26, Lei Zhang wrote:
> Ditto in this file, does this dcheck make sense now?

Done.

[vandebo (ex-Chrome), 2013-09-27 18:33:53]

I'm happy - LGTM

[commit-bot: I haz the power, 2013-09-27 19:34:12]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/tommycli@chromium.org/24269007/184001

[commit-bot: I haz the power, 2013-09-27 19:34:23]

Failed to apply patch for
chrome/browser/media_galleries/media_galleries_preferences.cc:
While running patch -p1 --forward --force --no-backup-if-mismatch;
  patching file chrome/browser/media_galleries/media_galleries_preferences.cc
  Hunk #2 succeeded at 33 (offset 1 line).
  Hunk #3 FAILED at 346.
  Hunk #4 FAILED at 444.
  Hunk #5 succeeded at 510 (offset 19 lines).
  Hunk #6 succeeded at 538 (offset 19 lines).
  Hunk #7 succeeded at 597 (offset 19 lines).
  Hunk #8 succeeded at 616 (offset 19 lines).
  Hunk #9 succeeded at 749 (offset 19 lines).
  Hunk #10 succeeded at 769 (offset 19 lines).
  Hunk #11 succeeded at 805 (offset 19 lines).
  Hunk #12 succeeded at 840 (offset 19 lines).
  Hunk #13 succeeded at 871 (offset 19 lines).
  Hunk #14 succeeded at 904 (offset 19 lines).
  Hunk #15 succeeded at 942 (offset 19 lines).
  Hunk #16 succeeded at 969 (offset 19 lines).
  Hunk #17 succeeded at 994 (offset 19 lines).
  Hunk #18 succeeded at 1012 (offset 19 lines).
  Hunk #19 succeeded at 1020 (offset 19 lines).
  2 out of 19 hunks FAILED -- saving rejects to file
chrome/browser/media_galleries/media_galleries_preferences.cc.rej

Patch:       chrome/browser/media_galleries/media_galleries_preferences.cc
Index: chrome/browser/media_galleries/media_galleries_preferences.cc
diff --git a/chrome/browser/media_galleries/media_galleries_preferences.cc
b/chrome/browser/media_galleries/media_galleries_preferences.cc
index
ac1e632cc163580878e9f65b5febe0db43e6b0b7..b298a818342fef34941e3bf990c58b8cbd21fa62
100644
--- a/chrome/browser/media_galleries/media_galleries_preferences.cc
+++ b/chrome/browser/media_galleries/media_galleries_preferences.cc
@@ -5,6 +5,7 @@
 #include "chrome/browser/media_galleries/media_galleries_preferences.h"
 
 #include "base/base_paths_posix.h"
+#include "base/callback.h"
 #include "base/i18n/time_formatting.h"
 #include "base/path_service.h"
 #include "base/prefs/pref_service.h"
@@ -31,6 +32,7 @@
 #include "chrome/common/extensions/permissions/permissions_data.h"
 #include "chrome/common/pref_names.h"
 #include "components/user_prefs/pref_registry_syncable.h"
+#include "content/public/browser/browser_thread.h"
 #include "grit/generated_resources.h"
 #include "ui/base/l10n/l10n_util.h"
 #include "ui/base/text/bytes_formatting.h"
@@ -344,34 +346,101 @@
MediaGalleriesPreferences::GalleryChangeObserver::~GalleryChangeObserver() {}
 
 MediaGalleriesPreferences::MediaGalleriesPreferences(Profile* profile)
     : weak_factory_(this),
+      initialized_(false),
+      pre_initialization_callbacks_waiting_(0),
       profile_(profile),
-      extension_prefs_for_testing_(NULL) {
+      extension_prefs_for_testing_(NULL) {}
+
+MediaGalleriesPreferences::~MediaGalleriesPreferences() {
+  if (StorageMonitor::GetInstance())
+    StorageMonitor::GetInstance()->RemoveObserver(this);
+}
+
+void MediaGalleriesPreferences::EnsureInitialized(base::Closure callback) {
+  DCHECK(content::BrowserThread::CurrentlyOn(content::BrowserThread::UI));
+
+  if (IsInitialized()) {
+    if (!callback.is_null())
+      callback.Run();
+    return;
+  }
+
+  on_initialize_callbacks_.push_back(callback);
+  if (on_initialize_callbacks_.size() > 1)
+    return;
+
   AddDefaultGalleriesIfFreshProfile();
 
+  // This counter must match the number of async methods dispatched below.
+  // It cannot be incremented inline with each callback, as some may return
+  // synchronously, decrement the counter to 0, and prematurely trigger
+  // FinishInitialization.
+  pre_initialization_callbacks_waiting_ = 2;
+
+  // Ensure StorageMonitor is initialized.
+  StorageMonitor::GetInstance()->EnsureInitialized(
+      base::Bind(&MediaGalleriesPreferences::OnInitializationCallbackReturned,
+                 weak_factory_.GetWeakPtr()));
+
   // Look for optional default galleries every time.
   itunes::ITunesFinder::FindITunesLibrary(
-      base::Bind(&MediaGalleriesPreferences::OnITunesDeviceID,
+      base::Bind(&MediaGalleriesPreferences::OnFinderDeviceID,
                  weak_factory_.GetWeakPtr()));
 
   // TODO(tommycli): Turn on when Picasa code is ready.
 #if 0
   picasa::PicasaFinder::FindPicasaDatabaseOnUIThread(
-      base::Bind(&MediaGalleriesPreferences::OnPicasaDeviceID,
+      base::Bind(&MediaGalleriesPreferences::OnFinderDeviceID,
                  weak_factory_.GetWeakPtr()));
 #endif
+}
+
+bool MediaGalleriesPreferences::IsInitialized() const { return initialized_; }
+
+Profile* MediaGalleriesPreferences::profile() { return profile_; }
+
+void MediaGalleriesPreferences::OnInitializationCallbackReturned() {
+  DCHECK(!IsInitialized());
+  DCHECK(pre_initialization_callbacks_waiting_ > 0);
+  if (--pre_initialization_callbacks_waiting_ == 0)
+    FinishInitialization();
+}
+
+void MediaGalleriesPreferences::FinishInitialization() {
+  DCHECK(!IsInitialized());
+
+  initialized_ = true;
+
+  StorageMonitor* monitor = StorageMonitor::GetInstance();
+  DCHECK(monitor->IsInitialized());
 
   InitFromPrefs();
 
   StorageMonitor::GetInstance()->AddObserver(this);
-}
 
-MediaGalleriesPreferences::~MediaGalleriesPreferences() {
-  if (StorageMonitor::GetInstance())
-    StorageMonitor::GetInstance()->RemoveObserver(this);
-}
+  std::vector<StorageInfo> existing_devices =
+      monitor->GetAllAvailableStorages();
+  for (size_t i = 0; i < existing_devices.size(); i++) {
+    if (!(StorageInfo::IsMediaDevice(existing_devices[i].device_id()) &&
+          StorageInfo::IsRemovableDevice(existing_devices[i].device_id())))
+      continue;
+    AddGallery(existing_devices[i].device_id(),
+               base::FilePath(),
+               false,
+               existing_devices[i].storage_label(),
+               existing_devices[i].vendor_name(),
+               existing_devices[i].model_name(),
+               existing_devices[i].total_size_in_bytes(),
+               base::Time::Now());
+  }
 
-Profile* MediaGalleriesPreferences::profile() {
-  return profile_;
+  for (std::vector<base::Closure>::iterator iter =
+           on_initialize_callbacks_.begin();
+       iter != on_initialize_callbacks_.end();
+       ++iter) {
+    iter->Run();
+  }
+  on_initialize_callbacks_.clear();
 }
 
 void MediaGalleriesPreferences::AddDefaultGalleriesIfFreshProfile() {
@@ -442,25 +511,23 @@ bool
MediaGalleriesPreferences::UpdateDeviceIDForSingletonType(
   return false;
 }
 
-void MediaGalleriesPreferences::OnITunesDeviceID(const std::string& device_id)
{
-  if (device_id.empty())
-    return;
-  if (!UpdateDeviceIDForSingletonType(device_id)) {
-    AddGalleryInternal(device_id, ASCIIToUTF16(kITunesGalleryName),
-                       base::FilePath(), false /*not user added*/,
-                       string16(), string16(), string16(), 0,
-                       base::Time(), false, 2);
-  }
-}
+void MediaGalleriesPreferences::OnFinderDeviceID(const std::string& device_id)
{
+  if (!device_id.empty() && !UpdateDeviceIDForSingletonType(device_id)) {
+    std::string gallery_name;
+    if (StorageInfo::IsITunesDevice(device_id))
+      gallery_name = kITunesGalleryName;
+    else if (StorageInfo::IsPicasaDevice(device_id))
+      gallery_name = kPicasaGalleryName;
+    else
+      NOTREACHED();
 
-void MediaGalleriesPreferences::OnPicasaDeviceID(const std::string& device_id)
{
-  DCHECK(!device_id.empty());
-  if (!UpdateDeviceIDForSingletonType(device_id)) {
-    AddGalleryInternal(device_id, ASCIIToUTF16(kPicasaGalleryName),
+    AddGalleryInternal(device_id, ASCIIToUTF16(gallery_name),
                        base::FilePath(), false /*not user added*/,
                        string16(), string16(), string16(), 0,
                        base::Time(), false, 2);
   }
+
+  OnInitializationCallbackReturned();
 }
 
 void MediaGalleriesPreferences::InitFromPrefs() {
@@ -489,16 +556,19 @@ void MediaGalleriesPreferences::InitFromPrefs() {
 
 void MediaGalleriesPreferences::AddGalleryChangeObserver(
     GalleryChangeObserver* observer) {
+  DCHECK(IsInitialized());
   gallery_change_observers_.AddObserver(observer);
 }
 
 void MediaGalleriesPreferences::RemoveGalleryChangeObserver(
     GalleryChangeObserver* observer) {
+  DCHECK(IsInitialized());
   gallery_change_observers_.RemoveObserver(observer);
 }
 
 void MediaGalleriesPreferences::OnRemovableStorageAttached(
     const StorageInfo& info) {
+  DCHECK(IsInitialized());
   if (!StorageInfo::IsMediaDevice(info.device_id()))
     return;
 
@@ -514,6 +584,7 @@ void MediaGalleriesPreferences::OnRemovableStorageAttached(
 bool MediaGalleriesPreferences::LookUpGalleryByPath(
     const base::FilePath& path,
     MediaGalleryPrefInfo* gallery_info) const {
+  DCHECK(IsInitialized());
   StorageInfo info;
   base::FilePath relative_path;
   if (!MediaStorageUtil::GetDeviceInfoFromPath(path, &info, &relative_path)) {
@@ -572,6 +643,7 @@ base::FilePath
MediaGalleriesPreferences::LookUpGalleryPathForExtension(
     MediaGalleryPrefId gallery_id,
     const extensions::Extension* extension,
     bool include_unpermitted_galleries) {
+  DCHECK(IsInitialized());
   DCHECK(extension);
   if (!include_unpermitted_galleries &&
       !ContainsKey(GalleriesForExtension(*extension), gallery_id))
@@ -590,6 +662,7 @@ MediaGalleryPrefId MediaGalleriesPreferences::AddGallery(
     const string16& volume_label, const string16& vendor_name,
     const string16& model_name, uint64 total_size_in_bytes,
     base::Time last_attach_time) {
+  DCHECK(IsInitialized());
   return AddGalleryInternal(device_id, string16(), relative_path, user_added,
                             volume_label, vendor_name, model_name,
                             total_size_in_bytes, last_attach_time, true, 2);
@@ -722,6 +795,7 @@ MediaGalleryPrefId
MediaGalleriesPreferences::AddGalleryInternal(
 
 MediaGalleryPrefId MediaGalleriesPreferences::AddGalleryByPath(
     const base::FilePath& path) {
+  DCHECK(IsInitialized());
   MediaGalleryPrefInfo gallery_info;
   if (LookUpGalleryByPath(path, &gallery_info) &&
       gall…
(message too large)

[commit-bot: I haz the power, 2013-09-27 20:17:58]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/tommycli@chromium.org/24269007/206001

[commit-bot: I haz the power, 2013-09-27 22:46:23]

Retried try job too often on chromium_presubmit for step(s) presubmit
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=chromium_p...

[Lei Zhang, 2013-09-27 23:54:43]

lgtm++

https://codereview.chromium.org/24269007/diff/206001/chrome/browser/extension...
File chrome/browser/extensions/api/media_galleries/media_galleries_api.cc
(right):

https://codereview.chromium.org/24269007/diff/206001/chrome/browser/extension...
chrome/browser/extensions/api/media_galleries/media_galleries_api.cc:26:
#include "chrome/browser/storage_monitor/storage_monitor.h"
You can remove storage_monitor.h BTW.

[tommycli, 2013-09-28 00:19:49]

https://codereview.chromium.org/24269007/diff/206001/chrome/browser/extension...
File chrome/browser/extensions/api/media_galleries/media_galleries_api.cc
(right):

https://codereview.chromium.org/24269007/diff/206001/chrome/browser/extension...
chrome/browser/extensions/api/media_galleries/media_galleries_api.cc:26:
#include "chrome/browser/storage_monitor/storage_monitor.h"
On 2013/09/27 23:54:44, Lei Zhang wrote:
> You can remove storage_monitor.h BTW.

Done.

[commit-bot: I haz the power, 2013-09-28 00:37:42]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/tommycli@chromium.org/24269007/220001

[commit-bot: I haz the power, 2013-09-28 04:22:28]

Retried try job too often on win_rel for step(s) unit_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...

[commit-bot: I haz the power, 2013-09-30 16:16:13]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/tommycli@chromium.org/24269007/220001

[commit-bot: I haz the power, 2013-09-30 18:16:20]

Retried try job too often on win7_aura for step(s) unit_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win7_aura&...

[commit-bot: I haz the power, 2013-09-30 20:26:37]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/tommycli@chromium.org/24269007/243001

[commit-bot: I haz the power, 2013-10-01 03:34:05]

Change committed as 226127