Start using the third_party/custom_tabs_client as support lib source

Start using the third_party/custom_tabs_client as support lib source

This removes the half baked copies for constants and aidls in chromium
and starts using proper Custom Tabs classes through the third party
repo for all calls.

BUG=522099
Committed: https://crrev.com/9993a5742289cf9d88953013b0a0c7883ea0db43
Cr-Commit-Position: refs/heads/master@{#396515}

[Yusuf, 2016-05-06 23:41:51]

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

[Yusuf, 2016-05-06 23:41:52]

I have very mixed feeling about landing this....

[Yusuf, 2016-05-06 23:43:33]

The CQ bit was checked by yusufo@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-05-06 23:44:02]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1954363002/1
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1954363002/1

[commit-bot: I haz the power, 2016-05-06 23:59:26]

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

[commit-bot: I haz the power, 2016-05-06 23:59:26]

Dry run: Try jobs failed on following builders:
  android_arm64_dbg_recipe on tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_arm6...)

[pasko, 2016-05-09 10:07:00]

Oh, this is great!!

would you like to fix the compilation first?

[Ian Wen, 2016-05-09 21:17:49]

Thanks for preparing this awesome change!

https://chromiumcodereview.appspot.com/1954363002/diff/1/chrome/android/java/...
File
chrome/android/java/src/org/chromium/chrome/browser/customtabs/ClientManager.java
(left):

https://chromiumcodereview.appspot.com/1954363002/diff/1/chrome/android/java/...
chrome/android/java/src/org/chromium/chrome/browser/customtabs/ClientManager.java:131:
onDisconnect.run(session);
Is it true that we no longer care about the case where process dies and
connection needs to be cleaned up? I don't see a counterpart in
CustomTabsSessionToken that represents this logic.

https://chromiumcodereview.appspot.com/1954363002/diff/1/chrome/android/java/...
File
chrome/android/java/src/org/chromium/chrome/browser/customtabs/CustomTabsConnectionService.java
(right):

https://chromiumcodereview.appspot.com/1954363002/diff/1/chrome/android/java/...
chrome/android/java/src/org/chromium/chrome/browser/customtabs/CustomTabsConnectionService.java:56:
return CustomTabsConnection.getInstance(getApplication()).mayLaunchUrl(
nit: maybe we can make the code shorter, by replacing
CustomTabsConnection.getInstance() with mConnection?

[Yusuf, 2016-05-09 21:28:02]

The CQ bit was checked by yusufo@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-05-09 21:28:25]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1954363002/20001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1954363002/20001

[Yusuf, 2016-05-09 21:28:53]

https://chromiumcodereview.appspot.com/1954363002/diff/1/chrome/android/java/...
File
chrome/android/java/src/org/chromium/chrome/browser/customtabs/ClientManager.java
(left):

https://chromiumcodereview.appspot.com/1954363002/diff/1/chrome/android/java/...
chrome/android/java/src/org/chromium/chrome/browser/customtabs/ClientManager.java:131:
onDisconnect.run(session);
On 2016/05/09 21:17:48, Ian Wen wrote:
> Is it true that we no longer care about the case where process dies and
> connection needs to be cleaned up? I don't see a counterpart in
> CustomTabsSessionToken that represents this logic.

See CustomTabsService for the logic that tracks that end does the clean up.

[commit-bot: I haz the power, 2016-05-09 23:39:38]

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

[commit-bot: I haz the power, 2016-05-09 23:39:39]

Dry run: Try jobs failed on following builders:
  linux_android_rel_ng on tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)

[Yusuf, 2016-05-11 20:52:05]

On 2016/05/09 10:07:00, pasko wrote:
> Oh, this is great!!
> 
> would you like to fix the compilation first?

Compile is fixed, I am trying to convince bots to recognize the added classes
for testing now.

Umm, other than the build files, nothing else will change to get that working so
feel free to
review now!

[pasko, 2016-05-12 12:08:23]

https://codereview.chromium.org/1954363002/diff/60001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/customtabs/ClientManager.java
(left):

https://codereview.chromium.org/1954363002/diff/60001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/customtabs/ClientManager.java:130:
cleanupSession(session);
with your change are we no longer doing cleanupSession() on connection death or
we are doing cleanup more than once unnecessarily at the moment?

https://codereview.chromium.org/1954363002/diff/60001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/sync/ui/PassphraseDialogFragment.java
(right):

https://codereview.chromium.org/1954363002/diff/60001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/sync/ui/PassphraseDialogFragment.java:183:
Intent intent = new Intent(Intent.ACTION_VIEW, syncDashboardUrl);
I cannot understand why this change is related to making it use
third_party/custom_tabs_client. Can you explain?

