Port custom_tabs_client from GYP to GN

Port custom_tabs_client from GYP to GN

BUG=535071
TEST=None

Committed: https://crrev.com/0a2255e0c2f960fc29c467d1b6a1ecc0dd49e14b
Cr-Commit-Position: refs/heads/master@{#352617}

[pkotwicz, 2015-09-26 03:28:04]

pkotwicz@chromium.org changed reviewers:
+ agrieve@chromium.org

[pkotwicz, 2015-09-26 03:28:04]

agrieve@ PTAL

[agrieve, 2015-09-28 14:52:49]

lgtm

https://codereview.chromium.org/1358243006/diff/1/build/config/android/rules.gni
File build/config/android/rules.gni (right):

https://codereview.chromium.org/1358243006/diff/1/build/config/android/rules....
build/config/android/rules.gni:1380: assert(run_findbugs || true)  # Mark as
used.
I think if you move this into java_library_impl, you can mark:

https://code.google.com/p/chromium/issues/detail?id=536092

as fixed :)

https://codereview.chromium.org/1358243006/diff/1/build/config/android/rules....
build/config/android/rules.gni:1388: assert(android_manifest != "")  # Mark as
used.
I think this should also be moved to within java_library_impl since it's the one
not properly marking it as used.

[pkotwicz, 2015-09-28 20:17:18]

pkotwicz@chromium.org changed reviewers:
+ pasko@chromium.org, yfriedman@google.com

[pkotwicz, 2015-09-28 20:17:18]

pasko@ for third_party/custom_tabs_client
yfriedman@ for tools/android

[Yaron, 2015-09-28 20:40:03]

yfriedman@chromium.org changed reviewers:
+ yfriedman@chromium.org

[Yaron, 2015-09-28 20:40:04]

looks like you still need a BUILD.gn OWNER

https://codereview.chromium.org/1358243006/diff/20001/tools/android/BUILD.gn
File tools/android/BUILD.gn (right):

https://codereview.chromium.org/1358243006/diff/20001/tools/android/BUILD.gn#...
tools/android/BUILD.gn:16: # GYP:
//android/tools/android_tools.gyp:customtabs_benchmark
I think this and the other target are //tools/android/android_tool.gyp:*

[pkotwicz, 2015-09-28 20:48:36]

Yaron, can you please take another look?

https://codereview.chromium.org/1358243006/diff/20001/tools/android/BUILD.gn
File tools/android/BUILD.gn (right):

https://codereview.chromium.org/1358243006/diff/20001/tools/android/BUILD.gn#...
tools/android/BUILD.gn:16: # GYP:
//android/tools/android_tools.gyp:customtabs_benchmark
On 2015/09/28 20:40:04, Yaron wrote:
> I think this and the other target are //tools/android/android_tool.gyp:*

Done.

[Yaron, 2015-09-28 20:49:28]

oops, meant to lgtm last round :)

[pasko, 2015-09-29 09:31:38]

pasko@chromium.org changed reviewers:
+ ianwen@chromium.org

[pasko, 2015-09-29 09:38:58]

Thanks for doing this! I agree with the general approach, but cannot l-g
immediately, a few questions first.

also, ianwen: FYI, please take a look at:
third_party/custom_tabs_client/BUILD.gn

https://codereview.chromium.org/1358243006/diff/40001/BUILD.gn
File BUILD.gn (right):

https://codereview.chromium.org/1358243006/diff/40001/BUILD.gn#newcode224
BUILD.gn:224: "//third_party/custom_tabs_client:custom_tabs_client_example_apk",
this puts custom_tabs_client_example_apk as a dependency to all android target
or I am reading it wrong?

The GYP target does not put itself anywhere except the All target, and also
conditionalized on customtabs==0.

I thought we trying to mirror the logic with GYP, then I am lacking context on
why these particular bits were decided not to be replicated.

Please educate me :)

https://codereview.chromium.org/1358243006/diff/40001/build/config/android/in...
File build/config/android/internal_rules.gni (right):

