customtabs: Convert to the new AIDL interface.

customtabs: Convert to the new AIDL interface, temporarily imported.

- Update the service implementation and the matching tests for Custom
  Tabs to the current external AIDL interfaces
- Temporarily import the files in Chrome
- Remove the chromium AIDL files
- Update GYP and GN files

TESTED=unittests and example application

[lizeb1, 2015-07-06 15:17:01]

lizeb@google.com changed reviewers:
+ lizeb@google.com

[lizeb1, 2015-07-06 15:17:01]

Hello yusufo,

This is the CL converting the current service to the new AIDL interface.
A lot of renamings, one less test, one more test.

Note that this is not ready to be submitted now, as the file paths may not be
correct, and for some reasons the chrome_test_apk doesn't compile due to a
proguard failure.

[lizeb1, 2015-07-06 15:17:27]

lizeb@google.com changed reviewers:
+ pasko@chromium.org, yusufo@chromium.org

[lizeb1, 2015-07-06 15:17:27]

Hello yusufo,

This is the CL converting the current service to the new AIDL interface.
A lot of renamings, one less test, one more test.

Note that this is not ready to be submitted now, as the file paths may not be
correct, and for some reasons the chrome_test_apk doesn't compile due to a
proguard failure.

[Yusuf, 2015-07-08 23:15:55]

Note that we need an update for the third_party parts.

Looks good overall. Just a few initial comments.

https://codereview.chromium.org/1224553003/diff/60001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/customtabs/CustomTabIntentDataProvider.java
(right):

https://codereview.chromium.org/1224553003/diff/60001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/customtabs/CustomTabIntentDataProvider.java:46:
"android.support.CUSTOM_TABS:keep_alive";
update all leftover extras here to be
android.support.customtabs.extra.CAPITALIZED_NAME

https://codereview.chromium.org/1224553003/diff/60001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/customtabs/CustomTabIntentDataProvider.java:103:
mSession = IntentUtils.safeGetBinderExtra(intent,
CustomTabsIntent.EXTRA_SESSION);
One thing that we should ensure is supporting a null as the IBinder here. If the
intent has the extra, we should allow them to customize the UI and not enforce
going through service binding, just to get a new session.

Can we create a new IBinder here if the extra is there but null. So hasExtra
returns true but the value when we get it is null?

https://codereview.chromium.org/1224553003/diff/60001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/customtabs/CustomTabsConnection.java
(right):

https://codereview.chromium.org/1224553003/diff/60001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/customtabs/CustomTabsConnection.java:210:
public boolean mayLaunchUrl(ICustomTabsCallback callback, Uri url, final Bundle
extras,
This is back to String now. :)

