Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
968663004:
Improve MidiManagerAlsa manufacturer reporting (Closed)
|
Created:
5 years, 9 months ago by Adam Goode Modified:
5 years, 9 months ago CC:
chromium-reviews, feature-media-reviews_chromium.org, Ryan Sleevi Base URL:
https://chromium.googlesource.com/chromium/src.git@udev Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionImprove MidiManagerAlsa manufacturer reporting
This is dependent on https://codereview.chromium.org/965903003/
BUG=377250
Committed: https://crrev.com/2e1bef845184269974b3dc42f7864add125b8466
Cr-Commit-Position: refs/heads/master@{#319597}
Patch Set 1 #Patch Set 2 : Fix building midi_manager_alsa_unittest for gn #Patch Set 3 : Try again to fix components build for midi_manager_alsa #Patch Set 4 : Fix gyp build of media_unittests #Patch Set 5 : Fix gyp component builds on linux #Patch Set 6 : Undo incorrect use of MEDIA_IMPLEMENTATION #
Total comments: 6
Patch Set 7 : Address reviewer comments #
Total comments: 2
Patch Set 8 : Fix declaration order of MidiManagerAlsa::UnescapeUdev #Patch Set 9 : Remove use of banned R"(...)" #
Total comments: 10
Patch Set 10 : Fix a few issues, separate out udev stuff more #
Total comments: 6
Patch Set 11 : Fix final comments #Patch Set 12 : Fix embarrassing mistake that broke driver and manufacturer #
Messages
Total messages: 32 (8 generated)
agoode@chromium.org changed reviewers: + armansito@chromium.org, wolenetz@chromium.org
agoode@chromium.org changed reviewers: + toyoshim@chromium.org - armansito@chromium.org
I think I've finally got all the gn and gyp stuff worked out.
thank you for your contribution again. https://codereview.chromium.org/968663004/diff/90001/media/midi/midi_manager_... File media/midi/midi_manager_alsa.h (right): https://codereview.chromium.org/968663004/diff/90001/media/midi/midi_manager_... media/midi/midi_manager_alsa.h:27: MEDIA_EXPORT_PRIVATE std::string UnescapeUdev(const std::string& s); Do you have any reason to place these two functions out side MidiManagerAlsa class, and inside a special namespace with MEDIA_EXPORT_PRIVATE? How about having them as private methods and using FRIEND_TEST_ALL_PREFIXES to make them accessible from you tests?
On 2015/03/04 09:33:37, Takashi Toyoshima (chromium) wrote: > thank you for your contribution again. > > https://codereview.chromium.org/968663004/diff/90001/media/midi/midi_manager_... > File media/midi/midi_manager_alsa.h (right): > > https://codereview.chromium.org/968663004/diff/90001/media/midi/midi_manager_... > media/midi/midi_manager_alsa.h:27: MEDIA_EXPORT_PRIVATE std::string > UnescapeUdev(const std::string& s); > Do you have any reason to place these two functions out side MidiManagerAlsa > class, and inside a special namespace with MEDIA_EXPORT_PRIVATE? > > How about having them as private methods and using FRIEND_TEST_ALL_PREFIXES to > make them accessible from you tests? Hi, I was attempting to follow this advice: https://code.google.com/p/googletest/wiki/AdvancedGuide#Static_Functions First I tried #including the .cc file directly into the test, which worked with gyp but failed in gn builds. Then there was a bit of a mess with the component build which is why I needed to MEDIA_EXPORT_PRIVATE. The 2 functions really are static, and not methods. I previously had them in the anonymous namespace, but they were untestable. If moving these to private methods is cleaner, I am happy to do it.
cc+=rsleevi due to comment on another CL: https://codereview.chromium.org/877323009/#msg50 : rsleevi@: is the usage of use_udev and use_alsa across .gn .gyp .cc *and* especially .h consistent and correct with your comment? If not, is there some grandfathering allowed? https://codereview.chromium.org/968663004/diff/90001/media/media.gyp File media/media.gyp (right): https://codereview.chromium.org/968663004/diff/90001/media/media.gyp#newcode1407 media/media.gyp:1407: ['OS=="linux"', { Why is this divergent from media.gn's use_alsa, and even the use_alsa gyp variable elsewhere in this same file? https://codereview.chromium.org/968663004/diff/90001/media/midi/midi_manager_... File media/midi/midi_manager_alsa.h (right): https://codereview.chromium.org/968663004/diff/90001/media/midi/midi_manager_... media/midi/midi_manager_alsa.h:27: MEDIA_EXPORT_PRIVATE std::string UnescapeUdev(const std::string& s); On 2015/03/04 09:33:37, Takashi Toyoshima (chromium) wrote: > Do you have any reason to place these two functions out side MidiManagerAlsa > class, and inside a special namespace with MEDIA_EXPORT_PRIVATE? > > How about having them as private methods and using FRIEND_TEST_ALL_PREFIXES to > make them accessible from you tests? +1. I'm not fully conversant with the pros/cons here, but ISTM that adding a new way of exporting test-only-visible media functionality for just these 2 functions is inconsistent with other test-only methods in media that are private and accessible by tests as friends.
rsleevi@chromium.org changed reviewers: + rsleevi@chromium.org
Yeah, the build system changes are OK because 1) The variable is declared in //build/common.gypi 2) The define is injected into all targets [Once a GN world is around, we'd be able to keep the variable and the define local] https://codereview.chromium.org/968663004/diff/90001/media/midi/midi_manager_... File media/midi/midi_manager_alsa.h (right): https://codereview.chromium.org/968663004/diff/90001/media/midi/midi_manager_... media/midi/midi_manager_alsa.h:18: #if defined(USE_UDEV) Yes, this is fine because it's forced into the entire tree by //build/common.gypi
https://codereview.chromium.org/968663004/diff/90001/media/media.gyp File media/media.gyp (right): https://codereview.chromium.org/968663004/diff/90001/media/media.gyp#newcode1407 media/media.gyp:1407: ['OS=="linux"', { On 2015/03/04 19:28:58, wolenetz wrote: > Why is this divergent from media.gn's use_alsa, and even the use_alsa gyp > variable elsewhere in this same file? Good catch. https://codereview.chromium.org/968663004/diff/90001/media/midi/midi_manager_... File media/midi/midi_manager_alsa.h (right): https://codereview.chromium.org/968663004/diff/90001/media/midi/midi_manager_... media/midi/midi_manager_alsa.h:27: MEDIA_EXPORT_PRIVATE std::string UnescapeUdev(const std::string& s); On 2015/03/04 19:28:58, wolenetz wrote: > On 2015/03/04 09:33:37, Takashi Toyoshima (chromium) wrote: > > Do you have any reason to place these two functions out side MidiManagerAlsa > > class, and inside a special namespace with MEDIA_EXPORT_PRIVATE? > > > > How about having them as private methods and using FRIEND_TEST_ALL_PREFIXES to > > make them accessible from you tests? > > +1. I'm not fully conversant with the pros/cons here, but ISTM that adding a new > way of exporting test-only-visible media functionality for just these 2 > functions is inconsistent with other test-only methods in media that are private > and accessible by tests as friends. Done.
lgtm Thank you, rsleevi@, for confirming your prior guidance in the context of this CL. https://codereview.chromium.org/968663004/diff/110001/media/midi/midi_manager... File media/midi/midi_manager_alsa.cc (right): https://codereview.chromium.org/968663004/diff/110001/media/midi/midi_manager... media/midi/midi_manager_alsa.cc:474: std::string MidiManagerAlsa::UnescapeUdev(const std::string& s) { nit: style: "Function declaration order should match function definition order." Though this seems often-overlooked, and even the EventReset(), EventLoop() orders already mismatched.
https://codereview.chromium.org/968663004/diff/110001/media/midi/midi_manager... File media/midi/midi_manager_alsa.cc (right): https://codereview.chromium.org/968663004/diff/110001/media/midi/midi_manager... media/midi/midi_manager_alsa.cc:474: std::string MidiManagerAlsa::UnescapeUdev(const std::string& s) { On 2015/03/04 22:18:35, wolenetz wrote: > nit: style: "Function declaration order should match function definition order." > Though this seems often-overlooked, and even the EventReset(), EventLoop() > orders already mismatched. Done.
On 2015/03/04 22:57:35, Adam Goode wrote: > https://codereview.chromium.org/968663004/diff/110001/media/midi/midi_manager... > File media/midi/midi_manager_alsa.cc (right): > > https://codereview.chromium.org/968663004/diff/110001/media/midi/midi_manager... > media/midi/midi_manager_alsa.cc:474: std::string > MidiManagerAlsa::UnescapeUdev(const std::string& s) { > On 2015/03/04 22:18:35, wolenetz wrote: > > nit: style: "Function declaration order should match function definition > order." > > Though this seems often-overlooked, and even the EventReset(), EventLoop() > > orders already mismatched. > > Done. Thank you. Please wait for media/midi owner's l-g-t-m before landing.
On 2015/03/04 23:07:27, wolenetz wrote: > On 2015/03/04 22:57:35, Adam Goode wrote: > > > https://codereview.chromium.org/968663004/diff/110001/media/midi/midi_manager... > > File media/midi/midi_manager_alsa.cc (right): > > > > > https://codereview.chromium.org/968663004/diff/110001/media/midi/midi_manager... > > media/midi/midi_manager_alsa.cc:474: std::string > > MidiManagerAlsa::UnescapeUdev(const std::string& s) { > > On 2015/03/04 22:18:35, wolenetz wrote: > > > nit: style: "Function declaration order should match function definition > > order." > > > Though this seems often-overlooked, and even the EventReset(), EventLoop() > > > orders already mismatched. > > > > Done. > > Thank you. Please wait for media/midi owner's l-g-t-m before landing. No problem. Thanks!
looks almost ok, but there are some nits, and one optional suggestion. https://codereview.chromium.org/968663004/diff/150001/media/midi/midi_manager... File media/midi/midi_manager_alsa.cc (right): https://codereview.chromium.org/968663004/diff/150001/media/midi/midi_manager... media/midi/midi_manager_alsa.cc:46: // Copied from components/storage_monitor/udev_util_linux.cc. I'm not an udev expert, but should we have a TODO here for factoring out common code into device/udev_linux or somewhere? https://codereview.chromium.org/968663004/diff/150001/media/midi/midi_manager... media/midi/midi_manager_alsa.cc:144: // hotplug is implemented. (See http://crbug.com/279097.) Maybe 431489 is better bug id for now, since 279097 was closed, and 43189 is separately filed for ALSA specific implementation. https://codereview.chromium.org/968663004/diff/150001/media/midi/midi_manager... media/midi/midi_manager_alsa.cc:189: std::string udev_id_vendor_enc; I slightly feel it's better to replace following udev dependent code outside this for-loop as an external function. To keep current unit tests work as is, how about following code? // one line code for for-loop here. const std::string manufacturer = GetManufacturer(longname, card_index); // and define two methods. std::string MidiManagerAlsa::GetManufacturer(const std::string alsa_longname, int card_index) { #if defined(USE_UDEV) // udev dependent code from line 193 to 202, and line 205. #else // Try a heuristic code from line 498 to 507. #endif } #if defined(USE_UDEV) // static std::string MidiManagerAlsa::ExtractManufacturer(int card_index) { // line 480 to 496, and 510. } #endif https://codereview.chromium.org/968663004/diff/150001/media/midi/midi_manager... media/midi/midi_manager_alsa.cc:474: std::string MidiManagerAlsa::ExtractManufacturer( It isn't mandatory, but usually we add an one line comment "// static" just before a static member method implementation. https://codereview.chromium.org/968663004/diff/150001/media/midi/midi_manager... media/midi/midi_manager_alsa.cc:480: // Let's try to determine the manufacturer. Here is the ordered preference I prefer to use #if defined(USE_UDEV) - #endif from line 480 to 496 if you don't like my previous proposal at line 189.
https://codereview.chromium.org/968663004/diff/150001/media/midi/midi_manager... File media/midi/midi_manager_alsa.cc (right): https://codereview.chromium.org/968663004/diff/150001/media/midi/midi_manager... media/midi/midi_manager_alsa.cc:46: // Copied from components/storage_monitor/udev_util_linux.cc. On 2015/03/05 07:23:22, Takashi Toyoshima (chromium) wrote: > I'm not an udev expert, but should we have a TODO here for factoring out common > code into device/udev_linux or somewhere? Done. https://codereview.chromium.org/968663004/diff/150001/media/midi/midi_manager... media/midi/midi_manager_alsa.cc:144: // hotplug is implemented. (See http://crbug.com/279097.) On 2015/03/05 07:23:22, Takashi Toyoshima (chromium) wrote: > Maybe 431489 is better bug id for now, since 279097 was closed, and 43189 is > separately filed for ALSA specific implementation. Done. https://codereview.chromium.org/968663004/diff/150001/media/midi/midi_manager... media/midi/midi_manager_alsa.cc:189: std::string udev_id_vendor_enc; On 2015/03/05 07:23:22, Takashi Toyoshima (chromium) wrote: > I slightly feel it's better to replace following udev dependent code outside > this for-loop as an external function. > > To keep current unit tests work as is, how about following code? > > // one line code for for-loop here. > const std::string manufacturer = GetManufacturer(longname, card_index); > > // and define two methods. > std::string MidiManagerAlsa::GetManufacturer(const std::string alsa_longname, > int card_index) { > #if defined(USE_UDEV) > // udev dependent code from line 193 to 202, and line 205. > #else > // Try a heuristic code from line 498 to 507. > #endif > } > > #if defined(USE_UDEV) > // static > std::string MidiManagerAlsa::ExtractManufacturer(int card_index) { > // line 480 to 496, and 510. > } > #endif I refactored a bit. Let me know what you think. It might still need some work. https://codereview.chromium.org/968663004/diff/150001/media/midi/midi_manager... media/midi/midi_manager_alsa.cc:474: std::string MidiManagerAlsa::ExtractManufacturer( On 2015/03/05 07:23:22, Takashi Toyoshima (chromium) wrote: > It isn't mandatory, but usually we add an one line comment "// static" just > before a static member method implementation. Done. https://codereview.chromium.org/968663004/diff/150001/media/midi/midi_manager... media/midi/midi_manager_alsa.cc:480: // Let's try to determine the manufacturer. Here is the ordered preference On 2015/03/05 07:23:22, Takashi Toyoshima (chromium) wrote: > I prefer to use #if defined(USE_UDEV) - #endif from line 480 to 496 if you don't > like my previous proposal at line 189. Acknowledged.
https://codereview.chromium.org/968663004/diff/150001/media/midi/midi_manager... File media/midi/midi_manager_alsa.cc (right): https://codereview.chromium.org/968663004/diff/150001/media/midi/midi_manager... media/midi/midi_manager_alsa.cc:46: // Copied from components/storage_monitor/udev_util_linux.cc. On 2015/03/05 07:23:22, Takashi Toyoshima (chromium) wrote: > I'm not an udev expert, but should we have a TODO here for factoring out common > code into device/udev_linux or somewhere? Done. https://codereview.chromium.org/968663004/diff/150001/media/midi/midi_manager... media/midi/midi_manager_alsa.cc:144: // hotplug is implemented. (See http://crbug.com/279097.) On 2015/03/05 07:23:22, Takashi Toyoshima (chromium) wrote: > Maybe 431489 is better bug id for now, since 279097 was closed, and 43189 is > separately filed for ALSA specific implementation. Done. https://codereview.chromium.org/968663004/diff/150001/media/midi/midi_manager... media/midi/midi_manager_alsa.cc:189: std::string udev_id_vendor_enc; On 2015/03/05 07:23:22, Takashi Toyoshima (chromium) wrote: > I slightly feel it's better to replace following udev dependent code outside > this for-loop as an external function. > > To keep current unit tests work as is, how about following code? > > // one line code for for-loop here. > const std::string manufacturer = GetManufacturer(longname, card_index); > > // and define two methods. > std::string MidiManagerAlsa::GetManufacturer(const std::string alsa_longname, > int card_index) { > #if defined(USE_UDEV) > // udev dependent code from line 193 to 202, and line 205. > #else > // Try a heuristic code from line 498 to 507. > #endif > } > > #if defined(USE_UDEV) > // static > std::string MidiManagerAlsa::ExtractManufacturer(int card_index) { > // line 480 to 496, and 510. > } > #endif I refactored a bit. Let me know what you think. It might still need some work. https://codereview.chromium.org/968663004/diff/150001/media/midi/midi_manager... media/midi/midi_manager_alsa.cc:474: std::string MidiManagerAlsa::ExtractManufacturer( On 2015/03/05 07:23:22, Takashi Toyoshima (chromium) wrote: > It isn't mandatory, but usually we add an one line comment "// static" just > before a static member method implementation. Done. https://codereview.chromium.org/968663004/diff/150001/media/midi/midi_manager... media/midi/midi_manager_alsa.cc:480: // Let's try to determine the manufacturer. Here is the ordered preference On 2015/03/05 07:23:22, Takashi Toyoshima (chromium) wrote: > I prefer to use #if defined(USE_UDEV) - #endif from line 480 to 496 if you don't > like my previous proposal at line 189. Acknowledged.
lgtm with nits and a question that may need a small trivial fix. https://codereview.chromium.org/968663004/diff/170001/media/midi/midi_manager... File media/midi/midi_manager_alsa.cc (right): https://codereview.chromium.org/968663004/diff/170001/media/midi/midi_manager... media/midi/midi_manager_alsa.cc:47: // TODO(agoode) Move this into a common place. Maybe device/udev_linux? ditto; TODO(agoode): Move... https://codereview.chromium.org/968663004/diff/170001/media/midi/midi_manager... media/midi/midi_manager_alsa.cc:524: // Ok, udev gave us nothing useful. So try a heuristic. Does this case happen even on USE_UDEV? If so, current code looks fine, but if this is only case for !USE_UDEV, I slightly prefer to move line 524 to 533 should be placed around line 357 inside #else - #endif https://codereview.chromium.org/968663004/diff/170001/media/midi/midi_manager... File media/midi/midi_manager_alsa.h (right): https://codereview.chromium.org/968663004/diff/170001/media/midi/midi_manager... media/midi/midi_manager_alsa.h:66: // TODO(agoode) Move this into a common place. Maybe device/udev_linux? Nit; ":" after TODO(agoode)
https://codereview.chromium.org/968663004/diff/170001/media/midi/midi_manager... File media/midi/midi_manager_alsa.cc (right): https://codereview.chromium.org/968663004/diff/170001/media/midi/midi_manager... media/midi/midi_manager_alsa.cc:47: // TODO(agoode) Move this into a common place. Maybe device/udev_linux? On 2015/03/06 08:00:12, Takashi Toyoshima (chromium) wrote: > ditto; TODO(agoode): Move... Done. https://codereview.chromium.org/968663004/diff/170001/media/midi/midi_manager... media/midi/midi_manager_alsa.cc:524: // Ok, udev gave us nothing useful. So try a heuristic. On 2015/03/06 08:00:12, Takashi Toyoshima (chromium) wrote: > Does this case happen even on USE_UDEV? If so, current code looks fine, but if > this is only case for !USE_UDEV, I slightly prefer to move line 524 to 533 > should be placed around line 357 inside #else - #endif Yes, udev may be available but not have useful information about the hardware. I've clarified the comment to say "udev gave us nothing useful, or was unavailable". https://codereview.chromium.org/968663004/diff/170001/media/midi/midi_manager... File media/midi/midi_manager_alsa.h (right): https://codereview.chromium.org/968663004/diff/170001/media/midi/midi_manager... media/midi/midi_manager_alsa.h:66: // TODO(agoode) Move this into a common place. Maybe device/udev_linux? On 2015/03/06 08:00:12, Takashi Toyoshima (chromium) wrote: > Nit; ":" after TODO(agoode) Done.
PTAL I fixed an embarrassing mistake where manufacturer and driver were not initialized.
Oh, I also overlooked it. Patch Set 12 LGTM.
The CQ bit was checked by agoode@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from wolenetz@chromium.org Link to the patchset: https://codereview.chromium.org/968663004/#ps210001 (title: "Fix embarrassing mistake that broke driver and manufacturer")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/968663004/210001
agoode@chromium.org changed reviewers: + reillyg@chromium.org
OWNERS lgtm needed for adding +device/udev_linux to DEPS
The CQ bit was unchecked by agoode@chromium.org
DEPS lgtm
The CQ bit was checked by agoode@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/968663004/210001
Message was sent while issue was closed.
Committed patchset #12 (id:210001)
Message was sent while issue was closed.
Patchset 12 (id:??) landed as https://crrev.com/2e1bef845184269974b3dc42f7864add125b8466 Cr-Commit-Position: refs/heads/master@{#319597} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
