Re-enable about:flags on Android

Re-enable about:flags on Android

* Re-connects hooks to the about://flags page so that we can access it.
* Adds a custom ApplicationLifetime::TerminateAndroid() for Android that hooks into
  Java-land to let the Activity know that the application must be restarted.

* Exposes the enable-WebGL flag that Android uses but other platforms don't.
* For Android, disables the disable-WebGL flag that the other platform uses.

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

[gone, 2012-12-20 00:56:07]

Hey Grace,
When you get a chance, can you give this a once over and see if the approach
makes sense, given our lack of clean shutdowns on Android?  There's an example
companion CL on gerrit that uses the hook added by this chromium CL to perform
the restart.

- Dan

[klobag.chromium, 2012-12-20 02:22:31]

https://chromiumcodereview.appspot.com/11633020/diff/1016/chrome/browser/life...
File chrome/browser/lifetime/application_lifetime.cc (right):

https://chromiumcodereview.appspot.com/11633020/diff/1016/chrome/browser/life...
chrome/browser/lifetime/application_lifetime.cc:239: #if !defined(OS_ANDROID)
Should we share the save session / pref?

https://chromiumcodereview.appspot.com/11633020/diff/1016/chrome/browser/reso...
File chrome/browser/resources/flags.html (right):

https://chromiumcodereview.appspot.com/11633020/diff/1016/chrome/browser/reso...
chrome/browser/resources/flags.html:6: <meta name="viewport"
content="width=device-width, initial-scale=1.0,
I think we only need width and user-scalable.

[gone, 2012-12-21 19:19:00]

https://chromiumcodereview.appspot.com/11633020/diff/1016/chrome/browser/life...
File chrome/browser/lifetime/application_lifetime.cc (right):

https://chromiumcodereview.appspot.com/11633020/diff/1016/chrome/browser/life...
chrome/browser/lifetime/application_lifetime.cc:239: #if !defined(OS_ANDROID)
On 2012/12/20 02:22:31, klobag.chromium wrote:
> Should we share the save session / pref?

An earlier version of this CL actually had that code uncommented out, but it
ended up pulling a bunch of other code that we've never linked.  I'm not
entirely clear on what that code is supposed to do given that we don't normally
run it when Chrome dies on Android, but I'll dig.

https://chromiumcodereview.appspot.com/11633020/diff/1016/chrome/browser/reso...
File chrome/browser/resources/flags.html (right):

https://chromiumcodereview.appspot.com/11633020/diff/1016/chrome/browser/reso...
chrome/browser/resources/flags.html:6: <meta name="viewport"
content="width=device-width, initial-scale=1.0,
On 2012/12/20 02:22:31, klobag.chromium wrote:
> I think we only need width and user-scalable.

Done.

[klobag.chromium, 2013-01-08 01:04:27]

On 2012/12/21 19:19:00, dfalcantara wrote:
>
https://chromiumcodereview.appspot.com/11633020/diff/1016/chrome/browser/life...
> File chrome/browser/lifetime/application_lifetime.cc (right):
> 
>
https://chromiumcodereview.appspot.com/11633020/diff/1016/chrome/browser/life...
> chrome/browser/lifetime/application_lifetime.cc:239: #if !defined(OS_ANDROID)
> On 2012/12/20 02:22:31, klobag.chromium wrote:
> > Should we share the save session / pref?
> 
> An earlier version of this CL actually had that code uncommented out, but it
> ended up pulling a bunch of other code that we've never linked.  I'm not
> entirely clear on what that code is supposed to do given that we don't
normally
> run it when Chrome dies on Android, but I'll dig.
>

What did you find out?
 
>
https://chromiumcodereview.appspot.com/11633020/diff/1016/chrome/browser/reso...
> File chrome/browser/resources/flags.html (right):
> 
>
https://chromiumcodereview.appspot.com/11633020/diff/1016/chrome/browser/reso...
> chrome/browser/resources/flags.html:6: <meta name="viewport"
> content="width=device-width, initial-scale=1.0,
> On 2012/12/20 02:22:31, klobag.chromium wrote:
> > I think we only need width and user-scalable.
> 
> Done.

[gone, 2013-01-08 01:07:45]

On 2013/01/08 01:04:27, klobag.chromium wrote:
> What did you find out?

Unfortunately didn't get to look into this for too long; the last update was
before the holidays and I got pulled onto other bugs after getting back from
vacation.  I'm starting to look into this again now.

[gone, 2013-01-11 02:50:59]

On 2013/01/08 01:07:45, dfalcantara wrote:
> On 2013/01/08 01:04:27, klobag.chromium wrote:
> > What did you find out?
> 
> Unfortunately didn't get to look into this for too long; the last update was
> before the holidays and I got pulled onto other bugs after getting back from
> vacation.  I'm starting to look into this again now.

Turns out we don't share much of the current pathway, at all.
* The pref that gets set (kWasRestarted) is used in startup_browser_creator.cc,
which we don't compile on Android.
* We don't use Browsers or BrowserLists on Android.

What I'm doing now seems to be correct, since the hook it exposes can be used to
trigger the Activity's death on the Android side.

[klobag.chromium, 2013-01-11 03:39:45]

lgtm

[gone, 2013-01-11 19:01:58]

+thakis & sky

Hey guys,
Can you look this over when you've got a chance?

- Dan

[Nico, 2013-01-12 00:16:55]

