Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
727443002:
[content/common] Convert VLOGs to DVLOGs (Closed)
|
Created:
6 years, 1 month ago by anujsharma Modified:
6 years, 1 month ago CC:
chromium-reviews, posciak+watch_chromium.org, jam, mcasas+watch_chromium.org, sievers+watch_chromium.org, jbauman+watch_chromium.org, feature-media-reviews_chromium.org, darin-cc_chromium.org, kalyank, piman+watch_chromium.org, danakj+watch_chromium.org, wjia+watch_chromium.org, jln+watch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Project:
chromium Visibility:
Public. |
Description[content/common] Convert VLOGs to DVLOGs
BUG=101424
Committed: https://crrev.com/ba71aa04f3836288ac8655a325c5f4df90114012
Cr-Commit-Position: refs/heads/master@{#305198}
Patch Set 1 #
Total comments: 2
Patch Set 2 : Updated comments #
Total comments: 3
Patch Set 3 : Fixed comments #
Total comments: 1
Patch Set 4 : Fixed comments #
Messages
Total messages: 28 (9 generated)
anujk.sharma@samsung.com changed reviewers: + avi@chromium.org
PTAL
posciak@chromium.org changed reviewers: + posciak@chromium.org
https://codereview.chromium.org/727443002/diff/1/content/common/gpu/media/gpu... File content/common/gpu/media/gpu_video_decode_accelerator.cc (right): https://codereview.chromium.org/727443002/diff/1/content/common/gpu/media/gpu... content/common/gpu/media/gpu_video_decode_accelerator.cc:277: DVLOG(1) << "HW video decode acceleration not available without " I would strongly prefer this to remain as VLOG(). This is intentional and a critical error.
On 2014/11/13 10:27:51, Pawel Osciak wrote: > https://codereview.chromium.org/727443002/diff/1/content/common/gpu/media/gpu... > File content/common/gpu/media/gpu_video_decode_accelerator.cc (right): > > https://codereview.chromium.org/727443002/diff/1/content/common/gpu/media/gpu... > content/common/gpu/media/gpu_video_decode_accelerator.cc:277: DVLOG(1) << "HW > video decode acceleration not available without " > I would strongly prefer this to remain as VLOG(). This is intentional and a > critical error. Ok Pawel, i will revert this "content/common/gpu/media/gpu_video_decode_accelerator.cc" change to VLOG. Thanks for reviewing this.
Dropped that file, by keeping VLOG Thanks!! https://codereview.chromium.org/727443002/diff/1/content/common/gpu/media/gpu... File content/common/gpu/media/gpu_video_decode_accelerator.cc (right): https://codereview.chromium.org/727443002/diff/1/content/common/gpu/media/gpu... content/common/gpu/media/gpu_video_decode_accelerator.cc:277: DVLOG(1) << "HW video decode acceleration not available without " On 2014/11/13 10:27:51, Pawel Osciak wrote: > I would strongly prefer this to remain as VLOG(). This is intentional and a > critical error. Done.
jln@google.com changed reviewers: + jln@google.com
https://codereview.chromium.org/727443002/diff/20001/content/common/sandbox_l... File content/common/sandbox_linux/sandbox_linux.cc (right): https://codereview.chromium.org/727443002/diff/20001/content/common/sandbox_l... content/common/sandbox_linux/sandbox_linux.cc:62: DVLOG(1) << activated_sandbox; Why this change? We like being able to see this in release mode, especially when asking users to help track down bug reports. Why is this needed?
Thanks jin for your feedback. https://codereview.chromium.org/727443002/diff/20001/content/common/sandbox_l... File content/common/sandbox_linux/sandbox_linux.cc (right): https://codereview.chromium.org/727443002/diff/20001/content/common/sandbox_l... content/common/sandbox_linux/sandbox_linux.cc:62: DVLOG(1) << activated_sandbox; On 2014/11/13 14:11:57, jln (DO NOT USE THIS) wrote: > Why this change? We like being able to see this in release mode, especially when > asking users to help track down bug reports. > > Why is this needed? Thanks for your review. I agreed for release mode, VLOG is required. Sorry for overlooking this. I will drop this change. https://codereview.chromium.org/727443002/diff/20001/content/common/sandbox_l... content/common/sandbox_linux/sandbox_linux.cc:156: DVLOG(1) << "Lacking support for seccomp-bpf sandbox."; I hope this change looks cool. Please correct me if i am wrong.
anujk.sharma@samsung.com changed reviewers: + jln@chromium.org
@jln, Pawel, Avi- PTAL
On 2014/11/14 13:48:10, anujsharma wrote: > @jln, Pawel, Avi- PTAL I'm OK with it for the plugin file. You need to get OKs from the security people for the sandbox file, and from the media people for the media code.
anujk.sharma@samsung.com changed reviewers: + xhwang@chromium.org
On 2014/11/14 16:12:41, Avi wrote: > On 2014/11/14 13:48:10, anujsharma wrote: > > @jln, Pawel, Avi- PTAL > > I'm OK with it for the plugin file. You need to get OKs from the security people > for the sandbox file, and from the media people for the media code. Thanks Avi for your time and suggestion. @xhwang- could you please review media related changes. @jlb- could you please give a thumbs up for sandbox related changes. Thanks!!
media lgtm
On 2014/11/14 18:13:01, xhwang wrote: > media lgtm Oh, officially, content/common/pepper_plugin_list.cc LGTM
content/common/gpu/media/vaapi_h264_decoder_unittest.cc LGTM
anujk.sharma@samsung.com changed reviewers: + jbauman@chromium.org
Thanks xhwang, Avi and Pawel for lgtm. @jbauman- could you please review content/common/gpu changes.
@jln and jbauman PTAL thanks!!
Sorry for the very long delay :( https://codereview.chromium.org/727443002/diff/40001/content/common/sandbox_l... File content/common/sandbox_linux/sandbox_linux.cc (right): https://codereview.chromium.org/727443002/diff/40001/content/common/sandbox_l... content/common/sandbox_linux/sandbox_linux.cc:156: DVLOG(1) << "Lacking support for seccomp-bpf sandbox."; I would rather not have this change either. Both of these messages are very useful to debug issues in the field with out users. Could you get rid of it?
On 2014/11/21 02:07:15, jln wrote: > Sorry for the very long delay :( > > https://codereview.chromium.org/727443002/diff/40001/content/common/sandbox_l... > File content/common/sandbox_linux/sandbox_linux.cc (right): > > https://codereview.chromium.org/727443002/diff/40001/content/common/sandbox_l... > content/common/sandbox_linux/sandbox_linux.cc:156: DVLOG(1) << "Lacking support > for seccomp-bpf sandbox."; > I would rather not have this change either. > > Both of these messages are very useful to debug issues in the field with out > users. Could you get rid of it? thanks jin for your time and review. Removed as per your suggestion :)
anujk.sharma@samsung.com changed reviewers: - jbauman@chromium.org
anujk.sharma@samsung.com changed reviewers: - jln@google.com
The CQ bit was checked by anujk.sharma@samsung.com
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/727443002/60001
Message was sent while issue was closed.
Committed patchset #4 (id:60001)
Message was sent while issue was closed.
Patchset 4 (id:??) landed as https://crrev.com/ba71aa04f3836288ac8655a325c5f4df90114012 Cr-Commit-Position: refs/heads/master@{#305198} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
