Calling Shutdown() for Android content_browsertests. Removed webrtc test hack.

Calling Shutdown() for Android content_browsertests. Removed webrtc test hack.

Should be landed after:
https://codereview.chromium.org/323513003/
https://codereview.chromium.org/314293004/

which fix related shutdown issues.

BUG=367409, 373249
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=277998

[bulach, 2014-06-09 09:26:46]

lgtm, thanks! small nits below.

https://codereview.chromium.org/319053006/diff/1/base/android/java/src/org/ch...
File base/android/java/src/org/chromium/base/SystemMessageHandler.java (right):

https://codereview.chromium.org/319053006/diff/1/base/android/java/src/org/ch...
base/android/java/src/org/chromium/base/SystemMessageHandler.java:49: private
void removeScheduledWork() {
nit: since this is only called on one place, I'd suggest renaming it to
"removeAllPendingMessages()"...

(obviously, need to rename the call in MessagePumpForUI::Quit() as well..)

https://codereview.chromium.org/319053006/diff/1/content/browser/browser_main...
File content/browser/browser_main_runner.cc (right):

https://codereview.chromium.org/319053006/diff/1/content/browser/browser_main...
content/browser/browser_main_runner.cc:153: // This is a hack for terminating
the RunLoop inside MessagePumpForUI,
nit: perhaps:

// Forcefully terminates the RunLoop inside MessagePumpForUI,
// ensuring proper shutdown in content_browsertests.
// Shutdown() is not used by actual browser.

[Victor Starodub, 2014-06-09 16:13:26]

Thanks for review. Fixed comments.

https://codereview.chromium.org/319053006/diff/1/base/android/java/src/org/ch...
File base/android/java/src/org/chromium/base/SystemMessageHandler.java (right):

https://codereview.chromium.org/319053006/diff/1/base/android/java/src/org/ch...
base/android/java/src/org/chromium/base/SystemMessageHandler.java:49: private
void removeScheduledWork() {
On 2014/06/09 09:26:45, bulach wrote:
> nit: since this is only called on one place, I'd suggest renaming it to
> "removeAllPendingMessages()"...
> 
> (obviously, need to rename the call in MessagePumpForUI::Quit() as well..)

Done.

https://codereview.chromium.org/319053006/diff/1/content/browser/browser_main...
File content/browser/browser_main_runner.cc (right):

https://codereview.chromium.org/319053006/diff/1/content/browser/browser_main...
content/browser/browser_main_runner.cc:153: // This is a hack for terminating
the RunLoop inside MessagePumpForUI,
On 2014/06/09 09:26:45, bulach wrote:
> nit: perhaps:
> 
> // Forcefully terminates the RunLoop inside MessagePumpForUI,
> // ensuring proper shutdown in content_browsertests.
> // Shutdown() is not used by actual browser.

Done.

[Paweł Hajdan Jr., 2014-06-13 13:46:47]

Removing the Sleep is awesome. Kudos for doing that.

Deferring to John for content/ and I'm not an expert on most code touched here.

[jam, 2014-06-13 17:14:01]

https://codereview.chromium.org/319053006/diff/20001/content/browser/browser_...
File content/browser/browser_main_runner.cc (right):

https://codereview.chromium.org/319053006/diff/20001/content/browser/browser_...
content/browser/browser_main_runner.cc:156:
base::MessageLoop::current()->QuitNow();
can we avoid putting the test-only code in this non test file?

could this call go in browser_test_base.cc where you changed the code? or does
it have to happen before line 158 below?

https://codereview.chromium.org/319053006/diff/20001/content/public/test/brow...
File content/public/test/browser_test_base.cc (right):

https://codereview.chromium.org/319053006/diff/20001/content/public/test/brow...
content/public/test/browser_test_base.cc:245: runner->Shutdown();
i believe you can call BrowserMain(params) instead of these four lines

[Victor Starodub, 2014-06-17 00:53:04]

Thanks for review. PTAL

https://codereview.chromium.org/319053006/diff/20001/content/browser/browser_...
File content/browser/browser_main_runner.cc (right):

https://codereview.chromium.org/319053006/diff/20001/content/browser/browser_...
content/browser/browser_main_runner.cc:156:
base::MessageLoop::current()->QuitNow();
FYI: the code is not actually test-specific. It's just it was decided not do a
proper shutdown on Android in the browser.

Unfortunately, as you've noticed, it has to go between
'main_loop_->ShutdownThreadsAndCleanUp()' and 'main_loop_.reset()'.

https://codereview.chromium.org/319053006/diff/20001/content/public/test/brow...
File content/public/test/browser_test_base.cc (right):

https://codereview.chromium.org/319053006/diff/20001/content/public/test/brow...
content/public/test/browser_test_base.cc:245: runner->Shutdown();
On 2014/06/13 17:14:01, jam wrote:
> i believe you can call BrowserMain(params) instead of these four lines

Done. Thanks!

[jam, 2014-06-18 00:39:24]

lgtm

[Victor Starodub, 2014-06-18 00:45:39]

+mark

Need an owner of message_pump_android.cc which was touched during the review.

[Mark Mentovai, 2014-06-18 02:04:17]

LGTM in base.

[Victor Starodub, 2014-06-18 02:55:16]

The CQ bit was checked by starodub@google.com

[commit-bot: I haz the power, 2014-06-18 02:56:54]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/starodub@google.com/319053006/40001

[commit-bot: I haz the power, 2014-06-18 09:04:38]

Change committed as 277998