Change access mode of disk devices when mounting based on config.

Change access mode of disk devices when mounting based on config.
BUG=629945
Committed: https://crrev.com/cc8186b7a74efde6bfd4e553f9fb3fc1fb72713c
Cr-Commit-Position: refs/heads/master@{#410309}

[yamaguchi, 2016-08-02 10:33:59]

yamaguchi@chromium.org changed reviewers:
+ fukino@chromium.org

[fukino, 2016-08-03 07:18:18]

fukino@chromium.org changed reviewers:
+ satorux@chromium.org

[fukino, 2016-08-03 07:18:19]

+satorux for chromeos/disks/.

https://codereview.chromium.org/2201023002/diff/1/chrome/browser/chromeos/ext...
File chrome/browser/chromeos/extensions/file_manager/private_api_mount.cc
(right):

https://codereview.chromium.org/2201023002/diff/1/chrome/browser/chromeos/ext...
chrome/browser/chromeos/extensions/file_manager/private_api_mount.cc:173:
display_name.AsUTF8Unsafe(), chromeos::MOUNT_TYPE_ARCHIVE, access_mode);
Should we specify access mode for mounted archive files?

https://codereview.chromium.org/2201023002/diff/1/chrome/browser/chromeos/fil...
File chrome/browser/chromeos/file_manager/volume_manager.cc (right):

https://codereview.chromium.org/2201023002/diff/1/chrome/browser/chromeos/fil...
chrome/browser/chromeos/file_manager/volume_manager.cc:497: 
nit: unintentional blank line?

https://codereview.chromium.org/2201023002/diff/1/chrome/browser/chromeos/fil...
chrome/browser/chromeos/file_manager/volume_manager.cc:885:
chromeos::MountAccessMode VolumeManager::GetExternalStorageAccessMode() {
This function can be defined under unnamed namespace as non-member function.
(Which returns access mode for given profile)

[satorux1, 2016-08-03 07:29:33]

chromeos/disks lgtm with a request

https://codereview.chromium.org/2201023002/diff/1/chromeos/disks/disk_mount_m...
File chromeos/disks/disk_mount_manager.h (right):

https://codereview.chromium.org/2201023002/diff/1/chromeos/disks/disk_mount_m...
chromeos/disks/disk_mount_manager.h:265: MountAccessMode access_mode) = 0;
while you are at it, could you add comments about the parameters?

[yamaguchi, 2016-08-03 10:22:14]

https://codereview.chromium.org/2201023002/diff/1/chrome/browser/chromeos/ext...
File chrome/browser/chromeos/extensions/file_manager/private_api_mount.cc
(right):

https://codereview.chromium.org/2201023002/diff/1/chrome/browser/chromeos/ext...
chrome/browser/chromeos/extensions/file_manager/private_api_mount.cc:173:
display_name.AsUTF8Unsafe(), chromeos::MOUNT_TYPE_ARCHIVE, access_mode);
On 2016/08/03 07:18:19, fukino wrote:
> Should we specify access mode for mounted archive files?

Done.
Mounted archives are treated as read-only by itself now.

https://codereview.chromium.org/2201023002/diff/1/chrome/browser/chromeos/fil...
File chrome/browser/chromeos/file_manager/volume_manager.cc (right):

https://codereview.chromium.org/2201023002/diff/1/chrome/browser/chromeos/fil...
chrome/browser/chromeos/file_manager/volume_manager.cc:497: 
On 2016/08/03 07:18:19, fukino wrote:
> nit: unintentional blank line?

Done.

https://codereview.chromium.org/2201023002/diff/1/chrome/browser/chromeos/fil...
chrome/browser/chromeos/file_manager/volume_manager.cc:885:
chromeos::MountAccessMode VolumeManager::GetExternalStorageAccessMode() {
On 2016/08/03 07:18:19, fukino wrote:
> This function can be defined under unnamed namespace as non-member function.
> (Which returns access mode for given profile)

Done.

https://codereview.chromium.org/2201023002/diff/1/chromeos/disks/disk_mount_m...
File chromeos/disks/disk_mount_manager.h (right):

https://codereview.chromium.org/2201023002/diff/1/chromeos/disks/disk_mount_m...
chromeos/disks/disk_mount_manager.h:265: MountAccessMode access_mode) = 0;
On 2016/08/03 07:29:32, satorux1 wrote:
> while you are at it, could you add comments about the parameters?

Done.

[fukino, 2016-08-04 02:22:38]

lgtm with a nit

https://codereview.chromium.org/2201023002/diff/30001/chromeos/disks/disk_mou...
File chromeos/disks/disk_mount_manager.h (right):

https://codereview.chromium.org/2201023002/diff/30001/chromeos/disks/disk_mou...
chromeos/disks/disk_mount_manager.h:261: // access_mode specifies read-only or
read-write mount mode for a device.
access_mode => |access_mode|

[fukino, 2016-08-04 02:22:39]

lgtm with a nit

[yamaguchi, 2016-08-04 02:32:57]

