Consolidate storage_monitor/MediaDeviceNotificationsUtils into MediaStorageUtil.

Consolidate storage_monitor/MediaDeviceNotificationsUtils into MediaStorageUtil.

BUG=NONE
TBR=thestig@chromium.org,vandebo@chromium.org

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

[Greg Billock, 2013-03-06 21:48:27]

https://codereview.chromium.org/12544005/diff/10002/chrome/browser/storage_mo...
File chrome/browser/storage_monitor/media_device_notifications_utils_unittest.cc
(left):

https://codereview.chromium.org/12544005/diff/10002/chrome/browser/storage_mo...
chrome/browser/storage_monitor/media_device_notifications_utils_unittest.cc:76:
// Test to verify that IsMediaDevice() function returns true for the given
Change name

https://codereview.chromium.org/12544005/diff/10002/chrome/browser/storage_mo...
chrome/browser/storage_monitor/media_device_notifications_utils_unittest.cc:88:
// Test to verify that IsMediaDevice() function returns false for a given
and here

https://codereview.chromium.org/12544005/diff/10002/chrome/browser/storage_mo...
File chrome/browser/storage_monitor/media_storage_util.h (right):

https://codereview.chromium.org/12544005/diff/10002/chrome/browser/storage_mo...
chrome/browser/storage_monitor/media_storage_util.h:42: // Returns true, if it
is a media device otherwise return false.
Can you improve the wording here? Perhaps "Returns true if it does, otherwise
returns false"

https://codereview.chromium.org/12544005/diff/10002/chrome/browser/storage_mo...
File chrome/browser/storage_monitor/media_storage_util_unittest.cc (right):

https://codereview.chromium.org/12544005/diff/10002/chrome/browser/storage_mo...
chrome/browser/storage_monitor/media_storage_util_unittest.cc:37:
file_thread_(BrowserThread::FILE) { }
No space between braces

https://codereview.chromium.org/12544005/diff/10002/chrome/browser/storage_mo...
chrome/browser/storage_monitor/media_storage_util_unittest.cc:41: void
checkDeviceType(const base::FilePath::StringType& mount_point,
Capitalize

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

https://codereview.chromium.org/12544005/diff/10002/chrome/browser/storage_mo...
chrome/browser/storage_monitor/storage_monitor_linux.cc:456: StorageInfo
storage_info(device_id, display_name, mount_point.value());
I'd get rid of this temporary here and elsewhere. (The one for display_name
seems fine to me.)

[Greg Billock, 2013-03-06 21:50:53]

https://codereview.chromium.org/12544005/diff/10002/chrome/browser/storage_mo...
File chrome/browser/storage_monitor/media_storage_util_unittest.cc (right):

https://codereview.chromium.org/12544005/diff/10002/chrome/browser/storage_mo...
chrome/browser/storage_monitor/media_storage_util_unittest.cc:158: MessageLoop
loop;
Probably need to use the test loop here.

[tommycli, 2013-03-07 19:36:48]

Addressed your comments to fix up the test and formatting.

Not sure if it fixed the unittest, as I think unittests are prematurely exiting
due to other reasons.

On 2013/03/06 21:50:53, Greg Billock wrote:
>
https://codereview.chromium.org/12544005/diff/10002/chrome/browser/storage_mo...
> File chrome/browser/storage_monitor/media_storage_util_unittest.cc (right):
> 
>
https://codereview.chromium.org/12544005/diff/10002/chrome/browser/storage_mo...
> chrome/browser/storage_monitor/media_storage_util_unittest.cc:158: MessageLoop
> loop;
> Probably need to use the test loop here.

[Greg Billock, 2013-03-07 20:05:21]

On 2013/03/07 19:36:48, tommycli wrote:
> Addressed your comments to fix up the test and formatting.
> 
> Not sure if it fixed the unittest, as I think unittests are prematurely
exiting
> due to other reasons.
> 
> On 2013/03/06 21:50:53, Greg Billock wrote:
> >
>
https://codereview.chromium.org/12544005/diff/10002/chrome/browser/storage_mo...
> > File chrome/browser/storage_monitor/media_storage_util_unittest.cc (right):
> > 
> >
>
https://codereview.chromium.org/12544005/diff/10002/chrome/browser/storage_mo...
> > chrome/browser/storage_monitor/media_storage_util_unittest.cc:158:
MessageLoop
> > loop;
> > Probably need to use the test loop here.

lgtm

[Greg Billock, 2013-03-07 20:05:29]

https://chromiumcodereview.appspot.com/12544005/diff/10002/chrome/browser/sto...
File chrome/browser/storage_monitor/media_storage_util.h (right):

https://chromiumcodereview.appspot.com/12544005/diff/10002/chrome/browser/sto...
chrome/browser/storage_monitor/media_storage_util.h:42: // Returns true, if it
is a media device otherwise return false.
On 2013/03/06 21:48:27, Greg Billock wrote:
> Can you improve the wording here? Perhaps "Returns true if it does, otherwise
> returns false"

Usual practice is to click 'Done' on these when you deal with them -- that then
gets summarized so the reviewer can go to any leftover spots easily.

