base: Remove the string logging from CHECK().

ui/base/template_expressions.cc

Index: ui/base/template_expressions.cc
diff --git a/ui/base/template_expressions.cc b/ui/base/template_expressions.cc
index 10aaf4fecf5a4e44040c427de461ba5704336fbf..4086c76bf17cce2d9671c7a36917b54d7ceea31a 100644
--- a/ui/base/template_expressions.cc
+++ b/ui/base/template_expressions.cc
@@ -54,8 +54,7 @@ std::string ReplaceTemplateExpressions(
     CHECK(!key.empty());
 
     TemplateReplacements::const_iterator value = replacements.find(key);
-    CHECK(value != replacements.end()) << "$i18n replacement key \"" << key
-                                       << "\" not found";
+    CHECK(value != replacements.end());

[Alexander Alekseev, 2016/12/09 06:14:02]

I just came across this CHECK and it turned out that I need debug build to debug
HTML!

Could we replace this with LOG_FATAL?

[dcheng, 2016/12/09 06:17:24]

LOG(FATAL) isn't equivalent: CHECK attempts (but fails on MSVC) to optimize out
the string completely in release builds. LOG(FATAL) will include the string no
matter what.

[Alexander Alekseev, 2016/12/09 07:14:50]

This means that that most people used CHECK() with log message incorrectly. As
you see, a lot of changes are in tests, where you would naturally expect error
messages to be always visible.

I used to think that it will display log message always, even in release builds.

I would suggest changing CHECK() so that it will always print error message. But
if it is unacceptable due to some reason, let's change all these occurrences to
LOG(FATAL).
Or, probably, you should ask the owners of individual components of what to do
with the error strings.
My chrome has just crashed here and I was very surprised that I received no
error message. So I believe that owners of this file also expect that message to
be always visible. You cannot understand what is wrong in your html without this
message.

[dcheng, 2016/12/09 07:29:36]

1. We shouldn't unconditionally emit the string: we pay a fair amount in binary
size to have a bunch of constant strings sitting around for no real reason. I
don't think anyone has numbers but it's non-trivial.

2. Similarly, LOG(...) is highly discouraged for that reason: it generates log
spam and puts a bunch of strings in the official binary that aren't generally
useful for end-users, yet take up a bunch of binary space. In fact, there's a
PRESUBMIT that tries to prevent LOG(...) statements.

For these particular CHECKs, they certainly look like they should be DCHECKs or
properly handled: they almost certainly shouldn't be CHECKs. Either we know what
we're passing into our templates, and we should never violate those invariants,
or we are processing untrusted data, and we need to properly handle invalid
inputs.

[Alexander Alekseev, 2016/12/09 07:41:23]

1) Do you mean that a single explanation message before chrome crash should be
considered as spam?

2) Do you think that CHECKs in test are spam? It is probably difficult to
replace them by ASSERT because of gtest limitatoins, but removing test failure
reasons completely seems weird.

[dcheng, 2016/12/09 07:56:11]

Not necessarily, but there's a binary size tradeoff involved here.
If something is a CHECK just so we can have a death test... it probably
shouldn't be a CHECK. Perhaps I'm misunderstanding this though.

 
     std::string replacement = value->second;
     if (context.empty()) {
@@ -64,7 +63,8 @@ std::string ReplaceTemplateExpressions(
     } else if (context == "Raw") {
       // Pass the replacement through unchanged.
     } else {
-      CHECK(false) << "Unknown context " << context;
+      // Unknown context |context|
+      CHECK(false);
     }
 
     formatted.append(replacement);