Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(611)

Issue 1439443002: Reland: Add code to deal with serial device disconnection detection on Windows (Closed)

Created:
5 years, 1 month ago by juncai
Modified:
5 years, 1 month ago
CC:
chromium-reviews, chromium-apps-reviews_chromium.org, extensions-reviews_chromium.org
Base URL:
https://chromium.googlesource.com/chromium/src.git@master
Target Ref:
refs/pending/heads/master
Project:
chromium
Visibility:
Public.

Description

Add code to deal with serial device disconnection detection on Windows. This patch added code to deal with serial device disconnection detection problem on Windows. It gets the COM port information from the device path and compare the COM port information with the port information that serial io handler holds. If they match, cancel read for that port. BUG=361606 Committed: https://crrev.com/195a0f202c1b89540d4a385d881cd483abe757fa Cr-Commit-Position: refs/heads/master@{#360193} Committed: https://crrev.com/8d09d9564ea10012d55ed634c0c2835efbd8d01f Cr-Commit-Position: refs/heads/master@{#360347} Committed: https://crrev.com/0c9eb038cae670b299c303a5e18547ec85e6e3b0 Cr-Commit-Position: refs/heads/master@{#361009}

Patch Set 1 : added code to deal with serial device disconnection detection #

Patch Set 2 : updated code to call thread helper from UI thread #

Total comments: 15

Patch Set 3 : address reillyg@'s comments #

Patch Set 4 : initialize helper_ to be nullptr #

Total comments: 8

Patch Set 5 : address reillgy@'s comments #

Total comments: 2

Patch Set 6 : moved scoped device files to device core directory #

Total comments: 6

Patch Set 7 : updated BUILD.gn and gyp files #

Total comments: 8

Patch Set 8 : address grt@'s comments #

Total comments: 2

Patch Set 9 : changed some DVPLOG back to VPLOG #

Total comments: 12

Patch Set 10 : address grt@'s comments #

Total comments: 12

Patch Set 11 : use static_cast to cast size_t to DWORD #

Patch Set 12 : address grt@'s comments #

Total comments: 8

Patch Set 13 : moved DeviceInfoQueryWin implementation into a .cc file #

Patch Set 14 : added documentation and comments #

Patch Set 15 : added device core dep to usb and serial build files #

Patch Set 16 : removed //device/core from core BUILD.gn #

Patch Set 17 : added setupapi.lib to device/core/BUILD.gn #

Unified diffs Side-by-side diffs Delta from patch set Stats (+256 lines, -74 lines) Patch
M device/core/BUILD.gn View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 2 chunks +6 lines, -0 lines 0 comments Download
M device/core/core.gyp View 1 2 3 4 5 6 7 8 9 10 11 12 1 chunk +2 lines, -0 lines 0 comments Download
A device/core/device_info_query_win.h View 1 2 3 4 5 6 7 8 9 10 11 12 13 1 chunk +49 lines, -0 lines 0 comments Download
A device/core/device_info_query_win.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 1 chunk +66 lines, -0 lines 0 comments Download
M device/serial/BUILD.gn View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 1 chunk +1 line, -0 lines 0 comments Download
M device/serial/serial.gyp View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 1 chunk +1 line, -0 lines 0 comments Download
M device/serial/serial_io_handler.h View 1 2 3 4 5 6 7 2 chunks +12 lines, -0 lines 0 comments Download
M device/serial/serial_io_handler.cc View 1 2 1 chunk +1 line, -0 lines 0 comments Download
M device/serial/serial_io_handler_win.h View 1 2 3 3 chunks +8 lines, -0 lines 0 comments Download
M device/serial/serial_io_handler_win.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 5 chunks +100 lines, -3 lines 0 comments Download
M device/usb/usb.gyp View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 1 chunk +1 line, -0 lines 0 comments Download
M device/usb/usb_service_impl.cc View 1 2 3 4 5 6 7 8 9 10 11 12 2 chunks +9 lines, -71 lines 0 comments Download

Messages

