Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
919643005:
Stop running PPAPITest and PPAPINaClGLibcTest Pepper tests. (Closed)
DescriptionStop running PPAPITest and PPAPINaClGLibcTest Pepper tests.
These don't add much value vs the other Pepper tests that are currently being run. This saves about 5% of the runtime of browser_tests.
BUG=344054
BUG=457506
Committed: https://crrev.com/d6a5ea53171a05c840f18996e3497b9a948a2e35
Cr-Commit-Position: refs/heads/master@{#317644}
Patch Set 1 #Patch Set 2 : bring back transitional #
Total comments: 1
Patch Set 3 : readd glibc audio tests #
Total comments: 4
Patch Set 4 : review comments #Messages
Total messages: 35 (4 generated)
jam@chromium.org changed reviewers: + dmichael@chromium.org
dmichael@chromium.org changed reviewers: + hidehiko@chromium.org
Hmm, we likely still want: PPAPINaClPNaClTransitionalNonSfiTest ...it's fairly different from other configs, and important for ARC I think. Sorry I didn't mention that in the bug +hidehiko
On 2015/02/11 23:15:02, dmichael wrote: > Hmm, we likely still want: > PPAPINaClPNaClTransitionalNonSfiTest > ...it's fairly different from other configs, and important for ARC I think. > Sorry I didn't mention that in the bug +hidehiko So I can better understand this, can you explain how they're different from say PPAPINaClNewlibTest with respect to Pepper? I understand the sandbox is different, but shouldn't the Pepper calls go through the same code paths?
On 2015/02/11 23:20:36, jam wrote: > On 2015/02/11 23:15:02, dmichael wrote: > > Hmm, we likely still want: > > PPAPINaClPNaClTransitionalNonSfiTest > > ...it's fairly different from other configs, and important for ARC I think. > > Sorry I didn't mention that in the bug +hidehiko > > So I can better understand this, can you explain how they're different from say > PPAPINaClNewlibTest with respect to Pepper? I understand the sandbox is > different, but shouldn't the Pepper calls go through the same code paths? "Nonsfi" tests run outside of NaCl, in a seccomp sandbox, and are generated by the PNaCl toolchain. So it's built by the NaCl toolchain but runs kind of like out-of-process. So arguably the Pepper stuff runs the same code paths, but it's a bit like running tests with Clang vs GCC, or Mac vs Linux... On the bright side, it only runs on Linux 32-bit: https://code.google.com/p/chromium/codesearch#chromium/src/chrome/test/nacl/n...
On 2015/02/11 23:38:05, dmichael wrote: > On 2015/02/11 23:20:36, jam wrote: > > On 2015/02/11 23:15:02, dmichael wrote: > > > Hmm, we likely still want: > > > PPAPINaClPNaClTransitionalNonSfiTest > > > ...it's fairly different from other configs, and important for ARC I think. > > > Sorry I didn't mention that in the bug +hidehiko > > > > So I can better understand this, can you explain how they're different from > say > > PPAPINaClNewlibTest with respect to Pepper? I understand the sandbox is > > different, but shouldn't the Pepper calls go through the same code paths? > "Nonsfi" tests run outside of NaCl, in a seccomp sandbox, and are generated by > the PNaCl toolchain. So it's built by the NaCl toolchain but runs kind of like > out-of-process. So arguably the Pepper stuff runs the same code paths, but it's > a bit like running tests with Clang vs GCC, or Mac vs Linux... It does sounds very similar to the OutOfProcessPPAPITest versions. Do you know if we've had regressions with PPAPINaClPNaClTransitionalNonSfiTest but not OutOfProcessPPAPITest? I lean towards not including PPAPINaClPNaClTransitionalNonSfiTest as a trial and seeing if we see any breakages in nonsfi in practice. > > On the bright side, it only runs on Linux 32-bit: > https://code.google.com/p/chromium/codesearch#chromium/src/chrome/test/nacl/n... The PPAPINaClPNaClTransitionalNonSfiTest tests are keyed off ARCH_CPU_X86_FAMILY, so they run on 64-bit builds on the CQ. i.e. see http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...
hidehiko@chromium.org changed reviewers: + mseaborn@chromium.org
[+mseaborn@] Can we keep the tests for now? The *TransitionalNonSfi* tests run on nacl_helper_nonsfi, while NonSfi* tests run on nacl_helper. These should work conceptually same, however totally different binary. We're switching the code from nacl_helper (current production) to nacl_helper_nonsfi (under development). We want to make sure that those behavior is consistent via the browser tests. Once the work is done, we can merge these tests into one. # And, that's why we have "transitional" in the test name. Thanks, - hidehiko
jam, I think it would make more sense for the NaCl/Pepper team to take on the task of paring these sets of tests down. We will need to make sure that we're not losing any important test coverage. We might need to keep some minimal subsets of the test suites. We'll probably want to deal with each of these test suites one by one, rather than all in one change. Thanks for pointing out how long these tests take to run. It's not obvious just from running trybots or from locally running subsets of tests manually.
On 2015/02/12 09:54:43, hidehiko wrote: > [+mseaborn@] > > Can we keep the tests for now? > The *TransitionalNonSfi* tests run on nacl_helper_nonsfi, while NonSfi* tests > run on nacl_helper. > These should work conceptually same, however totally different binary. > We're switching the code from nacl_helper (current production) to > nacl_helper_nonsfi (under development). > We want to make sure that those behavior is consistent via the browser tests. > Once the work is done, we can merge these tests into one. > # And, that's why we have "transitional" in the test name. > > Thanks, > - hidehiko ok, done.
On 2015/02/12 17:04:04, Mark Seaborn wrote: > jam, I think it would make more sense for the NaCl/Pepper team to take on the > task of paring these sets of tests down. We will need to make sure that we're > not losing any important test coverage. We might need to keep some minimal > subsets of the test suites. We'll probably want to deal with each of these test > suites one by one, rather than all in one change. > > Thanks for pointing out how long these tests take to run. It's not obvious just > from running trybots or from locally running subsets of tests manually. no problem. With the current patch of restoring the transitional non-sfi tests, what do you think is missing from this patch? It seemed that David and others on the NaCl team who I had hallway conversations were ok with disabling the tests in this patch.
I'm still personally OK with cutting out the glibc tests, or at least reducing them. From a Pepper standpoint, there's no difference in the code paths between glibc and newlib, and I can't recall one of them failing unless the other did as well (sans flake). https://codereview.chromium.org/919643005/diff/20001/chrome/test/ppapi/ppapi_... File chrome/test/ppapi/ppapi_browsertest.cc (left): https://codereview.chromium.org/919643005/diff/20001/chrome/test/ppapi/ppapi_... chrome/test/ppapi/ppapi_browsertest.cc:63: } ^^^ We can definitely kill off these IN_PROCESS tests, if you like, without waiting on resolving questions for glibc... (FWIW, there's still some small coverage of in-process in content_browsertests... we could kill most of those too, though I might like to keep 1 as a smoke test since in-process is still used for NaCl)
On 2015/02/13 17:12:02, dmichael wrote: > I'm still personally OK with cutting out the glibc tests, or at least reducing > them. From a Pepper standpoint, there's no difference in the code paths between > glibc and newlib, and I can't recall one of them failing unless the other did as > well (sans flake). Mark: are you still not ok with this? if not, perhaps a quick chat is faster than on email.
On 17 February 2015 at 10:47, <jam@chromium.org> wrote: > On 2015/02/13 17:12:02, dmichael wrote: > >> I'm still personally OK with cutting out the glibc tests, or at least >> reducing >> them. From a Pepper standpoint, there's no difference in the code paths >> between > > glibc and newlib, and I can't recall one of them failing unless the other >> did as > > well (sans flake). >> > > Mark: are you still not ok with this? if not, perhaps a quick chat is > faster > than on email. On PPAPINaClGLibcTest: I would like to keep the PPAPI audio tests enabled, because the PPB_Audio interface is a special case in PPAPI. For PPB_Audio, the PPAPI proxy calls back to user code in order to create a thread, and I'd like to ensure we continue testing that this works with nacl-glibc's pthread library. I'm OK with turning off the other tests in PPAPINaClGLibcTest though. On PPAPITest: If we are turning off testing of in-process PPAPI plugins, the change should also have BUG=344054 (https://crbug.com/344054). The change really ought to remove the "--ppapi-in-process" (kPpapiInProcess) switch as well (or that should be done soon after as a follow-up), so that we're not leaving an untested code path lying around. How about we divide this up? If you tweak the PPAPINaClGLibcTest part, I'll take on removing kPpapiInProcess. Cheers, Mark To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
On Wed, Feb 18, 2015 at 11:09 AM, Mark Seaborn <mseaborn@chromium.org> wrote: > On 17 February 2015 at 10:47, <jam@chromium.org> wrote: >> >> On 2015/02/13 17:12:02, dmichael wrote: >>> >>> I'm still personally OK with cutting out the glibc tests, or at least >>> reducing >>> them. From a Pepper standpoint, there's no difference in the code paths >>> between >>> >>> glibc and newlib, and I can't recall one of them failing unless the other >>> did as >>> >>> well (sans flake). >> >> >> Mark: are you still not ok with this? if not, perhaps a quick chat is >> faster >> than on email. > > > On PPAPINaClGLibcTest: I would like to keep the PPAPI audio tests enabled, > because the PPB_Audio interface is a special case in PPAPI. For PPB_Audio, > the PPAPI proxy calls back to user code in order to create a thread, and I'd > like to ensure we continue testing that this works with nacl-glibc's pthread > library. I'm OK with turning off the other tests in PPAPINaClGLibcTest > though. > > On PPAPITest: If we are turning off testing of in-process PPAPI plugins, > the change should also have BUG=344054 (https://crbug.com/344054). The > change really ought to remove the "--ppapi-in-process" (kPpapiInProcess) > switch as well (or that should be done soon after as a follow-up), so that > we're not leaving an untested code path lying around. This CL doesn't actually turn off all PPAPITest; components_browsertests has some. Arguably we still need some minimal "smoke test" since NaCl relies on it. OTOH, we could say that having the NaCl tests proves that the NaCl plugin is functioning. I was leaning towards having one or two PPAPITest smoke tests left in components_browsertests. Thanks for bringing up the flag; I would check with raymes@ (CCed) to make sure that out-of-process PDF is going to stick before getting rid of it. It hadn't occurred to me that NaCl doesn't need the flag anymore. > > How about we divide this up? If you tweak the PPAPINaClGLibcTest part, I'll > take on removing kPpapiInProcess. > > Cheers, > Mark > To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
On 2015/02/18 18:09:56, Mark Seaborn wrote: > On 17 February 2015 at 10:47, <mailto:jam@chromium.org> wrote: > > > On 2015/02/13 17:12:02, dmichael wrote: > > > >> I'm still personally OK with cutting out the glibc tests, or at least > >> reducing > >> them. From a Pepper standpoint, there's no difference in the code paths > >> between > > > > glibc and newlib, and I can't recall one of them failing unless the other > >> did as > > > > well (sans flake). > >> > > > > Mark: are you still not ok with this? if not, perhaps a quick chat is > > faster > > than on email. > > > On PPAPINaClGLibcTest: I would like to keep the PPAPI audio tests enabled, > because the PPB_Audio interface is a special case in PPAPI. For PPB_Audio, > the PPAPI proxy calls back to user code in order to create a thread, and > I'd like to ensure we continue testing that this works with nacl-glibc's > pthread library. I'm OK with turning off the other tests in > PPAPINaClGLibcTest though. > > On PPAPITest: If we are turning off testing of in-process PPAPI plugins, > the change should also have BUG=344054 (https://crbug.com/344054). The > change really ought to remove the "--ppapi-in-process" (kPpapiInProcess) > switch as well (or that should be done soon after as a follow-up), so that > we're not leaving an untested code path lying around. > > How about we divide this up? If you tweak the PPAPINaClGLibcTest part, > I'll take on removing kPpapiInProcess. > > Cheers, > Mark Ok, I readded the glibc audio tests. ptal
On 18 February 2015 at 10:34, <jam@chromium.org> wrote: > On 2015/02/18 18:09:56, Mark Seaborn wrote: > >> On 17 February 2015 at 10:47, <mailto:jam@chromium.org> wrote: >> > > > On 2015/02/13 17:12:02, dmichael wrote: >> > >> >> I'm still personally OK with cutting out the glibc tests, or at least >> >> reducing >> >> them. From a Pepper standpoint, there's no difference in the code paths >> >> between >> > >> > glibc and newlib, and I can't recall one of them failing unless the >> other >> >> did as >> > >> > well (sans flake). >> >> >> > >> > Mark: are you still not ok with this? if not, perhaps a quick chat is >> > faster >> > than on email. >> > > > On PPAPINaClGLibcTest: I would like to keep the PPAPI audio tests >> enabled, >> because the PPB_Audio interface is a special case in PPAPI. For >> PPB_Audio, >> the PPAPI proxy calls back to user code in order to create a thread, and >> I'd like to ensure we continue testing that this works with nacl-glibc's >> pthread library. I'm OK with turning off the other tests in >> PPAPINaClGLibcTest though. >> > > On PPAPITest: If we are turning off testing of in-process PPAPI plugins, >> the change should also have BUG=344054 (https://crbug.com/344054). The >> change really ought to remove the "--ppapi-in-process" (kPpapiInProcess) >> switch as well (or that should be done soon after as a follow-up), so that >> we're not leaving an untested code path lying around. >> > > How about we divide this up? If you tweak the PPAPINaClGLibcTest part, >> I'll take on removing kPpapiInProcess. >> > > Cheers, >> Mark >> > > Ok, I readded the glibc audio tests. ptal Thanks. Can you take out the removal of PPAPITest tests from this CL too, please, in the spirit of making one logical change per CL? We can reduce/remove PPAPITest in a separate change and consider what to do with kPpapiInProcess in that review. 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/919643005/diff/40001/chrome/test/ppapi/ppapi_... File chrome/test/ppapi/ppapi_browsertest.cc (left): https://codereview.chromium.org/919643005/diff/40001/chrome/test/ppapi/ppapi_... chrome/test/ppapi/ppapi_browsertest.cc:819: // This test only works as an in-process test. See crbug.com/241646. This comment isn't quite right now... maybe just change to: FileRef_ReadDirectoryEntries is flaky, so left out. See crbug.com/241646. ...for now. https://codereview.chromium.org/919643005/diff/40001/chrome/test/ppapi/ppapi_... chrome/test/ppapi/ppapi_browsertest.cc:1288: } Hmm, we don't seem to have any other coverage of this API. Would it work if you changed it from PPAPITest to OutOfProcessPPAPITest?
On 2015/02/18 19:06:20, Mark Seaborn wrote: > On 18 February 2015 at 10:34, <mailto:jam@chromium.org> wrote: > > > On 2015/02/18 18:09:56, Mark Seaborn wrote: > > > >> On 17 February 2015 at 10:47, <mailto:jam@chromium.org> wrote: > >> > > > > > On 2015/02/13 17:12:02, dmichael wrote: > >> > > >> >> I'm still personally OK with cutting out the glibc tests, or at least > >> >> reducing > >> >> them. From a Pepper standpoint, there's no difference in the code paths > >> >> between > >> > > >> > glibc and newlib, and I can't recall one of them failing unless the > >> other > >> >> did as > >> > > >> > well (sans flake). > >> >> > >> > > >> > Mark: are you still not ok with this? if not, perhaps a quick chat is > >> > faster > >> > than on email. > >> > > > > > > On PPAPINaClGLibcTest: I would like to keep the PPAPI audio tests > >> enabled, > >> because the PPB_Audio interface is a special case in PPAPI. For > >> PPB_Audio, > >> the PPAPI proxy calls back to user code in order to create a thread, and > >> I'd like to ensure we continue testing that this works with nacl-glibc's > >> pthread library. I'm OK with turning off the other tests in > >> PPAPINaClGLibcTest though. > >> > > > > On PPAPITest: If we are turning off testing of in-process PPAPI plugins, > >> the change should also have BUG=344054 (https://crbug.com/344054). The > >> change really ought to remove the "--ppapi-in-process" (kPpapiInProcess) > >> switch as well (or that should be done soon after as a follow-up), so that > >> we're not leaving an untested code path lying around. > >> > > > > How about we divide this up? If you tweak the PPAPINaClGLibcTest part, > >> I'll take on removing kPpapiInProcess. > >> > > > > Cheers, > >> Mark > >> > > > > Ok, I readded the glibc audio tests. ptal > > > Thanks. Can you take out the removal of PPAPITest tests from this CL too, > please, in the spirit of making one logical change per CL? We can > reduce/remove PPAPITest in a separate change and consider what to do > with kPpapiInProcess in that review. > I don't agree that we should reduce it much less. This isn't a very complicated cl that needs to be simplified to understand.
https://codereview.chromium.org/919643005/diff/40001/chrome/test/ppapi/ppapi_... File chrome/test/ppapi/ppapi_browsertest.cc (left): https://codereview.chromium.org/919643005/diff/40001/chrome/test/ppapi/ppapi_... chrome/test/ppapi/ppapi_browsertest.cc:819: // This test only works as an in-process test. See crbug.com/241646. On 2015/02/18 19:08:31, dmichael wrote: > This comment isn't quite right now... maybe just change to: > FileRef_ReadDirectoryEntries is flaky, so left out. See crbug.com/241646. > > ...for now. Done. https://codereview.chromium.org/919643005/diff/40001/chrome/test/ppapi/ppapi_... chrome/test/ppapi/ppapi_browsertest.cc:1288: } On 2015/02/18 19:08:31, dmichael wrote: > Hmm, we don't seem to have any other coverage of this API. Would it work if you > changed it from PPAPITest to OutOfProcessPPAPITest? Done.
lgtm fwiw
Can we wait 1-2 weeks to be safe with OOP PDF before removing any further dependencies? Thanks! On Thu Feb 19 2015 at 5:18:38 AM David Michael <dmichael@chromium.org> wrote: > On Wed, Feb 18, 2015 at 11:09 AM, Mark Seaborn <mseaborn@chromium.org> > wrote: > > On 17 February 2015 at 10:47, <jam@chromium.org> wrote: > >> > >> On 2015/02/13 17:12:02, dmichael wrote: > >>> > >>> I'm still personally OK with cutting out the glibc tests, or at least > >>> reducing > >>> them. From a Pepper standpoint, there's no difference in the code paths > >>> between > >>> > >>> glibc and newlib, and I can't recall one of them failing unless the > other > >>> did as > >>> > >>> well (sans flake). > >> > >> > >> Mark: are you still not ok with this? if not, perhaps a quick chat is > >> faster > >> than on email. > > > > > > On PPAPINaClGLibcTest: I would like to keep the PPAPI audio tests > enabled, > > because the PPB_Audio interface is a special case in PPAPI. For > PPB_Audio, > > the PPAPI proxy calls back to user code in order to create a thread, and > I'd > > like to ensure we continue testing that this works with nacl-glibc's > pthread > > library. I'm OK with turning off the other tests in PPAPINaClGLibcTest > > though. > > > > On PPAPITest: If we are turning off testing of in-process PPAPI plugins, > > the change should also have BUG=344054 (https://crbug.com/344054). The > > change really ought to remove the "--ppapi-in-process" (kPpapiInProcess) > > switch as well (or that should be done soon after as a follow-up), so > that > > we're not leaving an untested code path lying around. > This CL doesn't actually turn off all PPAPITest; > components_browsertests has some. Arguably we still need some minimal > "smoke test" since NaCl relies on it. OTOH, we could say that having > the NaCl tests proves that the NaCl plugin is functioning. I was > leaning towards having one or two PPAPITest smoke tests left in > components_browsertests. > > Thanks for bringing up the flag; I would check with raymes@ (CCed) to > make sure that out-of-process PDF is going to stick before getting rid > of it. It hadn't occurred to me that NaCl doesn't need the flag > anymore. > > > > > How about we divide this up? If you tweak the PPAPINaClGLibcTest part, > I'll > > take on removing kPpapiInProcess. > > > > Cheers, > > Mark > > > To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
On 2015/02/18 21:14:55, raymes wrote: > Can we wait 1-2 weeks to be safe with OOP PDF before removing any further > dependencies? Thanks! Out of all the reverts that would need to happen to switch PDF to in-process (my fault!), this is likely the simplest to do. It seems quite unlikely that the in-process code would break (since it's used and tested indirectly by nacl anyways) in the interim given the pace of changes in pepper at this point.
Ok sounds reasonable. On Thu Feb 19 2015 at 9:20:43 AM <jam@chromium.org> wrote: > On 2015/02/18 21:14:55, raymes wrote: > > Can we wait 1-2 weeks to be safe with OOP PDF before removing any further > > dependencies? Thanks! > > Out of all the reverts that would need to happen to switch PDF to > in-process (my > fault!), this is likely the simplest to do. It seems quite unlikely that > the > in-process code would break (since it's used and tested indirectly by nacl > anyways) in the interim given the pace of changes in pepper at this point. > > https://codereview.chromium.org/919643005/ > To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
On 18 February 2015 at 14:20, <jam@chromium.org> wrote: > On 2015/02/18 21:14:55, raymes wrote: > >> Can we wait 1-2 weeks to be safe with OOP PDF before removing any further >> dependencies? Thanks! >> > > Out of all the reverts that would need to happen to switch PDF to > in-process (my > fault!), this is likely the simplest to do. It seems quite unlikely that > the > in-process code would break (since it's used and tested indirectly by nacl > anyways) in the interim given the pace of changes in pepper at this point. So are you suggesting that we should hold off on doing follow-on cleanups like removing the "--ppapi-in-process" switch and removing other plumbing for in-process plugins (other than NaCl), and leave those code paths present but untested for a while? Mark To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
I think John was suggesting removing them for now and in the worst case we may have to revert. On Thu Feb 19 2015 at 9:36:02 AM Mark Seaborn <mseaborn@chromium.org> wrote: > On 18 February 2015 at 14:20, <jam@chromium.org> wrote: > >> On 2015/02/18 21:14:55, raymes wrote: >> >>> Can we wait 1-2 weeks to be safe with OOP PDF before removing any further >>> dependencies? Thanks! >>> >> >> Out of all the reverts that would need to happen to switch PDF to >> in-process (my >> fault!), this is likely the simplest to do. It seems quite unlikely that >> the >> in-process code would break (since it's used and tested indirectly by nacl >> anyways) in the interim given the pace of changes in pepper at this point. > > > So are you suggesting that we should hold off on doing follow-on cleanups > like removing the "--ppapi-in-process" switch and removing other plumbing > for in-process plugins (other than NaCl), and leave those code paths > present but untested for a while? > > Mark > > To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
On 2015/02/18 22:36:01, Mark Seaborn wrote: > On 18 February 2015 at 14:20, <mailto:jam@chromium.org> wrote: > > > On 2015/02/18 21:14:55, raymes wrote: > > > >> Can we wait 1-2 weeks to be safe with OOP PDF before removing any further > >> dependencies? Thanks! > >> > > > > Out of all the reverts that would need to happen to switch PDF to > > in-process (my > > fault!), this is likely the simplest to do. It seems quite unlikely that > > the > > in-process code would break (since it's used and tested indirectly by nacl > > anyways) in the interim given the pace of changes in pepper at this point. > > > So are you suggesting that we should hold off on doing follow-on cleanups > like removing the "--ppapi-in-process" switch and removing other plumbing > for in-process plugins (other than NaCl), and leave those code paths > present but untested for a while? As I mentioned, we still have PPAPITest tests running in content_browsertests that do exercise the flag and in-process. We can keep a small handful of those. > > Mark > > To unsubscribe from this group and stop receiving emails from it, send an email > to mailto:chromium-reviews+unsubscribe@chromium.org.
(though hopefully that's unlikely) On Thu Feb 19 2015 at 9:38:14 AM Raymes Khoury <raymes@chromium.org> wrote: > I think John was suggesting removing them for now and in the worst case we > may have to revert. > > On Thu Feb 19 2015 at 9:36:02 AM Mark Seaborn <mseaborn@chromium.org> > wrote: > >> On 18 February 2015 at 14:20, <jam@chromium.org> wrote: >> >>> On 2015/02/18 21:14:55, raymes wrote: >>> >>>> Can we wait 1-2 weeks to be safe with OOP PDF before removing any >>>> further >>>> dependencies? Thanks! >>>> >>> >>> Out of all the reverts that would need to happen to switch PDF to >>> in-process (my >>> fault!), this is likely the simplest to do. It seems quite unlikely that >>> the >>> in-process code would break (since it's used and tested indirectly by >>> nacl >>> anyways) in the interim given the pace of changes in pepper at this >>> point. >> >> >> So are you suggesting that we should hold off on doing follow-on cleanups >> like removing the "--ppapi-in-process" switch and removing other plumbing >> for in-process plugins (other than NaCl), and leave those code paths >> present but untested for a while? >> >> Mark >> >> To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
On 2015/02/18 22:38:23, dmichael wrote: > On 2015/02/18 22:36:01, Mark Seaborn wrote: > > On 18 February 2015 at 14:20, <mailto:jam@chromium.org> wrote: > > > > > On 2015/02/18 21:14:55, raymes wrote: > > > > > >> Can we wait 1-2 weeks to be safe with OOP PDF before removing any further > > >> dependencies? Thanks! > > >> > > > > > > Out of all the reverts that would need to happen to switch PDF to > > > in-process (my > > > fault!), this is likely the simplest to do. It seems quite unlikely that > > > the > > > in-process code would break (since it's used and tested indirectly by nacl > > > anyways) in the interim given the pace of changes in pepper at this point. > > > > > > So are you suggesting that we should hold off on doing follow-on cleanups > > like removing the "--ppapi-in-process" switch and removing other plumbing > > for in-process plugins (other than NaCl), and leave those code paths > > present but untested for a while? > As I mentioned, we still have PPAPITest tests running in content_browsertests > that do exercise the flag and in-process. We can keep a small handful of those. Just to add more context... those tests are here: https://code.google.com/p/chromium/codesearch#chromium/src/content/test/ppapi... ...so this CL doesn't delete all in-process PPAPI tests. I think it would be wise to keep one or two as "smoke tests" so long as PDF is not 100% certain to be shipped out-of-process and while the NaCl trusted plugin is still in-process. But the majority of the APIs are unused in in-process, and most of that code gets exercised in out-of-process anyway.
The CQ bit was checked by jam@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/919643005/60001
Message was sent while issue was closed.
Committed patchset #4 (id:60001)
Message was sent while issue was closed.
Patchset 4 (id:??) landed as https://crrev.com/d6a5ea53171a05c840f18996e3497b9a948a2e35 Cr-Commit-Position: refs/heads/master@{#317644} |
