Regularize ownerships and lifecycle for storage monitor platform classes.

Regularize ownerships and lifecycle for storage monitor platform classes.

This change regularizes the construction lifecycle of the StorageMonitor implementations so that they always exist for the construction of the Profile, but don't issue any notifications until after that construction is complete. This allows profile-based listeners to be sure they're synchronized with the monitor.

It also regularizes ownership of all storage-monitor-related objects. The base platform-specific object is owned by the ChromeBrowserMain object, and owns all subsidiary platform-specific objects needed to monitor and/or deal with volumes of various types (i.e. MTP).

R=vandebo@chromium.org
BUG=None


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

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

[Greg Billock, 2013-02-26 19:19:29]

On 2013/02/26 18:17:08, Greg Billock wrote:

This change as it stands introduces one significant sequencing change. Currently
the MTP observer in linux/chromeos is initialized in PostProfileInit, where it
gets the list of available storages from the MTP daemon and issues observer
notifications.

In the change as written, this moves up to PreProfileInit, so these
notifications are issued before the profile is initialized.

There's variation in how we initialize these objects currently. Windows does it
in PreMainMessageLoopRun, after the bit that calls through to
Pre/PostProfileInit. Mac does all the setup in PreProfileInit.

Shall I, as part of this change, standardize all the platforms to do this
initialization in PostProfileInit (or the moral equivalent), thus issuing any
notifications after the profile has had a chance to come up? I think we should
do it uniformly.

[vandebo (ex-Chrome), 2013-02-26 23:07:44]

On 2013/02/26 19:19:29, Greg Billock wrote:
> On 2013/02/26 18:17:08, Greg Billock wrote:
> 
> This change as it stands introduces one significant sequencing change.
Currently
> the MTP observer in linux/chromeos is initialized in PostProfileInit, where it
> gets the list of available storages from the MTP daemon and issues observer
> notifications.
> 
> In the change as written, this moves up to PreProfileInit, so these
> notifications are issued before the profile is initialized.
> 
> There's variation in how we initialize these objects currently. Windows does
it
> in PreMainMessageLoopRun, after the bit that calls through to
> Pre/PostProfileInit. Mac does all the setup in PreProfileInit.
> 
> Shall I, as part of this change, standardize all the platforms to do this
> initialization in PostProfileInit (or the moral equivalent), thus issuing any
> notifications after the profile has had a chance to come up? I think we should
> do it uniformly.

Hmm... storage monitor is a service, so it should logically come up before the
profile does its thing -- in case something in the profile bring up needs the
service.  On the other hand as you point out, notifications should go out after
the profile comes up.

In either case, it should be consistent.

How about adding a DCHECK that g_removable_storage_notifications isn't NULL in
GetInstance(). Along with a comment describing the problem.  i.e. if we need the
service to come up before the profile, we'll have to add an InitNotifications
method that can run after the profile comes up.

Also, with this change, I think you can remove a couple of the friends in
removable_storage_notifications.h

https://codereview.chromium.org/12334096/diff/1/chrome/browser/chrome_browser...
File chrome/browser/chrome_browser_main_mac.h (left):

https://codereview.chromium.org/12334096/diff/1/chrome/browser/chrome_browser...
chrome/browser/chrome_browser_main_mac.h:12: class ImageCaptureDeviceManager;
Do you need this in removable_device_notifications_mac.h ?

https://codereview.chromium.org/12334096/diff/1/chrome/browser/chrome_browser...
File chrome/browser/chrome_browser_main_mac.mm (left):

https://codereview.chromium.org/12334096/diff/1/chrome/browser/chrome_browser...
chrome/browser/chrome_browser_main_mac.mm:25: #include
"chrome/browser/storage_monitor/image_capture_device_manager.h"
Move to removable_device_notifications_mac.mm

[Greg Billock, 2013-02-27 19:53:52]

On 2013/02/26 23:07:44, vandebo wrote:
> On 2013/02/26 19:19:29, Greg Billock wrote:
> > On 2013/02/26 18:17:08, Greg Billock wrote:
> > 
> > This change as it stands introduces one significant sequencing change.
> Currently
> > the MTP observer in linux/chromeos is initialized in PostProfileInit, where
it
> > gets the list of available storages from the MTP daemon and issues observer
> > notifications.
> > 
> > In the change as written, this moves up to PreProfileInit, so these
> > notifications are issued before the profile is initialized.
> > 
> > There's variation in how we initialize these objects currently. Windows does
> it
> > in PreMainMessageLoopRun, after the bit that calls through to
> > Pre/PostProfileInit. Mac does all the setup in PreProfileInit.
> > 
> > Shall I, as part of this change, standardize all the platforms to do this
> > initialization in PostProfileInit (or the moral equivalent), thus issuing
any
> > notifications after the profile has had a chance to come up? I think we
should
> > do it uniformly.
> 
> Hmm... storage monitor is a service, so it should logically come up before the
> profile does its thing -- in case something in the profile bring up needs the
> service.  On the other hand as you point out, notifications should go out
after
> the profile comes up.
> 
> In either case, it should be consistent.
> 
> How about adding a DCHECK that g_removable_storage_notifications isn't NULL in
> GetInstance(). Along with a comment describing the problem.  i.e. if we need
the
> service to come up before the profile, we'll have to add an InitNotifications
> method that can run after the profile comes up.

