Activating code to start Chrome UI in Android Aura Chrome

Activating code to start Chrome UI in Android Aura Chrome

BUG=561669
Committed: https://crrev.com/92cb641686c41adcc5ceb80d780b6de6471d6191
Cr-Commit-Position: refs/heads/master@{#363216}

[bshe, 2015-11-24 18:44:37]

Patchset #2 (id:20001) has been deleted

[bshe, 2015-11-24 19:24:57]

bshe@chromium.org changed reviewers:
+ mfomitchev@chromium.org

[bshe, 2015-11-24 19:24:57]

Hi Mikhail.
Do you mind take a look at this CL?
It tries to upstream chrome_browser_main.* files in:
https://codereview.chromium.org/1391893003
I followed most of the ifdefs in the CL except for these two places:
1.
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/chr...
It deals with variation_service here but it is not clear why do we need it. So
keep it unchanged and we may or may not need to fix it in the future.
2.
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/chr...
This is addressed in Daniel's CL which introduces ANDROID_JAVA_UI.

Thanks

https://codereview.chromium.org/1473693002/diff/60001/chrome/browser/chrome_b...
File chrome/browser/chrome_browser_main.cc (right):

https://codereview.chromium.org/1473693002/diff/60001/chrome/browser/chrome_b...
chrome/browser/chrome_browser_main.cc:1626:
ThreadWatcherAndroid::RegisterApplicationStatusListener();
I am not sure what is this for. It looks like we use this class to watch for
state change for clank. Perhaps we want it for Aura Android too? If so, I can
file a bug to track it. This is change is copied from our prototype. So it is
probably necessary to make Aura Android compile and run. I will write a TODO if
this needs to work for Aura Android.

[bshe, 2015-11-25 19:44:08]

Description was changed from

==========
Activating code to start Chrome UI in Android Aura Chrome


BUG=507792
==========

to

==========
Activating code to start Chrome UI in Android Aura Chrome


BUG=561669
==========

[bshe, 2015-11-30 21:04:14]

bshe@chromium.org changed reviewers:
+ sky@chromium.org

[bshe, 2015-11-30 21:04:14]

friendly ping. also +sky for owner?

[mfomitchev, 2015-11-30 21:34:17]

https://codereview.chromium.org/1473693002/diff/60001/chrome/browser/chrome_b...
File chrome/browser/chrome_browser_main.cc (right):

https://codereview.chromium.org/1473693002/diff/60001/chrome/browser/chrome_b...
chrome/browser/chrome_browser_main.cc:1626:
ThreadWatcherAndroid::RegisterApplicationStatusListener();
On 2015/11/24 19:24:57, bshe wrote:
> I am not sure what is this for. It looks like we use this class to watch for
> state change for clank. Perhaps we want it for Aura Android too? If so, I can
> file a bug to track it. This is change is copied from our prototype. So it is
> probably necessary to make Aura Android compile and run. I will write a TODO
if
> this needs to work for Aura Android.

From a quick look, the class's purpose is deactivating the thread watcher when
the Activity is stopped and reactivating it when the Activity is restarted - in
order to preserve battery. So it seems like we'll want to use this on Aura
Android as well, albeit it's not urgent to make it work. So a TODO/crbug seems
like the right approach here.
it seems we'd want this on Aura Android. It's

[bshe, 2015-12-03 14:31:00]

Patchset #4 (id:80001) has been deleted

[bshe, 2015-12-03 14:33:05]

Filed a bug. PTAL. Thanks!

https://codereview.chromium.org/1473693002/diff/60001/chrome/browser/chrome_b...
File chrome/browser/chrome_browser_main.cc (right):

https://codereview.chromium.org/1473693002/diff/60001/chrome/browser/chrome_b...
chrome/browser/chrome_browser_main.cc:1626:
ThreadWatcherAndroid::RegisterApplicationStatusListener();
On 2015/11/30 21:34:17, mfomitchev wrote:
> On 2015/11/24 19:24:57, bshe wrote:
> > I am not sure what is this for. It looks like we use this class to watch for
> > state change for clank. Perhaps we want it for Aura Android too? If so, I
can
> > file a bug to track it. This is change is copied from our prototype. So it
is
> > probably necessary to make Aura Android compile and run. I will write a TODO
> if
> > this needs to work for Aura Android.
> 
> From a quick look, the class's purpose is deactivating the thread watcher when
> the Activity is stopped and reactivating it when the Activity is restarted -
in
> order to preserve battery. So it seems like we'll want to use this on Aura
> Android as well, albeit it's not urgent to make it work. So a TODO/crbug seems
> like the right approach here.
> it seems we'd want this on Aura Android. It's 

Added a TODO.

[mfomitchev, 2015-12-03 17:56:26]

https://codereview.chromium.org/1473693002/diff/100001/chrome/browser/chrome_...
File chrome/browser/chrome_browser_main.cc (right):

https://codereview.chromium.org/1473693002/diff/100001/chrome/browser/chrome_...
chrome/browser/chrome_browser_main.cc:1770: #if !defined(OS_ANDROID)
We are already inside and #else (for #if in line 1684), so !OS_ANDROID holds
here, so I don't think we need another #if.

https://codereview.chromium.org/1473693002/diff/100001/chrome/browser/chrome_...
chrome/browser/chrome_browser_main.cc:1774: #endif  // !defined(OS_ANDROID) &&
!defined(USE_AURA)
Notably, we should never have
!defined(OS_ANDROID) && !defined(USE_AURA)
Either
defined(OS_ANDROID) && !defined(USE_AURA)
or
!defined(OS_ANDROID) || defined(USE_AURA)
It's easy to mix this up, this is one reason Dan's new flag is great.

[bshe, 2015-12-03 18:18:18]

https://codereview.chromium.org/1473693002/diff/100001/chrome/browser/chrome_...
File chrome/browser/chrome_browser_main.cc (right):

https://codereview.chromium.org/1473693002/diff/100001/chrome/browser/chrome_...
chrome/browser/chrome_browser_main.cc:1770: #if !defined(OS_ANDROID)
On 2015/12/03 17:56:25, mfomitchev wrote:
> We are already inside and #else (for #if in line 1684), so !OS_ANDROID holds
> here, so I don't think we need another #if.

Good catch! Looks like I accidentally removed my change at line 1684 when try to
avoid a conflict with Dan's CL(His CL touches the same file and I dont want to
create conflict). I have added the line back. Basically, the #else condition
should not be !android. It should be !android || use_aura.
process_power_collector_ isn't used in Android in our prototype.

