Chromium Code Reviews
Help | Chromium Project | Gerrit Changes | Sign in
(1)

Issue 2891983002: Make AudioDebugFileWriter::AudioFileWriter use a SequencedTaskRunner (Closed)

Created:
6 months ago by Sébastien Marchand
Modified:
6 months ago
CC:
chromium-reviews, feature-media-reviews_chromium.org
Target Ref:
refs/heads/master
Project:
chromium
Visibility:
Public.

Description

Make AudioDebugFileWriter::AudioFileWriter use a SequencedTaskRunner Quoting gab@ from https://codereview.chromium.org/2885173002/diff/40001/media/audio/audio_debug_file_writer.h: AudioFileWriter::AudioFileWriter() merely takes the task runner to assert it's running on it, that can easily be turned into a SequencedTaskRunner but even better than that would be to have it use a SequenceChecker to check that all of its methods run in sequence (will need to DetachFromSequence in AudioFileWriter::AudioFileWriter() as it's not constructed on same sequence it's used -- see sequence_checker.h for details). SequenceChecker + implicit AssertIOAllowed() calls from using base file methods guarantees this class is being used in sequence with MayBlock() trait. That's preferred and cleaner than taking a task_runner just to assume running on it. Then there's OnThreadDeleter which could easily be made OnSequenceDeleter + SequenceTaskRunner. BUG=689520 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_optional_gpu_tests_rel;master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel Review-Url: https://codereview.chromium.org/2891983002 Cr-Commit-Position: refs/heads/master@{#473264} Committed: https://chromium.googlesource.com/chromium/src/+/45167d8df4bc03bdbecbc9009901528ce9349226

Patch Set 1 #

Total comments: 6

Patch Set 2 : Address Gab's comments #

Patch Set 3 : make the style checker happy. #

Total comments: 2

Patch Set 4 : move semantic #

Messages

Total messages: 32 (24 generated)
gab
lgtm w/ nit and suggestions for more cleanup :) https://codereview.chromium.org/2891983002/diff/1/media/audio/audio_debug_file_writer.cc File media/audio/audio_debug_file_writer.cc (right): https://codereview.chromium.org/2891983002/diff/1/media/audio/audio_debug_file_writer.cc#newcode189 media/audio/audio_debug_file_writer.cc:189: ...
6 months ago (2017-05-18 20:38:17 UTC) #8
Sébastien Marchand
+ henrika/tommi for owner approval. https://codereview.chromium.org/2891983002/diff/1/media/audio/audio_debug_file_writer.cc File media/audio/audio_debug_file_writer.cc (right): https://codereview.chromium.org/2891983002/diff/1/media/audio/audio_debug_file_writer.cc#newcode189 media/audio/audio_debug_file_writer.cc:189: : task_runner_(task_runner) {} On ...
6 months ago (2017-05-18 21:24:25 UTC) #12
gab
https://codereview.chromium.org/2891983002/diff/40001/media/audio/audio_debug_file_writer.h File media/audio/audio_debug_file_writer.h (right): https://codereview.chromium.org/2891983002/diff/40001/media/audio/audio_debug_file_writer.h#newcode73 media/audio/audio_debug_file_writer.h:73: OnSequenceDeleter& operator=(const OnSequenceDeleter&); Actually you want the matching move ...
6 months ago (2017-05-18 22:11:21 UTC) #17
Sébastien Marchand
https://codereview.chromium.org/2891983002/diff/40001/media/audio/audio_debug_file_writer.h File media/audio/audio_debug_file_writer.h (right): https://codereview.chromium.org/2891983002/diff/40001/media/audio/audio_debug_file_writer.h#newcode73 media/audio/audio_debug_file_writer.h:73: OnSequenceDeleter& operator=(const OnSequenceDeleter&); On 2017/05/18 22:11:21, gab wrote: > ...
6 months ago (2017-05-18 22:47:25 UTC) #18
Sébastien Marchand
+tguilbert@ for owner approval.
6 months ago (2017-05-19 14:48:43 UTC) #25
tguilbert
On 2017/05/19 14:48:43, Sébastien Marchand wrote: > +tguilbert@ for owner approval. media -- LGTM
6 months ago (2017-05-19 18:44:18 UTC) #26
commit-bot: I haz the power
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2891983002/60001
6 months ago (2017-05-19 18:53:31 UTC) #29
commit-bot: I haz the power
6 months ago (2017-05-19 18:59:54 UTC) #32
Message was sent while issue was closed.
Committed patchset #4 (id:60001) as
https://chromium.googlesource.com/chromium/src/+/45167d8df4bc03bdbecbc9009901...

Powered by Google App Engine
This is Rietveld efc10ee0f