Storage Monitor: Make StorageMonitorLinux own the MediaTransferProtocolManager.

Storage Monitor: Make StorageMonitorLinux own the MediaTransferProtocolManager.

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

[Lei Zhang, 2013-04-10 04:31:21]

This replaces https://codereview.chromium.org/12912013/

https://codereview.chromium.org/14016002/diff/1/chrome/browser/storage_monito...
File chrome/browser/storage_monitor/storage_monitor.h (right):

https://codereview.chromium.org/14016002/diff/1/chrome/browser/storage_monito...
chrome/browser/storage_monitor/storage_monitor.h:87: virtual
device::MediaTransferProtocolManager*
This is not ideal, as discussed in person at some point. Can you remind me what
ideas you had? Put this only in StorageMonitorLinux?

[Greg Billock, 2013-04-10 18:27:18]

On 2013/04/10 04:31:21, Lei Zhang wrote:
> This replaces https://codereview.chromium.org/12912013/
> 
>
https://codereview.chromium.org/14016002/diff/1/chrome/browser/storage_monito...
> File chrome/browser/storage_monitor/storage_monitor.h (right):
> 
>
https://codereview.chromium.org/14016002/diff/1/chrome/browser/storage_monito...
> chrome/browser/storage_monitor/storage_monitor.h:87: virtual
> device::MediaTransferProtocolManager*
> This is not ideal, as discussed in person at some point. Can you remind me
what
> ideas you had? Put this only in StorageMonitorLinux?

I think this is fine for now. It definitely moves us towards the goal. I think
it'd be best to make an MTP delegate hook into StorageMonitor and exchange the
right objects there. This is progress towards being able to do that, though.
Even further might be to have the MTP objects in media_galleries/linux take a
StorageMonitor instance arg, and then use that to access the manager instead of
using the static. That'd set up the right lines of control and show where they
need to be for the other platforms.

[Greg Billock, 2013-04-10 18:27:24]

https://codereview.chromium.org/14016002/diff/11001/chrome/browser/storage_mo...
File chrome/browser/storage_monitor/storage_monitor_linux.cc (left):

https://codereview.chromium.org/14016002/diff/11001/chrome/browser/storage_mo...
chrome/browser/storage_monitor/storage_monitor_linux.cc:240:
device::MediaTransferProtocolManager::Shutdown();
Still need to call this, right? Or does the destructor now do it?

https://codereview.chromium.org/14016002/diff/11001/chrome/browser/storage_mo...
File chrome/browser/storage_monitor/storage_monitor_linux.h (right):

https://codereview.chromium.org/14016002/diff/11001/chrome/browser/storage_mo...
chrome/browser/storage_monitor/storage_monitor_linux.h:55: void
UseDummyMediaTransferProtocolManagerForTest();
Could we make this take a mock object? Or is the manager not very mockable in
the way we need?

https://codereview.chromium.org/14016002/diff/11001/device/media_transfer_pro...
File device/media_transfer_protocol/media_transfer_protocol_manager.cc (right):

https://codereview.chromium.org/14016002/diff/11001/device/media_transfer_pro...
device/media_transfer_protocol/media_transfer_protocol_manager.cc:443: class
MediaTransferProtocolManagerDummyImpl
If we have this, can we stick it in a test-util somewhere and then inject it
into the StorageMonitorLinux?

https://codereview.chromium.org/14016002/diff/11001/device/media_transfer_pro...
File device/media_transfer_protocol/media_transfer_protocol_manager.h (right):

https://codereview.chromium.org/14016002/diff/11001/device/media_transfer_pro...
device/media_transfer_protocol/media_transfer_protocol_manager.h:135: // Creates
and return the global MediaTransferProtocolManager instance.
returns

[Lei Zhang, 2013-04-11 06:17:04]

