Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
336313009:
Showing devices transport method in one device (Closed)
DescriptionIf an app/extension requests permission to access more than one type of devices (e.g. USB, Bluetooth and Serial) we should show one message listing the transport method(s).
Currently we show a an install-time warning for each type of device the extension/app requests permission for. This is very excessive and this CL solves this problem by showing a single message that include the transport method.
Currently we have three transport methods: Bluetooth, Serial and USB. We could show the following messages depending on the requested permissions:
"Access your hardware devices connected via USB and Bluetooth" (how is this different
than kHid?)
"Access your hardware devices connected via USB and Serial"
"Access your hardware devices connected via Bluetooth and Serial"
and also keep the individual messages if only one category of hardware is
requested.
BUG=384353
R=jww, meacer, kalman
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=281665
Patch Set 1 #
Total comments: 4
Patch Set 2 : Collabsed the string but included the transport #
Total comments: 3
Patch Set 3 : Cleaned the code and added tests #
Total comments: 26
Patch Set 4 : Removed HID from the list and ensured consistency with other strings. #
Total comments: 8
Patch Set 5 : Cleaned some code and added extra tests. #
Total comments: 7
Patch Set 6 : Modifying HID string and removing parantheses from strings. #
Total comments: 10
Patch Set 7 : Alphabetically ordered some of the code. Coealiscd one more case." #
Total comments: 6
Patch Set 8 : Fixing the style of using overriding instead of options and adding const when needed #
Messages
Total messages: 30 (0 generated)
jww@ and meacer@ can you please have a look at this for me.
https://codereview.chromium.org/336313009/diff/1/chrome/common/extensions/per... File chrome/common/extensions/permissions/chrome_permission_message_provider.cc (right): https://codereview.chromium.org/336313009/diff/1/chrome/common/extensions/per... chrome/common/extensions/permissions/chrome_permission_message_provider.cc:137: case PermissionMessage::kUsbDevice: These should be kUsb and kBluetooth instead, as those are the permissions allowing device enumeration. As a quick test, you can create a simple manifest and check the permissions in chrome://extensions page: { "name": "Test," "manifest_version": 2, "version": "1.0", "permissions": ["usb", "bluetooth"] }
> These should be kUsb and kBluetooth instead, as those are the permissions
> allowing device enumeration. As a quick test, you can create a simple manifest
> and check the permissions in chrome://extensions page:
Isn't it the case that these two don't give access to devices but only to the
list of devices? This was what I understood from the discussion on the combining
the USB warnings bug.
> { "name": "Test,"
> "manifest_version": 2,
> "version": "1.0",
> "permissions": ["usb", "bluetooth"]
> }
whether this should be placed exactly to test?
On 2014/06/18 17:42:32, mhm wrote:
> > These should be kUsb and kBluetooth instead, as those are the permissions
> > allowing device enumeration. As a quick test, you can create a simple
manifest
> > and check the permissions in chrome://extensions page:
>
> Isn't it the case that these two don't give access to devices but only to the
> list of devices? This was what I understood from the discussion on the
combining
> the USB warnings bug.
>
>
> > { "name": "Test,"
> > "manifest_version": 2,
> > "version": "1.0",
> > "permissions": ["usb", "bluetooth"]
> > }
>
> whether this should be placed exactly to test?
Ah, I guess I misunderstood the purpose of this CL then. Looks like you want to
collapse the actual device access warnings into a generic string, and not only
the capability to list them.
That's actually a rather big change, as it was the security team who insisted on
listing each device separately. So I think this needs some more discussion.
One idea to get rid of install time warnings is to use the chooser model
(https://crbug.com/352720) The discussion there may provide more context.
> whether this should be placed exactly to test?
You can save it as manifest.json, put it in a directory and load that directory
as an extension in chrome://extensions page (you need to check "Developer
mode").
> Ah, I guess I misunderstood the purpose of this CL then. Looks like you want to > collapse the actual device access warnings into a generic string, and not only > the capability to list them. > > That's actually a rather big change, as it was the security team who insisted on > listing each device separately. So I think this needs some more discussion. SO the main idea (as discussed in the bug itself) that users don't care much about the transport method to access their devices. In this CL, if the extension asks for more than one type of device access we will show the general message "Access your hardware devices". However, if one type of device access is requested the detailed message will be shown. > One idea to get rid of install time warnings is to use the chooser model > (https://crbug.com/352720) The discussion there may provide more context. Thanks for referring me to this bug. I agree that the chooser model is better from security perspective. However, if the extension is asking to access all type of users' devices how many chooser message we will show to the user; one for each transport method?! I guess this is a discussion for another bug.
double checking hardware permission semantics with rockot https://codereview.chromium.org/336313009/diff/1/chrome/common/extensions/per... File chrome/common/extensions/permissions/chrome_permission_message_provider.cc (right): https://codereview.chromium.org/336313009/diff/1/chrome/common/extensions/per... chrome/common/extensions/permissions/chrome_permission_message_provider.cc:158: } I think the upgrade path here is a bit weird. What if I install an extensoin and it says "access to your bluetooth devices" and then it upgrades and says "access to your hardware". I'm ok with just grouping these no matter what ("access to hardware"). Perhaps ideally we'd turn usb + bluetooth into "your usb and bluetooth devices" but that would require setting up a full ManifestPermission for this: https://code.google.com/p/chromium/codesearch#chromium/src/extensions/common/...
> I think the upgrade path here is a bit weird. What if I install an extensoin and > it says "access to your bluetooth devices" and then it upgrades and says "access > to your hardware". Currently we have three transport methods: Bluetooth, Serial and USB. We could so the following messages depending on the permissions the extension requests: "Access your hardware connected via USB and Bluetooth" (how is this different than kHid?) "Access your hardware connected via USB and Serial" "Access your hardware connected via Bluetooth and Serial" and also keep the individual messages if only one category of hardware is requested.
https://codereview.chromium.org/336313009/diff/1/chrome/common/extensions/per... File chrome/common/extensions/permissions/chrome_permission_message_provider.cc (right): https://codereview.chromium.org/336313009/diff/1/chrome/common/extensions/per... chrome/common/extensions/permissions/chrome_permission_message_provider.cc:137: case PermissionMessage::kUsbDevice: On 2014/06/18 02:25:02, Mustafa Emre Acer wrote: > These should be kUsb and kBluetooth instead, as those are the permissions kUsb and kBluetooth are correct, yes. The k[Usb,Bluetooth]Device(s) permissions are for listing more specific device constraints (like VID/PID for a USB device). They're a practical prerequisite for using the APIs, but they are not interesting permissions in this case, I think.
https://codereview.chromium.org/336313009/diff/1/chrome/common/extensions/per... File chrome/common/extensions/permissions/chrome_permission_message_provider.cc (right): https://codereview.chromium.org/336313009/diff/1/chrome/common/extensions/per... chrome/common/extensions/permissions/chrome_permission_message_provider.cc:137: case PermissionMessage::kUsbDevice: > kUsb and kBluetooth are correct, yes. rockot@ aren't these two only used to list the Bluetooth/USb device and not to access the devices themselves?
On 2014/06/18 21:05:03, mhm wrote: > > I think the upgrade path here is a bit weird. What if I install an extensoin > and > > it says "access to your bluetooth devices" and then it upgrades and says > "access > > to your hardware". > > Currently we have three transport methods: Bluetooth, Serial and USB. We could > so the following messages depending on the permissions the extension requests: > > "Access your hardware connected via USB and Bluetooth" (how is this different > than kHid?) > "Access your hardware connected via USB and Serial" > "Access your hardware connected via Bluetooth and Serial" > > and also keep the individual messages if only one category of hardware is > requested. This is an exponential number of warning messages of course. Those poor translators, but I guess what else are you going to do, I'm not sure if the translations support lists of things. so ok.
kalman@ can you please have a look at this for me. I have collapsed the string and added the transport used to access the device. When we get this one merged along with the "accessibility" CL, I will rewrite the code to put the different messages in a set.
accidentally reviewed this CL. oh wel. https://codereview.chromium.org/336313009/diff/20001/chrome/app/generated_res... File chrome/app/generated_resources.grd (right): https://codereview.chromium.org/336313009/diff/20001/chrome/app/generated_res... chrome/app/generated_resources.grd:4339: + Access your hardware devices connected via USB and Bluetooth why not just "Access your USB and Bluetooth devices"? etc. https://codereview.chromium.org/336313009/diff/20001/chrome/app/generated_res... chrome/app/generated_resources.grd:4347: + <message name="IDS_EXTENSION_PROMPT_WARNING_DEVICES" desc="Permission string for access to Bluetooth, Serial and USB connected devices."> desc isn't really accurate here, or at least, it's more of an implementation detail. better: "Permission string for access to all the user's hardware devices".
oh, I was supposed to review this. https://codereview.chromium.org/336313009/diff/20001/chrome/common/extensions... File chrome/common/extensions/permissions/chrome_permission_message_provider.cc (right): https://codereview.chromium.org/336313009/diff/20001/chrome/common/extensions... chrome/common/extensions/permissions/chrome_permission_message_provider.cc:177: l10n_util::GetStringUTF16(IDS_EXTENSION_PROMPT_WARNING_HID)); apparently I was supposed to review this actually. anyway, the difference between HID and USB is so subtle. I wonder if it's better to just say USB either way? (not really a related change, more a pre-change)
I just realised something. This stuff needs to be tested.
meacer@ can you please have a look at this for me. The code is now cleaned (had to revert to in-place push of messages to preserve the order) and added tests.
https://codereview.chromium.org/336313009/diff/40001/chrome/app/generated_res... File chrome/app/generated_resources.grd (right): https://codereview.chromium.org/336313009/diff/40001/chrome/app/generated_res... chrome/app/generated_resources.grd:4366: + Access your USB and Bluetooth device(s) HID devices are input devices such as keyboard, mouse etc. They are still connected through USB and bluetooth, but they are only a subset of USB and bluetooth whereas this message implies otherwise. The API in fact blacklists keyboard and mouse but it allows other input devices. I'm kind of hesitant to make this change here: I feel like we should let the user know that their input devices can be controlled by an app. https://codereview.chromium.org/336313009/diff/40001/chrome/app/generated_res... chrome/app/generated_resources.grd:4374: + <message name="IDS_EXTENSION_PROMPT_WARNING_DEVICES" desc="Permission string for access all user's connected devices."> Maybe "IDS_EXTENSION_PROMPT_WARNING_ALL_DEVICES"? https://codereview.chromium.org/336313009/diff/40001/chrome/common/extensions... File chrome/common/extensions/permissions/chrome_permission_message_provider.cc (right): https://codereview.chromium.org/336313009/diff/40001/chrome/common/extensions... chrome/common/extensions/permissions/chrome_permission_message_provider.cc:48: bool CombineMessage(T& messages, int first_message, int second_message) { This needs a better name. Maybe ContainsMessages? https://codereview.chromium.org/336313009/diff/40001/chrome/common/extensions... chrome/common/extensions/permissions/chrome_permission_message_provider.cc:51: if (first != messages.end() && second != messages.end()) { Cleaner: return first != messages.end() && second != messages.end(); https://codereview.chromium.org/336313009/diff/40001/chrome/common/extensions... chrome/common/extensions/permissions/chrome_permission_message_provider.cc:138: messages, PermissionMessage::kUsb, PermissionMessage::kBluetooth)) { As I mentioned on the previous comment on IDS_EXTENSION_PROMPT_WARNING_HID, both usb and bluetooth can also be non-hid devices. Actually, hid is the odd case here because it doesn't have it's own hidDevices list; it uses either usbDevices or bluetoothDevices so it's hard to reason what the message should be. Maybe you could ignore "hid" for the time being and combine usb, serial and bluetooth? https://codereview.chromium.org/336313009/diff/40001/chrome/common/extensions... chrome/common/extensions/permissions/chrome_permission_message_provider.cc:146: bool media_galleries_read_copy_to = I would just put these inside the if condition where they are used: if (id == ..GalleriesRead || id == ..GalleriesCopyTo || id == ..GalleriesDelete) { ... if (CombineMessage(messages, ..Read, ..Delete)) { ... } } https://codereview.chromium.org/336313009/diff/40001/chrome/common/extensions... chrome/common/extensions/permissions/chrome_permission_message_provider.cc:150: bool media_galleries_read_delete = Same as media_galleries_copy_to https://codereview.chromium.org/336313009/diff/40001/chrome/common/extensions... chrome/common/extensions/permissions/chrome_permission_message_provider.cc:169: if (id == PermissionMessage::kUsb) { This seems cleaner: if (id == kUsb && ContainsMessages(messages, kUsb, kSerial)) { message_strings.push_back( l10n_util::GetStringUTF16( IDS_EXTENSION_PROMPT_WARNING_USB_SERIAL)); continue; } https://codereview.chromium.org/336313009/diff/40001/chrome/common/extensions... chrome/common/extensions/permissions/chrome_permission_message_provider.cc:180: if (id == PermissionMessage::kBluetooth) { Same as above, you could just check for kBluetooth: if (id == kBluetooth && ContainsMessages(messages, kBluetooth, kSerial)) { ... } https://codereview.chromium.org/336313009/diff/40001/chrome/common/extensions... chrome/common/extensions/permissions/chrome_permission_message_provider.cc:193: IDS_EXTENSION_PROMPT_WARNING_ACCESSIBILITY_FEATURES_READ_MODIFY)); Same: if (id == Read && CombineMessage(messages, Read, Modify)) { ... } https://codereview.chromium.org/336313009/diff/40001/chrome/common/extensions... chrome/common/extensions/permissions/chrome_permission_message_provider.cc:202: if (id == PermissionMessage::kAudioCapture) { Ditto. https://codereview.chromium.org/336313009/diff/40001/chrome/common/extensions... chrome/common/extensions/permissions/chrome_permission_message_provider.cc:212: if (id == PermissionMessage::kMediaGalleriesAllGalleriesRead) { Looks like this is always false, right? id can only be one of (Read, CopyTo, Delete) in the above if statement. https://codereview.chromium.org/336313009/diff/40001/chrome/common/extensions... File chrome/common/extensions/permissions/permission_set_unittest.cc (right): https://codereview.chromium.org/336313009/diff/40001/chrome/common/extensions... chrome/common/extensions/permissions/permission_set_unittest.cc:917: api_permissions.insert(APIPermission::kSerial); As I mentioned earlier, I think you could leave out hid for now. I'm not sure how we should handle it.
https://codereview.chromium.org/336313009/diff/40001/chrome/common/extensions... File chrome/common/extensions/permissions/chrome_permission_message_provider.cc (right): https://codereview.chromium.org/336313009/diff/40001/chrome/common/extensions... chrome/common/extensions/permissions/chrome_permission_message_provider.cc:212: if (id == PermissionMessage::kMediaGalleriesAllGalleriesRead) { On 2014/07/03 01:04:11, Mustafa Emre Acer wrote: > Looks like this is always false, right? > > id can only be one of (Read, CopyTo, Delete) in the above if statement. Never mind, my comment is wrong.
meacer@ can you please have a look at this for me. https://codereview.chromium.org/336313009/diff/40001/chrome/app/generated_res... File chrome/app/generated_resources.grd (right): https://codereview.chromium.org/336313009/diff/40001/chrome/app/generated_res... chrome/app/generated_resources.grd:4366: + Access your USB and Bluetooth device(s) On 2014/07/03 01:04:11, Mustafa Emre Acer wrote: > HID devices are input devices such as keyboard, mouse etc. They are still > connected through USB and bluetooth, but they are only a subset of USB and > bluetooth whereas this message implies otherwise. The API in fact blacklists > keyboard and mouse but it allows other input devices. > > I'm kind of hesitant to make this change here: I feel like we should let the > user know that their input devices can be controlled by an app. Done. https://codereview.chromium.org/336313009/diff/40001/chrome/app/generated_res... chrome/app/generated_resources.grd:4374: + <message name="IDS_EXTENSION_PROMPT_WARNING_DEVICES" desc="Permission string for access all user's connected devices."> On 2014/07/03 01:04:11, Mustafa Emre Acer wrote: > Maybe "IDS_EXTENSION_PROMPT_WARNING_ALL_DEVICES"? Done. https://codereview.chromium.org/336313009/diff/40001/chrome/common/extensions... File chrome/common/extensions/permissions/chrome_permission_message_provider.cc (right): https://codereview.chromium.org/336313009/diff/40001/chrome/common/extensions... chrome/common/extensions/permissions/chrome_permission_message_provider.cc:48: bool CombineMessage(T& messages, int first_message, int second_message) { On 2014/07/03 01:04:11, Mustafa Emre Acer wrote: > This needs a better name. Maybe ContainsMessages? Done. https://codereview.chromium.org/336313009/diff/40001/chrome/common/extensions... chrome/common/extensions/permissions/chrome_permission_message_provider.cc:51: if (first != messages.end() && second != messages.end()) { On 2014/07/03 01:04:11, Mustafa Emre Acer wrote: > Cleaner: > > return first != messages.end() && second != messages.end(); Done. https://codereview.chromium.org/336313009/diff/40001/chrome/common/extensions... chrome/common/extensions/permissions/chrome_permission_message_provider.cc:138: messages, PermissionMessage::kUsb, PermissionMessage::kBluetooth)) { On 2014/07/03 01:04:11, Mustafa Emre Acer wrote: > As I mentioned on the previous comment on IDS_EXTENSION_PROMPT_WARNING_HID, both > usb and bluetooth can also be non-hid devices. > > Actually, hid is the odd case here because it doesn't have it's own hidDevices > list; it uses either usbDevices or bluetoothDevices so it's hard to reason what > the message should be. > > Maybe you could ignore "hid" for the time being and combine usb, serial and > bluetooth? Done. https://codereview.chromium.org/336313009/diff/40001/chrome/common/extensions... chrome/common/extensions/permissions/chrome_permission_message_provider.cc:146: bool media_galleries_read_copy_to = On 2014/07/03 01:04:11, Mustafa Emre Acer wrote: > I would just put these inside the if condition where they are used: > > if (id == ..GalleriesRead || > id == ..GalleriesCopyTo || > id == ..GalleriesDelete) { > ... > if (CombineMessage(messages, ..Read, ..Delete)) { > ... > } > } Done. https://codereview.chromium.org/336313009/diff/40001/chrome/common/extensions... chrome/common/extensions/permissions/chrome_permission_message_provider.cc:150: bool media_galleries_read_delete = On 2014/07/03 01:04:11, Mustafa Emre Acer wrote: > Same as media_galleries_copy_to Done. https://codereview.chromium.org/336313009/diff/40001/chrome/common/extensions... chrome/common/extensions/permissions/chrome_permission_message_provider.cc:169: if (id == PermissionMessage::kUsb) { The issue is that we also need to have a continue in the case of: if (id == Serial && ContainMessages(messages, kUsb, kSerial) { continue; } https://codereview.chromium.org/336313009/diff/40001/chrome/common/extensions... chrome/common/extensions/permissions/chrome_permission_message_provider.cc:180: if (id == PermissionMessage::kBluetooth) { On 2014/07/03 01:04:11, Mustafa Emre Acer wrote: > Same as above, you could just check for kBluetooth: > > if (id == kBluetooth && ContainsMessages(messages, kBluetooth, kSerial)) { > ... > } Done. https://codereview.chromium.org/336313009/diff/40001/chrome/common/extensions... chrome/common/extensions/permissions/chrome_permission_message_provider.cc:193: IDS_EXTENSION_PROMPT_WARNING_ACCESSIBILITY_FEATURES_READ_MODIFY)); On 2014/07/03 01:04:11, Mustafa Emre Acer wrote: > Same: > > if (id == Read && CombineMessage(messages, Read, Modify)) { > ... > } Done. https://codereview.chromium.org/336313009/diff/40001/chrome/common/extensions... chrome/common/extensions/permissions/chrome_permission_message_provider.cc:202: if (id == PermissionMessage::kAudioCapture) { On 2014/07/03 01:04:11, Mustafa Emre Acer wrote: > Ditto. Done. https://codereview.chromium.org/336313009/diff/40001/chrome/common/extensions... chrome/common/extensions/permissions/chrome_permission_message_provider.cc:212: if (id == PermissionMessage::kMediaGalleriesAllGalleriesRead) { On 2014/07/03 01:09:59, Mustafa Emre Acer wrote: > On 2014/07/03 01:04:11, Mustafa Emre Acer wrote: > > Looks like this is always false, right? > > > > id can only be one of (Read, CopyTo, Delete) in the above if statement. > > Never mind, my comment is wrong. Done.
https://codereview.chromium.org/336313009/diff/60001/chrome/common/extensions... File chrome/common/extensions/permissions/chrome_permission_message_provider.cc (right): https://codereview.chromium.org/336313009/diff/60001/chrome/common/extensions... chrome/common/extensions/permissions/chrome_permission_message_provider.cc:48: bool ContainBothMessages(T& messages, int first_message, int second_message) { Does this actually need to be templatized? Looks like T& messages could just be const PermissionMessages& messages? https://codereview.chromium.org/336313009/diff/60001/chrome/common/extensions... chrome/common/extensions/permissions/chrome_permission_message_provider.cc:48: bool ContainBothMessages(T& messages, int first_message, int second_message) { Also, ContainBothMessages -> ContainsBothMessages https://codereview.chromium.org/336313009/diff/60001/chrome/common/extensions... chrome/common/extensions/permissions/chrome_permission_message_provider.cc:134: messages, PermissionMessage::kUsb, PermissionMessage::kSerial)) { nit: I think it'll look better if you format this like the previous statement: ContainsBothMessages(messages, PermissionMessage::kUsb, PermissionMessage::kSerial)) { https://codereview.chromium.org/336313009/diff/60001/chrome/common/extensions... chrome/common/extensions/permissions/chrome_permission_message_provider.cc:145: if (id == PermissionMessage::kUsb) { You might want to test this logic with multiple "usb" permissions too.
Thanks for your earlier comments meacer@ Can you please have a look at this for me. https://codereview.chromium.org/336313009/diff/60001/chrome/common/extensions... File chrome/common/extensions/permissions/chrome_permission_message_provider.cc (right): https://codereview.chromium.org/336313009/diff/60001/chrome/common/extensions... chrome/common/extensions/permissions/chrome_permission_message_provider.cc:48: bool ContainBothMessages(T& messages, int first_message, int second_message) { On 2014/07/07 21:57:32, Mustafa Emre Acer wrote: > Also, ContainBothMessages -> ContainsBothMessages Done. https://codereview.chromium.org/336313009/diff/60001/chrome/common/extensions... chrome/common/extensions/permissions/chrome_permission_message_provider.cc:48: bool ContainBothMessages(T& messages, int first_message, int second_message) { No it doesn't. I just followed the SuppressMessage signature. https://codereview.chromium.org/336313009/diff/60001/chrome/common/extensions... chrome/common/extensions/permissions/chrome_permission_message_provider.cc:134: messages, PermissionMessage::kUsb, PermissionMessage::kSerial)) { This was done with auto formatting "git cl format". Sometime it messes things up :-) https://codereview.chromium.org/336313009/diff/60001/chrome/common/extensions... chrome/common/extensions/permissions/chrome_permission_message_provider.cc:145: if (id == PermissionMessage::kUsb) { On 2014/07/07 21:57:31, Mustafa Emre Acer wrote: > You might want to test this logic with multiple "usb" permissions too. Done.
Lgtm with questions and nits. https://codereview.chromium.org/336313009/diff/80001/chrome/app/generated_res... File chrome/app/generated_resources.grd (right): https://codereview.chromium.org/336313009/diff/80001/chrome/app/generated_res... chrome/app/generated_resources.grd:4366: + Access your input devices such as keyboard and mouse HID api blocks access to keyboard and mouse and only allows non-standard human interface devices, so this string isn't correct. You can probably say "access your input devices". https://codereview.chromium.org/336313009/diff/80001/chrome/app/generated_res... chrome/app/generated_resources.grd:4369: + Access your USB and Bluetooth device(s) Nit: Might want to check the parenthesis with extension folks. https://codereview.chromium.org/336313009/diff/80001/extensions/extensions_st... File extensions/extensions_strings.grd (right): https://codereview.chromium.org/336313009/diff/80001/extensions/extensions_st... extensions/extensions_strings.grd:318: Access your USB device(s) Out of curiosity: Why is this string here and the others are in generated_resources.grd? Is it possible to move them here too?
Hey kalman@ can you please have a look at this for me. https://codereview.chromium.org/336313009/diff/80001/chrome/app/generated_res... File chrome/app/generated_resources.grd (right): https://codereview.chromium.org/336313009/diff/80001/chrome/app/generated_res... chrome/app/generated_resources.grd:4366: + Access your input devices such as keyboard and mouse On 2014/07/07 22:38:26, Mustafa Emre Acer wrote: > HID api blocks access to keyboard and mouse and only allows non-standard human > interface devices, so this string isn't correct. You can probably say "access > your input devices". Done. https://codereview.chromium.org/336313009/diff/80001/chrome/app/generated_res... chrome/app/generated_resources.grd:4369: + Access your USB and Bluetooth device(s) On 2014/07/07 22:38:26, Mustafa Emre Acer wrote: > Nit: Might want to check the parenthesis with extension folks. Done. https://codereview.chromium.org/336313009/diff/80001/extensions/extensions_st... File extensions/extensions_strings.grd (right): https://codereview.chromium.org/336313009/diff/80001/extensions/extensions_st... extensions/extensions_strings.grd:318: Access your USB device(s) I am not really sure. I totally agree with you that doesn't make a lot of sense. However, I think this can be done in another CL after knowing why.
https://codereview.chromium.org/336313009/diff/100001/chrome/common/extension... File chrome/common/extensions/permissions/chrome_permission_message_provider.cc (right): https://codereview.chromium.org/336313009/diff/100001/chrome/common/extension... chrome/common/extensions/permissions/chrome_permission_message_provider.cc:130: // Access to USB, Bluetooth and Serial these little comments don't add much, it's clear from the check what you're checking. however, a more general comment here might be helpful. https://codereview.chromium.org/336313009/diff/100001/chrome/common/extension... chrome/common/extensions/permissions/chrome_permission_message_provider.cc:134: ContainsBothMessages(messages, could you have a method like "ContainsAll" and overload that with a 2 and 3 version? because Contains(a, b) && Contains(a, c) is equivalent to Contains(a, b, c), right? and the latter looks much nicer. https://codereview.chromium.org/336313009/diff/100001/chrome/common/extension... chrome/common/extensions/permissions/chrome_permission_message_provider.cc:137: if (id == PermissionMessage::kUsb) { epic nit: can we have these in alphabetic ordering (a) in those Contains calls and (b) when choosing an arbitrary message ID. https://codereview.chromium.org/336313009/diff/100001/chrome/common/extension... chrome/common/extensions/permissions/chrome_permission_message_provider.cc:200: id == PermissionMessage::kMediaGalleriesAllGalleriesDelete) { what about read+copy+delete? https://codereview.chromium.org/336313009/diff/100001/chrome/common/extension... chrome/common/extensions/permissions/chrome_permission_message_provider.cc:210: } else if (ContainsBothMessages( no else after continue
https://codereview.chromium.org/336313009/diff/80001/chrome/app/generated_res... File chrome/app/generated_resources.grd (right): https://codereview.chromium.org/336313009/diff/80001/chrome/app/generated_res... chrome/app/generated_resources.grd:4369: + Access your USB and Bluetooth device(s) On 2014/07/07 22:50:04, mhm wrote: > On 2014/07/07 22:38:26, Mustafa Emre Acer wrote: > > Nit: Might want to check the parenthesis with extension folks. > > Done. just say "devices". "device(s)" is incorrect.
kalman@ thanks for your earlier comments. I think I have addressed all of them in this CL. https://codereview.chromium.org/336313009/diff/100001/chrome/common/extension... File chrome/common/extensions/permissions/chrome_permission_message_provider.cc (right): https://codereview.chromium.org/336313009/diff/100001/chrome/common/extension... chrome/common/extensions/permissions/chrome_permission_message_provider.cc:130: // Access to USB, Bluetooth and Serial On 2014/07/07 23:07:38, kalman wrote: > these little comments don't add much, it's clear from the check what you're > checking. however, a more general comment here might be helpful. Done. https://codereview.chromium.org/336313009/diff/100001/chrome/common/extension... chrome/common/extensions/permissions/chrome_permission_message_provider.cc:134: ContainsBothMessages(messages, On 2014/07/07 23:07:38, kalman wrote: > could you have a method like "ContainsAll" and overload that with a 2 and 3 > version? because > > Contains(a, b) && Contains(a, c) > > is equivalent to Contains(a, b, c), right? and the latter looks much nicer. Done. https://codereview.chromium.org/336313009/diff/100001/chrome/common/extension... chrome/common/extensions/permissions/chrome_permission_message_provider.cc:137: if (id == PermissionMessage::kUsb) { On 2014/07/07 23:07:38, kalman wrote: > epic nit: can we have these in alphabetic ordering (a) in those Contains calls > and (b) when choosing an arbitrary message ID. Done. https://codereview.chromium.org/336313009/diff/100001/chrome/common/extension... chrome/common/extensions/permissions/chrome_permission_message_provider.cc:200: id == PermissionMessage::kMediaGalleriesAllGalleriesDelete) { On 2014/07/07 23:07:38, kalman wrote: > what about read+copy+delete? The original logic didn't say anything about this case. It was written such that if read+copy exists it doesn't care about delete. However, I changed that now and added a new string for it. https://codereview.chromium.org/336313009/diff/100001/chrome/common/extension... chrome/common/extensions/permissions/chrome_permission_message_provider.cc:210: } else if (ContainsBothMessages( On 2014/07/07 23:07:38, kalman wrote: > no else after continue Done.
lgtm with some more changes https://codereview.chromium.org/336313009/diff/120001/chrome/common/extension... File chrome/common/extensions/permissions/chrome_permission_message_provider.cc (right): https://codereview.chromium.org/336313009/diff/120001/chrome/common/extension... chrome/common/extensions/permissions/chrome_permission_message_provider.cc:47: bool ContainsMessages(PermissionMessages& messages, also this should be "const PermissionMessages&" https://codereview.chromium.org/336313009/diff/120001/chrome/common/extension... chrome/common/extensions/permissions/chrome_permission_message_provider.cc:50: int third_message = PermissionMessage::kNone) { override the function, not optional arguments. that's not style-guidey. it looks fun when they call each other, too, something like bool ContainsMessage(const PermissionMessages& messages, int a, int b) { return FindMessageByID(messages, a) != messages.end() && FindMessageByID(messages, b) != messages.end(); } bool ContainsMessage(const PermissionMessages& messages, int a, int b, int c) { return ContainsMessage(messages, a, b) && FindMessageByID(messages, c) != messages.end(); } https://codereview.chromium.org/336313009/diff/120001/chrome/common/extension... chrome/common/extensions/permissions/chrome_permission_message_provider.cc:139: PermissionMessage::kUsb, alphabetic nits here too
https://codereview.chromium.org/336313009/diff/120001/chrome/common/extension... File chrome/common/extensions/permissions/chrome_permission_message_provider.cc (right): https://codereview.chromium.org/336313009/diff/120001/chrome/common/extension... chrome/common/extensions/permissions/chrome_permission_message_provider.cc:47: bool ContainsMessages(PermissionMessages& messages, On 2014/07/08 00:07:56, kalman wrote: > also this should be "const PermissionMessages&" Done. https://codereview.chromium.org/336313009/diff/120001/chrome/common/extension... chrome/common/extensions/permissions/chrome_permission_message_provider.cc:50: int third_message = PermissionMessage::kNone) { Sorry I didn't know that it is not style-guidely :-) https://codereview.chromium.org/336313009/diff/120001/chrome/common/extension... chrome/common/extensions/permissions/chrome_permission_message_provider.cc:139: PermissionMessage::kUsb, On 2014/07/08 00:07:56, kalman wrote: > alphabetic nits here too Done.
The CQ bit was checked by mohammed@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/mohammed@chromium.org/336313009/70007
Message was sent while issue was closed.
Change committed as 281665 |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
