Simplify and extend commenting in test::ScopedCLDDynamicDataHarness

chrome/browser/translate/translate_browser_test_utils.h

Index: chrome/browser/translate/translate_browser_test_utils.h
diff --git a/chrome/browser/translate/translate_browser_test_utils.h b/chrome/browser/translate/translate_browser_test_utils.h
index d5e054b6838f4b33529fe9e2985eb8530fe32060..3f74db29ccbaaf73abdd5e48227a311e1f39953e 100644
--- a/chrome/browser/translate/translate_browser_test_utils.h
+++ b/chrome/browser/translate/translate_browser_test_utils.h
@@ -11,13 +11,12 @@ namespace test {
 
 // A utility class that sets up CLD dynamic data upon calling Init() and cleans
 // it up when destroyed.
+// NB: Test data lives under: src/chrome/test/data/cld2_component

[msw, 2014/05/22 04:26:48]

nit: what does "NB:" itself add to this comment? It's a pet peeve of mine :)

[Andrew Hayden (chromium.org), 2014/05/22 10:33:45]

Funny thing, I first encountered "NB" about 3 years ago... somehow I managed to
avoid seeing it for about 10 years prior, the it started showing up. I hated it
initially, but it has slowly crept into my lexicon. I agree, it should die.
Removed.

 //
 // This class is intended to be instantiated within IN_PROC_BROWSER_TEST_F
 // test fixtures; it uses ASSERT macros for correctness, so that tests will
 // fail gracefully in error conditions. Sample use:
 //
-//   #include "chrome/browser/translate/translate_browser_test_utils.h"
-//
 //   IN_PROC_BROWSER_TEST_F(BrowserTest, PageLanguageDetection) {
 //     test::ScopedCLDDynamicDataHarness dynamic_data_scope;
 //     ASSERT_NO_FATAL_FAILURE(dynamic_data_scope.Init());
@@ -26,9 +25,25 @@ namespace test {
 //
 // If you have a lot of tests that need language translation features, you can
 // add an instance of the ScopedCLDDynamicDataHarness to your test class'
-// private member variables and add the call to Init() into your Setup method.
+// private member variables and add the call to Init() into SetUpOnMainThread.

[msw, 2014/05/22 04:26:48]

I'm not sure the example here adds much, in fact I'd do away with it and the
example above altogether. As long as the pattern is demonstrated by existing use
in the codebase, it'll tend to be followed (copied) elsewhere.

[Andrew Hayden (chromium.org), 2014/05/22 10:33:45]

That's fair, I suppose. "git gs ScopedCLD" would get you what you need. I think
the first example is still important, because it illustrates the use of
ASSERT_NO_FATAL_FAILURE, which is useful.

+// Sample use:
+//
+//   class MyTestClass : public InProcessBrowserTest {
+//    public:

[msw, 2014/05/22 04:26:48]

nit: remove lines not directly related to init; I'd axe lines 32-34 and 41-46.
(maybe add ellipses lines if you think readers would be confused otherwise)

[Andrew Hayden (chromium.org), 2014/05/22 10:33:45]

Done.

+//     virtual MyTestClass() {}
+//     virtual ~MyTestclass() {}
+//     virtual void SetUpOnMainThread() OVERRIDE {
+//       dynamic_data_scope.Init();

[msw, 2014/05/22 04:26:48]

If this doesn't need to ASSERT_NO_FATAL_FAILURE(...) as you note elsewhere,
perhaps those other comments should be qualified or removed?

[Andrew Hayden (chromium.org), 2014/05/22 10:33:45]

Thorny question. It doesn't make sense to ASSERT_NO_FATAL_FAILURE here because
doing so will cause the test not to run, because SetUpOnMainThread needs to be
called. If there IS a fatal failure, the test itself will fail because that
error will have been recorded in the same thread. You could call
HasFatalFailure() at the start of every test, but that would defeat much of the
good of moving this into the setup code to begin with.

So: In general, the assert is useful... but not here.

[msw, 2014/05/22 18:16:58]

Thanks for clarifying, I suppose what you have is the best we can do for now.

+//       InProcessBrowserTest::SetUpOnMainThread();
+//     }
+//    private:
+//     test::ScopedCLDDynamicDataHarness dynamic_data_scope;
+//     DISALLOW_COPY_AND_ASSIGN(MyTestClass);
+//   };
 //
-// NB: Test data lives under src/chrome/test/data/cld2_component
+//  IN_PROC_BROWSER_TEST_F(MyTestClass, MyTest) {
+//    (your code here)
+//  }
 class ScopedCLDDynamicDataHarness {
  public:
   // Constructs the object, but does nothing. Call Init() to prepare the