[commit-bot: I haz the power, 2013-03-07 20:40:19]

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

[commit-bot: I haz the power, 2013-03-07 23:10:42]

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

[commit-bot: I haz the power, 2013-03-08 00:02:32]

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

[commit-bot: I haz the power, 2013-03-08 01:37:17]

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

[commit-bot: I haz the power, 2013-03-08 02:00:26]

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

[commit-bot: I haz the power, 2013-03-08 02:41:33]

Sorry for I got bad news for ya.
Compile failed with a clobber build on linux_chromeos.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_chro...
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-08 18:34:44]

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

[commit-bot: I haz the power, 2013-03-08 20:28:31]

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

[commit-bot: I haz the power, 2013-03-08 21:54:15]

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

[commit-bot: I haz the power, 2013-03-08 23:02:23]

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

[commit-bot: I haz the power, 2013-03-11 17:28:10]

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

[tommycli, 2013-03-11 17:31:01]

In case you desire to review how I changed the test to make it not hang.

https://chromiumcodereview.appspot.com/12544005/diff/108001/chrome/browser/st...
File chrome/browser/storage_monitor/media_storage_util_unittest.cc (right):

https://chromiumcodereview.appspot.com/12544005/diff/108001/chrome/browser/st...
chrome/browser/storage_monitor/media_storage_util_unittest.cc:161:
base::WaitableEvent event(true, false);
Run FilterAttachedDevices on the FILE thread.

[Greg Billock, 2013-03-11 18:07:43]

lgtm

https://chromiumcodereview.appspot.com/12544005/diff/108001/chrome/browser/st...
File chrome/browser/storage_monitor/media_storage_util_unittest.cc (right):

https://chromiumcodereview.appspot.com/12544005/diff/108001/chrome/browser/st...
chrome/browser/storage_monitor/media_storage_util_unittest.cc:161:
base::WaitableEvent event(true, false);
On 2013/03/11 17:31:01, tommycli wrote:
> Run FilterAttachedDevices on the FILE thread.

This is fine. FilterAttachedDevices bounces to the FILE thread immediately
anyway.

[commit-bot: I haz the power, 2013-03-11 22:00:48]

Failed to apply patch for chrome/browser/storage_monitor/storage_monitor_mac.mm:
While running patch -p1 --forward --force --no-backup-if-mismatch;
  patching file chrome/browser/storage_monitor/storage_monitor_mac.mm
  Hunk #1 FAILED at 4.
  Hunk #2 succeeded at 153 (offset 53 lines).
  1 out of 2 hunks FAILED -- saving rejects to file
chrome/browser/storage_monitor/storage_monitor_mac.mm.rej

Patch:       chrome/browser/storage_monitor/storage_monitor_mac.mm
Index: chrome/browser/storage_monitor/storage_monitor_mac.mm
diff --git a/chrome/browser/storage_monitor/storage_monitor_mac.mm
b/chrome/browser/storage_monitor/storage_monitor_mac.mm
index
d98ddf184229766f9c3f00ef800686338f99ad45..ec6f73a3d2d7e44073dab8325acf076135809a57
100644
--- a/chrome/browser/storage_monitor/storage_monitor_mac.mm
+++ b/chrome/browser/storage_monitor/storage_monitor_mac.mm
@@ -4,7 +4,7 @@
 
 #include "chrome/browser/storage_monitor/storage_monitor_mac.h"
 
-#include "chrome/browser/storage_monitor/media_device_notifications_utils.h"
+#include "chrome/browser/storage_monitor/media_storage_util.h"
 #include "content/public/browser/browser_thread.h"
 
 namespace chrome {
@@ -100,7 +100,7 @@ void StorageMonitorMac::UpdateDisk(const DiskInfoMac& info,
     MediaStorageUtil::RecordDeviceInfoHistogram(true, info.device_id(),
                                                 info.device_name());
     if (ShouldPostNotificationForDisk(info)) {
-      string16 display_name = GetDisplayNameForDevice(
+      string16 display_name = MediaStorageUtil::GetDisplayNameForDevice(
           info.total_size_in_bytes(), info.device_name());
       receiver()->ProcessAttach(StorageInfo(
           info.device_id(), display_name, info.mount_point().value()));

[commit-bot: I haz the power, 2013-03-11 22:21:30]

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

[vandebo (ex-Chrome), 2013-03-11 22:27:02]

https://chromiumcodereview.appspot.com/12544005/diff/108001/chrome/browser/st...
File chrome/browser/storage_monitor/media_storage_util_unittest.cc (right):

https://chromiumcodereview.appspot.com/12544005/diff/108001/chrome/browser/st...
chrome/browser/storage_monitor/media_storage_util_unittest.cc:161:
base::WaitableEvent event(true, false);
On 2013/03/11 17:31:01, tommycli wrote:
> Run FilterAttachedDevices on the FILE thread.

FYI - I think another way that would have worked to fix this issue was to make a
message loop for the FILE thread.  Run it until idle first, then run the UI
message loop until idle.  Then you could remove the waitable event and just
check that you got the result.

[commit-bot: I haz the power, 2013-03-12 01:05:23]

Change committed as 187438