Support library changes to allow a callback notifying the selection of default items for Browser Ac…

Support library changes to allow a callback notifying the selection of default items for Browser Actions

Changes in support library to add a PendingIntent in BrowserActionsIntent
as the callback which will be launched if a default menu item is selected.
The PendingIntent will includes the id of the chosen default menu item.

This CL also adds a BroadcastReceiver in the CCT test app to handle the
callback PendingIntent and displays the id of the chosen item.

BUG=737814
Review-Url: https://codereview.chromium.org/2961203002
Committed: https://github.com/GoogleChrome/custom-tabs-client/commit/481ca67f75f35a6d0c6fe9778b1b69c21d6d9203

[ltian, 2017-06-29 00:52:43]

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

[ltian, 2017-06-29 00:52:44]

Can you take a look of this CL?

Thanks!

[Yusuf, 2017-06-30 17:59:49]

https://codereview.chromium.org/2961203002/diff/60001/customtabs/src/android/...
File
customtabs/src/android/support/customtabs/browseractions/BrowserActionsIntent.java
(right):

https://codereview.chromium.org/2961203002/diff/60001/customtabs/src/android/...
customtabs/src/android/support/customtabs/browseractions/BrowserActionsIntent.java:93:
public static final String EXTRA_DEFULT_ACTION =
EXTRA_SELECTED_ACTION_PENDING_INTENT

https://codereview.chromium.org/2961203002/diff/60001/customtabs/src/android/...
customtabs/src/android/support/customtabs/browseractions/BrowserActionsIntent.java:198:
* Set the PendingIntent to be launched when a default menu item is selected.
when a browser specified menu item is selected.