https://codereview.chromium.org/1954363002/diff/60001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/sync/ui/PassphraseDialogFragment.java:192:
public static Intent getViewIntentWithNoSession(String packageName, Uri data) {
shall we use this function in getResetText()? Otherwise unused ..

[Yusuf, 2016-05-12 17:49:58]

https://codereview.chromium.org/1954363002/diff/60001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/sync/ui/PassphraseDialogFragment.java
(right):

https://codereview.chromium.org/1954363002/diff/60001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/sync/ui/PassphraseDialogFragment.java:183:
Intent intent = new Intent(Intent.ACTION_VIEW, syncDashboardUrl);
On 2016/05/12 12:08:23, pasko wrote:
> I cannot understand why this change is related to making it use
> third_party/custom_tabs_client. Can you explain?

CustomTabsIntent.getViewIntentWithNoSession() was something we added to only the
chromium side copy of CustomTabsIntent. We don't have it in support lib (and I
think we would prefer to not have it, since for apps that are not Chrome, this
means the intent launches without warmup). So that function got deleted, so I
had to do the same thing through support lib calls.

https://codereview.chromium.org/1954363002/diff/60001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/sync/ui/PassphraseDialogFragment.java:192:
public static Intent getViewIntentWithNoSession(String packageName, Uri data) {
On 2016/05/12 12:08:23, pasko wrote:
> shall we use this function in getResetText()? Otherwise unused ..

Oops that is leftover from my attempts to handle this well. It just ended up
being less code, so I decided we don't need a separate call. Will take it out.

[Benoit L, 2016-05-13 15:51:31]

Codereview hates me today (can't publish the comment).

In ClientManager, we no longer call onDisconnect, or do we?

It's used to cancel a pending prerender.

On Thu, May 12, 2016 at 7:49 PM, <yusufo@chromium.org> wrote:

>
>
>
https://codereview.chromium.org/1954363002/diff/60001/chrome/android/java/src...
> File
>
>
chrome/android/java/src/org/chromium/chrome/browser/sync/ui/PassphraseDialogFragment.java
> (right):
>
>
>
https://codereview.chromium.org/1954363002/diff/60001/chrome/android/java/src...
>
>
chrome/android/java/src/org/chromium/chrome/browser/sync/ui/PassphraseDialogFragment.java:183:
> Intent intent = new Intent(Intent.ACTION_VIEW, syncDashboardUrl);
> On 2016/05/12 12:08:23, pasko wrote:
> > I cannot understand why this change is related to making it use
> > third_party/custom_tabs_client. Can you explain?
>
> CustomTabsIntent.getViewIntentWithNoSession() was something we added to
> only the chromium side copy of CustomTabsIntent. We don't have it in
> support lib (and I think we would prefer to not have it, since for apps
> that are not Chrome, this means the intent launches without warmup). So
> that function got deleted, so I had to do the same thing through support
> lib calls.
>
>
>
https://codereview.chromium.org/1954363002/diff/60001/chrome/android/java/src...
>
>
chrome/android/java/src/org/chromium/chrome/browser/sync/ui/PassphraseDialogFragment.java:192:
> public static Intent getViewIntentWithNoSession(String packageName, Uri
> data) {
> On 2016/05/12 12:08:23, pasko wrote:
> > shall we use this function in getResetText()? Otherwise unused ..
>
> Oops that is leftover from my attempts to handle this well. It just
> ended up being less code, so I decided we don't need a separate call.
> Will take it out.
>
> https://codereview.chromium.org/1954363002/
>

-- 
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[Yusuf, 2016-05-17 05:09:19]

On 2016/05/13 15:51:31, Benoit L wrote:
> Codereview hates me today (can't publish the comment).
> 
> In ClientManager, we no longer call onDisconnect, or do we?
> 
> It's used to cancel a pending prerender.
> 
> On Thu, May 12, 2016 at 7:49 PM, <mailto:yusufo@chromium.org> wrote:
> 
> >
> >
> >
>
https://codereview.chromium.org/1954363002/diff/60001/chrome/android/java/src...
> > File
> >
> >
>
chrome/android/java/src/org/chromium/chrome/browser/sync/ui/PassphraseDialogFragment.java
> > (right):
> >
> >
> >
>
https://codereview.chromium.org/1954363002/diff/60001/chrome/android/java/src...
> >
> >
>
chrome/android/java/src/org/chromium/chrome/browser/sync/ui/PassphraseDialogFragment.java:183:
> > Intent intent = new Intent(Intent.ACTION_VIEW, syncDashboardUrl);
> > On 2016/05/12 12:08:23, pasko wrote:
> > > I cannot understand why this change is related to making it use
> > > third_party/custom_tabs_client. Can you explain?
> >
> > CustomTabsIntent.getViewIntentWithNoSession() was something we added to
> > only the chromium side copy of CustomTabsIntent. We don't have it in
> > support lib (and I think we would prefer to not have it, since for apps
> > that are not Chrome, this means the intent launches without warmup). So
> > that function got deleted, so I had to do the same thing through support
> > lib calls.
> >
> >
> >
>
https://codereview.chromium.org/1954363002/diff/60001/chrome/android/java/src...
> >
> >
>
chrome/android/java/src/org/chromium/chrome/browser/sync/ui/PassphraseDialogFragment.java:192:
> > public static Intent getViewIntentWithNoSession(String packageName, Uri
> > data) {
> > On 2016/05/12 12:08:23, pasko wrote:
> > > shall we use this function in getResetText()? Otherwise unused ..
> >
> > Oops that is leftover from my attempts to handle this well. It just
> > ended up being less code, so I decided we don't need a separate call.
> > Will take it out.
> >
> > https://codereview.chromium.org/1954363002/
> >
> 
> -- 
> You received this message because you are subscribed to the Google Groups
> "Chromium-reviews" group.
> To unsubscribe from this group and stop receiving emails from it, send an
email
> to mailto:chromium-reviews+unsubscribe@chromium.org.

All comments addressed. Now I have 3 tests to fix.

[Yusuf, 2016-05-18 21:41:21]

The CQ bit was checked by yusufo@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-05-18 21:42:05]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1954363002/140001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1954363002/140001

[commit-bot: I haz the power, 2016-05-18 22:08:13]

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

[commit-bot: I haz the power, 2016-05-18 22:08:14]

Dry run: Try jobs failed on following builders:
  android_compile_dbg on tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_comp...)
  mac_chromium_gn_rel on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_gn_r...)

[Yusuf, 2016-05-18 22:10:49]

The CQ bit was checked by yusufo@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-05-18 22:11:39]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1954363002/160001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1954363002/160001

[commit-bot: I haz the power, 2016-05-18 23:11:16]

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

[commit-bot: I haz the power, 2016-05-18 23:11:17]

Dry run: Try jobs failed on following builders:
  android_clang_dbg_recipe on tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_clan...)

