[CrosDisksClient] Determine device type from DeviceMediaType property sent by cros-disks.

[CrosDisksClient] Determine device type from DeviceMediaType property sent by cros-disks.

BUG=None
TEST=trybots


Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=119980

[tbarzic, 2012-01-23 22:08:09]

Hey guys, can you take a look?

achuith -> imageburner_ui.cc
satorux, hashimoto -> cros_disks_client/disk_manager

[achuithb, 2012-01-23 22:17:17]

http://codereview.chromium.org/9019002/diff/3002/chrome/browser/ui/webui/chro...
File chrome/browser/ui/webui/chromeos/imageburner/imageburner_ui.cc (right):

http://codereview.chromium.org/9019002/diff/3002/chrome/browser/ui/webui/chro...
chrome/browser/ui/webui/chromeos/imageburner/imageburner_ui.cc:59: bool
AllowBurnToDevice(const DiskMountManager::Disk* disk) {
might be nicer to use const DiskMountManager::Disk& instead of Disk*. pointers
are generally used for out params, and references for in params.

http://codereview.chromium.org/9019002/diff/3002/chrome/browser/ui/webui/chro...
chrome/browser/ui/webui/chromeos/imageburner/imageburner_ui.cc:180: const
DiskMountManager::Disk* disk) {
Disk&?

[satorux1, 2012-01-23 22:18:54]

dbus stuff LGTM except for style nits.

http://codereview.chromium.org/9019002/diff/3002/chrome/browser/chromeos/dbus...
File chrome/browser/chromeos/dbus/cros_disks_client.cc (right):

http://codereview.chromium.org/9019002/diff/3002/chrome/browser/chromeos/dbus...
chrome/browser/chromeos/dbus/cros_disks_client.cc:30: // Translate enum used in
cros-disks to enum used in Chrome.
"Translates" per C++ style guide.

http://codereview.chromium.org/9019002/diff/3002/chrome/browser/chromeos/dbus...
chrome/browser/chromeos/dbus/cros_disks_client.cc:42: return
DEVICE_TYPE_OPTICAL;
DEVICE_TYPE_OPTICAL_DISC would be better for consistency.

http://codereview.chromium.org/9019002/diff/3002/chrome/browser/chromeos/dbus...
chrome/browser/chromeos/dbus/cros_disks_client.cc:50: // Check if retrieved
media type is in boundaries of DeviceMediaType.
Checks

http://codereview.chromium.org/9019002/diff/3002/chrome/browser/chromeos/dbus...
chrome/browser/chromeos/dbus/cros_disks_client.cc:51: bool
MediaTypeInBoundaries(uint32 type) {
maybe IsValidMediaType() would be a bit better name?

http://codereview.chromium.org/9019002/diff/3002/chrome/browser/chromeos/dbus...
chrome/browser/chromeos/dbus/cros_disks_client.cc:56: // Pop unit32 value and
converts it to DeviceMediaType when |reader| is not
Pops

http://codereview.chromium.org/9019002/diff/3002/chrome/browser/chromeos/dbus...
chrome/browser/chromeos/dbus/cros_disks_client.cc:62: uint32 media_type_uint32;
uint32 media_type_uint32 = 0;

just in case.

http://codereview.chromium.org/9019002/diff/3002/chrome/browser/chromeos/dbus...
File chrome/browser/chromeos/dbus/cros_disks_client.h (right):

http://codereview.chromium.org/9019002/diff/3002/chrome/browser/chromeos/dbus...
chrome/browser/chromeos/dbus/cros_disks_client.h:35: DEVICE_TYPE_OPTICAL,
Adding a comment may be helpful.

DEVICE_TYPE_OPTICAL,  // ex. DVD.

http://codereview.chromium.org/9019002/diff/3002/chrome/browser/chromeos/dbus...
chrome/browser/chromeos/dbus/cros_disks_client.h:36: DEVICE_TYPE_MOBILE
what's mobile? could you add some comment?

[tbarzic, 2012-01-23 22:42:39]

http://codereview.chromium.org/9019002/diff/3002/chrome/browser/chromeos/dbus...
File chrome/browser/chromeos/dbus/cros_disks_client.cc (right):

http://codereview.chromium.org/9019002/diff/3002/chrome/browser/chromeos/dbus...
chrome/browser/chromeos/dbus/cros_disks_client.cc:30: // Translate enum used in
cros-disks to enum used in Chrome.
On 2012/01/23 22:18:55, satorux1 wrote:
> "Translates" per C++ style guide.

Done.

http://codereview.chromium.org/9019002/diff/3002/chrome/browser/chromeos/dbus...
chrome/browser/chromeos/dbus/cros_disks_client.cc:42: return
DEVICE_TYPE_OPTICAL;
On 2012/01/23 22:18:55, satorux1 wrote:
> DEVICE_TYPE_OPTICAL_DISC would be better for consistency.

Done.

http://codereview.chromium.org/9019002/diff/3002/chrome/browser/chromeos/dbus...
chrome/browser/chromeos/dbus/cros_disks_client.cc:50: // Check if retrieved
media type is in boundaries of DeviceMediaType.
On 2012/01/23 22:18:55, satorux1 wrote:
> Checks

Done.

http://codereview.chromium.org/9019002/diff/3002/chrome/browser/chromeos/dbus...
chrome/browser/chromeos/dbus/cros_disks_client.cc:51: bool
MediaTypeInBoundaries(uint32 type) {
On 2012/01/23 22:18:55, satorux1 wrote:
> maybe IsValidMediaType() would be a bit better name?

Done.

http://codereview.chromium.org/9019002/diff/3002/chrome/browser/chromeos/dbus...
chrome/browser/chromeos/dbus/cros_disks_client.cc:56: // Pop unit32 value and
converts it to DeviceMediaType when |reader| is not
On 2012/01/23 22:18:55, satorux1 wrote:
> Pops

Done.

http://codereview.chromium.org/9019002/diff/3002/chrome/browser/chromeos/dbus...
chrome/browser/chromeos/dbus/cros_disks_client.cc:62: uint32 media_type_uint32;
On 2012/01/23 22:18:55, satorux1 wrote:
> uint32 media_type_uint32 = 0;
> 
> just in case.

Done.

http://codereview.chromium.org/9019002/diff/3002/chrome/browser/chromeos/dbus...
File chrome/browser/chromeos/dbus/cros_disks_client.h (right):

http://codereview.chromium.org/9019002/diff/3002/chrome/browser/chromeos/dbus...
chrome/browser/chromeos/dbus/cros_disks_client.h:35: DEVICE_TYPE_OPTICAL,
On 2012/01/23 22:18:55, satorux1 wrote:
> Adding a comment may be helpful.
> 
> DEVICE_TYPE_OPTICAL,  // ex. DVD.
> 

Done.

http://codereview.chromium.org/9019002/diff/3002/chrome/browser/chromeos/dbus...
chrome/browser/chromeos/dbus/cros_disks_client.h:36: DEVICE_TYPE_MOBILE
On 2012/01/23 22:18:55, satorux1 wrote:
> what's mobile? could you add some comment?

Done.

http://codereview.chromium.org/9019002/diff/3002/chrome/browser/ui/webui/chro...
File chrome/browser/ui/webui/chromeos/imageburner/imageburner_ui.cc (right):

http://codereview.chromium.org/9019002/diff/3002/chrome/browser/ui/webui/chro...
chrome/browser/ui/webui/chromeos/imageburner/imageburner_ui.cc:59: bool
AllowBurnToDevice(const DiskMountManager::Disk* disk) {
On 2012/01/23 22:17:17, achuith.bhandarkar wrote:
> might be nicer to use const DiskMountManager::Disk& instead of Disk*. pointers
> are generally used for out params, and references for in params.

Done.

http://codereview.chromium.org/9019002/diff/3002/chrome/browser/ui/webui/chro...
chrome/browser/ui/webui/chromeos/imageburner/imageburner_ui.cc:180: const
DiskMountManager::Disk* disk) {
On 2012/01/23 22:17:17, achuith.bhandarkar wrote:
> Disk&?

This is DiskMountManager observer method, so I would leave it like this for now
(we can change DiskMountManager to send reference instead of pointer later, in
separate CL)

[achuithb, 2012-01-23 22:44:11]

On 2012/01/23 22:42:39, tbarzic wrote:
>
http://codereview.chromium.org/9019002/diff/3002/chrome/browser/chromeos/dbus...
> File chrome/browser/chromeos/dbus/cros_disks_client.cc (right):
> 
>
http://codereview.chromium.org/9019002/diff/3002/chrome/browser/chromeos/dbus...
> chrome/browser/chromeos/dbus/cros_disks_client.cc:30: // Translate enum used
in
> cros-disks to enum used in Chrome.
> On 2012/01/23 22:18:55, satorux1 wrote:
> > "Translates" per C++ style guide.
> 
> Done.
> 
>
http://codereview.chromium.org/9019002/diff/3002/chrome/browser/chromeos/dbus...
> chrome/browser/chromeos/dbus/cros_disks_client.cc:42: return
> DEVICE_TYPE_OPTICAL;
> On 2012/01/23 22:18:55, satorux1 wrote:
> > DEVICE_TYPE_OPTICAL_DISC would be better for consistency.
> 
> Done.
> 
>
http://codereview.chromium.org/9019002/diff/3002/chrome/browser/chromeos/dbus...
> chrome/browser/chromeos/dbus/cros_disks_client.cc:50: // Check if retrieved
> media type is in boundaries of DeviceMediaType.
> On 2012/01/23 22:18:55, satorux1 wrote:
> > Checks
> 
> Done.
> 
>
http://codereview.chromium.org/9019002/diff/3002/chrome/browser/chromeos/dbus...
> chrome/browser/chromeos/dbus/cros_disks_client.cc:51: bool
> MediaTypeInBoundaries(uint32 type) {
> On 2012/01/23 22:18:55, satorux1 wrote:
> > maybe IsValidMediaType() would be a bit better name?
> 
> Done.
> 
>
http://codereview.chromium.org/9019002/diff/3002/chrome/browser/chromeos/dbus...
> chrome/browser/chromeos/dbus/cros_disks_client.cc:56: // Pop unit32 value and
> converts it to DeviceMediaType when |reader| is not
> On 2012/01/23 22:18:55, satorux1 wrote:
> > Pops
> 
> Done.
> 
>
http://codereview.chromium.org/9019002/diff/3002/chrome/browser/chromeos/dbus...
> chrome/browser/chromeos/dbus/cros_disks_client.cc:62: uint32
media_type_uint32;
> On 2012/01/23 22:18:55, satorux1 wrote:
> > uint32 media_type_uint32 = 0;
> > 
> > just in case.
> 
> Done.
> 
>
http://codereview.chromium.org/9019002/diff/3002/chrome/browser/chromeos/dbus...
> File chrome/browser/chromeos/dbus/cros_disks_client.h (right):
> 
>
http://codereview.chromium.org/9019002/diff/3002/chrome/browser/chromeos/dbus...
> chrome/browser/chromeos/dbus/cros_disks_client.h:35: DEVICE_TYPE_OPTICAL,
> On 2012/01/23 22:18:55, satorux1 wrote:
> > Adding a comment may be helpful.
> > 
> > DEVICE_TYPE_OPTICAL,  // ex. DVD.
> > 
> 
> Done.
> 
>
http://codereview.chromium.org/9019002/diff/3002/chrome/browser/chromeos/dbus...
> chrome/browser/chromeos/dbus/cros_disks_client.h:36: DEVICE_TYPE_MOBILE
> On 2012/01/23 22:18:55, satorux1 wrote:
> > what's mobile? could you add some comment?
> 
> Done.
> 
>
http://codereview.chromium.org/9019002/diff/3002/chrome/browser/ui/webui/chro...
> File chrome/browser/ui/webui/chromeos/imageburner/imageburner_ui.cc (right):
> 
>
http://codereview.chromium.org/9019002/diff/3002/chrome/browser/ui/webui/chro...
> chrome/browser/ui/webui/chromeos/imageburner/imageburner_ui.cc:59: bool
> AllowBurnToDevice(const DiskMountManager::Disk* disk) {
> On 2012/01/23 22:17:17, achuith.bhandarkar wrote:
> > might be nicer to use const DiskMountManager::Disk& instead of Disk*.
pointers
> > are generally used for out params, and references for in params.
> 
> Done.
> 
>
http://codereview.chromium.org/9019002/diff/3002/chrome/browser/ui/webui/chro...
> chrome/browser/ui/webui/chromeos/imageburner/imageburner_ui.cc:180: const
> DiskMountManager::Disk* disk) {
> On 2012/01/23 22:17:17, achuith.bhandarkar wrote:
> > Disk&?
> 
> This is DiskMountManager observer method, so I would leave it like this for
now
> (we can change DiskMountManager to send reference instead of pointer later, in
> separate CL)

owner lgtm for imageburner_ui.cc. Please note that I haven't looked at the rest
of the files.

[hashimoto, 2012-01-24 05:28:06]

LGTM with nit for cros_disks_client.* and disk_mount_manager.*

http://codereview.chromium.org/9019002/diff/17001/chrome/browser/chromeos/dbu...
File chrome/browser/chromeos/dbus/cros_disks_client.cc (right):

http://codereview.chromium.org/9019002/diff/17001/chrome/browser/chromeos/dbu...
chrome/browser/chromeos/dbus/cros_disks_client.cc:1: // Copyright (c) 2011 The
Chromium Authors. All rights reserved.
Please fix copyright year (in all files).

http://codereview.chromium.org/9019002/diff/17001/chrome/browser/chromeos/dbu...
chrome/browser/chromeos/dbus/cros_disks_client.cc:53: type >= 0;
Since |type| is unsigned, type >= 0 does nothing here.
It may be removed.

http://codereview.chromium.org/9019002/diff/17001/chrome/browser/chromeos/dbu...
chrome/browser/chromeos/dbus/cros_disks_client.cc:58: bool
MaybePopDeviceType(dbus::MessageReader* reader, DeviceType* type) {
Having MaybePopUint32 and doing something like 'device_type_ =
DeviceMediaTypeToDeviceType(media_type_uint32);' in InitializeFromResponse will
be more consistent with other MaybePopXX functions.

[tbarzic, 2012-01-24 19:47:33]

http://codereview.chromium.org/9019002/diff/17001/chrome/browser/chromeos/dbu...
File chrome/browser/chromeos/dbus/cros_disks_client.cc (right):

http://codereview.chromium.org/9019002/diff/17001/chrome/browser/chromeos/dbu...
chrome/browser/chromeos/dbus/cros_disks_client.cc:1: // Copyright (c) 2011 The
Chromium Authors. All rights reserved.
On 2012/01/24 05:28:06, hashimoto wrote:
> Please fix copyright year (in all files).

Done.

http://codereview.chromium.org/9019002/diff/17001/chrome/browser/chromeos/dbu...
chrome/browser/chromeos/dbus/cros_disks_client.cc:53: type >= 0;
On 2012/01/24 05:28:06, hashimoto wrote:
> Since |type| is unsigned, type >= 0 does nothing here.
> It may be removed.

Done.

http://codereview.chromium.org/9019002/diff/17001/chrome/browser/chromeos/dbu...
chrome/browser/chromeos/dbus/cros_disks_client.cc:58: bool
MaybePopDeviceType(dbus::MessageReader* reader, DeviceType* type) {
On 2012/01/24 05:28:06, hashimoto wrote:
> Having MaybePopUint32 and doing something like 'device_type_ =
> DeviceMediaTypeToDeviceType(media_type_uint32);' in InitializeFromResponse
will
> be more consistent with other MaybePopXX functions.

Done.

[asargent_no_longer_on_chrome, 2012-01-24 23:02:15]

chrome/{browser,common,test/data}/extensions/* lgtm

[hashimoto, 2012-01-25 04:15:44]

lgtm

lgtm

[commit-bot: I haz the power, 2012-01-26 00:20:19]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/tbarzic@chromium.org/9019002/23001

[commit-bot: I haz the power, 2012-01-26 00:20:27]

Presubmit check for 9019002-23001 failed and returned exit status 1.

Running presubmit commit checks ...

** Presubmit ERRORS **
Missing LGTM from an OWNER for:
chrome/browser/resources/file_manager/js/file_manager.js

Presubmit checks took 2.2s to calculate.

[tbarzic, 2012-01-26 00:24:06]

Dmitry,
I need an OWNERS approval for file_manager.js..

Toni

[Dmitry Zvorygin, 2012-01-27 11:14:54]

On 2012/01/26 00:24:06, tbarzic wrote:
> Dmitry,
> I need an OWNERS approval for file_manager.js..
> 
> Toni
Checked fileBrowserPrivate and file_manager. LGTM.

[commit-bot: I haz the power, 2012-01-31 21:25:24]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/tbarzic@chromium.org/9019002/34001

[commit-bot: I haz the power, 2012-01-31 23:07:00]

Change committed as 119980