Blink presubmit doesn't warn on LOG(...) statements |
||||
Issue description
I noticed that we have some logging statements that probably shouldn't be. For exmaple, DOMWindow::wrap has:
v8::Local<v8::Object> DOMWindow::wrap(v8::Isolate*,
v8::Local<v8::Object> creationContext) {
LOG(FATAL) << "DOMWindow must never be wrapped with wrap method. The "
"wrappers must be created at WindowProxy::createContext() and "
"setupWindowPrototypeChain().";
return v8::Local<v8::Object>();
}
which means this string gets included into official builds:
$ strings out/official/chrome | grep "WindowProxy::createContext"
DOMWindow must never be wrapped with wrap method. The wrappers must be created at WindowProxy::createContext() and setupWindowPrototypeChain().
These strings aren't useful in official builds. We should convert them to DCHECK/SECURITY_DCHECK/CHECK as appropriate. We should also decide what conventions we want in Blink: the current presubmit in Chromium only checks for LOG(INFO), but we might want to generally discourage LOG in Blink for binary bloat reasons.
,
Dec 12 2016
CHECK does; I don't think LOG does (I included a brief snippet above that shows log lines in an official build)
,
Dec 19 2016
+1 to disallowing LOG. VLOG and DLOG are much better. We'd have to disallow VLOG too tho if you want to prevent strings in official builds.
,
Dec 20 2016
Can we just remove LOG? Why does it exist if we want to disallow it?
,
Dec 20 2016
Interesting thought. It's very useful when debugging problems in release builds, and for printing output in tests sometimes. It's like we don't want to ship printf()s but the function is very useful to have. So idk..
,
Feb 16 2018
This issue has been Available for over a year. If it's no longer important or seems unlikely to be fixed, please consider closing it out. If it is important, please re-triage the issue. Sorry for the inconvenience if the bug really should have been left as Available. If you change it back, also remove the "Hotlist-Recharge-Cold" label. For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot
,
Feb 20 2018
,
Jan 11
Available, but no owner or component? Please find a component, as no one will ever find this without one. |
||||
►
Sign in to add a comment |
||||
Comment 1 by thakis@chromium.org
, Dec 12 2016