https://codereview.chromium.org/1224553003/diff/60001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/customtabs/CustomTabsConnection.java:409:
ok = mApplication.getApplicationContext().bindService(
I think we want this for low end devices only as a starting point.

[pasko, 2015-07-09 13:37:13]

sorry for jumping in late. Not using third_party would be the easier/faster way
to move forward. Details below.

https://codereview.chromium.org/1224553003/diff/80001/third_party/android_cus...
File third_party/android_customtabs/README.chromium (right):

https://codereview.chromium.org/1224553003/diff/80001/third_party/android_cus...
third_party/android_customtabs/README.chromium:8: Part of the Custom Tabs
support library.
If I were a third_party reviewer, I would not be happy with it because this
needs an extra explanation what part is imported, the fact that we commit to
removing this dependency soon, etc.

The review process is not the most lightweight (eng/oss/security):
https://www.chromium.org/developers/adding-3rd-party-libraries

We are not importing anything from a third party, the code is authored by our
team of chromium committers, and we can put it under the same license as all
chromium for the temporary period of time until we update the android support
library we use, and the equivalent code there gets used.

https://codereview.chromium.org/1224553003/diff/80001/third_party/android_cus...
File third_party/android_customtabs/android_customtabs.gyp (right):

https://codereview.chromium.org/1224553003/diff/80001/third_party/android_cus...
third_party/android_customtabs/android_customtabs.gyp:22: 'aidl_interface_file':
'customtabs/src/android/support/customtabs/common.aidl',
common.aidl is not necessary, you can create two targets, see:

https://chromium.googlesource.com/external/github.com/GoogleChrome/custom-tab...

https://codereview.chromium.org/1224553003/diff/80001/third_party/android_cus...
File third_party/android_customtabs/customtabs/api/current.txt (right):

https://codereview.chromium.org/1224553003/diff/80001/third_party/android_cus...
third_party/android_customtabs/customtabs/api/current.txt:18: public abstract
class CustomTabsService exrends android.app.Service {
extends?

[Benoit L, 2015-07-09 19:01:11]

Hello all,

This is a CL temporarily importing the customtabs support library inside
Chromium. There are a few things to note here:

- Proguard updates are here to:
  - make chrome_test_apk compile in release mode
  - Silence the error from reflection in IntentUtils.
- IntentUtils has to use reflection. I have verified (by looking at AOSP source
code) that the method we call through reflection does exist and does the right
thing for all the versions we support (the code hasn't changed).
- The code under android/support/customtabs will *not* stay in Chromium. It is
here to allow us to ship M45 with the proper names, constants, and interfaces.
It should *not* be updated.

Please review the code under org/chromium, which will stay in Chrome.


mariakhomenko: please review build files, proguard, IntentUtils changes.
yusufo, pasko: everything :-)

Thanks!

[Benoit L, 2015-07-09 19:02:36]

https://codereview.chromium.org/1224553003/diff/120001/chrome/android/java/sr...
File chrome/android/java/src/android/support/customtabs/CustomTabsIntent.java
(right):

https://codereview.chromium.org/1224553003/diff/120001/chrome/android/java/sr...
chrome/android/java/src/android/support/customtabs/CustomTabsIntent.java:87:
@SuppressLint("NewApi") // This is *never* called in Chrome, only for the
clients.
Note: I had to add this line to silence lint. We do not call this method in
Chrome (but still use this class), so using reflection as in IntentUtils is not
necessary.

[Yusuf, 2015-07-09 21:51:25]

https://codereview.chromium.org/1224553003/diff/120001/chrome/android/java/sr...
File
chrome/android/java/src/org/chromium/chrome/browser/customtabs/CustomTabIntentDataProvider.java
(right):

https://codereview.chromium.org/1224553003/diff/120001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/customtabs/CustomTabIntentDataProvider.java:46:
"android.support.customtabs.CLOSE_BUTTON_STYLE";
android.support.customtabs.extra.CLOSE_BUTTON_STYLE

https://codereview.chromium.org/1224553003/diff/120001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/customtabs/CustomTabIntentDataProvider.java:63:
"android.support.customtabs.TITLE_VISIBILITY";
android.support.customtabs.extra.TITLE_VISIBILITY

https://codereview.chromium.org/1224553003/diff/120001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/customtabs/CustomTabIntentDataProvider.java:103:
mIcon = IntentUtils.safeGetParcelableExtra(intent, CustomTabsIntent.KEY_ICON);
this should be mIcon = IntentUtils.safeGetParcelable(actionButtonBundle,
CustomTabsIntent.KEY_ICON);

the icon is inside the actionButtonBundle, not the intent directly.

https://codereview.chromium.org/1224553003/diff/120001/chrome/android/java/sr...
File
chrome/android/java/src/org/chromium/chrome/browser/customtabs/CustomTabsConnection.java
(right):

https://codereview.chromium.org/1224553003/diff/120001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/customtabs/CustomTabsConnection.java:58:
class CustomTabsConnection extends ICustomTabsService.Stub {
Do we not want to have a CustomTabsService on our end? At the least we should be
updating the intent filter in the manifest and remove the category and start
using the new action.

https://codereview.chromium.org/1224553003/diff/120001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/customtabs/CustomTabsConnection.java:378:
boolean keepAliveForSession(IBinder session, Intent intent) {
for low-end devices only?

https://codereview.chromium.org/1224553003/diff/120001/chrome/android/javates...
File
chrome/android/javatests/src/org/chromium/chrome/browser/customtabs/CustomTabActivityTestBase.java
(left):

https://codereview.chromium.org/1224553003/diff/120001/chrome/android/javates...
chrome/android/javatests/src/org/chromium/chrome/browser/customtabs/CustomTabActivityTestBase.java:83:
intent.putExtra(CustomTabIntentDataProvider.EXTRA_CUSTOM_TABS_SESSION_ID, -1);
should we not still put the extra as null?

[Yusuf, 2015-07-10 00:36:38]

https://codereview.chromium.org/1224553003/diff/120001/chrome/android/java/sr...
File
chrome/android/java/src/org/chromium/chrome/browser/customtabs/CustomTabsConnection.java
(right):

https://codereview.chromium.org/1224553003/diff/120001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/customtabs/CustomTabsConnection.java:337:
callback.onUserNavigationStarted(url, null);
Note that these are back to Uri now :)

[Maria, 2015-07-10 07:19:42]

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

[Maria, 2015-07-10 07:19:43]

https://codereview.chromium.org/1224553003/diff/120001/chrome/android/java/sr...
File chrome/android/java/src/android/support/customtabs/CustomTabsIntent.java
(right):

https://codereview.chromium.org/1224553003/diff/120001/chrome/android/java/sr...
chrome/android/java/src/android/support/customtabs/CustomTabsIntent.java:80: *
@param session     {@link CustomTabsSession} to associate the intent with. If
this is null, the
long line

https://codereview.chromium.org/1224553003/diff/120001/chrome/android/java/sr...
File chrome/android/java/src/android/support/customtabs/CustomTabsService.java
(right):

https://codereview.chromium.org/1224553003/diff/120001/chrome/android/java/sr...
chrome/android/java/src/android/support/customtabs/CustomTabsService.java:69: *
@return      Whether warm up was or had already been completed successfully.
Multiple successful
long line

https://codereview.chromium.org/1224553003/diff/120001/chrome/android/java/sr...
chrome/android/java/src/android/support/customtabs/CustomTabsService.java:103:
public abstract boolean mayLaunchUrl(ICustomTabsCallback callback, String url,
Why are warmup() and mayLaunchUrl() two separate functions? Seems a bit odd as
far as APIs go. What happens if warmup is called without this?

Also what does it mean for the call to be successful?

https://codereview.chromium.org/1224553003/diff/120001/chrome/android/java/sr...
File chrome/android/java/src/android/support/customtabs/CustomTabsSession.java
(right):

https://codereview.chromium.org/1224553003/diff/120001/chrome/android/java/sr...
chrome/android/java/src/android/support/customtabs/CustomTabsSession.java:54: * 
              {@link
ServiceConnection#onServiceConnected(android.content.ComponentName, IBinder)}
long line

[Benoit L, 2015-07-10 14:34:59]

Another day, another patch!

Sorry mariakhomenko, you were not included in the first message I sent... So you
didn't get the message explaining this CL (basically, this is lifted from the
upcoming support library).

https://codereview.chromium.org/1224553003/diff/120001/chrome/android/java/sr...
File chrome/android/java/src/android/support/customtabs/CustomTabsIntent.java
(right):

https://codereview.chromium.org/1224553003/diff/120001/chrome/android/java/sr...
chrome/android/java/src/android/support/customtabs/CustomTabsIntent.java:80: *
@param session     {@link CustomTabsSession} to associate the intent with. If
this is null, the
On 2015/07/10 07:19:42, Maria wrote:
> long line

Acknowledged.

https://codereview.chromium.org/1224553003/diff/120001/chrome/android/java/sr...
chrome/android/java/src/android/support/customtabs/CustomTabsIntent.java:87:
@SuppressLint("NewApi") // This is *never* called in Chrome, only for the
clients.
On 2015/07/09 19:02:36, lizeb wrote:
> Note: I had to add this line to silence lint. We do not call this method in
> Chrome (but still use this class), so using reflection as in IntentUtils is
not
> necessary.

Done.

https://codereview.chromium.org/1224553003/diff/120001/chrome/android/java/sr...
File
chrome/android/java/src/org/chromium/chrome/browser/customtabs/CustomTabIntentDataProvider.java
(right):

https://codereview.chromium.org/1224553003/diff/120001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/customtabs/CustomTabIntentDataProvider.java:46:
"android.support.customtabs.CLOSE_BUTTON_STYLE";
On 2015/07/09 21:51:24, Yusuf wrote:
> android.support.customtabs.extra.CLOSE_BUTTON_STYLE

Done.

https://codereview.chromium.org/1224553003/diff/120001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/customtabs/CustomTabIntentDataProvider.java:63:
"android.support.customtabs.TITLE_VISIBILITY";
On 2015/07/09 21:51:24, Yusuf wrote:
> android.support.customtabs.extra.TITLE_VISIBILITY

Done.

https://codereview.chromium.org/1224553003/diff/120001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/customtabs/CustomTabIntentDataProvider.java:103:
mIcon = IntentUtils.safeGetParcelableExtra(intent, CustomTabsIntent.KEY_ICON);
On 2015/07/09 21:51:24, Yusuf wrote:
> this should be mIcon = IntentUtils.safeGetParcelable(actionButtonBundle,
> CustomTabsIntent.KEY_ICON);
> 
> the icon is inside the actionButtonBundle, not the intent directly.

Done.

https://codereview.chromium.org/1224553003/diff/120001/chrome/android/java/sr...
File
chrome/android/java/src/org/chromium/chrome/browser/customtabs/CustomTabsConnection.java
(right):

https://codereview.chromium.org/1224553003/diff/120001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/customtabs/CustomTabsConnection.java:58:
class CustomTabsConnection extends ICustomTabsService.Stub {
On 2015/07/09 21:51:25, Yusuf wrote:
> Do we not want to have a CustomTabsService on our end? At the least we should
be
> updating the intent filter in the manifest and remove the category and start
> using the new action.

I forgot to include the manifest update in the previous patch set, Done.

For the CustomTabsService, we cannot right now, as discussed offline (one of the
reason is that we want to be able to return null from onBind).

https://codereview.chromium.org/1224553003/diff/120001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/customtabs/CustomTabsConnection.java:337:
callback.onUserNavigationStarted(url, null);
On 2015/07/10 00:36:38, Yusuf wrote:
> Note that these are back to Uri now :)

Done.

[Yusuf, 2015-07-10 16:42:00]

customtabs/ and document/ lgtm with one remaining nit.

https://codereview.chromium.org/1224553003/diff/180001/chrome/android/javates...
File
chrome/android/javatests/src/org/chromium/chrome/browser/customtabs/CustomTabActivityTestBase.java
(left):

https://codereview.chromium.org/1224553003/diff/180001/chrome/android/javates...
chrome/android/javatests/src/org/chromium/chrome/browser/customtabs/CustomTabActivityTestBase.java:83:
intent.putExtra(CustomTabIntentDataProvider.EXTRA_CUSTOM_TABS_SESSION_ID, -1);
Do we not need to add the SESSION EXTRA still?

[Benoit L, 2015-07-10 16:47:07]

https://codereview.chromium.org/1224553003/diff/180001/chrome/android/javates...
File
chrome/android/javatests/src/org/chromium/chrome/browser/customtabs/CustomTabActivityTestBase.java
(left):

https://codereview.chromium.org/1224553003/diff/180001/chrome/android/javates...
chrome/android/javatests/src/org/chromium/chrome/browser/customtabs/CustomTabActivityTestBase.java:83:
intent.putExtra(CustomTabIntentDataProvider.EXTRA_CUSTOM_TABS_SESSION_ID, -1);
On 2015/07/10 16:42:00, Yusuf wrote:
> Do we not need to add the SESSION EXTRA still?

Sorry, missed that one.
No, it is allowed to have an intent without a session, and it should stay this
way.

[Benoit L, 2015-07-10 17:19:22]

https://codereview.chromium.org/1224553003/diff/180001/chrome/android/javates...
File
chrome/android/javatests/src/org/chromium/chrome/browser/customtabs/CustomTabActivityTestBase.java
(left):

https://codereview.chromium.org/1224553003/diff/180001/chrome/android/javates...
chrome/android/javatests/src/org/chromium/chrome/browser/customtabs/CustomTabActivityTestBase.java:83:
intent.putExtra(CustomTabIntentDataProvider.EXTRA_CUSTOM_TABS_SESSION_ID, -1);
On 2015/07/10 16:47:06, lizeb wrote:
> On 2015/07/10 16:42:00, Yusuf wrote:
> > Do we not need to add the SESSION EXTRA still?
> 
> Sorry, missed that one.
> No, it is allowed to have an intent without a session, and it should stay this
> way.

Done.

[pasko, 2015-07-10 17:43:41]

lgtm

[Maria, 2015-07-10 18:22:12]

lgtm

https://codereview.chromium.org/1224553003/diff/200001/chrome/android/java/sr...
File
chrome/android/java/src/org/chromium/chrome/browser/customtabs/CustomTabsConnection.java
(right):

https://codereview.chromium.org/1224553003/diff/200001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/customtabs/CustomTabsConnection.java:151:
if (callback == null || mSessionParams.containsKey(callback)) return false;
doesn't this access to mSessionParams need to be lock guarded?

[Yusuf, 2015-07-10 18:26:10]

The CQ bit was checked by yusufo@chromium.org

[Yusuf, 2015-07-10 18:26:10]

The patchset sent to the CQ was uploaded after l-g-t-m from yusufo@chromium.org
Link to the patchset: https://codereview.chromium.org/1224553003/#ps200001
(title: "Add safePutBinder and a session extra in the testing activity.")

[commit-bot: I haz the power, 2015-07-10 18:28:29]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1224553003/200001

[commit-bot: I haz the power, 2015-07-10 18:50:30]

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

[commit-bot: I haz the power, 2015-07-10 18:50:31]

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

(exceeded global retry quota)

[Benoit L, 2015-07-15 08:01:56]

On 2015/07/10 18:50:31, commit-bot: I haz the power wrote:
> Try jobs failed on following builders:
>   android_chromium_gn_compile_dbg on tryserver.chromium.linux (JOB_FAILED,
>
http://build.chromium.org/p/tryserver.chromium.linux/builders/android_chromiu...)
> 
> (exceeded global retry quota)

Re-uploaded and committed by yusufo@ as crrev.com/1234643002.
Closing this issue.