[StorageMonitor] Move StorageMonitor ownership to BrowserProcessImpl

[StorageMonitor] Move StorageMonitor ownership to BrowserProcessImpl

This preserves all gross timings of setup and teardown of the storage
monitor.

BUG=None

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=212487

[Greg Billock, 2013-06-12 20:46:59]

thestig, can start review. I need to clean up a bit -- had a bunch of debug
statements to get multi-platform results in order, but the basic guts ought to
all be there now.

[Greg Billock, 2013-06-12 21:24:25]

On 2013/06/12 20:46:59, Greg Billock wrote:
> thestig, can start review. I need to clean up a bit -- had a bunch of debug
> statements to get multi-platform results in order, but the basic guts ought to
> all be there now.

OK. Should be good now; ready to have a look.

[Greg Billock, 2013-06-26 21:21:56]

On 2013/06/12 21:24:25, Greg Billock wrote:
> On 2013/06/12 20:46:59, Greg Billock wrote:
> > thestig, can start review. I need to clean up a bit -- had a bunch of debug
> > statements to get multi-platform results in order, but the basic guts ought
to
> > all be there now.
> 
> OK. Should be good now; ready to have a look.

Branch is passed. Want to have a look, Lei?

[Lei Zhang, 2013-06-27 00:09:38]

Should |g_storage_monitor| in storage_monitor.cc continue to exist? Should it be
removed in favor of g_browser_process->storage_monitor() ?

https://codereview.chromium.org/16703025/diff/60001/chrome/browser/browser_pr...
File chrome/browser/browser_process_impl.cc (right):

https://codereview.chromium.org/16703025/diff/60001/chrome/browser/browser_pr...
chrome/browser/browser_process_impl.cc:925:
storage_monitor_.reset(chrome::StorageMonitor::Create());
Should this also be !OS_IOS, or are we already in a !OS_IOS section?

https://codereview.chromium.org/16703025/diff/60001/chrome/browser/chrome_bro...
File chrome/browser/chrome_browser_main_linux.h (right):

https://codereview.chromium.org/16703025/diff/60001/chrome/browser/chrome_bro...
chrome/browser/chrome_browser_main_linux.h:11: #include
"base/memory/scoped_ptr.h"
remove

https://codereview.chromium.org/16703025/diff/60001/chrome/browser/chrome_bro...
File chrome/browser/chrome_browser_main_mac.h (right):

https://codereview.chromium.org/16703025/diff/60001/chrome/browser/chrome_bro...
chrome/browser/chrome_browser_main_mac.h:8: #include "base/memory/scoped_ptr.h"
remove

https://codereview.chromium.org/16703025/diff/60001/chrome/browser/chrome_bro...
File chrome/browser/chrome_browser_main_win.h (right):

https://codereview.chromium.org/16703025/diff/60001/chrome/browser/chrome_bro...
chrome/browser/chrome_browser_main_win.h:10: #include "base/memory/scoped_ptr.h"
remove

https://codereview.chromium.org/16703025/diff/60001/chrome/browser/extensions...
File
chrome/browser/extensions/api/system_info_storage/storage_info_provider_linux_unittest.cc
(right):

https://codereview.chromium.org/16703025/diff/60001/chrome/browser/extensions...
chrome/browser/extensions/api/system_info_storage/storage_info_provider_linux_unittest.cc:1:
// Copyright (c) 2012 The Chromium Authors. All rights reserved.
Does this test and storage_info_provider_unittest.cc work without a
TestStorageMonitor, or do they need a TestingBrowserProcess?

https://codereview.chromium.org/16703025/diff/60001/chrome/browser/extensions...
File
chrome/browser/extensions/api/system_info_storage/storage_info_provider_unittest.cc
(right):

https://codereview.chromium.org/16703025/diff/60001/chrome/browser/extensions...
chrome/browser/extensions/api/system_info_storage/storage_info_provider_unittest.cc:10:
#include "chrome/browser/storage_monitor/test_storage_monitor.h"
remove

https://codereview.chromium.org/16703025/diff/60001/chrome/browser/extensions...
chrome/browser/extensions/api/system_info_storage/storage_info_provider_unittest.cc:21:
using chrome::test::TestStorageMonitor;
remove

https://codereview.chromium.org/16703025/diff/60001/chrome/browser/media_gall...
File chrome/browser/media_galleries/media_galleries_preferences_unittest.cc
(right):

https://codereview.chromium.org/16703025/diff/60001/chrome/browser/media_gall...
chrome/browser/media_galleries/media_galleries_preferences_unittest.cc:813:
test::TestStorageMonitor monitor;
TestStorageMonitor still used here although the #include got removed.

https://codereview.chromium.org/16703025/diff/60001/chrome/browser/storage_mo...
File chrome/browser/storage_monitor/test_storage_monitor.cc (right):

