Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
10824086:
Add support for device removal/arrival detection with device type. For now this allows us to detect… (Closed)
|
Created:
8 years, 4 months ago by tommi (sloooow) - chröme Modified:
8 years, 4 months ago CC:
chromium-reviews, joi+watch-content_chromium.org, erikwright (departed), darin-cc_chromium.org, jam, brettw-cc_chromium.org Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionAdd support for device removal/arrival detection with device type. For now this allows us to detect when audio and video capture devices are connected/disconnected.
TEST=Try connecting and disconnecting USB audio/video devices. You should see notifications in the log about those operations being detected.
BUG=137799
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=149279
Patch Set 1 #Patch Set 2 : Ready for review #
Total comments: 2
Patch Set 3 : Fix constant name and reduce logging #
Total comments: 2
Patch Set 4 : Address comments #
Total comments: 7
Patch Set 5 : Address comments #
Total comments: 2
Patch Set 6 : Fix comment #
Messages
Total messages: 16 (0 generated)
wjia: main review piman: owner Lei: fyi
https://chromiumcodereview.appspot.com/10824086/diff/8001/content/browser/sys... File content/browser/system_message_window_win.cc (right): https://chromiumcodereview.appspot.com/10824086/diff/8001/content/browser/sys... content/browser/system_message_window_win.cc:126: DVLOG(1) << "Ignoring device of unknown type arrival/removal."; The Windows gamepad code needs this as well. You need to put gamepad devices in kDeviceCategoryMap.
http://codereview.chromium.org/10824086/diff/5001/content/browser/system_mess... File content/browser/system_message_window_win.cc (right): http://codereview.chromium.org/10824086/diff/5001/content/browser/system_mess... content/browser/system_message_window_win.cc:123: "an audio device." : "a video device"); When more device types are added, this code has to be changed as well. To make it easily expandable, it might be ok to use base::SystemMonitor::DeviceType, instead of strings.
Thanks. ptal. https://chromiumcodereview.appspot.com/10824086/diff/5001/content/browser/sys... File content/browser/system_message_window_win.cc (right): https://chromiumcodereview.appspot.com/10824086/diff/5001/content/browser/sys... content/browser/system_message_window_win.cc:123: "an audio device." : "a video device"); On 2012/07/31 06:12:26, wjia wrote: > When more device types are added, this code has to be changed as well. To make > it easily expandable, it might be ok to use base::SystemMonitor::DeviceType, > instead of strings. Agree. There already is a LOG() statement inside ProcessDevicesChanged which includes the device_type, so I just removed this one. https://chromiumcodereview.appspot.com/10824086/diff/8001/content/browser/sys... File content/browser/system_message_window_win.cc (right): https://chromiumcodereview.appspot.com/10824086/diff/8001/content/browser/sys... content/browser/system_message_window_win.cc:126: DVLOG(1) << "Ignoring device of unknown type arrival/removal."; On 2012/07/31 05:15:54, Lei Zhang wrote: > The Windows gamepad code needs this as well. You need to put gamepad devices in > kDeviceCategoryMap. We also get the DBT_DEVNODES_CHANGED notification above so the functionality for gamepad is unchanged. I removed this DVLOG and added comments to explain why we return here. Here's the background: The DBT_DEVICEREMOVECOMPLETE/DBT_DEVICEARRIVAL notifications are new and come in addition of the DBT_DEVNODES_CHANGED notification. When a gamepad device is plugged in/unplugged, we have the same functionality as today, but for an audio or video capture device, we'll get two or more notifications (>2 for composite devices). One of those notifications will have device_type set to AUDIO_CAPTURE/VIDEO_CAPTURE, but the others (received via DBT_DEVNODE_CHANGED) will have device_type==DEVTYPE_UNKNOWN. The MediaStreamManager will ignore notifications for DEVTYPE_UNKNOWN and handle only the audio/video ones. The gamepad code could do something similar as well, but when we add that functionality we have to update the devtype enum, add gamepad devices to the kDeviceCategoryMap, and then updated the linux and mac device detection code as well.
lgtm
A few nits http://codereview.chromium.org/10824086/diff/5002/content/browser/system_mess... File content/browser/system_message_window_win.cc (right): http://codereview.chromium.org/10824086/diff/5002/content/browser/system_mess... content/browser/system_message_window_win.cc:26: } nit: // anonymous namespace http://codereview.chromium.org/10824086/diff/5002/content/browser/system_mess... content/browser/system_message_window_win.cc:50: DEVICE_NOTIFY_WINDOW_HANDLE); nit: correct style is either: LongFuntionName(a, b, c); or LongFuntionName( a, b, c); http://codereview.chromium.org/10824086/diff/5002/content/browser/system_mess... content/browser/system_message_window_win.cc:147: static_cast<DWORD_PTR>(lparam)); why the need for 2 casts, one here and one in OnDeviceChange? Can we pass the data as a LPARAM and do a single cast up there? Or the opposite, do the cast here and pass Chrome types to OnDeviceChange?
ptal http://codereview.chromium.org/10824086/diff/5002/content/browser/system_mess... File content/browser/system_message_window_win.cc (right): http://codereview.chromium.org/10824086/diff/5002/content/browser/system_mess... content/browser/system_message_window_win.cc:26: } On 2012/07/31 18:24:55, piman wrote: > nit: // anonymous namespace Done. http://codereview.chromium.org/10824086/diff/5002/content/browser/system_mess... content/browser/system_message_window_win.cc:50: DEVICE_NOTIFY_WINDOW_HANDLE); On 2012/07/31 18:24:55, piman wrote: > nit: correct style is either: > > LongFuntionName(a, > b, > c); > > or > > LongFuntionName( > a, > b, > c); Fixed. However, I've seen this style elsewhere in Chrome so I looked it up in the style guide. Look for "AndAnotherLongFunctionCall" in the style guide: http://www.chromium.org/developers/coding-style http://codereview.chromium.org/10824086/diff/5002/content/browser/system_mess... content/browser/system_message_window_win.cc:147: static_cast<DWORD_PTR>(lparam)); On 2012/07/31 18:24:55, piman wrote: > why the need for 2 casts, one here and one in OnDeviceChange? > Can we pass the data as a LPARAM and do a single cast up there? > Or the opposite, do the cast here and pass Chrome types to OnDeviceChange? Good point. Done (did the former).
lgtm
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/tommi@chromium.org/10824086/2003
Try job failure for 10824086-2003 (retry) (retry) on mac_rel for steps "base_unittests, browser_tests, cacheinvalidation_unittests, jingle_unittests, remoting_unittests, interactive_ui_tests, crypto_unittests, net_unittests, unit_tests, gpu_unittests, nacl_integration, media_unittests, safe_browsing_tests, ipc_tests, printing_unittests, sql_unittests, content_unittests, sync_unit_tests". It's a second try, previously, steps "base_unittests, browser_tests, cacheinvalidation_unittests, jingle_unittests, remoting_unittests, interactive_ui_tests, crypto_unittests, net_unittests, unit_tests, gpu_unittests, nacl_integration, media_unittests, safe_browsing_tests, ipc_tests, printing_unittests, sql_unittests, content_unittests, sync_unit_tests" failed. http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=mac_rel&nu...
https://chromiumcodereview.appspot.com/10824086/diff/5002/content/browser/sys... File content/browser/system_message_window_win.cc (right): https://chromiumcodereview.appspot.com/10824086/diff/5002/content/browser/sys... content/browser/system_message_window_win.cc:26: } On 2012/07/31 18:50:09, tommi wrote: > On 2012/07/31 18:24:55, piman wrote: > > nit: // anonymous namespace > > Done. You need to upload a new patchset.
On 2012/07/31 19:46:45, Lei Zhang wrote: > https://chromiumcodereview.appspot.com/10824086/diff/5002/content/browser/sys... > File content/browser/system_message_window_win.cc (right): > > https://chromiumcodereview.appspot.com/10824086/diff/5002/content/browser/sys... > content/browser/system_message_window_win.cc:26: } > On 2012/07/31 18:50:09, tommi wrote: > > On 2012/07/31 18:24:55, piman wrote: > > > nit: // anonymous namespace > > > > Done. > > You need to upload a new patchset. Rather, I need to hit reload. :)
lgtm https://chromiumcodereview.appspot.com/10824086/diff/2003/content/browser/sys... File content/browser/system_message_window_win.cc (right): https://chromiumcodereview.appspot.com/10824086/diff/2003/content/browser/sys... content/browser/system_message_window_win.cc:26: } // anonymous namespace. It's actually just "} // namespace"
https://chromiumcodereview.appspot.com/10824086/diff/2003/content/browser/sys... File content/browser/system_message_window_win.cc (right): https://chromiumcodereview.appspot.com/10824086/diff/2003/content/browser/sys... content/browser/system_message_window_win.cc:26: } // anonymous namespace. On 2012/07/31 19:51:03, Lei Zhang wrote: > It's actually just "} // namespace" doh, of course. thanks and Done.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/tommi@chromium.org/10824086/20001
Change committed as 149279 |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