[Yusuf, 2016-05-19 17:29:49]

The CQ bit was checked by yusufo@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-05-19 17:30:29]

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

[commit-bot: I haz the power, 2016-05-19 19:38:32]

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

[commit-bot: I haz the power, 2016-05-19 19:38:33]

Dry run: Try jobs failed on following builders:
  linux_android_rel_ng on tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)

[Yusuf, 2016-05-24 22:10:02]

Passing on all tests finally!

This is good to go now. So PTAL.

[Yusuf, 2016-05-24 22:10:08]

The CQ bit was checked by yusufo@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-05-24 22:10:47]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1954363002/200001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1954363002/200001

[commit-bot: I haz the power, 2016-05-24 23:32:51]

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

[commit-bot: I haz the power, 2016-05-24 23:32:52]

Dry run: This issue passed the CQ dry run.

[commit-bot: I haz the power, 2016-05-24 23:33:06]

Dry run: None

[pasko, 2016-05-25 10:12:39]

lgtm, thank you, Yusuf!

Not suggesting now, rather a question: is it technically possible to separate
the IBinder->CustomTabsSessionToken transition and the massive deletion in two
changes? Does not seem possible, but I may be missing something. Separation
would have had benefits, and I would suggest to do it in case this change is
reverted :)

[Ian Wen, 2016-05-25 18:09:59]

Thank you Yusuf! I see no problem with this CL. Only one optional nit.

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

https://codereview.chromium.org/1954363002/diff/200001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/customtabs/ClientManager.java:97:
public interface DisconnectCallback { public void run(CustomTabsSessionToken
session); }
Nit: Usually a pubic interface should be exposed at the top of the file.

[Ian Wen, 2016-05-25 18:10:22]

lgtm

[Benoit L, 2016-05-26 15:42:00]

lgtm with a few small comments.

Thanks for doing that! No code is best code.

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

