Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
225513005:
chrome.hid : enrich device info with Top-Level collections usages (Closed)
|
Created:
6 years, 8 months ago by jracle (use Gerrit) Modified:
6 years, 8 months ago Reviewers:
Ken Rockot(use gerrit already) Base URL:
https://chromium.googlesource.com/chromium/src.git@master Visibility:
Public. |
Descriptionchrome.hid : enrich device info with Top-Level collections usages
- works on Windows and Mac.
- change ongoing for Chrome OS / Linux
-> will be part of separate commit
BUG=359560
TBR=rockot@chromium.org
TBR=scheib@chromium.org
Patch Set 1 #
Total comments: 1
Patch Set 2 : enrich device info with hid usages #
Total comments: 9
Patch Set 3 : formatted code (git cl format) #
Total comments: 46
Patch Set 4 : renaming 'parent_item' #
Total comments: 12
Patch Set 5 : refactoring + style aligment #
Total comments: 12
Patch Set 6 : move ostream operator << more DCHECKs #Patch Set 7 : add missing comment #Patch Set 8 : add cleanup TODO comment #Patch Set 9 : rebase wrt master changes WRONG #Patch Set 10 : rebasing wrt master changes #
Total comments: 4
Patch Set 11 : HidUsageAndPage construction #
Total comments: 2
Patch Set 12 : HidUsageAndPage::Page #
Messages
Total messages: 63 (0 generated)
I believe I'm here as an OWNER. jracl, typically add OWNERS only after reviews are obtained from first line reviewers. Ken, you likely should be an owner for HID now, right? Add yourself to c/b/e/api/hid c/c/e/api/file:hid at minimum or more likely to just the extensions/OWNERS+duplicates. The bug is missing details that from what I can tell would be relevant to share and explain in public on the bug, regarding why the issue exists and how it's being solved. Tests: Automated tests would be best - though I appreciate not always possible. If not feasible, please provide a working test app and attach to the bug with instructions so that anyone not familiar with HID specifics can run it and verify the fix. https://codereview.chromium.org/225513005/diff/1/device/hid/hid_device_info.h File device/hid/hid_device_info.h (right): https://codereview.chromium.org/225513005/diff/1/device/hid/hid_device_info.h... device/hid/hid_device_info.h:34: struct HidUsageAndPage { [optional] This would be cleaner if nested inside HidDeviceInfo.
Hi Mr Scheib, my bad, I missed how OWNERS were being handled. Indeed, the bug deserves more explanation, which I have made public already. Please see the updated description of the issue. BTW I'm fixing a couple of issues in chrome.hid, helping Ken with the experience with HID devices we have here at Logitech.We've shared a documents with the fixes (including the reason for this one) which are currently being done. I'll share it with you FYI. As regards test automation, I'll have trouble adding it since it requires real devices to be enumerated on both 3 platforms (Chrome OS, Windows, Mac). My test app is a JavaScript version of our Logitech Unifying Software, developed on the occasion of chrome.hid. It may at some point be shared with Google and with the public (which I cannot state without management approval). Many thanks for your feedback! Best Regards, Julien Racle On 2014/04/04 20:06:11, scheib wrote: > I believe I'm here as an OWNER. > > jracl, typically add OWNERS only after reviews are obtained from first line > reviewers. > > Ken, you likely should be an owner for HID now, right? Add yourself to > c/b/e/api/hid c/c/e/api/file:hid at minimum or more likely to just the > extensions/OWNERS+duplicates. > > The bug is missing details that from what I can tell would be relevant to share > and explain in public on the bug, regarding why the issue exists and how it's > being solved. > > Tests: Automated tests would be best - though I appreciate not always possible. > If not feasible, please provide a working test app and attach to the bug with > instructions so that anyone not familiar with HID specifics can run it and > verify the fix. > > https://codereview.chromium.org/225513005/diff/1/device/hid/hid_device_info.h > File device/hid/hid_device_info.h (right): > > https://codereview.chromium.org/225513005/diff/1/device/hid/hid_device_info.h... > device/hid/hid_device_info.h:34: struct HidUsageAndPage { > [optional] This would be cleaner if nested inside HidDeviceInfo.
Updated issue https://code.google.com/p/chromium/issues/detail?id=359560 with link to our (now publicly avaiable) doc
lgtm
Hi guys, I've submitted a new patch set which also targets Linux (hidraw). Best, Julien PS: I'll work on a test app in JS to show those changes work
Please notice you'll have to possibly add udev rules in order for chrome.hid to work on Linux/Chrome OS. See point 6 in Logitech's investigations : https://docs.google.com/document/d/1K4pGRVfwiwt19jo3SfoCqvvskNNE7H5c_Kz5xUoeDVs/
In general this looks OK to me, but there are dozens of formatting problems all over the place. Please run git cl format from your local branch and re-upload. If you have any trouble with this let me know and I can upload a new patchset with clang_format applied. I'll finish reviewing once this is formatted. https://codereview.chromium.org/225513005/diff/20001/device/hid/hid_report_de... File device/hid/hid_report_descriptor.cc (right): https://codereview.chromium.org/225513005/diff/20001/device/hid/hid_report_de... device/hid/hid_report_descriptor.cc:18: : bytes_(bytes) Please, commas at the end of lines not at the beginning. : bytes_(bytes), previous_(previous), parent_(NULL) { https://codereview.chromium.org/225513005/diff/20001/device/hid/hid_report_de... device/hid/hid_report_descriptor.cc:21: Please remove some of this extra vertical whitespace. The style guide discourages this. https://codereview.chromium.org/225513005/diff/20001/device/hid/hid_report_de... device/hid/hid_report_descriptor.cc:23: Whitespace https://codereview.chromium.org/225513005/diff/20001/device/hid/hid_report_de... device/hid/hid_report_descriptor.cc:25: Whitespace https://codereview.chromium.org/225513005/diff/20001/device/hid/hid_report_de... device/hid/hid_report_descriptor.cc:33: etc... https://codereview.chromium.org/225513005/diff/20001/device/hid/hid_report_de... device/hid/hid_report_descriptor.cc:53: if (parnt) Please do not use abbreviated names like this. parent_item would be much better than parnt. https://codereview.chromium.org/225513005/diff/20001/device/hid/hid_report_de... File device/hid/hid_report_descriptor_unittest.cc (right): https://codereview.chromium.org/225513005/diff/20001/device/hid/hid_report_de... device/hid/hid_report_descriptor_unittest.cc:348: nit: No need for vertical whitespace at the top of all these tests. https://codereview.chromium.org/225513005/diff/20001/device/hid/hid_service_m... File device/hid/hid_service_mac.cc (right): https://codereview.chromium.org/225513005/diff/20001/device/hid/hid_service_m... device/hid/hid_service_mac.cc:152: CFTypeRef deviceUsagePairsRaw = IOHIDDeviceGetProperty(hid_device, nit: Please align arguments. In this case you can move hid_device to the next line. hid_device, CFSTR(kIIOHID...)); is fine, as is: hid_device, CFSTR(kIOHID...)); https://codereview.chromium.org/225513005/diff/20001/device/hid/hid_service_m... device/hid/hid_service_mac.cc:157: for (CFIndex i=0; i<deviceUsagePairsCount; i++) { nit: Please format this code. Missing whitespace ("i=0", "i<foo"). Also prefix increment is preferred ("++i").
On 2014/04/15 17:53:37, Ken Rockot wrote: > In general this looks OK to me, but there are dozens of formatting problems all > over the place. Please run > > git cl format > > from your local branch and re-upload. If you have any trouble with this let me > know and I can upload a new patchset with clang_format applied. > > I'll finish reviewing once this is formatted. > > https://codereview.chromium.org/225513005/diff/20001/device/hid/hid_report_de... > File device/hid/hid_report_descriptor.cc (right): > > https://codereview.chromium.org/225513005/diff/20001/device/hid/hid_report_de... > device/hid/hid_report_descriptor.cc:18: : bytes_(bytes) > Please, commas at the end of lines not at the beginning. > > : bytes_(bytes), > previous_(previous), > parent_(NULL) { > > https://codereview.chromium.org/225513005/diff/20001/device/hid/hid_report_de... > device/hid/hid_report_descriptor.cc:21: > Please remove some of this extra vertical whitespace. The style guide > discourages this. > > https://codereview.chromium.org/225513005/diff/20001/device/hid/hid_report_de... > device/hid/hid_report_descriptor.cc:23: > Whitespace > > https://codereview.chromium.org/225513005/diff/20001/device/hid/hid_report_de... > device/hid/hid_report_descriptor.cc:25: > Whitespace > > https://codereview.chromium.org/225513005/diff/20001/device/hid/hid_report_de... > device/hid/hid_report_descriptor.cc:33: > etc... > > https://codereview.chromium.org/225513005/diff/20001/device/hid/hid_report_de... > device/hid/hid_report_descriptor.cc:53: if (parnt) > Please do not use abbreviated names like this. parent_item would be much better > than parnt. > > https://codereview.chromium.org/225513005/diff/20001/device/hid/hid_report_de... > File device/hid/hid_report_descriptor_unittest.cc (right): > > https://codereview.chromium.org/225513005/diff/20001/device/hid/hid_report_de... > device/hid/hid_report_descriptor_unittest.cc:348: > nit: No need for vertical whitespace at the top of all these tests. > > https://codereview.chromium.org/225513005/diff/20001/device/hid/hid_service_m... > File device/hid/hid_service_mac.cc (right): > > https://codereview.chromium.org/225513005/diff/20001/device/hid/hid_service_m... > device/hid/hid_service_mac.cc:152: CFTypeRef deviceUsagePairsRaw = > IOHIDDeviceGetProperty(hid_device, > nit: Please align arguments. In this case you can move hid_device to the next > line. > > hid_device, > CFSTR(kIIOHID...)); > > is fine, as is: > > hid_device, CFSTR(kIOHID...)); > > https://codereview.chromium.org/225513005/diff/20001/device/hid/hid_service_m... > device/hid/hid_service_mac.cc:157: for (CFIndex i=0; i<deviceUsagePairsCount; > i++) { > nit: Please format this code. Missing whitespace ("i=0", "i<foo"). Also prefix > increment is preferred ("++i"). Hi Ken, apologies for that, I'm new to Chromium's rules, and am discovering your guidelines (which make sense for sure), though I read couple of C++ do's/don't on chromium.org. I followed pre git-cl-upload script verifications and removed all warnings before uploading, so looks like I missed running the command you say. Sorry again. Currently I gonna give a lecture and will come back home in 2hours. If I get a chance to do it tonight, I'll remote connect to the office and will apply your changes. Thanks so much for checking. Julien
Ken, I managed to re-format the code with 'git cl format'. Let me know if it is fine, I'll log-in later on in the evening. Thanks again for your kind help, Julien
Hi Julien, Thanks for formatting. I have lots of comments! Please do not be discouraged :) It's a good patch, and I understand it takes a while to get used to chromium style guidelines. If you would like me to help directly to get this CL in shape, I can take some time to do that. Otherwise please do not hesitate to ask questions and send more pings for review! Cheers https://codereview.chromium.org/225513005/diff/40001/device/hid/hid_report_de... File device/hid/hid_report_descriptor.cc (right): https://codereview.chromium.org/225513005/diff/40001/device/hid/hid_report_de... device/hid/hid_report_descriptor.cc:19: Sorry - I guess the auto-format tool does not do this, but please ensure that at the very least, there is no vertical whitespace at the beginning of any block. The only exception is namespace blocks. So you should never see FooMethod() { foocode; This should always be rewritten as FooMethod() { foocode; https://codereview.chromium.org/225513005/diff/40001/device/hid/hid_report_de... device/hid/hid_report_descriptor.cc:51: if (parnt) Please rename parnt. As a general rule, always prefer longer or more specific names over shorter or abbreviated names. parent_item would work well in this case. https://codereview.chromium.org/225513005/diff/40001/device/hid/hid_report_de... File device/hid/hid_report_descriptor.h (right): https://codereview.chromium.org/225513005/diff/40001/device/hid/hid_report_de... device/hid/hid_report_descriptor.h:17: class HidReportDescriptorItem { I think this is large enough that it deserves its own header and cc files. https://codereview.chromium.org/225513005/diff/40001/device/hid/hid_report_de... device/hid/hid_report_descriptor.h:19: #pragma pack(push, 1) Please do not use packed structs to map onto a data buffer like this. Just parse/unpack the data into standalone fields in the HRDI class. A little more work up front, but much easier for any passerby or consumer to understand what's going on. https://codereview.chromium.org/225513005/diff/40001/device/hid/hid_report_de... device/hid/hid_report_descriptor.h:23: kTypeMain = 0, Please move Type into the next public block since none of the tag enums depend on it. The fewer private/public blocks, the better. https://codereview.chromium.org/225513005/diff/40001/device/hid/hid_report_de... device/hid/hid_report_descriptor.h:33: kMainTagDefault = 0x00, // 0000 As these are only used internally, please move them out of the header. They can live in an anonymous namespace. https://codereview.chromium.org/225513005/diff/40001/device/hid/hid_report_de... device/hid/hid_report_descriptor.h:107: friend std::ostream& operator<<(std::ostream& os, const Tag& tag) { Please do not inline this, and in fact it doesn't even need to live inside any class. It can be in an anonymous namespace in some implementation file. https://codereview.chromium.org/225513005/diff/40001/device/hid/hid_report_de... device/hid/hid_report_descriptor.h:207: struct Header { I'm jumping around this CL a lot as I get a better understanding of it, so please excuse any repetition. :) I would much rather see HidReportDescriptorItem do all of the parsing work up front and actually store its own (unpacked) fields from the result. There is no reason (as far as I can tell) that it should need to expose the detail of a long vs short header. It can just expose size, type, tag, yes? Then these packed structs can disappear altogether. https://codereview.chromium.org/225513005/diff/40001/device/hid/hid_report_de... device/hid/hid_report_descriptor.h:223: struct Default { Default what? A more descriptive name may be in order here. https://codereview.chromium.org/225513005/diff/40001/device/hid/hid_report_de... device/hid/hid_report_descriptor.h:225: friend std::ostream& operator<<(std::ostream& os, const Default& data) { See the comment in Input_Output_Feature about moving these ostream operators out of the structs altogether. I think it applies to every struct defined in this file. https://codereview.chromium.org/225513005/diff/40001/device/hid/hid_report_de... device/hid/hid_report_descriptor.h:230: struct Input_Output_Feature { InputOutputFeature, please https://codereview.chromium.org/225513005/diff/40001/device/hid/hid_report_de... device/hid/hid_report_descriptor.h:231: uint8_t data_or_constant : 1; Same as previous comments. I know it's a lot of fields, but please do not map this directly onto the buffer. I'm OK with the bit fields in this case, but an instance of this structure should just live inside of HRDI as a *copy* of whatever information was in the descriptor buffer at construction time. https://codereview.chromium.org/225513005/diff/40001/device/hid/hid_report_de... device/hid/hid_report_descriptor.h:244: const Input_Output_Feature& data) { Please do not inline this. In fact once again, this does not need to belong to the struct at all. Please define it in an anonymous namespace in an implementation file. https://codereview.chromium.org/225513005/diff/40001/device/hid/hid_report_de... device/hid/hid_report_descriptor.h:282: Whitespace https://codereview.chromium.org/225513005/diff/40001/device/hid/hid_report_de... device/hid/hid_report_descriptor.h:297: CollectionType collectionType() const { Please do not inline this. Same goes for the rest of these methods. Please move all of these implementations to a cc file. Also it's not a trivial accessor, so please rename GetCollectionType(). Actually, on second thought, don't store a uint8_t in Collection at all. Store a CollectionType. If you want to keep the value-to-type mapping logic isolated to Collection, define a static method, like: static CollectionType GetCollectionTypeFromValue(uint8_t value); and call that method wherever you're creating/initializing a Collection. If you do this then collectionType() becomes a trivial accessor. Also please note, camelCase is always wrong in Chrome code (it's only used in blink/WebKit and code which implements their interfaces). Please use lowercase + underscores for accessors. So in this case if you do make collectionType a trivial accessor, it should be renamed to collection_type(). Otherwise it should be GetCollectionType(). https://codereview.chromium.org/225513005/diff/40001/device/hid/hid_report_de... device/hid/hid_report_descriptor.h:393: struct LogicalMaximum { There's a lot of code duplication going on here and a lot of nested type definitions. Maybe you could simplify with something like struct Int32Data { int32_t value; } struct Uint32Data { uint32_t value; } of course in that case it becomes clear to me that there is no need for all these types. They don't add any value to the code, so why not just store them in Data as their underlying types. That is: struct Data { ... int32_t logical_minimum; int32_t logical_maximum; ... } Until there is actually a need for code to treat these as different data types (which there doesn't appear to be yet), it will be much cleaner to do it this way. https://codereview.chromium.org/225513005/diff/40001/device/hid/hid_report_de... device/hid/hid_report_descriptor.h:625: HidReportDescriptorItem* previous = NULL); Please, no default argument values. Please specify use two separate constructors: explicit HidReportDescriptorItem(const uint8_t* bytes); HidReportDescriptorItem(const uint8_t* bytes, HidReportDescriptorItem...) In fact I think you could just get rid of the default NULL value and force callers to specify NULL. Also, as was mentioned in other places, please do not just map structures on top of |bytes|, and in turn, please do not even retain a pointer to |bytes|. Parse the data from the buffer right away and forget about the buffer. And also please document after doing this that the constructor does not take ownership of the buffer. Finally, it looks like (appropriately) these are only constructed and destroyed by HidReportDescriptor. Please make the constructor and destructor private and make HidReportDescriptor a friend. https://codereview.chromium.org/225513005/diff/40001/device/hid/hid_report_de... device/hid/hid_report_descriptor.h:628: HidReportDescriptorItem* previous() const { Please make it clear that neither this item nor the caller owns the returned pointer, but that any HidReportDescriptorItem is always owned by the HidReportDescriptor that created it. This should be clarified for previous() next() and parent(). https://codereview.chromium.org/225513005/diff/40001/device/hid/hid_report_de... device/hid/hid_report_descriptor.h:637: size_t depth() const; GetDepth() please. Lowercase names are for trivial accessors only. https://codereview.chromium.org/225513005/diff/40001/device/hid/hid_report_de... device/hid/hid_report_descriptor.h:639: bool isLong() const; IsLong() please. https://codereview.chromium.org/225513005/diff/40001/device/hid/hid_report_de... device/hid/hid_report_descriptor.h:640: size_t headerSize() const; GetHeaderSize() please. https://codereview.chromium.org/225513005/diff/40001/device/hid/hid_report_de... device/hid/hid_report_descriptor.h:641: size_t payloadSize() const; GetPayloadSize() please. https://codereview.chromium.org/225513005/diff/40001/device/hid/hid_report_de... device/hid/hid_report_descriptor.h:642: size_t size() const; GetSize() Also because none of these are trivial accessors, please document each of them briefly here. Someone reading this code may not know what "IsLong?" is asking. They may not know what "GetDepth" is returning. https://codereview.chromium.org/225513005/diff/40001/device/hid/hid_report_de... device/hid/hid_report_descriptor.h:644: Header* header() const; This (and longHeader) make me extremely uncomfortable. I would much rather you add methods to access the underlying fields by value. https://codereview.chromium.org/225513005/diff/40001/device/hid/hid_report_de... device/hid/hid_report_descriptor.h:647: Tag tag() const; GetTag() please. https://codereview.chromium.org/225513005/diff/40001/device/hid/hid_report_de... device/hid/hid_report_descriptor.h:648: Data* data() const; Since Data is a POD type, and you can change it so an instance is actually owned by this class (rather than mapping the union on top of a raw buffer), you can just return a copy here. Please do so, and then data() can be a trivial inlined accessor. https://codereview.chromium.org/225513005/diff/40001/device/hid/hid_report_de... device/hid/hid_report_descriptor.h:652: const HidReportDescriptorItem& item) { Please see base/strings/stringprintf.h for the base::StringPrintf function. I think after all, this may be strongly preferred over using ostream operator overloads and stringstreams. https://codereview.chromium.org/225513005/diff/40001/device/hid/hid_report_de... device/hid/hid_report_descriptor.h:777: const std::vector<HidReportDescriptorItem*>& items() const; items is a trivial accessor; please feel free to inline it here. https://codereview.chromium.org/225513005/diff/40001/device/hid/hid_report_de... device/hid/hid_report_descriptor.h:778: std::vector<HidUsageAndPage> topLevelCollections() const; GetTopLevelCollections() please. This is not a trivial accessor. Also please consider having this populate a std::vector<HidUsageAndPage>* argument rather than returning a vector. https://codereview.chromium.org/225513005/diff/40001/device/hid/hid_report_de... device/hid/hid_report_descriptor.h:782: const HidReportDescriptor& descriptor) { Please do not inline this definition. In fact it doesn't need to belong to HidReportDescriptor or be a friend. It can use items() to access. Please move this to an anonymous namespace in the cc file. https://codereview.chromium.org/225513005/diff/40001/device/hid/hid_report_de... device/hid/hid_report_descriptor.h:798: std::vector<HidReportDescriptorItem*> items_; Please use linked_ptr here std::vector<linked_ptr<HidReportDescriptorItem> > items_ https://codereview.chromium.org/225513005/diff/60001/device/hid/hid_usage_and... File device/hid/hid_usage_and_page.h (right): https://codereview.chromium.org/225513005/diff/60001/device/hid/hid_usage_and... device/hid/hid_usage_and_page.h:16: struct HidUsageAndPage { Vertical whitespace https://codereview.chromium.org/225513005/diff/60001/device/hid/hid_usage_and... device/hid/hid_usage_and_page.h:56: HidUsageAndPage(uint16_t usage_page, uint16_t usage) This is confusing. Please just have one constructor. HidUsageAndPage(uint16_t usage, Page usage_page) Also note that the argument order is reversed to match the name of the struct. https://codereview.chromium.org/225513005/diff/60001/device/hid/hid_usage_and... device/hid/hid_usage_and_page.h:61: uint16_t usage_page; Why not store this as a Page? https://codereview.chromium.org/225513005/diff/60001/device/hid/hid_usage_and... device/hid/hid_usage_and_page.h:64: bool operator==(const HidUsageAndPage& other) const { Please do not overload operators (the ostream operators are an exception, and should be limited to the scope of a single implementation file anyway). In this case an IsEqual method would be fine. https://codereview.chromium.org/225513005/diff/60001/device/hid/hid_usage_and... device/hid/hid_usage_and_page.h:70: const HidUsageAndPage& usage_and_page) { Please do not inline this, and it doesn't need to belong to or be a friend of the struct. https://codereview.chromium.org/225513005/diff/60001/device/hid/hid_usage_and... device/hid/hid_usage_and_page.h:76: friend std::ostream& operator<<(std::ostream& os, const Page& usage_page) { Please do not inline this, and it doesn't need to belong to or be a friend of the struct.
Hi Ken, once again, thanks A LOT for your comments, and the time you spend reviewing this CL. You are more than welcome to do a lot of comments, which do not discourage me but on the contrary increase my knowledge of good coding. I don't want to take too much of your time, but I'll submit more patch sets for your review with pleasure. So tomorrow morning Swiss time (now it's 11PM I'll review your comments and will prepare a new CL). Thanks very much for your patience! Cheers, Julien PS: after we'll land the CL, there are a couple of things I need to submit, and I'll document them thoroughly for your best understanding.
Hi Ken, I replied to your comments, and submitted a new patch set as well, with some great deal of refactoring. Feel free to submit a new one with corrections if you get a chance! Thanks very much. -Julien https://codereview.chromium.org/225513005/diff/40001/device/hid/hid_report_de... File device/hid/hid_report_descriptor.cc (right): https://codereview.chromium.org/225513005/diff/40001/device/hid/hid_report_de... device/hid/hid_report_descriptor.cc:19: Oh sorry, got it now. https://codereview.chromium.org/225513005/diff/40001/device/hid/hid_report_de... device/hid/hid_report_descriptor.cc:51: if (parnt) Sorry, was done in separate CL. I violated my own practice there.. https://codereview.chromium.org/225513005/diff/40001/device/hid/hid_report_de... File device/hid/hid_report_descriptor.h (right): https://codereview.chromium.org/225513005/diff/40001/device/hid/hid_report_de... device/hid/hid_report_descriptor.h:17: class HidReportDescriptorItem { Sure, done. https://codereview.chromium.org/225513005/diff/40001/device/hid/hid_report_de... device/hid/hid_report_descriptor.h:19: #pragma pack(push, 1) Got your point, I will refactor in next patch set. https://codereview.chromium.org/225513005/diff/40001/device/hid/hid_report_de... device/hid/hid_report_descriptor.h:107: friend std::ostream& operator<<(std::ostream& os, const Tag& tag) { Done https://codereview.chromium.org/225513005/diff/40001/device/hid/hid_report_de... device/hid/hid_report_descriptor.h:207: struct Header { Got it, no point to show those details. Confusing. https://codereview.chromium.org/225513005/diff/40001/device/hid/hid_report_de... device/hid/hid_report_descriptor.h:223: struct Default { Oops. Not meaningful. https://codereview.chromium.org/225513005/diff/40001/device/hid/hid_report_de... device/hid/hid_report_descriptor.h:225: friend std::ostream& operator<<(std::ostream& os, const Default& data) { Indeed. https://codereview.chromium.org/225513005/diff/40001/device/hid/hid_report_de... device/hid/hid_report_descriptor.h:625: HidReportDescriptorItem* previous = NULL); Done https://codereview.chromium.org/225513005/diff/40001/device/hid/hid_report_de... device/hid/hid_report_descriptor.h:644: Header* header() const; Now private in next patch-set, and accessed by value. https://codereview.chromium.org/225513005/diff/40001/device/hid/hid_report_de... device/hid/hid_report_descriptor.h:648: Data* data() const; Data is actually known only for short items. It has a max size of uint32_t in that case. Hence GetShortData() in next patch set (I removed the union and my dumb structs) https://codereview.chromium.org/225513005/diff/40001/device/hid/hid_report_de... device/hid/hid_report_descriptor.h:652: const HidReportDescriptorItem& item) { This I didn't catch yet, can you handle it in a next patch-set please? (or give me more info on how to use it without creating 'ToString()' or 'Dump()' methods. https://codereview.chromium.org/225513005/diff/40001/device/hid/hid_report_de... device/hid/hid_report_descriptor.h:777: const std::vector<HidReportDescriptorItem*>& items() const; OK https://codereview.chromium.org/225513005/diff/40001/device/hid/hid_report_de... device/hid/hid_report_descriptor.h:778: std::vector<HidUsageAndPage> topLevelCollections() const; OK https://codereview.chromium.org/225513005/diff/40001/device/hid/hid_report_de... device/hid/hid_report_descriptor.h:798: std::vector<HidReportDescriptorItem*> items_; Indeed https://codereview.chromium.org/225513005/diff/60001/device/hid/hid_usage_and... File device/hid/hid_usage_and_page.h (right): https://codereview.chromium.org/225513005/diff/60001/device/hid/hid_usage_and... device/hid/hid_usage_and_page.h:16: struct HidUsageAndPage { Done https://codereview.chromium.org/225513005/diff/60001/device/hid/hid_usage_and... device/hid/hid_usage_and_page.h:56: HidUsageAndPage(uint16_t usage_page, uint16_t usage) Makes sense! https://codereview.chromium.org/225513005/diff/60001/device/hid/hid_usage_and... device/hid/hid_usage_and_page.h:61: uint16_t usage_page; Indeed (feared to get out-of-range values wrt page enum, but that shouldn't be the case) https://codereview.chromium.org/225513005/diff/60001/device/hid/hid_usage_and... device/hid/hid_usage_and_page.h:64: bool operator==(const HidUsageAndPage& other) const { This is needed for unit test assertions https://codereview.chromium.org/225513005/diff/60001/device/hid/hid_usage_and... device/hid/hid_usage_and_page.h:70: const HidUsageAndPage& usage_and_page) { OK https://codereview.chromium.org/225513005/diff/60001/device/hid/hid_usage_and... device/hid/hid_usage_and_page.h:76: friend std::ostream& operator<<(std::ostream& os, const Page& usage_page) { OK
Looking good :) The biggest comment I have is regarding all the ostream operator<< overloads. I'm not convinced you even need stringification of any kind for any of these types, especially if the only use case is for writing tests. Removing all of that would significantly reduce the size of this code. https://codereview.chromium.org/225513005/diff/20002/device/hid/hid_report_de... File device/hid/hid_report_descriptor.h (right): https://codereview.chromium.org/225513005/diff/20002/device/hid/hid_report_de... device/hid/hid_report_descriptor.h:37: const HidReportDescriptor& descriptor); Two concerns: 1. It is really not idiomatic in chromium code to use these operators. I should have stated this before (sorry), but please just implement a ToString() method on the type if you really need stringification. But see #2. 2. Do you really even need stringification? If this is only here to make writing tests easier, please remove it. It is much better to test individual bits of the structure than to stringify the whole thing and test the string output. https://codereview.chromium.org/225513005/diff/20002/device/hid/hid_report_de... File device/hid/hid_report_descriptor_item.cc (right): https://codereview.chromium.org/225513005/diff/20002/device/hid/hid_report_de... device/hid/hid_report_descriptor_item.cc:210: LongHeader* long_header = (LongHeader*)&bytes[0]; Please do not do this. You already have the short header and you only use the data size. You can delete LongHeader's definition and just set payload_size_ = bytes[1] here; use a comment to document why. https://codereview.chromium.org/225513005/diff/20002/device/hid/hid_report_de... device/hid/hid_report_descriptor_item.cc:213: payload_size_ = header->size; Please verify that payload_size_ <= sizeof(shortData_) https://codereview.chromium.org/225513005/diff/20002/device/hid/hid_report_de... device/hid/hid_report_descriptor_item.cc:217: if (previous) { Please at least DCHECK(!previous->next_) to clarify the assumption. https://codereview.chromium.org/225513005/diff/20002/device/hid/hid_report_de... File device/hid/hid_report_descriptor_item.h (right): https://codereview.chromium.org/225513005/diff/20002/device/hid/hid_report_de... device/hid/hid_report_descriptor_item.h:15: class HidReportDescriptorItem { Use a "private:" specifier please, just for clarity. https://codereview.chromium.org/225513005/diff/20002/device/hid/hid_report_de... device/hid/hid_report_descriptor_item.h:171: size_t payloadSize() const { return payload_size_; } payload_size() please.
https://codereview.chromium.org/225513005/diff/20002/device/hid/hid_report_de... File device/hid/hid_report_descriptor_item.cc (right): https://codereview.chromium.org/225513005/diff/20002/device/hid/hid_report_de... device/hid/hid_report_descriptor_item.cc:210: LongHeader* long_header = (LongHeader*)&bytes[0]; Sure, it will be equivalent in terms of clarity if I comment the code, agree. https://codereview.chromium.org/225513005/diff/20002/device/hid/hid_report_de... device/hid/hid_report_descriptor_item.cc:213: payload_size_ = header->size; oops, thanks. Don't wanna end-up to another a-la openssl issue! https://codereview.chromium.org/225513005/diff/20002/device/hid/hid_report_de... device/hid/hid_report_descriptor_item.cc:217: if (previous) { OK https://codereview.chromium.org/225513005/diff/20002/device/hid/hid_report_de... File device/hid/hid_report_descriptor_item.h (right): https://codereview.chromium.org/225513005/diff/20002/device/hid/hid_report_de... device/hid/hid_report_descriptor_item.h:15: class HidReportDescriptorItem { Got it https://codereview.chromium.org/225513005/diff/20002/device/hid/hid_report_de... device/hid/hid_report_descriptor_item.h:171: size_t payloadSize() const { return payload_size_; } Sorry OK
https://codereview.chromium.org/225513005/diff/20002/device/hid/hid_report_de... File device/hid/hid_report_descriptor.h (right): https://codereview.chromium.org/225513005/diff/20002/device/hid/hid_report_de... device/hid/hid_report_descriptor.h:37: const HidReportDescriptor& descriptor); Since it is not idiomatic, and I don't see a wide usage of ToString() nor usage out of my tests, I'm in favor of point 2. Well, it'll be point 2' ;), move operator<< into unit tests. Purpose is to maintain parsing of HID report descriptor, but only in tests. Looking at a parsed report descriptor is very common and useful practice IMO. I prefer to keep it, even if it does not directly concern our code. Indeed, I first looked at report descriptor before I did implement the code to retrieve top-level collection's usages. It was helpful to see report was correctly parsed.
On 2014/04/23 13:06:47, jracle wrote: > https://codereview.chromium.org/225513005/diff/20002/device/hid/hid_report_de... > File device/hid/hid_report_descriptor.h (right): > > https://codereview.chromium.org/225513005/diff/20002/device/hid/hid_report_de... > device/hid/hid_report_descriptor.h:37: const HidReportDescriptor& descriptor); > Since it is not idiomatic, and I don't see a wide usage of ToString() nor usage > out of my tests, I'm in favor of point 2. > > Well, it'll be point 2' ;), move operator<< into unit tests. Purpose is to > maintain parsing of HID report descriptor, but only in tests. Looking at a > parsed report descriptor is very common and useful practice IMO. I prefer to > keep it, even if it does not directly concern our code. Indeed, I first looked > at report descriptor before I did implement the code to retrieve top-level > collection's usages. It was helpful to see report was correctly parsed. I think there's a slight misunderstanding here. It is reasonable (and in fact good) to test the descriptor parsing functionality. What is NOT good, is testing it by this process: 1. Parse descriptor 2. Stringify parsed descriptor using arbitrary stringification that is never used anywhere else. 3. Compare string against expected string. What would be much better, and what I will actually be willing to approve is: 1. Parse descriptor 2. Test individual fields of descriptor for expected values. In other words, if I have struct Foo { int x; int y; } I do not want to test this by defining (pseudocode): Foo::Stringify() { return "X=" + x + ", Y=" + y; } TEST_EQ(foo.Stringify(), "X=5, Y=6") when it would be much clearer, simpler, and more direct to instead write: TEST_EQ(foo.x, 5) TEST_EQ(foo.y, 6)
On 2014/04/23 20:34:14, Ken Rockot wrote: > On 2014/04/23 13:06:47, jracle wrote: > > > https://codereview.chromium.org/225513005/diff/20002/device/hid/hid_report_de... > > File device/hid/hid_report_descriptor.h (right): > > > > > https://codereview.chromium.org/225513005/diff/20002/device/hid/hid_report_de... > > device/hid/hid_report_descriptor.h:37: const HidReportDescriptor& descriptor); > > Since it is not idiomatic, and I don't see a wide usage of ToString() nor > usage > > out of my tests, I'm in favor of point 2. > > > > Well, it'll be point 2' ;), move operator<< into unit tests. Purpose is to > > maintain parsing of HID report descriptor, but only in tests. Looking at a > > parsed report descriptor is very common and useful practice IMO. I prefer to > > keep it, even if it does not directly concern our code. Indeed, I first looked > > at report descriptor before I did implement the code to retrieve top-level > > collection's usages. It was helpful to see report was correctly parsed. > > I think there's a slight misunderstanding here. It is reasonable (and in fact > good) to test the descriptor parsing functionality. What is NOT good, is testing > it by this process: > > 1. Parse descriptor > 2. Stringify parsed descriptor using arbitrary stringification that is never > used anywhere else. > 3. Compare string against expected string. > > What would be much better, and what I will actually be willing to approve is: > > 1. Parse descriptor > 2. Test individual fields of descriptor for expected values. > > In other words, if I have > > struct Foo { > int x; > int y; > } > > I do not want to test this by defining (pseudocode): > > Foo::Stringify() { > return "X=" + x + ", Y=" + y; > } > > TEST_EQ(foo.Stringify(), "X=5, Y=6") > > when it would be much clearer, simpler, and more direct to instead write: > > TEST_EQ(foo.x, 5) > TEST_EQ(foo.y, 6) Actually, how about this: I really like this patch otherwise, so could you please just add a cleanup TODO comment to the test module for now? It is not worth holding this CL up over a testing strategy that is not ideal but still sufficient.
On 2014/04/23 20:38:55, Ken Rockot wrote: > On 2014/04/23 20:34:14, Ken Rockot wrote: > > On 2014/04/23 13:06:47, jracle wrote: > > > > > > https://codereview.chromium.org/225513005/diff/20002/device/hid/hid_report_de... > > > File device/hid/hid_report_descriptor.h (right): > > > > > > > > > https://codereview.chromium.org/225513005/diff/20002/device/hid/hid_report_de... > > > device/hid/hid_report_descriptor.h:37: const HidReportDescriptor& > descriptor); > > > Since it is not idiomatic, and I don't see a wide usage of ToString() nor > > usage > > > out of my tests, I'm in favor of point 2. > > > > > > Well, it'll be point 2' ;), move operator<< into unit tests. Purpose is to > > > maintain parsing of HID report descriptor, but only in tests. Looking at a > > > parsed report descriptor is very common and useful practice IMO. I prefer to > > > keep it, even if it does not directly concern our code. Indeed, I first > looked > > > at report descriptor before I did implement the code to retrieve top-level > > > collection's usages. It was helpful to see report was correctly parsed. > > > > I think there's a slight misunderstanding here. It is reasonable (and in fact > > good) to test the descriptor parsing functionality. What is NOT good, is > testing > > it by this process: > > > > 1. Parse descriptor > > 2. Stringify parsed descriptor using arbitrary stringification that is never > > used anywhere else. > > 3. Compare string against expected string. > > > > What would be much better, and what I will actually be willing to approve is: > > > > 1. Parse descriptor > > 2. Test individual fields of descriptor for expected values. > > > > In other words, if I have > > > > struct Foo { > > int x; > > int y; > > } > > > > I do not want to test this by defining (pseudocode): > > > > Foo::Stringify() { > > return "X=" + x + ", Y=" + y; > > } > > > > TEST_EQ(foo.Stringify(), "X=5, Y=6") > > > > when it would be much clearer, simpler, and more direct to instead write: > > > > TEST_EQ(foo.x, 5) > > TEST_EQ(foo.y, 6) > > Actually, how about this: I really like this patch otherwise, so could you > please just add a cleanup TODO comment to the test module for now? It is not > worth holding this CL up over a testing strategy that is not ideal but still > sufficient. Oh and pending the TODO, LGTM!
Ken, now I get it sorry. Anyway thanks very much for the mitigation, I just submitted a new patch with companion TODO comment (hope format + content is fine). I'll come with a few improvements, and will improve this test later on then. Many thanks for your help! Cheers, Julien
On 2014/04/23 21:13:14, jracle wrote: Thanks!
On 2014/04/23 21:13:14, jracle wrote: Thanks!
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/225513005/120001
The CQ bit was unchecked by commit-bot@chromium.org
Failed to apply patch for device/hid/hid_service_linux.cc:
While running patch -p1 --forward --force --no-backup-if-mismatch;
patching file device/hid/hid_service_linux.cc
Hunk #1 FAILED at 2.
Hunk #2 succeeded at 17 with fuzz 2 (offset 1 line).
Hunk #3 succeeded at 26 with fuzz 2 (offset -1 lines).
Hunk #4 FAILED at 85.
Hunk #5 succeeded at 102 (offset -82 lines).
Hunk #6 succeeded at 148 with fuzz 1 (offset -84 lines).
2 out of 6 hunks FAILED -- saving rejects to file
device/hid/hid_service_linux.cc.rej
Patch: device/hid/hid_service_linux.cc
Index: device/hid/hid_service_linux.cc
diff --git a/device/hid/hid_service_linux.cc b/device/hid/hid_service_linux.cc
index
04a3c160664468d68db60c74e28f4925e52ec63f..cce80fa1725e02670115bb9af468711248828c25
100644
--- a/device/hid/hid_service_linux.cc
+++ b/device/hid/hid_service_linux.cc
@@ -2,12 +2,14 @@
// Use of this source code is governed by a BSD-style license that can be
// found in the LICENSE file.
-#include <libudev.h>
+#include <linux/hidraw.h>
+#include <sys/ioctl.h>
+
#include <stdint.h>
#include <string>
-#include <vector>
+#include "base/files/file_path.h"
#include "base/logging.h"
#include "base/platform_file.h"
#include "base/stl_util.h"
@@ -16,6 +18,7 @@
#include "base/strings/string_split.h"
#include "device/hid/hid_connection_linux.h"
#include "device/hid/hid_device_info.h"
+#include "device/hid/hid_report_descriptor.h"
#include "device/hid/hid_service_linux.h"
namespace device {
@@ -26,7 +29,7 @@ const char kUdevName[] = "udev";
const char kUdevActionAdd[] = "add";
const char kUdevActionRemove[] = "remove";
const char kHIDSubSystem[] = "hid";
-
+const char kHidrawSubsystem[] = "hidraw";
const char kHIDID[] = "HID_ID";
const char kHIDName[] = "HID_NAME";
const char kHIDUnique[] = "HID_UNIQ";
@@ -84,8 +87,14 @@ scoped_refptr<HidConnection> HidServiceLinux::Connect(
ScopedUdevDevicePtr hid_device(
udev_device_new_from_syspath(udev_.get(),
device_info.device_id.c_str()));
+
if (hid_device) {
- return new HidConnectionLinux(device_info, hid_device.Pass());
+ std::string dev_node;
+ if (!FindHidrawDevNode(hid_device.get(), &dev_node)) {
+ LOG(ERROR) << "Cannot open HID device as hidraw device.";
+ return NULL;
+ }
+ return new HidConnectionLinux(device_info, dev_node);
}
return NULL;
}
@@ -183,6 +192,43 @@ void HidServiceLinux::PlatformAddDevice(udev_device*
device) {
if (str_property != NULL)
device_info.product_name = str_property;
+ std::string dev_node;
+ if (!FindHidrawDevNode(device, &dev_node)) {
+ LOG(ERROR) << "Cannot open HID device as hidraw device.";
+ return;
+ }
+
+ int flags = base::File::FLAG_OPEN | base::File::FLAG_READ;
+
+ base::File device_file(base::FilePath(dev_node), flags);
+ if (!device_file.IsValid()) {
+ LOG(ERROR) << device_file.error_details();
+ return;
+ }
+
+ int desc_size = 0;
+ int res = ioctl(device_file.GetPlatformFile(), HIDIOCGRDESCSIZE, &desc_size);
+ if (res < 0) {
+ LOG(ERROR) << "HIDIOCGRDESCSIZE failed.";
+ device_file.Close();
+ return;
+ }
+
+ hidraw_report_descriptor rpt_desc;
+ rpt_desc.size = desc_size;
+
+ res = ioctl(device_file.GetPlatformFile(), HIDIOCGRDESC, &rpt_desc);
+ if (res < 0) {
+ LOG(ERROR) << "HIDIOCGRDESC failed.";
+ device_file.Close();
+ return;
+ }
+
+ device_file.Close();
+
+ HidReportDescriptor report_descriptor(rpt_desc.value, rpt_desc.size);
+ report_descriptor.GetTopLevelCollections(&device_info.usages);
+
AddDevice(device_info);
}
@@ -194,4 +240,44 @@ void HidServiceLinux::PlatformRemoveDevice(udev_device*
raw_dev) {
RemoveDevice(device_path);
}
+bool HidServiceLinux::FindHidrawDevNode(udev_device* parent,
+ std::string* result) {
+ udev* udev = udev_device_get_udev(parent);
+ if (!udev) {
+ return false;
+ }
+ ScopedUdevEnumeratePtr enumerate(udev_enumerate_new(udev));
+ if (!enumerate) {
+ return false;
+ }
+ if (udev_enumerate_add_match_subsystem(enumerate.get(), kHidrawSubsystem)) {
+ return false;
+ }
+ if (udev_enumerate_scan_devices(enumerate.get())) {
+ return false;
+ }
+ std::string parent_path(udev_device_get_devpath(parent));
+ if (parent_path.length() == 0 || *parent_path.rbegin() != '/')
+ parent_path += '/';
+ udev_list_entry* devices = udev_enumerate_get_list_entry(enumerate.get());
+ for (udev_list_entry* i = devices; i != NULL;
+ i = udev_list_entry_get_next(i)) {
+ ScopedUdevDevicePtr hid_dev(
+ udev_device_new_from_syspath(udev, udev_list_entry_get_name(i)));
+ const char* raw_path = udev_device_get_devnode(hid_dev.get());
+ std::string device_path = udev_device_get_devpath(hid_dev.get());
+ if (raw_path &&
+ !device_path.compare(0, parent_path.length(), parent_path)) {
+ std::string sub_path = device_path.substr(parent_path.length());
+ if (sub_path.substr(0, sizeof(kHidrawSubsystem) - 1) ==
+ kHidrawSubsystem) {
+ *result = raw_path;
+ return true;
+ }
+ }
+ }
+
+ return false;
+}
+
} // namespace dev
Ah you may need to manually rebase onto tip and reupload On Wed, Apr 23, 2014 at 2:27 PM, <commit-bot@chromium.org> wrote: > Failed to apply patch for device/hid/hid_service_linux.cc: > While running patch -p1 --forward --force --no-backup-if-mismatch; > patching file device/hid/hid_service_linux.cc > Hunk #1 FAILED at 2. > Hunk #2 succeeded at 17 with fuzz 2 (offset 1 line). > Hunk #3 succeeded at 26 with fuzz 2 (offset -1 lines). > Hunk #4 FAILED at 85. > Hunk #5 succeeded at 102 (offset -82 lines). > Hunk #6 succeeded at 148 with fuzz 1 (offset -84 lines). > 2 out of 6 hunks FAILED -- saving rejects to file > device/hid/hid_service_linux.cc.rej > > Patch: device/hid/hid_service_linux.cc > Index: device/hid/hid_service_linux.cc > diff --git a/device/hid/hid_service_linux.cc b/device/hid/hid_service_ > linux.cc > index > 04a3c160664468d68db60c74e28f4925e52ec63f..cce80fa1725e02670115bb9af46871 > 1248828c25 > 100644 > --- a/device/hid/hid_service_linux.cc > +++ b/device/hid/hid_service_linux.cc > @@ -2,12 +2,14 @@ > // Use of this source code is governed by a BSD-style license that can be > // found in the LICENSE file. > > -#include <libudev.h> > +#include <linux/hidraw.h> > +#include <sys/ioctl.h> > + > #include <stdint.h> > > #include <string> > -#include <vector> > > +#include "base/files/file_path.h" > #include "base/logging.h" > #include "base/platform_file.h" > #include "base/stl_util.h" > @@ -16,6 +18,7 @@ > #include "base/strings/string_split.h" > #include "device/hid/hid_connection_linux.h" > #include "device/hid/hid_device_info.h" > +#include "device/hid/hid_report_descriptor.h" > #include "device/hid/hid_service_linux.h" > > namespace device { > @@ -26,7 +29,7 @@ const char kUdevName[] = "udev"; > const char kUdevActionAdd[] = "add"; > const char kUdevActionRemove[] = "remove"; > const char kHIDSubSystem[] = "hid"; > - > +const char kHidrawSubsystem[] = "hidraw"; > const char kHIDID[] = "HID_ID"; > const char kHIDName[] = "HID_NAME"; > const char kHIDUnique[] = "HID_UNIQ"; > @@ -84,8 +87,14 @@ scoped_refptr<HidConnection> HidServiceLinux::Connect( > > ScopedUdevDevicePtr hid_device( > udev_device_new_from_syspath(udev_.get(), > device_info.device_id.c_str())); > + > if (hid_device) { > - return new HidConnectionLinux(device_info, hid_device.Pass()); > + std::string dev_node; > + if (!FindHidrawDevNode(hid_device.get(), &dev_node)) { > + LOG(ERROR) << "Cannot open HID device as hidraw device."; > + return NULL; > + } > + return new HidConnectionLinux(device_info, dev_node); > } > return NULL; > } > @@ -183,6 +192,43 @@ void HidServiceLinux::PlatformAddDevice(udev_device* > device) { > if (str_property != NULL) > device_info.product_name = str_property; > > + std::string dev_node; > + if (!FindHidrawDevNode(device, &dev_node)) { > + LOG(ERROR) << "Cannot open HID device as hidraw device."; > + return; > + } > + > + int flags = base::File::FLAG_OPEN | base::File::FLAG_READ; > + > + base::File device_file(base::FilePath(dev_node), flags); > + if (!device_file.IsValid()) { > + LOG(ERROR) << device_file.error_details(); > + return; > + } > + > + int desc_size = 0; > + int res = ioctl(device_file.GetPlatformFile(), HIDIOCGRDESCSIZE, > &desc_size); > + if (res < 0) { > + LOG(ERROR) << "HIDIOCGRDESCSIZE failed."; > + device_file.Close(); > + return; > + } > + > + hidraw_report_descriptor rpt_desc; > + rpt_desc.size = desc_size; > + > + res = ioctl(device_file.GetPlatformFile(), HIDIOCGRDESC, &rpt_desc); > + if (res < 0) { > + LOG(ERROR) << "HIDIOCGRDESC failed."; > + device_file.Close(); > + return; > + } > + > + device_file.Close(); > + > + HidReportDescriptor report_descriptor(rpt_desc.value, rpt_desc.size); > + report_descriptor.GetTopLevelCollections(&device_info.usages); > + > AddDevice(device_info); > } > > @@ -194,4 +240,44 @@ void HidServiceLinux::PlatformRemoveDevice(udev_ > device* > raw_dev) { > RemoveDevice(device_path); > } > > +bool HidServiceLinux::FindHidrawDevNode(udev_device* parent, > + std::string* result) { > + udev* udev = udev_device_get_udev(parent); > + if (!udev) { > + return false; > + } > + ScopedUdevEnumeratePtr enumerate(udev_enumerate_new(udev)); > + if (!enumerate) { > + return false; > + } > + if (udev_enumerate_add_match_subsystem(enumerate.get(), > kHidrawSubsystem)) { > + return false; > + } > + if (udev_enumerate_scan_devices(enumerate.get())) { > + return false; > + } > + std::string parent_path(udev_device_get_devpath(parent)); > + if (parent_path.length() == 0 || *parent_path.rbegin() != '/') > + parent_path += '/'; > + udev_list_entry* devices = udev_enumerate_get_list_entry( > enumerate.get()); > + for (udev_list_entry* i = devices; i != NULL; > + i = udev_list_entry_get_next(i)) { > + ScopedUdevDevicePtr hid_dev( > + udev_device_new_from_syspath(udev, udev_list_entry_get_name(i))); > + const char* raw_path = udev_device_get_devnode(hid_dev.get()); > + std::string device_path = udev_device_get_devpath(hid_dev.get()); > + if (raw_path && > + !device_path.compare(0, parent_path.length(), parent_path)) { > + std::string sub_path = device_path.substr(parent_path.length()); > + if (sub_path.substr(0, sizeof(kHidrawSubsystem) - 1) == > + kHidrawSubsystem) { > + *result = raw_path; > + return true; > + } > + } > + } > + > + return false; > +} > + > } // namespace dev > > > https://codereview.chromium.org/225513005/ > To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
On 2014/04/23 21:29:15, Ken Rockot wrote:
> Ah you may need to manually rebase onto tip and reupload
>
>
> On Wed, Apr 23, 2014 at 2:27 PM, <mailto:commit-bot@chromium.org> wrote:
>
> > Failed to apply patch for device/hid/hid_service_linux.cc:
> > While running patch -p1 --forward --force --no-backup-if-mismatch;
> > patching file device/hid/hid_service_linux.cc
> > Hunk #1 FAILED at 2.
> > Hunk #2 succeeded at 17 with fuzz 2 (offset 1 line).
> > Hunk #3 succeeded at 26 with fuzz 2 (offset -1 lines).
> > Hunk #4 FAILED at 85.
> > Hunk #5 succeeded at 102 (offset -82 lines).
> > Hunk #6 succeeded at 148 with fuzz 1 (offset -84 lines).
> > 2 out of 6 hunks FAILED -- saving rejects to file
> > device/hid/hid_service_linux.cc.rej
> >
> > Patch: device/hid/hid_service_linux.cc
> > Index: device/hid/hid_service_linux.cc
> > diff --git a/device/hid/hid_service_linux.cc b/device/hid/hid_service_
> > linux.cc
> > index
> > 04a3c160664468d68db60c74e28f4925e52ec63f..cce80fa1725e02670115bb9af46871
> > 1248828c25
> > 100644
> > --- a/device/hid/hid_service_linux.cc
> > +++ b/device/hid/hid_service_linux.cc
> > @@ -2,12 +2,14 @@
> > // Use of this source code is governed by a BSD-style license that can be
> > // found in the LICENSE file.
> >
> > -#include <libudev.h>
> > +#include <linux/hidraw.h>
> > +#include <sys/ioctl.h>
> > +
> > #include <stdint.h>
> >
> > #include <string>
> > -#include <vector>
> >
> > +#include "base/files/file_path.h"
> > #include "base/logging.h"
> > #include "base/platform_file.h"
> > #include "base/stl_util.h"
> > @@ -16,6 +18,7 @@
> > #include "base/strings/string_split.h"
> > #include "device/hid/hid_connection_linux.h"
> > #include "device/hid/hid_device_info.h"
> > +#include "device/hid/hid_report_descriptor.h"
> > #include "device/hid/hid_service_linux.h"
> >
> > namespace device {
> > @@ -26,7 +29,7 @@ const char kUdevName[] = "udev";
> > const char kUdevActionAdd[] = "add";
> > const char kUdevActionRemove[] = "remove";
> > const char kHIDSubSystem[] = "hid";
> > -
> > +const char kHidrawSubsystem[] = "hidraw";
> > const char kHIDID[] = "HID_ID";
> > const char kHIDName[] = "HID_NAME";
> > const char kHIDUnique[] = "HID_UNIQ";
> > @@ -84,8 +87,14 @@ scoped_refptr<HidConnection> HidServiceLinux::Connect(
> >
> > ScopedUdevDevicePtr hid_device(
> > udev_device_new_from_syspath(udev_.get(),
> > device_info.device_id.c_str()));
> > +
> > if (hid_device) {
> > - return new HidConnectionLinux(device_info, hid_device.Pass());
> > + std::string dev_node;
> > + if (!FindHidrawDevNode(hid_device.get(), &dev_node)) {
> > + LOG(ERROR) << "Cannot open HID device as hidraw device.";
> > + return NULL;
> > + }
> > + return new HidConnectionLinux(device_info, dev_node);
> > }
> > return NULL;
> > }
> > @@ -183,6 +192,43 @@ void HidServiceLinux::PlatformAddDevice(udev_device*
> > device) {
> > if (str_property != NULL)
> > device_info.product_name = str_property;
> >
> > + std::string dev_node;
> > + if (!FindHidrawDevNode(device, &dev_node)) {
> > + LOG(ERROR) << "Cannot open HID device as hidraw device.";
> > + return;
> > + }
> > +
> > + int flags = base::File::FLAG_OPEN | base::File::FLAG_READ;
> > +
> > + base::File device_file(base::FilePath(dev_node), flags);
> > + if (!device_file.IsValid()) {
> > + LOG(ERROR) << device_file.error_details();
> > + return;
> > + }
> > +
> > + int desc_size = 0;
> > + int res = ioctl(device_file.GetPlatformFile(), HIDIOCGRDESCSIZE,
> > &desc_size);
> > + if (res < 0) {
> > + LOG(ERROR) << "HIDIOCGRDESCSIZE failed.";
> > + device_file.Close();
> > + return;
> > + }
> > +
> > + hidraw_report_descriptor rpt_desc;
> > + rpt_desc.size = desc_size;
> > +
> > + res = ioctl(device_file.GetPlatformFile(), HIDIOCGRDESC, &rpt_desc);
> > + if (res < 0) {
> > + LOG(ERROR) << "HIDIOCGRDESC failed.";
> > + device_file.Close();
> > + return;
> > + }
> > +
> > + device_file.Close();
> > +
> > + HidReportDescriptor report_descriptor(rpt_desc.value, rpt_desc.size);
> > + report_descriptor.GetTopLevelCollections(&device_info.usages);
> > +
> > AddDevice(device_info);
> > }
> >
> > @@ -194,4 +240,44 @@ void HidServiceLinux::PlatformRemoveDevice(udev_
> > device*
> > raw_dev) {
> > RemoveDevice(device_path);
> > }
> >
> > +bool HidServiceLinux::FindHidrawDevNode(udev_device* parent,
> > + std::string* result) {
> > + udev* udev = udev_device_get_udev(parent);
> > + if (!udev) {
> > + return false;
> > + }
> > + ScopedUdevEnumeratePtr enumerate(udev_enumerate_new(udev));
> > + if (!enumerate) {
> > + return false;
> > + }
> > + if (udev_enumerate_add_match_subsystem(enumerate.get(),
> > kHidrawSubsystem)) {
> > + return false;
> > + }
> > + if (udev_enumerate_scan_devices(enumerate.get())) {
> > + return false;
> > + }
> > + std::string parent_path(udev_device_get_devpath(parent));
> > + if (parent_path.length() == 0 || *parent_path.rbegin() != '/')
> > + parent_path += '/';
> > + udev_list_entry* devices = udev_enumerate_get_list_entry(
> > enumerate.get());
> > + for (udev_list_entry* i = devices; i != NULL;
> > + i = udev_list_entry_get_next(i)) {
> > + ScopedUdevDevicePtr hid_dev(
> > + udev_device_new_from_syspath(udev, udev_list_entry_get_name(i)));
> > + const char* raw_path = udev_device_get_devnode(hid_dev.get());
> > + std::string device_path = udev_device_get_devpath(hid_dev.get());
> > + if (raw_path &&
> > + !device_path.compare(0, parent_path.length(), parent_path)) {
> > + std::string sub_path = device_path.substr(parent_path.length());
> > + if (sub_path.substr(0, sizeof(kHidrawSubsystem) - 1) ==
> > + kHidrawSubsystem) {
> > + *result = raw_path;
> > + return true;
> > + }
> > + }
> > + }
> > +
> > + return false;
> > +}
> > +
> > } // namespace dev
> >
> >
> > https://codereview.chromium.org/225513005/
> >
>
> To unsubscribe from this group and stop receiving emails from it, send an
email
> to mailto:chromium-reviews+unsubscribe@chromium.org.
FYI I am rebasing this patch for commit
On 2014/04/23 23:19:15, Ken Rockot wrote:
> On 2014/04/23 21:29:15, Ken Rockot wrote:
> > Ah you may need to manually rebase onto tip and reupload
> >
> >
> > On Wed, Apr 23, 2014 at 2:27 PM, <mailto:commit-bot@chromium.org> wrote:
> >
> > > Failed to apply patch for device/hid/hid_service_linux.cc:
> > > While running patch -p1 --forward --force --no-backup-if-mismatch;
> > > patching file device/hid/hid_service_linux.cc
> > > Hunk #1 FAILED at 2.
> > > Hunk #2 succeeded at 17 with fuzz 2 (offset 1 line).
> > > Hunk #3 succeeded at 26 with fuzz 2 (offset -1 lines).
> > > Hunk #4 FAILED at 85.
> > > Hunk #5 succeeded at 102 (offset -82 lines).
> > > Hunk #6 succeeded at 148 with fuzz 1 (offset -84 lines).
> > > 2 out of 6 hunks FAILED -- saving rejects to file
> > > device/hid/hid_service_linux.cc.rej
> > >
> > > Patch: device/hid/hid_service_linux.cc
> > > Index: device/hid/hid_service_linux.cc
> > > diff --git a/device/hid/hid_service_linux.cc b/device/hid/hid_service_
> > > linux.cc
> > > index
> > > 04a3c160664468d68db60c74e28f4925e52ec63f..cce80fa1725e02670115bb9af46871
> > > 1248828c25
> > > 100644
> > > --- a/device/hid/hid_service_linux.cc
> > > +++ b/device/hid/hid_service_linux.cc
> > > @@ -2,12 +2,14 @@
> > > // Use of this source code is governed by a BSD-style license that can be
> > > // found in the LICENSE file.
> > >
> > > -#include <libudev.h>
> > > +#include <linux/hidraw.h>
> > > +#include <sys/ioctl.h>
> > > +
> > > #include <stdint.h>
> > >
> > > #include <string>
> > > -#include <vector>
> > >
> > > +#include "base/files/file_path.h"
> > > #include "base/logging.h"
> > > #include "base/platform_file.h"
> > > #include "base/stl_util.h"
> > > @@ -16,6 +18,7 @@
> > > #include "base/strings/string_split.h"
> > > #include "device/hid/hid_connection_linux.h"
> > > #include "device/hid/hid_device_info.h"
> > > +#include "device/hid/hid_report_descriptor.h"
> > > #include "device/hid/hid_service_linux.h"
> > >
> > > namespace device {
> > > @@ -26,7 +29,7 @@ const char kUdevName[] = "udev";
> > > const char kUdevActionAdd[] = "add";
> > > const char kUdevActionRemove[] = "remove";
> > > const char kHIDSubSystem[] = "hid";
> > > -
> > > +const char kHidrawSubsystem[] = "hidraw";
> > > const char kHIDID[] = "HID_ID";
> > > const char kHIDName[] = "HID_NAME";
> > > const char kHIDUnique[] = "HID_UNIQ";
> > > @@ -84,8 +87,14 @@ scoped_refptr<HidConnection> HidServiceLinux::Connect(
> > >
> > > ScopedUdevDevicePtr hid_device(
> > > udev_device_new_from_syspath(udev_.get(),
> > > device_info.device_id.c_str()));
> > > +
> > > if (hid_device) {
> > > - return new HidConnectionLinux(device_info, hid_device.Pass());
> > > + std::string dev_node;
> > > + if (!FindHidrawDevNode(hid_device.get(), &dev_node)) {
> > > + LOG(ERROR) << "Cannot open HID device as hidraw device.";
> > > + return NULL;
> > > + }
> > > + return new HidConnectionLinux(device_info, dev_node);
> > > }
> > > return NULL;
> > > }
> > > @@ -183,6 +192,43 @@ void HidServiceLinux::PlatformAddDevice(udev_device*
> > > device) {
> > > if (str_property != NULL)
> > > device_info.product_name = str_property;
> > >
> > > + std::string dev_node;
> > > + if (!FindHidrawDevNode(device, &dev_node)) {
> > > + LOG(ERROR) << "Cannot open HID device as hidraw device.";
> > > + return;
> > > + }
> > > +
> > > + int flags = base::File::FLAG_OPEN | base::File::FLAG_READ;
> > > +
> > > + base::File device_file(base::FilePath(dev_node), flags);
> > > + if (!device_file.IsValid()) {
> > > + LOG(ERROR) << device_file.error_details();
> > > + return;
> > > + }
> > > +
> > > + int desc_size = 0;
> > > + int res = ioctl(device_file.GetPlatformFile(), HIDIOCGRDESCSIZE,
> > > &desc_size);
> > > + if (res < 0) {
> > > + LOG(ERROR) << "HIDIOCGRDESCSIZE failed.";
> > > + device_file.Close();
> > > + return;
> > > + }
> > > +
> > > + hidraw_report_descriptor rpt_desc;
> > > + rpt_desc.size = desc_size;
> > > +
> > > + res = ioctl(device_file.GetPlatformFile(), HIDIOCGRDESC, &rpt_desc);
> > > + if (res < 0) {
> > > + LOG(ERROR) << "HIDIOCGRDESC failed.";
> > > + device_file.Close();
> > > + return;
> > > + }
> > > +
> > > + device_file.Close();
> > > +
> > > + HidReportDescriptor report_descriptor(rpt_desc.value, rpt_desc.size);
> > > + report_descriptor.GetTopLevelCollections(&device_info.usages);
> > > +
> > > AddDevice(device_info);
> > > }
> > >
> > > @@ -194,4 +240,44 @@ void HidServiceLinux::PlatformRemoveDevice(udev_
> > > device*
> > > raw_dev) {
> > > RemoveDevice(device_path);
> > > }
> > >
> > > +bool HidServiceLinux::FindHidrawDevNode(udev_device* parent,
> > > + std::string* result) {
> > > + udev* udev = udev_device_get_udev(parent);
> > > + if (!udev) {
> > > + return false;
> > > + }
> > > + ScopedUdevEnumeratePtr enumerate(udev_enumerate_new(udev));
> > > + if (!enumerate) {
> > > + return false;
> > > + }
> > > + if (udev_enumerate_add_match_subsystem(enumerate.get(),
> > > kHidrawSubsystem)) {
> > > + return false;
> > > + }
> > > + if (udev_enumerate_scan_devices(enumerate.get())) {
> > > + return false;
> > > + }
> > > + std::string parent_path(udev_device_get_devpath(parent));
> > > + if (parent_path.length() == 0 || *parent_path.rbegin() != '/')
> > > + parent_path += '/';
> > > + udev_list_entry* devices = udev_enumerate_get_list_entry(
> > > enumerate.get());
> > > + for (udev_list_entry* i = devices; i != NULL;
> > > + i = udev_list_entry_get_next(i)) {
> > > + ScopedUdevDevicePtr hid_dev(
> > > + udev_device_new_from_syspath(udev, udev_list_entry_get_name(i)));
> > > + const char* raw_path = udev_device_get_devnode(hid_dev.get());
> > > + std::string device_path = udev_device_get_devpath(hid_dev.get());
> > > + if (raw_path &&
> > > + !device_path.compare(0, parent_path.length(), parent_path)) {
> > > + std::string sub_path = device_path.substr(parent_path.length());
> > > + if (sub_path.substr(0, sizeof(kHidrawSubsystem) - 1) ==
> > > + kHidrawSubsystem) {
> > > + *result = raw_path;
> > > + return true;
> > > + }
> > > + }
> > > + }
> > > +
> > > + return false;
> > > +}
> > > +
> > > } // namespace dev
> > >
> > >
> > > https://codereview.chromium.org/225513005/
> > >
> >
> > To unsubscribe from this group and stop receiving emails from it, send an
> email
> > to mailto:chromium-reviews+unsubscribe@chromium.org.
>
> FYI I am rebasing this patch for commit
Just kidding. Apparently I can't do that any more. You'll have to rebase this on
your own.
Specifically, note that HidServiceLinux delegates work to HidDeviceMonitor now.
Should be pretty straightforward.
Ken, I rebased my local branch on top of master, creating a new one.. Ended-up with a new CL : https://codereview.chromium.org/250523002/ Best, Julien
Attempted rebase..
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/225513005/140001
The CQ bit was unchecked by jracle@logitech.com
Ken, patch set 9 is incorrect, it does not contain last changes. See for instance operator<<, issue is there. I gonna attempt again rebase from my old branch in a moment. Sorry for that.
On 2014/04/24 15:13:20, jracle wrote: Ok. Sorry about this. I don't know what's going wrong, but let's not waste time on it. Moving over to your new CL...
Sorry Ken, my fault, forget about new CL, forgot to rebase --continue and aborted rebase. I'm re-attempting rebase at the moment, will need some time.
Ah OK. I was just about to say, even the new CL isn't correct. :) Thanks. (I forget to rebase --continue regularly. No worries) On Thu, Apr 24, 2014 at 8:23 AM, <jracle@logitech.com> wrote: > Sorry Ken, my fault, forget about new CL, forgot to rebase --continue and > aborted rebase. > I'm re-attempting rebase at the moment, will need some time. > > https://codereview.chromium.org/225513005/ > To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
On 2014/04/24 16:41:43, jracle wrote: Great! LGTM
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/225513005/160001
https://codereview.chromium.org/225513005/diff/160001/device/hid/hid_service_... File device/hid/hid_service_mac.cc (right): https://codereview.chromium.org/225513005/diff/160001/device/hid/hid_service_... device/hid/hid_service_mac.cc:168: device_info.usages.push_back(usage_and_page); Please just read values into local temporaries and construct the temporary HUAP at the push_back callsite: uint16_t usage; UsagePage page; .. .. foo(bar, baz, &usage); .. foo(bar, baz, &page); .. device_info.usages.push_back(HidUsageAndPage(usage, page)); https://codereview.chromium.org/225513005/diff/160001/device/hid/hid_service_... File device/hid/hid_service_win.cc (right): https://codereview.chromium.org/225513005/diff/160001/device/hid/hid_service_... device/hid/hid_service_win.cc:181: HidUsageAndPage usage_and_page; Actually please just rewrite this all in one line: device_info.usages.push_back(HidUsageAndPage( capabilities.Usage, capabilities.UsagePage));
https://codereview.chromium.org/225513005/diff/160001/device/hid/hid_service_... File device/hid/hid_service_mac.cc (right): https://codereview.chromium.org/225513005/diff/160001/device/hid/hid_service_... device/hid/hid_service_mac.cc:168: device_info.usages.push_back(usage_and_page); OK, makes more sense. Thanks for noticing it! https://codereview.chromium.org/225513005/diff/160001/device/hid/hid_service_... File device/hid/hid_service_win.cc (right): https://codereview.chromium.org/225513005/diff/160001/device/hid/hid_service_... device/hid/hid_service_win.cc:181: HidUsageAndPage usage_and_page; Sure
https://codereview.chromium.org/225513005/diff/170001/device/hid/hid_service_... File device/hid/hid_service_mac.cc (right): https://codereview.chromium.org/225513005/diff/170001/device/hid/hid_service_... device/hid/hid_service_mac.cc:166: UsagePage page; Gah. Sorry. I should have been more careful about what I wrote here. UsagePage isn't a type. HidUsageAndPage::Page is the type.
Wish I could rebuild.. retrying https://codereview.chromium.org/225513005/diff/170001/device/hid/hid_service_... File device/hid/hid_service_mac.cc (right): https://codereview.chromium.org/225513005/diff/170001/device/hid/hid_service_... device/hid/hid_service_mac.cc:166: UsagePage page; Gasp, wish I could rebuild. Let's retry, if we reach 15 I owe you a beer!
The CQ bit was checked by rockot@chromium.org
On 2014/04/24 19:47:00, jracle wrote: > Wish I could rebuild.. retrying > > https://codereview.chromium.org/225513005/diff/170001/device/hid/hid_service_... > File device/hid/hid_service_mac.cc (right): > > https://codereview.chromium.org/225513005/diff/170001/device/hid/hid_service_... > device/hid/hid_service_mac.cc:166: UsagePage page; > Gasp, wish I could rebuild. > Let's retry, if we reach 15 I owe you a beer! Woohoo! Free beer! I hope we're good now. If we do actually reach 15 I may go directly to whiskey.
Try jobs failed on following builders: tryserver.chromium on mac_chromium_compile_dbg tryserver.chromium on mac_chromium_rel tryserver.chromium on win_chromium_compile_dbg tryserver.chromium on win_chromium_rel tryserver.chromium on win_chromium_x64_rel
The CQ bit was unchecked by rockot@chromium.org
The CQ bit was checked by rockot@chromium.org
The CQ bit was unchecked by rockot@chromium.org
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/225513005/190001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: tryserver.chromium on win_chromium_compile_dbg
On 2014/04/24 23:05:07, I haz the power (commit-bot) wrote: > Try jobs failed on following builders: > tryserver.chromium on win_chromium_compile_dbg Sorry, I could not watch this sit for another night. There was a small compiler error in the Windows code. I fixed it and committed manually: https://codereview.chromium.org/256673002/
MANY Thanks!
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
