Aura on Android: Compile extensions gn target.

Aura on Android: Compile extensions gn target.

BUG=548797
Committed: https://crrev.com/d3b26c09b19052043a33bacbd9a3e4250f2eab0e
Cr-Commit-Position: refs/heads/master@{#360642}

[Hadi, 2015-11-10 20:54:47]

PTAL. These changes are enough to compile extensions gn target after applying
CL's 1416513006 and 1434663004.

The changes are browser_navigate_params.{h,cc} are to compile
chrome_runtime_api_delegate.cc and extension_tab_util.cc.

[Hadi, 2015-11-10 20:55:31]

moshayedi@chromium.org changed reviewers:
+ mfomitchev@chromium.org, sievers@chromium.org

[Hadi, 2015-11-10 20:55:31]

(forgot to add reviewers in the previous message)

PTAL. These changes are enough to compile extensions gn target after applying
CL's 1416513006 and 1434663004.

The changes are browser_navigate_params.{h,cc} are to compile
chrome_runtime_api_delegate.cc and extension_tab_util.cc.

[mfomitchev, 2015-11-10 21:27:28]

https://codereview.chromium.org/1419103010/diff/60001/build/config/features.gni
File build/config/features.gni (right):

https://codereview.chromium.org/1419103010/diff/60001/build/config/features.g...
build/config/features.gni:27: enable_plugins = (!is_android && !is_ios) ||
is_chromecast || use_aura
as @sievers pointed out in another CL, this could be simplified:
is_mac || use_aura || is_chromecast

https://codereview.chromium.org/1419103010/diff/60001/chrome/browser/extensio...
File
chrome/browser/extensions/api/image_writer_private/image_writer_private_api.cc
(right):

https://codereview.chromium.org/1419103010/diff/60001/chrome/browser/extensio...
chrome/browser/extensions/api/image_writer_private/image_writer_private_api.cc:9:
#include <jni.h>
There were some suggestions in https://codereview.chromium.org/1415733008/ on a
better way to do this. Also, it's probably best to do it in a separate CL.

https://codereview.chromium.org/1419103010/diff/60001/chrome/browser/extensio...
File chrome/browser/extensions/component_loader.cc (right):

https://codereview.chromium.org/1419103010/diff/60001/chrome/browser/extensio...
chrome/browser/extensions/component_loader.cc:69: #if defined(ENABLE_APP_LIST)
|| defined(ENABLE_EXTENSIONS)
No need for this.

https://codereview.chromium.org/1419103010/diff/60001/chrome/browser/ui/brows...
File chrome/browser/ui/browser_navigator_params.h (right):

https://codereview.chromium.org/1419103010/diff/60001/chrome/browser/ui/brows...
chrome/browser/ui/browser_navigator_params.h:59: #if !defined(OS_ANDROID) ||
defined(USE_AURA)
I don't see any changes to browser_navigator_params.h/cc in auraclank2. Do you
have any idea why we didn't need them in the branch, but we need them now? I
don't see any recent changes in chrome_runtime_api_delegate, which could've
affect this.

[Hadi, 2015-11-12 16:33:36]

https://codereview.chromium.org/1419103010/diff/60001/build/config/features.gni
File build/config/features.gni (right):

https://codereview.chromium.org/1419103010/diff/60001/build/config/features.g...
build/config/features.gni:27: enable_plugins = (!is_android && !is_ios) ||
is_chromecast || use_aura
On 2015/11/10 21:27:28, mfomitchev wrote:
> as @sievers pointed out in another CL, this could be simplified:
> is_mac || use_aura || is_chromecast

Done. Also checking for defined(use_aura) to avoid errors when use_aura is not
defined explicitly.

https://codereview.chromium.org/1419103010/diff/60001/chrome/browser/extensio...
File
chrome/browser/extensions/api/image_writer_private/image_writer_private_api.cc
(right):

https://codereview.chromium.org/1419103010/diff/60001/chrome/browser/extensio...
chrome/browser/extensions/api/image_writer_private/image_writer_private_api.cc:9:
#include <jni.h>
On 2015/11/10 21:27:28, mfomitchev wrote:
> There were some suggestions in https://codereview.chromium.org/1415733008/ on
a
> better way to do this. Also, it's probably best to do it in a separate CL.

Fixing this in https://codereview.chromium.org/1432293003/.

https://codereview.chromium.org/1419103010/diff/60001/chrome/browser/extensio...
File chrome/browser/extensions/component_loader.cc (right):

https://codereview.chromium.org/1419103010/diff/60001/chrome/browser/extensio...
chrome/browser/extensions/component_loader.cc:69: #if defined(ENABLE_APP_LIST)
|| defined(ENABLE_EXTENSIONS)
On 2015/11/10 21:27:28, mfomitchev wrote:
> No need for this.

If we don't enable app list in features.gni, then we'll need this to compile. In
our local discussion we thought we can keep app list as off for now.

https://codereview.chromium.org/1419103010/diff/60001/chrome/browser/ui/brows...
File chrome/browser/ui/browser_navigator_params.h (right):

https://codereview.chromium.org/1419103010/diff/60001/chrome/browser/ui/brows...
chrome/browser/ui/browser_navigator_params.h:59: #if !defined(OS_ANDROID) ||
defined(USE_AURA)
On 2015/11/10 21:27:28, mfomitchev wrote:
> I don't see any changes to browser_navigator_params.h/cc in auraclank2. Do you
> have any idea why we didn't need them in the branch, but we need them now? I
> don't see any recent changes in chrome_runtime_api_delegate, which could've
> affect this.

I spent some time on this and couldn't figure out why we didn't need this in
auraclank2.

I think somehow chrome_runtime_api_delegate.cc and extension_tab_util.cc don't
get compiled there, but I couldn't figure out what we did it to disable them.

[mfomitchev, 2015-11-12 16:55:22]

https://codereview.chromium.org/1419103010/diff/80001/build/config/features.gni
File build/config/features.gni (right):

https://codereview.chromium.org/1419103010/diff/80001/build/config/features.g...
build/config/features.gni:27: enable_plugins = is_mac || is_chromecast ||
(defined(use_aura) && use_aura)
The is_mac simplification doesn't work when use_aura is undefined.

[Hadi, 2015-11-12 17:34:04]

https://codereview.chromium.org/1419103010/diff/80001/build/config/features.gni
File build/config/features.gni (right):

https://codereview.chromium.org/1419103010/diff/80001/build/config/features.g...
build/config/features.gni:27: enable_plugins = is_mac || is_chromecast ||
(defined(use_aura) && use_aura)
On 2015/11/12 16:55:22, mfomitchev wrote:
> The is_mac simplification doesn't work when use_aura is undefined.

Yeah, this change was causing some trybots to fail.

Done. Also added comments to explain why we check for defined(use_aura).

[Hadi, 2015-11-12 18:25:33]

On 2015/11/12 17:34:04, Hadi wrote:
>
https://codereview.chromium.org/1419103010/diff/80001/build/config/features.gni
> File build/config/features.gni (right):
> 
>
https://codereview.chromium.org/1419103010/diff/80001/build/config/features.g...
> build/config/features.gni:27: enable_plugins = is_mac || is_chromecast ||
> (defined(use_aura) && use_aura)
> On 2015/11/12 16:55:22, mfomitchev wrote:
> > The is_mac simplification doesn't work when use_aura is undefined.
> 
> Yeah, this change was causing some trybots to fail.
> 
> Done. Also added comments to explain why we check for defined(use_aura).

Updated the comment similar to the suggestion in CL 1416513006.

[mfomitchev, 2015-11-12 18:30:23]

LGTM

[Hadi, 2015-11-12 18:40:40]

moshayedi@chromium.org changed reviewers:
+ brettw@chromium.org, reillyg@chromium.org, sky@chromium.org

[Hadi, 2015-11-12 18:40:41]

+sky@ for chrome/browser/ui OWNERS
+reillyg@ for chrome/browser/extensions OWNERS
+brettw@ for build/config OWNERS

thanks.

[brettw, 2015-11-12 18:56:27]

https://codereview.chromium.org/1419103010/diff/160001/build/config/features.gni
File build/config/features.gni (right):

https://codereview.chromium.org/1419103010/diff/160001/build/config/features....
build/config/features.gni:28: # use_aura is assigned a value in ui.gni, so it
will be undefined here unless
The fact that build args are set globally is a bug I'd like to fix. The global
build args should only be set after the corresponding thing is in a declare_args
block. So please don't write code that depends on this behavior.

We could move stuff around to express this nicely. But it doesn't follow to me
that if I enable aura on Android that I get plugins support. These seems sort of
independent, it just so happens that you're only bothering to implement plugins
on aura. So maybe just leave aura out of the defaults and have people explicitly
enable plugins on android if they want them.

If we do that you should note in the comment that it's supported on Android only
in aura.

[Reilly Grant (use Gerrit), 2015-11-12 19:34:16]

https://codereview.chromium.org/1419103010/diff/160001/chrome/browser/extensi...
File chrome/browser/extensions/component_loader.cc (right):

https://codereview.chromium.org/1419103010/diff/160001/chrome/browser/extensi...
chrome/browser/extensions/component_loader.cc:69: #if defined(ENABLE_APP_LIST)
|| defined(ENABLE_EXTENSIONS)
Since this file should only be compiled if ENABLE_EXTENSIONS then this #if can
simply be removed.

[sky, 2015-11-12 22:07:38]

LGTM

[Hadi, 2015-11-17 16:49:58]

https://codereview.chromium.org/1419103010/diff/160001/build/config/features.gni
File build/config/features.gni (right):

https://codereview.chromium.org/1419103010/diff/160001/build/config/features....
build/config/features.gni:28: # use_aura is assigned a value in ui.gni, so it
will be undefined here unless
On 2015/11/12 18:56:27, brettw wrote:
> The fact that build args are set globally is a bug I'd like to fix. The global
> build args should only be set after the corresponding thing is in a
declare_args
> block. So please don't write code that depends on this behavior.
> 
> We could move stuff around to express this nicely. But it doesn't follow to me
> that if I enable aura on Android that I get plugins support. These seems sort
of
> independent, it just so happens that you're only bothering to implement
plugins
> on aura. So maybe just leave aura out of the defaults and have people
explicitly
> enable plugins on android if they want them.
> 
> If we do that you should note in the comment that it's supported on Android
only
> in aura.

Removed the changes to features.gni per internal discussion. Planning to address
this change and similar changes to build config in a separate CL.

https://codereview.chromium.org/1419103010/diff/160001/chrome/browser/extensi...
File chrome/browser/extensions/component_loader.cc (right):

https://codereview.chromium.org/1419103010/diff/160001/chrome/browser/extensi...
chrome/browser/extensions/component_loader.cc:69: #if defined(ENABLE_APP_LIST)
|| defined(ENABLE_EXTENSIONS)
On 2015/11/12 19:34:15, Reilly Grant wrote:
> Since this file should only be compiled if ENABLE_EXTENSIONS then this #if can
> simply be removed.

Done.

[Reilly Grant (use Gerrit), 2015-11-19 18:11:47]

lgtm

[Hadi, 2015-11-19 18:31:28]

The CQ bit was checked by moshayedi@chromium.org

[Hadi, 2015-11-19 18:31:29]

The patchset sent to the CQ was uploaded after l-g-t-m from sky@chromium.org,
mfomitchev@chromium.org
Link to the patchset: https://codereview.chromium.org/1419103010/#ps180001
(title: "Addressed feedback from patchset 9.")

[commit-bot: I haz the power, 2015-11-19 18:32:21]

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

[commit-bot: I haz the power, 2015-11-19 19:29:44]

Committed patchset #10 (id:180001)

[commit-bot: I haz the power, 2015-11-19 19:30:33]

Patchset 10 (id:??) landed as
https://crrev.com/d3b26c09b19052043a33bacbd9a3e4250f2eab0e
Cr-Commit-Position: refs/heads/master@{#360642}