https://codereview.chromium.org/16703025/diff/60001/chrome/browser/storage_mo...
chrome/browser/storage_monitor/test_storage_monitor.cc:29: void
TestStorageMonitor::RemoveSingleton() {
Why do we have this and RemoveSingletonForTesting?

https://codereview.chromium.org/16703025/diff/60001/chrome/test/base/testing_...
File chrome/test/base/testing_browser_process.cc (right):

https://codereview.chromium.org/16703025/diff/60001/chrome/test/base/testing_...
chrome/test/base/testing_browser_process.cc:53: storage_monitor_(new
chrome::test::TestStorageMonitor),
What's preventing this from only being created when storage_monitor() is called?

[Greg Billock, 2013-06-27 18:25:42]

https://codereview.chromium.org/16703025/diff/60001/chrome/browser/browser_pr...
File chrome/browser/browser_process_impl.cc (right):

https://codereview.chromium.org/16703025/diff/60001/chrome/browser/browser_pr...
chrome/browser/browser_process_impl.cc:925:
storage_monitor_.reset(chrome::StorageMonitor::Create());
On 2013/06/27 00:09:38, Lei Zhang wrote:
> Should this also be !OS_IOS, or are we already in a !OS_IOS section?

Not sure. I'll make this explicit.

https://codereview.chromium.org/16703025/diff/60001/chrome/browser/chrome_bro...
File chrome/browser/chrome_browser_main_linux.h (right):

https://codereview.chromium.org/16703025/diff/60001/chrome/browser/chrome_bro...
chrome/browser/chrome_browser_main_linux.h:11: #include
"base/memory/scoped_ptr.h"
On 2013/06/27 00:09:38, Lei Zhang wrote:
> remove

Done.

https://codereview.chromium.org/16703025/diff/60001/chrome/browser/chrome_bro...
File chrome/browser/chrome_browser_main_mac.h (right):

https://codereview.chromium.org/16703025/diff/60001/chrome/browser/chrome_bro...
chrome/browser/chrome_browser_main_mac.h:8: #include "base/memory/scoped_ptr.h"
On 2013/06/27 00:09:38, Lei Zhang wrote:
> remove

Done.

https://codereview.chromium.org/16703025/diff/60001/chrome/browser/chrome_bro...
File chrome/browser/chrome_browser_main_win.h (right):

https://codereview.chromium.org/16703025/diff/60001/chrome/browser/chrome_bro...
chrome/browser/chrome_browser_main_win.h:10: #include "base/memory/scoped_ptr.h"
On 2013/06/27 00:09:38, Lei Zhang wrote:
> remove

Done.

https://codereview.chromium.org/16703025/diff/60001/chrome/browser/extensions...
File
chrome/browser/extensions/api/system_info_storage/storage_info_provider_linux_unittest.cc
(right):

https://codereview.chromium.org/16703025/diff/60001/chrome/browser/extensions...
chrome/browser/extensions/api/system_info_storage/storage_info_provider_linux_unittest.cc:1:
// Copyright (c) 2012 The Chromium Authors. All rights reserved.
On 2013/06/27 00:09:38, Lei Zhang wrote:
> Does this test and storage_info_provider_unittest.cc work without a
> TestStorageMonitor, or do they need a TestingBrowserProcess?

Oops. I was thinking these got the TestingBrowserProcess, but that's just
browser tests. Reverting some of the unit test changes.

https://codereview.chromium.org/16703025/diff/60001/chrome/browser/extensions...
File
chrome/browser/extensions/api/system_info_storage/storage_info_provider_unittest.cc
(right):

https://codereview.chromium.org/16703025/diff/60001/chrome/browser/extensions...
chrome/browser/extensions/api/system_info_storage/storage_info_provider_unittest.cc:10:
#include "chrome/browser/storage_monitor/test_storage_monitor.h"
On 2013/06/27 00:09:38, Lei Zhang wrote:
> remove

Done.

https://codereview.chromium.org/16703025/diff/60001/chrome/browser/extensions...
chrome/browser/extensions/api/system_info_storage/storage_info_provider_unittest.cc:21:
using chrome::test::TestStorageMonitor;
On 2013/06/27 00:09:38, Lei Zhang wrote:
> remove

Done.

https://codereview.chromium.org/16703025/diff/60001/chrome/browser/media_gall...
File chrome/browser/media_galleries/media_galleries_preferences_unittest.cc
(right):

https://codereview.chromium.org/16703025/diff/60001/chrome/browser/media_gall...
chrome/browser/media_galleries/media_galleries_preferences_unittest.cc:813:
test::TestStorageMonitor monitor;
On 2013/06/27 00:09:38, Lei Zhang wrote:
> TestStorageMonitor still used here although the #include got removed.

Oops. Shouldn't need it.

https://codereview.chromium.org/16703025/diff/60001/chrome/browser/storage_mo...
File chrome/browser/storage_monitor/test_storage_monitor.cc (right):

https://codereview.chromium.org/16703025/diff/60001/chrome/browser/storage_mo...
chrome/browser/storage_monitor/test_storage_monitor.cc:29: void
TestStorageMonitor::RemoveSingleton() {
On 2013/06/27 00:09:38, Lei Zhang wrote:
> Why do we have this and RemoveSingletonForTesting?

Getting rid of the StorageMonitor singleton will let us kill the
RemoveSingletonForTesting method, so it is marked for removal. I was going to do
that in a follow-up, since this change is already quite extensive.

https://codereview.chromium.org/16703025/diff/60001/chrome/test/base/testing_...
File chrome/test/base/testing_browser_process.cc (right):

https://codereview.chromium.org/16703025/diff/60001/chrome/test/base/testing_...
chrome/test/base/testing_browser_process.cc:53: storage_monitor_(new
chrome::test::TestStorageMonitor),
On 2013/06/27 00:09:38, Lei Zhang wrote:
> What's preventing this from only being created when storage_monitor() is
called?

Nothing. I can do that.

[Greg Billock, 2013-06-28 18:34:13]

On 2013/06/27 18:25:42, Greg Billock wrote:
>
https://codereview.chromium.org/16703025/diff/60001/chrome/browser/browser_pr...
> File chrome/browser/browser_process_impl.cc (right):
> 
>
https://codereview.chromium.org/16703025/diff/60001/chrome/browser/browser_pr...
> chrome/browser/browser_process_impl.cc:925:
> storage_monitor_.reset(chrome::StorageMonitor::Create());
> On 2013/06/27 00:09:38, Lei Zhang wrote:
> > Should this also be !OS_IOS, or are we already in a !OS_IOS section?
> 
> Not sure. I'll make this explicit.
> 
>
https://codereview.chromium.org/16703025/diff/60001/chrome/browser/chrome_bro...
> File chrome/browser/chrome_browser_main_linux.h (right):
> 
>
https://codereview.chromium.org/16703025/diff/60001/chrome/browser/chrome_bro...
> chrome/browser/chrome_browser_main_linux.h:11: #include
> "base/memory/scoped_ptr.h"
> On 2013/06/27 00:09:38, Lei Zhang wrote:
> > remove
> 
> Done.
> 
>
https://codereview.chromium.org/16703025/diff/60001/chrome/browser/chrome_bro...
> File chrome/browser/chrome_browser_main_mac.h (right):
> 
>
https://codereview.chromium.org/16703025/diff/60001/chrome/browser/chrome_bro...
> chrome/browser/chrome_browser_main_mac.h:8: #include
"base/memory/scoped_ptr.h"
> On 2013/06/27 00:09:38, Lei Zhang wrote:
> > remove
> 
> Done.
> 
>
https://codereview.chromium.org/16703025/diff/60001/chrome/browser/chrome_bro...
> File chrome/browser/chrome_browser_main_win.h (right):
> 
>
https://codereview.chromium.org/16703025/diff/60001/chrome/browser/chrome_bro...
> chrome/browser/chrome_browser_main_win.h:10: #include
"base/memory/scoped_ptr.h"
> On 2013/06/27 00:09:38, Lei Zhang wrote:
> > remove
> 
> Done.
> 
>
https://codereview.chromium.org/16703025/diff/60001/chrome/browser/extensions...
> File
>
chrome/browser/extensions/api/system_info_storage/storage_info_provider_linux_unittest.cc
> (right):
> 
>
https://codereview.chromium.org/16703025/diff/60001/chrome/browser/extensions...
>
chrome/browser/extensions/api/system_info_storage/storage_info_provider_linux_unittest.cc:1:
> // Copyright (c) 2012 The Chromium Authors. All rights reserved.
> On 2013/06/27 00:09:38, Lei Zhang wrote:
> > Does this test and storage_info_provider_unittest.cc work without a
> > TestStorageMonitor, or do they need a TestingBrowserProcess?
> 
> Oops. I was thinking these got the TestingBrowserProcess, but that's just
> browser tests. Reverting some of the unit test changes.
> 
>
https://codereview.chromium.org/16703025/diff/60001/chrome/browser/extensions...
> File
>
chrome/browser/extensions/api/system_info_storage/storage_info_provider_unittest.cc
> (right):
> 
>
https://codereview.chromium.org/16703025/diff/60001/chrome/browser/extensions...
>
chrome/browser/extensions/api/system_info_storage/storage_info_provider_unittest.cc:10:
> #include "chrome/browser/storage_monitor/test_storage_monitor.h"
> On 2013/06/27 00:09:38, Lei Zhang wrote:
> > remove
> 
> Done.
> 
>
https://codereview.chromium.org/16703025/diff/60001/chrome/browser/extensions...
>
chrome/browser/extensions/api/system_info_storage/storage_info_provider_unittest.cc:21:
> using chrome::test::TestStorageMonitor;
> On 2013/06/27 00:09:38, Lei Zhang wrote:
> > remove
> 
> Done.
> 
>
https://codereview.chromium.org/16703025/diff/60001/chrome/browser/media_gall...
> File chrome/browser/media_galleries/media_galleries_preferences_unittest.cc
> (right):
> 
>
https://codereview.chromium.org/16703025/diff/60001/chrome/browser/media_gall...
> chrome/browser/media_galleries/media_galleries_preferences_unittest.cc:813:
> test::TestStorageMonitor monitor;
> On 2013/06/27 00:09:38, Lei Zhang wrote:
> > TestStorageMonitor still used here although the #include got removed.
> 
> Oops. Shouldn't need it.
> 
>
https://codereview.chromium.org/16703025/diff/60001/chrome/browser/storage_mo...
> File chrome/browser/storage_monitor/test_storage_monitor.cc (right):
> 
>
https://codereview.chromium.org/16703025/diff/60001/chrome/browser/storage_mo...
> chrome/browser/storage_monitor/test_storage_monitor.cc:29: void
> TestStorageMonitor::RemoveSingleton() {
> On 2013/06/27 00:09:38, Lei Zhang wrote:
> > Why do we have this and RemoveSingletonForTesting?
> 
> Getting rid of the StorageMonitor singleton will let us kill the
> RemoveSingletonForTesting method, so it is marked for removal. I was going to
do
> that in a follow-up, since this change is already quite extensive.
> 
>
https://codereview.chromium.org/16703025/diff/60001/chrome/test/base/testing_...
> File chrome/test/base/testing_browser_process.cc (right):
> 
>
https://codereview.chromium.org/16703025/diff/60001/chrome/test/base/testing_...
> chrome/test/base/testing_browser_process.cc:53: storage_monitor_(new
> chrome::test::TestStorageMonitor),
> On 2013/06/27 00:09:38, Lei Zhang wrote:
> > What's preventing this from only being created when storage_monitor() is
> called?
> 
> Nothing. I can do that.

OK, Lei. I took out the storage monitor global. The implications were quite a
bit more invasive than I expected -- the reason is that browser tests don't use
the TestingBrowserProcess, and there's one test where we have to load the test
mock into the real object, which didn't have any support. (In addition, I got
screwed up by the extension fixtures blocking all the normal browser test setup
methods...) I'm not sure why we don't use the TestingBrowserProcess in browser
tests, but there's probably a good reason and I doubt we should change it. The
other most promising option is to insert the mock at
StorageMonitor::GetInstance(), but that's basically equivalent to having the
global there, albeit just for testing purposes. :-( Dissatisfying.

[Lei Zhang, 2013-07-02 23:16:57]

https://codereview.chromium.org/16703025/diff/108001/chrome/browser/browser_p...
File chrome/browser/browser_process_impl.h (right):

https://codereview.chromium.org/16703025/diff/108001/chrome/browser/browser_p...
chrome/browser/browser_process_impl.h:162: void
SetStorageMonitorForTesting(chrome::StorageMonitor* monitor);
I think we can make this public, just like
set_background_mode_manager_for_test() above that was recently added. Probably
should make it take a scoped_ptr for the same reason as
set_background_mode_manager_for_test() - make it clear that the
BrowserProcessImpl is taking ownership of the StorageMonitor.

https://codereview.chromium.org/16703025/diff/108001/chrome/browser/extension...
File
chrome/browser/extensions/api/media_galleries_private/media_galleries_eject_apitest.cc
(right):

https://codereview.chromium.org/16703025/diff/108001/chrome/browser/extension...
chrome/browser/extensions/api/media_galleries_private/media_galleries_eject_apitest.cc:120:
monitor_ = chrome::test::TestStorageMonitor::CreateForBrowserTests();
Just override InProcesBrowserTest::SetUpOnMainThread() and do it there instead
of repeating it in every test case.

https://codereview.chromium.org/16703025/diff/108001/chrome/browser/extension...
File
chrome/browser/extensions/api/media_galleries_private/media_galleries_private_apitest.cc
(right):

https://codereview.chromium.org/16703025/diff/108001/chrome/browser/extension...
chrome/browser/extensions/api/media_galleries_private/media_galleries_private_apitest.cc:112:
chrome::test::TestStorageMonitor::SyncInitialize();
We no longer need to put a TestStorageMonitor in g_browser_process ? Wouldn't we
end up using a real StorageMonitor now? Ditto for SystemInfoStorageApiTest.

https://codereview.chromium.org/16703025/diff/108001/chrome/browser/storage_m...
File chrome/browser/storage_monitor/storage_monitor.h (right):

https://codereview.chromium.org/16703025/diff/108001/chrome/browser/storage_m...
chrome/browser/storage_monitor/storage_monitor.h:75: // Returns a pointer to an
object owned by the BrowserMainParts, with lifetime
update comment

https://codereview.chromium.org/16703025/diff/108001/chrome/browser/storage_m...
chrome/browser/storage_monitor/storage_monitor.h:80: virtual ~StorageMonitor();
nit: blank line after

https://codereview.chromium.org/16703025/diff/108001/chrome/browser/storage_m...
File chrome/browser/storage_monitor/storage_monitor_win_unittest.cc (right):

https://codereview.chromium.org/16703025/diff/108001/chrome/browser/storage_m...
chrome/browser/storage_monitor/storage_monitor_win_unittest.cc:90:
test::TestStorageMonitor::RemoveSingleton();
Is this needed in unit tests where there's probably no g_browser_process ? Ditto
for other unit tests.

https://codereview.chromium.org/16703025/diff/108001/chrome/browser/storage_m...
File chrome/browser/storage_monitor/test_storage_monitor.h (right):

https://codereview.chromium.org/16703025/diff/108001/chrome/browser/storage_m...
chrome/browser/storage_monitor/test_storage_monitor.h:26: static
TestStorageMonitor* Create();
I have trouble telling the difference between Create() and
CreateForBrowserTests(). They seem to do the same thing in slightly different
ways.

https://codereview.chromium.org/16703025/diff/108001/chrome/test/base/testing...
File chrome/test/base/testing_browser_process.h (right):

https://codereview.chromium.org/16703025/diff/108001/chrome/test/base/testing...
chrome/test/base/testing_browser_process.h:128: static void
SetStorageMonitorForBrowserTests(
I don't think we need this. Just put SetStorageMonitorForTesting() in
browser_process.h, and callers can call
g_browser_process->SetStorageMonitorForTesting() directly.

[Greg Billock, 2013-07-03 17:49:17]

https://codereview.chromium.org/16703025/diff/108001/chrome/browser/browser_p...
File chrome/browser/browser_process_impl.h (right):

https://codereview.chromium.org/16703025/diff/108001/chrome/browser/browser_p...
chrome/browser/browser_process_impl.h:162: void
SetStorageMonitorForTesting(chrome::StorageMonitor* monitor);
On 2013/07/02 23:16:57, Lei Zhang wrote:
> I think we can make this public, just like
> set_background_mode_manager_for_test() above that was recently added. Probably
> should make it take a scoped_ptr for the same reason as
> set_background_mode_manager_for_test() - make it clear that the
> BrowserProcessImpl is taking ownership of the StorageMonitor.

Done.

https://codereview.chromium.org/16703025/diff/108001/chrome/browser/extension...
File
chrome/browser/extensions/api/media_galleries_private/media_galleries_eject_apitest.cc
(right):

https://codereview.chromium.org/16703025/diff/108001/chrome/browser/extension...
chrome/browser/extensions/api/media_galleries_private/media_galleries_eject_apitest.cc:120:
monitor_ = chrome::test::TestStorageMonitor::CreateForBrowserTests();
On 2013/07/02 23:16:57, Lei Zhang wrote:
> Just override InProcesBrowserTest::SetUpOnMainThread() and do it there instead
> of repeating it in every test case.

That's what I initially did. There's some kind of blockage upstream, though, and
SetUpOnMainThread never gets called. The extension test fixture is blocking it
by making it non-virtual. I can see the IPBT method get called, but it doesn't
come through. I'm not sure whether that's a mistake or on purpose, but we have
just a couple tests that need it, so it's probably ok to just do it here.

https://codereview.chromium.org/16703025/diff/108001/chrome/browser/extension...
File
chrome/browser/extensions/api/media_galleries_private/media_galleries_private_apitest.cc
(right):

https://codereview.chromium.org/16703025/diff/108001/chrome/browser/extension...
chrome/browser/extensions/api/media_galleries_private/media_galleries_private_apitest.cc:112:
chrome::test::TestStorageMonitor::SyncInitialize();
On 2013/07/02 23:16:57, Lei Zhang wrote:
> We no longer need to put a TestStorageMonitor in g_browser_process ? Wouldn't
we
> end up using a real StorageMonitor now? Ditto for SystemInfoStorageApiTest.

Yes. We end up using the real one. We can sub in the test if we want, but for
this test, we just need it initialized; the real one is fine.

https://codereview.chromium.org/16703025/diff/108001/chrome/browser/storage_m...
File chrome/browser/storage_monitor/storage_monitor.h (right):

https://codereview.chromium.org/16703025/diff/108001/chrome/browser/storage_m...
chrome/browser/storage_monitor/storage_monitor.h:75: // Returns a pointer to an
object owned by the BrowserMainParts, with lifetime
On 2013/07/02 23:16:57, Lei Zhang wrote:
> update comment

Done.

https://codereview.chromium.org/16703025/diff/108001/chrome/browser/storage_m...
chrome/browser/storage_monitor/storage_monitor.h:80: virtual ~StorageMonitor();
On 2013/07/02 23:16:57, Lei Zhang wrote:
> nit: blank line after

Done.

https://codereview.chromium.org/16703025/diff/108001/chrome/browser/storage_m...
File chrome/browser/storage_monitor/storage_monitor_win_unittest.cc (right):

https://codereview.chromium.org/16703025/diff/108001/chrome/browser/storage_m...
chrome/browser/storage_monitor/storage_monitor_win_unittest.cc:90:
test::TestStorageMonitor::RemoveSingleton();
On 2013/07/02 23:16:57, Lei Zhang wrote:
> Is this needed in unit tests where there's probably no g_browser_process ?
Ditto
> for other unit tests.

In unit tests you get the TestingBrowserProcess as part of the chrome suite
fixture. (Then the browser tests explicitly delete it so the BrowserMain() can
make the real one.)

https://codereview.chromium.org/16703025/diff/108001/chrome/browser/storage_m...
File chrome/browser/storage_monitor/test_storage_monitor.h (right):

https://codereview.chromium.org/16703025/diff/108001/chrome/browser/storage_m...
chrome/browser/storage_monitor/test_storage_monitor.h:26: static
TestStorageMonitor* Create();
On 2013/07/02 23:16:57, Lei Zhang wrote:
> I have trouble telling the difference between Create() and
> CreateForBrowserTests(). They seem to do the same thing in slightly different
> ways.

Yes. Create() puts it in the TestingBrowserProcess, and CreateForBrowserTests
puts it into BrowserProcessImpl. I'll clarify this.

https://codereview.chromium.org/16703025/diff/108001/chrome/test/base/testing...
File chrome/test/base/testing_browser_process.h (right):

https://codereview.chromium.org/16703025/diff/108001/chrome/test/base/testing...
chrome/test/base/testing_browser_process.h:128: static void
SetStorageMonitorForBrowserTests(
On 2013/07/02 23:16:57, Lei Zhang wrote:
> I don't think we need this. Just put SetStorageMonitorForTesting() in
> browser_process.h, and callers can call
> g_browser_process->SetStorageMonitorForTesting() directly.

I'd like to keep the testing interface out of browser_process.h

I'll add a comment though.

[Lei Zhang, 2013-07-03 23:25:10]

Some of the unit tests have multiple StorageMonitors existing at the same time.
I'll follow up with some more comments regarding that.

https://codereview.chromium.org/16703025/diff/108001/chrome/browser/extension...
File
chrome/browser/extensions/api/media_galleries_private/media_galleries_eject_apitest.cc
(right):

https://codereview.chromium.org/16703025/diff/108001/chrome/browser/extension...
chrome/browser/extensions/api/media_galleries_private/media_galleries_eject_apitest.cc:120:
monitor_ = chrome::test::TestStorageMonitor::CreateForBrowserTests();
On 2013/07/03 17:49:17, Greg Billock wrote:
> On 2013/07/02 23:16:57, Lei Zhang wrote:
> > Just override InProcesBrowserTest::SetUpOnMainThread() and do it there
instead
> > of repeating it in every test case.
> 
> That's what I initially did. There's some kind of blockage upstream, though,
and
> SetUpOnMainThread never gets called. The extension test fixture is blocking it
> by making it non-virtual. I can see the IPBT method get called, but it doesn't
> come through. I'm not sure whether that's a mistake or on purpose, but we have
> just a couple tests that need it, so it's probably ok to just do it here.

I tried it and it seems to work for me. SetUpOnMainThread() gets called here.

https://codereview.chromium.org/16703025/diff/108001/chrome/test/base/testing...
File chrome/test/base/testing_browser_process.h (right):

https://codereview.chromium.org/16703025/diff/108001/chrome/test/base/testing...
chrome/test/base/testing_browser_process.h:128: static void
SetStorageMonitorForBrowserTests(
On 2013/07/03 17:49:17, Greg Billock wrote:
> On 2013/07/02 23:16:57, Lei Zhang wrote:
> > I don't think we need this. Just put SetStorageMonitorForTesting() in
> > browser_process.h, and callers can call
> > g_browser_process->SetStorageMonitorForTesting() directly.
> 
> I'd like to keep the testing interface out of browser_process.h
> 
> I'll add a comment though.

set_background_mode_manager_for_test() already exists in browser_process.h. Do
you thinking adding set_storage_monitor_for_test() would help simplify the code
at the expense of adding a test method to the public interface?

https://codereview.chromium.org/16703025/diff/118001/chrome/browser/storage_m...
File chrome/browser/storage_monitor/storage_monitor.h (right):

https://codereview.chromium.org/16703025/diff/118001/chrome/browser/storage_m...
chrome/browser/storage_monitor/storage_monitor.h:80: StorageMonitor();
The ctor should be protected, so one has to use GetInstance()

https://codereview.chromium.org/16703025/diff/118001/chrome/browser/storage_m...
File chrome/browser/storage_monitor/test_storage_monitor.h (right):

https://codereview.chromium.org/16703025/diff/118001/chrome/browser/storage_m...
chrome/browser/storage_monitor/test_storage_monitor.h:24: // Create an
initialize a new TestStorageMonitor, and install it
typo: and

[Lei Zhang, 2013-07-03 23:46:17]

https://codereview.chromium.org/16703025/diff/118001/chrome/browser/storage_m...
File chrome/browser/storage_monitor/storage_monitor_linux_unittest.cc (right):

https://codereview.chromium.org/16703025/diff/118001/chrome/browser/storage_m...
chrome/browser/storage_monitor/storage_monitor_linux_unittest.cc:186:
monitor_.reset(new TestStorageMonitorLinux(mtab_file_, &message_loop_));
We should set the TestBrowserProcess's storage_monitor to the one created here,
and change |monitor_| to a raw pointer, and do
test::TestStorageMonitor::RemoveSingleton() in TearDown() in place of
monitor_.reset();

Otherwise we'll have two StorageMonitors co-existing in this test at some point.

Same pattern exists in StorageMonitor{Win,Mac,ChromeOS}.

[Greg Billock, 2013-07-08 18:50:07]

https://codereview.chromium.org/16703025/diff/108001/chrome/browser/extension...
File
chrome/browser/extensions/api/media_galleries_private/media_galleries_eject_apitest.cc
(right):

https://codereview.chromium.org/16703025/diff/108001/chrome/browser/extension...
chrome/browser/extensions/api/media_galleries_private/media_galleries_eject_apitest.cc:120:
monitor_ = chrome::test::TestStorageMonitor::CreateForBrowserTests();
On 2013/07/03 23:25:10, Lei Zhang wrote:
> On 2013/07/03 17:49:17, Greg Billock wrote:
> > On 2013/07/02 23:16:57, Lei Zhang wrote:
> > > Just override InProcesBrowserTest::SetUpOnMainThread() and do it there
> instead
> > > of repeating it in every test case.
> > 
> > That's what I initially did. There's some kind of blockage upstream, though,
> and
> > SetUpOnMainThread never gets called. The extension test fixture is blocking
it
> > by making it non-virtual. I can see the IPBT method get called, but it
doesn't
> > come through. I'm not sure whether that's a mistake or on purpose, but we
have
> > just a couple tests that need it, so it's probably ok to just do it here.
> 
> I tried it and it seems to work for me. SetUpOnMainThread() gets called here.

Yeah. I have no idea what I was doing wrong. Tried it again and it works fine.
I'd suspect myself of typing "SetupOnMainThread" or something, but I spent a
while trying to figure out what was up, including copy-pasting the method
signature directly from the base class in. Sigh. Anyway, now changed.

https://codereview.chromium.org/16703025/diff/108001/chrome/test/base/testing...
File chrome/test/base/testing_browser_process.h (right):

https://codereview.chromium.org/16703025/diff/108001/chrome/test/base/testing...
chrome/test/base/testing_browser_process.h:128: static void
SetStorageMonitorForBrowserTests(
On 2013/07/03 23:25:10, Lei Zhang wrote:
> On 2013/07/03 17:49:17, Greg Billock wrote:
> > On 2013/07/02 23:16:57, Lei Zhang wrote:
> > > I don't think we need this. Just put SetStorageMonitorForTesting() in
> > > browser_process.h, and callers can call
> > > g_browser_process->SetStorageMonitorForTesting() directly.
> > 
> > I'd like to keep the testing interface out of browser_process.h
> > 
> > I'll add a comment though.
> 
> set_background_mode_manager_for_test() already exists in browser_process.h. Do
> you thinking adding set_storage_monitor_for_test() would help simplify the
code
> at the expense of adding a test method to the public interface?

Yeah, I saw that. I don't think it simplifies that much, though.

https://codereview.chromium.org/16703025/diff/118001/chrome/browser/storage_m...
File chrome/browser/storage_monitor/storage_monitor.h (right):

https://codereview.chromium.org/16703025/diff/118001/chrome/browser/storage_m...
chrome/browser/storage_monitor/storage_monitor.h:80: StorageMonitor();
On 2013/07/03 23:25:10, Lei Zhang wrote:
> The ctor should be protected, so one has to use GetInstance()

Done. Shall I move Create() and friend needed classes as well? Or just leave it?

https://codereview.chromium.org/16703025/diff/118001/chrome/browser/storage_m...
File chrome/browser/storage_monitor/storage_monitor_linux_unittest.cc (right):

https://codereview.chromium.org/16703025/diff/118001/chrome/browser/storage_m...
chrome/browser/storage_monitor/storage_monitor_linux_unittest.cc:186:
monitor_.reset(new TestStorageMonitorLinux(mtab_file_, &message_loop_));
On 2013/07/03 23:46:18, Lei Zhang wrote:
> We should set the TestBrowserProcess's storage_monitor to the one created
here,
> and change |monitor_| to a raw pointer, and do
> test::TestStorageMonitor::RemoveSingleton() in TearDown() in place of
> monitor_.reset();
> 
> Otherwise we'll have two StorageMonitors co-existing in this test at some
point.
> 
> Same pattern exists in StorageMonitor{Win,Mac,ChromeOS}.

True. I think all these tests just use the local storage monitor, though, so it
shouldn't matter that the browser process has a separate one. It's probably a
trap for the unwary though... I can see it being confusing... I'll inject it
into the fixture.

https://codereview.chromium.org/16703025/diff/118001/chrome/browser/storage_m...
File chrome/browser/storage_monitor/test_storage_monitor.h (right):

https://codereview.chromium.org/16703025/diff/118001/chrome/browser/storage_m...
chrome/browser/storage_monitor/test_storage_monitor.h:24: // Create an
initialize a new TestStorageMonitor, and install it
On 2013/07/03 23:25:10, Lei Zhang wrote:
> typo: and

Done.

[vandebo (ex-Chrome), 2013-07-09 15:38:12]

https://codereview.chromium.org/16703025/diff/126001/chrome/browser/browser_p...
File chrome/browser/browser_process_impl.cc (right):

https://codereview.chromium.org/16703025/diff/126001/chrome/browser/browser_p...
chrome/browser/browser_process_impl.cc:279: #if !defined(OS_ANDROID) &&
!defined(OS_IOS)
Without any other context, I would guess this should go between
ExtensionRendererState shutdown and MessageCenter shutdown.  If the names
suggest the right thing to my pattern matching, MediaFileSystemRegistry is
listening for stuff from MessageCenter.

https://codereview.chromium.org/16703025/diff/126001/chrome/browser/browser_p...
chrome/browser/browser_process_impl.cc:939:
storage_monitor_.reset(chrome::StorageMonitor::Create());
Why not create this on first access like MediaFileSystemRegistry ?

https://codereview.chromium.org/16703025/diff/126001/chrome/browser/extension...
File
chrome/browser/extensions/api/media_galleries_private/media_galleries_eject_apitest.cc
(right):

https://codereview.chromium.org/16703025/diff/126001/chrome/browser/extension...
chrome/browser/extensions/api/media_galleries_private/media_galleries_eject_apitest.cc:112:
// Weak ptr to the storage monitor owned by the browser process.
This is not a WeakPtr<>.

It looks like this member is only used in two places.  Can you just get it from
the BrowserProcess in the place you need it?  Comment applies to several unit
tests.

https://codereview.chromium.org/16703025/diff/126001/chrome/browser/media_gal...
File chrome/browser/media_galleries/mac/mtp_device_delegate_impl_mac_unittest.mm
(right):

https://codereview.chromium.org/16703025/diff/126001/chrome/browser/media_gal...
chrome/browser/media_galleries/mac/mtp_device_delegate_impl_mac_unittest.mm:22:
#include "chrome/test/base/testing_browser_process.h"
Needed?

https://codereview.chromium.org/16703025/diff/126001/chrome/browser/storage_m...
File chrome/browser/storage_monitor/storage_monitor_win.h (right):

https://codereview.chromium.org/16703025/diff/126001/chrome/browser/storage_m...
chrome/browser/storage_monitor/storage_monitor_win.h:31:
StorageMonitorWin(VolumeMountWatcherWin* volume_mount_watcher,
Can this stay private?

[Greg Billock, 2013-07-09 17:35:20]

https://codereview.chromium.org/16703025/diff/126001/chrome/browser/browser_p...
File chrome/browser/browser_process_impl.cc (right):

https://codereview.chromium.org/16703025/diff/126001/chrome/browser/browser_p...
chrome/browser/browser_process_impl.cc:279: #if !defined(OS_ANDROID) &&
!defined(OS_IOS)
On 2013/07/09 15:38:12, vandebo wrote:
> Without any other context, I would guess this should go between
> ExtensionRendererState shutdown and MessageCenter shutdown.  If the names
> suggest the right thing to my pattern matching, MediaFileSystemRegistry is
> listening for stuff from MessageCenter.

Is it? OK, moved.

https://codereview.chromium.org/16703025/diff/126001/chrome/browser/browser_p...
chrome/browser/browser_process_impl.cc:939:
storage_monitor_.reset(chrome::StorageMonitor::Create());
On 2013/07/09 15:38:12, vandebo wrote:
> Why not create this on first access like MediaFileSystemRegistry ?

My aim here is to maintain exactly the same startup/shutdown sequence we have
now. I do think we want to change it around -- create on first access and
destroy on BrowserProcess destruction.

https://codereview.chromium.org/16703025/diff/126001/chrome/browser/extension...
File
chrome/browser/extensions/api/media_galleries_private/media_galleries_eject_apitest.cc
(right):

https://codereview.chromium.org/16703025/diff/126001/chrome/browser/extension...
chrome/browser/extensions/api/media_galleries_private/media_galleries_eject_apitest.cc:112:
// Weak ptr to the storage monitor owned by the browser process.
On 2013/07/09 15:38:12, vandebo wrote:
> This is not a WeakPtr<>.
> 
> It looks like this member is only used in two places.  Can you just get it
from
> the BrowserProcess in the place you need it?  Comment applies to several unit
> tests.

The type from BrowserProcess is just the StorageMonitor, so I'd need a cast. I
think keeping it around like this is fine.

I'll change the comment though.

https://codereview.chromium.org/16703025/diff/126001/chrome/browser/media_gal...
File chrome/browser/media_galleries/mac/mtp_device_delegate_impl_mac_unittest.mm
(right):

https://codereview.chromium.org/16703025/diff/126001/chrome/browser/media_gal...
chrome/browser/media_galleries/mac/mtp_device_delegate_impl_mac_unittest.mm:22:
#include "chrome/test/base/testing_browser_process.h"
On 2013/07/09 15:38:12, vandebo wrote:
> Needed?

nope

https://codereview.chromium.org/16703025/diff/126001/chrome/browser/storage_m...
File chrome/browser/storage_monitor/storage_monitor_win.h (right):

https://codereview.chromium.org/16703025/diff/126001/chrome/browser/storage_m...
chrome/browser/storage_monitor/storage_monitor_win.h:31:
StorageMonitorWin(VolumeMountWatcherWin* volume_mount_watcher,
On 2013/07/09 15:38:12, vandebo wrote:
> Can this stay private?

Taking out Create() here means the factory method calls the constructor. The
test does as well. I could just add more friends. Shall I do that?

[vandebo (ex-Chrome), 2013-07-09 21:14:11]

https://codereview.chromium.org/16703025/diff/126001/chrome/browser/browser_p...
File chrome/browser/browser_process_impl.cc (right):

https://codereview.chromium.org/16703025/diff/126001/chrome/browser/browser_p...
chrome/browser/browser_process_impl.cc:279: #if !defined(OS_ANDROID) &&
!defined(OS_IOS)
On 2013/07/09 17:35:21, Greg Billock wrote:
> On 2013/07/09 15:38:12, vandebo wrote:
> > Without any other context, I would guess this should go between
> > ExtensionRendererState shutdown and MessageCenter shutdown.  If the names
> > suggest the right thing to my pattern matching, MediaFileSystemRegistry is
> > listening for stuff from MessageCenter.
> 
> Is it? OK, moved.

content::NOTIFICATION_RENDERER_PROCESS_TERMINATED,
content::NOTIFICATION_WEB_CONTENTS_DESTROYED, etc eventually come from
MessageCenter, right?

https://codereview.chromium.org/16703025/diff/126001/chrome/browser/storage_m...
File chrome/browser/storage_monitor/storage_monitor_win.h (right):

https://codereview.chromium.org/16703025/diff/126001/chrome/browser/storage_m...
chrome/browser/storage_monitor/storage_monitor_win.h:31:
StorageMonitorWin(VolumeMountWatcherWin* volume_mount_watcher,
On 2013/07/09 17:35:21, Greg Billock wrote:
> On 2013/07/09 15:38:12, vandebo wrote:
> > Can this stay private?
> 
> Taking out Create() here means the factory method calls the constructor. The
> test does as well. I could just add more friends. Shall I do that?

I think this constructor is only used for testing, right?  If, so it should be
private with friends.

https://codereview.chromium.org/16703025/diff/147001/chrome/browser/extension...
File
chrome/browser/extensions/api/media_galleries_private/media_galleries_eject_apitest.cc
(right):

https://codereview.chromium.org/16703025/diff/147001/chrome/browser/extension...
chrome/browser/extensions/api/media_galleries_private/media_galleries_eject_apitest.cc:112:
// Pointer to the storage monitor owned by the browser process.
nit: "Owned by BrowserProcessImpl."

[Greg Billock, 2013-07-09 21:29:06]

https://codereview.chromium.org/16703025/diff/126001/chrome/browser/browser_p...
File chrome/browser/browser_process_impl.cc (right):

https://codereview.chromium.org/16703025/diff/126001/chrome/browser/browser_p...
chrome/browser/browser_process_impl.cc:279: #if !defined(OS_ANDROID) &&
!defined(OS_IOS)
On 2013/07/09 21:14:11, vandebo wrote:
> On 2013/07/09 17:35:21, Greg Billock wrote:
> > On 2013/07/09 15:38:12, vandebo wrote:
> > > Without any other context, I would guess this should go between
> > > ExtensionRendererState shutdown and MessageCenter shutdown.  If the names
> > > suggest the right thing to my pattern matching, MediaFileSystemRegistry is
> > > listening for stuff from MessageCenter.
> > 
> > Is it? OK, moved.
> 
> content::NOTIFICATION_RENDERER_PROCESS_TERMINATED,
> content::NOTIFICATION_WEB_CONTENTS_DESTROYED, etc eventually come from
> MessageCenter, right?

Those are through the NotificationService. Looks like MessageCenter is the UI
facility to display user-facing notifications. The notification service gets
destroyed in the browser main chain, in BrowserMainRunner::Shutdown

https://codereview.chromium.org/16703025/diff/126001/chrome/browser/storage_m...
File chrome/browser/storage_monitor/storage_monitor_win.h (right):

https://codereview.chromium.org/16703025/diff/126001/chrome/browser/storage_m...
chrome/browser/storage_monitor/storage_monitor_win.h:31:
StorageMonitorWin(VolumeMountWatcherWin* volume_mount_watcher,
On 2013/07/09 21:14:11, vandebo wrote:
> On 2013/07/09 17:35:21, Greg Billock wrote:
> > On 2013/07/09 15:38:12, vandebo wrote:
> > > Can this stay private?
> > 
> > Taking out Create() here means the factory method calls the constructor. The
> > test does as well. I could just add more friends. Shall I do that?
> 
> I think this constructor is only used for testing, right?  If, so it should be
> private with friends.

It's used by the factory as well. (The factory used to use the Create() method
above, but that's just confusing since we have the parent class Create method.)

https://codereview.chromium.org/16703025/diff/147001/chrome/browser/extension...
File
chrome/browser/extensions/api/media_galleries_private/media_galleries_eject_apitest.cc
(right):

https://codereview.chromium.org/16703025/diff/147001/chrome/browser/extension...
chrome/browser/extensions/api/media_galleries_private/media_galleries_eject_apitest.cc:112:
// Pointer to the storage monitor owned by the browser process.
On 2013/07/09 21:14:11, vandebo wrote:
> nit: "Owned by BrowserProcessImpl."

or the testing mock... I think this gets a bit too into the implementation of
the browser process.

[vandebo (ex-Chrome), 2013-07-10 16:57:44]

https://codereview.chromium.org/16703025/diff/126001/chrome/browser/storage_m...
File chrome/browser/storage_monitor/storage_monitor_win.h (right):

https://codereview.chromium.org/16703025/diff/126001/chrome/browser/storage_m...
chrome/browser/storage_monitor/storage_monitor_win.h:31:
StorageMonitorWin(VolumeMountWatcherWin* volume_mount_watcher,
On 2013/07/09 21:29:06, Greg Billock wrote:
> On 2013/07/09 21:14:11, vandebo wrote:
> > On 2013/07/09 17:35:21, Greg Billock wrote:
> > > On 2013/07/09 15:38:12, vandebo wrote:
> > > > Can this stay private?
> > > 
> > > Taking out Create() here means the factory method calls the constructor.
The
> > > test does as well. I could just add more friends. Shall I do that?
> > 
> > I think this constructor is only used for testing, right?  If, so it should
be
> > private with friends.
> 
> It's used by the factory as well. (The factory used to use the Create() method
> above, but that's just confusing since we have the parent class Create
method.)

Which factory?  Searching the diff shows it used in unit tests and the Create
method.

https://codereview.chromium.org/16703025/diff/147001/chrome/browser/extension...
File
chrome/browser/extensions/api/media_galleries_private/media_galleries_eject_apitest.cc
(right):

https://codereview.chromium.org/16703025/diff/147001/chrome/browser/extension...
chrome/browser/extensions/api/media_galleries_private/media_galleries_eject_apitest.cc:112:
// Pointer to the storage monitor owned by the browser process.
On 2013/07/09 21:29:06, Greg Billock wrote:
> On 2013/07/09 21:14:11, vandebo wrote:
> > nit: "Owned by BrowserProcessImpl."
> 
> or the testing mock... I think this gets a bit too into the implementation of
> the browser process.

Then is a comment needed? "Pointer to the storage monitor..." just restates the
line of code.  The only interesting bit of the comment is that it is owned by
someone else and this being a test, I'm not sure it's actually that important to
point that out.

https://codereview.chromium.org/16703025/diff/167001/chrome/browser/extension...
File
chrome/browser/extensions/api/media_galleries_private/media_galleries_private_apitest.cc
(right):

https://codereview.chromium.org/16703025/diff/167001/chrome/browser/extension...
chrome/browser/extensions/api/media_galleries_private/media_galleries_private_apitest.cc:112:
chrome::test::TestStorageMonitor::SyncInitialize();
Why is the test storage monitor setup different in eject_apitest and
private_apitest ?

https://codereview.chromium.org/16703025/diff/167001/chrome/browser/media_gal...
File chrome/browser/media_galleries/mac/mtp_device_delegate_impl_mac_unittest.mm
(right):

https://codereview.chromium.org/16703025/diff/167001/chrome/browser/media_gal...
chrome/browser/media_galleries/mac/mtp_device_delegate_impl_mac_unittest.mm:187:
chrome::test::TestStorageMonitor* monitor =
Is this a memory leak?

https://codereview.chromium.org/16703025/diff/167001/chrome/browser/media_gal...
File chrome/browser/media_galleries/win/mtp_device_delegate_impl_win_unittest.cc
(right):

https://codereview.chromium.org/16703025/diff/167001/chrome/browser/media_gal...
chrome/browser/media_galleries/win/mtp_device_delegate_impl_win_unittest.cc:84:
if (browser_process)
When will this be false?  If it is false, monitor_ is leaked.

https://codereview.chromium.org/16703025/diff/167001/chrome/browser/storage_m...
File chrome/browser/storage_monitor/image_capture_device_manager_unittest.mm
(right):

https://codereview.chromium.org/16703025/diff/167001/chrome/browser/storage_m...
chrome/browser/storage_monitor/image_capture_device_manager_unittest.mm:262:
monitor_ = chrome::test::TestStorageMonitor::Create();
Leak?

https://codereview.chromium.org/16703025/diff/167001/chrome/browser/storage_m...
chrome/browser/storage_monitor/image_capture_device_manager_unittest.mm:285: //
Weak ptr to storage monitor owned by browser process.
Not Weak ptr

https://codereview.chromium.org/16703025/diff/167001/chrome/browser/storage_m...
File chrome/browser/storage_monitor/media_storage_util_unittest.cc (right):

https://codereview.chromium.org/16703025/diff/167001/chrome/browser/storage_m...
chrome/browser/storage_monitor/media_storage_util_unittest.cc:66: monitor_ =
chrome::test::TestStorageMonitor::Create();
leak?

https://codereview.chromium.org/16703025/diff/167001/chrome/browser/storage_m...
chrome/browser/storage_monitor/media_storage_util_unittest.cc:91: // Weak
pointer to TestingBrowserProcess-owned storage monitor.
not weak ptr

https://codereview.chromium.org/16703025/diff/167001/chrome/browser/storage_m...
File
chrome/browser/storage_monitor/media_transfer_protocol_device_observer_linux_unittest.cc
(right):

https://codereview.chromium.org/16703025/diff/167001/chrome/browser/storage_m...
chrome/browser/storage_monitor/media_transfer_protocol_device_observer_linux_unittest.cc:97:
chrome::test::TestStorageMonitor* monitor =
leak?

https://codereview.chromium.org/16703025/diff/167001/chrome/browser/storage_m...
File chrome/browser/storage_monitor/storage_monitor.h (right):

https://codereview.chromium.org/16703025/diff/167001/chrome/browser/storage_m...
chrome/browser/storage_monitor/storage_monitor.h:72: // interface.
Should this only be called by BrowserProcess (especially with respect to the
following comment?)  If that's the case, then maybe Create should be private
with BrowserProcess as a friend?

https://codereview.chromium.org/16703025/diff/167001/chrome/browser/storage_m...
chrome/browser/storage_monitor/storage_monitor.h:77: // shutdown.
Outside that lifetime this returns Null?

https://codereview.chromium.org/16703025/diff/167001/chrome/browser/storage_m...
chrome/browser/storage_monitor/storage_monitor.h:143: virtual Receiver*
receiver() const;
aside: weren't we going to get rid of this method once we passed receiver_ into
the constructor of all of the implementations?

https://codereview.chromium.org/16703025/diff/167001/chrome/browser/storage_m...
File chrome/browser/storage_monitor/storage_monitor_chromeos_unittest.cc
(right):

https://codereview.chromium.org/16703025/diff/167001/chrome/browser/storage_m...
chrome/browser/storage_monitor/storage_monitor_chromeos_unittest.cc:185: if
(browser_process)
monitor_ leaks if browser_process is NULL

https://codereview.chromium.org/16703025/diff/167001/chrome/browser/storage_m...
File chrome/browser/storage_monitor/storage_monitor_linux_unittest.cc (right):

https://codereview.chromium.org/16703025/diff/167001/chrome/browser/storage_m...
chrome/browser/storage_monitor/storage_monitor_linux_unittest.cc:189: if
(browser_process)
Same here.

https://codereview.chromium.org/16703025/diff/167001/chrome/browser/storage_m...
File chrome/browser/storage_monitor/storage_monitor_mac_unittest.mm (right):

https://codereview.chromium.org/16703025/diff/167001/chrome/browser/storage_m...
chrome/browser/storage_monitor/storage_monitor_mac_unittest.mm:52: if
(browser_process)
And here.

https://codereview.chromium.org/16703025/diff/167001/chrome/browser/storage_m...
File chrome/browser/storage_monitor/storage_monitor_unittest.cc (right):

https://codereview.chromium.org/16703025/diff/167001/chrome/browser/storage_m...
chrome/browser/storage_monitor/storage_monitor_unittest.cc:20:
base::WaitableEvent event(false, false);
aside: WaitableEvent seems like the wrong primitive here, nothing waits on it,
nor does it need to.

https://codereview.chromium.org/16703025/diff/167001/chrome/browser/storage_m...
chrome/browser/storage_monitor/storage_monitor_unittest.cc:36:
test::TestStorageMonitor::RemoveSingleton();
nit: bump to the top of the test?

https://codereview.chromium.org/16703025/diff/167001/chrome/browser/storage_m...
File chrome/browser/storage_monitor/storage_monitor_win.cc (right):

https://codereview.chromium.org/16703025/diff/167001/chrome/browser/storage_m...
chrome/browser/storage_monitor/storage_monitor_win.cc:165: return new
StorageMonitorWin(new VolumeMountWatcherWin(),
Is this what you mean by the factory?  Would making the constructor protected
solve the problem?

https://codereview.chromium.org/16703025/diff/167001/chrome/browser/storage_m...
File chrome/browser/storage_monitor/storage_monitor_win_unittest.cc (right):

https://codereview.chromium.org/16703025/diff/167001/chrome/browser/storage_m...
chrome/browser/storage_monitor/storage_monitor_win_unittest.cc:97: if
(browser_process)
leak

https://codereview.chromium.org/16703025/diff/167001/chrome/browser/storage_m...
chrome/browser/storage_monitor/storage_monitor_win_unittest.cc:136: if
(browser_process)
leak

https://codereview.chromium.org/16703025/diff/167001/chrome/browser/storage_m...
File chrome/browser/storage_monitor/test_storage_monitor.cc (right):

https://codereview.chromium.org/16703025/diff/167001/chrome/browser/storage_m...
chrome/browser/storage_monitor/test_storage_monitor.cc:38:
TestingBrowserProcess* browser_process = TestingBrowserProcess::GetGlobal();
This pattern looks like it is repeated in a lot of unit tests, could you just
call this method instead?

https://codereview.chromium.org/16703025/diff/167001/chrome/browser/storage_m...
chrome/browser/storage_monitor/test_storage_monitor.cc:39: if (browser_process)
leak

https://codereview.chromium.org/16703025/diff/167001/chrome/browser/storage_m...
chrome/browser/storage_monitor/test_storage_monitor.cc:64: void
TestStorageMonitor::SyncInitialize() {
It seems like this method is only used from one test.  Would it make more sense
to move this code into that test?

https://codereview.chromium.org/16703025/diff/167001/chrome/browser/storage_m...
File chrome/browser/storage_monitor/test_storage_monitor.h (right):

https://codereview.chromium.org/16703025/diff/167001/chrome/browser/storage_m...
chrome/browser/storage_monitor/test_storage_monitor.h:24: // Create an
initialize a new TestStorageMonitor and install it
an -> and

https://codereview.chromium.org/16703025/diff/167001/chrome/browser/storage_m...
chrome/browser/storage_monitor/test_storage_monitor.h:28: // Create an
initialize a new TestStorageMonitor, and install it
an -> and

https://codereview.chromium.org/16703025/diff/167001/chrome/browser/storage_m...
chrome/browser/storage_monitor/test_storage_monitor.h:33: static void
RemoveSingleton();
Add comment

https://codereview.chromium.org/16703025/diff/167001/chrome/test/base/testing...
File chrome/test/base/testing_browser_process.cc (right):

https://codereview.chromium.org/16703025/diff/167001/chrome/test/base/testing...
chrome/test/base/testing_browser_process.cc:423: chrome::StorageMonitor*
storage_monitor) {
Should this take a scoped_ptr<chrome::StorageMonitor> ?

[Greg Billock, 2013-07-10 21:11:16]

https://codereview.chromium.org/16703025/diff/126001/chrome/browser/storage_m...
File chrome/browser/storage_monitor/storage_monitor_win.h (right):

https://codereview.chromium.org/16703025/diff/126001/chrome/browser/storage_m...
chrome/browser/storage_monitor/storage_monitor_win.h:31:
StorageMonitorWin(VolumeMountWatcherWin* volume_mount_watcher,
On 2013/07/10 16:57:45, vandebo wrote:
> On 2013/07/09 21:29:06, Greg Billock wrote:
> > On 2013/07/09 21:14:11, vandebo wrote:
> > > On 2013/07/09 17:35:21, Greg Billock wrote:
> > > > On 2013/07/09 15:38:12, vandebo wrote:
> > > > > Can this stay private?
> > > > 
> > > > Taking out Create() here means the factory method calls the constructor.
> The
> > > > test does as well. I could just add more friends. Shall I do that?
> > > 
> > > I think this constructor is only used for testing, right?  If, so it
should
> be
> > > private with friends.
> > 
> > It's used by the factory as well. (The factory used to use the Create()
method
> > above, but that's just confusing since we have the parent class Create
> method.)
> 
> Which factory?  Searching the diff shows it used in unit tests and the Create
> method.

The StorageMonitor::Create method was what I meant.

https://codereview.chromium.org/16703025/diff/147001/chrome/browser/extension...
File
chrome/browser/extensions/api/media_galleries_private/media_galleries_eject_apitest.cc
(right):

https://codereview.chromium.org/16703025/diff/147001/chrome/browser/extension...
chrome/browser/extensions/api/media_galleries_private/media_galleries_eject_apitest.cc:112:
// Pointer to the storage monitor owned by the browser process.
On 2013/07/10 16:57:45, vandebo wrote:
> On 2013/07/09 21:29:06, Greg Billock wrote:
> > On 2013/07/09 21:14:11, vandebo wrote:
> > > nit: "Owned by BrowserProcessImpl."
> > 
> > or the testing mock... I think this gets a bit too into the implementation
of
> > the browser process.
> 
> Then is a comment needed? "Pointer to the storage monitor..." just restates
the
> line of code.  The only interesting bit of the comment is that it is owned by
> someone else and this being a test, I'm not sure it's actually that important
to
> point that out.

ok, I'll kill these off.

https://codereview.chromium.org/16703025/diff/167001/chrome/browser/extension...
File
chrome/browser/extensions/api/media_galleries_private/media_galleries_private_apitest.cc
(right):

https://codereview.chromium.org/16703025/diff/167001/chrome/browser/extension...
chrome/browser/extensions/api/media_galleries_private/media_galleries_private_apitest.cc:112:
chrome::test::TestStorageMonitor::SyncInitialize();
On 2013/07/10 16:57:45, vandebo wrote:
> Why is the test storage monitor setup different in eject_apitest and
> private_apitest ?

The eject one needs the mock object (we check for the eject call in the test).
This test can use the live object, but it needs to make sure the object is
initialized for the various force-fed registration calls it does.

https://codereview.chromium.org/16703025/diff/167001/chrome/browser/media_gal...
File chrome/browser/media_galleries/mac/mtp_device_delegate_impl_mac_unittest.mm
(right):

https://codereview.chromium.org/16703025/diff/167001/chrome/browser/media_gal...
chrome/browser/media_galleries/mac/mtp_device_delegate_impl_mac_unittest.mm:187:
chrome::test::TestStorageMonitor* monitor =
On 2013/07/10 16:57:45, vandebo wrote:
> Is this a memory leak?

No. The Create method sets it up owned by the browser process. (You sure those
ownership comments are unneeded :-))

https://codereview.chromium.org/16703025/diff/167001/chrome/browser/media_gal...
File chrome/browser/media_galleries/win/mtp_device_delegate_impl_win_unittest.cc
(right):

https://codereview.chromium.org/16703025/diff/167001/chrome/browser/media_gal...
chrome/browser/media_galleries/win/mtp_device_delegate_impl_win_unittest.cc:84:
if (browser_process)
On 2013/07/10 16:57:45, vandebo wrote:
> When will this be false?  If it is false, monitor_ is leaked.

I think this was for platforms where there are still patchy exclusion of this
code. IOS, perhaps? Shall I else { delete monitor_; } on these? It'll likely at
some point become dead code when we get exclusion figured out.

https://codereview.chromium.org/16703025/diff/167001/chrome/browser/storage_m...
File chrome/browser/storage_monitor/image_capture_device_manager_unittest.mm
(right):

https://codereview.chromium.org/16703025/diff/167001/chrome/browser/storage_m...
chrome/browser/storage_monitor/image_capture_device_manager_unittest.mm:262:
monitor_ = chrome::test::TestStorageMonitor::Create();
On 2013/07/10 16:57:45, vandebo wrote:
> Leak?

no

https://codereview.chromium.org/16703025/diff/167001/chrome/browser/storage_m...
chrome/browser/storage_monitor/image_capture_device_manager_unittest.mm:285: //
Weak ptr to storage monitor owned by browser process.
On 2013/07/10 16:57:45, vandebo wrote:
> Not Weak ptr

Done.

https://codereview.chromium.org/16703025/diff/167001/chrome/browser/storage_m...
File chrome/browser/storage_monitor/media_storage_util_unittest.cc (right):

https://codereview.chromium.org/16703025/diff/167001/chrome/browser/storage_m...
chrome/browser/storage_monitor/media_storage_util_unittest.cc:66: monitor_ =
chrome::test::TestStorageMonitor::Create();
On 2013/07/10 16:57:45, vandebo wrote:
> leak?

no

https://codereview.chromium.org/16703025/diff/167001/chrome/browser/storage_m...
chrome/browser/storage_monitor/media_storage_util_unittest.cc:91: // Weak
pointer to TestingBrowserProcess-owned storage monitor.
On 2013/07/10 16:57:45, vandebo wrote:
> not weak ptr

Done.

https://codereview.chromium.org/16703025/diff/167001/chrome/browser/storage_m...
File chrome/browser/storage_monitor/storage_monitor.h (right):

https://codereview.chromium.org/16703025/diff/167001/chrome/browser/storage_m...
chrome/browser/storage_monitor/storage_monitor.h:77: // shutdown.
On 2013/07/10 16:57:45, vandebo wrote:
> Outside that lifetime this returns Null?

Added comment

https://codereview.chromium.org/16703025/diff/167001/chrome/browser/storage_m...
chrome/browser/storage_monitor/storage_monitor.h:143: virtual Receiver*
receiver() const;
On 2013/07/10 16:57:45, vandebo wrote:
> aside: weren't we going to get rid of this method once we passed receiver_
into
> the constructor of all of the implementations?

I think so. There are still various tests that use it. Still some work to do
there...

https://codereview.chromium.org/16703025/diff/167001/chrome/browser/storage_m...
File chrome/browser/storage_monitor/storage_monitor_unittest.cc (right):

https://codereview.chromium.org/16703025/diff/167001/chrome/browser/storage_m...
chrome/browser/storage_monitor/storage_monitor_unittest.cc:20:
base::WaitableEvent event(false, false);
On 2013/07/10 16:57:45, vandebo wrote:
> aside: WaitableEvent seems like the wrong primitive here, nothing waits on it,
> nor does it need to.

Yeah. It's a handy latch. I don't think we have a handier one.

https://codereview.chromium.org/16703025/diff/167001/chrome/browser/storage_m...
chrome/browser/storage_monitor/storage_monitor_unittest.cc:36:
test::TestStorageMonitor::RemoveSingleton();
On 2013/07/10 16:57:45, vandebo wrote:
> nit: bump to the top of the test?

Done.

https://codereview.chromium.org/16703025/diff/167001/chrome/browser/storage_m...
File chrome/browser/storage_monitor/storage_monitor_win.cc (right):

https://codereview.chromium.org/16703025/diff/167001/chrome/browser/storage_m...
chrome/browser/storage_monitor/storage_monitor_win.cc:165: return new
StorageMonitorWin(new VolumeMountWatcherWin(),
On 2013/07/10 16:57:45, vandebo wrote:
> Is this what you mean by the factory?  Would making the constructor protected
> solve the problem?

Yes, this is the windows factory impl. You mean make the StorageMonitor
constructor protected? It is now.

https://codereview.chromium.org/16703025/diff/167001/chrome/browser/storage_m...
File chrome/browser/storage_monitor/test_storage_monitor.cc (right):

https://codereview.chromium.org/16703025/diff/167001/chrome/browser/storage_m...
chrome/browser/storage_monitor/test_storage_monitor.cc:38:
TestingBrowserProcess* browser_process = TestingBrowserProcess::GetGlobal();
On 2013/07/10 16:57:45, vandebo wrote:
> This pattern looks like it is repeated in a lot of unit tests, could you just
> call this method instead?

I do where possible. Perhaps I should split out the installer, though. Where it
isn't the TestStorageMonitor, we install platform-specific ones.

https://codereview.chromium.org/16703025/diff/167001/chrome/browser/storage_m...
chrome/browser/storage_monitor/test_storage_monitor.cc:64: void
TestStorageMonitor::SyncInitialize() {
On 2013/07/10 16:57:45, vandebo wrote:
> It seems like this method is only used from one test.  Would it make more
sense
> to move this code into that test?

I thought about that, but it seems more a util fixture method -- isn't specific
to any StoraegMonitor.

https://codereview.chromium.org/16703025/diff/167001/chrome/browser/storage_m...
File chrome/browser/storage_monitor/test_storage_monitor.h (right):

https://codereview.chromium.org/16703025/diff/167001/chrome/browser/storage_m...
chrome/browser/storage_monitor/test_storage_monitor.h:24: // Create an
initialize a new TestStorageMonitor and install it
On 2013/07/10 16:57:45, vandebo wrote:
> an -> and

Done.

https://codereview.chromium.org/16703025/diff/167001/chrome/browser/storage_m...
chrome/browser/storage_monitor/test_storage_monitor.h:28: // Create an
initialize a new TestStorageMonitor, and install it
On 2013/07/10 16:57:45, vandebo wrote:
> an -> and

Done.

https://codereview.chromium.org/16703025/diff/167001/chrome/browser/storage_m...
chrome/browser/storage_monitor/test_storage_monitor.h:33: static void
RemoveSingleton();
On 2013/07/10 16:57:45, vandebo wrote:
> Add comment

Done.

https://codereview.chromium.org/16703025/diff/167001/chrome/test/base/testing...
File chrome/test/base/testing_browser_process.cc (right):

https://codereview.chromium.org/16703025/diff/167001/chrome/test/base/testing...
chrome/test/base/testing_browser_process.cc:423: chrome::StorageMonitor*
storage_monitor) {
On 2013/07/10 16:57:45, vandebo wrote:
> Should this take a scoped_ptr<chrome::StorageMonitor> ?

Yeah, it takes ownership. That's probably better.

[Greg Billock, 2013-07-10 21:57:17]

https://codereview.chromium.org/16703025/diff/167001/chrome/browser/storage_m...
File
chrome/browser/storage_monitor/media_transfer_protocol_device_observer_linux_unittest.cc
(right):

https://codereview.chromium.org/16703025/diff/167001/chrome/browser/storage_m...
chrome/browser/storage_monitor/media_transfer_protocol_device_observer_linux_unittest.cc:97:
chrome::test::TestStorageMonitor* monitor =
no

On 2013/07/10 16:57:45, vandebo wrote:
> leak?

https://codereview.chromium.org/16703025/diff/167001/chrome/browser/storage_m...
File chrome/browser/storage_monitor/storage_monitor.h (right):

https://codereview.chromium.org/16703025/diff/167001/chrome/browser/storage_m...
chrome/browser/storage_monitor/storage_monitor.h:72: // interface.
On 2013/07/10 16:57:45, vandebo wrote:
> Should this only be called by BrowserProcess (especially with respect to the
> following comment?)  If that's the case, then maybe Create should be private
> with BrowserProcess as a friend?

Hmmm. Could do.

https://codereview.chromium.org/16703025/diff/167001/chrome/browser/storage_m...
File chrome/browser/storage_monitor/storage_monitor_chromeos_unittest.cc
(right):

https://codereview.chromium.org/16703025/diff/167001/chrome/browser/storage_m...
chrome/browser/storage_monitor/storage_monitor_chromeos_unittest.cc:185: if
(browser_process)
On 2013/07/10 16:57:45, vandebo wrote:
> monitor_ leaks if browser_process is NULL

Done.

https://codereview.chromium.org/16703025/diff/167001/chrome/browser/storage_m...
File chrome/browser/storage_monitor/storage_monitor_linux_unittest.cc (right):

https://codereview.chromium.org/16703025/diff/167001/chrome/browser/storage_m...
chrome/browser/storage_monitor/storage_monitor_linux_unittest.cc:189: if
(browser_process)
On 2013/07/10 16:57:45, vandebo wrote:
> Same here.

Done.

https://codereview.chromium.org/16703025/diff/167001/chrome/browser/storage_m...
File chrome/browser/storage_monitor/storage_monitor_mac_unittest.mm (right):

https://codereview.chromium.org/16703025/diff/167001/chrome/browser/storage_m...
chrome/browser/storage_monitor/storage_monitor_mac_unittest.mm:52: if
(browser_process)
On 2013/07/10 16:57:45, vandebo wrote:
> And here.

Done.

https://codereview.chromium.org/16703025/diff/167001/chrome/browser/storage_m...
File chrome/browser/storage_monitor/storage_monitor_win_unittest.cc (right):

https://codereview.chromium.org/16703025/diff/167001/chrome/browser/storage_m...
chrome/browser/storage_monitor/storage_monitor_win_unittest.cc:97: if
(browser_process)
On 2013/07/10 16:57:45, vandebo wrote:
> leak

Done.

https://codereview.chromium.org/16703025/diff/167001/chrome/browser/storage_m...
File chrome/browser/storage_monitor/test_storage_monitor.cc (right):

https://codereview.chromium.org/16703025/diff/167001/chrome/browser/storage_m...
chrome/browser/storage_monitor/test_storage_monitor.cc:39: if (browser_process)
On 2013/07/10 16:57:45, vandebo wrote:
> leak

Done.

[Greg Billock, 2013-07-10 21:59:19]

https://codereview.chromium.org/16703025/diff/167001/chrome/browser/storage_m...
File chrome/browser/storage_monitor/storage_monitor_win_unittest.cc (right):

https://codereview.chromium.org/16703025/diff/167001/chrome/browser/storage_m...
chrome/browser/storage_monitor/storage_monitor_win_unittest.cc:136: if
(browser_process)
On 2013/07/10 16:57:45, vandebo wrote:
> leak

Done.

[vandebo (ex-Chrome), 2013-07-11 15:53:00]

LGTM

https://codereview.chromium.org/16703025/diff/167001/chrome/browser/media_gal...
File chrome/browser/media_galleries/mac/mtp_device_delegate_impl_mac_unittest.mm
(right):

https://codereview.chromium.org/16703025/diff/167001/chrome/browser/media_gal...
chrome/browser/media_galleries/mac/mtp_device_delegate_impl_mac_unittest.mm:187:
chrome::test::TestStorageMonitor* monitor =
On 2013/07/10 21:11:16, Greg Billock wrote:
> On 2013/07/10 16:57:45, vandebo wrote:
> > Is this a memory leak?
> 
> No. The Create method sets it up owned by the browser process. (You sure those
> ownership comments are unneeded :-))

The new name helps some, but a Create method that returns the thing created
implies that the caller is responsible for cleaning up the thing.  Now that I
understand (again?) it's ok.  If you think it'd be an improvement though, you
might want to make the create method return void and have a seperate Get method.

https://codereview.chromium.org/16703025/diff/167001/chrome/browser/media_gal...
File chrome/browser/media_galleries/win/mtp_device_delegate_impl_win_unittest.cc
(right):

https://codereview.chromium.org/16703025/diff/167001/chrome/browser/media_gal...
chrome/browser/media_galleries/win/mtp_device_delegate_impl_win_unittest.cc:84:
if (browser_process)
On 2013/07/10 21:11:16, Greg Billock wrote:
> On 2013/07/10 16:57:45, vandebo wrote:
> > When will this be false?  If it is false, monitor_ is leaked.
> 
> I think this was for platforms where there are still patchy exclusion of this
> code. IOS, perhaps? Shall I else { delete monitor_; } on these? It'll likely
at
> some point become dead code when we get exclusion figured out.

The quickest way to make sure you have it all figured out is to change this to a
CHECK and send it to the bot.  I think it has to be dealt with one way or
another (excluded or freed) before check in though -- and it looks like you did
both.

https://codereview.chromium.org/16703025/diff/167001/chrome/browser/storage_m...
File chrome/browser/storage_monitor/storage_monitor_unittest.cc (right):

https://codereview.chromium.org/16703025/diff/167001/chrome/browser/storage_m...
chrome/browser/storage_monitor/storage_monitor_unittest.cc:20:
base::WaitableEvent event(false, false);
On 2013/07/10 21:11:16, Greg Billock wrote:
> On 2013/07/10 16:57:45, vandebo wrote:
> > aside: WaitableEvent seems like the wrong primitive here, nothing waits on
it,
> > nor does it need to.
> 
> Yeah. It's a handy latch. I don't think we have a handier one.

This is only slightly more code and is easier to understand:

void SetIfCalled(bool* called) {
 *called = true;
}

bool called = false;
monit.EnsureInitialized(base::Bind(&SetIfCalled, base::Unretained(&called)));
...

https://codereview.chromium.org/16703025/diff/167001/chrome/browser/storage_m...
File chrome/browser/storage_monitor/storage_monitor_win.cc (right):

https://codereview.chromium.org/16703025/diff/167001/chrome/browser/storage_m...
chrome/browser/storage_monitor/storage_monitor_win.cc:165: return new
StorageMonitorWin(new VolumeMountWatcherWin(),
On 2013/07/10 21:11:16, Greg Billock wrote:
> On 2013/07/10 16:57:45, vandebo wrote:
> > Is this what you mean by the factory?  Would making the constructor
protected
> > solve the problem?
> 
> Yes, this is the windows factory impl. You mean make the StorageMonitor
> constructor protected? It is now.

The StorageMonitorWin constructor is currently public.

[Greg Billock, 2013-07-11 20:53:34]

https://codereview.chromium.org/16703025/diff/167001/chrome/browser/media_gal...
File chrome/browser/media_galleries/mac/mtp_device_delegate_impl_mac_unittest.mm
(right):

https://codereview.chromium.org/16703025/diff/167001/chrome/browser/media_gal...
chrome/browser/media_galleries/mac/mtp_device_delegate_impl_mac_unittest.mm:187:
chrome::test::TestStorageMonitor* monitor =
On 2013/07/11 15:53:00, vandebo wrote:
> On 2013/07/10 21:11:16, Greg Billock wrote:
> > On 2013/07/10 16:57:45, vandebo wrote:
> > > Is this a memory leak?
> > 
> > No. The Create method sets it up owned by the browser process. (You sure
those
> > ownership comments are unneeded :-))
> 
> The new name helps some, but a Create method that returns the thing created
> implies that the caller is responsible for cleaning up the thing.  Now that I
> understand (again?) it's ok.  If you think it'd be an improvement though, you
> might want to make the create method return void and have a seperate Get
method.

That's definitely a confusion. It's a test helper, so I think it's nice for it
to just do what we usually want. I'll change the name if we can come up with a
better one. Just "install"?

https://codereview.chromium.org/16703025/diff/167001/chrome/browser/media_gal...
File chrome/browser/media_galleries/win/mtp_device_delegate_impl_win_unittest.cc
(right):

https://codereview.chromium.org/16703025/diff/167001/chrome/browser/media_gal...
chrome/browser/media_galleries/win/mtp_device_delegate_impl_win_unittest.cc:84:
if (browser_process)
On 2013/07/11 15:53:00, vandebo wrote:
> On 2013/07/10 21:11:16, Greg Billock wrote:
> > On 2013/07/10 16:57:45, vandebo wrote:
> > > When will this be false?  If it is false, monitor_ is leaked.
> > 
> > I think this was for platforms where there are still patchy exclusion of
this
> > code. IOS, perhaps? Shall I else { delete monitor_; } on these? It'll likely
> at
> > some point become dead code when we get exclusion figured out.
> 
> The quickest way to make sure you have it all figured out is to change this to
a
> CHECK and send it to the bot.  I think it has to be dealt with one way or
> another (excluded or freed) before check in though -- and it looks like you
did
> both.
> 

Yes. Where I'm confident we're fine I just switched to DCHECK which is better.
Bots seem happy, so I think this is solved.

https://codereview.chromium.org/16703025/diff/167001/chrome/browser/storage_m...
File chrome/browser/storage_monitor/storage_monitor_unittest.cc (right):

https://codereview.chromium.org/16703025/diff/167001/chrome/browser/storage_m...
chrome/browser/storage_monitor/storage_monitor_unittest.cc:20:
base::WaitableEvent event(false, false);
ok. done.

On 2013/07/11 15:53:00, vandebo wrote:
> On 2013/07/10 21:11:16, Greg Billock wrote:
> > On 2013/07/10 16:57:45, vandebo wrote:
> > > aside: WaitableEvent seems like the wrong primitive here, nothing waits on
> it,
> > > nor does it need to.
> > 
> > Yeah. It's a handy latch. I don't think we have a handier one.
> 
> This is only slightly more code and is easier to understand:
> 
> void SetIfCalled(bool* called) {
>  *called = true;
> }
> 
> bool called = false;
> monit.EnsureInitialized(base::Bind(&SetIfCalled, base::Unretained(&called)));
> ...

[Greg Billock, 2013-07-11 20:58:57]

On 2013/07/11 20:53:34, Greg Billock wrote:
>
https://codereview.chromium.org/16703025/diff/167001/chrome/browser/media_gal...
> File
chrome/browser/media_galleries/mac/mtp_device_delegate_impl_mac_unittest.mm
> (right):
> 
>
https://codereview.chromium.org/16703025/diff/167001/chrome/browser/media_gal...
>
chrome/browser/media_galleries/mac/mtp_device_delegate_impl_mac_unittest.mm:187:
> chrome::test::TestStorageMonitor* monitor =
> On 2013/07/11 15:53:00, vandebo wrote:
> > On 2013/07/10 21:11:16, Greg Billock wrote:
> > > On 2013/07/10 16:57:45, vandebo wrote:
> > > > Is this a memory leak?
> > > 
> > > No. The Create method sets it up owned by the browser process. (You sure
> those
> > > ownership comments are unneeded :-))
> > 
> > The new name helps some, but a Create method that returns the thing created
> > implies that the caller is responsible for cleaning up the thing.  Now that
I
> > understand (again?) it's ok.  If you think it'd be an improvement though,
you
> > might want to make the create method return void and have a seperate Get
> method.
> 
> That's definitely a confusion. It's a test helper, so I think it's nice for it
> to just do what we usually want. I'll change the name if we can come up with a
> better one. Just "install"?
> 
>
https://codereview.chromium.org/16703025/diff/167001/chrome/browser/media_gal...
> File
chrome/browser/media_galleries/win/mtp_device_delegate_impl_win_unittest.cc
> (right):
> 
>
https://codereview.chromium.org/16703025/diff/167001/chrome/browser/media_gal...
>
chrome/browser/media_galleries/win/mtp_device_delegate_impl_win_unittest.cc:84:
> if (browser_process)
> On 2013/07/11 15:53:00, vandebo wrote:
> > On 2013/07/10 21:11:16, Greg Billock wrote:
> > > On 2013/07/10 16:57:45, vandebo wrote:
> > > > When will this be false?  If it is false, monitor_ is leaked.
> > > 
> > > I think this was for platforms where there are still patchy exclusion of
> this
> > > code. IOS, perhaps? Shall I else { delete monitor_; } on these? It'll
likely
> > at
> > > some point become dead code when we get exclusion figured out.
> > 
> > The quickest way to make sure you have it all figured out is to change this
to
> a
> > CHECK and send it to the bot.  I think it has to be dealt with one way or
> > another (excluded or freed) before check in though -- and it looks like you
> did
> > both.
> > 
> 
> Yes. Where I'm confident we're fine I just switched to DCHECK which is better.
> Bots seem happy, so I think this is solved.
> 
>
https://codereview.chromium.org/16703025/diff/167001/chrome/browser/storage_m...
> File chrome/browser/storage_monitor/storage_monitor_unittest.cc (right):
> 
>
https://codereview.chromium.org/16703025/diff/167001/chrome/browser/storage_m...
> chrome/browser/storage_monitor/storage_monitor_unittest.cc:20:
> base::WaitableEvent event(false, false);
> ok. done.
> 
> On 2013/07/11 15:53:00, vandebo wrote:
> > On 2013/07/10 21:11:16, Greg Billock wrote:
> > > On 2013/07/10 16:57:45, vandebo wrote:
> > > > aside: WaitableEvent seems like the wrong primitive here, nothing waits
on
> > it,
> > > > nor does it need to.
> > > 
> > > Yeah. It's a handy latch. I don't think we have a handier one.
> > 
> > This is only slightly more code and is easier to understand:
> > 
> > void SetIfCalled(bool* called) {
> >  *called = true;
> > }
> > 
> > bool called = false;
> > monit.EnsureInitialized(base::Bind(&SetIfCalled,
base::Unretained(&called)));
> > ...

Adding owners:

sky: chrome/browser, chrome/test, and chrome/*  -- Please look carefully at the
change to BrowserProcess, this moves the StorageMonitor object from being a
singleton initialized in the startup sequence to be being owned by
BrowserProcess. The intention is that the startup/shutdown sequence should not
be changed from the existing behavior.

estade: ui/gtk (should be near-tbr)
sail: ui/cocoa (should be near-tbr)

[Evan Stade, 2013-07-11 22:05:46]

gtk lgtm

[sky, 2013-07-15 17:45:36]

LGTM

https://codereview.chromium.org/16703025/diff/233001/chrome/browser/browser_p...
File chrome/browser/browser_process_impl.h (right):

https://codereview.chromium.org/16703025/diff/233001/chrome/browser/browser_p...
chrome/browser/browser_process_impl.h:203: #if !defined(OS_ANDROID) &&
!defined(OS_IOS)
Would it be clearer to have a feature define, eg:
if defined(STORAGE_MONITOR) ?

https://codereview.chromium.org/16703025/diff/233001/chrome/browser/extension...
File
chrome/browser/extensions/api/media_galleries_private/media_galleries_eject_apitest.cc
(right):

https://codereview.chromium.org/16703025/diff/233001/chrome/browser/extension...
chrome/browser/extensions/api/media_galleries_private/media_galleries_eject_apitest.cc:57:
: device_id_(GetDeviceId()), monitor_(NULL) {}
nit: when you wrap, each param on its own line.

[Greg Billock, 2013-07-15 20:00:17]

https://codereview.chromium.org/16703025/diff/233001/chrome/browser/browser_p...
File chrome/browser/browser_process_impl.h (right):

https://codereview.chromium.org/16703025/diff/233001/chrome/browser/browser_p...
chrome/browser/browser_process_impl.h:203: #if !defined(OS_ANDROID) &&
!defined(OS_IOS)
On 2013/07/15 17:45:37, sky wrote:
> Would it be clearer to have a feature define, eg:
> if defined(STORAGE_MONITOR) ?

That might be a good idea. We'll talk about doing that.

https://codereview.chromium.org/16703025/diff/233001/chrome/browser/extension...
File
chrome/browser/extensions/api/media_galleries_private/media_galleries_eject_apitest.cc
(right):

https://codereview.chromium.org/16703025/diff/233001/chrome/browser/extension...
chrome/browser/extensions/api/media_galleries_private/media_galleries_eject_apitest.cc:57:
: device_id_(GetDeviceId()), monitor_(NULL) {}
On 2013/07/15 17:45:37, sky wrote:
> nit: when you wrap, each param on its own line.

Done.

[commit-bot: I haz the power, 2013-07-15 20:03:10]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/gbillock@chromium.org/16703025/239001

[commit-bot: I haz the power, 2013-07-16 00:50:56]

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

[commit-bot: I haz the power, 2013-07-16 00:51:04]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/gbillock@chromium.org/16703025/247001

[commit-bot: I haz the power, 2013-07-16 08:42:58]

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

[commit-bot: I haz the power, 2013-07-16 15:44:18]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/gbillock@chromium.org/16703025/262001

[commit-bot: I haz the power, 2013-07-16 17:18:17]

Sorry for I got bad news for ya.
Compile failed with a clobber build on win_rel.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...
Your code is likely broken or HEAD is junk. Please ensure your
code is not broken then alert the build sheriffs.
Look at the try server FAQ for more details.

[commit-bot: I haz the power, 2013-07-16 20:14:26]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/gbillock@chromium.org/16703025/274003

[commit-bot: I haz the power, 2013-07-17 03:00:21]

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

[commit-bot: I haz the power, 2013-07-18 17:03:02]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/gbillock@chromium.org/16703025/309001

[commit-bot: I haz the power, 2013-07-19 01:29:04]

Change committed as 212487