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

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

(Re-landing with fixes from https://codereview.chromium.org/111563006/ )

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:
https://codereview.chromium.org/61063003/

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

BUG=298339
No change to mseaborn owned file since last LGTM:
TBR=mseaborn@chromium.org

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

[scheib, 2014-01-02 19:11:48]

Fixing errors in previous version of this testing patch.

[yzshen1, 2014-01-03 22:10:10]

https://codereview.chromium.org/112663007/diff/40001/chrome/browser/extension...
File chrome/browser/extensions/app_background_page_apitest.cc (right):

https://codereview.chromium.org/112663007/diff/40001/chrome/browser/extension...
chrome/browser/extensions/app_background_page_apitest.cc:613:
ASSERT_NO_FATAL_FAILURE(active_impulse_counter.Wait());
Why do we need this? ASSERT_TRUE() in Wait() is sufficient, right? (Maybe I
didn't understand the usage of this macro.)

https://codereview.chromium.org/112663007/diff/40001/chrome/browser/extension...
chrome/browser/extensions/app_background_page_apitest.cc:628:
ASSERT_NO_FATAL_FAILURE(idle_impulse_counter.Wait());
ditto

https://codereview.chromium.org/112663007/diff/40001/extensions/browser/proce...
File extensions/browser/process_manager.h (right):

https://codereview.chromium.org/112663007/diff/40001/extensions/browser/proce...
extensions/browser/process_manager.h:125: // return true they must be released
to free refcounted resources.
It seems there are two ways:
1) keep the void return value, and let the testing code call
ClearKeepaliveImpulseCallbackForTesting() (where it decides to return true in
the current patch).

2) If we would like to have the bool return value, I think the comment could be
revised to something like:
"If the callback returns true, this class clears the callback and it won't be
notified anymore."

I personally think (1) is more clear. What do you think?

[scheib, 2014-01-03 23:11:29]

Here's an alternate fix which doesn't change ProcessManager to be aware of
clearing callbacks.

It's less code, and straight forward. It doesn't have the ease of ASSERTing that
the message loop references are gone, but the main ASAN builders should catch
any reintroduction of that error. I did not attempt to modify patch 2 further to
clear the callbacks, because the utility class would need to be quite messy to
know which callback to clear, or to assume it could always clear both.

https://codereview.chromium.org/112663007/diff/40001/chrome/browser/extension...
File chrome/browser/extensions/app_background_page_apitest.cc (right):

https://codereview.chromium.org/112663007/diff/40001/chrome/browser/extension...
chrome/browser/extensions/app_background_page_apitest.cc:613:
ASSERT_NO_FATAL_FAILURE(active_impulse_counter.Wait());
On 2014/01/03 22:10:10, yzshen1 wrote:
> Why do we need this? ASSERT_TRUE() in Wait() is sufficient, right? (Maybe I
> didn't understand the usage of this macro.)

Correct gtest form is to wrap any subroutine call that contains an assert with
ASSERT_NO_FATAL_FAILURE. Otherwise, the assert won't stop the test. 
https://code.google.com/p/googletest/wiki/AdvancedGuide
While it's true that in these tests there are no commands after the subroutine,
if they are ever modified there could be, and so it's best to add the call here
now.

https://codereview.chromium.org/112663007/diff/40001/chrome/browser/extension...
chrome/browser/extensions/app_background_page_apitest.cc:628:
ASSERT_NO_FATAL_FAILURE(idle_impulse_counter.Wait());
On 2014/01/03 22:10:10, yzshen1 wrote:
> ditto
ditto

https://codereview.chromium.org/112663007/diff/40001/extensions/browser/proce...
File extensions/browser/process_manager.h (right):

https://codereview.chromium.org/112663007/diff/40001/extensions/browser/proce...
extensions/browser/process_manager.h:125: // return true they must be released
to free refcounted resources.
On 2014/01/03 22:10:10, yzshen1 wrote:
> It seems there are two ways:
> 1) keep the void return value, and let the testing code call
> ClearKeepaliveImpulseCallbackForTesting() (where it decides to return true in
> the current patch).
> 
> 2) If we would like to have the bool return value, I think the comment could
be
> revised to something like:
> "If the callback returns true, this class clears the callback and it won't be
> notified anymore."
> 
> I personally think (1) is more clear. What do you think?

I've changed to having the tests clear the callbacks.

[yzshen1, 2014-01-03 23:23:28]

LGTM

Thanks!

https://codereview.chromium.org/112663007/diff/40001/chrome/browser/extension...
File chrome/browser/extensions/app_background_page_apitest.cc (right):

https://codereview.chromium.org/112663007/diff/40001/chrome/browser/extension...
chrome/browser/extensions/app_background_page_apitest.cc:613:
ASSERT_NO_FATAL_FAILURE(active_impulse_counter.Wait());
Thanks, I didn't know that!
On 2014/01/03 23:11:29, scheib wrote:
> On 2014/01/03 22:10:10, yzshen1 wrote:
> > Why do we need this? ASSERT_TRUE() in Wait() is sufficient, right? (Maybe I
> > didn't understand the usage of this macro.)
> 
> Correct gtest form is to wrap any subroutine call that contains an assert with
> ASSERT_NO_FATAL_FAILURE. Otherwise, the assert won't stop the test. 
> https://code.google.com/p/googletest/wiki/AdvancedGuide
> While it's true that in these tests there are no commands after the
subroutine,
> if they are ever modified there could be, and so it's best to add the call
here
> now.

[commit-bot: I haz the power, 2014-01-04 02:41:50]

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

[commit-bot: I haz the power, 2014-01-04 12:16:51]

Change committed as 243021

[ckocagil, 2014-01-04 22:29:52]

A revert of this CL has been created in
https://codereview.chromium.org/124313005/ by ckocagil@chromium.org.

The reason for reverting is: Broke XP bots:
http://build.chromium.org/p/chromium.win/builders/XP%20Tests%20%282%29/builds....