Yes. We have that for Linux/ChromeOS. For Mac/Win we don't do that.

I found another wrinkle, which is that the MTP subsystem won't initialize
without an MTP manager, and we don't make one of those in tests. I changed the
code to make it OK for the MTP manager not to exist, but I think this is risky,
and instead we should make a mock object. What's the right approach there, Lei?

> 
> Also, with this change, I think you can remove a couple of the friends in
> removable_storage_notifications.h
> 
>
https://codereview.chromium.org/12334096/diff/1/chrome/browser/chrome_browser...
> File chrome/browser/chrome_browser_main_mac.h (left):
> 
>
https://codereview.chromium.org/12334096/diff/1/chrome/browser/chrome_browser...
> chrome/browser/chrome_browser_main_mac.h:12: class ImageCaptureDeviceManager;
> Do you need this in removable_device_notifications_mac.h ?
> 
>
https://codereview.chromium.org/12334096/diff/1/chrome/browser/chrome_browser...
> File chrome/browser/chrome_browser_main_mac.mm (left):
> 
>
https://codereview.chromium.org/12334096/diff/1/chrome/browser/chrome_browser...
> chrome/browser/chrome_browser_main_mac.mm:25: #include
> "chrome/browser/storage_monitor/image_capture_device_manager.h"
> Move to removable_device_notifications_mac.mm

[Greg Billock, 2013-02-27 19:54:06]

https://codereview.chromium.org/12334096/diff/1/chrome/browser/chrome_browser...
File chrome/browser/chrome_browser_main_mac.h (left):

https://codereview.chromium.org/12334096/diff/1/chrome/browser/chrome_browser...
chrome/browser/chrome_browser_main_mac.h:12: class ImageCaptureDeviceManager;
On 2013/02/26 23:07:44, vandebo wrote:
> Do you need this in removable_device_notifications_mac.h ?

Yes. Discovered this in the try run and fixed.

https://codereview.chromium.org/12334096/diff/1/chrome/browser/chrome_browser...
File chrome/browser/chrome_browser_main_mac.mm (left):

https://codereview.chromium.org/12334096/diff/1/chrome/browser/chrome_browser...
chrome/browser/chrome_browser_main_mac.mm:25: #include
"chrome/browser/storage_monitor/image_capture_device_manager.h"
On 2013/02/26 23:07:44, vandebo wrote:
> Move to removable_device_notifications_mac.mm

Done.

[Lei Zhang, 2013-02-28 02:14:43]

I'd like to see all platforms behave consistently w.r.t. when they get
initialized and when they send notifications.

Since we are unifying things here, it should be easy to add a method to
chrome::StorageMonitor to allow notifications, and call it in PostProfileInit,
right?

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

https://codereview.chromium.org/12334096/diff/14001/chrome/browser/chrome_bro...
chrome/browser/chrome_browser_main_linux.h:21: class
MediaTransferProtocolDeviceObserverLinux;
remove as well

https://codereview.chromium.org/12334096/diff/14001/chrome/browser/storage_mo...
File
chrome/browser/storage_monitor/media_transfer_protocol_device_observer_linux.cc
(right):

