Add USB device permissions to individual site settings on Android.

Add USB device permissions to individual site settings on Android.

This patch makes it possible to view and revoke permission to access a
USB device by going to an origin's "site settings" page.

BUG=430999, 601627
Committed: https://crrev.com/7280deea31d7794becbc7f86fb0870a129c54439
Cr-Commit-Position: refs/heads/master@{#405887}

[Reilly Grant (use Gerrit), 2016-07-12 21:42:31]

Description was changed from

==========
Add USB device permissions to Site Settings on Android.

BUG=601627
==========

to

==========
Add USB device permissions to individual site settings on Android.

This patch makes it possible to view and revoke permission to access a
USB device by going to an origin's "site settings" page.

BUG=430999,601627
==========

[Reilly Grant (use Gerrit), 2016-07-12 21:42:31]

reillyg@chromium.org changed reviewers:
+ twellington@chromium.org

[Reilly Grant (use Gerrit), 2016-07-12 21:45:58]

reillyg@chromium.org changed reviewers:
+ bauerb@chromium.org, yusufo@chromium.org

[Reilly Grant (use Gerrit), 2016-07-12 21:45:58]

bauerb@, please review //chrome/browser/android.
twellington@, please review //chrome/android/java/src.
yusufo@, please review //chrome/android/java/res.

[Theresa, 2016-07-13 00:11:58]

https://codereview.chromium.org/2134463002/diff/20001/chrome/android/java/res...
File chrome/android/java/res/layout/usb_permission.xml (right):

https://codereview.chromium.org/2134463002/diff/20001/chrome/android/java/res...
chrome/android/java/res/layout/usb_permission.xml:10:
android:layout_height="wrap_content"
This should have an android:contentDescription so that when accessibility mode
is turned out something meaningful is read out rather than "Button 456729".

https://codereview.chromium.org/2134463002/diff/20001/chrome/android/java/res...
chrome/android/java/res/layout/usb_permission.xml:12:
chrome:tint="@color/blue_mode_tint"/>
Is the 'x' supposed to be blue? iirc, in the mocks it was grey.

As an aside, can you please include a link to the mocks in one of the bugs and
screenshots of the in-flight CL?

https://codereview.chromium.org/2134463002/diff/20001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/preferences/website/UsbPermission.java
(right):

https://codereview.chromium.org/2134463002/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/preferences/website/UsbPermission.java:24:
// TODO(reillyg): Add a content description.
Can this be handled as part of this CL rather than a follow-up?

[Reilly Grant (use Gerrit), 2016-07-13 00:45:19]

https://codereview.chromium.org/2134463002/diff/20001/chrome/android/java/res...
File chrome/android/java/res/layout/usb_permission.xml (right):

https://codereview.chromium.org/2134463002/diff/20001/chrome/android/java/res...
chrome/android/java/res/layout/usb_permission.xml:10:
android:layout_height="wrap_content"
On 2016/07/13 at 00:11:58, Theresa Wellington wrote:
> This should have an android:contentDescription so that when accessibility mode
is turned out something meaningful is read out rather than "Button 456729".

Done.

https://codereview.chromium.org/2134463002/diff/20001/chrome/android/java/res...
chrome/android/java/res/layout/usb_permission.xml:12:
chrome:tint="@color/blue_mode_tint"/>
On 2016/07/13 at 00:11:58, Theresa Wellington wrote:
> Is the 'x' supposed to be blue? iirc, in the mocks it was grey.

Fixed.

> As an aside, can you please include a link to the mocks in one of the bugs and
screenshots of the in-flight CL?

Added a screenshot to crbug.com/601627.

https://codereview.chromium.org/2134463002/diff/20001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/preferences/website/UsbPermission.java
(right):

https://codereview.chromium.org/2134463002/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/preferences/website/UsbPermission.java:24:
// TODO(reillyg): Add a content description.
On 2016/07/13 at 00:11:58, Theresa Wellington wrote:
> Can this be handled as part of this CL rather than a follow-up?

Done.

[Bernhard Bauer, 2016-07-13 08:54:45]

https://codereview.chromium.org/2134463002/diff/40001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/preferences/website/UsbInfo.java
(right):

https://codereview.chromium.org/2134463002/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/preferences/website/UsbInfo.java:12:
public class UsbInfo implements Serializable {
I would consider using Parcelable instead of Serializable if it's only going to
be used in memory (see
http://stackoverflow.com/questions/3611843/is-using-serializable-in-android-b...).

https://codereview.chromium.org/2134463002/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/preferences/website/UsbInfo.java:15:
private final boolean mIsIncognito;
This is never used :)

https://codereview.chromium.org/2134463002/diff/40001/chrome/browser/android/...
File chrome/browser/android/preferences/website_preference_bridge.cc (right):

https://codereview.chromium.org/2134463002/diff/40001/chrome/browser/android/...
chrome/browser/android/preferences/website_preference_bridge.cc:445: for (const
auto& object : context->GetAllGrantedObjects()) {
Can you use the actual type here instead of auto?

https://codereview.chromium.org/2134463002/diff/40001/chrome/browser/android/...
chrome/browser/android/preferences/website_preference_bridge.cc:446: // Remove
the trailing backslashes so that origins are matched correctly in
That's a (forward) slash.

https://codereview.chromium.org/2134463002/diff/40001/chrome/browser/android/...
chrome/browser/android/preferences/website_preference_bridge.cc:449:
DCHECK(origin[origin.size() - 1] == '/');
DCHECK_EQ?

https://codereview.chromium.org/2134463002/diff/40001/chrome/browser/android/...
chrome/browser/android/preferences/website_preference_bridge.cc:455: embedder =
embedder.substr(0, embedder.size() - 1);
You could use embedder.resize().

https://codereview.chromium.org/2134463002/diff/40001/chrome/browser/android/...
chrome/browser/android/preferences/website_preference_bridge.cc:465:
JSONStringValueSerializer serializer(&serialized);
You could use JSONWriter::Write (which is what this uses internally anyway) to
be consistent with JSONReader below.

[Reilly Grant (use Gerrit), 2016-07-13 21:15:19]

Patchset #4 (id:60001) has been deleted

[Reilly Grant (use Gerrit), 2016-07-13 21:20:16]

https://codereview.chromium.org/2134463002/diff/40001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/preferences/website/UsbInfo.java
(right):

https://codereview.chromium.org/2134463002/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/preferences/website/UsbInfo.java:12:
public class UsbInfo implements Serializable {
On 2016/07/13 at 08:54:45, Bernhard Bauer wrote:
> I would consider using Parcelable instead of Serializable if it's only going
to be used in memory (see
http://stackoverflow.com/questions/3611843/is-using-serializable-in-android-b...).

Done.

https://codereview.chromium.org/2134463002/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/preferences/website/UsbInfo.java:15:
private final boolean mIsIncognito;
On 2016/07/13 at 08:54:45, Bernhard Bauer wrote:
> This is never used :)

Removed.

https://codereview.chromium.org/2134463002/diff/40001/chrome/browser/android/...
File chrome/browser/android/preferences/website_preference_bridge.cc (right):

https://codereview.chromium.org/2134463002/diff/40001/chrome/browser/android/...
chrome/browser/android/preferences/website_preference_bridge.cc:445: for (const
auto& object : context->GetAllGrantedObjects()) {
On 2016/07/13 at 08:54:45, Bernhard Bauer wrote:
> Can you use the actual type here instead of auto?

It's rather long. We use auto above in GetOrigins.

https://codereview.chromium.org/2134463002/diff/40001/chrome/browser/android/...
chrome/browser/android/preferences/website_preference_bridge.cc:446: // Remove
the trailing backslashes so that origins are matched correctly in
On 2016/07/13 at 08:54:45, Bernhard Bauer wrote:
> That's a (forward) slash.

Fixed.

https://codereview.chromium.org/2134463002/diff/40001/chrome/browser/android/...
chrome/browser/android/preferences/website_preference_bridge.cc:449:
DCHECK(origin[origin.size() - 1] == '/');
On 2016/07/13 at 08:54:45, Bernhard Bauer wrote:
> DCHECK_EQ?

Done.

https://codereview.chromium.org/2134463002/diff/40001/chrome/browser/android/...
chrome/browser/android/preferences/website_preference_bridge.cc:455: embedder =
embedder.substr(0, embedder.size() - 1);
On 2016/07/13 at 08:54:45, Bernhard Bauer wrote:
> You could use embedder.resize().

Someone came by and switched all the other instances of this pattern in this
file to use back() and pop_back() so I've done the same for this and origin
above.

https://codereview.chromium.org/2134463002/diff/40001/chrome/browser/android/...
chrome/browser/android/preferences/website_preference_bridge.cc:465:
JSONStringValueSerializer serializer(&serialized);
On 2016/07/13 at 08:54:45, Bernhard Bauer wrote:
> You could use JSONWriter::Write (which is what this uses internally anyway) to
be consistent with JSONReader below.

Done.

[Bernhard Bauer, 2016-07-14 10:18:12]

lgtm

[Theresa, 2016-07-14 17:15:25]

https://codereview.chromium.org/2134463002/diff/80001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/preferences/website/SingleWebsitePreferences.java
(right):

https://codereview.chromium.org/2134463002/diff/80001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/preferences/website/SingleWebsitePreferences.java:656:
if (!hasPermissionsPreferences()) {
Should this just check the usb count rather than checking if there are any
permissions preferences?

https://codereview.chromium.org/2134463002/diff/80001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/preferences/website/Website.java
(right):

https://codereview.chromium.org/2134463002/diff/80001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/preferences/website/Website.java:46:
private final List<UsbInfo> mUsbInfo = new ArrayList<UsbInfo>();
findbugs complains about this:

SE_BAD_FIELD: Non-transient non-serializable instance field in serializable
class
  In class org.chromium.chrome.browser.preferences.website.Website
  Field org.chromium.chrome.browser.preferences.website.Website.mUsbInfo
  In Website.java


I think you need to change UsbInfo back to Serializable.

https://codereview.chromium.org/2134463002/diff/80001/chrome/android/java/str...
File chrome/android/java/strings/android_chrome_strings.grd (right):

https://codereview.chromium.org/2134463002/diff/80001/chrome/android/java/str...
chrome/android/java/strings/android_chrome_strings.grd:825: <message
name="IDS_WEBSITE_SETTINGS_REVOKE_DEVICE_PERMISSION" desc="Content description
for a device permission entry.">
nit: this is going to be used by translators to determine how to translate the
string so being really explicit is good. e.g. "Content description for revoking
a website's permission to access a device" or something like that." I know what
"device permission entry" means, but someone who's never looked at the screen or
code might not.

[Reilly Grant (use Gerrit), 2016-07-15 00:03:35]

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

[Reilly Grant (use Gerrit), 2016-07-15 00:03:44]

https://codereview.chromium.org/2134463002/diff/80001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/preferences/website/SingleWebsitePreferences.java
(right):

https://codereview.chromium.org/2134463002/diff/80001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/preferences/website/SingleWebsitePreferences.java:656:
if (!hasPermissionsPreferences()) {
On 2016/07/14 at 17:15:25, Theresa Wellington wrote:
> Should this just check the usb count rather than checking if there are any
permissions preferences?

We need to check hasPermissionsPreferences() because if there are no more
permissions this is going to remove the whole permission section.

https://codereview.chromium.org/2134463002/diff/80001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/preferences/website/Website.java
(right):

https://codereview.chromium.org/2134463002/diff/80001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/preferences/website/Website.java:46:
private final List<UsbInfo> mUsbInfo = new ArrayList<UsbInfo>();
On 2016/07/14 at 17:15:25, Theresa Wellington wrote:
> findbugs complains about this:
> 
> SE_BAD_FIELD: Non-transient non-serializable instance field in serializable
class
>   In class org.chromium.chrome.browser.preferences.website.Website
>   Field org.chromium.chrome.browser.preferences.website.Website.mUsbInfo
>   In Website.java
> 
> 
> I think you need to change UsbInfo back to Serializable.

Okay, I've switched it back.

https://codereview.chromium.org/2134463002/diff/80001/chrome/android/java/str...
File chrome/android/java/strings/android_chrome_strings.grd (right):

https://codereview.chromium.org/2134463002/diff/80001/chrome/android/java/str...
chrome/android/java/strings/android_chrome_strings.grd:825: <message
name="IDS_WEBSITE_SETTINGS_REVOKE_DEVICE_PERMISSION" desc="Content description
for a device permission entry.">
On 2016/07/14 at 17:15:25, Theresa Wellington wrote:
> nit: this is going to be used by translators to determine how to translate the
string so being really explicit is good. e.g. "Content description for revoking
a website's permission to access a device" or something like that." I know what
"device permission entry" means, but someone who's never looked at the screen or
code might not.

Done.

[commit-bot: I haz the power, 2016-07-15 00:03:55]

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

[Yusuf, 2016-07-15 00:21:42]

mostly styling nits.

https://codereview.chromium.org/2134463002/diff/100001/chrome/android/java/sr...
File
chrome/android/java/src/org/chromium/chrome/browser/preferences/website/SingleWebsitePreferences.java
(right):

https://codereview.chromium.org/2134463002/diff/100001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/preferences/website/SingleWebsitePreferences.java:267:
if (origin.equals(usbInfo.getOrigin())) {
one line

https://codereview.chromium.org/2134463002/diff/100001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/preferences/website/SingleWebsitePreferences.java:343:
if (Arrays.asList(PERMISSION_PREFERENCE_KEYS).contains(preference.getKey())) {
can we cache this list outside the loop so that we don't keep doing the
conversion?

https://codereview.chromium.org/2134463002/diff/100001/chrome/android/java/sr...
File
chrome/android/java/src/org/chromium/chrome/browser/preferences/website/UsbInfo.java
(right):

https://codereview.chromium.org/2134463002/diff/100001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/preferences/website/UsbInfo.java:18:
UsbInfo(String origin, String embedder, String name, String object) {
javadocs on this file. We usually put even the dummy getter docs. Feel free to
cut them short.

revoke needs explanation for sure.

https://codereview.chromium.org/2134463002/diff/100001/chrome/android/java/sr...
File
chrome/android/java/src/org/chromium/chrome/browser/preferences/website/Website.java
(right):

https://codereview.chromium.org/2134463002/diff/100001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/preferences/website/Website.java:507:
public void addUsbInfo(UsbInfo info) {
javadocs

[commit-bot: I haz the power, 2016-07-15 01:09:09]

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

[commit-bot: I haz the power, 2016-07-15 01:09:10]

Dry run: This issue passed the CQ dry run.

[Theresa, 2016-07-15 16:37:48]

https://codereview.chromium.org/2134463002/diff/80001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/preferences/website/SingleWebsitePreferences.java
(right):

https://codereview.chromium.org/2134463002/diff/80001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/preferences/website/SingleWebsitePreferences.java:656:
if (!hasPermissionsPreferences()) {
On 2016/07/15 00:03:44, Reilly Grant wrote:
> On 2016/07/14 at 17:15:25, Theresa Wellington wrote:
> > Should this just check the usb count rather than checking if there are any
> permissions preferences?
> 
> We need to check hasPermissionsPreferences() because if there are no more
> permissions this is going to remove the whole permission section.

Ah, right, sorry, I thought this was removing the USB header but looking at the
screenshot again, it looks like there isn't a USB header.

[Reilly Grant (use Gerrit), 2016-07-15 21:13:23]

https://codereview.chromium.org/2134463002/diff/100001/chrome/android/java/sr...
File
chrome/android/java/src/org/chromium/chrome/browser/preferences/website/SingleWebsitePreferences.java
(right):

https://codereview.chromium.org/2134463002/diff/100001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/preferences/website/SingleWebsitePreferences.java:267:
if (origin.equals(usbInfo.getOrigin())) {
On 2016/07/15 at 00:21:42, Yusuf wrote:
> one line

Done.

https://codereview.chromium.org/2134463002/diff/100001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/preferences/website/SingleWebsitePreferences.java:343:
if (Arrays.asList(PERMISSION_PREFERENCE_KEYS).contains(preference.getKey())) {
On 2016/07/15 at 00:21:42, Yusuf wrote:
> can we cache this list outside the loop so that we don't keep doing the
conversion?

Done, I'll convert it to a Set while I'm at it.

https://codereview.chromium.org/2134463002/diff/100001/chrome/android/java/sr...
File
chrome/android/java/src/org/chromium/chrome/browser/preferences/website/UsbInfo.java
(right):

https://codereview.chromium.org/2134463002/diff/100001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/preferences/website/UsbInfo.java:18:
UsbInfo(String origin, String embedder, String name, String object) {
On 2016/07/15 at 00:21:42, Yusuf wrote:
> javadocs on this file. We usually put even the dummy getter docs. Feel free to
cut them short.
> 
> revoke needs explanation for sure.

Done.

https://codereview.chromium.org/2134463002/diff/100001/chrome/android/java/sr...
File
chrome/android/java/src/org/chromium/chrome/browser/preferences/website/Website.java
(right):

https://codereview.chromium.org/2134463002/diff/100001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/preferences/website/Website.java:507:
public void addUsbInfo(UsbInfo info) {
On 2016/07/15 at 00:21:42, Yusuf wrote:
> javadocs

Done.

[Reilly Grant (use Gerrit), 2016-07-15 21:13:27]

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

[commit-bot: I haz the power, 2016-07-15 21:13:50]

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

[Theresa, 2016-07-15 21:19:45]

lgtm

[commit-bot: I haz the power, 2016-07-15 22:33:18]

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

[commit-bot: I haz the power, 2016-07-15 22:33:20]

Dry run: This issue passed the CQ dry run.

[Reilly Grant (use Gerrit), 2016-07-15 22:46:09]

The CQ bit was checked by reillyg@chromium.org

[Reilly Grant (use Gerrit), 2016-07-15 22:46:10]

The patchset sent to the CQ was uploaded after l-g-t-m from bauerb@chromium.org
Link to the patchset: https://codereview.chromium.org/2134463002/#ps120001
(title: "Addressed yusufo@'s comments.")

[commit-bot: I haz the power, 2016-07-15 22:46:47]

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-07-15 22:51:54]

Committed patchset #6 (id:120001)

[commit-bot: I haz the power, 2016-07-15 22:52:09]

CQ bit was unchecked.

[commit-bot: I haz the power, 2016-07-15 22:54:08]

Description was changed from

==========
Add USB device permissions to individual site settings on Android.

This patch makes it possible to view and revoke permission to access a
USB device by going to an origin's "site settings" page.

BUG=430999,601627
==========

to

==========
Add USB device permissions to individual site settings on Android.

This patch makes it possible to view and revoke permission to access a
USB device by going to an origin's "site settings" page.

BUG=430999,601627

Committed: https://crrev.com/7280deea31d7794becbc7f86fb0870a129c54439
Cr-Commit-Position: refs/heads/master@{#405887}
==========

[commit-bot: I haz the power, 2016-07-15 22:54:09]

Patchset 6 (id:??) landed as
https://crrev.com/7280deea31d7794becbc7f86fb0870a129c54439
Cr-Commit-Position: refs/heads/master@{#405887}