Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1903933002:
Store devices that only need to be enumerated once (Closed)
|
Created:
4 years, 8 months ago by juncai Modified:
4 years, 7 months ago Reviewers:
Reilly Grant (use Gerrit) CC:
chromium-reviews Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionStore devices that only need to be enumerated once
Some devices that only need to be enumerated once and then
can be ignored (for example, hub devices, devices that
failed enumeration, etc.). Currently these devices are
enumerated each time UsbServiceImpl::EnumerateDevice() is
called since they are added to |new_devices| at
UsbServiceImpl::OnDeviceList(). This patch add code to
store those devices and not enumerate them again.
BUG=604792
Committed: https://crrev.com/d23f2ba75f50575513dcbde328c6041e4d47716c
Cr-Commit-Position: refs/heads/master@{#390234}
Patch Set 1 : only enumerate hub devices once #Patch Set 2 : address reillyg@'s comment #
Total comments: 8
Patch Set 3 : address reillyg@'s comments #
Total comments: 2
Patch Set 4 : clean up code #Patch Set 5 : fixed device leak at usb_service_impl.cc #Messages
Total messages: 20 (9 generated)
Description was changed from ========== Only enumerate hub devices once The hub devices only need to be enumerated once. BUG=604792 ========== to ========== Only enumerate hub devices once The hub devices only need to be enumerated once. Currently the hub devices are enumerated each time UsbServiceImpl::EnumerateDevice() is called since they are added to |new_devices| at UsbServiceImpl::OnDeviceList(). This patch add code to enumerate hub devices once once. BUG=604792 ==========
Description was changed from ========== Only enumerate hub devices once The hub devices only need to be enumerated once. Currently the hub devices are enumerated each time UsbServiceImpl::EnumerateDevice() is called since they are added to |new_devices| at UsbServiceImpl::OnDeviceList(). This patch add code to enumerate hub devices once once. BUG=604792 ========== to ========== Only enumerate hub devices once The hub devices only need to be enumerated once. Currently the hub devices are enumerated each time UsbServiceImpl::EnumerateDevice() is called since they are added to |new_devices| at UsbServiceImpl::OnDeviceList(). This patch add code to enumerate hub devices once once. BUG=604792 ==========
juncai@chromium.org changed reviewers: + reillyg@chromium.org
Please take a look.
This is a good start but this should also handle the case of failed enumerations such as when we try to open a device and get permission denied.
Made some modification to address the comment. Please take a look.
Description was changed from ========== Only enumerate hub devices once The hub devices only need to be enumerated once. Currently the hub devices are enumerated each time UsbServiceImpl::EnumerateDevice() is called since they are added to |new_devices| at UsbServiceImpl::OnDeviceList(). This patch add code to enumerate hub devices once once. BUG=604792 ========== to ========== Ignore devices that only need to be enumerated once Some devices that only need to be enumerated once and then can be ignored (for example, hub devices, devices that failed enumeration, etc.). Currently these devices are enumerated each time UsbServiceImpl::EnumerateDevice() is called since they are added to |new_devices| at UsbServiceImpl::OnDeviceList(). This patch add code to address this issue. BUG=604792 ==========
Description was changed from ========== Ignore devices that only need to be enumerated once Some devices that only need to be enumerated once and then can be ignored (for example, hub devices, devices that failed enumeration, etc.). Currently these devices are enumerated each time UsbServiceImpl::EnumerateDevice() is called since they are added to |new_devices| at UsbServiceImpl::OnDeviceList(). This patch add code to address this issue. BUG=604792 ========== to ========== Store devices that only need to be enumerated once Some devices that only need to be enumerated once and then can be ignored (for example, hub devices, devices that failed enumeration, etc.). Currently these devices are enumerated each time UsbServiceImpl::EnumerateDevice() is called since they are added to |new_devices| at UsbServiceImpl::OnDeviceList(). This patch add code to store those devices and not enumerate them again. BUG=604792 ==========
Description was changed from ========== Store devices that only need to be enumerated once Some devices that only need to be enumerated once and then can be ignored (for example, hub devices, devices that failed enumeration, etc.). Currently these devices are enumerated each time UsbServiceImpl::EnumerateDevice() is called since they are added to |new_devices| at UsbServiceImpl::OnDeviceList(). This patch add code to store those devices and not enumerate them again. BUG=604792 ========== to ========== Store devices that only need to be enumerated once Some devices that only need to be enumerated once and then can be ignored (for example, hub devices, devices that failed enumeration, etc.). Currently these devices are enumerated each time UsbServiceImpl::EnumerateDevice() is called since they are added to |new_devices| at UsbServiceImpl::OnDeviceList(). This patch add code to store those devices and not enumerate them again. BUG=604792 ==========
https://codereview.chromium.org/1903933002/diff/20001/device/usb/usb_service_... File device/usb/usb_service_impl.cc (right): https://codereview.chromium.org/1903933002/diff/20001/device/usb/usb_service_... device/usb/usb_service_impl.cc:430: if (ignored_devices_.find(platform_device) != ignored_devices_.end()) { if (ContainsValue(ignored_devices, platform_device)) https://codereview.chromium.org/1903933002/diff/20001/device/usb/usb_service_... device/usb/usb_service_impl.cc:465: existing_ignored_devices.end()) if (!ContainsValue(existing_ignore_devices, *current)) https://codereview.chromium.org/1903933002/diff/20001/device/usb/usb_service_... device/usb/usb_service_impl.cc:531: refresh_complete, add_ignored_device)); Instead of passing 3 callbacks here can we just pass two: add_device and enumeration_failed? enumeration_failed would AddIgnoredDevice and then run refresh_complete just like AddDevice adds the device and then runs refresh_complete. https://codereview.chromium.org/1903933002/diff/20001/device/usb/usb_service_... device/usb/usb_service_impl.cc:645: ignored_devices_.insert(platform_device); We should probably call libusb_ref_device(platform_device) here so that the pointer remains value.
https://codereview.chromium.org/1903933002/diff/20001/device/usb/usb_service_... File device/usb/usb_service_impl.cc (right): https://codereview.chromium.org/1903933002/diff/20001/device/usb/usb_service_... device/usb/usb_service_impl.cc:430: if (ignored_devices_.find(platform_device) != ignored_devices_.end()) { On 2016/04/22 21:12:04, Reilly Grant wrote: > if (ContainsValue(ignored_devices, platform_device)) Done. https://codereview.chromium.org/1903933002/diff/20001/device/usb/usb_service_... device/usb/usb_service_impl.cc:465: existing_ignored_devices.end()) On 2016/04/22 21:12:04, Reilly Grant wrote: > if (!ContainsValue(existing_ignore_devices, *current)) Done. https://codereview.chromium.org/1903933002/diff/20001/device/usb/usb_service_... device/usb/usb_service_impl.cc:531: refresh_complete, add_ignored_device)); On 2016/04/22 21:12:04, Reilly Grant wrote: > Instead of passing 3 callbacks here can we just pass two: add_device and > enumeration_failed? > > enumeration_failed would AddIgnoredDevice and then run refresh_complete just > like AddDevice adds the device and then runs refresh_complete. Done. https://codereview.chromium.org/1903933002/diff/20001/device/usb/usb_service_... device/usb/usb_service_impl.cc:645: ignored_devices_.insert(platform_device); On 2016/04/22 21:12:04, Reilly Grant wrote: > We should probably call libusb_ref_device(platform_device) here so that the > pointer remains value. Done.
https://codereview.chromium.org/1903933002/diff/40001/device/usb/usb_service_... File device/usb/usb_service_impl.cc (right): https://codereview.chromium.org/1903933002/diff/40001/device/usb/usb_service_... device/usb/usb_service_impl.cc:461: ignored_devices_.erase(current); We should libusb_unref_device here and in the UsbServiceImpl destructor. Can you do a sanity test (by adding an OutputDebugString in libusb) that we are indeed not leaking libusb_device objects when they are unplugged?
https://codereview.chromium.org/1903933002/diff/40001/device/usb/usb_service_... File device/usb/usb_service_impl.cc (right): https://codereview.chromium.org/1903933002/diff/40001/device/usb/usb_service_... device/usb/usb_service_impl.cc:461: ignored_devices_.erase(current); On 2016/04/22 22:21:24, Reilly Grant wrote: > We should libusb_unref_device here and in the UsbServiceImpl destructor. Can you > do a sanity test (by adding an OutputDebugString in libusb) that we are indeed > not leaking libusb_device objects when they are unplugged? Done. Also did some sanity check and found a potential device leak at function UsbServiceImpl::OnDeviceList() and fixed it. I think after the fix, when unplugging either hub or non-hub device, Chrome doesn't leak any device.
lgtm
The CQ bit was checked by juncai@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1903933002/80001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1903933002/80001
Message was sent while issue was closed.
Description was changed from ========== Store devices that only need to be enumerated once Some devices that only need to be enumerated once and then can be ignored (for example, hub devices, devices that failed enumeration, etc.). Currently these devices are enumerated each time UsbServiceImpl::EnumerateDevice() is called since they are added to |new_devices| at UsbServiceImpl::OnDeviceList(). This patch add code to store those devices and not enumerate them again. BUG=604792 ========== to ========== Store devices that only need to be enumerated once Some devices that only need to be enumerated once and then can be ignored (for example, hub devices, devices that failed enumeration, etc.). Currently these devices are enumerated each time UsbServiceImpl::EnumerateDevice() is called since they are added to |new_devices| at UsbServiceImpl::OnDeviceList(). This patch add code to store those devices and not enumerate them again. BUG=604792 ==========
Message was sent while issue was closed.
Committed patchset #5 (id:80001)
Message was sent while issue was closed.
Patchset 5 (id:??) landed as https://crrev.com/d23f2ba75f50575513dcbde328c6041e4d47716c Cr-Commit-Position: refs/heads/master@{#390234}
Message was sent while issue was closed.
Description was changed from ========== Store devices that only need to be enumerated once Some devices that only need to be enumerated once and then can be ignored (for example, hub devices, devices that failed enumeration, etc.). Currently these devices are enumerated each time UsbServiceImpl::EnumerateDevice() is called since they are added to |new_devices| at UsbServiceImpl::OnDeviceList(). This patch add code to store those devices and not enumerate them again. BUG=604792 ========== to ========== Store devices that only need to be enumerated once Some devices that only need to be enumerated once and then can be ignored (for example, hub devices, devices that failed enumeration, etc.). Currently these devices are enumerated each time UsbServiceImpl::EnumerateDevice() is called since they are added to |new_devices| at UsbServiceImpl::OnDeviceList(). This patch add code to store those devices and not enumerate them again. BUG=604792 Committed: https://crrev.com/d23f2ba75f50575513dcbde328c6041e4d47716c Cr-Commit-Position: refs/heads/master@{#390234} ========== |