https://codereview.chromium.org/12334096/diff/14001/chrome/browser/storage_mo...
chrome/browser/storage_monitor/media_transfer_protocol_device_observer_linux.cc:132:
if (mtp_manager) {
As is, |mtp_manager| is always true here, because
MediaTransferProtocolDeviceObserverLinux and MediaTransferProtocolManager are
both only instanciated under the same condition.

Perhaps MediaTransferProtocolManager should be owned by
RemovableDeviceNotifications as well. MediaTransferProtocolDeviceObserverLinux
can take a MediaTransferProtocolManager in the ctor instead? We can probably get
rid of MediaTransferProtocolManager::GetInstance() and have clearer ownership.

[Greg Billock, 2013-02-28 22:35:37]

Made all invocations consistent across platforms WRT profile and init behavior
synchronization.

There's a couple adjustments: On mac, I don't make the session for
DiskArbitrarion until Init(). I think that's correct. Does anyone know that
library? Docs are thin, but the ones I found indicated we should schedule the
session after installing callbacks, which makes a lot of sense, so I made that
change.

The change to put the MTP manager in the storage monitor seems good, Lei. I'm
getting an angry log in tests that it gets shut down without initialization, but
that probably means we should be initializing it somewhere, maybe? Or is that an
assumption that we should weaken due to unit tests.

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

https://codereview.chromium.org/12334096/diff/14001/chrome/browser/chrome_bro...
chrome/browser/chrome_browser_main_linux.h:21: class
MediaTransferProtocolDeviceObserverLinux;
On 2013/02/28 02:14:43, Lei Zhang wrote:
> remove as well

Done.

https://codereview.chromium.org/12334096/diff/14001/chrome/browser/storage_mo...
File
chrome/browser/storage_monitor/media_transfer_protocol_device_observer_linux.cc
(right):

https://codereview.chromium.org/12334096/diff/14001/chrome/browser/storage_mo...
chrome/browser/storage_monitor/media_transfer_protocol_device_observer_linux.cc:132:
if (mtp_manager) {
On 2013/02/28 02:14:43, Lei Zhang wrote:
> As is, |mtp_manager| is always true here, because
> MediaTransferProtocolDeviceObserverLinux and MediaTransferProtocolManager are
> both only instanciated under the same condition.
> 
> Perhaps MediaTransferProtocolManager should be owned by
> RemovableDeviceNotifications as well. MediaTransferProtocolDeviceObserverLinux
> can take a MediaTransferProtocolManager in the ctor instead? We can probably
get
> rid of MediaTransferProtocolManager::GetInstance() and have clearer ownership.

That sounds like a great option. I'll add that to this change.

[Greg Billock, 2013-02-28 22:43:56]

On 2013/02/28 22:35:37, Greg Billock wrote:
> Made all invocations consistent across platforms WRT profile and init behavior
> synchronization.
> 
> There's a couple adjustments: On mac, I don't make the session for
> DiskArbitrarion until Init(). I think that's correct. Does anyone know that
> library? Docs are thin, but the ones I found indicated we should schedule the
> session after installing callbacks, which makes a lot of sense, so I made that
> change.
> 
> The change to put the MTP manager in the storage monitor seems good, Lei. I'm
> getting an angry log in tests that it gets shut down without initialization,
but
> that probably means we should be initializing it somewhere, maybe? Or is that
an
> assumption that we should weaken due to unit tests.
> 
>
https://codereview.chromium.org/12334096/diff/14001/chrome/browser/chrome_bro...
> File chrome/browser/chrome_browser_main_linux.h (right):
> 
>
https://codereview.chromium.org/12334096/diff/14001/chrome/browser/chrome_bro...
> chrome/browser/chrome_browser_main_linux.h:21: class
> MediaTransferProtocolDeviceObserverLinux;
> On 2013/02/28 02:14:43, Lei Zhang wrote:
> > remove as well
> 
> Done.
> 
>
https://codereview.chromium.org/12334096/diff/14001/chrome/browser/storage_mo...
> File
>
chrome/browser/storage_monitor/media_transfer_protocol_device_observer_linux.cc
> (right):
> 
>
https://codereview.chromium.org/12334096/diff/14001/chrome/browser/storage_mo...
>
chrome/browser/storage_monitor/media_transfer_protocol_device_observer_linux.cc:132:
> if (mtp_manager) {
> On 2013/02/28 02:14:43, Lei Zhang wrote:
> > As is, |mtp_manager| is always true here, because
> > MediaTransferProtocolDeviceObserverLinux and MediaTransferProtocolManager
are
> > both only instanciated under the same condition.
> > 
> > Perhaps MediaTransferProtocolManager should be owned by
> > RemovableDeviceNotifications as well.
MediaTransferProtocolDeviceObserverLinux
> > can take a MediaTransferProtocolManager in the ctor instead? We can probably
> get
> > rid of MediaTransferProtocolManager::GetInstance() and have clearer
ownership.
> 
> That sounds like a great option. I'll add that to this change.

Improved CL title/description to follow scope...

[Lei Zhang, 2013-03-01 04:40:15]

lgtm

https://codereview.chromium.org/12334096/diff/22008/chrome/browser/chrome_bro...
File chrome/browser/chrome_browser_main_linux.cc (right):

https://codereview.chromium.org/12334096/diff/22008/chrome/browser/chrome_bro...
chrome/browser/chrome_browser_main_linux.cc:7: #include
"base/message_loop_proxy.h"
message_loop_proxy.h is no longer needed.
chrome_switches.h can go to line 22.

https://codereview.chromium.org/12334096/diff/22008/chrome/browser/chrome_bro...
File chrome/browser/chrome_browser_main_mac.mm (right):

https://codereview.chromium.org/12334096/diff/22008/chrome/browser/chrome_bro...
chrome/browser/chrome_browser_main_mac.mm:290: void
ChromeBrowserMainPartsMac::PostProfileInit() {
This didn't get added to the header.

https://codereview.chromium.org/12334096/diff/22008/chrome/browser/storage_mo...
File chrome/browser/storage_monitor/removable_device_notifications_chromeos.cc
(right):

https://codereview.chromium.org/12334096/diff/22008/chrome/browser/storage_mo...
chrome/browser/storage_monitor/removable_device_notifications_chromeos.cc:97:
device::MediaTransferProtocolManager::Shutdown();
Add a check before calling this to avoid the shutdown dcheck?

if (!CommandLine::ForCurrentProcess()->HasSwitch(switches::kTestType))
...

https://codereview.chromium.org/12334096/diff/22008/chrome/browser/storage_mo...
File chrome/browser/storage_monitor/removable_device_notifications_linux.cc
(right):

https://codereview.chromium.org/12334096/diff/22008/chrome/browser/storage_mo...
chrome/browser/storage_monitor/removable_device_notifications_linux.cc:248:
device::MediaTransferProtocolManager::Shutdown();
ditto

[Greg Billock, 2013-03-01 18:00:50]

https://codereview.chromium.org/12334096/diff/22008/chrome/browser/chrome_bro...
File chrome/browser/chrome_browser_main_linux.cc (right):

https://codereview.chromium.org/12334096/diff/22008/chrome/browser/chrome_bro...
chrome/browser/chrome_browser_main_linux.cc:7: #include
"base/message_loop_proxy.h"
On 2013/03/01 04:40:15, Lei Zhang wrote:
> message_loop_proxy.h is no longer needed.
> chrome_switches.h can go to line 22.

Done.

https://codereview.chromium.org/12334096/diff/22008/chrome/browser/chrome_bro...
File chrome/browser/chrome_browser_main_mac.mm (right):

https://codereview.chromium.org/12334096/diff/22008/chrome/browser/chrome_bro...
chrome/browser/chrome_browser_main_mac.mm:290: void
ChromeBrowserMainPartsMac::PostProfileInit() {
On 2013/03/01 04:40:15, Lei Zhang wrote:
> This didn't get added to the header.

Done.

https://codereview.chromium.org/12334096/diff/22008/chrome/browser/storage_mo...
File chrome/browser/storage_monitor/removable_device_notifications_chromeos.cc
(right):

https://codereview.chromium.org/12334096/diff/22008/chrome/browser/storage_mo...
chrome/browser/storage_monitor/removable_device_notifications_chromeos.cc:97:
device::MediaTransferProtocolManager::Shutdown();
On 2013/03/01 04:40:15, Lei Zhang wrote:
> Add a check before calling this to avoid the shutdown dcheck?
> 
> if (!CommandLine::ForCurrentProcess()->HasSwitch(switches::kTestType))
> ...

Done.

https://codereview.chromium.org/12334096/diff/22008/chrome/browser/storage_mo...
File chrome/browser/storage_monitor/removable_device_notifications_linux.cc
(right):

https://codereview.chromium.org/12334096/diff/22008/chrome/browser/storage_mo...
chrome/browser/storage_monitor/removable_device_notifications_linux.cc:248:
device::MediaTransferProtocolManager::Shutdown();
On 2013/03/01 04:40:15, Lei Zhang wrote:
> ditto

Done.

[vandebo (ex-Chrome), 2013-03-01 22:36:41]

LGTM

https://codereview.chromium.org/12334096/diff/18002/chrome/browser/storage_mo...
File chrome/browser/storage_monitor/removable_device_notifications_mac.h
(right):

https://codereview.chromium.org/12334096/diff/18002/chrome/browser/storage_mo...
chrome/browser/storage_monitor/removable_device_notifications_mac.h:35: void
Init();
Should Init go on StorageMonitor, since we expect all implementations to do two
phase startup?

[Greg Billock, 2013-03-01 23:08:03]

https://codereview.chromium.org/12334096/diff/18002/chrome/browser/storage_mo...
File chrome/browser/storage_monitor/removable_device_notifications_mac.h
(right):

https://codereview.chromium.org/12334096/diff/18002/chrome/browser/storage_mo...
chrome/browser/storage_monitor/removable_device_notifications_mac.h:35: void
Init();
On 2013/03/01 22:36:41, vandebo wrote:
> Should Init go on StorageMonitor, since we expect all implementations to do
two
> phase startup?

I thought about this. I think we do want two-phase init standard. One thought I
had was that there may end up being platform differences in arguments. We could
end up taking some of these created elements as arguments, for instance, which
might ease testing. But mostly this keeps it out of the client interface, which
I thought was good, but realistically the purest thing is probably to make an
interface for that. I guess what I'm saying is I don't have a strong opinion.
:-)

[vandebo (ex-Chrome), 2013-03-01 23:13:01]

On 2013/03/01 23:08:03, Greg Billock wrote:
>
https://codereview.chromium.org/12334096/diff/18002/chrome/browser/storage_mo...
> File chrome/browser/storage_monitor/removable_device_notifications_mac.h
> (right):
> 
>
https://codereview.chromium.org/12334096/diff/18002/chrome/browser/storage_mo...
> chrome/browser/storage_monitor/removable_device_notifications_mac.h:35: void
> Init();
> On 2013/03/01 22:36:41, vandebo wrote:
> > Should Init go on StorageMonitor, since we expect all implementations to do
> two
> > phase startup?
> 
> I thought about this. I think we do want two-phase init standard. One thought
I
> had was that there may end up being platform differences in arguments. We
could
> end up taking some of these created elements as arguments, for instance, which
> might ease testing. But mostly this keeps it out of the client interface,
which
> I thought was good, but realistically the purest thing is probably to make an
> interface for that. I guess what I'm saying is I don't have a strong opinion.
> :-)

I could see each platform having different init arguments. If that's not
currently the case (I don't think it is), I think putting it on the base class
would be cleaner.  So lets do that until we need platform specific Init methods.

[Greg Billock, 2013-03-04 19:21:08]

On 2013/03/01 23:13:01, vandebo wrote:
> On 2013/03/01 23:08:03, Greg Billock wrote:
> >
>
https://codereview.chromium.org/12334096/diff/18002/chrome/browser/storage_mo...
> > File chrome/browser/storage_monitor/removable_device_notifications_mac.h
> > (right):
> > 
> >
>
https://codereview.chromium.org/12334096/diff/18002/chrome/browser/storage_mo...
> > chrome/browser/storage_monitor/removable_device_notifications_mac.h:35: void
> > Init();
> > On 2013/03/01 22:36:41, vandebo wrote:
> > > Should Init go on StorageMonitor, since we expect all implementations to
do
> > two
> > > phase startup?
> > 
> > I thought about this. I think we do want two-phase init standard. One
thought
> I
> > had was that there may end up being platform differences in arguments. We
> could
> > end up taking some of these created elements as arguments, for instance,
which
> > might ease testing. But mostly this keeps it out of the client interface,
> which
> > I thought was good, but realistically the purest thing is probably to make
an
> > interface for that. I guess what I'm saying is I don't have a strong
opinion.
> > :-)
> 
> I could see each platform having different init arguments. If that's not
> currently the case (I don't think it is), I think putting it on the base class
> would be cleaner.  So lets do that until we need platform specific Init
methods.

+zel for chromeos OWNERS. Any issue with normalizing the storage monitor in
ChromeOS with the other platforms?

[commit-bot: I haz the power, 2013-03-05 20:00:23]

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

[commit-bot: I haz the power, 2013-03-05 20:00:29]

Failed to apply patch for chrome/browser/chrome_browser_main_linux.cc:
While running patch -p1 --forward --force --no-backup-if-mismatch;
  patching file chrome/browser/chrome_browser_main_linux.cc
  Hunk #1 succeeded at 4 with fuzz 2.
  Hunk #4 FAILED at 128.
  Hunk #5 succeeded at 163 with fuzz 1 (offset -1 lines).
  1 out of 5 hunks FAILED -- saving rejects to file
chrome/browser/chrome_browser_main_linux.cc.rej

Patch:       chrome/browser/chrome_browser_main_linux.cc
Index: chrome/browser/chrome_browser_main_linux.cc
diff --git a/chrome/browser/chrome_browser_main_linux.cc
b/chrome/browser/chrome_browser_main_linux.cc
index
d7d1c2cbf96a89da05760be962510a10970bb65f..ddf507aac5f0068bcfce91f777d8822cc9b34895
100644
--- a/chrome/browser/chrome_browser_main_linux.cc
+++ b/chrome/browser/chrome_browser_main_linux.cc
@@ -4,11 +4,6 @@
 
 #include "chrome/browser/chrome_browser_main_linux.h"
 
-#include "base/message_loop_proxy.h"
-#include
"chrome/browser/storage_monitor/media_transfer_protocol_device_observer_linux.h"
-#include "chrome/common/chrome_switches.h"
-#include "device/media_transfer_protocol/media_transfer_protocol_manager.h"
-
 #if !defined(OS_CHROMEOS)
 #include
"chrome/browser/storage_monitor/removable_device_notifications_linux.h"
 #include "content/public/browser/browser_thread.h"
@@ -21,6 +16,7 @@
 #include "base/linux_util.h"
 #include "base/prefs/pref_service.h"
 #include "chrome/app/breakpad_linux.h"
+#include "chrome/common/chrome_switches.h"
 #include "chrome/common/env_vars.h"
 #include "chrome/common/pref_names.h"
 
@@ -108,13 +104,10 @@ bool IsCrashReportingEnabled(const PrefService*
local_state) {
 
 ChromeBrowserMainPartsLinux::ChromeBrowserMainPartsLinux(
     const content::MainFunctionParams& parameters)
-    : ChromeBrowserMainPartsPosix(parameters),
-      initialized_media_transfer_protocol_manager_(false) {
+    : ChromeBrowserMainPartsPosix(parameters) {
 }
 
 ChromeBrowserMainPartsLinux::~ChromeBrowserMainPartsLinux() {
-  if (initialized_media_transfer_protocol_manager_)
-    device::MediaTransferProtocolManager::Shutdown();
 }
 
 void ChromeBrowserMainPartsLinux::PreProfileInit() {
@@ -135,30 +128,15 @@ void ChromeBrowserMainPartsLinux::PreProfileInit() {
   const base::FilePath kDefaultMtabPath("/etc/mtab");
   removable_device_notifications_linux_ =
       new chrome::RemovableDeviceNotificationsLinux(kDefaultMtabPath);
-  removable_device_notifications_linux_->Init();
 #endif
 
-  if (!CommandLine::ForCurrentProcess()->HasSwitch(switches::kTestType)) {
-    scoped_refptr<base::MessageLoopProxy> loop_proxy;
-#if !defined(OS_CHROMEOS)
-    loop_proxy = content::BrowserThread::GetMessageLoopProxyForThread(
-        content::BrowserThread::FILE);
-#endif
-    device::MediaTransferProtocolManager::Initialize(loop_proxy);
-    initialized_media_transfer_protocol_manager_ = true;
-  }
-
   ChromeBrowserMainPartsPosix::PreProfileInit();
 }
 
 void ChromeBrowserMainPartsLinux::PostProfileInit() {
-  // TODO(gbillock): Make this owned by RemovableDeviceNotificationsLinux.
-  if (!CommandLine::ForCurrentProcess()->HasSwitch(switches::kTestType)) {
-    media_transfer_protocol_device_observer_.reset(
-        new chrome::MediaTransferProtocolDeviceObserverLinux());
-    media_transfer_protocol_device_observer_->SetNotifications(
-      chrome::StorageMonitor::GetInstance()->receiver());
-  }
+#if !defined(OS_CHROMEOS)
+  removable_device_notifications_linux_->Init();
+#endif
 
   ChromeBrowserMainPartsPosix::PostProfileInit();
 }
@@ -171,7 +149,5 @@ void ChromeBrowserMainPartsLinux::PostMainMessageLoopRun() {
   removable_device_notifications_linux_ = NULL;
 #endif
 
-  media_transfer_protocol_device_observer_.reset();
-
   ChromeBrowserMainPartsPosix::PostMainMessageLoopRun();
 }

[commit-bot: I haz the power, 2013-03-06 21:55:43]

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

[commit-bot: I haz the power, 2013-03-07 02:22:58]

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

[commit-bot: I haz the power, 2013-03-07 17:22:46]

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

[commit-bot: I haz the power, 2013-03-07 21:48:13]

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

[commit-bot: I haz the power, 2013-03-11 16:56:09]

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

[commit-bot: I haz the power, 2013-03-11 18:59:23]

Change committed as 187332

[Joao da Silva, 2013-03-12 14:15:23]

Chrome doesn't shut down cleanly on Linux after this change, see inline.

https://chromiumcodereview.appspot.com/12334096/diff/60001/chrome/browser/sto...
File chrome/browser/storage_monitor/storage_monitor_linux.cc (right):

https://chromiumcodereview.appspot.com/12334096/diff/60001/chrome/browser/sto...
chrome/browser/storage_monitor/storage_monitor_linux.cc:297:
device::MediaTransferProtocolManager::Initialize(loop_proxy);
This DCHECKS on Linux at shutdown.

::Initialize creates the MediaTransferProtocolManagerImpl on the current thread
(UI?), but then ::Shutdown() above deletes it on FILE.

That is a problem because that class has a WeakPtrFactory, which must be deleted
on the same thread on which it was created (WeakPtrs are only valid on the
thread that owns the factory; they can be copied to other threads, but only
tested/used/invalidated on the owner thread.)

Solutions:

- call ::Initialize from InitOnFileThread (this is probably not correct, that
class was being created & used from UI before)

- or post the Shutdown call to the thread that calls Init() (UI?)

- or something else :-)

[Greg Billock, 2013-03-12 16:08:18]

Did you roll back? If not, I can do it.


On Tue, Mar 12, 2013 at 7:15 AM, <joaodasilva@chromium.org> wrote:

> Chrome doesn't shut down cleanly on Linux after this change, see inline.
>
>
> https://chromiumcodereview.**appspot.com/12334096/diff/**
>
60001/chrome/browser/storage_**monitor/storage_monitor_linux.**cc<https://chromiumcodereview.appspot.com/12334096/diff/60001/chrome/browser/storage_monitor/storage_monitor_linux.cc>
> File chrome/browser/storage_**monitor/storage_monitor_linux.**cc (right):
>
> https://chromiumcodereview.**appspot.com/12334096/diff/**
> 60001/chrome/browser/storage_**monitor/storage_monitor_linux.**
>
cc#newcode297<https://chromiumcodereview.appspot.com/12334096/diff/60001/chrome/browser/storage_monitor/storage_monitor_linux.cc#newcode297>
> chrome/browser/storage_**monitor/storage_monitor_linux.**cc:297:
> device::**MediaTransferProtocolManager::**Initialize(loop_proxy);
> This DCHECKS on Linux at shutdown.
>
> ::Initialize creates the MediaTransferProtocolManagerIm**pl on the current
> thread (UI?), but then ::Shutdown() above deletes it on FILE.
>
> That is a problem because that class has a WeakPtrFactory, which must be
> deleted on the same thread on which it was created (WeakPtrs are only
> valid on the thread that owns the factory; they can be copied to other
> threads, but only tested/used/invalidated on the owner thread.)
>
> Solutions:
>
> - call ::Initialize from InitOnFileThread (this is probably not correct,
> that class was being created & used from UI before)
>
> - or post the Shutdown call to the thread that calls Init() (UI?)
>
> - or something else :-)
>
>
https://chromiumcodereview.**appspot.com/12334096/<https://chromiumcodereview...
>

[Joao da Silva, 2013-03-12 16:40:36]

On 2013/03/12 16:08:18, Greg Billock wrote:
> Did you roll back? If not, I can do it.
> 

I didn't, I wasn't sure whether you wanted to fix it or not :-) Go ahead with
the rollback if you think it's better for now.

> 
> On Tue, Mar 12, 2013 at 7:15 AM, <mailto:joaodasilva@chromium.org> wrote:
> 
> > Chrome doesn't shut down cleanly on Linux after this change, see inline.
> >
> >
> > https://chromiumcodereview.**appspot.com/12334096/diff/**
> >
>
60001/chrome/browser/storage_**monitor/storage_monitor_linux.**cc<https://chromiumcodereview.appspot.com/12334096/diff/60001/chrome/browser/storage_monitor/storage_monitor_linux.cc>
> > File chrome/browser/storage_**monitor/storage_monitor_linux.**cc (right):
> >
> > https://chromiumcodereview.**appspot.com/12334096/diff/**
> > 60001/chrome/browser/storage_**monitor/storage_monitor_linux.**
> >
>
cc#newcode297<https://chromiumcodereview.appspot.com/12334096/diff/60001/chrome/browser/storage_monitor/storage_monitor_linux.cc#newcode297>
> > chrome/browser/storage_**monitor/storage_monitor_linux.**cc:297:
> > device::**MediaTransferProtocolManager::**Initialize(loop_proxy);
> > This DCHECKS on Linux at shutdown.
> >
> > ::Initialize creates the MediaTransferProtocolManagerIm**pl on the current
> > thread (UI?), but then ::Shutdown() above deletes it on FILE.
> >
> > That is a problem because that class has a WeakPtrFactory, which must be
> > deleted on the same thread on which it was created (WeakPtrs are only
> > valid on the thread that owns the factory; they can be copied to other
> > threads, but only tested/used/invalidated on the owner thread.)
> >
> > Solutions:
> >
> > - call ::Initialize from InitOnFileThread (this is probably not correct,
> > that class was being created & used from UI before)
> >
> > - or post the Shutdown call to the thread that calls Init() (UI?)
> >
> > - or something else :-)
> >
> >
>
https://chromiumcodereview.**appspot.com/12334096/%3Chttps://chromiumcoderevi...>
> >

[Greg Billock, 2013-03-12 16:49:33]

Done


On Tue, Mar 12, 2013 at 9:40 AM, <joaodasilva@chromium.org> wrote:

> On 2013/03/12 16:08:18, Greg Billock wrote:
>
>> Did you roll back? If not, I can do it.
>>
>
>
> I didn't, I wasn't sure whether you wanted to fix it or not :-) Go ahead
> with
> the rollback if you think it's better for now.
>
>
>
>  On Tue, Mar 12, 2013 at 7:15 AM,
<mailto:joaodasilva@chromium.**org<joaodasilva@chromium.org>>
>> wrote:
>>
>
>  > Chrome doesn't shut down cleanly on Linux after this change, see inline.
>> >
>> >
>> >
https://chromiumcodereview.**a**ppspot.com/12334096/diff/**<http://appspot.co...
>> >
>>
>
> 60001/chrome/browser/storage_****monitor/storage_monitor_**linux.**cc<
> https://**chromiumcodereview.appspot.**com/12334096/diff/60001/**
>
chrome/browser/storage_**monitor/storage_monitor_linux.**cc<https://chromiumcodereview.appspot.com/12334096/diff/60001/chrome/browser/storage_monitor/storage_monitor_linux.cc>
> >
>
>> > File chrome/browser/storage_****monitor/storage_monitor_linux.****cc
>> (right):
>> >
>> >
https://chromiumcodereview.**a**ppspot.com/12334096/diff/**<http://appspot.co...
>> > 60001/chrome/browser/storage_****monitor/storage_monitor_**linux.**
>> >
>>
>
> cc#newcode297<https://**chromiumcodereview.appspot.**
> com/12334096/diff/60001/**chrome/browser/storage_**
>
monitor/storage_monitor_linux.**cc#newcode297<https://chromiumcodereview.appspot.com/12334096/diff/60001/chrome/browser/storage_monitor/storage_monitor_linux.cc#newcode297>
> >
>
>> > chrome/browser/storage_****monitor/storage_monitor_linux.****cc:297:
>> > device::****MediaTransferProtocolManager::****Initialize(loop_proxy);
>>
>> > This DCHECKS on Linux at shutdown.
>> >
>> > ::Initialize creates the MediaTransferProtocolManagerIm****pl on the
>> current
>>
>> > thread (UI?), but then ::Shutdown() above deletes it on FILE.
>> >
>> > That is a problem because that class has a WeakPtrFactory, which must be
>> > deleted on the same thread on which it was created (WeakPtrs are only
>> > valid on the thread that owns the factory; they can be copied to other
>> > threads, but only tested/used/invalidated on the owner thread.)
>> >
>> > Solutions:
>> >
>> > - call ::Initialize from InitOnFileThread (this is probably not correct,
>> > that class was being created & used from UI before)
>> >
>> > - or post the Shutdown call to the thread that calls Init() (UI?)
>> >
>> > - or something else :-)
>> >
>> >
>>
>
> https://chromiumcodereview.**a**ppspot.com/12334096/%3Chttps:/**
>
/chromiumcodereview.appspot.**com/12334096/<http://appspot.com/12334096/%3Chttps://chromiumcodereview.appspot.com/12334096/>
> >
>
>> >
>>
>
>
>
>
https://chromiumcodereview.**appspot.com/12334096/<https://chromiumcodereview...
>

[Greg Billock, 2013-03-12 21:15:29]

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

https://codereview.chromium.org/12334096/diff/60001/chrome/browser/storage_mo...
chrome/browser/storage_monitor/storage_monitor_linux.cc:297:
device::MediaTransferProtocolManager::Initialize(loop_proxy);
Yes, we need to post back to the UI thread to shutdown the manager. (This
construction is running in the startup thread.) Thanks. I'll roll back.

On 2013/03/12 14:15:23, Joao da Silva wrote:
> This DCHECKS on Linux at shutdown.
> 
> ::Initialize creates the MediaTransferProtocolManagerImpl on the current
thread
> (UI?), but then ::Shutdown() above deletes it on FILE.
> 
> That is a problem because that class has a WeakPtrFactory, which must be
deleted
> on the same thread on which it was created (WeakPtrs are only valid on the
> thread that owns the factory; they can be copied to other threads, but only
> tested/used/invalidated on the owner thread.)
> 
> Solutions:
> 
> - call ::Initialize from InitOnFileThread (this is probably not correct, that
> class was being created & used from UI before)
> 
> - or post the Shutdown call to the thread that calls Init() (UI?)
> 
> - or something else :-)

[Greg Billock, 2013-03-12 21:17:11]

On 2013/03/12 21:15:29, Greg Billock wrote:
>
https://codereview.chromium.org/12334096/diff/60001/chrome/browser/storage_mo...
> File chrome/browser/storage_monitor/storage_monitor_linux.cc (right):
> 
>
https://codereview.chromium.org/12334096/diff/60001/chrome/browser/storage_mo...
> chrome/browser/storage_monitor/storage_monitor_linux.cc:297:
> device::MediaTransferProtocolManager::Initialize(loop_proxy);
> Yes, we need to post back to the UI thread to shutdown the manager. (This
> construction is running in the startup thread.) Thanks. I'll roll back.
> 
> On 2013/03/12 14:15:23, Joao da Silva wrote:
> > This DCHECKS on Linux at shutdown.
> > 
> > ::Initialize creates the MediaTransferProtocolManagerImpl on the current
> thread
> > (UI?), but then ::Shutdown() above deletes it on FILE.
> > 
> > That is a problem because that class has a WeakPtrFactory, which must be
> deleted
> > on the same thread on which it was created (WeakPtrs are only valid on the
> > thread that owns the factory; they can be copied to other threads, but only
> > tested/used/invalidated on the owner thread.)
> > 
> > Solutions:
> > 
> > - call ::Initialize from InitOnFileThread (this is probably not correct,
that
> > class was being created & used from UI before)
> > 
> > - or post the Shutdown call to the thread that calls Init() (UI?)
> > 
> > - or something else :-)

Updated to do this. Lei, want to have another look?

[Lei Zhang, 2013-03-18 09:37:51]

lgtm

I'll probably refactor StorageMonitorLinux to separate the UI thread and FILE
thread bits... when I have time.

[commit-bot: I haz the power, 2013-03-18 16:55:21]

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

[commit-bot: I haz the power, 2013-03-18 17:51:20]

Sorry for I got bad news for ya.
Compile failed with a clobber build on mac.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=mac&number...
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-03-19 16:14:40]

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

[commit-bot: I haz the power, 2013-03-19 20:23:29]

Change committed as 189078