Non-SFI mode: Enable browser_tests for nacl_helper_nonsfi.

chrome/test/ppapi/ppapi_test.h

 // Copyright (c) 2012 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #ifndef CHROME_TEST_PPAPI_PPAPI_TEST_H_
 #define CHROME_TEST_PPAPI_PPAPI_TEST_H_
 
 #include <string>
 
 #include "base/basictypes.h"
@@ skipping 165 matching lines @@
 
 // Test Non-SFI Mode, using PNaCl toolchain to produce nexes.
 class PPAPINaClPNaClNonSfiTest : public PPAPINaClTest {
  public:
   virtual void SetUpCommandLine(base::CommandLine* command_line);
 
   virtual std::string BuildQuery(const std::string& base,
                                  const std::string& test_case) override;
 };
 
+class PPAPINaClPNaClNaClHelperNonSfiTest : public PPAPINaClPNaClNonSfiTest {

[dmichael (off chromium), 2014/11/14 16:25:19]

This name is really long and doesn't really explain the difference anyway. If
I'm reading the bug right, the difference is it's using a nacl_helper that's
linked against newlib instead of glibc. Also, it seems that this newer way is
the one we want to use going forward.

Is it the case that right now, we need to support both, but we hope to get rid
of the glibc version?

If so, maybe it would make sense to flip this around and make
PPAPINaClPNaClNonSfiTest refer to the new, newlib way. And give the old,
soon-to-go-away test the longer/uglier name. Also, it seems OK to elide some of
the words to make it not so hard to read, and explain what it tests in a comment
on the class.

Maybe cut out the first NaCl and let it be implied, and add Glibc to specify
what's actually different:
PPAPIPNaClGlibcNaClHelperNonSfiTest
Not sure. There's not a good way to name it, but I think it will get deleted
anyway.

[Mark Seaborn, 2014/11/14 16:58:41]

Yeah, this needs a name that indicates that these tests are covering a
non-default, transitional version that will eventually become the default.

If the current name is "PPAPINaClPNaClNonSfiTest", maybe we could add "New" or
"Transitional" to that?  Maybe "PPAPINaClPNaClNonSfiTransitionalTest"?

And then add a comment explaining what "transitional" means here, plus TODOs to
remove it when we switch over.
I don't think we should do that yet: I think it's better if the current
"PPAPINaClPNaClNonSfiTest" tests what we ship.  The longer/uglier name should be
for the non-shipping version.
Putting "newlib" or "glibc" in the names might be too confusing, since they
normally refer to the NaCl-user-code versions of the libraries, rather than what
Chromium system code is linked with.

[hidehiko, 2014/11/14 18:45:32]

I'm open for naming. Some my thoughts:

Probably it'd be better to keep the current tests as is for now,
because these are prod. Although nacl_helper in Non-SFI mode will be deprecated,
but not yet.

As Dave said, I'm planning to replace these with ones for nacl_helper_nonsfi
eventually. The renaming/replacing of tests should be done when it's officially
released.

So, temporarily I'd like to choose longer/uglier name for the newly introduced
tests, which is for nacl_helper_nonsfi, as Mark suggested.

I +1 to avoid "newlib" "glibc" words. As Mark said, these words are used for the
NaCl's user code, rather than the nacl_helper code. Mixing them sounds a bit
confusing.

Also, probably avoiding "New" would be better, IMHO. Its "text" looks similar to
"Newlib", as we have more words around it so maybe it looks slightly confusing,
too.

Why I chose to use NaClHelperNonSfi in the test name is actually, it's test for
nacl_helper_nonsfi. Isn't it clear enough?

So, I'd like to again propose to use NaClHelperNonSfi, but I'm happy to rename
it anything better, of course.

WDYT?

[dmichael (off chromium), 2014/11/14 19:26:52]

Sounds good.
I also find "nacl_helper_nonsfi" to be a confusing name, because it's really the
"newlib" nacl_helper; it's the newlib part that is different, not the fact that
it uses nacl_helper.

Hence I slightly prefer Mark's suggestion of "Transitional"
I lean towards Mark's suggestion of Transitional, but I'm OK with either. Either
way, please add a comment about the difference and reference the bug.

+ public:
+  virtual void SetUpCommandLine(base::CommandLine* command_line) override;
+};
+
 class PPAPIPrivateNaClPNaClNonSfiTest : public PPAPINaClPNaClNonSfiTest {
  protected:
   virtual void SetUpCommandLine(base::CommandLine* command_line) override;
 };
 
+class PPAPIPrivateNaClPNaClNaClHelperNonSfiTest
+    : public PPAPIPrivateNaClPNaClNonSfiTest {
+ protected:
+  virtual void SetUpCommandLine(base::CommandLine* command_line) override;
+};
+
+
 class PPAPINaClTestDisallowedSockets : public PPAPITestBase {
  public:
   virtual void SetUpCommandLine(base::CommandLine* command_line) override;
 
   virtual std::string BuildQuery(const std::string& base,
                                  const std::string& test_case) override;
 };
 
 class PPAPIBrokerInfoBarTest : public OutOfProcessPPAPITest {
  public:
   // PPAPITestBase override:
   virtual void SetUpOnMainThread() override;
 };
 
 #endif  // CHROME_TEST_PPAPI_PPAPI_TEST_H_