https://codereview.chromium.org/1954363002/diff/200001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/customtabs/ClientManager.java:117:
CustomTabsSessionToken session, int uid, final DisconnectCallback onDisconnect)
{
nit: unnecessary final.

https://codereview.chromium.org/1954363002/diff/200001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/customtabs/ClientManager.java:354:
params.disconnectCallback.run(session);
This assumes that disconnectCallback() is never null, which is the case in the
code, but not in the tests it seems.
Worth adding a null check?

https://codereview.chromium.org/1954363002/diff/200001/chrome/android/javates...
File
chrome/android/javatests/src/org/chromium/chrome/browser/customtabs/CustomTabActivityTest.java
(right):

https://codereview.chromium.org/1954363002/diff/200001/chrome/android/javates...
chrome/android/javatests/src/org/chromium/chrome/browser/customtabs/CustomTabActivityTest.java:792:
final AtomicReference<Intent> intent = new AtomicReference<Intent>(null);
volatile is equivalent here, and less scary-looking to me.

[Yusuf, 2016-05-26 19:30:33]

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

https://codereview.chromium.org/1954363002/diff/200001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/customtabs/ClientManager.java:97:
public interface DisconnectCallback { public void run(CustomTabsSessionToken
session); }
On 2016/05/25 18:09:59, Ian Wen wrote:
> Nit: Usually a pubic interface should be exposed at the top of the file.

Done.

https://codereview.chromium.org/1954363002/diff/200001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/customtabs/ClientManager.java:117:
CustomTabsSessionToken session, int uid, final DisconnectCallback onDisconnect)
{
On 2016/05/26 15:42:00, Benoit L wrote:
> nit: unnecessary final.

Done.

https://codereview.chromium.org/1954363002/diff/200001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/customtabs/ClientManager.java:354:
params.disconnectCallback.run(session);
On 2016/05/26 15:42:00, Benoit L wrote:
> This assumes that disconnectCallback() is never null, which is the case in the
> code, but not in the tests it seems.
> Worth adding a null check?

Done.

[Yusuf, 2016-05-26 19:30:42]

The CQ bit was checked by yusufo@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-05-26 19:31:42]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1954363002/220001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1954363002/220001

[commit-bot: I haz the power, 2016-05-26 21:03:27]

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

[commit-bot: I haz the power, 2016-05-26 21:03:28]

Dry run: This issue passed the CQ dry run.

[commit-bot: I haz the power, 2016-05-26 21:03:43]

Dry run: None

[Yusuf, 2016-05-27 17:37:47]

The CQ bit was checked by yusufo@chromium.org

[Yusuf, 2016-05-27 17:37:48]

The patchset sent to the CQ was uploaded after l-g-t-m from pasko@chromium.org,
ianwen@chromium.org, lizeb@chromium.org
Link to the patchset: https://codereview.chromium.org/1954363002/#ps220001
(title: "Nits")

[commit-bot: I haz the power, 2016-05-27 17:38:20]

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

[commit-bot: I haz the power, 2016-05-27 17:46:25]

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

[commit-bot: I haz the power, 2016-05-27 17:46:27]

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

[Yusuf, 2016-05-27 17:52:08]

dfalcantara@ for chrome/android/java

specifically changes related with :

-deleting chrome/android/java/src/android/support/customtabs/*
-replacing a call in
chrome/android/java/src/org/chromium/chrome/browser/sync/ui/

[Yusuf, 2016-05-27 17:52:22]

yusufo@chromium.org changed reviewers:
+ dfalcantara@chromium.org

[gone, 2016-05-27 18:10:38]

lgtm for the things you asked me to look over.

[Yusuf, 2016-05-27 18:11:31]

The CQ bit was checked by yusufo@chromium.org

[commit-bot: I haz the power, 2016-05-27 18:11:52]

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

[commit-bot: I haz the power, 2016-05-27 18:17:48]

Committed patchset #12 (id:220001)

[commit-bot: I haz the power, 2016-05-27 18:21:04]

Description was changed from

==========
Start using the third_party/custom_tabs_client as support lib source

This removes the half baked copies for constants and aidls in chromium
and starts using proper Custom Tabs classes through the third party
repo for all calls.

BUG=522099
==========

to

==========
Start using the third_party/custom_tabs_client as support lib source

This removes the half baked copies for constants and aidls in chromium
and starts using proper Custom Tabs classes through the third party
repo for all calls.

BUG=522099

Committed: https://crrev.com/9993a5742289cf9d88953013b0a0c7883ea0db43
Cr-Commit-Position: refs/heads/master@{#396515}
==========

[commit-bot: I haz the power, 2016-05-27 18:21:06]

Patchset 12 (id:??) landed as
https://crrev.com/9993a5742289cf9d88953013b0a0c7883ea0db43
Cr-Commit-Position: refs/heads/master@{#396515}