Address presubmit warnings in //content/renderer/media/webrtc |
||||||
Issue descriptionr535775 moved a few files from //content/renderer/media to //content/renderer/media/webrtc `git cl upload` identified inconsistencies, probably modern style nits etc. Someone should do something :-) and, it's probably a GoodfirstBug ! ** Presubmit Warnings ** Banned functions were used. content/renderer/media/processed_local_audio_source.h:115: Consider using THREAD_CHECKER macros instead of the class directly. content/renderer/media/processed_local_audio_source_unittest.cc:106: Please consider using base::Bind{Once,Repeating} instead of base::Bind. (crbug.com/714018) content/renderer/media/webrtc/peer_connection_tracker.h:229: Consider using THREAD_CHECKER macros instead of the class directly. content/renderer/media/webrtc/rtc_data_channel_handler.h:105: Consider using THREAD_CHECKER macros instead of the class directly. content/renderer/media/webrtc/rtc_dtmf_sender_handler.cc:47: Consider using THREAD_CHECKER macros instead of the class directly. content/renderer/media/webrtc/rtc_peer_connection_handler.cc:208: Please consider using base::{Once,Repeating}Closure instead of base::Closure. (crbug.com/714018) content/renderer/media/webrtc/rtc_peer_connection_handler.cc:214: Please consider using base::{Once,Repeating}Closure instead of base::Closure. (crbug.com/714018) content/renderer/media/webrtc/rtc_peer_connection_handler.cc:225: Please consider using base::{Once,Repeating}Callback instead of base::Callback. (crbug.com/714018) content/renderer/media/webrtc/rtc_peer_connection_handler.cc:413: Consider using THREAD_CHECKER macros instead of the class directly. content/renderer/media/webrtc/rtc_peer_connection_handler.cc:656: Consider using THREAD_CHECKER macros instead of the class directly. content/renderer/media/webrtc/rtc_peer_connection_handler.cc:696: Consider using THREAD_CHECKER macros instead of the class directly. content/renderer/media/webrtc/rtc_peer_connection_handler.cc:1556: Please consider using base::Bind{Once,Repeating} instead of base::Bind. (crbug.com/714018) content/renderer/media/webrtc/rtc_peer_connection_handler.cc:1620: Please consider using base::Bind{Once,Repeating} instead of base::Bind. (crbug.com/714018) content/renderer/media/webrtc/rtc_peer_connection_handler.cc:1640: Please consider using base::{Once,Repeating}Callback instead of base::Callback. (crbug.com/714018) content/renderer/media/webrtc/rtc_peer_connection_handler.cc:1641: Please consider using base::Bind{Once,Repeating} instead of base::Bind. (crbug.com/714018) content/renderer/media/webrtc/rtc_peer_connection_handler.cc:1644: Please consider using base::Bind{Once,Repeating} instead of base::Bind. (crbug.com/714018) content/renderer/media/webrtc/rtc_peer_connection_handler.cc:1659: Please consider using base::{Once,Repeating}Callback instead of base::Callback. (crbug.com/714018) content/renderer/media/webrtc/rtc_peer_connection_handler.cc:1660: Please consider using base::Bind{Once,Repeating} instead of base::Bind. (crbug.com/714018) content/renderer/media/webrtc/rtc_peer_connection_handler.cc:1663: Please consider using base::Bind{Once,Repeating} instead of base::Bind. (crbug.com/714018) content/renderer/media/webrtc/rtc_peer_connection_handler.cc:2384: Please consider using base::{Once,Repeating}Closure instead of base::Closure. (crbug.com/714018) content/renderer/media/webrtc/rtc_peer_connection_handler.h:254: Please consider using base::{Once,Repeating}Closure instead of base::Closure. (crbug.com/714018) content/renderer/media/webrtc/rtc_peer_connection_handler.h:261: Consider using THREAD_CHECKER macros instead of the class directly. content/renderer/media/webrtc/rtc_peer_connection_handler_unittest.cc:315: Please consider using base::Bind{Once,Repeating} instead of base::Bind. (crbug.com/714018) content/renderer/media/webrtc/rtc_peer_connection_handler_unittest.cc:406: Please consider using base::Bind{Once,Repeating} instead of base::Bind. (crbug.com/714018) content/renderer/media/webrtc/rtc_peer_connection_handler_unittest.cc:427: Please consider using base::Bind{Once,Repeating} instead of base::Bind. (crbug.com/714018) content/renderer/media/webrtc/rtc_peer_connection_handler_unittest.cc:435: Please consider using base::Bind{Once,Repeating} instead of base::Bind. (crbug.com/714018) content/renderer/media/webrtc/rtc_peer_connection_handler_unittest.cc:446: Please consider using base::Bind{Once,Repeating} instead of base::Bind. (crbug.com/714018) content/renderer/media/webrtc/rtc_peer_connection_handler_unittest.cc:1288: Please consider using base::Bind{Once,Repeating} instead of base::Bind. (crbug.com/714018) content/renderer/media/webrtc/rtc_peer_connection_handler_unittest.cc:1295: Please consider using base::Bind{Once,Repeating} instead of base::Bind. (crbug.com/714018) content/renderer/media/webrtc/rtc_video_decoder.cc:448: base::TimeXXX::FromInternalValue() and ToInternalValue() are deprecated (http://crbug.com/634507). Please avoid converting away from the Time types in Chromium code, especially if any math is being done on time values. For interfacing with platform/library APIs, use FromMicroseconds() or InMicroseconds(), or one of the other type converter methods instead. For faking TimeXXX values (for unit testing only), use TimeXXX() + TimeDelta::FromMicroseconds(N). For other use cases, please contact base/time/OWNERS. content/renderer/media/webrtc/rtc_video_decoder.cc:465: Please consider using base::Bind{Once,Repeating} instead of base::Bind. (crbug.com/714018) content/renderer/media/webrtc/rtc_video_decoder.cc:569: base::TimeXXX::FromInternalValue() and ToInternalValue() are deprecated (http://crbug.com/634507). Please avoid converting away from the Time types in Chromium code, especially if any math is being done on time values. For interfacing with platform/library APIs, use FromMicroseconds() or InMicroseconds(), or one of the other type converter methods instead. For faking TimeXXX values (for unit testing only), use TimeXXX() + TimeDelta::FromMicroseconds(N). For other use cases, please contact base/time/OWNERS. content/renderer/media/webrtc/rtc_video_encoder.cc:214: Consider using THREAD_CHECKER macros instead of the class directly. content/renderer/media/webrtc/rtc_video_encoder.cc:662: Please consider using base::Bind{Once,Repeating} instead of base::Bind. (crbug.com/714018) content/renderer/media/webrtc/rtc_video_encoder_unittest.cc:43: Please consider using base::{Once,Repeating}Callback instead of base::Callback. (crbug.com/714018) content/renderer/media/webrtc/rtc_video_encoder_unittest.cc:306: Please consider using base::Bind{Once,Repeating} instead of base::Bind. (crbug.com/714018) content/renderer/media/webrtc/webrtc_audio_device_impl.h:208: Consider using THREAD_CHECKER macros instead of the class directly. content/renderer/media/webrtc/webrtc_audio_device_impl.h:210: Consider using THREAD_CHECKER macros instead of the class directly. content/renderer/media/webrtc/webrtc_audio_device_impl.h:211: Consider using THREAD_CHECKER macros instead of the class directly. content/renderer/media/webrtc/webrtc_audio_device_impl.h:212: Consider using THREAD_CHECKER macros instead of the class directly. content/renderer/media/webrtc/webrtc_audio_renderer.cc:58: Please consider using base::{Once,Repeating}Callback instead of base::Callback. (crbug.com/714018) content/renderer/media/webrtc/webrtc_audio_renderer.cc:153: Consider using THREAD_CHECKER macros instead of the class directly. content/renderer/media/webrtc/webrtc_audio_renderer.cc:233: Please consider using base::Bind{Once,Repeating} instead of base::Bind. (crbug.com/714018) content/renderer/media/webrtc/webrtc_audio_renderer.cc:437: Please consider using base::Bind{Once,Repeating} instead of base::Bind. (crbug.com/714018) content/renderer/media/webrtc/webrtc_audio_renderer.cc:689: Please consider using base::Bind{Once,Repeating} instead of base::Bind. (crbug.com/714018) content/renderer/media/webrtc/webrtc_audio_renderer.h:161: Consider using THREAD_CHECKER macros instead of the class directly. content/renderer/media/webrtc/webrtc_audio_renderer_unittest.cc:248: Please consider using base::Bind{Once,Repeating} instead of base::Bind. (crbug.com/714018) content/renderer/media/webrtc/webrtc_audio_renderer_unittest.cc:273: Please consider using base::Bind{Once,Repeating} instead of base::Bind. (crbug.com/714018) content/renderer/media/processed_local_audio_source.cc:274 OS_MACOSX macro is used without including build/build_config.h. content/renderer/media/webrtc/peer_connection_tracker.cc:415 OS_ANDROID macro is used without including build/build_config.h. content/renderer/media/webrtc/rtc_video_decoder.cc:27 OS_WIN macro is used without including build/build_config.h. content/renderer/media/webrtc/rtc_video_decoder_unittest.cc:21 OS_WIN macro is used without including build/build_config.h. content/renderer/media/webrtc/webrtc_audio_device_not_impl.cc:164 OS_IOS macro is used without including build/build_config.h. content/renderer/media/webrtc/webrtc_audio_device_not_impl.h:74 OS_IOS macro is used without including build/build_config.h. The src directory requires source formatting. Please run: git cl format Found unprefixed crbug.com URL(s), consider prepending https:// content/renderer/media/webrtc/rtc_peer_connection_handler.h:294 // crbug.com/705901 content/renderer/media/webrtc/webrtc_audio_renderer.cc:613 // crbug.com/697256.
,
Feb 9 2018
,
Feb 10 2018
I'd like to patch it. Will I just run 'git cl format' or modify clang-format rule to solve warnings? In my local, latest version of master(Sat Feb 10 12:05:18 2018 +0000) presubmit checks passed. What should I do to patch and contribute?
,
Feb 11 2018
bbvch13531@, first, thanks for volunteering! Make yourself familiar with http://dev.chromium.org/developers/contributing-code commit your changes in a local branch and upload for review. In particular check out the ICLA thing. Note that `git cl format` would only cover one of the presubmit warnings -- this is fine for a single CL, but just saying. Including build/build_config.h and prefixing https:// in the crbug.com's look easy enough that can be addressed at the same time. Using THREAD_CHECKER macros is also a quite straightforward 1:1 substitution, but I'd do it in a subsequent CL. Correcting base::Bind/base::Callback uses to their Once/Repeating variants needs a more nuanced approach and in some cases changing member variables and method signatures.
,
Feb 12 2018
,
Feb 15 2018
,
Aug 29
While working with this issue, there are some ambiguous things to handle by myself. rtc_peer_connection_handler.h has two methods void RunSynchronousClosureOnSignalingThread(const base::Closure& closure, const char* trace_event_name); and void RunSynchronousOnceClosureOnSignalingThread(base::OnceClosure closure, const char* trace_event_name); I'm gonna change RunSynchronousClosureOnSignalingThread's first parameter 'const base::Closure& closure' to 'base::OnceClosure closure' but RunSynchronousOnceClosureOnSignalingThread is already using 'base::OnceClosure closure'. I think remove RunSynchronousClosureOnSignalingThread methods looks fine. How you think about it?
,
Aug 29
#7: probably good, pending the actual CL, guidou@ might know more.
,
Aug 29
It seems that all call sites pass a temporary repeating closure to RunSynchronousClosureOnSignalingThread(), so it's probably OK to remove the first one. cc hbos@ for input, since he knows this code best.
,
Aug 29
base::BindOnce should be fine in all the cases I'm aware of, it's just PostTask stuff using the old way of binding. base::BindOnce would allow us to std::move arguments properly if applicable.
,
Oct 13
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/1f2ac8d36b6121c66449a58e15c25c022d44180c commit 1f2ac8d36b6121c66449a58e15c25c022d44180c Author: bbvch13531 <bbvch13531@gmail.com> Date: Sat Oct 13 03:32:23 2018 Refactor base::ThreadChecker to THREAD_CHECKER macros. This CL covers presubmit warning due to base::ThreadChecker. Bug: 810841 Change-Id: I2b8718c33a4cb2c38e5d5a7a96387d904dc659d3 Reviewed-on: https://chromium-review.googlesource.com/c/1191342 Reviewed-by: Guido Urdaneta <guidou@chromium.org> Commit-Queue: Jinho Bang <jinho.bang@samsung.com> Cr-Commit-Position: refs/heads/master@{#599481} [modify] https://crrev.com/1f2ac8d36b6121c66449a58e15c25c022d44180c/content/renderer/media/webrtc/peer_connection_tracker.cc [modify] https://crrev.com/1f2ac8d36b6121c66449a58e15c25c022d44180c/content/renderer/media/webrtc/peer_connection_tracker.h [modify] https://crrev.com/1f2ac8d36b6121c66449a58e15c25c022d44180c/content/renderer/media/webrtc/rtc_data_channel_handler.cc [modify] https://crrev.com/1f2ac8d36b6121c66449a58e15c25c022d44180c/content/renderer/media/webrtc/rtc_data_channel_handler.h [modify] https://crrev.com/1f2ac8d36b6121c66449a58e15c25c022d44180c/content/renderer/media/webrtc/rtc_dtmf_sender_handler.cc [modify] https://crrev.com/1f2ac8d36b6121c66449a58e15c25c022d44180c/content/renderer/media/webrtc/rtc_peer_connection_handler.cc [modify] https://crrev.com/1f2ac8d36b6121c66449a58e15c25c022d44180c/content/renderer/media/webrtc/rtc_video_encoder.cc [modify] https://crrev.com/1f2ac8d36b6121c66449a58e15c25c022d44180c/content/renderer/media/webrtc/webrtc_audio_device_impl.cc [modify] https://crrev.com/1f2ac8d36b6121c66449a58e15c25c022d44180c/content/renderer/media/webrtc/webrtc_audio_device_impl.h [modify] https://crrev.com/1f2ac8d36b6121c66449a58e15c25c022d44180c/content/renderer/media/webrtc/webrtc_audio_renderer.cc [modify] https://crrev.com/1f2ac8d36b6121c66449a58e15c25c022d44180c/content/renderer/media/webrtc/webrtc_audio_renderer.h
,
Oct 19
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/1e30e78e5ae40b7fe1124ce6763d5ad1ad0b3ed9 commit 1e30e78e5ae40b7fe1124ce6763d5ad1ad0b3ed9 Author: Kyungyoung Heo <bbvch13531@gmail.com> Date: Fri Oct 19 20:35:55 2018 Add build/build_config.h to using OS macro Add build/build_config.h to using OS macro without presubmit warning. Bug: 810841 Change-Id: I81fc8bfdbcb539983d17b72d6541ee4119ea9410 Reviewed-on: https://chromium-review.googlesource.com/c/1278457 Commit-Queue: Jinho Bang <jinho.bang@samsung.com> Reviewed-by: Guido Urdaneta <guidou@chromium.org> Reviewed-by: Miguel Casas <mcasas@chromium.org> Cr-Commit-Position: refs/heads/master@{#601283} [modify] https://crrev.com/1e30e78e5ae40b7fe1124ce6763d5ad1ad0b3ed9/content/renderer/media/webrtc/peer_connection_tracker.cc [modify] https://crrev.com/1e30e78e5ae40b7fe1124ce6763d5ad1ad0b3ed9/content/renderer/media/webrtc/webrtc_audio_device_not_impl.h
,
Nov 19
|
||||||
►
Sign in to add a comment |
||||||
Comment 1 by mcasas@chromium.org
, Feb 9 2018