https://codereview.chromium.org/1358243006/diff/40001/build/config/android/in...
build/config/android/internal_rules.gni:1189: if
(defined(invoker.android_manifest)) {
I am not familiar with the concept of invoker, is there a place to read about
it?

[pkotwicz, 2015-09-29 18:56:32]

https://codereview.chromium.org/1358243006/diff/40001/build/config/android/in...
File build/config/android/internal_rules.gni (right):

https://codereview.chromium.org/1358243006/diff/40001/build/config/android/in...
build/config/android/internal_rules.gni:1189: if
(defined(invoker.android_manifest)) {
The GN documentation is checked into the chromium source tree. "invoker" is
documented here:
https://chromium.googlesource.com/chromium/src/+/master/tools/gn/docs/languag...

I have also added a link to moma/ to the GN documentation (if you search for
"chromium GN")

[pkotwicz, 2015-09-29 19:23:38]

Patchset #4 (id:60001) has been deleted

[pkotwicz, 2015-09-29 19:25:10]

Patchset #4 (id:80001) has been deleted

[pasko, 2015-09-29 20:14:26]

https://codereview.chromium.org/1358243006/diff/40001/build/config/android/in...
File build/config/android/internal_rules.gni (right):

https://codereview.chromium.org/1358243006/diff/40001/build/config/android/in...
build/config/android/internal_rules.gni:1189: if
(defined(invoker.android_manifest)) {
On 2015/09/29 18:56:32, pkotwicz wrote:
> The GN documentation is checked into the chromium source tree. "invoker" is
> documented here:
>
https://chromium.googlesource.com/chromium/src/+/master/tools/gn/docs/languag...
> 
> I have also added a link to moma/ to the GN documentation (if you search for
> "chromium GN")

thanks! now clearer

[Ian Wen, 2015-09-29 22:19:13]

https://codereview.chromium.org/1358243006/diff/120001/third_party/custom_tab...
File third_party/custom_tabs_client/BUILD.gn (right):

https://codereview.chromium.org/1358243006/diff/120001/third_party/custom_tab...
third_party/custom_tabs_client/BUILD.gn:41:
android_aidl("chrome_custom_tabs_service_aidl") {
In gyp we have to make a common.aidl to let two aidl files compile. Do we still
have the same constraint in GN? I know in Android Studio/Gradle, common.aidl is
not needed.

[pkotwicz, 2015-09-30 00:44:58]

Patchset #6 (id:140001) has been deleted

[pkotwicz, 2015-09-30 00:53:54]

pasko@: I have made BUILD.gn match all.gyp more closely (This CL now depends on
https://codereview.chromium.org/1379693002/ which is in the commit queue)

ianwen@: What is the best way of figuring if chrome_custom_tabs_service_aidl is
needed? I tried removing the dependency of custom_tabs_support_lib on
chrome_custom_tabs_service_aidl in BUILD.gn and custom_tabs_client_example_apk
does not compile

[Ian Wen, 2015-09-30 01:03:51]

On 2015/09/30 00:53:54, pkotwicz wrote:
> pasko@: I have made BUILD.gn match all.gyp more closely (This CL now depends
on
> https://codereview.chromium.org/1379693002/ which is in the commit queue)
> 
> ianwen@: What is the best way of figuring if chrome_custom_tabs_service_aidl
is
> needed? I tried removing the dependency of custom_tabs_support_lib on
> chrome_custom_tabs_service_aidl in BUILD.gn and custom_tabs_client_example_apk
> does not compile

chrome_custom_tabs_service_aidl is definitely required to compile the apk. What
I was asking is that can we make two android_aidl target instead of one?
Something like:

android_aidl("custom_tabs_callback_aidl") {
  interface_file =
"src/customtabs/src/android/support/customtabs/ICustomTabsCallback.aidl"
}

android_aidl("custom_tabs_service_aidl") {
  interface_file =
"src/customtabs/src/android/support/customtabs/ICustomTabsService.aidl"
}

In gyp our previous experiments showed having two aidl targets is impossible in
gyp, so we added a common.aidl to group them together. Now I am wondering if GN
is smarter and is able to handle two aidl targets.

Anyway my question doesn't affect the correctness of this CL, it is just a thing
that is worth checking.

lgtm

[pasko, 2015-09-30 07:20:19]

lgtm

[pkotwicz, 2015-09-30 14:51:57]

dpranke@ can you please take a look at the changes in the root BUILD.gn file?

[pkotwicz, 2015-09-30 17:26:44]

ianwen@: It looks like common.aidl is still necessary with GN

[pkotwicz, 2015-09-30 17:27:03]

pkotwicz@chromium.org changed reviewers:
+ dpranke@google.com

[pkotwicz, 2015-09-30 17:27:14]

dpranke@ can you please take a look at the changes in the root BUILD.gn file?

[pkotwicz, 2015-09-30 17:27:40]

pkotwicz@chromium.org changed reviewers:
+ dpranke@chromium.org
- dpranke@google.com

[Ian Wen, 2015-09-30 17:40:15]

On 2015/09/30 17:26:44, pkotwicz wrote:
> ianwen@: It looks like common.aidl is still necessary with GN

Gotcha. Thanks!

[Dirk Pranke, 2015-09-30 18:07:00]

lgtm

[pasko, 2015-10-02 09:47:13]

why is this not in CQ yet? :)

[pkotwicz, 2015-10-02 17:28:29]

This is not in the CQ yet because I was a sheriff yesterday and today

[pkotwicz, 2015-10-05 18:22:33]

The CQ bit was checked by pkotwicz@chromium.org

[pkotwicz, 2015-10-05 18:22:33]

The patchset sent to the CQ was uploaded after l-g-t-m from
agrieve@chromium.org, yfriedman@chromium.org, dpranke@chromium.org,
pasko@chromium.org, ianwen@chromium.org
Link to the patchset: https://codereview.chromium.org/1358243006/#ps180001
(title: " ")

[commit-bot: I haz the power, 2015-10-05 18:24:14]

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

[commit-bot: I haz the power, 2015-10-05 23:51:29]

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

[commit-bot: I haz the power, 2015-10-05 23:51:30]

Try jobs failed on following builders:
  linux_android_rel_ng on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_android_r...)

[pkotwicz, 2015-10-06 00:08:57]

The CQ bit was checked by pkotwicz@chromium.org

[pkotwicz, 2015-10-06 00:09:01]

The CQ bit was unchecked by pkotwicz@chromium.org

[pkotwicz, 2015-10-06 15:03:07]

The CQ bit was checked by pkotwicz@chromium.org

[commit-bot: I haz the power, 2015-10-06 15:04:26]

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

[commit-bot: I haz the power, 2015-10-06 16:29:14]

Committed patchset #7 (id:180001)

[commit-bot: I haz the power, 2015-10-06 16:30:28]

Patchset 7 (id:??) landed as
https://crrev.com/0a2255e0c2f960fc29c467d1b6a1ecc0dd49e14b
Cr-Commit-Position: refs/heads/master@{#352617}