Sure. But what sucks currently is StorageMonitorLinux checks to see if it is
running in a browser test and then selects the real MTPManager vs a dummy one.
Thus the dummy one has to be compiled into the chrome binary. Even if we move
the "is this a test" check out of StorageMonitorLinux, it would end up in
ChromeBrowserMainLinux, which is still part of the chrome binary.

Instead, if we make |g_browser_process| own StorageMonitor, then we can:
1) Only inject the DummyMTPManager from TestingBrowserProcess. Thus
DummyMTPManager won't be in the chrome binary.
2) Stop having to muck around with multiple chrome_browser_main* files.

How about we do it the sucky way here and put in a TODO to move to
g_browser_process ?

[Lei Zhang, 2013-04-11 06:54:04]

https://codereview.chromium.org/14016002/diff/11001/chrome/browser/storage_mo...
File chrome/browser/storage_monitor/storage_monitor_linux.cc (left):

https://codereview.chromium.org/14016002/diff/11001/chrome/browser/storage_mo...
chrome/browser/storage_monitor/storage_monitor_linux.cc:240:
device::MediaTransferProtocolManager::Shutdown();
On 2013/04/10 18:27:25, Greg Billock wrote:
> Still need to call this, right? Or does the destructor now do it?

The dtor does it now.

https://codereview.chromium.org/14016002/diff/11001/chrome/browser/storage_mo...
File chrome/browser/storage_monitor/storage_monitor_linux.h (right):

https://codereview.chromium.org/14016002/diff/11001/chrome/browser/storage_mo...
chrome/browser/storage_monitor/storage_monitor_linux.h:55: void
UseDummyMediaTransferProtocolManagerForTest();
On 2013/04/10 18:27:25, Greg Billock wrote:
> Could we make this take a mock object? Or is the manager not very mockable in
> the way we need?

Done.

https://codereview.chromium.org/14016002/diff/11001/device/media_transfer_pro...
File device/media_transfer_protocol/media_transfer_protocol_manager.cc (right):

https://codereview.chromium.org/14016002/diff/11001/device/media_transfer_pro...
device/media_transfer_protocol/media_transfer_protocol_manager.cc:443: class
MediaTransferProtocolManagerDummyImpl
On 2013/04/10 18:27:25, Greg Billock wrote:
> If we have this, can we stick it in a test-util somewhere and then inject it
> into the StorageMonitorLinux?

Done.

https://codereview.chromium.org/14016002/diff/11001/device/media_transfer_pro...
File device/media_transfer_protocol/media_transfer_protocol_manager.h (right):

https://codereview.chromium.org/14016002/diff/11001/device/media_transfer_pro...
device/media_transfer_protocol/media_transfer_protocol_manager.h:135: // Creates
and return the global MediaTransferProtocolManager instance.
On 2013/04/10 18:27:25, Greg Billock wrote:
> returns

Done.

[Lei Zhang, 2013-04-16 20:32:43]

PTAL at patch set 4.

[Greg Billock, 2013-04-16 22:52:04]

lgtm

[commit-bot: I haz the power, 2013-04-18 00:11:34]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/thestig@chromium.org/14016002/51001

[commit-bot: I haz the power, 2013-04-18 00:48:39]

Sorry for I got bad news for ya.
Compile failed with a clobber build on android_clang_dbg.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_cl...
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.

[Lei Zhang, 2013-04-18 02:06:56]

There's a couple small fixes in patch set 6 and 7.

[Greg Billock, 2013-04-18 02:49:46]

On 2013/04/18 02:06:56, Lei Zhang wrote:
> There's a couple small fixes in patch set 6 and 7.

lgtm

[commit-bot: I haz the power, 2013-04-18 02:57:19]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/thestig@chromium.org/14016002/96001

[commit-bot: I haz the power, 2013-04-18 05:47:17]

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

[commit-bot: I haz the power, 2013-04-18 19:12:30]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/thestig@chromium.org/14016002/96001

[commit-bot: I haz the power, 2013-04-18 20:36:41]

Change committed as 194994