Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
724323002:
Non-SFI mode: Enable browser_tests for nacl_helper_nonsfi. (Closed)
DescriptionNon-SFI mode: Enable browser_tests for nacl_helper_nonsfi.
This CL adds browser_tests which run on nacl_helper in Non-SFI mode
to the nacl_helper_nonsfi, too.
As nacl_helper in Non-SFI mode is in prod, the existing tests are
kept as is, and duplicated for the nacl_helper_nonsfi rather than
switching.
Along with the change, PPAPIPrivateNaClPNaClNonSfiTest.FILEIO_Private is fixed. (The fixture was wrong).
BUG=358465
TEST=Ran trybots.
CQ_EXTRA_TRYBOTS=tryserver.chromium.linux:linux_rel_precise32
Committed: https://crrev.com/32b1aa8926148188671689fe2af812615b0ada95
Cr-Commit-Position: refs/heads/master@{#304942}
Patch Set 1 : #
Total comments: 11
Patch Set 2 : #
Total comments: 3
Patch Set 3 : #
Total comments: 14
Patch Set 4 : Rebase #Patch Set 5 : #
Messages
Total messages: 23 (6 generated)
Patchset #1 (id:1) has been deleted
Patchset #1 (id:20001) has been deleted
Patchset #1 (id:40001) has been deleted
Patchset #1 (id:60001) has been deleted
hidehiko@chromium.org changed reviewers: + csharp@chromium.org, dmichael@chromium.org, mseaborn@chromium.org
Could you take a look? mseaborn@: NaCl tests dmichael@: PPAPI tests csharp@: .isolate files. Thank you for review in advance! - hidehiko
isolate files lgtm with nit https://codereview.chromium.org/724323002/diff/80001/chrome/chrome.isolate File chrome/chrome.isolate (right): https://codereview.chromium.org/724323002/diff/80001/chrome/chrome.isolate#ne... chrome/chrome.isolate:23: ['disable_nacl==0 and OS=="linux" and (target_arch=="x64" or target_arch=="ia32")', { Please break into two lines
https://codereview.chromium.org/724323002/diff/80001/chrome/test/ppapi/ppapi_... File chrome/test/ppapi/ppapi_test.h (right): https://codereview.chromium.org/724323002/diff/80001/chrome/test/ppapi/ppapi_... chrome/test/ppapi/ppapi_test.h:186: class PPAPINaClPNaClNaClHelperNonSfiTest : public PPAPINaClPNaClNonSfiTest { 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.
https://codereview.chromium.org/724323002/diff/80001/chrome/browser_tests.iso... File chrome/browser_tests.isolate (right): https://codereview.chromium.org/724323002/diff/80001/chrome/browser_tests.iso... chrome/browser_tests.isolate:226: ['chromeos==1 and (target_arch=="x64" or target_arch=="ia32")', { Why restrict to chromeos==1? We want to run these tests on the normal Linux bots, don't we? https://codereview.chromium.org/724323002/diff/80001/chrome/browser_tests.iso... chrome/browser_tests.isolate:229: '<(PRODUCT_DIR)/nacl_helper_nonsfi', Don't we get this via chrome.isolate below? https://codereview.chromium.org/724323002/diff/80001/chrome/test/ppapi/ppapi_... File chrome/test/ppapi/ppapi_test.h (right): https://codereview.chromium.org/724323002/diff/80001/chrome/test/ppapi/ppapi_... chrome/test/ppapi/ppapi_test.h:186: class PPAPINaClPNaClNaClHelperNonSfiTest : public PPAPINaClPNaClNonSfiTest { On 2014/11/14 16:25:19, dmichael wrote: > This name is really long and doesn't really explain the difference anyway. 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. > 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. 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. > 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. 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.
Thank you for review. PTAL. https://codereview.chromium.org/724323002/diff/80001/chrome/browser_tests.iso... File chrome/browser_tests.isolate (right): https://codereview.chromium.org/724323002/diff/80001/chrome/browser_tests.iso... chrome/browser_tests.isolate:226: ['chromeos==1 and (target_arch=="x64" or target_arch=="ia32")', { On 2014/11/14 16:58:41, Mark Seaborn wrote: > Why restrict to chromeos==1? We want to run these tests on the normal Linux > bots, don't we? This is here, because nacl_helper is listed above section (condition is chromeos==1). Isn't it needed, too? csharp@, any thoughts? https://codereview.chromium.org/724323002/diff/80001/chrome/browser_tests.iso... chrome/browser_tests.isolate:229: '<(PRODUCT_DIR)/nacl_helper_nonsfi', On 2014/11/14 16:58:41, Mark Seaborn wrote: > Don't we get this via chrome.isolate below? Acknowledged. https://codereview.chromium.org/724323002/diff/80001/chrome/chrome.isolate File chrome/chrome.isolate (right): https://codereview.chromium.org/724323002/diff/80001/chrome/chrome.isolate#ne... chrome/chrome.isolate:23: ['disable_nacl==0 and OS=="linux" and (target_arch=="x64" or target_arch=="ia32")', { On 2014/11/14 14:19:59, csharp wrote: > Please break into two lines Done. https://codereview.chromium.org/724323002/diff/80001/chrome/test/ppapi/ppapi_... File chrome/test/ppapi/ppapi_test.h (right): https://codereview.chromium.org/724323002/diff/80001/chrome/test/ppapi/ppapi_... chrome/test/ppapi/ppapi_test.h:186: class PPAPINaClPNaClNaClHelperNonSfiTest : public PPAPINaClPNaClNonSfiTest { On 2014/11/14 16:58:41, Mark Seaborn wrote: > On 2014/11/14 16:25:19, dmichael wrote: > > This name is really long and doesn't really explain the difference anyway. > > Yeah, this needs a name that indicates that these tests are covering a > non-default, transitional version that will eventually become the default. > Why I chose to use NaClHelperNonSfi in the test name is actually, it's test for nacl_helper_nonsfi. Isn't it clear enough? > 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. > > > 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. > > 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. > > > 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. > > 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. 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?
https://codereview.chromium.org/724323002/diff/80001/chrome/test/ppapi/ppapi_... File chrome/test/ppapi/ppapi_test.h (right): https://codereview.chromium.org/724323002/diff/80001/chrome/test/ppapi/ppapi_... chrome/test/ppapi/ppapi_test.h:186: class PPAPINaClPNaClNaClHelperNonSfiTest : public PPAPINaClPNaClNonSfiTest { > 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. Sounds good. > Why I chose to use NaClHelperNonSfi in the test name is actually, it's test for > nacl_helper_nonsfi. Isn't it clear enough? 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" > > So, I'd like to again propose to use NaClHelperNonSfi, but I'm happy to rename > it anything better, of course. > > WDYT? 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.
On 14 November 2014 10:45, <hidehiko@chromium.org> wrote: > https://codereview.chromium.org/724323002/diff/80001/ > chrome/browser_tests.isolate#newcode229 > chrome/browser_tests.isolate:229: '<(PRODUCT_DIR)/nacl_helper_nonsfi', > On 2014/11/14 16:58:41, Mark Seaborn wrote: > >> Don't we get this via chrome.isolate below? >> > > Acknowledged. So if the dependency is brought in by chrome.isolate, can we avoid duplicating it in browser_tests.isolate? > > 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. >> > > 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. >> > > > 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. >> > > 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. >> > > 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? > I still think the name "PPAPINaClPNaClNaClHelperNonSfiTest" is really hard to understand. It contains "NaCl" three times in a row (with a "P" in the middle). The pre-existing "NaClPNaCl" name isn't great, but adding a third "NaCl" makes it worse. Most people reading this won't know that "NaClHelperNonSfi" should be parsed out as a phrase. They won't know that it corresponds to the "nacl_helper_nonsfi" executable, and they won't know that this executable is the newlib-based version of "nacl_helper" for implementing Non-SFI mode. So understanding the name "PPAPINaClPNaClNaClHelperNonSfiTest" requires an awful lot of background knowledge. No name is going to be ideal, so whatever name is used, the code needs some comments to explain what this extra run of tests is for. Cheers, Mark -- You received this message because you are subscribed to the Google Groups "Native-Client-Reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to native-client-reviews+unsubscribe@googlegroups.com. To post to this group, send email to native-client-reviews@googlegroups.com. Visit this group at http://groups.google.com/group/native-client-reviews. For more options, visit https://groups.google.com/d/optout.
On 14 November 2014 10:45, <hidehiko@chromium.org> wrote: > https://codereview.chromium.org/724323002/diff/80001/ > chrome/browser_tests.isolate#newcode229 > chrome/browser_tests.isolate:229: '<(PRODUCT_DIR)/nacl_helper_nonsfi', > On 2014/11/14 16:58:41, Mark Seaborn wrote: > >> Don't we get this via chrome.isolate below? >> > > Acknowledged. So if the dependency is brought in by chrome.isolate, can we avoid duplicating it in browser_tests.isolate? > > 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. >> > > 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. >> > > > 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. >> > > 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. >> > > 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? > I still think the name "PPAPINaClPNaClNaClHelperNonSfiTest" is really hard to understand. It contains "NaCl" three times in a row (with a "P" in the middle). The pre-existing "NaClPNaCl" name isn't great, but adding a third "NaCl" makes it worse. Most people reading this won't know that "NaClHelperNonSfi" should be parsed out as a phrase. They won't know that it corresponds to the "nacl_helper_nonsfi" executable, and they won't know that this executable is the newlib-based version of "nacl_helper" for implementing Non-SFI mode. So understanding the name "PPAPINaClPNaClNaClHelperNonSfiTest" requires an awful lot of background knowledge. No name is going to be ideal, so whatever name is used, the code needs some comments to explain what this extra run of tests is for. Cheers, Mark To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
https://codereview.chromium.org/724323002/diff/100001/chrome/test/nacl/nacl_b... File chrome/test/nacl/nacl_browsertest_util.h (right): https://codereview.chromium.org/724323002/diff/100001/chrome/test/nacl/nacl_b... chrome/test/nacl/nacl_browsertest_util.h:216: #if defined(OS_LINUX) && defined(ARCH_CPU_X86) Shouldn't this be ARCH_CPU_X86_FAMILY so that these tests get enabled on the x86-64 bots too? (Plus add a comment to say why the old and new versions differ in this respect.)
PTAL. Renamed NaClHelperNonSfi to TransitionalNonSfi with comments. >>> Don't we get this via chrome.isolate below? >>> >> >> Acknowledged. > > So if the dependency is brought in by chrome.isolate, can we avoid > duplicating it in browser_tests.isolate? If so, I think, yes, we can get rid of both nacl_helper and nacl_helper_nonsfi at the same time. Though, now I haven't touched about it just in case (and removing nacl_helper from the list is not the focus of this CL. It should be safer to do it in a separate CL if necessary). csharp@, friendly ping? https://codereview.chromium.org/724323002/diff/100001/chrome/test/nacl/nacl_b... File chrome/test/nacl/nacl_browsertest_util.h (right): https://codereview.chromium.org/724323002/diff/100001/chrome/test/nacl/nacl_b... chrome/test/nacl/nacl_browsertest_util.h:216: #if defined(OS_LINUX) && defined(ARCH_CPU_X86) On 2014/11/17 18:35:19, Mark Seaborn wrote: > Shouldn't this be ARCH_CPU_X86_FAMILY so that these tests get enabled on the > x86-64 bots too? > > (Plus add a comment to say why the old and new versions differ in this respect.) Non-SFI testee binaries are built only for ia32 arch.
LGTM https://codereview.chromium.org/724323002/diff/80001/chrome/browser_tests.iso... File chrome/browser_tests.isolate (right): https://codereview.chromium.org/724323002/diff/80001/chrome/browser_tests.iso... chrome/browser_tests.isolate:226: ['chromeos==1 and (target_arch=="x64" or target_arch=="ia32")', { On 2014/11/14 18:45:31, hidehiko wrote: > On 2014/11/14 16:58:41, Mark Seaborn wrote: > > Why restrict to chromeos==1? We want to run these tests on the normal Linux > > bots, don't we? > > This is here, because nacl_helper is listed above section (condition is > chromeos==1). Isn't it needed, too? > > csharp@, any thoughts? Presumably the tests pass without adding this to browser_tests.isolate? If so, can you not add it, please? This seems to be doubly-redundant: Gating it on chromeos==1 means it's a no-op on the main bots, and it seems to be redundant with the decl in chrome.isolate (as in the earlier comment). I don't think you should add something unnecessary just to be consistent with a nearby part of the code. :-) https://codereview.chromium.org/724323002/diff/100001/chrome/test/nacl/nacl_b... File chrome/test/nacl/nacl_browsertest_util.h (right): https://codereview.chromium.org/724323002/diff/100001/chrome/test/nacl/nacl_b... chrome/test/nacl/nacl_browsertest_util.h:216: #if defined(OS_LINUX) && defined(ARCH_CPU_X86) On 2014/11/18 04:39:37, hidehiko wrote: > On 2014/11/17 18:35:19, Mark Seaborn wrote: > > Shouldn't this be ARCH_CPU_X86_FAMILY so that these tests get enabled on the > > x86-64 bots too? > > > > (Plus add a comment to say why the old and new versions differ in this > respect.) > > Non-SFI testee binaries are built only for ia32 arch. OK. I assume you'll fix this in a later change? https://codereview.chromium.org/724323002/diff/120001/chrome/test/nacl/nacl_b... File chrome/test/nacl/nacl_browsertest_util.h (right): https://codereview.chromium.org/724323002/diff/120001/chrome/test/nacl/nacl_b... chrome/test/nacl/nacl_browsertest_util.h:132: // "Transitional" here means that, this uses nacl_helper_nonsfi which has Grammar nit: remove comma after "that" https://codereview.chromium.org/724323002/diff/120001/chrome/test/nacl/nacl_b... chrome/test/nacl/nacl_browsertest_util.h:135: // TODO(hidehiko): Switch for NonSfi tests to use nacl_helper_nonsfi, when Nit: "Switch for" -> "Switch"? Same below... https://codereview.chromium.org/724323002/diff/120001/chrome/test/nacl/nacl_b... chrome/test/nacl/nacl_browsertest_util.h:203: // Currently, we only supports it on x86-32 architecture. "only support" https://codereview.chromium.org/724323002/diff/120001/chrome/test/nacl/nacl_b... chrome/test/nacl/nacl_browsertest_util.h:208: # define MAYBE_NACL_HELPER_NONSFI(test_case) test_case How about naming these #defines as "TRANSITIONAL" rather than "NACL_HELPER" too, for consistency? https://codereview.chromium.org/724323002/diff/120001/chrome/test/ppapi/ppapi... File chrome/test/ppapi/ppapi_browsertest.cc (right): https://codereview.chromium.org/724323002/diff/120001/chrome/test/ppapi/ppapi... chrome/test/ppapi/ppapi_browsertest.cc:762: IN_PROC_BROWSER_TEST_F(PPAPIPrivateNaClPNaClNonSfiTest, Is this fixing an existing mistake? If so you should call this out in the commit message (or do it as a separate change). https://codereview.chromium.org/724323002/diff/120001/chrome/test/ppapi/ppapi... chrome/test/ppapi/ppapi_browsertest.cc:1491: // TODO(hidehiko): Switch for NonSfi tests to use nacl_helper_nonsfi, when "Switch for" -> "Switch" https://codereview.chromium.org/724323002/diff/120001/chrome/test/ppapi/ppapi... File chrome/test/ppapi/ppapi_test.cc (right): https://codereview.chromium.org/724323002/diff/120001/chrome/test/ppapi/ppapi... chrome/test/ppapi/ppapi_test.cc:337: PPAPITestBase::SetUpCommandLine(command_line); I'm not sure why you're moving this. Isn't it simpler to keep these SetUp calls inside the #ifs?
chrome/test/ppapi lgtm
Thank you for review! Submitting. For the record; I talked to csharp@ offline about browser_tests.isolate reverting, and he said ok. As for test enabling on x64, I'll try in another CL, in parallel to other tasks, like sandbox implementation. https://codereview.chromium.org/724323002/diff/120001/chrome/test/nacl/nacl_b... File chrome/test/nacl/nacl_browsertest_util.h (right): https://codereview.chromium.org/724323002/diff/120001/chrome/test/nacl/nacl_b... chrome/test/nacl/nacl_browsertest_util.h:132: // "Transitional" here means that, this uses nacl_helper_nonsfi which has On 2014/11/19 06:23:54, Mark Seaborn wrote: > Grammar nit: remove comma after "that" Done. https://codereview.chromium.org/724323002/diff/120001/chrome/test/nacl/nacl_b... chrome/test/nacl/nacl_browsertest_util.h:135: // TODO(hidehiko): Switch for NonSfi tests to use nacl_helper_nonsfi, when On 2014/11/19 06:23:54, Mark Seaborn wrote: > Nit: "Switch for" -> "Switch"? Same below... Done. https://codereview.chromium.org/724323002/diff/120001/chrome/test/nacl/nacl_b... chrome/test/nacl/nacl_browsertest_util.h:203: // Currently, we only supports it on x86-32 architecture. On 2014/11/19 06:23:54, Mark Seaborn wrote: > "only support" Done. https://codereview.chromium.org/724323002/diff/120001/chrome/test/nacl/nacl_b... chrome/test/nacl/nacl_browsertest_util.h:208: # define MAYBE_NACL_HELPER_NONSFI(test_case) test_case On 2014/11/19 06:23:54, Mark Seaborn wrote: > How about naming these #defines as "TRANSITIONAL" rather than "NACL_HELPER" too, > for consistency? Done. https://codereview.chromium.org/724323002/diff/120001/chrome/test/ppapi/ppapi... File chrome/test/ppapi/ppapi_browsertest.cc (right): https://codereview.chromium.org/724323002/diff/120001/chrome/test/ppapi/ppapi... chrome/test/ppapi/ppapi_browsertest.cc:762: IN_PROC_BROWSER_TEST_F(PPAPIPrivateNaClPNaClNonSfiTest, On 2014/11/19 06:23:54, Mark Seaborn wrote: > Is this fixing an existing mistake? If so you should call this out in the > commit message (or do it as a separate change). Done. https://codereview.chromium.org/724323002/diff/120001/chrome/test/ppapi/ppapi... chrome/test/ppapi/ppapi_browsertest.cc:1491: // TODO(hidehiko): Switch for NonSfi tests to use nacl_helper_nonsfi, when On 2014/11/19 06:23:54, Mark Seaborn wrote: > "Switch for" -> "Switch" Done. https://codereview.chromium.org/724323002/diff/120001/chrome/test/ppapi/ppapi... File chrome/test/ppapi/ppapi_test.cc (right): https://codereview.chromium.org/724323002/diff/120001/chrome/test/ppapi/ppapi... chrome/test/ppapi/ppapi_test.cc:337: PPAPITestBase::SetUpCommandLine(command_line); On 2014/11/19 06:23:54, Mark Seaborn wrote: > I'm not sure why you're moving this. Isn't it simpler to keep these SetUp calls > inside the #ifs? Should be out of #if, IMHO. As PPAPITetsBase::SetUpCommandLine does something regardless DISABLE_NACL macro, so it should be called.
The CQ bit was checked by hidehiko@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/724323002/160001
Message was sent while issue was closed.
Committed patchset #5 (id:160001)
Message was sent while issue was closed.
Patchset 5 (id:??) landed as https://crrev.com/32b1aa8926148188671689fe2af812615b0ada95 Cr-Commit-Position: refs/heads/master@{#304942}
Message was sent while issue was closed.
This broke TSan v2 builds, see below. Are these tests supposed to compile with disable_nacl=1? http://build.chromium.org/p/chromium.memory.fyi/builders/Linux%20Builder%20%2... |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