https://codereview.chromium.org/1473693002/diff/100001/chrome/browser/chrome_...
chrome/browser/chrome_browser_main.cc:1774: #endif  // !defined(OS_ANDROID) &&
!defined(USE_AURA)
On 2015/12/03 17:56:25, mfomitchev wrote:
> Notably, we should never have
> !defined(OS_ANDROID) && !defined(USE_AURA)
> Either
> defined(OS_ANDROID) && !defined(USE_AURA)
> or
> !defined(OS_ANDROID) || defined(USE_AURA)
> It's easy to mix this up, this is one reason Dan's new flag is great.

oops. missed this one. Done

[mfomitchev, 2015-12-03 18:47:08]

LGTM

[sky, 2015-12-03 20:25:17]

LGTM

[bshe, 2015-12-03 21:55:20]

The CQ bit was checked by bshe@chromium.org

[commit-bot: I haz the power, 2015-12-03 21:57:02]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1473693002/120001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1473693002/120001

[commit-bot: I haz the power, 2015-12-04 00:08:40]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-12-04 00:08:40]

Try jobs failed on following builders:
  win8_chromium_ng on tryserver.chromium.win (JOB_TIMED_OUT, no build URL)
  win_chromium_compile_dbg_ng on tryserver.chromium.win (JOB_TIMED_OUT, no build
URL)
  win_chromium_rel_ng on tryserver.chromium.win (JOB_TIMED_OUT, no build URL)
  win_chromium_x64_rel_ng on tryserver.chromium.win (JOB_TIMED_OUT, no build
URL)

[bshe, 2015-12-04 15:35:16]

The CQ bit was checked by bshe@chromium.org

[commit-bot: I haz the power, 2015-12-04 15:35:41]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1473693002/120001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1473693002/120001

[commit-bot: I haz the power, 2015-12-04 16:21:47]

Description was changed from

==========
Activating code to start Chrome UI in Android Aura Chrome


BUG=561669
==========

to

==========
Activating code to start Chrome UI in Android Aura Chrome


BUG=561669
==========

[commit-bot: I haz the power, 2015-12-04 16:21:48]

Committed patchset #5 (id:120001)

[commit-bot: I haz the power, 2015-12-04 16:22:58]

Description was changed from

==========
Activating code to start Chrome UI in Android Aura Chrome


BUG=561669
==========

to

==========
Activating code to start Chrome UI in Android Aura Chrome

BUG=561669

Committed: https://crrev.com/92cb641686c41adcc5ceb80d780b6de6471d6191
Cr-Commit-Position: refs/heads/master@{#363216}
==========

[commit-bot: I haz the power, 2015-12-04 16:22:59]

Patchset 5 (id:??) landed as
https://crrev.com/92cb641686c41adcc5ceb80d780b6de6471d6191
Cr-Commit-Position: refs/heads/master@{#363216}

[aboxhall, 2015-12-04 22:35:40]

A revert of this CL (patchset #5 id:120001) has been created in
https://codereview.chromium.org/1499943004/ by aboxhall@chromium.org.

The reason for reverting is: Potential cause of many layout test crashes:
https://build.chromium.org/p/chromium.webkit/builders/WebKit%20Linux%20%28dbg....