Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
317783010:
chrome.hid: enrich model with report IDs (Closed)
|
Created:
6 years, 6 months ago by jracle (use Gerrit) Modified:
6 years, 5 months ago CC:
chromium-reviews, chromium-apps-reviews_chromium.org, extensions-reviews_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Visibility:
Public. |
Descriptionchrome.hid: enrich model with report IDs
- add report IDs and max report size
- don't expose sensitive usages
BUG=364423
R=rockot@chromium.org
TESTS=run device_unittests (HidReportDescriptorTest)
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=281133
Patch Set 1 : Enrich JavaScript model (no incoming report filter) #
Total comments: 44
Patch Set 2 : Filter reports + refactor HID connections (ongoing) #
Total comments: 30
Patch Set 3 : Remove HidConnection2 #
Total comments: 20
Patch Set 4 : #
Total comments: 11
Patch Set 5 : Refactor CompleteRead #
Total comments: 20
Patch Set 6 : Fix ProcessReadQueue #
Total comments: 2
Patch Set 7 : Fix hid.idl comment #
Messages
Total messages: 45 (0 generated)
Hi Ken, I've enriched chrome.hid with top-level collection information : report IDs + max report sizes (this is required so that we know what buffer size to allocate when communicating with device). It corresponds to step 1 + 1/2 of step 2 in our previous e-mail : I say 1/2 of step 2, cause I now return only devices which contain at list one non-sensitive usage. Remaining work is to filter out reports based on report IDs. I've also enriched report descriptor unit test with (much more) complex reports, so I can validate report sizes are found correctly. As you said, it seems that slight differences are found depending on the platform, but I prefer (if you don't mind) landing this CL and tackle bugs afterwards. The bug I see for instance is min report size of 1 when a report ID is encountered on MAC platform. Cheers, Julien
Looking totally awesome. No major comments, just lots of little things. https://codereview.chromium.org/317783010/diff/1/chrome/browser/extensions/ap... File chrome/browser/extensions/api/hid/hid_device_manager.cc (right): https://codereview.chromium.org/317783010/diff/1/chrome/browser/extensions/ap... chrome/browser/extensions/api/hid/hid_device_manager.cc:39: nit: No need to add vertical whitespace https://codereview.chromium.org/317783010/diff/1/chrome/browser/extensions/ap... chrome/browser/extensions/api/hid/hid_device_manager.cc:69: if (collection.usage.IsSensitive()) nit: Please include braces around single-line blocks, here and elsewhere. https://codereview.chromium.org/317783010/diff/1/chrome/browser/extensions/ap... chrome/browser/extensions/api/hid/hid_device_manager.cc:77: for (std::set<int>::const_iterator report_ids_iter = I think you could just: api_collection->report_ids.resize(collection.report_ids.size()); std::copy(collection.report_ids.begin(), collection.report_ids.end(), api->collection->report_ids.begin()); Yes? https://codereview.chromium.org/317783010/diff/1/chrome/browser/extensions/ap... chrome/browser/extensions/api/hid/hid_device_manager.cc:85: api_device_info.collections.push_back(collection_ptr); Instead of naming the linked_ptr, just do api_device_info.collections.push_back(make_linked_ptr(api_collection)); https://codereview.chromium.org/317783010/diff/1/device/hid/hid_collection_in... File device/hid/hid_collection_info.h (right): https://codereview.chromium.org/317783010/diff/1/device/hid/hid_collection_in... device/hid/hid_collection_info.h:26: } Vertical Whitespace between }; and } lines And last bit should be "} // namespace device" https://codereview.chromium.org/317783010/diff/1/device/hid/hid_connection_wi... File device/hid/hid_connection_win.cc (right): https://codereview.chromium.org/317783010/diff/1/device/hid/hid_connection_wi... device/hid/hid_connection_win.cc:42: bool DeviceHasReportId(const HidDeviceInfo& device_info) { Very nice. https://codereview.chromium.org/317783010/diff/1/device/hid/hid_connection_wi... device/hid/hid_connection_win.cc:169: expected_buffer_size -= 1; It's this adjust-by-1 logic which exists throughout the Windows code that I have big concerns about. It would be really awesome if you could do some validation of this (testing with a device which uses report IDs and with one that doesn't). But if you'd rather defer on that, as long as the logic in this change matches the logic which existed before, I'm OK with leaving it as-is. https://codereview.chromium.org/317783010/diff/1/device/hid/hid_report_descri... File device/hid/hid_report_descriptor.cc (right): https://codereview.chromium.org/317783010/diff/1/device/hid/hid_report_descri... device/hid/hid_report_descriptor.cc:13: const int kOneByte = 8; How about "kBitsPerByte" https://codereview.chromium.org/317783010/diff/1/device/hid/hid_report_descri... device/hid/hid_report_descriptor.cc:63: switch (current_item->tag()) { In general this is awesome. I wish it were a little cleaner than one big long switch statement, but I don't have any good suggestions off the top of my head. https://codereview.chromium.org/317783010/diff/1/device/hid/hid_report_descri... File device/hid/hid_report_descriptor.h (right): https://codereview.chromium.org/317783010/diff/1/device/hid/hid_report_descri... device/hid/hid_report_descriptor.h:30: void GetTopLevelCollections( If this is going to return report sizes as well, it should be renamed. How about GetDetails instead of GetTopLevelCollections? https://codereview.chromium.org/317783010/diff/1/device/hid/hid_service_mac.cc File device/hid/hid_service_mac.cc (right): https://codereview.chromium.org/317783010/diff/1/device/hid/hid_service_mac.c... device/hid/hid_service_mac.cc:57: for (CFIndex j = 0; j < childrenCount; j++) { nit: ++j https://codereview.chromium.org/317783010/diff/1/device/hid/hid_service_mac.c... device/hid/hid_service_mac.cc:59: (IOHIDElementRef)CFArrayGetValueAtIndex(children, j); static_cast please https://codereview.chromium.org/317783010/diff/1/device/hid/hid_service_mac.c... device/hid/hid_service_mac.cc:172: CFMutableDictionaryRef collectionsMatch = Can you please move this blob of code into its own function (maybe in anonymous namespace?) Specifically all the stuff related to matching collections and populating a list. Also please remember, we_use_underscores, not camelCase for variable names. How about collections_filter instead of "collectionsMatch" https://codereview.chromium.org/317783010/diff/1/device/hid/hid_service_mac.c... device/hid/hid_service_mac.cc:177: int type = kIOHIDElementTypeCollection; How about: const int kCollectionTypeVlaue = kIOHIDElementTypeCollection https://codereview.chromium.org/317783010/diff/1/device/hid/hid_service_mac.c... device/hid/hid_service_mac.cc:178: CFNumberRef typeCFNumberRef = How about CFNumberRef collection_type_id = CFNumberCreate(...); https://codereview.chromium.org/317783010/diff/1/device/hid/hid_service_mac.c... device/hid/hid_service_mac.cc:188: (IOHIDElementRef)CFArrayGetValueAtIndex(collections, i); static_cast https://codereview.chromium.org/317783010/diff/1/device/hid/hid_service_mac.c... device/hid/hid_service_mac.cc:189: DCHECK(IOHIDElementGetType(collection) == kIOHIDElementTypeCollection); Short of the world imploding or IOKit being fundamentally broken, can this check really ever fail? If not, please remove. https://codereview.chromium.org/317783010/diff/1/device/hid/hid_service_mac.c... device/hid/hid_service_mac.cc:194: (HidUsageAndPage::Page)IOHIDElementGetUsagePage(collection); static_cast https://codereview.chromium.org/317783010/diff/1/device/hid/hid_service_win.cc File device/hid/hid_service_win.cc (right): https://codereview.chromium.org/317783010/diff/1/device/hid/hid_service_win.c... device/hid/hid_service_win.cc:204: if (HidP_GetButtonCaps(HidP_Input, I assume there is no overlap between what GetButtonCaps and GetValueCaps enumerate? Also assuming the union of the two sets is the complete set of collections for a device? https://codereview.chromium.org/317783010/diff/1/device/hid/hid_usage_and_page.h File device/hid/hid_usage_and_page.h (right): https://codereview.chromium.org/317783010/diff/1/device/hid/hid_usage_and_pag... device/hid/hid_usage_and_page.h:129: // Such usages can raise security or privacy concerns. Function docs should be third-person declarative statements e.g.: // Indicates whether this usage is protected by Chrome. https://codereview.chromium.org/317783010/diff/1/device/hid/hid_usage_and_pag... device/hid/hid_usage_and_page.h:130: bool IsSensitive() const; How about IsProtected()
Oh also - the sensitive report filtering absolutely must be added to this CL if you want to remove whole-device filtering.
Oh also - the sensitive report filtering absolutely must be added to this CL if you want to remove whole-device filtering.
https://codereview.chromium.org/317783010/diff/1/chrome/browser/extensions/ap... File chrome/browser/extensions/api/hid/hid_device_manager.cc (right): https://codereview.chromium.org/317783010/diff/1/chrome/browser/extensions/ap... chrome/browser/extensions/api/hid/hid_device_manager.cc:39: OK. Though I'm a bit puzzled by this rule, for me code lacks readability when too many groups of code are found. I also have hard time not starting with a new line after '{', but this is subject to (big) debate in my team! On 2014/06/06 20:04:43, Ken Rockot wrote: > nit: No need to add vertical whitespace https://codereview.chromium.org/317783010/diff/1/chrome/browser/extensions/ap... chrome/browser/extensions/api/hid/hid_device_manager.cc:69: if (collection.usage.IsSensitive()) This I sign 100%, that's safest.. I recall Apple's OpenSSL bug On 2014/06/06 20:04:43, Ken Rockot wrote: > nit: Please include braces around single-line blocks, here and elsewhere. https://codereview.chromium.org/317783010/diff/1/chrome/browser/extensions/ap... chrome/browser/extensions/api/hid/hid_device_manager.cc:77: for (std::set<int>::const_iterator report_ids_iter = Oh yes! On 2014/06/06 20:04:43, Ken Rockot wrote: > I think you could just: > > api_collection->report_ids.resize(collection.report_ids.size()); > std::copy(collection.report_ids.begin(), collection.report_ids.end(), > api->collection->report_ids.begin()); > > Yes? https://codereview.chromium.org/317783010/diff/1/chrome/browser/extensions/ap... chrome/browser/extensions/api/hid/hid_device_manager.cc:85: api_device_info.collections.push_back(collection_ptr); gr8, sure. Much better. On 2014/06/06 20:04:43, Ken Rockot wrote: > Instead of naming the linked_ptr, just do > > api_device_info.collections.push_back(make_linked_ptr(api_collection)); https://codereview.chromium.org/317783010/diff/1/device/hid/hid_collection_in... File device/hid/hid_collection_info.h (right): https://codereview.chromium.org/317783010/diff/1/device/hid/hid_collection_in... device/hid/hid_collection_info.h:26: } Sorry, that one is being removed by git cl format.. On 2014/06/06 20:04:43, Ken Rockot wrote: > Vertical Whitespace between }; and } lines > > And last bit should be "} // namespace device" https://codereview.chromium.org/317783010/diff/1/device/hid/hid_connection_wi... File device/hid/hid_connection_win.cc (right): https://codereview.chromium.org/317783010/diff/1/device/hid/hid_connection_wi... device/hid/hid_connection_win.cc:42: bool DeviceHasReportId(const HidDeviceInfo& device_info) { Thx. Wish I could use lambdas and C++11 though. Sorry for my baby-knowledge of C++03, I'm so used to lambdas in other languages that I have hard time reminding me of good std:c++ patterns!! On 2014/06/06 20:04:43, Ken Rockot wrote: > Very nice. https://codereview.chromium.org/317783010/diff/1/device/hid/hid_connection_wi... device/hid/hid_connection_win.cc:169: expected_buffer_size -= 1; I totally follow you on that point, and will validate as you propose. I'm very uncomfortable with that black-magic (though this would mean removing my great DeviceHasReportId matcher ;). Not kidding, I'll try to fix it in that CL (cause, even if I submit today - I know I'm supposed to be off, just here for 2hs, I'll have probably to submit other patch sets next week regarding protected reports filtering). On 2014/06/06 20:04:43, Ken Rockot wrote: > It's this adjust-by-1 logic which exists throughout the Windows code that I have > big concerns about. > > It would be really awesome if you could do some validation of this (testing with > a device which uses report IDs and with one that doesn't). > > But if you'd rather defer on that, as long as the logic in this change matches > the logic which existed before, I'm OK with leaving it as-is. https://codereview.chromium.org/317783010/diff/1/device/hid/hid_report_descri... File device/hid/hid_report_descriptor.cc (right): https://codereview.chromium.org/317783010/diff/1/device/hid/hid_report_descri... device/hid/hid_report_descriptor.cc:13: const int kOneByte = 8; +2! On 2014/06/06 20:04:43, Ken Rockot wrote: > How about "kBitsPerByte" https://codereview.chromium.org/317783010/diff/1/device/hid/hid_report_descri... device/hid/hid_report_descriptor.cc:63: switch (current_item->tag()) { Thanks, I'm also a bit uncomfortable with it, but the reason I kept it as is are : - keeping 1 for loop. - keeping logic understandable. - fixing bugs (!): I tried to save some code, failed poorly cause every time I was missing sth (like lately push/pop stuff.. a killer) I'm not against refactoring this when possible. On 2014/06/06 20:04:43, Ken Rockot wrote: > In general this is awesome. I wish it were a little cleaner than one big long > switch statement, but I don't have any good suggestions off the top of my head. https://codereview.chromium.org/317783010/diff/1/device/hid/hid_report_descri... File device/hid/hid_report_descriptor.h (right): https://codereview.chromium.org/317783010/diff/1/device/hid/hid_report_descri... device/hid/hid_report_descriptor.h:30: void GetTopLevelCollections( +1 On 2014/06/06 20:04:43, Ken Rockot wrote: > If this is going to return report sizes as well, it should be renamed. How about > GetDetails instead of GetTopLevelCollections? https://codereview.chromium.org/317783010/diff/1/device/hid/hid_service_mac.cc File device/hid/hid_service_mac.cc (right): https://codereview.chromium.org/317783010/diff/1/device/hid/hid_service_mac.c... device/hid/hid_service_mac.cc:57: for (CFIndex j = 0; j < childrenCount; j++) { I would have bet :). Had that before, jeez! You're right, more performant On 2014/06/06 20:04:43, Ken Rockot wrote: > nit: ++j https://codereview.chromium.org/317783010/diff/1/device/hid/hid_service_mac.c... device/hid/hid_service_mac.cc:59: (IOHIDElementRef)CFArrayGetValueAtIndex(children, j); Sorry. C-ish stuff again On 2014/06/06 20:04:43, Ken Rockot wrote: > static_cast please https://codereview.chromium.org/317783010/diff/1/device/hid/hid_service_mac.c... device/hid/hid_service_mac.cc:172: CFMutableDictionaryRef collectionsMatch = It's like you were reading in my brains, fightening! I've forgot to refactor this. To add to your comment, I gonna also look at 'hid_utils_mac.h/cc' ; I'm not the originator of that file, but am pretty uncomfortable seeing it only used by hid_service_mac.cc. Will extract all that in its anonymous namespace if you don't mind, and will remove that hid_utils_mac stuff. On 2014/06/06 20:04:44, Ken Rockot wrote: > Can you please move this blob of code into its own function (maybe in anonymous > namespace?) Specifically all the stuff related to matching collections and > populating a list. > > Also please remember, we_use_underscores, not camelCase for variable names. > > How about collections_filter instead of "collectionsMatch" https://codereview.chromium.org/317783010/diff/1/device/hid/hid_service_mac.c... device/hid/hid_service_mac.cc:177: int type = kIOHIDElementTypeCollection; sure On 2014/06/06 20:04:44, Ken Rockot wrote: > How about: > > const int kCollectionTypeVlaue = kIOHIDElementTypeCollection https://codereview.chromium.org/317783010/diff/1/device/hid/hid_service_mac.c... device/hid/hid_service_mac.cc:178: CFNumberRef typeCFNumberRef = sorry, that's awful variable name I wished to rename.. and forgot to do it! On 2014/06/06 20:04:43, Ken Rockot wrote: > How about > > CFNumberRef collection_type_id = CFNumberCreate(...); https://codereview.chromium.org/317783010/diff/1/device/hid/hid_service_mac.c... device/hid/hid_service_mac.cc:188: (IOHIDElementRef)CFArrayGetValueAtIndex(collections, i); sure On 2014/06/06 20:04:43, Ken Rockot wrote: > static_cast https://codereview.chromium.org/317783010/diff/1/device/hid/hid_service_mac.c... device/hid/hid_service_mac.cc:189: DCHECK(IOHIDElementGetType(collection) == kIOHIDElementTypeCollection); Let me think half a sec.. That's indeed stupid defensive code from my part oops. On 2014/06/06 20:04:43, Ken Rockot wrote: > Short of the world imploding or IOKit being fundamentally broken, can this check > really ever fail? If not, please remove. https://codereview.chromium.org/317783010/diff/1/device/hid/hid_service_mac.c... device/hid/hid_service_mac.cc:194: (HidUsageAndPage::Page)IOHIDElementGetUsagePage(collection); yes On 2014/06/06 20:04:44, Ken Rockot wrote: > static_cast https://codereview.chromium.org/317783010/diff/1/device/hid/hid_service_win.cc File device/hid/hid_service_win.cc (right): https://codereview.chromium.org/317783010/diff/1/device/hid/hid_service_win.c... device/hid/hid_service_win.cc:204: if (HidP_GetButtonCaps(HidP_Input, That's my understanding. On 2014/06/06 20:04:44, Ken Rockot wrote: > I assume there is no overlap between what GetButtonCaps and GetValueCaps > enumerate? Also assuming the union of the two sets is the complete set of > collections for a device? https://codereview.chromium.org/317783010/diff/1/device/hid/hid_usage_and_page.h File device/hid/hid_usage_and_page.h (right): https://codereview.chromium.org/317783010/diff/1/device/hid/hid_usage_and_pag... device/hid/hid_usage_and_page.h:129: // Such usages can raise security or privacy concerns. Got it. On 2014/06/06 20:04:44, Ken Rockot wrote: > Function docs should be third-person declarative statements e.g.: > > // Indicates whether this usage is protected by Chrome. https://codereview.chromium.org/317783010/diff/1/device/hid/hid_usage_and_pag... device/hid/hid_usage_and_page.h:130: bool IsSensitive() const; I'll rename with you proposal. Understand mine is not satisfactory. But maybe we can find another one with a name which would reflect what those usages are (represent) inherently, as OOP tells us, not what their usage (pardon the pun) is intended to become. To be clearer, mouse reports lead to privacy concerns, but are not by themselves 'protected' (at least not on all OSes). You see my point? On 2014/06/06 20:04:44, Ken Rockot wrote: > How about IsProtected()
Ken, I'll refrain myself from comitting today. I've got most the changes you request, but need to do the tests on windows (report ID stuff) + also filter protected reports. Have a nice WE and ttyl https://codereview.chromium.org/317783010/diff/1/device/hid/hid_collection_in... File device/hid/hid_collection_info.h (right): https://codereview.chromium.org/317783010/diff/1/device/hid/hid_collection_in... device/hid/hid_collection_info.h:26: } Oh I see, this was cause "} // namespace device" was missing On 2014/06/07 12:56:21, jracle wrote: > Sorry, that one is being removed by git cl format.. > > On 2014/06/06 20:04:43, Ken Rockot wrote: > > Vertical Whitespace between }; and } lines > > > > And last bit should be "} // namespace device" >
https://codereview.chromium.org/317783010/diff/1/device/hid/hid_connection_wi... File device/hid/hid_connection_win.cc (right): https://codereview.chromium.org/317783010/diff/1/device/hid/hid_connection_wi... device/hid/hid_connection_win.cc:169: expected_buffer_size -= 1; Hi Ken, today I've ben given a special firmware to try out above logic, and it appears that it is correct! Windows pads with a leading 0x00 reports without ID. Yet, I'm also grouping logic for filtering out protected usages now, and hence will commit tomorrow or the day after. Cheers On 2014/06/07 12:56:21, jracle wrote: > I totally follow you on that point, and will validate as you propose. I'm very > uncomfortable with that black-magic (though this would mean removing my great > DeviceHasReportId matcher ;). > > Not kidding, I'll try to fix it in that CL (cause, even if I submit today - I > know I'm supposed to be off, just here for 2hs, I'll have probably to submit > other patch sets next week regarding protected reports filtering). > > On 2014/06/06 20:04:43, Ken Rockot wrote: > > It's this adjust-by-1 logic which exists throughout the Windows code that I > have > > big concerns about. > > > > It would be really awesome if you could do some validation of this (testing > with > > a device which uses report IDs and with one that doesn't). > > > > But if you'd rather defer on that, as long as the logic in this change matches > > the logic which existed before, I'm OK with leaving it as-is. >
On 2014/06/10 14:07:40, jracle wrote: > https://codereview.chromium.org/317783010/diff/1/device/hid/hid_connection_wi... > File device/hid/hid_connection_win.cc (right): > > https://codereview.chromium.org/317783010/diff/1/device/hid/hid_connection_wi... > device/hid/hid_connection_win.cc:169: expected_buffer_size -= 1; > Hi Ken, > > today I've ben given a special firmware to try out above logic, and it appears > that it is correct! Windows pads with a leading 0x00 reports without ID. > > Yet, I'm also grouping logic for filtering out protected usages now, and hence > will commit tomorrow or the day after. > > Cheers > > On 2014/06/07 12:56:21, jracle wrote: > > I totally follow you on that point, and will validate as you propose. I'm very > > uncomfortable with that black-magic (though this would mean removing my great > > DeviceHasReportId matcher ;). > > > > Not kidding, I'll try to fix it in that CL (cause, even if I submit today - I > > know I'm supposed to be off, just here for 2hs, I'll have probably to submit > > other patch sets next week regarding protected reports filtering). > > > > On 2014/06/06 20:04:43, Ken Rockot wrote: > > > It's this adjust-by-1 logic which exists throughout the Windows code that I > > have > > > big concerns about. > > > > > > It would be really awesome if you could do some validation of this (testing > > with > > > a device which uses report IDs and with one that doesn't). > > > > > > But if you'd rather defer on that, as long as the logic in this change > matches > > > the logic which existed before, I'm OK with leaving it as-is. > > That's great news!
Hi Ken, I submitted a new patch (huge one..) in which report filtering is on, but also refactoring of HidConnection* classes.. and I'd need your help commenting it out :). Thanks very much in advance! Julien https://codereview.chromium.org/317783010/diff/20001/device/hid/hid_connectio... File device/hid/hid_connection.cc (right): https://codereview.chromium.org/317783010/diff/20001/device/hid/hid_connectio... device/hid/hid_connection.cc:71: has_report_id_ = HasReportId(device_info); I prefer to cache this info.. https://codereview.chromium.org/317783010/diff/20001/device/hid/hid_connectio... device/hid/hid_connection.cc:108: if (IsReportIdProtected(report_id)) { We need to prevent R/W of protected reports. https://codereview.chromium.org/317783010/diff/20001/device/hid/hid_connectio... device/hid/hid_connection.cc:161: bool HidConnection::FilterInputReport( This is new filtering method, used together with next one and matchers available in anonymous namespace. https://codereview.chromium.org/317783010/diff/20001/device/hid/hid_connection.h File device/hid/hid_connection.h (right): https://codereview.chromium.org/317783010/diff/20001/device/hid/hid_connectio... device/hid/hid_connection.h:29: void Read(scoped_refptr<net::IOBufferWithSize> buffer, I made those methods the single entry-point, and create abstract Platform* methods called within them. Purpose is to concentrate report validation logic here, as a must-do step. https://codereview.chromium.org/317783010/diff/20001/device/hid/hid_connectio... device/hid/hid_connection.h:75: struct PendingHidReport { I'm very incomfortable with those 2 structs. Their naming is also bizarre (Pending?) (where is HID stuff in a buffer?). There should be a means to merge them with structs used in Windows. https://codereview.chromium.org/317783010/diff/20001/device/hid/hid_connectio... device/hid/hid_connection.h:90: class HidConnection2 : public HidConnection { This is of course 'temporary code' before refactoring. We can try to move this code in parent class, or find relevant name for this class, which is base class used by HidConnectionLinux and HidConnectionMac. https://codereview.chromium.org/317783010/diff/20001/device/hid/hid_connectio... device/hid/hid_connection.h:105: std::queue<PendingHidReport> pending_reports_; see above comments https://codereview.chromium.org/317783010/diff/20001/device/hid/hid_connectio... File device/hid/hid_connection_win.cc (right): https://codereview.chromium.org/317783010/diff/20001/device/hid/hid_connectio... device/hid/hid_connection_win.cc:29: const uint8_t kNullReportId = 0x00; (duplication) https://codereview.chromium.org/317783010/diff/20001/device/hid/hid_connectio... device/hid/hid_connection_win.cc:33: struct PendingHidTransfer : public base::RefCounted<PendingHidTransfer>, That's sth. we could merge with HidConnection2's 'PendingHid*' structs. https://codereview.chromium.org/317783010/diff/20001/device/hid/hid_connectio... File device/hid/hid_connection_win.h (right): https://codereview.chromium.org/317783010/diff/20001/device/hid/hid_connectio... device/hid/hid_connection_win.h:17: struct PendingHidTransfer; I wish we could merge this with HidConnection2's stuff!!
https://codereview.chromium.org/317783010/diff/20001/chrome/common/extensions... File chrome/common/extensions/api/hid.idl (right): https://codereview.chromium.org/317783010/diff/20001/chrome/common/extensions... chrome/common/extensions/api/hid.idl:24: HidUsageAndPage usage; nit: I think I'd prefer the API consumer to be able to just say "collection.usage" and "collection.usage_page" rather than "collection.usage.usage" and "collection.usage.usage_page". Could you just inline the usage and usage_page fields within HidCollectionInfo? I think it makes sense for the C++ code to keep a separate struct for this data as it does now, but the API could expose it without the extra level of indirection. https://codereview.chromium.org/317783010/diff/20001/chrome/common/extensions... chrome/common/extensions/api/hid.idl:34: // |maxInputReportSize|: Top-level collection's max input report size. Tangent: Could each HidCollectionInfo struct also include information about max report sizes for each specific collection? https://codereview.chromium.org/317783010/diff/20001/device/hid/hid_connectio... File device/hid/hid_connection.cc (right): https://codereview.chromium.org/317783010/diff/20001/device/hid/hid_connectio... device/hid/hid_connection.cc:14: const uint8_t kInvalidReportId = 0xFF; Shouldn't this be kAnyReportId given the usage? https://codereview.chromium.org/317783010/diff/20001/device/hid/hid_connectio... device/hid/hid_connection.cc:42: bool HasReportId(const HidDeviceInfo& device_info, This doesn't quite do what the name implies. How about FindCollectionByReportId? Also no default arguments please. It's fine if NULL implies that the info need not be returned, but force callers to pass it explicitly. If you wanted to, you could define a wrapper like bool HasReportId(const HidDeviceInfo& device_info) { return FindCollectionByReportId(device_info, kAnyReportId, NULL); } I think that would clarify what's going on in all of this. https://codereview.chromium.org/317783010/diff/20001/device/hid/hid_connectio... device/hid/hid_connection.cc:71: has_report_id_ = HasReportId(device_info); On 2014/06/11 11:20:04, jracle wrote: > I prefer to cache this info.. Sure, sounds fine to me. https://codereview.chromium.org/317783010/diff/20001/device/hid/hid_connectio... device/hid/hid_connection.cc:201: void HidConnection2::PlatformRead(scoped_refptr<net::IOBufferWithSize> buffer, Ah, I see why you created this temporary class now. Um. https://codereview.chromium.org/317783010/diff/20001/device/hid/hid_connection.h File device/hid/hid_connection.h (right): https://codereview.chromium.org/317783010/diff/20001/device/hid/hid_connectio... device/hid/hid_connection.h:29: void Read(scoped_refptr<net::IOBufferWithSize> buffer, On 2014/06/11 11:20:04, jracle wrote: > I made those methods the single entry-point, and create abstract Platform* > methods called within them. Purpose is to concentrate report validation logic > here, as a must-do step. Great! https://codereview.chromium.org/317783010/diff/20001/device/hid/hid_connectio... device/hid/hid_connection.h:61: bool FilterInputReport(scoped_refptr<net::IOBufferWithSize> buffer, Can this be private? https://codereview.chromium.org/317783010/diff/20001/device/hid/hid_connectio... device/hid/hid_connection.h:75: struct PendingHidReport { On 2014/06/11 11:20:04, jracle wrote: > I'm very incomfortable with those 2 structs. Their naming is also bizarre > (Pending?) (where is HID stuff in a buffer?). > > There should be a means to merge them with structs used in Windows. I agree they could be cleaned up, but I'd recommend waiting for a future CL. Trying to unify this right now is making the CL way more complicated than it needs to be. What if for now HidConnection just exposed a protoected CompleteRead method, which receives a filled IOBufferWithSize and an IOCallback. This method can perform the filtering and call the callback if allowed. Then each platform can just issue a CompleteRead(buffer, callback) instead of running the callback where they're doing it now. FilterInputReport can then be private, there's no need for a refactor of HidConnection or pending report structures, no need to unify Windows+Linux+Mac queueing. (I think this is all true - sorry if I'm missing something) https://codereview.chromium.org/317783010/diff/20001/device/hid/hid_connectio... device/hid/hid_connection.h:90: class HidConnection2 : public HidConnection { On 2014/06/11 11:20:04, jracle wrote: > This is of course 'temporary code' before refactoring. > We can try to move this code in parent class, or find relevant name for this > class, which is base class used by HidConnectionLinux and HidConnectionMac. I am very confused by this. https://codereview.chromium.org/317783010/diff/20001/device/hid/hid_connectio... File device/hid/hid_connection_linux.h (right): https://codereview.chromium.org/317783010/diff/20001/device/hid/hid_connectio... device/hid/hid_connection_linux.h:17: explicit HidConnectionLinux(HidDeviceInfo device_info, std::string dev_node); Only need explicit for single-argument constructors.
Hi Ken, thanks for your comments, see mine. I need to look at them more in depth tomorrow. In the meantime, I'll send you the patches in case you need them. Regards, Julien https://codereview.chromium.org/317783010/diff/20001/chrome/common/extensions... File chrome/common/extensions/api/hid.idl (right): https://codereview.chromium.org/317783010/diff/20001/chrome/common/extensions... chrome/common/extensions/api/hid.idl:24: HidUsageAndPage usage; Sure. Agree. On 2014/06/19 19:29:56, Ken Rockot wrote: > nit: I think I'd prefer the API consumer to be able to just say > "collection.usage" and "collection.usage_page" rather than > "collection.usage.usage" and "collection.usage.usage_page". Could you just > inline the usage and usage_page fields within HidCollectionInfo? > > I think it makes sense for the C++ code to keep a separate struct for this data > as it does now, but the API could expose it without the extra level of > indirection. https://codereview.chromium.org/317783010/diff/20001/chrome/common/extensions... chrome/common/extensions/api/hid.idl:34: // |maxInputReportSize|: Top-level collection's max input report size. This one is more problematic, since apart from linux where we need to compute max report size ourselves, device APIs aggregate them. We need max report size to allocate sufficient buffer size for read/write. On 2014/06/19 19:29:56, Ken Rockot wrote: > Tangent: Could each HidCollectionInfo struct also include information about max > report sizes for each specific collection? https://codereview.chromium.org/317783010/diff/20001/device/hid/hid_connectio... File device/hid/hid_connection.cc (right): https://codereview.chromium.org/317783010/diff/20001/device/hid/hid_connectio... device/hid/hid_connection.cc:14: const uint8_t kInvalidReportId = 0xFF; Right, that's indeed what it means. On 2014/06/19 19:29:56, Ken Rockot wrote: > Shouldn't this be kAnyReportId given the usage? https://codereview.chromium.org/317783010/diff/20001/device/hid/hid_connectio... device/hid/hid_connection.cc:42: bool HasReportId(const HidDeviceInfo& device_info, Thanks so much for noticing it! See, indeed default arguments in this case are dangerous : I merged 2 functions which indeed have 2 meaning, and you've found them.. That's a great review. On 2014/06/19 19:29:56, Ken Rockot wrote: > This doesn't quite do what the name implies. How about FindCollectionByReportId? > > Also no default arguments please. It's fine if NULL implies that the info need > not be returned, but force callers to pass it explicitly. > > If you wanted to, you could define a wrapper like > > bool HasReportId(const HidDeviceInfo& device_info) { > return FindCollectionByReportId(device_info, kAnyReportId, NULL); > } > > I think that would clarify what's going on in all of this. https://codereview.chromium.org/317783010/diff/20001/device/hid/hid_connectio... device/hid/hid_connection.cc:71: has_report_id_ = HasReportId(device_info); Thx. On 2014/06/19 19:29:56, Ken Rockot wrote: > On 2014/06/11 11:20:04, jracle wrote: > > I prefer to cache this info.. > > Sure, sounds fine to me. https://codereview.chromium.org/317783010/diff/20001/device/hid/hid_connectio... device/hid/hid_connection.cc:201: void HidConnection2::PlatformRead(scoped_refptr<net::IOBufferWithSize> buffer, Yes sorry, this was to eliminate duplication. Hence, name is not acceptable as is, of course. I didn't proceed further refactoring prior to your comments, and was not inspired to find a relevant name for this class if it has to remain for this CL. Any idea? :) On 2014/06/19 19:29:56, Ken Rockot wrote: > Ah, I see why you created this temporary class now. Um. https://codereview.chromium.org/317783010/diff/20001/device/hid/hid_connection.h File device/hid/hid_connection.h (right): https://codereview.chromium.org/317783010/diff/20001/device/hid/hid_connectio... device/hid/hid_connection.h:75: struct PendingHidReport { Agree we should keep them in this CL, that'd be too many changes at a time. It's a bit late for me to try out your proposal in my brain, but wild guess is that you've got a good catch there. On 2014/06/19 19:29:57, Ken Rockot wrote: > On 2014/06/11 11:20:04, jracle wrote: > > I'm very incomfortable with those 2 structs. Their naming is also bizarre > > (Pending?) (where is HID stuff in a buffer?). > > > > There should be a means to merge them with structs used in Windows. > > I agree they could be cleaned up, but I'd recommend waiting for a future CL. > > Trying to unify this right now is making the CL way more complicated than it > needs to be. What if for now HidConnection just exposed a protoected > CompleteRead method, which receives a filled IOBufferWithSize and an IOCallback. > This method can perform the filtering and call the callback if allowed. > > Then each platform can just issue a CompleteRead(buffer, callback) instead of > running the callback where they're doing it now. > > FilterInputReport can then be private, there's no need for a refactor of > HidConnection or pending report structures, no need to unify Windows+Linux+Mac > queueing. > > (I think this is all true - sorry if I'm missing something) https://codereview.chromium.org/317783010/diff/20001/device/hid/hid_connectio... device/hid/hid_connection.h:90: class HidConnection2 : public HidConnection { Code duplication removal. On 2014/06/19 19:29:56, Ken Rockot wrote: > On 2014/06/11 11:20:04, jracle wrote: > > This is of course 'temporary code' before refactoring. > > We can try to move this code in parent class, or find relevant name for this > > class, which is base class used by HidConnectionLinux and HidConnectionMac. > > I am very confused by this. https://codereview.chromium.org/317783010/diff/20001/device/hid/hid_connectio... File device/hid/hid_connection_linux.h (right): https://codereview.chromium.org/317783010/diff/20001/device/hid/hid_connectio... device/hid/hid_connection_linux.h:17: explicit HidConnectionLinux(HidDeviceInfo device_info, std::string dev_node); oh indeed! Thanks On 2014/06/19 19:29:57, Ken Rockot wrote: > Only need explicit for single-argument constructors.
Looking really good; structure is great. I have no large change requests. Mostly documentation and naming nits. https://codereview.chromium.org/317783010/diff/40001/chrome/common/extensions... File chrome/common/extensions/api/hid.idl (right): https://codereview.chromium.org/317783010/diff/40001/chrome/common/extensions... chrome/common/extensions/api/hid.idl:15: long usage_page; usagePage https://codereview.chromium.org/317783010/diff/40001/device/hid/hid_connectio... File device/hid/hid_connection.cc (right): https://codereview.chromium.org/317783010/diff/40001/device/hid/hid_connectio... device/hid/hid_connection.cc:14: const uint8_t kAnyReportId = 0xFF; How about moving these constants to hid_connection_info.h so they can be shared across modules? Seems like a reasonable place since HidConnectionInfo stores report IDs. https://codereview.chromium.org/317783010/diff/40001/device/hid/hid_connectio... device/hid/hid_connection.cc:16: struct CollectionHasReportId { Maybe a brief comment that this is a functor used to filter collections by report ID. https://codereview.chromium.org/317783010/diff/40001/device/hid/hid_connectio... device/hid/hid_connection.cc:36: struct CollectionIsProtected { Brief comment here as well. https://codereview.chromium.org/317783010/diff/40001/device/hid/hid_connectio... device/hid/hid_connection.cc:73: DCHECK(thread_checker_.CalledOnValidThread()); Actually I realize this was in the other implementations before, but there's really no need for this DCHECK. The valid thread is by definition (by construction of ThreadChecker) the thread on which the constructor is called. This will always be true. https://codereview.chromium.org/317783010/diff/40001/device/hid/hid_connection.h File device/hid/hid_connection.h (right): https://codereview.chromium.org/317783010/diff/40001/device/hid/hid_connectio... device/hid/hid_connection.h:59: void CompleteRead(scoped_refptr<net::IOBufferWithSize> buffer, Please document that platform implementations must call this rather than directly running the callback received by PlatformRead. https://codereview.chromium.org/317783010/diff/40001/device/hid/hid_connectio... File device/hid/hid_connection_win.cc (right): https://codereview.chromium.org/317783010/diff/40001/device/hid/hid_connectio... device/hid/hid_connection_win.cc:228: if (transfer->receive_buffer_) { Could you please preserve the documentation that was here before? You can shorten it if you like, but it may not be obvious to someone why this receive_buffer business is being done. https://codereview.chromium.org/317783010/diff/40001/device/hid/hid_report_de... File device/hid/hid_report_descriptor_unittest.cc (right): https://codereview.chromium.org/317783010/diff/40001/device/hid/hid_report_de... device/hid/hid_report_descriptor_unittest.cc:374: void DumpDescriptor(const HidReportDescriptor& descriptor) { It looks like if you get rid of this, you can get rid of all the ostream operators. I realize this dumps potentially useful debugging info to the console, but it doesn't look like it's needed for any of the tests. The tests are looking good. I would encourage you to consider removing this and all the operator overloads. It's a lot of unnecessary code. https://codereview.chromium.org/317783010/diff/40001/device/hid/hid_report_de... device/hid/hid_report_descriptor_unittest.cc:663: void GetDetails(const std::vector<HidCollectionInfo>& expected_collections, ValidateDetails? https://codereview.chromium.org/317783010/diff/40001/device/hid/hid_service_m... File device/hid/hid_service_mac.cc (right): https://codereview.chromium.org/317783010/diff/40001/device/hid/hid_service_m... device/hid/hid_service_mac.cc:120: IOHIDElementRef collection = static_cast<IOHIDElementRef>( Is there any reason you can't cast to a const IOHIDElementRef instead of casting away the const-ness of the void*?
https://codereview.chromium.org/317783010/diff/40001/chrome/common/extensions... File chrome/common/extensions/api/hid.idl (right): https://codereview.chromium.org/317783010/diff/40001/chrome/common/extensions... chrome/common/extensions/api/hid.idl:15: long usage_page; my bad. On 2014/06/27 14:50:22, Ken Rockot wrote: > usagePage https://codereview.chromium.org/317783010/diff/40001/device/hid/hid_connectio... File device/hid/hid_connection.cc (right): https://codereview.chromium.org/317783010/diff/40001/device/hid/hid_connectio... device/hid/hid_connection.cc:14: const uint8_t kAnyReportId = 0xFF; Makes sense. On 2014/06/27 14:50:22, Ken Rockot wrote: > How about moving these constants to hid_connection_info.h so they can be shared > across modules? Seems like a reasonable place since HidConnectionInfo stores > report IDs. https://codereview.chromium.org/317783010/diff/40001/device/hid/hid_connectio... device/hid/hid_connection.cc:16: struct CollectionHasReportId { Sure. On 2014/06/27 14:50:22, Ken Rockot wrote: > Maybe a brief comment that this is a functor used to filter collections by > report ID. https://codereview.chromium.org/317783010/diff/40001/device/hid/hid_connectio... device/hid/hid_connection.cc:36: struct CollectionIsProtected { OK. On 2014/06/27 14:50:22, Ken Rockot wrote: > Brief comment here as well. https://codereview.chromium.org/317783010/diff/40001/device/hid/hid_connectio... device/hid/hid_connection.cc:73: DCHECK(thread_checker_.CalledOnValidThread()); Sorry, got it now..! Useless indeed On 2014/06/27 14:50:22, Ken Rockot wrote: > Actually I realize this was in the other implementations before, but there's > really no need for this DCHECK. The valid thread is by definition (by > construction of ThreadChecker) the thread on which the constructor is called. > This will always be true. https://codereview.chromium.org/317783010/diff/40001/device/hid/hid_connection.h File device/hid/hid_connection.h (right): https://codereview.chromium.org/317783010/diff/40001/device/hid/hid_connectio... device/hid/hid_connection.h:59: void CompleteRead(scoped_refptr<net::IOBufferWithSize> buffer, Sure. On 2014/06/27 14:50:22, Ken Rockot wrote: > Please document that platform implementations must call this rather than > directly running the callback received by PlatformRead. https://codereview.chromium.org/317783010/diff/40001/device/hid/hid_connectio... File device/hid/hid_connection_win.cc (right): https://codereview.chromium.org/317783010/diff/40001/device/hid/hid_connectio... device/hid/hid_connection_win.cc:228: if (transfer->receive_buffer_) { Good catch. That's unfortunate I didn't keep it! On 2014/06/27 14:50:22, Ken Rockot wrote: > Could you please preserve the documentation that was here before? You can > shorten it if you like, but it may not be obvious to someone why this > receive_buffer business is being done. https://codereview.chromium.org/317783010/diff/40001/device/hid/hid_report_de... File device/hid/hid_report_descriptor_unittest.cc (right): https://codereview.chromium.org/317783010/diff/40001/device/hid/hid_report_de... device/hid/hid_report_descriptor_unittest.cc:374: void DumpDescriptor(const HidReportDescriptor& descriptor) { Sure, now we can indeed. On 2014/06/27 14:50:22, Ken Rockot wrote: > It looks like if you get rid of this, you can get rid of all the ostream > operators. I realize this dumps potentially useful debugging info to the > console, but it doesn't look like it's needed for any of the tests. > > The tests are looking good. I would encourage you to consider removing this and > all the operator overloads. It's a lot of unnecessary code. https://codereview.chromium.org/317783010/diff/40001/device/hid/hid_report_de... device/hid/hid_report_descriptor_unittest.cc:663: void GetDetails(const std::vector<HidCollectionInfo>& expected_collections, Much better, reflects what it does.. On 2014/06/27 14:50:22, Ken Rockot wrote: > ValidateDetails? https://codereview.chromium.org/317783010/diff/40001/device/hid/hid_service_m... File device/hid/hid_service_mac.cc (right): https://codereview.chromium.org/317783010/diff/40001/device/hid/hid_service_m... device/hid/hid_service_mac.cc:120: IOHIDElementRef collection = static_cast<IOHIDElementRef>( oh I did an horror.. I should use only const_cast in extreme situation! (that's like fixing a unit test to make a code pass!) On 2014/06/27 14:50:23, Ken Rockot wrote: > Is there any reason you can't cast to a const IOHIDElementRef instead of casting > away the const-ness of the void*?
Guys, I've posted a new patch set. Thanks to review.
ALMOST THERE! :D https://codereview.chromium.org/317783010/diff/60001/device/hid/hid_connection.h File device/hid/hid_connection.h (right): https://codereview.chromium.org/317783010/diff/60001/device/hid/hid_connectio... device/hid/hid_connection.h:67: // empty buffer in such case. "It filters protected reports and runs the callback." would be fine for the last sentence. https://codereview.chromium.org/317783010/diff/60001/device/hid/hid_connectio... File device/hid/hid_connection_linux.cc (right): https://codereview.chromium.org/317783010/diff/60001/device/hid/hid_connectio... device/hid/hid_connection_linux.cc:223: CompleteRead(report.buffer, read.callback); Let me propose one final significant change. What if CompleteRead returned a boolean indicating whether or not it called the callback. Then in the case where a report is filtered, CompleteRead won't actually call the callback at all; instead it will just return false, and the platform implementation can requeue the pending read. This has the nice effect of API users not getting lots of empty reports, which I think will be kind of annoying othwerwise (imagine monitoring a keyboard's battery life, and constantly having to check if the HID report you received was empty because the user keeps typing on the keyboard and generating filtered input reports.) https://codereview.chromium.org/317783010/diff/60001/device/hid/hid_service_m... File device/hid/hid_service_mac.cc (right): https://codereview.chromium.org/317783010/diff/60001/device/hid/hid_service_m... device/hid/hid_service_mac.cc:121: const_cast<void*>(CFArrayGetValueAtIndex(collections, i))); Also please get rid of the const_cast :)
https://codereview.chromium.org/317783010/diff/60001/device/hid/hid_connection.h File device/hid/hid_connection.h (right): https://codereview.chromium.org/317783010/diff/60001/device/hid/hid_connectio... device/hid/hid_connection.h:67: // empty buffer in such case. Sure. On 2014/06/30 15:19:46, Ken Rockot wrote: > "It filters protected reports and runs the callback." would be fine for the last > sentence. https://codereview.chromium.org/317783010/diff/60001/device/hid/hid_connectio... File device/hid/hid_connection_linux.cc (right): https://codereview.chromium.org/317783010/diff/60001/device/hid/hid_connectio... device/hid/hid_connection_linux.cc:223: CompleteRead(report.buffer, read.callback); That's true!.. On 2014/06/30 15:19:46, Ken Rockot wrote: > Let me propose one final significant change. What if CompleteRead returned a > boolean indicating whether or not it called the callback. > > Then in the case where a report is filtered, CompleteRead won't actually call > the callback at all; instead it will just return false, and the platform > implementation can requeue the pending read. > > This has the nice effect of API users not getting lots of empty reports, which I > think will be kind of annoying othwerwise (imagine monitoring a keyboard's > battery life, and constantly having to check if the HID report you received was > empty because the user keeps typing on the keyboard and generating filtered > input reports.) https://codereview.chromium.org/317783010/diff/60001/device/hid/hid_service_m... File device/hid/hid_service_mac.cc (right): https://codereview.chromium.org/317783010/diff/60001/device/hid/hid_service_m... device/hid/hid_service_mac.cc:121: const_cast<void*>(CFArrayGetValueAtIndex(collections, i))); Ken, sorry I don't catch correct way to do it : CFArrayGetValueAtIndex returns const void* ; direct static_cast<IOHIDElementRef> does not compile. See also other const_cast<void*> in this class that you did. On 2014/06/30 15:19:46, Ken Rockot wrote: > Also please get rid of the const_cast :)
https://codereview.chromium.org/317783010/diff/60001/device/hid/hid_service_m... File device/hid/hid_service_mac.cc (right): https://codereview.chromium.org/317783010/diff/60001/device/hid/hid_service_m... device/hid/hid_service_mac.cc:121: const_cast<void*>(CFArrayGetValueAtIndex(collections, i))); On 2014/06/30 15:38:39, jracle wrote: > Ken, sorry I don't catch correct way to do it : > CFArrayGetValueAtIndex returns const void* ; direct static_cast<IOHIDElementRef> > does not compile. > > See also other const_cast<void*> in this class that you did. > > On 2014/06/30 15:19:46, Ken Rockot wrote: > > Also please get rid of the const_cast :) > Sorry, can you not static_cast<const IOHIDElementRef>(CFArrayGetValueAtIndex...) ?
https://codereview.chromium.org/317783010/diff/60001/device/hid/hid_service_m... File device/hid/hid_service_mac.cc (right): https://codereview.chromium.org/317783010/diff/60001/device/hid/hid_service_m... device/hid/hid_service_mac.cc:121: const_cast<void*>(CFArrayGetValueAtIndex(collections, i))); On 2014/06/30 15:39:45, Ken Rockot wrote: > On 2014/06/30 15:38:39, jracle wrote: > > Ken, sorry I don't catch correct way to do it : > > CFArrayGetValueAtIndex returns const void* ; direct > static_cast<IOHIDElementRef> > > does not compile. > > > > See also other const_cast<void*> in this class that you did. > > > > On 2014/06/30 15:19:46, Ken Rockot wrote: > > > Also please get rid of the const_cast :) > > > > Sorry, can you not static_cast<const IOHIDElementRef>(CFArrayGetValueAtIndex...) > ? Also you're right, we are doing this elsewhere. That's unfortunate, since I don't think it's necessary (we should be using const IOHIDDeviceRefs everywhere.) Please try static_cast<const IOHIDElementRef>. If no luck, no worries.
https://codereview.chromium.org/317783010/diff/60001/device/hid/hid_service_m... File device/hid/hid_service_mac.cc (right): https://codereview.chromium.org/317783010/diff/60001/device/hid/hid_service_m... device/hid/hid_service_mac.cc:121: const_cast<void*>(CFArrayGetValueAtIndex(collections, i))); Nope.. ../../device/hid/hid_service_mac.cc:120:40: error: static_cast from 'const void *' to 'const IOHIDElementRef' (aka '__IOHIDElement *const') casts away qualifiers const IOHIDElementRef collection = static_cast<const IOHIDElementRef>( With : + const IOHIDElementRef collection = static_cast<const IOHIDElementRef>( + CFArrayGetValueAtIndex(collections, i)); See also line 40 in same file (think you did write this line :) On 2014/06/30 15:39:45, Ken Rockot wrote: > On 2014/06/30 15:38:39, jracle wrote: > > Ken, sorry I don't catch correct way to do it : > > CFArrayGetValueAtIndex returns const void* ; direct > static_cast<IOHIDElementRef> > > does not compile. > > > > See also other const_cast<void*> in this class that you did. > > > > On 2014/06/30 15:19:46, Ken Rockot wrote: > > > Also please get rid of the const_cast :) > > > > Sorry, can you not static_cast<const IOHIDElementRef>(CFArrayGetValueAtIndex...) > ?
Hi Ken, thanks for your almost-live comments. I've got to go to another meeting now, hence will tackle your proposed change asap (most probably tomorrow). In the meantime, 2 questions : - when are we supposed to land chrome.hid in stable branch (36?)? - are you considering also patch I sent for plug-unplug on windows (I can work on it this week)? Many thanks! -Julien https://codereview.chromium.org/317783010/diff/60001/device/hid/hid_service_m... File device/hid/hid_service_mac.cc (right): https://codereview.chromium.org/317783010/diff/60001/device/hid/hid_service_m... device/hid/hid_service_mac.cc:121: const_cast<void*>(CFArrayGetValueAtIndex(collections, i))); Got your message! Seems no luck indeed. Will do the modif. for CompleteRead tomorrow. On 2014/06/30 15:44:51, jracle wrote: > Nope.. > > ../../device/hid/hid_service_mac.cc:120:40: error: static_cast from 'const void > *' to 'const IOHIDElementRef' (aka '__IOHIDElement *const') casts away > qualifiers > const IOHIDElementRef collection = static_cast<const IOHIDElementRef>( > > With : > + const IOHIDElementRef collection = static_cast<const IOHIDElementRef>( > + CFArrayGetValueAtIndex(collections, i)); > > See also line 40 in same file (think you did write this line :) > > On 2014/06/30 15:39:45, Ken Rockot wrote: > > On 2014/06/30 15:38:39, jracle wrote: > > > Ken, sorry I don't catch correct way to do it : > > > CFArrayGetValueAtIndex returns const void* ; direct > > static_cast<IOHIDElementRef> > > > does not compile. > > > > > > See also other const_cast<void*> in this class that you did. > > > > > > On 2014/06/30 15:19:46, Ken Rockot wrote: > > > > Also please get rid of the const_cast :) > > > > > > > Sorry, can you not static_cast<const > IOHIDElementRef>(CFArrayGetValueAtIndex...) > > ? >
https://codereview.chromium.org/317783010/diff/60001/device/hid/hid_connectio... File device/hid/hid_connection_linux.cc (right): https://codereview.chromium.org/317783010/diff/60001/device/hid/hid_connectio... device/hid/hid_connection_linux.cc:223: CompleteRead(report.buffer, read.callback); We might also get rid of FilterInputReport helper, no? I'm a bit uncomfortable with it. On 2014/06/30 15:38:39, jracle wrote: > That's true!.. > > On 2014/06/30 15:19:46, Ken Rockot wrote: > > Let me propose one final significant change. What if CompleteRead returned a > > boolean indicating whether or not it called the callback. > > > > Then in the case where a report is filtered, CompleteRead won't actually call > > the callback at all; instead it will just return false, and the platform > > implementation can requeue the pending read. > > > > This has the nice effect of API users not getting lots of empty reports, which > I > > think will be kind of annoying othwerwise (imagine monitoring a keyboard's > > battery life, and constantly having to check if the HID report you received > was > > empty because the user keeps typing on the keyboard and generating filtered > > input reports.) >
New patch set. Plz review. Best, Julien
https://codereview.chromium.org/317783010/diff/80001/device/hid/hid_connectio... File device/hid/hid_connection_linux.cc (right): https://codereview.chromium.org/317783010/diff/80001/device/hid/hid_connectio... device/hid/hid_connection_linux.cc:214: pending_reads_.pop(); Don't pop() here. https://codereview.chromium.org/317783010/diff/80001/device/hid/hid_connectio... device/hid/hid_connection_linux.cc:218: read.callback.Run(false, report.buffer->size()); Actually, almost missed this. You want CompleteRead here, not calling the callback directly. https://codereview.chromium.org/317783010/diff/80001/device/hid/hid_connectio... device/hid/hid_connection_linux.cc:224: } Only pop() from pending_reads_ if your call to CompleteRead (in either branch) returned true. https://codereview.chromium.org/317783010/diff/80001/device/hid/hid_connectio... File device/hid/hid_connection_mac.cc (right): https://codereview.chromium.org/317783010/diff/80001/device/hid/hid_connectio... device/hid/hid_connection_mac.cc:154: PendingHidReport report = pending_reports_.front(); Same idea as the Linux notes here. Don't pop from pending_reads_ unless CompleteRead returns true, and use CompleteRead instead of calling the callback directly below. https://codereview.chromium.org/317783010/diff/80001/device/hid/hid_connectio... File device/hid/hid_connection_win.cc (right): https://codereview.chromium.org/317783010/diff/80001/device/hid/hid_connectio... device/hid/hid_connection_win.cc:229: DCHECK(transfer->receive_buffer_->data()[0] == An assert here is pretty aggressive since we can't control what real hardware might do. How about just testing and if there's an unexpected value, VLOG(1) it: VLOG(1) << "Unexpected report ID in HID report." and feel just discard the report (without completing a read). https://codereview.chromium.org/317783010/diff/80001/device/hid/hid_connectio... device/hid/hid_connection_win.cc:244: CompleteRead(transfer->target_buffer_, transfer->callback_); Boo. So the way this is structured (not having separate receive and callback queues like Linux and Mac have) makes it really awkward to behave like I'd hope. You can't really push the transfer request back into the front of the queue, because any previously issued ReadFile calls will still execute in-order, and each ReadFile is tied directly to a callback. This is where it would be nice to have what you were working on before, which is to unify the queuing across platform implementations. However I still think it's too much to cover for this CL, so let's accept that API consumers on Windows will have to deal with the occasional empty input report until this is fixed, as long as that can happen before 38 branches.
Also I take back part of my comment regarding unexpected report IDs - because we can't requeue a read request, don't discard the read in that case, just complete with a falure i.e. (0, false).
https://codereview.chromium.org/317783010/diff/80001/device/hid/hid_connectio... File device/hid/hid_connection_linux.cc (right): https://codereview.chromium.org/317783010/diff/80001/device/hid/hid_connectio... device/hid/hid_connection_linux.cc:214: pending_reads_.pop(); OK On 2014/07/01 14:55:23, Ken Rockot wrote: > Don't pop() here. https://codereview.chromium.org/317783010/diff/80001/device/hid/hid_connectio... device/hid/hid_connection_linux.cc:218: read.callback.Run(false, report.buffer->size()); Sorry, here I have trouble using read.buffer vs report.buffer. What should be the CompleteRead args to use then? Thanks On 2014/07/01 14:55:23, Ken Rockot wrote: > Actually, almost missed this. You want CompleteRead here, not calling the > callback directly. https://codereview.chromium.org/317783010/diff/80001/device/hid/hid_connectio... device/hid/hid_connection_linux.cc:224: } Sure On 2014/07/01 14:55:23, Ken Rockot wrote: > Only pop() from pending_reads_ if your call to CompleteRead (in either branch) > returned true. https://codereview.chromium.org/317783010/diff/80001/device/hid/hid_connectio... File device/hid/hid_connection_mac.cc (right): https://codereview.chromium.org/317783010/diff/80001/device/hid/hid_connectio... device/hid/hid_connection_mac.cc:154: PendingHidReport report = pending_reports_.front(); Yes On 2014/07/01 14:55:23, Ken Rockot wrote: > Same idea as the Linux notes here. Don't pop from pending_reads_ unless > CompleteRead returns true, and use CompleteRead instead of calling the callback > directly below. https://codereview.chromium.org/317783010/diff/80001/device/hid/hid_connectio... File device/hid/hid_connection_win.cc (right): https://codereview.chromium.org/317783010/diff/80001/device/hid/hid_connectio... device/hid/hid_connection_win.cc:229: DCHECK(transfer->receive_buffer_->data()[0] == Ok, Should I pass report ID as an argument? VLOG(..) << "Unexpected report ID in HID report." << report_id what is the correct syntax plz? On 2014/07/01 14:55:23, Ken Rockot wrote: > An assert here is pretty aggressive since we can't control what real hardware > might do. How about just testing and if there's an unexpected value, VLOG(1) it: > > VLOG(1) << "Unexpected report ID in HID report." > > and feel just discard the report (without completing a read). https://codereview.chromium.org/317783010/diff/80001/device/hid/hid_connectio... device/hid/hid_connection_win.cc:244: CompleteRead(transfer->target_buffer_, transfer->callback_); Indeed.. Do I have to handle CompleteRead return result? No I guess in that case, correct? On 2014/07/01 14:55:23, Ken Rockot wrote: > Boo. So the way this is structured (not having separate receive and callback > queues like Linux and Mac have) makes it really awkward to behave like I'd hope. > You can't really push the transfer request back into the front of the queue, > because any previously issued ReadFile calls will still execute in-order, and > each ReadFile is tied directly to a callback. > > This is where it would be nice to have what you were working on before, which is > to unify the queuing across platform implementations. > > However I still think it's too much to cover for this CL, so let's accept that > API consumers on Windows will have to deal with the occasional empty input > report until this is fixed, as long as that can happen before 38 branches.
https://codereview.chromium.org/317783010/diff/80001/device/hid/hid_connectio... File device/hid/hid_connection_linux.cc (right): https://codereview.chromium.org/317783010/diff/80001/device/hid/hid_connectio... device/hid/hid_connection_linux.cc:218: read.callback.Run(false, report.buffer->size()); On 2014/07/01 15:32:38, jracle wrote: > Sorry, here I have trouble using read.buffer vs report.buffer. > > What should be the CompleteRead args to use then? > > Thanks > On 2014/07/01 14:55:23, Ken Rockot wrote: > > Actually, almost missed this. You want CompleteRead here, not calling the > > callback directly. > Oh. Doh. Sorry, misunderstood the logic here. We do want the read to fail in this case. However it seems totally incorrect and confusing that we're sending a non-zero size. So please just do: read.callback.Run(false, 0) https://codereview.chromium.org/317783010/diff/80001/device/hid/hid_connectio... File device/hid/hid_connection_mac.cc (right): https://codereview.chromium.org/317783010/diff/80001/device/hid/hid_connectio... device/hid/hid_connection_mac.cc:154: PendingHidReport report = pending_reports_.front(); On 2014/07/01 15:32:38, jracle wrote: > Yes > > On 2014/07/01 14:55:23, Ken Rockot wrote: > > Same idea as the Linux notes here. Don't pop from pending_reads_ unless > > CompleteRead returns true, and use CompleteRead instead of calling the > callback > > directly below. > Same deal with my new Linux comment. Please change the failure callback to read.callback.Run(false, 0). https://codereview.chromium.org/317783010/diff/80001/device/hid/hid_connectio... File device/hid/hid_connection_win.cc (right): https://codereview.chromium.org/317783010/diff/80001/device/hid/hid_connectio... device/hid/hid_connection_win.cc:229: DCHECK(transfer->receive_buffer_->data()[0] == On 2014/07/01 15:32:38, jracle wrote: > Ok, Should I pass report ID as an argument? > > VLOG(..) << "Unexpected report ID in HID report." << report_id > > what is the correct syntax plz? > > On 2014/07/01 14:55:23, Ken Rockot wrote: > > An assert here is pretty aggressive since we can't control what real hardware > > might do. How about just testing and if there's an unexpected value, VLOG(1) > it: > > > > VLOG(1) << "Unexpected report ID in HID report." > > > > and feel just discard the report (without completing a read). > Doesn't much matter IMO. But yes if you want to include the ID in the log message, that's the right syntax. Would use different punctuation though, like: VLOG(1) << "Unexpected report ID in HID report: " << report_id; https://codereview.chromium.org/317783010/diff/80001/device/hid/hid_connectio... device/hid/hid_connection_win.cc:244: CompleteRead(transfer->target_buffer_, transfer->callback_); On 2014/07/01 15:32:38, jracle wrote: > Indeed.. > > Do I have to handle CompleteRead return result? No I guess in that case, > correct? > > On 2014/07/01 14:55:23, Ken Rockot wrote: > > Boo. So the way this is structured (not having separate receive and callback > > queues like Linux and Mac have) makes it really awkward to behave like I'd > hope. > > You can't really push the transfer request back into the front of the queue, > > because any previously issued ReadFile calls will still execute in-order, and > > each ReadFile is tied directly to a callback. > > > > This is where it would be nice to have what you were working on before, which > is > > to unify the queuing across platform implementations. > > > > However I still think it's too much to cover for this CL, so let's accept that > > API consumers on Windows will have to deal with the occasional empty input > > report until this is fixed, as long as that can happen before 38 branches. > Nope, not much you can do with it yet.
Thanks Ken, I'll have to do a break for the soccer game (Swiss team playing Argentina). Will resume later on or tomorrow :) sorry Cheers, Julien https://codereview.chromium.org/317783010/diff/80001/device/hid/hid_connectio... File device/hid/hid_connection_linux.cc (right): https://codereview.chromium.org/317783010/diff/80001/device/hid/hid_connectio... device/hid/hid_connection_linux.cc:218: read.callback.Run(false, report.buffer->size()); Indeed, and we need to pop pending_reads_ in that case, correct? On 2014/07/01 15:37:14, Ken Rockot wrote: > On 2014/07/01 15:32:38, jracle wrote: > > Sorry, here I have trouble using read.buffer vs report.buffer. > > > > What should be the CompleteRead args to use then? > > > > Thanks > > On 2014/07/01 14:55:23, Ken Rockot wrote: > > > Actually, almost missed this. You want CompleteRead here, not calling the > > > callback directly. > > > > Oh. Doh. Sorry, misunderstood the logic here. We do want the read to fail in > this case. However it seems totally incorrect and confusing that we're sending a > non-zero size. So please just do: > > read.callback.Run(false, 0) https://codereview.chromium.org/317783010/diff/80001/device/hid/hid_connectio... File device/hid/hid_connection_mac.cc (right): https://codereview.chromium.org/317783010/diff/80001/device/hid/hid_connectio... device/hid/hid_connection_mac.cc:154: PendingHidReport report = pending_reports_.front(); See my comment too.. On 2014/07/01 15:37:14, Ken Rockot wrote: > On 2014/07/01 15:32:38, jracle wrote: > > Yes > > > > On 2014/07/01 14:55:23, Ken Rockot wrote: > > > Same idea as the Linux notes here. Don't pop from pending_reads_ unless > > > CompleteRead returns true, and use CompleteRead instead of calling the > > callback > > > directly below. > > > > Same deal with my new Linux comment. Please change the failure callback to > read.callback.Run(false, 0). https://codereview.chromium.org/317783010/diff/80001/device/hid/hid_connectio... File device/hid/hid_connection_win.cc (right): https://codereview.chromium.org/317783010/diff/80001/device/hid/hid_connectio... device/hid/hid_connection_win.cc:229: DCHECK(transfer->receive_buffer_->data()[0] == OK thanks On 2014/07/01 15:37:14, Ken Rockot wrote: > On 2014/07/01 15:32:38, jracle wrote: > > Ok, Should I pass report ID as an argument? > > > > VLOG(..) << "Unexpected report ID in HID report." << report_id > > > > what is the correct syntax plz? > > > > On 2014/07/01 14:55:23, Ken Rockot wrote: > > > An assert here is pretty aggressive since we can't control what real > hardware > > > might do. How about just testing and if there's an unexpected value, VLOG(1) > > it: > > > > > > VLOG(1) << "Unexpected report ID in HID report." > > > > > > and feel just discard the report (without completing a read). > > > > Doesn't much matter IMO. But yes if you want to include the ID in the log > message, that's the right syntax. Would use different punctuation though, like: > > VLOG(1) << "Unexpected report ID in HID report: " << report_id; https://codereview.chromium.org/317783010/diff/80001/device/hid/hid_connectio... device/hid/hid_connection_win.cc:244: CompleteRead(transfer->target_buffer_, transfer->callback_); got it. On 2014/07/01 15:37:14, Ken Rockot wrote: > On 2014/07/01 15:32:38, jracle wrote: > > Indeed.. > > > > Do I have to handle CompleteRead return result? No I guess in that case, > > correct? > > > > On 2014/07/01 14:55:23, Ken Rockot wrote: > > > Boo. So the way this is structured (not having separate receive and callback > > > queues like Linux and Mac have) makes it really awkward to behave like I'd > > hope. > > > You can't really push the transfer request back into the front of the queue, > > > because any previously issued ReadFile calls will still execute in-order, > and > > > each ReadFile is tied directly to a callback. > > > > > > This is where it would be nice to have what you were working on before, > which > > is > > > to unify the queuing across platform implementations. > > > > > > However I still think it's too much to cover for this CL, so let's accept > that > > > API consumers on Windows will have to deal with the occasional empty input > > > report until this is fixed, as long as that can happen before 38 branches. > > > > > Nope, not much you can do with it yet.
See also my comments.. Le 1 juil. 2014 17:43, <jracle@logitech.com> a écrit : > Thanks Ken, > > I'll have to do a break for the soccer game (Swiss team playing > Argentina). > Will resume later on or tomorrow :) sorry > > Cheers, > Julien > > > https://codereview.chromium.org/317783010/diff/80001/ > device/hid/hid_connection_linux.cc > File device/hid/hid_connection_linux.cc (right): > > https://codereview.chromium.org/317783010/diff/80001/ > device/hid/hid_connection_linux.cc#newcode218 > device/hid/hid_connection_linux.cc:218: read.callback.Run(false, > report.buffer->size()); > Indeed, and we need to pop pending_reads_ in that case, correct? > > On 2014/07/01 15:37:14, Ken Rockot wrote: > >> On 2014/07/01 15:32:38, jracle wrote: >> > Sorry, here I have trouble using read.buffer vs report.buffer. >> > >> > What should be the CompleteRead args to use then? >> > >> > Thanks >> > On 2014/07/01 14:55:23, Ken Rockot wrote: >> > > Actually, almost missed this. You want CompleteRead here, not >> > calling the > >> > > callback directly. >> > >> > > Oh. Doh. Sorry, misunderstood the logic here. We do want the read to >> > fail in > >> this case. However it seems totally incorrect and confusing that we're >> > sending a > >> non-zero size. So please just do: >> > > read.callback.Run(false, 0) >> > > https://codereview.chromium.org/317783010/diff/80001/ > device/hid/hid_connection_mac.cc > File device/hid/hid_connection_mac.cc (right): > > https://codereview.chromium.org/317783010/diff/80001/ > device/hid/hid_connection_mac.cc#newcode154 > device/hid/hid_connection_mac.cc:154: PendingHidReport report = > pending_reports_.front(); > See my comment too.. > > On 2014/07/01 15:37:14, Ken Rockot wrote: > >> On 2014/07/01 15:32:38, jracle wrote: >> > Yes >> > >> > On 2014/07/01 14:55:23, Ken Rockot wrote: >> > > Same idea as the Linux notes here. Don't pop from pending_reads_ >> > unless > >> > > CompleteRead returns true, and use CompleteRead instead of calling >> > the > >> > callback >> > > directly below. >> > >> > > Same deal with my new Linux comment. Please change the failure >> > callback to > >> read.callback.Run(false, 0). >> > > https://codereview.chromium.org/317783010/diff/80001/ > device/hid/hid_connection_win.cc > File device/hid/hid_connection_win.cc (right): > > https://codereview.chromium.org/317783010/diff/80001/ > device/hid/hid_connection_win.cc#newcode229 > device/hid/hid_connection_win.cc:229: > DCHECK(transfer->receive_buffer_->data()[0] == > OK thanks > > On 2014/07/01 15:37:14, Ken Rockot wrote: > >> On 2014/07/01 15:32:38, jracle wrote: >> > Ok, Should I pass report ID as an argument? >> > >> > VLOG(..) << "Unexpected report ID in HID report." << report_id >> > >> > what is the correct syntax plz? >> > >> > On 2014/07/01 14:55:23, Ken Rockot wrote: >> > > An assert here is pretty aggressive since we can't control what >> > real > >> hardware >> > > might do. How about just testing and if there's an unexpected >> > value, VLOG(1) > >> > it: >> > > >> > > VLOG(1) << "Unexpected report ID in HID report." >> > > >> > > and feel just discard the report (without completing a read). >> > >> > > Doesn't much matter IMO. But yes if you want to include the ID in the >> > log > >> message, that's the right syntax. Would use different punctuation >> > though, like: > > VLOG(1) << "Unexpected report ID in HID report: " << report_id; >> > > https://codereview.chromium.org/317783010/diff/80001/ > device/hid/hid_connection_win.cc#newcode244 > device/hid/hid_connection_win.cc:244: > CompleteRead(transfer->target_buffer_, transfer->callback_); > got it. > > On 2014/07/01 15:37:14, Ken Rockot wrote: > >> On 2014/07/01 15:32:38, jracle wrote: >> > Indeed.. >> > >> > Do I have to handle CompleteRead return result? No I guess in that >> > case, > >> > correct? >> > >> > On 2014/07/01 14:55:23, Ken Rockot wrote: >> > > Boo. So the way this is structured (not having separate receive >> > and callback > >> > > queues like Linux and Mac have) makes it really awkward to behave >> > like I'd > >> > hope. >> > > You can't really push the transfer request back into the front of >> > the queue, > >> > > because any previously issued ReadFile calls will still execute >> > in-order, > >> and >> > > each ReadFile is tied directly to a callback. >> > > >> > > This is where it would be nice to have what you were working on >> > before, > >> which >> > is >> > > to unify the queuing across platform implementations. >> > > >> > > However I still think it's too much to cover for this CL, so let's >> > accept > >> that >> > > API consumers on Windows will have to deal with the occasional >> > empty input > >> > > report until this is fixed, as long as that can happen before 38 >> > branches. > >> > >> > > > Nope, not much you can do with it yet. >> > > https://codereview.chromium.org/317783010/ > To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
*drumroll please* LGTM!
On 2014/07/02 15:24:51, Ken Rockot wrote: > *drumroll please* > > LGTM! Champaign! thanks
The CQ bit was checked by jracle@logitech.com
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/jracle@logitech.com/317783010/100001
FYI, CQ is re-trying this CL (attempt #1). The failing builders are: chromium_presubmit on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/chromium_presubmit/bu...) linux_chromium_gn_rel on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_gn_rel...) win_chromium_x64_rel on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_x64_rel/...)
Doh. kalman could you look at the IDL?
The CQ bit was unchecked by rockot@chromium.org
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: chromium_presubmit on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/chromium_presubmit/bu...)
rslgtm, no idea what I'm looking at. though please take out the TODO in the CL description. https://codereview.chromium.org/317783010/diff/100001/chrome/common/extension... File chrome/common/extensions/api/hid.idl (right): https://codereview.chromium.org/317783010/diff/100001/chrome/common/extension... chrome/common/extensions/api/hid.idl:11: // |usage_page|: HID usage page identifier. s/usage_page/usagePage/
https://codereview.chromium.org/317783010/diff/100001/chrome/common/extension... File chrome/common/extensions/api/hid.idl (right): https://codereview.chromium.org/317783010/diff/100001/chrome/common/extension... chrome/common/extensions/api/hid.idl:11: // |usage_page|: HID usage page identifier. Oops, good catch! Thanks On 2014/07/02 20:50:35, kalman wrote: > s/usage_page/usagePage/
The CQ bit was checked by rockot@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/jracle@logitech.com/317783010/120001
Message was sent while issue was closed.
Change committed as 281133 |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