https://codereview.chromium.org/2961203002/diff/60001/customtabs/src/android/...
customtabs/src/android/support/customtabs/browseractions/BrowserActionsIntent.java:341:
if (icon != null) {
one line

https://codereview.chromium.org/2961203002/diff/60001/customtabs/src/android/...
customtabs/src/android/support/customtabs/browseractions/BrowserActionsIntent.java:346:
Log.e(TAG, "Missing action for item: " + i);
I dont think we allow this no? These should be asserts at the beginning

[ltian, 2017-06-30 22:14:33]

https://codereview.chromium.org/2961203002/diff/60001/customtabs/src/android/...
File
customtabs/src/android/support/customtabs/browseractions/BrowserActionsIntent.java
(right):

https://codereview.chromium.org/2961203002/diff/60001/customtabs/src/android/...
customtabs/src/android/support/customtabs/browseractions/BrowserActionsIntent.java:93:
public static final String EXTRA_DEFULT_ACTION =
On 2017/06/30 17:59:49, Yusuf wrote:
> EXTRA_SELECTED_ACTION_PENDING_INTENT

Done.

https://codereview.chromium.org/2961203002/diff/60001/customtabs/src/android/...
customtabs/src/android/support/customtabs/browseractions/BrowserActionsIntent.java:198:
* Set the PendingIntent to be launched when a default menu item is selected.
On 2017/06/30 17:59:49, Yusuf wrote:
> when a browser specified menu item is selected.

Done.

https://codereview.chromium.org/2961203002/diff/60001/customtabs/src/android/...
customtabs/src/android/support/customtabs/browseractions/BrowserActionsIntent.java:341:
if (icon != null) {
On 2017/06/30 17:59:49, Yusuf wrote:
> one line

Done.

https://codereview.chromium.org/2961203002/diff/60001/customtabs/src/android/...
customtabs/src/android/support/customtabs/browseractions/BrowserActionsIntent.java:346:
Log.e(TAG, "Missing action for item: " + i);
On 2017/06/30 17:59:49, Yusuf wrote:
> I dont think we allow this no? These should be asserts at the beginning

I will add the assert in the setter in the Builder class. But since we also
expose the Intent to let 1P apps add extras directly on it, I am concerned that
they might skip calling the setter, so I feel it is safer we keep these checks
to give more information?

[Yusuf, 2017-07-07 21:36:35]

https://codereview.chromium.org/2961203002/diff/60001/customtabs/src/android/...
File
customtabs/src/android/support/customtabs/browseractions/BrowserActionsIntent.java
(right):

https://codereview.chromium.org/2961203002/diff/60001/customtabs/src/android/...
customtabs/src/android/support/customtabs/browseractions/BrowserActionsIntent.java:346:
Log.e(TAG, "Missing action for item: " + i);
On 2017/06/30 22:14:32, ltian wrote:
> On 2017/06/30 17:59:49, Yusuf wrote:
> > I dont think we allow this no? These should be asserts at the beginning
> 
> I will add the assert in the setter in the Builder class. But since we also
> expose the Intent to let 1P apps add extras directly on it, I am concerned
that
> they might skip calling the setter, so I feel it is safer we keep these checks
> to give more information?

We should still enforce the right way to use these APIs. See
IllegalArgumentException thrown in CustomTabIntent.

this ifs should be at the very beginning and if any of them are correct, we
should throw an IllegalStateException and say both action and title are NonNull.
Feel free to add that to the javadoc as well.

https://codereview.chromium.org/2961203002/diff/80001/Application/src/main/ja...
File
Application/src/main/java/org/chromium/customtabsclient/BrowserActionsReceiver.java
(right):

https://codereview.chromium.org/2961203002/diff/80001/Application/src/main/ja...
Application/src/main/java/org/chromium/customtabsclient/BrowserActionsReceiver.java:1:
package org.chromium.customtabsclient;
copyright missing

https://codereview.chromium.org/2961203002/diff/80001/customtabs/src/android/...
File
customtabs/src/android/support/customtabs/browseractions/BrowserActionsIntent.java
(right):

https://codereview.chromium.org/2961203002/diff/80001/customtabs/src/android/...
customtabs/src/android/support/customtabs/browseractions/BrowserActionsIntent.java:94:
"android.support.customtabs.browseractions.extra.DEFAULT_ACTION";
pls change the wording in the actual string also. DEFAULT->Selected

https://codereview.chromium.org/2961203002/diff/80001/customtabs/src/android/...
customtabs/src/android/support/customtabs/browseractions/BrowserActionsIntent.java:115:
/**
TODO() to find a long term solution for this, since we can't really use this in
the final API.

https://codereview.chromium.org/2961203002/diff/80001/customtabs/src/android/...
customtabs/src/android/support/customtabs/browseractions/BrowserActionsIntent.java:156:
private PendingIntent mPendingIntent = null;
mOnItemSelectedPendingIntent

https://codereview.chromium.org/2961203002/diff/80001/customtabs/src/android/...
customtabs/src/android/support/customtabs/browseractions/BrowserActionsIntent.java:203:
public Builder setDefaultActions(PendingIntent pendingIntent) {
default->SelectedAction

DefaultAction really gets confusing with BrowserActions and it kinda reads like
this is the action the browser takes if you dont add any custom ones.

https://codereview.chromium.org/2961203002/diff/80001/customtabs/src/android/...
customtabs/src/android/support/customtabs/browseractions/BrowserActionsIntent.java:243:
public static void openBrowserAction(Context context, Uri uri) {
I would say openBrowserAction(*) types here need some cleanup overall. There is
too much code duplication going around. There should be a way to call one from
the other and avoid this much repetition.


Also maybe we don't need one/some of these? The one that launches with
BrowserActionIntent seems like overdoing convenience calls a bit? If they are
creating that they can just launch it as well, right?

https://codereview.chromium.org/2961203002/diff/80001/customtabs/src/android/...
customtabs/src/android/support/customtabs/browseractions/BrowserActionsIntent.java:245:
if (handlers != null && !handlers.isEmpty()) {
maybe launchIntent has some of this logic about fallback etc?

https://codereview.chromium.org/2961203002/diff/80001/customtabs/src/android/...
customtabs/src/android/support/customtabs/browseractions/BrowserActionsIntent.java:322:
ResolveInfo defaultHandler = context.getPackageManager().resolveActivity(
maybe we can have a shared, getDefaultHandlerForAction in
android.support.customtabs?

[ltian, 2017-07-10 18:45:56]

https://codereview.chromium.org/2961203002/diff/80001/Application/src/main/ja...
File
Application/src/main/java/org/chromium/customtabsclient/BrowserActionsReceiver.java
(right):

https://codereview.chromium.org/2961203002/diff/80001/Application/src/main/ja...
Application/src/main/java/org/chromium/customtabsclient/BrowserActionsReceiver.java:1:
package org.chromium.customtabsclient;
On 2017/07/07 21:36:35, Yusuf wrote:
> copyright missing

Done.

https://codereview.chromium.org/2961203002/diff/80001/customtabs/src/android/...
File
customtabs/src/android/support/customtabs/browseractions/BrowserActionsIntent.java
(right):

https://codereview.chromium.org/2961203002/diff/80001/customtabs/src/android/...
customtabs/src/android/support/customtabs/browseractions/BrowserActionsIntent.java:94:
"android.support.customtabs.browseractions.extra.DEFAULT_ACTION";
On 2017/07/07 21:36:35, Yusuf wrote:
> pls change the wording in the actual string also. DEFAULT->Selected

Oh, sorry I forgot changing it!

https://codereview.chromium.org/2961203002/diff/80001/customtabs/src/android/...
customtabs/src/android/support/customtabs/browseractions/BrowserActionsIntent.java:115:
/**
On 2017/07/07 21:36:35, Yusuf wrote:
> TODO() to find a long term solution for this, since we can't really use this
in
> the final API.

Done.

https://codereview.chromium.org/2961203002/diff/80001/customtabs/src/android/...
customtabs/src/android/support/customtabs/browseractions/BrowserActionsIntent.java:156:
private PendingIntent mPendingIntent = null;
On 2017/07/07 21:36:35, Yusuf wrote:
> mOnItemSelectedPendingIntent

Done.

https://codereview.chromium.org/2961203002/diff/80001/customtabs/src/android/...
customtabs/src/android/support/customtabs/browseractions/BrowserActionsIntent.java:203:
public Builder setDefaultActions(PendingIntent pendingIntent) {
On 2017/07/07 21:36:35, Yusuf wrote:
> default->SelectedAction
> 
> DefaultAction really gets confusing with BrowserActions and it kinda reads
like
> this is the action the browser takes if you dont add any custom ones.

Done.

https://codereview.chromium.org/2961203002/diff/80001/customtabs/src/android/...
customtabs/src/android/support/customtabs/browseractions/BrowserActionsIntent.java:243:
public static void openBrowserAction(Context context, Uri uri) {
On 2017/07/07 21:36:35, Yusuf wrote:
> I would say openBrowserAction(*) types here need some cleanup overall. There
is
> too much code duplication going around. There should be a way to call one from
> the other and avoid this much repetition.
> 
> 
> Also maybe we don't need one/some of these? The one that launches with
> BrowserActionIntent seems like overdoing convenience calls a bit? If they are
> creating that they can just launch it as well, right?

The reason I have launches with BrowserActionIntent is the logics about deciding
launcher and opening in fallback menu. But if moving the those logics into
launchIntent I think both openBrowserAction with Intent and BrowserActionIntent
are not necessary. They can be directly called through launchIntent.

https://codereview.chromium.org/2961203002/diff/80001/customtabs/src/android/...
customtabs/src/android/support/customtabs/browseractions/BrowserActionsIntent.java:245:
if (handlers != null && !handlers.isEmpty()) {
On 2017/07/07 21:36:35, Yusuf wrote:
> maybe launchIntent has some of this logic about fallback etc?

Done.

https://codereview.chromium.org/2961203002/diff/80001/customtabs/src/android/...
customtabs/src/android/support/customtabs/browseractions/BrowserActionsIntent.java:322:
ResolveInfo defaultHandler = context.getPackageManager().resolveActivity(
On 2017/07/07 21:36:35, Yusuf wrote:
> maybe we can have a shared, getDefaultHandlerForAction in
> android.support.customtabs?

Done.

[Yusuf, 2017-07-14 05:35:35]

one last round and I think we will be OK.

https://codereview.chromium.org/2961203002/diff/100001/customtabs/src/android...
File customtabs/src/android/support/customtabs/CustomTabsClient.java (right):

https://codereview.chromium.org/2961203002/diff/100001/customtabs/src/android...
customtabs/src/android/support/customtabs/CustomTabsClient.java:257: public
static ResolveInfo getDefaultHandlerForIntent(PackageManager pm, Intent intent)
{
I know I said to do this, but now that we have it here, it seems a bit
superflous. Sorry for the back and forth but I think it is better to not add a
change in CustomTabsClient for this.

https://codereview.chromium.org/2961203002/diff/100001/customtabs/src/android...
File
customtabs/src/android/support/customtabs/browseractions/BrowserActionsIntent.java
(right):

https://codereview.chromium.org/2961203002/diff/100001/customtabs/src/android...
customtabs/src/android/support/customtabs/browseractions/BrowserActionsIntent.java:194:
assert items.get(i).getTitle() != null;
assert would not help with production builds so throw IllegalArgumentException
here. See examples in CustomTabsIntent.Builder

https://codereview.chromium.org/2961203002/diff/100001/customtabs/src/android...
customtabs/src/android/support/customtabs/browseractions/BrowserActionsIntent.java:205:
public Builder setSelectedAction(PendingIntent onItemSelectedPendingIntent) {
setOnItemSelectedAction

https://codereview.chromium.org/2961203002/diff/100001/customtabs/src/android...
customtabs/src/android/support/customtabs/browseractions/BrowserActionsIntent.java:286:
ResolveInfo defaultHandler =
CustomTabsClient.getDefaultHandlerForIntent(context.getPackageManager(),
viewIntent);
indentation wrong here?

https://codereview.chromium.org/2961203002/diff/100001/customtabs/src/android...
customtabs/src/android/support/customtabs/browseractions/BrowserActionsIntent.java:357:
Log.e(TAG, "Missing action for item: " + i);
These are still here?

[ltian, 2017-07-14 22:15:03]

https://codereview.chromium.org/2961203002/diff/100001/customtabs/src/android...
File customtabs/src/android/support/customtabs/CustomTabsClient.java (right):

https://codereview.chromium.org/2961203002/diff/100001/customtabs/src/android...
customtabs/src/android/support/customtabs/CustomTabsClient.java:257: public
static ResolveInfo getDefaultHandlerForIntent(PackageManager pm, Intent intent)
{
On 2017/07/14 05:35:34, Yusuf wrote:
> I know I said to do this, but now that we have it here, it seems a bit
> superflous. Sorry for the back and forth but I think it is better to not add a
> change in CustomTabsClient for this.

Done.

https://codereview.chromium.org/2961203002/diff/100001/customtabs/src/android...
File
customtabs/src/android/support/customtabs/browseractions/BrowserActionsIntent.java
(right):

https://codereview.chromium.org/2961203002/diff/100001/customtabs/src/android...
customtabs/src/android/support/customtabs/browseractions/BrowserActionsIntent.java:194:
assert items.get(i).getTitle() != null;
On 2017/07/14 05:35:34, Yusuf wrote:
> assert would not help with production builds so throw IllegalArgumentException
> here. See examples in CustomTabsIntent.Builder

Done.

https://codereview.chromium.org/2961203002/diff/100001/customtabs/src/android...
customtabs/src/android/support/customtabs/browseractions/BrowserActionsIntent.java:205:
public Builder setSelectedAction(PendingIntent onItemSelectedPendingIntent) {
On 2017/07/14 05:35:35, Yusuf wrote:
> setOnItemSelectedAction

Done.

https://codereview.chromium.org/2961203002/diff/100001/customtabs/src/android...
customtabs/src/android/support/customtabs/browseractions/BrowserActionsIntent.java:286:
ResolveInfo defaultHandler =
CustomTabsClient.getDefaultHandlerForIntent(context.getPackageManager(),
viewIntent);
On 2017/07/14 05:35:34, Yusuf wrote:
> indentation wrong here?

After revert it back, there should be no problem.

https://codereview.chromium.org/2961203002/diff/100001/customtabs/src/android...
customtabs/src/android/support/customtabs/browseractions/BrowserActionsIntent.java:357:
Log.e(TAG, "Missing action for item: " + i);
On 2017/07/14 05:35:34, Yusuf wrote:
> These are still here?

The reason I still leave these checks here is because if the Intent is not
constructed by the Builder, it is possible that title or action are null, these
checks help Chrome side to detect the problem.

[Yusuf, 2017-07-17 17:08:53]

lgtm with two comments

https://codereview.chromium.org/2961203002/diff/100001/customtabs/src/android...
File
customtabs/src/android/support/customtabs/browseractions/BrowserActionsIntent.java
(right):

https://codereview.chromium.org/2961203002/diff/100001/customtabs/src/android...
customtabs/src/android/support/customtabs/browseractions/BrowserActionsIntent.java:357:
Log.e(TAG, "Missing action for item: " + i);
On 2017/07/14 22:15:02, ltian wrote:
> On 2017/07/14 05:35:34, Yusuf wrote:
> > These are still here?
> 
> The reason I still leave these checks here is because if the Intent is not
> constructed by the Builder, it is possible that title or action are null,
these
> checks help Chrome side to detect the problem.

Then again, we should be throwing IllegalArgumentExceptions here and not letting
these through. Note that this way it will crash on the client app, so that they
will know something is wrong.

https://codereview.chromium.org/2961203002/diff/120001/customtabs/src/android...
File
customtabs/src/android/support/customtabs/browseractions/BrowserActionsIntent.java
(right):

https://codereview.chromium.org/2961203002/diff/120001/customtabs/src/android...
customtabs/src/android/support/customtabs/browseractions/BrowserActionsIntent.java:291:
ResolveInfo defaultHandler = pm.resolveActivity(viewIntent,
PackageManager.MATCH_DEFAULT_ONLY);
>100 lines?

[ltian, 2017-07-17 17:43:37]

https://codereview.chromium.org/2961203002/diff/120001/customtabs/src/android...
File
customtabs/src/android/support/customtabs/browseractions/BrowserActionsIntent.java
(right):

https://codereview.chromium.org/2961203002/diff/120001/customtabs/src/android...
customtabs/src/android/support/customtabs/browseractions/BrowserActionsIntent.java:291:
ResolveInfo defaultHandler = pm.resolveActivity(viewIntent,
PackageManager.MATCH_DEFAULT_ONLY);
On 2017/07/17 17:08:53, Yusuf wrote:
> >100 lines?

Yes, looks like the git cl upload does not check format for support library.

[ltian, 2017-07-17 18:04:32]

The CQ bit was checked by ltian@chromium.org

[ltian, 2017-07-17 18:04:33]

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/2961203002/#ps140001
(title: "Update based on Yusuf's comments.")

[commit-bot: I haz the power, 2017-07-17 18:04:41]

CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-07-17 18:06:01]

CQ is committing da patch.

Bot data: {"patchset_id": 140001, "attempt_start_ts": 1500314672716750,
"parent_rev": "08bd54d3b9ef387c54cc4d4967a400b27c394e05", "commit_rev":
"481ca67f75f35a6d0c6fe9778b1b69c21d6d9203"}

[commit-bot: I haz the power, 2017-07-17 18:06:05]

Description was changed from

==========
Support library changes to allow a callback notifying the selection of default
items for Browser Actions

Changes in support library to add a PendingIntent in BrowserActionsIntent
as the callback which will be launched if a default menu item is selected.
The PendingIntent will includes the id of the chosen default menu item.

This CL also adds a BroadcastReceiver in the CCT test app to handle the
callback PendingIntent and displays the id of the chosen item.

BUG=737814
==========

to

==========
Support library changes to allow a callback notifying the selection of default
items for Browser Actions

Changes in support library to add a PendingIntent in BrowserActionsIntent
as the callback which will be launched if a default menu item is selected.
The PendingIntent will includes the id of the chosen default menu item.

This CL also adds a BroadcastReceiver in the CCT test app to handle the
callback PendingIntent and displays the id of the chosen item.

BUG=737814

Review-Url: https://codereview.chromium.org/2961203002
Committed:
https://github.com/GoogleChrome/custom-tabs-client/commit/481ca67f75f35a6d0c6...
==========

[commit-bot: I haz the power, 2017-07-17 18:06:06]

Committed patchset #8 (id:140001) as
https://github.com/GoogleChrome/custom-tabs-client/commit/481ca67f75f35a6d0c6...