https://codereview.chromium.org/2201023002/diff/30001/chromeos/disks/disk_mou...
File chromeos/disks/disk_mount_manager.h (right):

https://codereview.chromium.org/2201023002/diff/30001/chromeos/disks/disk_mou...
chromeos/disks/disk_mount_manager.h:261: // access_mode specifies read-only or
read-write mount mode for a device.
On 2016/08/04 02:22:38, fukino wrote:
> access_mode => |access_mode|

Done.

[yamaguchi, 2016-08-04 02:33:02]

The CQ bit was checked by yamaguchi@chromium.org

[yamaguchi, 2016-08-04 02:33:03]

The patchset sent to the CQ was uploaded after l-g-t-m from
satorux@chromium.org, fukino@chromium.org
Link to the patchset: https://codereview.chromium.org/2201023002/#ps50001
(title: "Fix comment.")

[commit-bot: I haz the power, 2016-08-04 02:33:23]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-08-04 02:51:09]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-08-04 02:51:10]

Try jobs failed on following builders:
  linux_chromium_chromeos_compile_dbg_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[yamaguchi, 2016-08-04 04:43:14]

The CQ bit was checked by yamaguchi@chromium.org to run a CQ dry run

[yamaguchi, 2016-08-04 04:43:19]

The CQ bit was unchecked by yamaguchi@chromium.org

[yamaguchi, 2016-08-04 04:43:20]

The CQ bit was checked by yamaguchi@chromium.org

[yamaguchi, 2016-08-04 04:43:21]

The CQ bit was unchecked by yamaguchi@chromium.org

[yamaguchi, 2016-08-04 04:43:21]

The patchset sent to the CQ was uploaded after l-g-t-m from
satorux@chromium.org, fukino@chromium.org
Link to the patchset: https://codereview.chromium.org/2201023002/#ps70001
(title: "Update fake class and tests to follow the change.")

[yamaguchi, 2016-08-04 04:43:25]

The CQ bit was unchecked by yamaguchi@chromium.org

[commit-bot: I haz the power, 2016-08-04 04:43:28]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[yamaguchi, 2016-08-04 04:43:32]

The CQ bit was checked by yamaguchi@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-08-04 04:43:48]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[yamaguchi, 2016-08-04 04:50:55]

Updated the fake class and some tests to resolve compilation errors. patl.

[commit-bot: I haz the power, 2016-08-04 05:25:47]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-08-04 05:25:48]

Dry run: Try jobs failed on following builders:
  linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[yamaguchi, 2016-08-04 05:48:30]

The CQ bit was checked by yamaguchi@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-08-04 05:49:05]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-08-04 06:26:07]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-08-04 06:26:07]

Dry run: Try jobs failed on following builders:
  linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[yamaguchi, 2016-08-04 08:06:37]

The CQ bit was checked by yamaguchi@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-08-04 08:06:48]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[yamaguchi, 2016-08-04 08:23:38]

The CQ bit was checked by yamaguchi@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-08-04 08:23:46]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-08-04 08:46:32]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-08-04 08:46:34]

Dry run: Try jobs failed on following builders:
  linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[yamaguchi, 2016-08-04 08:59:17]

The CQ bit was checked by yamaguchi@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-08-04 08:59:27]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[yamaguchi, 2016-08-04 09:22:55]

The CQ bit was checked by yamaguchi@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-08-04 09:23:12]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[yamaguchi, 2016-08-04 09:43:31]

PTAL.
Do we have any other ways to avoid errors when adding preference values like
this?

Trybot dryrun is still running, but I think it's likely to pass as the
previously failed ones are passing.

https://codereview.chromium.org/2201023002/diff/90001/chrome/browser/chromeos...
File chrome/browser/chromeos/file_manager/volume_manager.cc (right):

https://codereview.chromium.org/2201023002/diff/90001/chrome/browser/chromeos...
chrome/browser/chromeos/file_manager/volume_manager.cc:140:
profile->GetPrefs()->GetBoolean(prefs::kExternalStorageReadOnly);
GetBoolean returns false when the value did not exist, but DCHECK() will abort
the unit test (VolumeManagerTest.OnDiskEvent_Changed) and Debug build.

[commit-bot: I haz the power, 2016-08-04 10:09:00]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-08-04 10:09:01]

Dry run: This issue passed the CQ dry run.

[yamaguchi, 2016-08-05 05:41:33]

The CQ bit was checked by yamaguchi@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-08-05 05:41:54]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-08-05 05:44:29]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-08-05 05:44:31]

Dry run: This issue passed the CQ dry run.

[yamaguchi, 2016-08-05 05:47:49]

The CQ bit was checked by yamaguchi@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-08-05 05:48:02]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[yamaguchi, 2016-08-05 06:11:38]

yamaguchi@chromium.org changed reviewers:
+ hashimoto@chromium.org

[yamaguchi, 2016-08-05 06:11:39]

+hashimoto for the code handling preference.

[yamaguchi, 2016-08-05 06:14:44]

