Make USB permissions work in the new permission message system

Make USB permissions work in the new permission message system

Skipping presubmit because of renamed histogram entry. Also TBRing the histograms.xml change.
NOPRESUBMIT=true
TBR=asvitkine@chromium.org

BUG=522842
Committed: https://crrev.com/7a1bde6edae7b421b1115cb6c792189d2d846963
Cr-Commit-Position: refs/heads/master@{#344729}

[Marc Treib, 2015-08-20 14:02:37]

treib@chromium.org changed reviewers:
+ rdevlin.cronin@chromium.org

[Marc Treib, 2015-08-20 14:02:37]

Here, have some USB permissions cleanup! :)

[Devlin, 2015-08-20 16:33:07]

Oh, so much nicer! :)

LGTM with nitty nits.

https://codereview.chromium.org/1300353002/diff/1/chrome/common/extensions/pe...
File chrome/common/extensions/permissions/chrome_permission_message_rules.cc
(right):

https://codereview.chromium.org/1300353002/diff/1/chrome/common/extensions/pe...
chrome/common/extensions/permissions/chrome_permission_message_rules.cc:191: if
(permissions.size() == 1) {
nit: I think these would be more readable in helper functions:
return permissions.size() == 1 ?
    GetItemMessage(*permissions.begin()) :
    GetMultiItemMessage(permissions);

https://codereview.chromium.org/1300353002/diff/1/extensions/common/permissio...
File extensions/common/permissions/usb_device_permission.cc (right):

https://codereview.chromium.org/1300353002/diff/1/extensions/common/permissio...
extensions/common/permissions/usb_device_permission.cc:52: for (const uint16&
vendor_id : unknown_product_vendors) {
hmm... a const& to a uint16 almost sounds _more_ expensive than a copy. ;)

https://codereview.chromium.org/1300353002/diff/1/extensions/common/permissio...
extensions/common/permissions/usb_device_permission.cc:53: const char* vendor =
device::UsbIds::GetVendorName(vendor_id);
Actually, looks like all of this could just go in the else above, and get rid of
unknown_product_vendors, right?

[Marc Treib, 2015-08-21 08:00:49]

https://codereview.chromium.org/1300353002/diff/1/chrome/common/extensions/pe...
File chrome/common/extensions/permissions/chrome_permission_message_rules.cc
(right):

https://codereview.chromium.org/1300353002/diff/1/chrome/common/extensions/pe...
chrome/common/extensions/permissions/chrome_permission_message_rules.cc:191: if
(permissions.size() == 1) {
On 2015/08/20 16:33:07, Devlin wrote:
> nit: I think these would be more readable in helper functions:
> return permissions.size() == 1 ?
>     GetItemMessage(*permissions.begin()) :
>     GetMultiItemMessage(permissions);

Done, mostly - I still have to pass the PermissionIDSet to GetItemMessage, since
I need a Set for CoalescedPermissionMessage.

https://codereview.chromium.org/1300353002/diff/1/extensions/common/permissio...
File extensions/common/permissions/usb_device_permission.cc (right):

https://codereview.chromium.org/1300353002/diff/1/extensions/common/permissio...
extensions/common/permissions/usb_device_permission.cc:52: for (const uint16&
vendor_id : unknown_product_vendors) {
On 2015/08/20 16:33:07, Devlin wrote:
> hmm... a const& to a uint16 almost sounds _more_ expensive than a copy. ;)

Indeed. That's a leftover from a previous version where I was storing strings...
Done.

https://codereview.chromium.org/1300353002/diff/1/extensions/common/permissio...
extensions/common/permissions/usb_device_permission.cc:53: const char* vendor =
device::UsbIds::GetVendorName(vendor_id);
On 2015/08/20 16:33:07, Devlin wrote:
> Actually, looks like all of this could just go in the else above, and get rid
of
> unknown_product_vendors, right?

No. The intermediate set is for deduping, since we could have multiple unknown
devices with the same vendor, and we only want it to show up once.

[Marc Treib, 2015-08-21 08:01:01]

The CQ bit was checked by treib@chromium.org

[Marc Treib, 2015-08-21 08:01:01]

The patchset sent to the CQ was uploaded after l-g-t-m from
rdevlin.cronin@chromium.org
Link to the patchset: https://codereview.chromium.org/1300353002/#ps20001
(title: "review")

[commit-bot: I haz the power, 2015-08-21 08:01:25]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1300353002/20001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1300353002/20001

[commit-bot: I haz the power, 2015-08-21 09:53:53]

Committed patchset #2 (id:20001)

[commit-bot: I haz the power, 2015-08-21 09:54:42]

Patchset 2 (id:??) landed as
https://crrev.com/7a1bde6edae7b421b1115cb6c792189d2d846963
Cr-Commit-Position: refs/heads/master@{#344729}

[Alexei Svitkine (slow), 2015-08-24 15:19:53]

asvitkine@chromium.org changed reviewers:
+ asvitkine@chromium.org

[Alexei Svitkine (slow), 2015-08-24 15:19:54]

lgtm

Though, please avoid tbr-ing histograms changes in the future, as there's been
some problematic changes introduced like that in the past. There's a number of
owners, so if one is away, it should be easy to substitute them for someone
else.

[Marc Treib, 2015-08-24 15:25:19]

On 2015/08/24 15:19:54, Alexei Svitkine (OOO Aug15-24) wrote:
> lgtm
> 
> Though, please avoid tbr-ing histograms changes in the future, as there's been
> some problematic changes introduced like that in the past. There's a number of
> owners, so if one is away, it should be easy to substitute them for someone
> else.

Acknowledged.
This wasn't about finding an owner, I just thought it was too simple/mechanical
to bother any of you. I'll add a proper reviewer next time.