Total messages: 45 (14 generated)
juncai
Please review this patch.
5 years, 1 month ago (2015-11-11 03:35:53 UTC) #4
Reilly Grant (use Gerrit)
https://codereview.chromium.org/1439443002/diff/20001/device/serial/serial_io_handler_win.cc File device/serial/serial_io_handler_win.cc (right): https://codereview.chromium.org/1439443002/diff/20001/device/serial/serial_io_handler_win.cc#newcode141 device/serial/serial_io_handler_win.cc:141: class ScopedDeviceInfoList { Let's see if we can get ...
5 years, 1 month ago (2015-11-11 20:43:59 UTC) #5
juncai
https://codereview.chromium.org/1439443002/diff/20001/device/serial/serial_io_handler_win.cc File device/serial/serial_io_handler_win.cc (right): https://codereview.chromium.org/1439443002/diff/20001/device/serial/serial_io_handler_win.cc#newcode141 device/serial/serial_io_handler_win.cc:141: class ScopedDeviceInfoList { On 2015/11/11 20:43:59, Reilly Grant wrote: ...
5 years, 1 month ago (2015-11-12 05:27:08 UTC) #6
Reilly Grant (use Gerrit)
lgtm with nits, please add a //base/win reviewer. https://codereview.chromium.org/1439443002/diff/60001/base/win/scoped_device_info_object.h File base/win/scoped_device_info_object.h (right): https://codereview.chromium.org/1439443002/diff/60001/base/win/scoped_device_info_object.h#newcode1 base/win/scoped_device_info_object.h:1: // ...
5 years, 1 month ago (2015-11-12 19:30:02 UTC) #7
juncai
https://codereview.chromium.org/1439443002/diff/60001/base/win/scoped_device_info_object.h File base/win/scoped_device_info_object.h (right): https://codereview.chromium.org/1439443002/diff/60001/base/win/scoped_device_info_object.h#newcode1 base/win/scoped_device_info_object.h:1: // Copyright (c) 2015 The Chromium Authors. All rights ...
5 years, 1 month ago (2015-11-12 23:21:39 UTC) #8
juncai
cpu@chromium.org: Please review changes in //base
5 years, 1 month ago (2015-11-12 23:22:11 UTC) #10
juncai
grt@chromium.org: Please review changes in //base/win thestig@chromium.org: Please review changes in //base/BUILD.gn //base/base.gypi
5 years, 1 month ago (2015-11-13 01:09:56 UTC) #13
Lei Zhang
Can the new scopers stay in device/ until we find some reason to require them ...
5 years, 1 month ago (2015-11-13 01:38:26 UTC) #14
Lei Zhang
https://codereview.chromium.org/1439443002/diff/20001/device/serial/serial_io_handler_win.cc File device/serial/serial_io_handler_win.cc (right): https://codereview.chromium.org/1439443002/diff/20001/device/serial/serial_io_handler_win.cc#newcode141 device/serial/serial_io_handler_win.cc:141: class ScopedDeviceInfoList { On 2015/11/11 20:43:59, Reilly Grant wrote: ...
5 years, 1 month ago (2015-11-13 01:40:50 UTC) #15
grt (UTC plus 2)
https://codereview.chromium.org/1439443002/diff/100001/device/core/scoped_device_info_list.h File device/core/scoped_device_info_list.h (right): https://codereview.chromium.org/1439443002/diff/100001/device/core/scoped_device_info_list.h#newcode13 device/core/scoped_device_info_list.h:13: namespace base { this shouldn't be in base::win any ...
5 years, 1 month ago (2015-11-13 19:40:05 UTC) #16
juncai
https://codereview.chromium.org/1439443002/diff/100001/device/core/scoped_device_info_list.h File device/core/scoped_device_info_list.h (right): https://codereview.chromium.org/1439443002/diff/100001/device/core/scoped_device_info_list.h#newcode13 device/core/scoped_device_info_list.h:13: namespace base { On 2015/11/13 19:40:04, grt wrote: > ...
5 years, 1 month ago (2015-11-14 01:51:24 UTC) #17
Reilly Grant (use Gerrit)
https://codereview.chromium.org/1439443002/diff/140001/device/serial/serial_io_handler_win.cc File device/serial/serial_io_handler_win.cc (left): https://codereview.chromium.org/1439443002/diff/140001/device/serial/serial_io_handler_win.cc#oldcode169 device/serial/serial_io_handler_win.cc:169: VPLOG(1) << "Failed to set serial timeouts"; Don't change ...
5 years, 1 month ago (2015-11-14 01:54:54 UTC) #18
grt (UTC plus 2)
thanks, this is looking cleaner. https://codereview.chromium.org/1439443002/diff/160001/device/core/device_info_query_win.h File device/core/device_info_query_win.h (right): https://codereview.chromium.org/1439443002/diff/160001/device/core/device_info_query_win.h#newcode20 device/core/device_info_query_win.h:20: : device_info_list_(SetupDiCreateDeviceInfoList(NULL, NULL)) { ...
5 years, 1 month ago (2015-11-16 16:14:16 UTC) #19
juncai
https://codereview.chromium.org/1439443002/diff/140001/device/serial/serial_io_handler_win.cc File device/serial/serial_io_handler_win.cc (left): https://codereview.chromium.org/1439443002/diff/140001/device/serial/serial_io_handler_win.cc#oldcode169 device/serial/serial_io_handler_win.cc:169: VPLOG(1) << "Failed to set serial timeouts"; On 2015/11/14 ...
5 years, 1 month ago (2015-11-16 21:10:34 UTC) #20
grt (UTC plus 2)
https://codereview.chromium.org/1439443002/diff/180001/device/core/device_info_query_win.h File device/core/device_info_query_win.h (right): https://codereview.chromium.org/1439443002/diff/180001/device/core/device_info_query_win.h#newcode30 device/core/device_info_query_win.h:30: if (device_info_list_ != INVALID_HANDLE_VALUE) { nit: if (device_info_list_valid()) { ...
5 years, 1 month ago (2015-11-16 21:41:56 UTC) #21
juncai
https://codereview.chromium.org/1439443002/diff/180001/device/core/device_info_query_win.h File device/core/device_info_query_win.h (right): https://codereview.chromium.org/1439443002/diff/180001/device/core/device_info_query_win.h#newcode30 device/core/device_info_query_win.h:30: if (device_info_list_ != INVALID_HANDLE_VALUE) { On 2015/11/16 21:41:56, grt ...
5 years, 1 month ago (2015-11-16 23:37:51 UTC) #22
grt (UTC plus 2)
This looks much better, thanks. Please read http://www.chromium.org/developers/coding-style/cpp-dos-and-donts#TOC-Stop-inlining-code-in-headers and then move the non-trivial impl of ...
5 years, 1 month ago (2015-11-17 16:06:56 UTC) #23
juncai
https://codereview.chromium.org/1439443002/diff/220001/device/core/device_info_query_win.h File device/core/device_info_query_win.h (right): https://codereview.chromium.org/1439443002/diff/220001/device/core/device_info_query_win.h#newcode78 device/core/device_info_query_win.h:78: HDEVINFO device_info_list() { return device_info_list_; } On 2015/11/17 16:06:56, ...
5 years, 1 month ago (2015-11-17 19:14:07 UTC) #24
grt (UTC plus 2)
the change i've been commenting on lgtm, although i'd prefer some doc comments.
5 years, 1 month ago (2015-11-17 19:29:09 UTC) #25
juncai
On 2015/11/17 19:29:09, grt wrote: > the change i've been commenting on lgtm, although i'd ...
5 years, 1 month ago (2015-11-17 21:50:21 UTC) #26
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1439443002/260001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1439443002/260001
5 years, 1 month ago (2015-11-17 22:50:00 UTC) #29
commit-bot: I haz the power
Committed patchset #14 (id:260001)
5 years, 1 month ago (2015-11-17 23:08:53 UTC) #30
commit-bot: I haz the power
Patchset 14 (id:??) landed as https://crrev.com/195a0f202c1b89540d4a385d881cd483abe757fa Cr-Commit-Position: refs/heads/master@{#360193}
5 years, 1 month ago (2015-11-17 23:09:57 UTC) #31
hcarmona
A revert of this CL (patchset #14 id:260001) has been created in https://codereview.chromium.org/1456793002/ by hcarmona@chromium.org. ...
5 years, 1 month ago (2015-11-17 23:50:16 UTC) #32
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1439443002/300001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1439443002/300001
5 years, 1 month ago (2015-11-18 16:44:21 UTC) #36
commit-bot: I haz the power
Committed patchset #16 (id:300001)
5 years, 1 month ago (2015-11-18 16:50:09 UTC) #37
commit-bot: I haz the power
Patchset 16 (id:??) landed as https://crrev.com/8d09d9564ea10012d55ed634c0c2835efbd8d01f Cr-Commit-Position: refs/heads/master@{#360347}
5 years, 1 month ago (2015-11-18 16:51:11 UTC) #38
alph
A revert of this CL (patchset #16 id:300001) has been created in https://codereview.chromium.org/1460743002/ by alph@chromium.org. ...
5 years, 1 month ago (2015-11-18 17:30:32 UTC) #39
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1439443002/320001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1439443002/320001
5 years, 1 month ago (2015-11-21 05:50:27 UTC) #43
commit-bot: I haz the power
Committed patchset #17 (id:320001)
5 years, 1 month ago (2015-11-21 05:54:34 UTC) #44
commit-bot: I haz the power
5 years, 1 month ago (2015-11-21 05:55:37 UTC) #45
Message was sent while issue was closed.
Patchset 17 (id:??) landed as
https://crrev.com/0c9eb038cae670b299c303a5e18547ec85e6e3b0
Cr-Commit-Position: refs/heads/master@{#361009}

Powered by Google App Engine
This is Rietveld 408576698