Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2036583002:
Clear logging in Widevine CDM and default component installer (Closed)
DescriptionClear logging in Widevine CDM and default component installer
The bug has been fixed. Conventing LOG(WARNING)s into DLOG(WARNING)s.
BUG=614745
TEST=No functionality change.
Committed: https://crrev.com/c4f2d5df530b9e1139af40df5ba53bdca5502e7c
Cr-Commit-Position: refs/heads/master@{#397527}
Patch Set 1 #
Total comments: 4
Patch Set 2 : #
Total comments: 2
Patch Set 3 : #Patch Set 4 : DVLOG(1) everywhere #
Messages
Total messages: 22 (7 generated)
xhwang@chromium.org changed reviewers: + sorin@chromium.org, waffles@chromium.org
As promised. PTAL I found it awkward to use DLOG(WARNING) in some cases because it is really not an error case (e.g. when a component isn't bundled). But it seems nobody is using DLOG(INFO) today. Does it make sense to use DVLOG(n) instead of DLOGs?
thank you! https://chromiumcodereview.appspot.com/2036583002/diff/1/components/component... File components/component_updater/default_component_installer.cc (right): https://chromiumcodereview.appspot.com/2036583002/diff/1/components/component... components/component_updater/default_component_installer.cc:334: NOTREACHED() << "Component registration failed for " is this correct? Why assert on a condition that can be false at runtime?
Thanks for closing the loop. My question is, can we actually simply remove most this logging? Especially in DefaultComponentInstaller - is there a particular reason we need it around?
On 2016/06/02 00:37:52, waffles wrote: > Thanks for closing the loop. My question is, can we actually simply remove most > this logging? Especially in DefaultComponentInstaller - is there a particular > reason we need it around? I found these logs useful. Next time when someone tries to bundle a component these logs could be useful again. Since it's DLOG now it won't affect release builds.
https://chromiumcodereview.appspot.com/2036583002/diff/1/components/component... File components/component_updater/default_component_installer.cc (right): https://chromiumcodereview.appspot.com/2036583002/diff/1/components/component... components/component_updater/default_component_installer.cc:334: NOTREACHED() << "Component registration failed for " On 2016/06/02 00:28:39, Sorin Jianu wrote: > is this correct? Why assert on a condition that can be false at runtime? It was NOTREACHED() before, I thought there's a reason for that, like this should never fail... (see https://chromiumcodereview.appspot.com/2022273002/).
thank you! https://chromiumcodereview.appspot.com/2036583002/diff/1/components/component... File components/component_updater/default_component_installer.cc (right): https://chromiumcodereview.appspot.com/2036583002/diff/1/components/component... components/component_updater/default_component_installer.cc:334: NOTREACHED() << "Component registration failed for " On 2016/06/02 04:00:02, xhwang wrote: > On 2016/06/02 00:28:39, Sorin Jianu wrote: > > is this correct? Why assert on a condition that can be false at runtime? > > It was NOTREACHED() before, I thought there's a reason for that, like this > should never fail... (see https://chromiumcodereview.appspot.com/2022273002/). a VLOG statement would be appropriate in my opinion. if () NOTREACHED() is an anti-pattern.
I chose to use LOG(ERROR) since I feel those are pretty bad errors that should be displayed by default. Also, what do you think about DLOG(WARNING) vs. DVLOG(1) for those non-error false cases? https://chromiumcodereview.appspot.com/2036583002/diff/1/components/component... File components/component_updater/default_component_installer.cc (right): https://chromiumcodereview.appspot.com/2036583002/diff/1/components/component... components/component_updater/default_component_installer.cc:334: NOTREACHED() << "Component registration failed for " On 2016/06/02 17:32:52, Sorin Jianu wrote: > On 2016/06/02 04:00:02, xhwang wrote: > > On 2016/06/02 00:28:39, Sorin Jianu wrote: > > > is this correct? Why assert on a condition that can be false at runtime? > > > > It was NOTREACHED() before, I thought there's a reason for that, like this > > should never fail... (see https://chromiumcodereview.appspot.com/2022273002/). > > a VLOG statement would be appropriate in my opinion. if () NOTREACHED() is an > anti-pattern. Done.
lgtm Thank you! https://chromiumcodereview.appspot.com/2036583002/diff/20001/components/compo... File components/component_updater/default_component_installer.cc (right): https://chromiumcodereview.appspot.com/2036583002/diff/20001/components/compo... components/component_updater/default_component_installer.cc:158: DLOG(WARNING) << "DIR_COMPONENT_PREINSTALLED does not exist."; I've been using VLOG or DVLOG. My opinion is that it is annoying to see in the debug log lines that come from some random module in Chrome and no ability to control the logging.
Agreed, I would suggest using DVLOG(1) everywhere.
I've updated the CL to use DVLOG(1) instead of DLOG(WARNING) wherever applicable. PTAL again. https://chromiumcodereview.appspot.com/2036583002/diff/20001/components/compo... File components/component_updater/default_component_installer.cc (right): https://chromiumcodereview.appspot.com/2036583002/diff/20001/components/compo... components/component_updater/default_component_installer.cc:158: DLOG(WARNING) << "DIR_COMPONENT_PREINSTALLED does not exist."; On 2016/06/02 19:30:10, Sorin Jianu wrote: > I've been using VLOG or DVLOG. My opinion is that it is annoying to see in the > debug log lines that come from some random module in Chrome and no ability to > control the logging. Done.
The CQ bit was checked by xhwang@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/2036583002/60001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/2036583002/60001
lgtm
The CQ bit was unchecked by xhwang@chromium.org
The CQ bit was checked by xhwang@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from sorin@chromium.org Link to the patchset: https://chromiumcodereview.appspot.com/2036583002/#ps60001 (title: "DVLOG(1) everywhere")
The CQ bit was checked by xhwang@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/2036583002/60001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/2036583002/60001
Message was sent while issue was closed.
Committed patchset #4 (id:60001)
Message was sent while issue was closed.
Description was changed from ========== Clear logging in Widevine CDM and default component installer The bug has been fixed. Conventing LOG(WARNING)s into DLOG(WARNING)s. BUG=614745 TEST=No functionality change. ========== to ========== Clear logging in Widevine CDM and default component installer The bug has been fixed. Conventing LOG(WARNING)s into DLOG(WARNING)s. BUG=614745 TEST=No functionality change. Committed: https://crrev.com/c4f2d5df530b9e1139af40df5ba53bdca5502e7c Cr-Commit-Position: refs/heads/master@{#397527} ==========
Message was sent while issue was closed.
Patchset 4 (id:??) landed as https://crrev.com/c4f2d5df530b9e1139af40df5ba53bdca5502e7c Cr-Commit-Position: refs/heads/master@{#397527} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
