Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
105043008:
Introduce USB MIDI descriptor parser (Closed)
|
Created:
7 years ago by yhirano Modified:
6 years, 11 months ago CC:
chromium-reviews, feature-media-reviews_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Visibility:
Public. |
DescriptionIntroduce USB MIDI descriptor parser
In order to recognize USB MIDI jacks associated with endpoints, this CL
introduces a USB descriptor parser.
BUG=303596
R=toyoshim@chromium.org
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=244101
Patch Set 1 #Patch Set 2 : #Patch Set 3 : #
Total comments: 37
Patch Set 4 : #Patch Set 5 : #Patch Set 6 : #
Total comments: 2
Patch Set 7 : #Patch Set 8 : #Patch Set 9 : #
Total comments: 30
Patch Set 10 : #Patch Set 11 : #Patch Set 12 : #Patch Set 13 : #
Total comments: 8
Patch Set 14 : #Patch Set 15 : #
Total comments: 2
Patch Set 16 : #
Messages
Total messages: 17 (0 generated)
https://codereview.chromium.org/105043008/diff/40001/media/media.gyp File media/media.gyp (right): https://codereview.chromium.org/105043008/diff/40001/media/media.gyp#newcode421 media/media.gyp:421: 'midi/usb_midi_descriptor_parser.cc', midi/android/ sounds better place to have them now. https://codereview.chromium.org/105043008/diff/40001/media/media.gyp#newcode1011 media/media.gyp:1011: 'midi/usb_midi_descriptor_parser_unittest.cc', ditto https://codereview.chromium.org/105043008/diff/40001/media/midi/usb_midi_desc... File media/midi/usb_midi_descriptor_parser.cc (right): https://codereview.chromium.org/105043008/diff/40001/media/midi/usb_midi_desc... media/midi/usb_midi_descriptor_parser.cc:14: DEVICE = 1, optional: I prefer to have TYPE_ prefix, e.g. TYPE_DEVICE, and so on. But it's up to you. https://codereview.chromium.org/105043008/diff/40001/media/midi/usb_midi_desc... media/midi/usb_midi_descriptor_parser.cc:41: const uint8 kAudioInterfaceClass = 1; Can you add some comments on them for someone who don't know about USB spec. "These values are defined in USB spec" or something is enough, I think. https://codereview.chromium.org/105043008/diff/40001/media/midi/usb_midi_desc... media/midi/usb_midi_descriptor_parser.cc:67: size_t size) { If this function assumes that jacks_ is empty, it's better to add DCHECK here, or to move Clear() from Parse() to here. https://codereview.chromium.org/105043008/diff/40001/media/midi/usb_midi_desc... media/midi/usb_midi_descriptor_parser.cc:79: uint8 descriptor_type = current[1]; Why don't you use DescriptorType instead of uint8? https://codereview.chromium.org/105043008/diff/40001/media/midi/usb_midi_desc... media/midi/usb_midi_descriptor_parser.cc:82: if (!ParseInterface(device, current, length)) return false; I don't think this style is common in chromium. Can you insert a line break before "return false"? FYI; % git grep 'if (.*) return' | wc -l 110 All of them are placed in cast/* or mp4/* https://codereview.chromium.org/105043008/diff/40001/media/midi/usb_midi_desc... media/midi/usb_midi_descriptor_parser.cc:122: if (!is_parsing_usb_midi_interface_) return true; style https://codereview.chromium.org/105043008/diff/40001/media/midi/usb_midi_desc... media/midi/usb_midi_descriptor_parser.cc:130: uint8 subtype = data[2]; DescriptorSubType https://codereview.chromium.org/105043008/diff/40001/media/midi/usb_midi_desc... media/midi/usb_midi_descriptor_parser.cc:148: break; break at line 148 should be between line 146 and 147. https://codereview.chromium.org/105043008/diff/40001/media/midi/usb_midi_desc... media/midi/usb_midi_descriptor_parser.cc:196: } Can we assume CS_ENDPOINT always follows associated CS_INTERFACE and ENDPOINT? If so, it's better to have comment that spec ensure their descriptor order. https://codereview.chromium.org/105043008/diff/40001/media/midi/usb_midi_desc... File media/midi/usb_midi_descriptor_parser.h (right): https://codereview.chromium.org/105043008/diff/40001/media/midi/usb_midi_desc... media/midi/usb_midi_descriptor_parser.h:28: // never crashes. So, when does it return false? Or why it may return against invalid inputs? That may be a common question when someone read this comment. Can you elaborate it? https://codereview.chromium.org/105043008/diff/40001/media/midi/usb_midi_desc... media/midi/usb_midi_descriptor_parser.h:46: std::vector<UsbMidiJack> jacks_; Some comments? They are not trivial, and comments will be helpful to understand how this class parse inputs. https://codereview.chromium.org/105043008/diff/40001/media/midi/usb_midi_desc... media/midi/usb_midi_descriptor_parser.h:47: base::hash_map<uint8, UsbMidiJack> incomplete_jacks_; DISALLOW_COPY_AND_ASSIGN() https://codereview.chromium.org/105043008/diff/40001/media/midi/usb_midi_desc... File media/midi/usb_midi_descriptor_parser_unittest.cc (right): https://codereview.chromium.org/105043008/diff/40001/media/midi/usb_midi_desc... media/midi/usb_midi_descriptor_parser_unittest.cc:30: uint8 data[] = { It's better to have a comment that describes summary of following descriptor. https://codereview.chromium.org/105043008/diff/40001/media/midi/usb_midi_desc... media/midi/usb_midi_descriptor_parser_unittest.cc:43: uint8 data[] = { ditto https://codereview.chromium.org/105043008/diff/40001/media/midi/usb_midi_desc... media/midi/usb_midi_descriptor_parser_unittest.cc:48: EXPECT_TRUE(parser.Parse(NULL, data, arraysize(data))); So, here is a question for the function design. Why don't you return false for incomplete (unresolved?) jacks? https://codereview.chromium.org/105043008/diff/40001/media/midi/usb_midi_desc... media/midi/usb_midi_descriptor_parser_unittest.cc:54: uint8 data[] = { ditto
https://codereview.chromium.org/105043008/diff/40001/media/media.gyp File media/media.gyp (right): https://codereview.chromium.org/105043008/diff/40001/media/media.gyp#newcode421 media/media.gyp:421: 'midi/usb_midi_descriptor_parser.cc', On 2013/12/19 05:54:23, Takashi Toyoshima (chromium) wrote: > midi/android/ sounds better place to have them now. As discussed offline, I would like to place them here in order to run unittests on non-android environments. https://codereview.chromium.org/105043008/diff/40001/media/midi/usb_midi_desc... File media/midi/usb_midi_descriptor_parser.cc (right): https://codereview.chromium.org/105043008/diff/40001/media/midi/usb_midi_desc... media/midi/usb_midi_descriptor_parser.cc:14: DEVICE = 1, On 2013/12/19 05:54:23, Takashi Toyoshima (chromium) wrote: > optional: I prefer to have TYPE_ prefix, e.g. TYPE_DEVICE, and so on. But it's > up to you. Done. https://codereview.chromium.org/105043008/diff/40001/media/midi/usb_midi_desc... media/midi/usb_midi_descriptor_parser.cc:41: const uint8 kAudioInterfaceClass = 1; On 2013/12/19 05:54:23, Takashi Toyoshima (chromium) wrote: > Can you add some comments on them for someone who don't know about USB spec. > "These values are defined in USB spec" or something is enough, I think. Done in L13 https://codereview.chromium.org/105043008/diff/40001/media/midi/usb_midi_desc... media/midi/usb_midi_descriptor_parser.cc:67: size_t size) { On 2013/12/19 05:54:23, Takashi Toyoshima (chromium) wrote: > If this function assumes that jacks_ is empty, it's better to add DCHECK here, > or to move Clear() from Parse() to here. Done. https://codereview.chromium.org/105043008/diff/40001/media/midi/usb_midi_desc... media/midi/usb_midi_descriptor_parser.cc:79: uint8 descriptor_type = current[1]; On 2013/12/19 05:54:23, Takashi Toyoshima (chromium) wrote: > Why don't you use DescriptorType instead of uint8? Done. https://codereview.chromium.org/105043008/diff/40001/media/midi/usb_midi_desc... media/midi/usb_midi_descriptor_parser.cc:82: if (!ParseInterface(device, current, length)) return false; On 2013/12/19 05:54:23, Takashi Toyoshima (chromium) wrote: > I don't think this style is common in chromium. > Can you insert a line break before "return false"? > > FYI; > % git grep 'if (.*) return' | wc -l > 110 > All of them are placed in cast/* or mp4/* Done. https://codereview.chromium.org/105043008/diff/40001/media/midi/usb_midi_desc... media/midi/usb_midi_descriptor_parser.cc:122: if (!is_parsing_usb_midi_interface_) return true; On 2013/12/19 05:54:23, Takashi Toyoshima (chromium) wrote: > style Done. https://codereview.chromium.org/105043008/diff/40001/media/midi/usb_midi_desc... media/midi/usb_midi_descriptor_parser.cc:130: uint8 subtype = data[2]; On 2013/12/19 05:54:23, Takashi Toyoshima (chromium) wrote: > DescriptorSubType Done. https://codereview.chromium.org/105043008/diff/40001/media/midi/usb_midi_desc... media/midi/usb_midi_descriptor_parser.cc:148: break; On 2013/12/19 05:54:23, Takashi Toyoshima (chromium) wrote: > break at line 148 should be between line 146 and 147. Done. https://codereview.chromium.org/105043008/diff/40001/media/midi/usb_midi_desc... media/midi/usb_midi_descriptor_parser.cc:196: } On 2013/12/19 05:54:23, Takashi Toyoshima (chromium) wrote: > Can we assume CS_ENDPOINT always follows associated CS_INTERFACE and ENDPOINT? > If so, it's better to have comment that spec ensure their descriptor order. Done here and at L121 https://codereview.chromium.org/105043008/diff/40001/media/midi/usb_midi_desc... File media/midi/usb_midi_descriptor_parser.h (right): https://codereview.chromium.org/105043008/diff/40001/media/midi/usb_midi_desc... media/midi/usb_midi_descriptor_parser.h:28: // never crashes. On 2013/12/19 05:54:23, Takashi Toyoshima (chromium) wrote: > So, when does it return false? Or why it may return against invalid inputs? > That may be a common question when someone read this comment. > Can you elaborate it? It may return false when the input is not correct. I wanted to say that this function does not fully check the descriptor. For example, it doesn't check the content of DEVICE descriptor. https://codereview.chromium.org/105043008/diff/40001/media/midi/usb_midi_desc... media/midi/usb_midi_descriptor_parser.h:46: std::vector<UsbMidiJack> jacks_; On 2013/12/19 05:54:23, Takashi Toyoshima (chromium) wrote: > Some comments? They are not trivial, and comments will be helpful to understand > how this class parse inputs. Done. https://codereview.chromium.org/105043008/diff/40001/media/midi/usb_midi_desc... media/midi/usb_midi_descriptor_parser.h:47: base::hash_map<uint8, UsbMidiJack> incomplete_jacks_; On 2013/12/19 05:54:23, Takashi Toyoshima (chromium) wrote: > DISALLOW_COPY_AND_ASSIGN() Done. https://codereview.chromium.org/105043008/diff/40001/media/midi/usb_midi_desc... File media/midi/usb_midi_descriptor_parser_unittest.cc (right): https://codereview.chromium.org/105043008/diff/40001/media/midi/usb_midi_desc... media/midi/usb_midi_descriptor_parser_unittest.cc:30: uint8 data[] = { On 2013/12/19 05:54:23, Takashi Toyoshima (chromium) wrote: > It's better to have a comment that describes summary of following descriptor. Done. https://codereview.chromium.org/105043008/diff/40001/media/midi/usb_midi_desc... media/midi/usb_midi_descriptor_parser_unittest.cc:43: uint8 data[] = { On 2013/12/19 05:54:23, Takashi Toyoshima (chromium) wrote: > ditto Done. https://codereview.chromium.org/105043008/diff/40001/media/midi/usb_midi_desc... media/midi/usb_midi_descriptor_parser_unittest.cc:48: EXPECT_TRUE(parser.Parse(NULL, data, arraysize(data))); On 2013/12/19 05:54:23, Takashi Toyoshima (chromium) wrote: > So, here is a question for the function design. > Why don't you return false for incomplete (unresolved?) jacks? Done. https://codereview.chromium.org/105043008/diff/40001/media/midi/usb_midi_desc... media/midi/usb_midi_descriptor_parser_unittest.cc:54: uint8 data[] = { On 2013/12/19 05:54:23, Takashi Toyoshima (chromium) wrote: > ditto Done.
LGTM. https://codereview.chromium.org/105043008/diff/40001/media/midi/usb_midi_desc... File media/midi/usb_midi_descriptor_parser_unittest.cc (right): https://codereview.chromium.org/105043008/diff/40001/media/midi/usb_midi_desc... media/midi/usb_midi_descriptor_parser_unittest.cc:48: EXPECT_TRUE(parser.Parse(NULL, data, arraysize(data))); Ah, my original question was wrong. I'm not sure it's useful to return true when INTERFACE descriptor is missing. Do you have a special reason? If you have, it's better to describe it around Parse() function comments.
https://codereview.chromium.org/105043008/diff/40001/media/midi/usb_midi_desc... File media/midi/usb_midi_descriptor_parser_unittest.cc (right): https://codereview.chromium.org/105043008/diff/40001/media/midi/usb_midi_desc... media/midi/usb_midi_descriptor_parser_unittest.cc:48: EXPECT_TRUE(parser.Parse(NULL, data, arraysize(data))); On 2013/12/19 07:48:48, Takashi Toyoshima (chromium) wrote: > Ah, my original question was wrong. > I'm not sure it's useful to return true when INTERFACE descriptor is missing. Do > you have a special reason? If you have, it's better to describe it around > Parse() function comments. Sorry, I mis-read your comment. There is no special reason for returning true, I only want to check that the parser doesn't crash. We can return false by adding some code, but we are currently skipping many checks intentionally because checking all errors is very hard.
So, please add TODOs where you think we need more strict checker in the future.
https://codereview.chromium.org/105043008/diff/90001/media/midi/usb_midi_desc... File media/midi/usb_midi_descriptor_parser.cc (right): https://codereview.chromium.org/105043008/diff/90001/media/midi/usb_midi_desc... media/midi/usb_midi_descriptor_parser.cc:101: default: Need a comment to skip uninteresting types which isn't related to MIDI or something?
+dalecurtis for OWNER review. https://codereview.chromium.org/105043008/diff/90001/media/midi/usb_midi_desc... File media/midi/usb_midi_descriptor_parser.cc (right): https://codereview.chromium.org/105043008/diff/90001/media/midi/usb_midi_desc... media/midi/usb_midi_descriptor_parser.cc:101: default: On 2013/12/19 08:33:14, Takashi Toyoshima (chromium) wrote: > Need a comment to skip uninteresting types which isn't related to MIDI or > something? Done.
Sorry, I don't have time to review this before the year end.
https://codereview.chromium.org/105043008/diff/150001/media/midi/usb_midi_des... File media/midi/usb_midi_descriptor_parser.cc (right): https://codereview.chromium.org/105043008/diff/150001/media/midi/usb_midi_des... media/midi/usb_midi_descriptor_parser.cc:59: Clear(); You seem to be always clearing the internal state after and before very call. Why not instead return the jacks list from this method and use local variables inside ParseInternal? I'm a little confused on what the data stream will look like. A description of what it looks like in the header of this file would be very helpful. Will the order of interfaces vary between Parse() calls? Are there multiple instances of each interface in the stream? This code spends some effort to ensure it can handle any order of interfaces gracefully. I.e., Instead, if an interface is always expected to follow another it'd make sense to bundle those parse calls together. https://codereview.chromium.org/105043008/diff/150001/media/midi/usb_midi_des... media/midi/usb_midi_descriptor_parser.cc:73: while (current < &data[size]) { The notation here is slightly confusing. It'd be clearer to use "current < data + size". https://codereview.chromium.org/105043008/diff/150001/media/midi/usb_midi_des... media/midi/usb_midi_descriptor_parser.cc:79: if (¤t[length] > &data[size]) { Ditto. https://codereview.chromium.org/105043008/diff/150001/media/midi/usb_midi_des... media/midi/usb_midi_descriptor_parser.cc:102: // Ignore unintersting types. uninteresting https://codereview.chromium.org/105043008/diff/150001/media/midi/usb_midi_des... media/midi/usb_midi_descriptor_parser.cc:125: (interface_class == kAudioInterfaceClass && No need for parens. https://codereview.chromium.org/105043008/diff/150001/media/midi/usb_midi_des... media/midi/usb_midi_descriptor_parser.cc:134: return true; Should this be false? Here and all other methods. https://codereview.chromium.org/105043008/diff/150001/media/midi/usb_midi_des... media/midi/usb_midi_descriptor_parser.cc:137: if (size <= 2) { < 3 for consistency. https://codereview.chromium.org/105043008/diff/150001/media/midi/usb_midi_des... media/midi/usb_midi_descriptor_parser.cc:146: // FALL_THROUGH Label is unnecessary since you're not doing anything different for this case. https://codereview.chromium.org/105043008/diff/150001/media/midi/usb_midi_des... media/midi/usb_midi_descriptor_parser.cc:156: incomplete_jacks_.insert( incomplete_jacks_[id] = ... ? no need for make_pair then. How many entries do you expect to be in this structure on a regular basis? If it's only a few, you should probably just use a vector instead. https://codereview.chromium.org/105043008/diff/150001/media/midi/usb_midi_des... media/midi/usb_midi_descriptor_parser.cc:173: if (size < 4) { Ditto. https://codereview.chromium.org/105043008/diff/150001/media/midi/usb_midi_des... media/midi/usb_midi_descriptor_parser.cc:194: if (size != static_cast<size_t>(4 + num_jacks)) { 4 is used multiple times, so it should be extracted to a local constant. https://codereview.chromium.org/105043008/diff/150001/media/midi/usb_midi_des... media/midi/usb_midi_descriptor_parser.cc:207: if (current_cable_number_ >= 0x10) { Using > 0xF is clearer and more consistent with the comment. https://codereview.chromium.org/105043008/diff/150001/media/midi/usb_midi_des... File media/midi/usb_midi_descriptor_parser.h (right): https://codereview.chromium.org/105043008/diff/150001/media/midi/usb_midi_des... media/midi/usb_midi_descriptor_parser.h:1: // Copyright (c) 2013 The Chromium Authors. All rights reserved. No (c) for the year anymore. https://codereview.chromium.org/105043008/diff/150001/media/midi/usb_midi_jack.h File media/midi/usb_midi_jack.h (right): https://codereview.chromium.org/105043008/diff/150001/media/midi/usb_midi_jac... media/midi/usb_midi_jack.h:41: Direction GetDirection() const { use hacker style for inline methods. Since it has methods this should be a class and not a struct. https://codereview.chromium.org/105043008/diff/150001/media/midi/usb_midi_jac... media/midi/usb_midi_jack.h:42: return (endpoint_address & 0x80) ? IN: OUT; space before :
https://codereview.chromium.org/105043008/diff/150001/media/midi/usb_midi_des... File media/midi/usb_midi_descriptor_parser.cc (right): https://codereview.chromium.org/105043008/diff/150001/media/midi/usb_midi_des... media/midi/usb_midi_descriptor_parser.cc:59: Clear(); On 2014/01/03 00:56:49, DaleCurtis wrote: > You seem to be always clearing the internal state after and before very call. > Why not instead return the jacks list from this method and use local variables > inside ParseInternal? > > I'm a little confused on what the data stream will look like. A description of > what it looks like in the header of this file would be very helpful. > > Will the order of interfaces vary between Parse() calls? Are there multiple > instances of each interface in the stream? This code spends some effort to > ensure it can handle any order of interfaces gracefully. I.e., Instead, if an > interface is always expected to follow another it'd make sense to bundle those > parse calls together. Removing jacks_ is done. This is not a generic parser and I think this style is reasonable to ignore uninteresting descriptors without writing much code. For example, it is difficult to find the end of a non-MIDI INTERFACE without writing more detailed code, but layered parser requires it. Added some comments in the header and ParseInternal. https://codereview.chromium.org/105043008/diff/150001/media/midi/usb_midi_des... media/midi/usb_midi_descriptor_parser.cc:73: while (current < &data[size]) { On 2014/01/03 00:56:49, DaleCurtis wrote: > The notation here is slightly confusing. It'd be clearer to use "current < data > + size". Done. https://codereview.chromium.org/105043008/diff/150001/media/midi/usb_midi_des... media/midi/usb_midi_descriptor_parser.cc:79: if (¤t[length] > &data[size]) { On 2014/01/03 00:56:49, DaleCurtis wrote: > Ditto. Done. https://codereview.chromium.org/105043008/diff/150001/media/midi/usb_midi_des... media/midi/usb_midi_descriptor_parser.cc:102: // Ignore unintersting types. On 2014/01/03 00:56:49, DaleCurtis wrote: > uninteresting Done. https://codereview.chromium.org/105043008/diff/150001/media/midi/usb_midi_des... media/midi/usb_midi_descriptor_parser.cc:125: (interface_class == kAudioInterfaceClass && On 2014/01/03 00:56:49, DaleCurtis wrote: > No need for parens. Done. https://codereview.chromium.org/105043008/diff/150001/media/midi/usb_midi_des... media/midi/usb_midi_descriptor_parser.cc:134: return true; On 2014/01/03 00:56:49, DaleCurtis wrote: > Should this be false? Here and all other methods. This should be true, we want to ignore uninteresting descriptors. https://codereview.chromium.org/105043008/diff/150001/media/midi/usb_midi_des... media/midi/usb_midi_descriptor_parser.cc:137: if (size <= 2) { On 2014/01/03 00:56:49, DaleCurtis wrote: > < 3 for consistency. Done. https://codereview.chromium.org/105043008/diff/150001/media/midi/usb_midi_des... media/midi/usb_midi_descriptor_parser.cc:146: // FALL_THROUGH On 2014/01/03 00:56:49, DaleCurtis wrote: > Label is unnecessary since you're not doing anything different for this case. Done. https://codereview.chromium.org/105043008/diff/150001/media/midi/usb_midi_des... media/midi/usb_midi_descriptor_parser.cc:156: incomplete_jacks_.insert( On 2014/01/03 00:56:49, DaleCurtis wrote: > incomplete_jacks_[id] = ... ? no need for make_pair then. How many entries do > you expect to be in this structure on a regular basis? If it's only a few, you > should probably just use a vector instead. Done. https://codereview.chromium.org/105043008/diff/150001/media/midi/usb_midi_des... media/midi/usb_midi_descriptor_parser.cc:173: if (size < 4) { On 2014/01/03 00:56:49, DaleCurtis wrote: > Ditto. Sorry, what ditto? https://codereview.chromium.org/105043008/diff/150001/media/midi/usb_midi_des... media/midi/usb_midi_descriptor_parser.cc:194: if (size != static_cast<size_t>(4 + num_jacks)) { On 2014/01/03 00:56:49, DaleCurtis wrote: > 4 is used multiple times, so it should be extracted to a local constant. Done. https://codereview.chromium.org/105043008/diff/150001/media/midi/usb_midi_des... media/midi/usb_midi_descriptor_parser.cc:207: if (current_cable_number_ >= 0x10) { On 2014/01/03 00:56:49, DaleCurtis wrote: > Using > 0xF is clearer and more consistent with the comment. Done. https://codereview.chromium.org/105043008/diff/150001/media/midi/usb_midi_des... File media/midi/usb_midi_descriptor_parser.h (right): https://codereview.chromium.org/105043008/diff/150001/media/midi/usb_midi_des... media/midi/usb_midi_descriptor_parser.h:1: // Copyright (c) 2013 The Chromium Authors. All rights reserved. On 2014/01/03 00:56:49, DaleCurtis wrote: > No (c) for the year anymore. Done. https://codereview.chromium.org/105043008/diff/150001/media/midi/usb_midi_jack.h File media/midi/usb_midi_jack.h (right): https://codereview.chromium.org/105043008/diff/150001/media/midi/usb_midi_jac... media/midi/usb_midi_jack.h:41: Direction GetDirection() const { On 2014/01/03 00:56:49, DaleCurtis wrote: > use hacker style for inline methods. Since it has methods this should be a > class and not a struct. Done. https://codereview.chromium.org/105043008/diff/150001/media/midi/usb_midi_jac... media/midi/usb_midi_jack.h:42: return (endpoint_address & 0x80) ? IN: OUT; On 2014/01/03 00:56:49, DaleCurtis wrote: > space before : Done.
lgtm % comments. https://codereview.chromium.org/105043008/diff/390001/media/midi/usb_midi_des... File media/midi/usb_midi_descriptor_parser.cc (right): https://codereview.chromium.org/105043008/diff/390001/media/midi/usb_midi_des... media/midi/usb_midi_descriptor_parser.cc:91: if (current + length > data + size) { current + length >= data + size https://codereview.chromium.org/105043008/diff/390001/media/midi/usb_midi_des... media/midi/usb_midi_descriptor_parser.cc:96: switch (descriptor_type) { Instead of checking !is_parsing_usb_midi_interface_ in all the methods, how about: const bool is_interface = descriptor_type == TYPE_INTERFACE; if (is_interface && !ParseInterface(current, length)) return false; if (is_interface || !is_parsing_usb_midi_interface_) continue; If you rephrase the while like so, you can avoid duplicating the advance logic: for (const uint8* current = data; current < data + size; current += current[0]) {} https://codereview.chromium.org/105043008/diff/390001/media/midi/usb_midi_des... media/midi/usb_midi_descriptor_parser.cc:159: switch (subtype) { Replace the switch with an early return if !IN_JACK && !OUT_JACK return true? Allows you to deindent this whole section a few tabs. https://codereview.chromium.org/105043008/diff/390001/media/midi/usb_midi_jack.h File media/midi/usb_midi_jack.h (right): https://codereview.chromium.org/105043008/diff/390001/media/midi/usb_midi_jac... media/midi/usb_midi_jack.h:16: struct MEDIA_EXPORT UsbMidiJack { Class? If you think other classes will use your JackMatcher, just add an operator== here instead.
https://codereview.chromium.org/105043008/diff/390001/media/midi/usb_midi_des... File media/midi/usb_midi_descriptor_parser.cc (right): https://codereview.chromium.org/105043008/diff/390001/media/midi/usb_midi_des... media/midi/usb_midi_descriptor_parser.cc:91: if (current + length > data + size) { On 2014/01/10 01:55:57, DaleCurtis wrote: > current + length >= data + size current_length == data + size means that it is the (valid) last descriptor, so it's OK. https://codereview.chromium.org/105043008/diff/390001/media/midi/usb_midi_des... media/midi/usb_midi_descriptor_parser.cc:96: switch (descriptor_type) { On 2014/01/10 01:55:57, DaleCurtis wrote: > Instead of checking !is_parsing_usb_midi_interface_ in all the methods, how > about: > > const bool is_interface = descriptor_type == TYPE_INTERFACE; > if (is_interface && !ParseInterface(current, length)) > return false; > > if (is_interface || !is_parsing_usb_midi_interface_) > continue; > > If you rephrase the while like so, you can avoid duplicating the advance logic: > for (const uint8* current = data; current < data + size; current += current[0]) > {} Thanks, done. https://codereview.chromium.org/105043008/diff/390001/media/midi/usb_midi_des... media/midi/usb_midi_descriptor_parser.cc:159: switch (subtype) { On 2014/01/10 01:55:57, DaleCurtis wrote: > Replace the switch with an early return if !IN_JACK && !OUT_JACK return true? > Allows you to deindent this whole section a few tabs. Done. https://codereview.chromium.org/105043008/diff/390001/media/midi/usb_midi_jack.h File media/midi/usb_midi_jack.h (right): https://codereview.chromium.org/105043008/diff/390001/media/midi/usb_midi_jac... media/midi/usb_midi_jack.h:16: struct MEDIA_EXPORT UsbMidiJack { On 2014/01/10 01:55:57, DaleCurtis wrote: > Class? If you think other classes will use your JackMatcher, just add an > operator== here instead. A jack is fully identified with (device, endpoint_address, jack_id), since endpoint address is unique in a USB device and jack ID is unique in a USB interface. Hence JackMatcher is not a generic matcher and I want to define it there. (note: We find an jack with JackMatcher in incomplete jacks that the current parsing interface contains.)
lgtm https://codereview.chromium.org/105043008/diff/450006/media/midi/usb_midi_des... File media/midi/usb_midi_descriptor_parser.cc (right): https://codereview.chromium.org/105043008/diff/450006/media/midi/usb_midi_des... media/midi/usb_midi_descriptor_parser.cc:99: const bool is_interface = descriptor_type == TYPE_INTERFACE; Unnecessary to extract as const if you're only using it once.
https://codereview.chromium.org/105043008/diff/450006/media/midi/usb_midi_des... File media/midi/usb_midi_descriptor_parser.cc (right): https://codereview.chromium.org/105043008/diff/450006/media/midi/usb_midi_des... media/midi/usb_midi_descriptor_parser.cc:99: const bool is_interface = descriptor_type == TYPE_INTERFACE; On 2014/01/10 02:40:14, DaleCurtis wrote: > Unnecessary to extract as const if you're only using it once. Done.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/yhirano@chromium.org/105043008/520001
Message was sent while issue was closed.
Change committed as 244101 |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
