Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2799743006:
Remove MessageLoop destruction observer from hid_connection_linux.cc (Closed)
|
Created:
3 years, 8 months ago by Reilly Grant (use Gerrit) Modified:
3 years, 6 months ago CC:
chromium-reviews Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
DescriptionRemove MessageLoop destruction observer from hid_connection_linux.cc
This patch makes HidConnectionLinux::FileThreadHelper to no longer a
MessageLoop::DestructionObserver and cleans up ownership of the device
file descriptor.
BUG=650723
Review-Url: https://codereview.chromium.org/2799743006
Cr-Commit-Position: refs/heads/master@{#463103}
Committed: https://chromium.googlesource.com/chromium/src/+/6a2cc0923f353e6ab781ba7ef21b664106145c1a
Patch Set 1 #
Total comments: 25
Patch Set 2 : Addressed feedback #
Total comments: 2
Patch Set 3 : Last review comments #
Messages
Total messages: 25 (14 generated)
reillyg@chromium.org changed reviewers: + robliao@chromium.org
PTAL
robliao@chromium.org changed reviewers: + fdoray@chromium.org
Adding fdoray@ who did a lot of the MessageLoop destruction observer removals.
Task usage looks okay. I'm not familiar with the posix IO behavior, so I didn't look at that too closely. Some questions and comments. https://codereview.chromium.org/2799743006/diff/1/device/hid/hid_connection_l... File device/hid/hid_connection_linux.cc (right): https://codereview.chromium.org/2799743006/diff/1/device/hid/hid_connection_l... device/hid/hid_connection_linux.cc:39: scoped_refptr<base::SingleThreadTaskRunner> task_runner) Does this have to be a SingleThreadTaskRunner or can SequenceTaskRunner also do the job (if STR is okay, this change can be separate from this CL). It seems like it just pulls it from the current thread task runner handle, which I presume is the UI thread? https://codereview.chromium.org/2799743006/diff/1/device/hid/hid_connection_l... device/hid/hid_connection_linux.cc:52: sequence_checker_.DetachFromSequence(); I think you can move this call to the constructor since all subsequent FileThreadHelper calls occur on the blocking task runner. After that, you can DCHECK that we're called on a valid sequence here and subsequent calls will be checked against this sequence. https://codereview.chromium.org/2799743006/diff/1/device/hid/hid_connection_l... device/hid/hid_connection_linux.cc:184: // By closing the device on the FILE thread 1) the requirement that s/FILE thread/blocking sequenced task runner/ https://codereview.chromium.org/2799743006/diff/1/device/hid/hid_service_linu... File device/hid/hid_service_linux.cc (right): https://codereview.chromium.org/2799743006/diff/1/device/hid/hid_service_linu... device/hid/hid_service_linux.cc:67: scoped_refptr<base::SequencedTaskRunner> file_task_runner; Rename to blocking_task_runner to harmonize with HidConnectionLinux. Same with below.
The CQ bit was checked by fdoray@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
https://codereview.chromium.org/2799743006/diff/1/device/hid/hid_connection_l... File device/hid/hid_connection_linux.cc (right): https://codereview.chromium.org/2799743006/diff/1/device/hid/hid_connection_l... device/hid/hid_connection_linux.cc:39: scoped_refptr<base::SingleThreadTaskRunner> task_runner) s/task_runner/origin_task_runner/? https://codereview.chromium.org/2799743006/diff/1/device/hid/hid_connection_l... device/hid/hid_connection_linux.cc:68: if (static_cast<size_t>(result) != size) { no braces https://codereview.chromium.org/2799743006/diff/1/device/hid/hid_connection_l... device/hid/hid_connection_linux.cc:170: blocking_task_runner_(blocking_task_runner), std::move(blocking_task_runner) https://codereview.chromium.org/2799743006/diff/1/device/hid/hid_connection_l... device/hid/hid_connection_linux.cc:172: helper_.reset(new FileThreadHelper(std::move(fd), device_info, MakeUnique https://codereview.chromium.org/2799743006/diff/1/device/hid/hid_connection_l... File device/hid/hid_connection_linux.h (right): https://codereview.chromium.org/2799743006/diff/1/device/hid/hid_connection_l... device/hid/hid_connection_linux.h:53: int fd_; // Copy of the file descriptor owned by |helper_|. |fd_| isn't used. https://codereview.chromium.org/2799743006/diff/1/device/hid/hid_connection_l... device/hid/hid_connection_linux.h:56: scoped_refptr<base::SingleThreadTaskRunner> task_runner_; const scoped_refptr https://codereview.chromium.org/2799743006/diff/1/device/hid/hid_connection_l... device/hid/hid_connection_linux.h:57: scoped_refptr<base::SequencedTaskRunner> blocking_task_runner_; const scoped_refptr https://codereview.chromium.org/2799743006/diff/1/device/hid/hid_connection_l... device/hid/hid_connection_linux.h:62: base::ThreadChecker thread_checker_; missing include
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Addressed feedback
https://codereview.chromium.org/2799743006/diff/1/device/hid/hid_connection_l... File device/hid/hid_connection_linux.cc (right): https://codereview.chromium.org/2799743006/diff/1/device/hid/hid_connection_l... device/hid/hid_connection_linux.cc:39: scoped_refptr<base::SingleThreadTaskRunner> task_runner) On 2017/04/07 01:08:24, robliao wrote: > Does this have to be a SingleThreadTaskRunner or can SequenceTaskRunner also do > the job (if STR is okay, this change can be separate from this CL). > > It seems like it just pulls it from the current thread task runner handle, which > I presume is the UI thread? In the USB code SingleThreadTaskRunner is necessary for some historical reasons but here I can switch to SequencedTaskRunner, so I did. https://codereview.chromium.org/2799743006/diff/1/device/hid/hid_connection_l... device/hid/hid_connection_linux.cc:52: sequence_checker_.DetachFromSequence(); On 2017/04/07 01:08:24, robliao wrote: > I think you can move this call to the constructor since all subsequent > FileThreadHelper calls occur on the blocking task runner. > > After that, you can DCHECK that we're called on a valid sequence here and > subsequent calls will be checked against this sequence. Done. https://codereview.chromium.org/2799743006/diff/1/device/hid/hid_connection_l... device/hid/hid_connection_linux.cc:68: if (static_cast<size_t>(result) != size) { On 2017/04/07 20:26:23, fdoray wrote: > no braces Done. https://codereview.chromium.org/2799743006/diff/1/device/hid/hid_connection_l... device/hid/hid_connection_linux.cc:170: blocking_task_runner_(blocking_task_runner), On 2017/04/07 20:26:23, fdoray wrote: > std::move(blocking_task_runner) Done. https://codereview.chromium.org/2799743006/diff/1/device/hid/hid_connection_l... device/hid/hid_connection_linux.cc:172: helper_.reset(new FileThreadHelper(std::move(fd), device_info, On 2017/04/07 20:26:23, fdoray wrote: > MakeUnique Done. https://codereview.chromium.org/2799743006/diff/1/device/hid/hid_connection_l... device/hid/hid_connection_linux.cc:184: // By closing the device on the FILE thread 1) the requirement that On 2017/04/07 01:08:24, robliao wrote: > s/FILE thread/blocking sequenced task runner/ Done. https://codereview.chromium.org/2799743006/diff/1/device/hid/hid_connection_l... File device/hid/hid_connection_linux.h (right): https://codereview.chromium.org/2799743006/diff/1/device/hid/hid_connection_l... device/hid/hid_connection_linux.h:53: int fd_; // Copy of the file descriptor owned by |helper_|. On 2017/04/07 20:26:23, fdoray wrote: > |fd_| isn't used. Done. https://codereview.chromium.org/2799743006/diff/1/device/hid/hid_connection_l... device/hid/hid_connection_linux.h:56: scoped_refptr<base::SingleThreadTaskRunner> task_runner_; On 2017/04/07 20:26:23, fdoray wrote: > const scoped_refptr Done. https://codereview.chromium.org/2799743006/diff/1/device/hid/hid_connection_l... device/hid/hid_connection_linux.h:57: scoped_refptr<base::SequencedTaskRunner> blocking_task_runner_; On 2017/04/07 20:26:23, fdoray wrote: > const scoped_refptr Done. https://codereview.chromium.org/2799743006/diff/1/device/hid/hid_connection_l... device/hid/hid_connection_linux.h:62: base::ThreadChecker thread_checker_; On 2017/04/07 20:26:23, fdoray wrote: > missing include Done. https://codereview.chromium.org/2799743006/diff/1/device/hid/hid_service_linu... File device/hid/hid_service_linux.cc (right): https://codereview.chromium.org/2799743006/diff/1/device/hid/hid_service_linu... device/hid/hid_service_linux.cc:67: scoped_refptr<base::SequencedTaskRunner> file_task_runner; On 2017/04/07 01:08:24, robliao wrote: > Rename to blocking_task_runner to harmonize with HidConnectionLinux. Same with > below. Done.
The CQ bit was checked by reillyg@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
lgtm + 2 comments. https://codereview.chromium.org/2799743006/diff/1/device/hid/hid_connection_l... File device/hid/hid_connection_linux.cc (right): https://codereview.chromium.org/2799743006/diff/1/device/hid/hid_connection_l... device/hid/hid_connection_linux.cc:39: scoped_refptr<base::SingleThreadTaskRunner> task_runner) On 2017/04/07 22:10:48, Reilly Grant wrote: > On 2017/04/07 01:08:24, robliao wrote: > > Does this have to be a SingleThreadTaskRunner or can SequenceTaskRunner also > do > > the job (if STR is okay, this change can be separate from this CL). > > > > It seems like it just pulls it from the current thread task runner handle, > which > > I presume is the UI thread? > > In the USB code SingleThreadTaskRunner is necessary for some historical reasons > but here I can switch to SequencedTaskRunner, so I did. Ping for fdoray's comment: s/task_runner/origin_task_runner/ https://codereview.chromium.org/2799743006/diff/20001/device/hid/hid_connecti... File device/hid/hid_connection_linux.cc (right): https://codereview.chromium.org/2799743006/diff/20001/device/hid/hid_connecti... device/hid/hid_connection_linux.cc:55: base::ThreadRestrictions::AssertIOAllowed(); You still want to add DCHECK(sequence_checker_.CalledOnValidSequence()) to set the sequence for the sequence checker.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Last review comments
https://codereview.chromium.org/2799743006/diff/1/device/hid/hid_connection_l... File device/hid/hid_connection_linux.cc (right): https://codereview.chromium.org/2799743006/diff/1/device/hid/hid_connection_l... device/hid/hid_connection_linux.cc:39: scoped_refptr<base::SingleThreadTaskRunner> task_runner) On 2017/04/07 23:24:45, robliao wrote: > On 2017/04/07 22:10:48, Reilly Grant wrote: > > On 2017/04/07 01:08:24, robliao wrote: > > > Does this have to be a SingleThreadTaskRunner or can SequenceTaskRunner also > > do > > > the job (if STR is okay, this change can be separate from this CL). > > > > > > It seems like it just pulls it from the current thread task runner handle, > > which > > > I presume is the UI thread? > > > > In the USB code SingleThreadTaskRunner is necessary for some historical > reasons > > but here I can switch to SequencedTaskRunner, so I did. > > Ping for fdoray's comment: > s/task_runner/origin_task_runner/ Done. https://codereview.chromium.org/2799743006/diff/20001/device/hid/hid_connecti... File device/hid/hid_connection_linux.cc (right): https://codereview.chromium.org/2799743006/diff/20001/device/hid/hid_connecti... device/hid/hid_connection_linux.cc:55: base::ThreadRestrictions::AssertIOAllowed(); On 2017/04/07 23:24:45, robliao wrote: > You still want to add DCHECK(sequence_checker_.CalledOnValidSequence()) to set > the sequence for the sequence checker. Done.
The CQ bit was checked by reillyg@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from robliao@chromium.org Link to the patchset: https://codereview.chromium.org/2799743006/#ps40001 (title: "Last review comments")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 40001, "attempt_start_ts": 1491613158119140,
"parent_rev": "6ddf4ad01151a7bd788db7bd9b985b8fc7c008d7", "commit_rev":
"6a2cc0923f353e6ab781ba7ef21b664106145c1a"}
Message was sent while issue was closed.
Description was changed from ========== Remove MessageLoop destruction observer from hid_connection_linux.cc This patch makes HidConnectionLinux::FileThreadHelper to no longer a MessageLoop::DestructionObserver and cleans up ownership of the device file descriptor. BUG=650723 ========== to ========== Remove MessageLoop destruction observer from hid_connection_linux.cc This patch makes HidConnectionLinux::FileThreadHelper to no longer a MessageLoop::DestructionObserver and cleans up ownership of the device file descriptor. BUG=650723 Review-Url: https://codereview.chromium.org/2799743006 Cr-Commit-Position: refs/heads/master@{#463103} Committed: https://chromium.googlesource.com/chromium/src/+/6a2cc0923f353e6ab781ba7ef21b... ==========
Message was sent while issue was closed.
Committed patchset #3 (id:40001) as https://chromium.googlesource.com/chromium/src/+/6a2cc0923f353e6ab781ba7ef21b... |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