I would've done the actual flag change in a different CL and just reenabled
about:flags in this CL, but oh well.


Are the upgradedetector changes in this CL related to about:flags? Why?


Other than that, looks fine.

https://chromiumcodereview.appspot.com/11633020/diff/24001/chrome/browser/pre...
File chrome/browser/prefs/browser_prefs.cc (right):

https://chromiumcodereview.appspot.com/11633020/diff/24001/chrome/browser/pre...
chrome/browser/prefs/browser_prefs.cc:166:
UpgradeDetector::RegisterPrefs(local_state);
Is this related?

https://chromiumcodereview.appspot.com/11633020/diff/24001/chrome/chrome_brow...
File chrome/chrome_browser.gypi (left):

https://chromiumcodereview.appspot.com/11633020/diff/24001/chrome/chrome_brow...
chrome/chrome_browser.gypi:2794: 'browser/upgrade_detector.h',
Likewise

[gone, 2013-01-12 00:46:54]

On 2013/01/12 00:16:55, Nico wrote:
> I would've done the actual flag change in a different CL and just reenabled
> about:flags in this CL, but oh well.
I think I was hitting a compile error because of the ENABLE_WEBGL flag; Android
defines one but not the other.

> Are the upgradedetector changes in this CL related to about:flags? Why?
The UpgradeDetector change is only required for its
prefs::kRestartLastSessionOnShutdown registration in
application_lifetime_android.cc... which git ignored during the upload.  I'm
honestly surprised the bots didn't complain about compile failures, but the
files are there now.

Sorry about that.

[Nico, 2013-01-15 20:33:27]

Sorry for the delay.

Two things:
1. Please make sure that restarts through about:flags do not get counted as
crashes / unclean shotdowns. With this, lgtm.

2. If there aren't any yet, please write restart tests for android. This can
happen in a separate CL, in that case please file a tracking bug on you.

https://chromiumcodereview.appspot.com/11633020/diff/30001/chrome/browser/lif...
File chrome/browser/lifetime/application_lifetime.cc (right):

https://chromiumcodereview.appspot.com/11633020/diff/30001/chrome/browser/lif...
chrome/browser/lifetime/application_lifetime.cc:286: // Android doesn't use
Browser.
So you don't need the call to MarkAsCleanShutdown()?

https://chromiumcodereview.appspot.com/11633020/diff/30001/chrome/browser/lif...
File chrome/browser/lifetime/application_lifetime_android.cc (right):

https://chromiumcodereview.appspot.com/11633020/diff/30001/chrome/browser/lif...
chrome/browser/lifetime/application_lifetime_android.cc:30: void
TerminateAndroid() {
Do you have any restart tests (since this code is very separate from the restart
code on other platforms)?

[gone, 2013-01-15 22:18:54]

Thanks!

https://chromiumcodereview.appspot.com/11633020/diff/30001/chrome/browser/lif...
File chrome/browser/lifetime/application_lifetime.cc (right):

https://chromiumcodereview.appspot.com/11633020/diff/30001/chrome/browser/lif...
chrome/browser/lifetime/application_lifetime.cc:286: // Android doesn't use
Browser.
On 2013/01/15 20:33:27, Nico wrote:
> So you don't need the call to MarkAsCleanShutdown()?

Yeah, AreAllBrowsersCloseable() goes through a list of Browsers to see if they
need to be marked as cleanly shutting down, but AFAICT we don't use (or even
compile) browser.cc.

The Android-specific code for terminating the app triggers the normal Android
Activity onPause()/onDestroy() pathway, which should mark it as a clean shutdown
on our side after the corresponding internal CL lands.

https://chromiumcodereview.appspot.com/11633020/diff/30001/chrome/browser/lif...
File chrome/browser/lifetime/application_lifetime_android.cc (right):

https://chromiumcodereview.appspot.com/11633020/diff/30001/chrome/browser/lif...
chrome/browser/lifetime/application_lifetime_android.cc:30: void
TerminateAndroid() {
On 2013/01/15 20:33:27, Nico wrote:
> Do you have any restart tests (since this code is very separate from the
restart
> code on other platforms)?

Not yet.  Shutting down anything on Android is already a strange thing to be
doing, since you're supposed to let Android end your task whenever you feel like
it, but we need to do it to force the native Chromium library to be reloaded
with the new flags in effect.

I can add a bug to track the tests.

[gone, 2013-01-15 22:20:10]

> Not yet.  Shutting down anything on Android is already a strange thing to be
> doing, since you're supposed to let Android end your task whenever you feel
like
> it, but we need to do it to force the native Chromium library to be reloaded
> with the new flags in effect.
Meant to say that Android can kill it whenever necessary; it's highly uncommon
for a program to end itself in the traditional sense.

[commit-bot: I haz the power, 2013-01-15 22:45:42]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/dfalcantara@chromium.org/11633020/44015

[nilesh, 2013-01-16 00:54:21]

LGTM for patch 14

[commit-bot: I haz the power, 2013-01-16 00:56:56]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/dfalcantara@chromium.org/11633020/57001

[commit-bot: I haz the power, 2013-01-16 02:26:30]

Retried try job too often on win for step(s) compile

[commit-bot: I haz the power, 2013-01-16 02:48:00]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/dfalcantara@chromium.org/11633020/57001

[commit-bot: I haz the power, 2013-01-16 05:31:03]

Change committed as 177088