Test Keeping NaCl plugins used in app background pages alive when active.

Test Keeping NaCl plugins used in app background pages alive when active.

Activity in Native Client plugins results in IPC messages 
sent to the BrowserPpapiHostImpl and routed to call 
extensions::ProcessManager::KeepaliveImpulse. 

Testing patch, builds on implementation. See: 
https://codereview.chromium.org/61063003/ Implementation.
https://codereview.chromium.org/105873003/ Cumulative patch. 

Design doc: 
https://docs.google.com/a/chromium.org/document/d/1mI0lS1rfAf-BAGLmWAEcWy37Xq9dOvgfMx8OqeUMXts/edit# 

BUG=298339
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=242061

[yzshen1, 2013-12-14 01:07:00]

https://codereview.chromium.org/111563006/diff/110001/chrome/browser/extensio...
File chrome/browser/extensions/app_background_page_apitest.cc (right):

https://codereview.chromium.org/111563006/diff/110001/chrome/browser/extensio...
chrome/browser/extensions/app_background_page_apitest.cc:116: namespace {
Is there any reason that this namespace covers the class below but not the one
above?

https://codereview.chromium.org/111563006/diff/110001/chrome/browser/extensio...
chrome/browser/extensions/app_background_page_apitest.cc:164: const
extensions::ProcessManager::ImpulseCallbackForTesting
nit: this return value doesn't need to be const

https://codereview.chromium.org/111563006/diff/110001/chrome/browser/extensio...
chrome/browser/extensions/app_background_page_apitest.cc:179: void
ImpulseCallback(base::Closure quit_callback,
const &, and please move it to the next line.

https://codereview.chromium.org/111563006/diff/110001/chrome/browser/extensio...
chrome/browser/extensions/app_background_page_apitest.cc:603:
active_impulse_counter.SetGoalAndGetCallback(40));
Is 1 second too long for a single test? Maybe we can make it shorter?

https://codereview.chromium.org/111563006/diff/110001/ppapi/tests/extensions/...
File ppapi/tests/extensions/background_keepalive/background.cc (right):

https://codereview.chromium.org/111563006/diff/110001/ppapi/tests/extensions/...
ppapi/tests/extensions/background_keepalive/background.cc:6: #include <string>
empty line between two sections, please.

https://codereview.chromium.org/111563006/diff/110001/ppapi/tests/extensions/...
ppapi/tests/extensions/background_keepalive/background.cc:44: int delay_;
Please comment about the unit or rename the variable.

[scheib, 2013-12-14 05:43:49]

Thanks

https://codereview.chromium.org/111563006/diff/110001/chrome/browser/extensio...
File chrome/browser/extensions/app_background_page_apitest.cc (right):

https://codereview.chromium.org/111563006/diff/110001/chrome/browser/extensio...
chrome/browser/extensions/app_background_page_apitest.cc:116: namespace {
On 2013/12/14 01:07:01, yzshen1 wrote:
> Is there any reason that this namespace covers the class below but not the one
> above?

Yes, the above class is given Friend access outside of this compilation unit.

https://codereview.chromium.org/111563006/diff/110001/chrome/browser/extensio...
chrome/browser/extensions/app_background_page_apitest.cc:164: const
extensions::ProcessManager::ImpulseCallbackForTesting
On 2013/12/14 01:07:01, yzshen1 wrote:
> nit: this return value doesn't need to be const

Done.

https://codereview.chromium.org/111563006/diff/110001/chrome/browser/extensio...
chrome/browser/extensions/app_background_page_apitest.cc:179: void
ImpulseCallback(base::Closure quit_callback,
On 2013/12/14 01:07:01, yzshen1 wrote:
> const &, and please move it to the next line.

Done.

https://codereview.chromium.org/111563006/diff/110001/chrome/browser/extensio...
chrome/browser/extensions/app_background_page_apitest.cc:603:
active_impulse_counter.SetGoalAndGetCallback(40));
On 2013/12/14 01:07:01, yzshen1 wrote:
> Is 1 second too long for a single test? Maybe we can make it shorter?

Done. Changed to 20 ~~ .5 seconds. This is fairly small for the overhead of
bringing a browser test up, but still long enough to ensure we're seeing
sustained behavior.

https://codereview.chromium.org/111563006/diff/110001/ppapi/tests/extensions/...
File ppapi/tests/extensions/background_keepalive/background.cc (right):

https://codereview.chromium.org/111563006/diff/110001/ppapi/tests/extensions/...
ppapi/tests/extensions/background_keepalive/background.cc:6: #include <string>
On 2013/12/14 01:07:01, yzshen1 wrote:
> empty line between two sections, please.

Done.

https://codereview.chromium.org/111563006/diff/110001/ppapi/tests/extensions/...
ppapi/tests/extensions/background_keepalive/background.cc:44: int delay_;
On 2013/12/14 01:07:01, yzshen1 wrote:
> Please comment about the unit or rename the variable.

Done.

[scheib, 2013-12-14 06:20:14]

mseaborn, just for owners of:
  components/nacl/browser/nacl_process_host.cc

[yzshen1, 2013-12-16 17:55:41]

lgtm

[Mark Seaborn, 2013-12-18 21:07:57]

> mseaborn, just for owners of:
>   components/nacl/browser/nacl_process_host.cc

LGTM

https://codereview.chromium.org/111563006/diff/150001/chrome/browser/extensio...
File chrome/browser/extensions/app_background_page_apitest.cc (right):

https://codereview.chromium.org/111563006/diff/150001/chrome/browser/extensio...
chrome/browser/extensions/app_background_page_apitest.cc:602: // Target .5
seconds: .5 seconds / 50ms throttle * 2 embeds == 20 impulses.
I didn't follow the test closely enough to tell whether this might be flaky on a
heavily loaded bot.  It seems like it wouldn't be because you're just waiting
for 20 impulses to occur.

Using fake time would be a more ideal way to deal with that.  Does base/ provide
any facilities for faking time?  (e.g. See "Faking time" in
http://blog.golang.org/playground)  I don't think it does, but just thought I'd
ask. :-)

https://codereview.chromium.org/111563006/diff/150001/ppapi/shared_impl/ppapi...
File ppapi/shared_impl/ppapi_switches.cc (right):

https://codereview.chromium.org/111563006/diff/150001/ppapi/shared_impl/ppapi...
ppapi/shared_impl/ppapi_switches.cc:12: // Specifies throttling time in
milliseconds for PpapiHostMsg_Keepalive IPCs.
Can the comment make it clear that this flag is for testing purposes only?

https://codereview.chromium.org/111563006/diff/150001/ppapi/tests/extensions/...
File ppapi/tests/extensions/background_keepalive/background.cc (right):

https://codereview.chromium.org/111563006/diff/150001/ppapi/tests/extensions/...
ppapi/tests/extensions/background_keepalive/background.cc:31: return;
This is superfluous in this context

https://codereview.chromium.org/111563006/diff/150001/ppapi/tests/extensions/...
File ppapi/tests/extensions/extensions.gyp (right):

https://codereview.chromium.org/111563006/diff/150001/ppapi/tests/extensions/...
ppapi/tests/extensions/extensions.gyp:38: 'build_glibc': 0,
Is there a specific reason to disable this on glibc and PNaCl?  Maybe this could
use a comment.

[scheib, 2013-12-19 01:31:37]

Thanks

https://codereview.chromium.org/111563006/diff/150001/chrome/browser/extensio...
File chrome/browser/extensions/app_background_page_apitest.cc (right):

https://codereview.chromium.org/111563006/diff/150001/chrome/browser/extensio...
chrome/browser/extensions/app_background_page_apitest.cc:602: // Target .5
seconds: .5 seconds / 50ms throttle * 2 embeds == 20 impulses.
On 2013/12/18 21:07:57, Mark Seaborn wrote:
> I didn't follow the test closely enough to tell whether this might be flaky on
a
> heavily loaded bot.  It seems like it wouldn't be because you're just waiting
> for 20 impulses to occur.
> 
> Using fake time would be a more ideal way to deal with that.  Does base/
provide
> any facilities for faking time?  (e.g. See "Faking time" in
> http://blog.golang.org/playground)  I don't think it does, but just thought
I'd
> ask. :-)

Worst case the test runs long, and it would have to be 20x slower to start to
time out.

Faking time would be expensive, because not only browser side but also plugin
side would need faking and time advancement infrastructure.

https://codereview.chromium.org/111563006/diff/150001/ppapi/shared_impl/ppapi...
File ppapi/shared_impl/ppapi_switches.cc (right):

https://codereview.chromium.org/111563006/diff/150001/ppapi/shared_impl/ppapi...
ppapi/shared_impl/ppapi_switches.cc:12: // Specifies throttling time in
milliseconds for PpapiHostMsg_Keepalive IPCs.
On 2013/12/18 21:07:57, Mark Seaborn wrote:
> Can the comment make it clear that this flag is for testing purposes only?

We might opt to use this for non-testing in the future, though. E.g. if we did
adjust the system to not send any IPCs for non extension NaCl plugins.

https://codereview.chromium.org/111563006/diff/150001/ppapi/tests/extensions/...
File ppapi/tests/extensions/background_keepalive/background.cc (right):

https://codereview.chromium.org/111563006/diff/150001/ppapi/tests/extensions/...
ppapi/tests/extensions/background_keepalive/background.cc:31: return;
On 2013/12/18 21:07:57, Mark Seaborn wrote:
> This is superfluous in this context

Done.

https://codereview.chromium.org/111563006/diff/150001/ppapi/tests/extensions/...
File ppapi/tests/extensions/extensions.gyp (right):

https://codereview.chromium.org/111563006/diff/150001/ppapi/tests/extensions/...
ppapi/tests/extensions/extensions.gyp:38: 'build_glibc': 0,
On 2013/12/18 21:07:57, Mark Seaborn wrote:
> Is there a specific reason to disable this on glibc and PNaCl?  Maybe this
could
> use a comment.

Comment added.

The AppBackgroundPageNaClTest fixture only uses the newlib build. I don't think
there's a reason to span the test over all build types.

[commit-bot: I haz the power, 2013-12-19 01:35:10]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/scheib@chromium.org/111563006/210001

[commit-bot: I haz the power, 2013-12-19 01:58:34]

Retried try job too often on chromium_presubmit for step(s) presubmit
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=chromium_p...

[commit-bot: I haz the power, 2013-12-19 18:22:33]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/scheib@chromium.org/111563006/230001

[commit-bot: I haz the power, 2013-12-19 23:49:17]

Retried try job too often on linux_aura for step(s) browser_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_aura...

[commit-bot: I haz the power, 2013-12-20 07:21:35]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/scheib@chromium.org/111563006/230001

[commit-bot: I haz the power, 2013-12-20 10:40:29]

Change committed as 242061