Propagate the read_only_hardware flag of volumes to js.

Propagate the read_only_hardware flag of volumes to js.
Only for removable disk partitions.
The property will be used to distinguish between write-protect devices
and restricted write access by the policy. UI code will be able to
provide better messages based on that.

BUG=643693, 655003
TEST=manual test. Deploy to a device and set breakpoint in backgroun_script.js at the beginning of DeviceHandler.prototype.onMountCompleted(). See event.volumeMetadata has isReadoOnlyHardware value reflecting the attached device.

Committed: https://crrev.com/dd19a48cddc73cdfea90e644ad05084aeb75ffb0
Cr-Commit-Position: refs/heads/master@{#429223}

[yamaguchi, 2016-10-25 09:44:08]

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

[commit-bot: I haz the power, 2016-10-25 09:44:28]

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-10-25 10:55:19]

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

[commit-bot: I haz the power, 2016-10-25 10:55:20]

Dry run: Try jobs failed on following builders:
  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-10-25 12:36:10]

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

[commit-bot: I haz the power, 2016-10-25 12:36:24]

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

[yamaguchi, 2016-10-25 13:15:19]

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

[commit-bot: I haz the power, 2016-10-25 13:15:33]

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-10-25 14:17:36]

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

[commit-bot: I haz the power, 2016-10-25 14:17:37]

Dry run: This issue passed the CQ dry run.

[yamaguchi, 2016-10-26 03:41:02]

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

[commit-bot: I haz the power, 2016-10-26 03:41:38]

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-10-26 04:06:56]

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

[commit-bot: I haz the power, 2016-10-26 04:06:57]

Dry run: Try jobs failed on following builders:
  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-10-26 04:45:49]

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

[commit-bot: I haz the power, 2016-10-26 04:46:11]

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-10-26 05:56:05]

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

[commit-bot: I haz the power, 2016-10-26 05:56:06]

Dry run: This issue passed the CQ dry run.

[yamaguchi, 2016-10-26 06:13:10]

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

[yamaguchi, 2016-10-26 06:13:10]

ptal

[fukino, 2016-10-26 07:23:32]

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

https://codereview.chromium.org/2451713002/diff/60001/chrome/browser/chromeos...
chrome/browser/chromeos/file_manager/volume_manager.cc:157: watchable_(false) {}
nit: looks unnecessary change (the style is not consistent with the dtor)

https://codereview.chromium.org/2451713002/diff/60001/chrome/browser/chromeos...
File chrome/browser/chromeos/file_manager/volume_manager.h (right):

https://codereview.chromium.org/2451713002/diff/60001/chrome/browser/chromeos...
chrome/browser/chromeos/file_manager/volume_manager.h:124: // When this value is
true, is_read_only() is also true.
Could you mention an example in which is_read_only_ is true but
is_read_only_hardware_ is false.

In CL description or code comment, could you explain why we need to have these
two separated fields.

[yamaguchi, 2016-10-26 11:05:07]

Description was changed from

==========
Propagate the read_only_hardware flag of volumes to js.
Only for removable disk partitions.

BUG=643693,655003
TEST=manual test. Deploy to a device and set breakpoint in backgroun_script.js
at the beginning of DeviceHandler.prototype.onMountCompleted(). See
event.volumeMetadata has isReadoOnlyHardware value reflecting the attached
device.
==========

to

==========
Propagate the read_only_hardware flag of volumes to js.
Only for removable disk partitions.
The property will be used to distinguish between write-protect devices and
restricted write access by the policy. UI code will be allowed to provide better
messaging using the field value.

BUG=643693,655003
TEST=manual test. Deploy to a device and set breakpoint in backgroun_script.js
at the beginning of DeviceHandler.prototype.onMountCompleted(). See
event.volumeMetadata has isReadoOnlyHardware value reflecting the attached
device.
==========

[yamaguchi, 2016-10-26 11:08:07]

Description was changed from

==========
Propagate the read_only_hardware flag of volumes to js.
Only for removable disk partitions.
The property will be used to distinguish between write-protect devices and
restricted write access by the policy. UI code will be allowed to provide better
messaging using the field value.

BUG=643693,655003
TEST=manual test. Deploy to a device and set breakpoint in backgroun_script.js
at the beginning of DeviceHandler.prototype.onMountCompleted(). See
event.volumeMetadata has isReadoOnlyHardware value reflecting the attached
device.
==========

to

==========
Propagate the read_only_hardware flag of volumes to js.
Only for removable disk partitions.
The property will be used to distinguish between write-protect devices
and restricted write access by the policy. UI code will be able to
provide better messages based on that.

