Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2787723002:
[Android] Add public API for Browser Action in support library (Closed)
|
Created:
3 years, 8 months ago by ltian Modified:
3 years, 8 months ago Target Ref:
refs/heads/master Project:
custom-tabs-client Visibility:
Public. |
Description[Android] Add public API for Browser Action in support library
Add a public API for Browser Action in Custom Tabs support library. The
API checks whether Chrome is available to handle the Browser Action
request. If so, wrap the request info in a Browser Action Intent and
send out to let Chrome handle it.
The detail design of the public API and Browser Action Intent wrapper
are here:
https://docs.google.com/a/google.com/document/d/1CkRNEsXjFnzo2X7SEZP2sluIIhhKRcNGbEWwAkJd4OI/edit?usp=sharing.
BUG=706695
Review-Url: https://codereview.chromium.org/2787723002
Committed: https://github.com/GoogleChrome/custom-tabs-client/commit/5dd7021848797eea6f696303ba4d7749d51588d9
Patch Set 1 #
Total comments: 32
Patch Set 2 : Update based on Yusuf's comments. #
Total comments: 24
Patch Set 3 : Update based on Yusuf's comments and add logic to get creator package name. #
Total comments: 16
Patch Set 4 : Update based on Yusuf's new comments. #
Total comments: 8
Patch Set 5 : Update for the new comments from Yusuf. #
Messages
Total messages: 19 (7 generated)
ltian@chromium.org changed reviewers: + tedchoc@chromium.org, yusufo@chromium.org
Can you take a look of this CL to see whether the design makes sense to you? Thanks!
https://codereview.chromium.org/2787723002/diff/1/Application/src/main/java/o... File Application/src/main/java/org/chromium/customtabsclient/MainActivity.java (right): https://codereview.chromium.org/2787723002/diff/1/Application/src/main/java/o... Application/src/main/java/org/chromium/customtabsclient/MainActivity.java:127: mUrlLinkText = (TextView) findViewById(R.id.url_link); We will need another TextView above for an explanation of what this is for. Mainly for testers. Launch browser actions from here: Sample browser actions launcher: https://codereview.chromium.org/2787723002/diff/1/customtabs/src/android/supp... File customtabs/src/android/support/customtabs/browseraction/BrowserActionCreator.java (right): https://codereview.chromium.org/2787723002/diff/1/customtabs/src/android/supp... customtabs/src/android/support/customtabs/browseraction/BrowserActionCreator.java:16: package android.support.customtabs.browseraction; browseractions https://codereview.chromium.org/2787723002/diff/1/customtabs/src/android/supp... customtabs/src/android/support/customtabs/browseraction/BrowserActionCreator.java:30: public class BrowserActionCreator { I think we should combine this into BrowserActionsIntent. It is a bit confusing where to start for a new developer right now. We have a Builder class and a Creator class. https://codereview.chromium.org/2787723002/diff/1/customtabs/src/android/supp... customtabs/src/android/support/customtabs/browseraction/BrowserActionCreator.java:33: * It first checks if Chrome is available to create a browser action menu. no mention of Chrome here. "any BrowserActions provider is available to create a menu." But in general, this will probably be implementation details for the client developer. This would be useful if this was a browser side class, but not here. https://codereview.chromium.org/2787723002/diff/1/customtabs/src/android/supp... customtabs/src/android/support/customtabs/browseraction/BrowserActionCreator.java:62: pm.queryIntentActivities(intent, PackageManager.MATCH_DEFAULT_ONLY); if we say MATCH_DEFAULT_ONLY, we will always fail this check on the first try and that will be problematic since this support lib will probably be the only code that issues this new action. Here we should look for any handlers for this intent even if they are not default. https://codereview.chromium.org/2787723002/diff/1/customtabs/src/android/supp... File customtabs/src/android/support/customtabs/browseraction/BrowserActionIntent.java (right): https://codereview.chromium.org/2787723002/diff/1/customtabs/src/android/supp... customtabs/src/android/support/customtabs/browseraction/BrowserActionIntent.java:40: private final static String TEST_URL = "https://www.google.com"; Since this is not a VIEW intent, but a custom ACTION we are defining, I don't think you need to set data. https://codereview.chromium.org/2787723002/diff/1/customtabs/src/android/supp... customtabs/src/android/support/customtabs/browseraction/BrowserActionIntent.java:61: public static final String KEY_ACTION = "android.support.browseraction.ACTION"; this should follow the packagename android.support.customtabs.browseraction..... https://codereview.chromium.org/2787723002/diff/1/customtabs/src/android/supp... customtabs/src/android/support/customtabs/browseraction/BrowserActionIntent.java:95: @NonNull We should have @NonNull in the constructor rather than here. https://codereview.chromium.org/2787723002/diff/1/customtabs/src/android/supp... customtabs/src/android/support/customtabs/browseraction/BrowserActionIntent.java:120: * Sets the type of Browser Action context menu. The terminology is a bit confusing here. We say MEDIA_TYPE above, we use UrlType in function name, and then we say type in comments. I would say lets pick one (URL_TYPE, UrlType, "url type") and use that everywhere. (@BrowserActionsUrlType, setUrlType, URL_TYPE_XYZ...) https://codereview.chromium.org/2787723002/diff/1/customtabs/src/android/supp... customtabs/src/android/support/customtabs/browseraction/BrowserActionIntent.java:172: * @param url The URL to load in the Custom Tab. Update comment. https://codereview.chromium.org/2787723002/diff/1/customtabs/src/android/supp... customtabs/src/android/support/customtabs/browseraction/BrowserActionIntent.java:180: * Method to create a lightweight Intent to check whether Chrome is available for browser action no mention of Chrome https://codereview.chromium.org/2787723002/diff/1/customtabs/src/android/supp... File customtabs/src/android/support/customtabs/browseraction/BrowserActionItem.java (right): https://codereview.chromium.org/2787723002/diff/1/customtabs/src/android/supp... customtabs/src/android/support/customtabs/browseraction/BrowserActionItem.java:27: private String mTitle; final for title and action https://codereview.chromium.org/2787723002/diff/1/customtabs/src/android/supp... customtabs/src/android/support/customtabs/browseraction/BrowserActionItem.java:37: public BrowserActionItem(String title, PendingIntent action, Bitmap icon) { @Nonnull for the two that cant be null https://codereview.chromium.org/2787723002/diff/1/customtabs/src/android/supp... customtabs/src/android/support/customtabs/browseraction/BrowserActionItem.java:48: public BrowserActionItem(String title, PendingIntent action) { @NonNull for both https://codereview.chromium.org/2787723002/diff/1/customtabs/src/android/supp... customtabs/src/android/support/customtabs/browseraction/BrowserActionItem.java:49: mTitle = title; call the above constructor with params set from here. https://codereview.chromium.org/2787723002/diff/1/customtabs/src/android/supp... customtabs/src/android/support/customtabs/browseraction/BrowserActionItem.java:53: public void setIcon(Bitmap icon) { javadocs here and on other getters.
https://codereview.chromium.org/2787723002/diff/1/Application/src/main/java/o... File Application/src/main/java/org/chromium/customtabsclient/MainActivity.java (right): https://codereview.chromium.org/2787723002/diff/1/Application/src/main/java/o... Application/src/main/java/org/chromium/customtabsclient/MainActivity.java:127: mUrlLinkText = (TextView) findViewById(R.id.url_link); On 2017/04/03 18:00:48, Yusuf wrote: > We will need another TextView above for an explanation of what this is for. > Mainly for testers. > > Launch browser actions from here: > > Sample browser actions launcher: Done. https://codereview.chromium.org/2787723002/diff/1/customtabs/src/android/supp... File customtabs/src/android/support/customtabs/browseraction/BrowserActionCreator.java (right): https://codereview.chromium.org/2787723002/diff/1/customtabs/src/android/supp... customtabs/src/android/support/customtabs/browseraction/BrowserActionCreator.java:16: package android.support.customtabs.browseraction; On 2017/04/03 18:00:48, Yusuf wrote: > browseractions Done. https://codereview.chromium.org/2787723002/diff/1/customtabs/src/android/supp... customtabs/src/android/support/customtabs/browseraction/BrowserActionCreator.java:30: public class BrowserActionCreator { On 2017/04/03 18:00:48, Yusuf wrote: > I think we should combine this into BrowserActionsIntent. It is a bit confusing > where to start for a new developer right now. We have a Builder class and a > Creator class. Done. https://codereview.chromium.org/2787723002/diff/1/customtabs/src/android/supp... customtabs/src/android/support/customtabs/browseraction/BrowserActionCreator.java:33: * It first checks if Chrome is available to create a browser action menu. On 2017/04/03 18:00:48, Yusuf wrote: > no mention of Chrome here. "any BrowserActions provider is available to create a > menu." > > But in general, this will probably be implementation details for the client > developer. This would be useful if this was a browser side class, but not here. Done. https://codereview.chromium.org/2787723002/diff/1/customtabs/src/android/supp... customtabs/src/android/support/customtabs/browseraction/BrowserActionCreator.java:62: pm.queryIntentActivities(intent, PackageManager.MATCH_DEFAULT_ONLY); On 2017/04/03 18:00:48, Yusuf wrote: > if we say MATCH_DEFAULT_ONLY, we will always fail this check on the first try > and that will be problematic since this support lib will probably be the only > code that issues this new action. Here we should look for any handlers for this > intent even if they are not default. Done. https://codereview.chromium.org/2787723002/diff/1/customtabs/src/android/supp... File customtabs/src/android/support/customtabs/browseraction/BrowserActionIntent.java (right): https://codereview.chromium.org/2787723002/diff/1/customtabs/src/android/supp... customtabs/src/android/support/customtabs/browseraction/BrowserActionIntent.java:40: private final static String TEST_URL = "https://www.google.com"; On 2017/04/03 18:00:48, Yusuf wrote: > Since this is not a VIEW intent, but a custom ACTION we are defining, I don't > think you need to set data. I find we still need this to set data because in AndroidManifest I set the data scheme for the intent filter as http or https. Should I remove those schemes? https://codereview.chromium.org/2787723002/diff/1/customtabs/src/android/supp... customtabs/src/android/support/customtabs/browseraction/BrowserActionIntent.java:61: public static final String KEY_ACTION = "android.support.browseraction.ACTION"; On 2017/04/03 18:00:48, Yusuf wrote: > this should follow the packagename android.support.customtabs.browseraction..... Done. https://codereview.chromium.org/2787723002/diff/1/customtabs/src/android/supp... customtabs/src/android/support/customtabs/browseraction/BrowserActionIntent.java:95: @NonNull On 2017/04/03 18:00:48, Yusuf wrote: > We should have @NonNull in the constructor rather than here. Done. https://codereview.chromium.org/2787723002/diff/1/customtabs/src/android/supp... customtabs/src/android/support/customtabs/browseraction/BrowserActionIntent.java:120: * Sets the type of Browser Action context menu. On 2017/04/03 18:00:48, Yusuf wrote: > The terminology is a bit confusing here. We say MEDIA_TYPE above, we use UrlType > in function name, and then we say type in comments. I would say lets pick one > (URL_TYPE, UrlType, "url type") and use that everywhere. > (@BrowserActionsUrlType, setUrlType, URL_TYPE_XYZ...) Done. https://codereview.chromium.org/2787723002/diff/1/customtabs/src/android/supp... customtabs/src/android/support/customtabs/browseraction/BrowserActionIntent.java:172: * @param url The URL to load in the Custom Tab. On 2017/04/03 18:00:48, Yusuf wrote: > Update comment. Done. https://codereview.chromium.org/2787723002/diff/1/customtabs/src/android/supp... customtabs/src/android/support/customtabs/browseraction/BrowserActionIntent.java:180: * Method to create a lightweight Intent to check whether Chrome is available for browser action On 2017/04/03 18:00:48, Yusuf wrote: > no mention of Chrome Done. https://codereview.chromium.org/2787723002/diff/1/customtabs/src/android/supp... File customtabs/src/android/support/customtabs/browseraction/BrowserActionItem.java (right): https://codereview.chromium.org/2787723002/diff/1/customtabs/src/android/supp... customtabs/src/android/support/customtabs/browseraction/BrowserActionItem.java:27: private String mTitle; On 2017/04/03 18:00:49, Yusuf wrote: > final for title and action Done. https://codereview.chromium.org/2787723002/diff/1/customtabs/src/android/supp... customtabs/src/android/support/customtabs/browseraction/BrowserActionItem.java:37: public BrowserActionItem(String title, PendingIntent action, Bitmap icon) { On 2017/04/03 18:00:49, Yusuf wrote: > @Nonnull for the two that cant be null Done. https://codereview.chromium.org/2787723002/diff/1/customtabs/src/android/supp... customtabs/src/android/support/customtabs/browseraction/BrowserActionItem.java:48: public BrowserActionItem(String title, PendingIntent action) { On 2017/04/03 18:00:49, Yusuf wrote: > @NonNull for both Done. https://codereview.chromium.org/2787723002/diff/1/customtabs/src/android/supp... customtabs/src/android/support/customtabs/browseraction/BrowserActionItem.java:49: mTitle = title; On 2017/04/03 18:00:49, Yusuf wrote: > call the above constructor with params set from here. Done. https://codereview.chromium.org/2787723002/diff/1/customtabs/src/android/supp... customtabs/src/android/support/customtabs/browseraction/BrowserActionItem.java:53: public void setIcon(Bitmap icon) { On 2017/04/03 18:00:48, Yusuf wrote: > javadocs here and on other getters. Done.
https://codereview.chromium.org/2787723002/diff/20001/Application/src/main/re... File Application/src/main/res/layout/main.xml (right): https://codereview.chromium.org/2787723002/diff/20001/Application/src/main/re... Application/src/main/res/layout/main.xml:96: android:text="Launch Browser Actions by long press the url below:" /> add to strings xml https://codereview.chromium.org/2787723002/diff/20001/customtabs/src/android/... File customtabs/src/android/support/customtabs/browseractions/BrowserActionItem.java (right): https://codereview.chromium.org/2787723002/diff/20001/customtabs/src/android/... customtabs/src/android/support/customtabs/browseractions/BrowserActionItem.java:28: final private String mTitle; private final https://codereview.chromium.org/2787723002/diff/20001/customtabs/src/android/... customtabs/src/android/support/customtabs/browseractions/BrowserActionItem.java:63: * @return The icon of a custom item. just @return is fine https://codereview.chromium.org/2787723002/diff/20001/customtabs/src/android/... customtabs/src/android/support/customtabs/browseractions/BrowserActionItem.java:71: * @return The title of a custom item. just @return for calls with no param and a single return value. https://codereview.chromium.org/2787723002/diff/20001/customtabs/src/android/... File customtabs/src/android/support/customtabs/browseractions/BrowserActionsIntent.java (right): https://codereview.chromium.org/2787723002/diff/20001/customtabs/src/android/... customtabs/src/android/support/customtabs/browseractions/BrowserActionsIntent.java:43: private final static String TEST_URL = "https://www.example.com"; yes, we should still get rid of this . relying on a scheme is not something we embed on library right now https://codereview.chromium.org/2787723002/diff/20001/customtabs/src/android/... customtabs/src/android/support/customtabs/browseractions/BrowserActionsIntent.java:101: javadoc https://codereview.chromium.org/2787723002/diff/20001/customtabs/src/android/... customtabs/src/android/support/customtabs/browseractions/BrowserActionsIntent.java:125: public Builder(String url) { Uri for all url related public params https://codereview.chromium.org/2787723002/diff/20001/customtabs/src/android/... customtabs/src/android/support/customtabs/browseractions/BrowserActionsIntent.java:141: * Sets the custom items list. Comments on max items and usage. https://codereview.chromium.org/2787723002/diff/20001/customtabs/src/android/... customtabs/src/android/support/customtabs/browseractions/BrowserActionsIntent.java:165: bundle.putParcelable(KEY_ICON, item.getIcon()); one line https://codereview.chromium.org/2787723002/diff/20001/customtabs/src/android/... customtabs/src/android/support/customtabs/browseractions/BrowserActionsIntent.java:214: builder.setCustomItems(items); stack these calls on top of each other https://codereview.chromium.org/2787723002/diff/20001/customtabs/src/android/... customtabs/src/android/support/customtabs/browseractions/BrowserActionsIntent.java:239: * @param context The context requests for a Browser Actions menu. requesting https://codereview.chromium.org/2787723002/diff/20001/customtabs/src/android/... customtabs/src/android/support/customtabs/browseractions/BrowserActionsIntent.java:244: private static void openLocalBrowserActions( openFallbackBrowserActionsMenu
https://codereview.chromium.org/2787723002/diff/20001/Application/src/main/re... File Application/src/main/res/layout/main.xml (right): https://codereview.chromium.org/2787723002/diff/20001/Application/src/main/re... Application/src/main/res/layout/main.xml:96: android:text="Launch Browser Actions by long press the url below:" /> On 2017/04/05 02:38:30, Yusuf wrote: > add to strings xml Done. https://codereview.chromium.org/2787723002/diff/20001/customtabs/src/android/... File customtabs/src/android/support/customtabs/browseractions/BrowserActionItem.java (right): https://codereview.chromium.org/2787723002/diff/20001/customtabs/src/android/... customtabs/src/android/support/customtabs/browseractions/BrowserActionItem.java:28: final private String mTitle; On 2017/04/05 02:38:30, Yusuf wrote: > private final Done. https://codereview.chromium.org/2787723002/diff/20001/customtabs/src/android/... customtabs/src/android/support/customtabs/browseractions/BrowserActionItem.java:63: * @return The icon of a custom item. On 2017/04/05 02:38:30, Yusuf wrote: > just @return is fine Done. https://codereview.chromium.org/2787723002/diff/20001/customtabs/src/android/... customtabs/src/android/support/customtabs/browseractions/BrowserActionItem.java:71: * @return The title of a custom item. On 2017/04/05 02:38:30, Yusuf wrote: > just @return for calls with no param and a single return value. Done. https://codereview.chromium.org/2787723002/diff/20001/customtabs/src/android/... File customtabs/src/android/support/customtabs/browseractions/BrowserActionsIntent.java (right): https://codereview.chromium.org/2787723002/diff/20001/customtabs/src/android/... customtabs/src/android/support/customtabs/browseractions/BrowserActionsIntent.java:43: private final static String TEST_URL = "https://www.example.com"; On 2017/04/05 02:38:30, Yusuf wrote: > yes, we should still get rid of this . relying on a scheme is not something we > embed on library right now We might need this because the intent filter needs to have "http" and "https" schemes and the test intent need to match any of those two schemes. https://codereview.chromium.org/2787723002/diff/20001/customtabs/src/android/... customtabs/src/android/support/customtabs/browseractions/BrowserActionsIntent.java:101: On 2017/04/05 02:38:30, Yusuf wrote: > javadoc Done. https://codereview.chromium.org/2787723002/diff/20001/customtabs/src/android/... customtabs/src/android/support/customtabs/browseractions/BrowserActionsIntent.java:125: public Builder(String url) { On 2017/04/05 02:38:30, Yusuf wrote: > Uri for all url related public params Done. https://codereview.chromium.org/2787723002/diff/20001/customtabs/src/android/... customtabs/src/android/support/customtabs/browseractions/BrowserActionsIntent.java:141: * Sets the custom items list. On 2017/04/05 02:38:31, Yusuf wrote: > Comments on max items and usage. Done. https://codereview.chromium.org/2787723002/diff/20001/customtabs/src/android/... customtabs/src/android/support/customtabs/browseractions/BrowserActionsIntent.java:165: bundle.putParcelable(KEY_ICON, item.getIcon()); On 2017/04/05 02:38:30, Yusuf wrote: > one line Done. https://codereview.chromium.org/2787723002/diff/20001/customtabs/src/android/... customtabs/src/android/support/customtabs/browseractions/BrowserActionsIntent.java:214: builder.setCustomItems(items); On 2017/04/05 02:38:31, Yusuf wrote: > stack these calls on top of each other Done. https://codereview.chromium.org/2787723002/diff/20001/customtabs/src/android/... customtabs/src/android/support/customtabs/browseractions/BrowserActionsIntent.java:239: * @param context The context requests for a Browser Actions menu. On 2017/04/05 02:38:31, Yusuf wrote: > requesting Done. https://codereview.chromium.org/2787723002/diff/20001/customtabs/src/android/... customtabs/src/android/support/customtabs/browseractions/BrowserActionsIntent.java:244: private static void openLocalBrowserActions( On 2017/04/05 02:38:31, Yusuf wrote: > openFallbackBrowserActionsMenu Done.
pretty close to done here. probably the final one. Will try to get back to this immediately. https://codereview.chromium.org/2787723002/diff/40001/customtabs/src/android/... File customtabs/src/android/support/customtabs/browseractions/BrowserActionItem.java (right): https://codereview.chromium.org/2787723002/diff/40001/customtabs/src/android/... customtabs/src/android/support/customtabs/browseractions/BrowserActionItem.java:63: * @return. sorry I meant @return The icon of of a custom item. https://codereview.chromium.org/2787723002/diff/40001/customtabs/src/android/... customtabs/src/android/support/customtabs/browseractions/BrowserActionItem.java:70: * Gets the title of a custom item. @return The title of a custom item. https://codereview.chromium.org/2787723002/diff/40001/customtabs/src/android/... File customtabs/src/android/support/customtabs/browseractions/BrowserActionsIntent.java (right): https://codereview.chromium.org/2787723002/diff/40001/customtabs/src/android/... customtabs/src/android/support/customtabs/browseractions/BrowserActionsIntent.java:47: * Extra that specifies {@link PendingIntent} indicates which Application sends the {@link BrowserActionsIntent}. indicating https://codereview.chromium.org/2787723002/diff/40001/customtabs/src/android/... customtabs/src/android/support/customtabs/browseractions/BrowserActionsIntent.java:109: * Gets the Intent of {@link BrowserActionsIntent}. @return The Intent to be used in {@link BrowserActionsIntent}. https://codereview.chromium.org/2787723002/diff/40001/customtabs/src/android/... customtabs/src/android/support/customtabs/browseractions/BrowserActionsIntent.java:136: public Builder(Context context, Uri uri) { add Context to javadoc https://codereview.chromium.org/2787723002/diff/40001/customtabs/src/android/... customtabs/src/android/support/customtabs/browseractions/BrowserActionsIntent.java:154: * Only maximum 5 custom items are allowed, otherwise throws an {@link IllegalStateException}. @link to MAX_CUSTOM_ITEMS here. https://codereview.chromium.org/2787723002/diff/40001/customtabs/src/android/... customtabs/src/android/support/customtabs/browseractions/BrowserActionsIntent.java:224: BrowserActionsIntent intent = new BrowserActionsIntent.Builder(context, uri).setUrlType(type).setCustomItems(items).build(); alignment issue here? https://codereview.chromium.org/2787723002/diff/40001/customtabs/src/android/... customtabs/src/android/support/customtabs/browseractions/BrowserActionsIntent.java:264: return intent.getParcelableExtra(BrowserActionsIntent.EXTRA_APP_ID); this should return String and we should do getCreatorPackage on the pending intent ourselves. We shouldn't leak the pending intent here.
https://codereview.chromium.org/2787723002/diff/40001/customtabs/src/android/... File customtabs/src/android/support/customtabs/browseractions/BrowserActionItem.java (right): https://codereview.chromium.org/2787723002/diff/40001/customtabs/src/android/... customtabs/src/android/support/customtabs/browseractions/BrowserActionItem.java:63: * @return. On 2017/04/14 21:58:11, Yusuf wrote: > sorry I meant > > @return The icon of of a custom item. Done. https://codereview.chromium.org/2787723002/diff/40001/customtabs/src/android/... customtabs/src/android/support/customtabs/browseractions/BrowserActionItem.java:70: * Gets the title of a custom item. On 2017/04/14 21:58:11, Yusuf wrote: > @return The title of a custom item. Done. https://codereview.chromium.org/2787723002/diff/40001/customtabs/src/android/... File customtabs/src/android/support/customtabs/browseractions/BrowserActionsIntent.java (right): https://codereview.chromium.org/2787723002/diff/40001/customtabs/src/android/... customtabs/src/android/support/customtabs/browseractions/BrowserActionsIntent.java:47: * Extra that specifies {@link PendingIntent} indicates which Application sends the {@link BrowserActionsIntent}. On 2017/04/14 21:58:11, Yusuf wrote: > indicating Done. https://codereview.chromium.org/2787723002/diff/40001/customtabs/src/android/... customtabs/src/android/support/customtabs/browseractions/BrowserActionsIntent.java:109: * Gets the Intent of {@link BrowserActionsIntent}. On 2017/04/14 21:58:11, Yusuf wrote: > @return The Intent to be used in {@link BrowserActionsIntent}. Done. https://codereview.chromium.org/2787723002/diff/40001/customtabs/src/android/... customtabs/src/android/support/customtabs/browseractions/BrowserActionsIntent.java:136: public Builder(Context context, Uri uri) { On 2017/04/14 21:58:11, Yusuf wrote: > add Context to javadoc Done. https://codereview.chromium.org/2787723002/diff/40001/customtabs/src/android/... customtabs/src/android/support/customtabs/browseractions/BrowserActionsIntent.java:154: * Only maximum 5 custom items are allowed, otherwise throws an {@link IllegalStateException}. On 2017/04/14 21:58:11, Yusuf wrote: > @link to MAX_CUSTOM_ITEMS here. Done. https://codereview.chromium.org/2787723002/diff/40001/customtabs/src/android/... customtabs/src/android/support/customtabs/browseractions/BrowserActionsIntent.java:224: BrowserActionsIntent intent = new BrowserActionsIntent.Builder(context, uri).setUrlType(type).setCustomItems(items).build(); On 2017/04/14 21:58:11, Yusuf wrote: > alignment issue here? Done. https://codereview.chromium.org/2787723002/diff/40001/customtabs/src/android/... customtabs/src/android/support/customtabs/browseractions/BrowserActionsIntent.java:264: return intent.getParcelableExtra(BrowserActionsIntent.EXTRA_APP_ID); On 2017/04/14 21:58:11, Yusuf wrote: > this should return String and we should do getCreatorPackage on the pending > intent ourselves. We shouldn't leak the pending intent here. Done.
lgtm with final styling nits https://codereview.chromium.org/2787723002/diff/60001/customtabs/src/android/... File customtabs/src/android/support/customtabs/browseractions/BrowserActionItem.java (right): https://codereview.chromium.org/2787723002/diff/60001/customtabs/src/android/... customtabs/src/android/support/customtabs/browseractions/BrowserActionItem.java:62: * Gets the icon of of a custom item. /** * @return the icon of of a custom item. */ here. So just a single line with @return at the beginning and that the description of what it returns. https://codereview.chromium.org/2787723002/diff/60001/customtabs/src/android/... customtabs/src/android/support/customtabs/browseractions/BrowserActionItem.java:71: * @return the title of a custom item.. here as well https://codereview.chromium.org/2787723002/diff/60001/customtabs/src/android/... customtabs/src/android/support/customtabs/browseractions/BrowserActionItem.java:79: * @return the action of a custom item. ditto https://codereview.chromium.org/2787723002/diff/60001/customtabs/src/android/... File customtabs/src/android/support/customtabs/browseractions/BrowserActionsIntent.java (right): https://codereview.chromium.org/2787723002/diff/60001/customtabs/src/android/... customtabs/src/android/support/customtabs/browseractions/BrowserActionsIntent.java:229: .setUrlType(type) 8 space, but keep the alignment.
The CQ bit was checked by ltian@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by ltian@chromium.org
https://codereview.chromium.org/2787723002/diff/60001/customtabs/src/android/... File customtabs/src/android/support/customtabs/browseractions/BrowserActionItem.java (right): https://codereview.chromium.org/2787723002/diff/60001/customtabs/src/android/... customtabs/src/android/support/customtabs/browseractions/BrowserActionItem.java:62: * Gets the icon of of a custom item. On 2017/04/18 17:44:16, Yusuf wrote: > /** > * @return the icon of of a custom item. > */ > > here. So just a single line with @return at the beginning and that the > description of what it returns. Done. https://codereview.chromium.org/2787723002/diff/60001/customtabs/src/android/... customtabs/src/android/support/customtabs/browseractions/BrowserActionItem.java:71: * @return the title of a custom item.. On 2017/04/18 17:44:16, Yusuf wrote: > here as well Done. https://codereview.chromium.org/2787723002/diff/60001/customtabs/src/android/... customtabs/src/android/support/customtabs/browseractions/BrowserActionItem.java:79: * @return the action of a custom item. On 2017/04/18 17:44:16, Yusuf wrote: > ditto Done. https://codereview.chromium.org/2787723002/diff/60001/customtabs/src/android/... File customtabs/src/android/support/customtabs/browseractions/BrowserActionsIntent.java (right): https://codereview.chromium.org/2787723002/diff/60001/customtabs/src/android/... customtabs/src/android/support/customtabs/browseractions/BrowserActionsIntent.java:229: .setUrlType(type) On 2017/04/18 17:44:17, Yusuf wrote: > 8 space, but keep the alignment. Done.
The CQ bit was checked by ltian@chromium.org
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/2787723002/#ps80001 (title: "Update for the new comments from Yusuf.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 80001, "attempt_start_ts": 1492554639021630,
"parent_rev": "22714ed88155b0d9e7b7b963944935ed7cd7879a", "commit_rev":
"5dd7021848797eea6f696303ba4d7749d51588d9"}
Message was sent while issue was closed.
Description was changed from ========== [Android] Add public API for Browser Action in support library Add a public API for Browser Action in Custom Tabs support library. The API checks whether Chrome is available to handle the Browser Action request. If so, wrap the request info in a Browser Action Intent and send out to let Chrome handle it. The detail design of the public API and Browser Action Intent wrapper are here: https://docs.google.com/a/google.com/document/d/1CkRNEsXjFnzo2X7SEZP2sluIIhhK.... BUG=706695 ========== to ========== [Android] Add public API for Browser Action in support library Add a public API for Browser Action in Custom Tabs support library. The API checks whether Chrome is available to handle the Browser Action request. If so, wrap the request info in a Browser Action Intent and send out to let Chrome handle it. The detail design of the public API and Browser Action Intent wrapper are here: https://docs.google.com/a/google.com/document/d/1CkRNEsXjFnzo2X7SEZP2sluIIhhK.... BUG=706695 Review-Url: https://codereview.chromium.org/2787723002 Committed: https://github.com/GoogleChrome/custom-tabs-client/commit/5dd7021848797eea6f6... ==========
Message was sent while issue was closed.
Committed patchset #5 (id:80001) as https://github.com/GoogleChrome/custom-tabs-client/commit/5dd7021848797eea6f6... |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