https://codereview.chromium.org/2201023002/diff/110001/chrome/browser/chromeo...
File chrome/browser/chromeos/file_manager/volume_manager.cc (right):

https://codereview.chromium.org/2201023002/diff/110001/chrome/browser/chromeo...
chrome/browser/chromeos/file_manager/volume_manager.cc:140:
profile->GetPrefs()->HasPrefPath(prefs::kExternalStorageReadOnly)
Reverted this check in Patch Set 8 because Has*() should return the default
value if the path is registered.

[hashimoto, 2016-08-05 06:24:28]

lgtm

https://codereview.chromium.org/2201023002/diff/130001/chrome/browser/chromeo...
File chrome/browser/chromeos/file_manager/volume_manager_unittest.cc (right):

https://codereview.chromium.org/2201023002/diff/130001/chrome/browser/chromeo...
chrome/browser/chromeos/file_manager/volume_manager_unittest.cc:675:
TEST_F(VolumeManagerTest, OnExternalStorageDisabledChanged) {
Are we going to have a test which ensures that the value of
kExternalStorageReadonly pref affects the access mode value?
IIUC currently there is no such test yet.

[commit-bot: I haz the power, 2016-08-05 06:55:53]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-08-05 06:55:55]

Dry run: This issue passed the CQ dry run.

[yamaguchi, 2016-08-05 07:05:12]

plat

https://codereview.chromium.org/2201023002/diff/130001/chrome/browser/chromeo...
File chrome/browser/chromeos/file_manager/volume_manager_unittest.cc (right):

https://codereview.chromium.org/2201023002/diff/130001/chrome/browser/chromeo...
chrome/browser/chromeos/file_manager/volume_manager_unittest.cc:675:
TEST_F(VolumeManagerTest, OnExternalStorageDisabledChanged) {
On 2016/08/05 06:24:28, hashimoto wrote:
> Are we going to have a test which ensures that the value of
> kExternalStorageReadonly pref affects the access mode value?
> IIUC currently there is no such test yet.

Done.

[yamaguchi, 2016-08-05 07:05:22]

PTAL

[hashimoto, 2016-08-05 07:27:47]

https://codereview.chromium.org/2201023002/diff/150001/chrome/browser/chromeo...
File chrome/browser/chromeos/file_manager/fake_disk_mount_manager.cc (right):

https://codereview.chromium.org/2201023002/diff/150001/chrome/browser/chromeo...
chrome/browser/chromeos/file_manager/fake_disk_mount_manager.cc:17: bool
read_only)
Shouldn't this be MountAccessMode instead of bool?

[yamaguchi, 2016-08-08 03:36:29]

The CQ bit was checked by yamaguchi@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-08-08 03:36:44]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-08-08 04:33:58]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-08-08 04:33:59]

Dry run: This issue passed the CQ dry run.

[yamaguchi, 2016-08-08 05:31:49]

The CQ bit was checked by yamaguchi@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-08-08 05:32:02]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[yamaguchi, 2016-08-08 05:53:44]

https://codereview.chromium.org/2201023002/diff/150001/chrome/browser/chromeo...
File chrome/browser/chromeos/file_manager/fake_disk_mount_manager.cc (right):

https://codereview.chromium.org/2201023002/diff/150001/chrome/browser/chromeo...
chrome/browser/chromeos/file_manager/fake_disk_mount_manager.cc:17: bool
read_only)
On 2016/08/05 07:27:46, hashimoto wrote:
> Shouldn't this be MountAccessMode instead of bool?

Done.

[hashimoto, 2016-08-08 05:58:25]

lgtm

[commit-bot: I haz the power, 2016-08-08 06:18:27]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-08-08 06:18:28]

Dry run: This issue passed the CQ dry run.

[yamaguchi, 2016-08-08 06:35:42]

The CQ bit was checked by yamaguchi@chromium.org

[yamaguchi, 2016-08-08 06:35:42]

The patchset sent to the CQ was uploaded after l-g-t-m from
satorux@chromium.org, fukino@chromium.org
Link to the patchset: https://codereview.chromium.org/2201023002/#ps190001
(title: "Fix typo.")

[commit-bot: I haz the power, 2016-08-08 06:35:59]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-08-08 06:39:13]

Committed patchset #11 (id:190001)

[commit-bot: I haz the power, 2016-08-08 06:41:11]

Description was changed from

==========
Change access mode of disk devices when mounting based on config.
BUG=629945
==========

to

==========
Change access mode of disk devices when mounting based on config.
BUG=629945

Committed: https://crrev.com/cc8186b7a74efde6bfd4e553f9fb3fc1fb72713c
Cr-Commit-Position: refs/heads/master@{#410309}
==========

[commit-bot: I haz the power, 2016-08-08 06:41:13]

Patchset 11 (id:??) landed as
https://crrev.com/cc8186b7a74efde6bfd4e553f9fb3fc1fb72713c
Cr-Commit-Position: refs/heads/master@{#410309}