BUG=643693,655003
TEST=manual test. Deploy to a device and set breakpoint in backgroun_script.js
at the beginning of DeviceHandler.prototype.onMountCompleted(). See
event.volumeMetadata has isReadoOnlyHardware value reflecting the attached
device.
==========

[yamaguchi, 2016-10-26 11:14:20]

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

https://codereview.chromium.org/2451713002/diff/60001/chrome/browser/chromeos...
chrome/browser/chromeos/file_manager/volume_manager.cc:157: watchable_(false) {}
On 2016/10/26 07:23:31, fukino wrote:
> nit: looks unnecessary change (the style is not consistent with the dtor)

Done.

https://codereview.chromium.org/2451713002/diff/60001/chrome/browser/chromeos...
File chrome/browser/chromeos/file_manager/volume_manager.h (right):

https://codereview.chromium.org/2451713002/diff/60001/chrome/browser/chromeos...
chrome/browser/chromeos/file_manager/volume_manager.h:124: // When this value is
true, is_read_only() is also true.
On 2016/10/26 07:23:31, fukino wrote:
> Could you mention an example in which is_read_only_ is true but
> is_read_only_hardware_ is false.
> 
> In CL description or code comment, could you explain why we need to have these
> two separated fields.

Done.
Described for what this change is, in the CL description.

[fukino, 2016-10-27 05:27:34]

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

https://codereview.chromium.org/2451713002/diff/100001/chrome/browser/chromeo...
chrome/browser/chromeos/file_manager/volume_manager.cc:215:
volume->is_read_only_ = volume->is_read_only_hardware_ =
Should is_read_only_hardware_ be true?
The archive is not hardware so I'm a bit confused here.

https://codereview.chromium.org/2451713002/diff/100001/chrome/browser/chromeo...
chrome/browser/chromeos/file_manager/volume_manager.cc:249:
volume->is_read_only_hardware_ = volume->is_read_only_;
ditto.
FPS volumes are not necessary haredware.

https://codereview.chromium.org/2451713002/diff/100001/chrome/browser/chromeo...
File chrome/browser/chromeos/file_manager/volume_manager.h (right):

https://codereview.chromium.org/2451713002/diff/100001/chrome/browser/chromeo...
chrome/browser/chromeos/file_manager/volume_manager.h:128: bool
is_read_only_hardware() const { return is_read_only_hardware_; }
optional nit: is_read_only_device might be more consistent naming with
SOURCE_DEVICE?

[yamaguchi, 2016-10-27 07:12:36]

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

[commit-bot: I haz the power, 2016-10-27 07:13:04]

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

[yamaguchi, 2016-10-27 07:32:54]

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

https://codereview.chromium.org/2451713002/diff/100001/chrome/browser/chromeo...
chrome/browser/chromeos/file_manager/volume_manager.cc:215:
volume->is_read_only_ = volume->is_read_only_hardware_ =
On 2016/10/27 05:27:34, fukino wrote:
> Should is_read_only_hardware_ be true?
> The archive is not hardware so I'm a bit confused here.

Now that we state the is_read_only_device field is only valid for removable
device partitions, I think it should leave the field in its default value for
other cases.

https://codereview.chromium.org/2451713002/diff/100001/chrome/browser/chromeo...
chrome/browser/chromeos/file_manager/volume_manager.cc:249:
volume->is_read_only_hardware_ = volume->is_read_only_;
On 2016/10/27 05:27:34, fukino wrote:
> ditto.
> FPS volumes are not necessary haredware.

Done.

https://codereview.chromium.org/2451713002/diff/100001/chrome/browser/chromeo...
File chrome/browser/chromeos/file_manager/volume_manager.h (right):

https://codereview.chromium.org/2451713002/diff/100001/chrome/browser/chromeo...
chrome/browser/chromeos/file_manager/volume_manager.h:128: bool
is_read_only_hardware() const { return is_read_only_hardware_; }
On 2016/10/27 05:27:34, fukino wrote:
> optional nit: is_read_only_device might be more consistent naming with
> SOURCE_DEVICE?

In Disk class, "hardware" was adopted for the field name to emphasize the
difference between mount option (given by software) and the device itself.
Regarding the new fields in this change, I agree that it'd be better to have
relevant name to either the VOLUME_TYPE_REMOVABLE_DISK_PARTITION or
SOURCE_DEVICE.

[commit-bot: I haz the power, 2016-10-27 09:35:44]

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

[commit-bot: I haz the power, 2016-10-27 09:35:45]

Dry run: This issue passed the CQ dry run.

[fukino, 2016-10-27 11:39:09]

https://codereview.chromium.org/2451713002/diff/120001/chrome/browser/chromeo...
File chrome/browser/chromeos/file_manager/volume_manager.h (right):

https://codereview.chromium.org/2451713002/diff/120001/chrome/browser/chromeo...
chrome/browser/chromeos/file_manager/volume_manager.h:128: bool
is_read_only_device() const { return is_read_only_device_; }
This field is valid only when the volume type is removable disk AND the source
is device.
To avoid misuse of this field, how about use more descriptive(longer) name, like
is_read_only_removable_device?

When we add more meaning for this field, let's change the name.

[yamaguchi, 2016-10-27 11:59:59]

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

[commit-bot: I haz the power, 2016-10-27 12:00: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-10-27 12:58:24]

https://codereview.chromium.org/2451713002/diff/120001/chrome/browser/chromeo...
File chrome/browser/chromeos/file_manager/volume_manager.h (right):

https://codereview.chromium.org/2451713002/diff/120001/chrome/browser/chromeo...
chrome/browser/chromeos/file_manager/volume_manager.h:128: bool
is_read_only_device() const { return is_read_only_device_; }
On 2016/10/27 11:39:08, fukino wrote:
> This field is valid only when the volume type is removable disk AND the source
> is device.
> To avoid misuse of this field, how about use more descriptive(longer) name,
like
> is_read_only_removable_device?
> 
> When we add more meaning for this field, let's change the name. 

Agreed. Done.

[fukino, 2016-10-27 13:04:11]

lgtm

[commit-bot: I haz the power, 2016-10-27 13:14:53]

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

[commit-bot: I haz the power, 2016-10-27 13:14:56]

Dry run: This issue passed the CQ dry run.

[yamaguchi, 2016-10-27 13:33:28]

The CQ bit was checked by yamaguchi@chromium.org

[commit-bot: I haz the power, 2016-10-27 13:33:40]

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-10-27 13:39:59]

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

[commit-bot: I haz the power, 2016-10-27 13:40:00]

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

[yamaguchi, 2016-10-27 13:46:56]

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

[yamaguchi, 2016-10-27 13:46:57]

+mtomasz
Will you review and approve the change to
chrome/common/extensions/api/file_manager_private.idl ?

[mtomasz, 2016-11-01 06:16:07]

lgtm. Sorry for late. Please always ping me if I don't respond in 24h.

[mtomasz, 2016-11-01 06:16:07]

lgtm. Sorry for late. Please always ping me if I don't respond in 24h.

[yamaguchi, 2016-11-02 05:48:36]

The CQ bit was checked by yamaguchi@chromium.org

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

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-11-02 06:47:23]

Description was changed from

==========
Propagate the read_only_hardware flag of volumes to js.
Only for removable disk partitions.
The property will be used to distinguish between write-protect devices
and restricted write access by the policy. UI code will be able to
provide better messages based on that.

BUG=643693,655003
TEST=manual test. Deploy to a device and set breakpoint in backgroun_script.js
at the beginning of DeviceHandler.prototype.onMountCompleted(). See
event.volumeMetadata has isReadoOnlyHardware value reflecting the attached
device.
==========

to

==========
Propagate the read_only_hardware flag of volumes to js.
Only for removable disk partitions.
The property will be used to distinguish between write-protect devices
and restricted write access by the policy. UI code will be able to
provide better messages based on that.

BUG=643693,655003
TEST=manual test. Deploy to a device and set breakpoint in backgroun_script.js
at the beginning of DeviceHandler.prototype.onMountCompleted(). See
event.volumeMetadata has isReadoOnlyHardware value reflecting the attached
device.
==========

[commit-bot: I haz the power, 2016-11-02 06:47:25]

Committed patchset #8 (id:140001)

[commit-bot: I haz the power, 2016-11-02 06:49:40]

Description was changed from

==========
Propagate the read_only_hardware flag of volumes to js.
Only for removable disk partitions.
The property will be used to distinguish between write-protect devices
and restricted write access by the policy. UI code will be able to
provide better messages based on that.

BUG=643693,655003
TEST=manual test. Deploy to a device and set breakpoint in backgroun_script.js
at the beginning of DeviceHandler.prototype.onMountCompleted(). See
event.volumeMetadata has isReadoOnlyHardware value reflecting the attached
device.
==========

to

==========
Propagate the read_only_hardware flag of volumes to js.
Only for removable disk partitions.
The property will be used to distinguish between write-protect devices
and restricted write access by the policy. UI code will be able to
provide better messages based on that.

BUG=643693,655003
TEST=manual test. Deploy to a device and set breakpoint in backgroun_script.js
at the beginning of DeviceHandler.prototype.onMountCompleted(). See
event.volumeMetadata has isReadoOnlyHardware value reflecting the attached
device.

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

[commit-bot: I haz the power, 2016-11-02 06:49:42]

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