Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1291083004:
[Custom Tabs]Add API for updating action button in service (Closed)
Description[Custom Tabs]Add API for updating action button in service
This CL adds a new API in ICustomTabsService.aidl, letting client app
update the toolbar UI for an ongoing session. Currently only updating
the action button is allowed.
BUG=510256
Committed: https://crrev.com/773b9523d10279efa4a83642fbc82ae4eebe1219
Cr-Commit-Position: refs/heads/master@{#350047}
Patch Set 1 #
Total comments: 11
Patch Set 2 : #
Total comments: 16
Patch Set 3 : address comments #
Total comments: 10
Patch Set 4 : make updateToolbarUrl return boolean #
Total comments: 26
Patch Set 5 : move more logic to ActionButtonParams #Patch Set 6 : change signature from updateToolbar to updateVisual #
Total comments: 1
Patch Set 7 : fix test #Patch Set 8 : cr.->cr_ #Depends on Patchset: Messages
Total messages: 33 (9 generated)
ianwen@chromium.org changed reviewers: + lizeb@chromium.org, pasko@chromium.org, yusufo@chromium.org
https://codereview.chromium.org/1291083004/diff/1/chrome/android/java/src/org... File chrome/android/java/src/org/chromium/chrome/browser/customtabs/CustomTabActivity.java (right): https://codereview.chromium.org/1291083004/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/customtabs/CustomTabActivity.java:101: static CustomTabContentHandler getActiveContentHandler() { have a static call that checks if the session matches with the active content handler rather than having a getter and exposing the static member here. https://codereview.chromium.org/1291083004/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/customtabs/CustomTabActivity.java:224: public boolean updateActionButton(final ActionButtonParams params) { check if session matches. https://codereview.chromium.org/1291083004/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/customtabs/CustomTabActivity.java:226: Drawable icon = mIntentDataProvider.getActionButtonIcon(getResources(), params); params already has an mIcon and that seems to be checked properly. Why do we need this here? https://codereview.chromium.org/1291083004/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/customtabs/CustomTabActivity.java:227: getToolbarManager().addCustomActionButton(icon, params.mDescription, Note that the request is mostly focused around just changing the icon(and description) here. Most likely we won't get a click listener update here in a lot of cases. In that case we should avoid updating the click listener, but just update the icon. https://codereview.chromium.org/1291083004/diff/1/chrome/android/java/src/org... File chrome/android/java/src/org/chromium/chrome/browser/customtabs/CustomTabIntentDataProvider.java (right): https://codereview.chromium.org/1291083004/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/customtabs/CustomTabIntentDataProvider.java:80: Bitmap mIcon; final https://codereview.chromium.org/1291083004/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/customtabs/CustomTabIntentDataProvider.java:81: String mDescription; final https://codereview.chromium.org/1291083004/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/customtabs/CustomTabIntentDataProvider.java:82: PendingIntent mPendingIntent; final
Thank you for doing this, it is probably the good way to update the UI. A few nits, and a more significant comment, since the service calls are not executing on the UI thread. https://codereview.chromium.org/1291083004/diff/20001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/customtabs/CustomTabActivity.java (right): https://codereview.chromium.org/1291083004/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/customtabs/CustomTabActivity.java:330: getToolbarManager().addCustomActionButton( nit: Should "addCustomActionButton" be renamed "setCustomActionButton"? https://codereview.chromium.org/1291083004/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/customtabs/CustomTabActivity.java:330: getToolbarManager().addCustomActionButton( Also a stupid UX/UI question from someone who knows nothing about it: should we animate the transition between the two buttons? https://codereview.chromium.org/1291083004/diff/20001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/customtabs/CustomTabIntentDataProvider.java (right): https://codereview.chromium.org/1291083004/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/customtabs/CustomTabIntentDataProvider.java:80: Bitmap mIcon; Shouldn't these members be final? https://codereview.chromium.org/1291083004/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/customtabs/CustomTabIntentDataProvider.java:87: public ActionButtonParams(Bitmap icon, String description, PendingIntent pendingIntent) { nit: You could make this constructor private, and have a public static method "fromBundle" with the same content as parseActionButtonBundle(). This way you are sure that no invalid ActionButtonParams can be built (with a null description for instance). https://codereview.chromium.org/1291083004/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/customtabs/CustomTabIntentDataProvider.java:333: assert mActionButtonParams != null; nit: assert shouldShowActionButton() ? https://codereview.chromium.org/1291083004/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/customtabs/CustomTabIntentDataProvider.java:343: private static boolean checkCustomButtonIconWithinBounds(Context context, Bitmap bitmap) { Should this move to ActionButtonParams? You can do all the checks you have to at creation time, and then not bother with it later, since everything is final in this class. https://codereview.chromium.org/1291083004/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/customtabs/CustomTabIntentDataProvider.java:363: static ActionButtonParams parseActionButtonBundle(Context context, Bundle bundle) { See above comment about moving this to ActionButtonParams#fromBundle(). https://codereview.chromium.org/1291083004/diff/20001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/customtabs/CustomTabsConnection.java (right): https://codereview.chromium.org/1291083004/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/customtabs/CustomTabsConnection.java:311: public boolean updateToolbarUI(ICustomTabsCallback callback, Bundle bundle) { This function can be called on *any* thread, since we are using an AIDL-based service. This means that you probably want to post a task to the UI thread to do the UI update. Parsing the Bundle and constructing the ActionButtonParams instance here is fine though. And since you have to return a boolean, this seems to be an additional reason why you want to separate the parsing and error-checking in ActionButtonParams from the actual UI manipulation.
Thanks for the comments. All addressed. In patchset 3 I couldn't find an elegant way to let the API return a boolean, while at the same time not wait for the task in UI thread to finish. Therefore I modified the aidl to be a void method. Any suggestions? https://codereview.chromium.org/1291083004/diff/1/chrome/android/java/src/org... File chrome/android/java/src/org/chromium/chrome/browser/customtabs/CustomTabActivity.java (right): https://codereview.chromium.org/1291083004/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/customtabs/CustomTabActivity.java:101: static CustomTabContentHandler getActiveContentHandler() { On 2015/08/19 06:29:16, Yusuf wrote: > have a static call that checks if the session matches with the active content > handler rather than having a getter and exposing the static member here. Agreed that getter for a static variable is dangerous and may cause memory leak. Done. https://codereview.chromium.org/1291083004/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/customtabs/CustomTabActivity.java:224: public boolean updateActionButton(final ActionButtonParams params) { On 2015/08/19 06:29:16, Yusuf wrote: > check if session matches. Done in the static method above. https://codereview.chromium.org/1291083004/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/customtabs/CustomTabActivity.java:226: Drawable icon = mIntentDataProvider.getActionButtonIcon(getResources(), params); On 2015/08/19 06:29:16, Yusuf wrote: > params already has an mIcon and that seems to be checked properly. Why do we > need this here? Yes. I removed this line. Done. https://codereview.chromium.org/1291083004/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/customtabs/CustomTabActivity.java:227: getToolbarManager().addCustomActionButton(icon, params.mDescription, On 2015/08/19 06:29:16, Yusuf wrote: > Note that the request is mostly focused around just changing the icon(and > description) here. Most likely we won't get a click listener update here in a > lot of cases. In that case we should avoid updating the click listener, but just > update the icon. Done. https://codereview.chromium.org/1291083004/diff/20001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/customtabs/CustomTabActivity.java (right): https://codereview.chromium.org/1291083004/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/customtabs/CustomTabActivity.java:330: getToolbarManager().addCustomActionButton( On 2015/08/19 09:08:19, Benoit L wrote: > nit: Should "addCustomActionButton" be renamed "setCustomActionButton"? Done. https://codereview.chromium.org/1291083004/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/customtabs/CustomTabActivity.java:330: getToolbarManager().addCustomActionButton( On 2015/08/19 09:08:18, Benoit L wrote: > Also a stupid UX/UI question from someone who knows nothing about it: should we > animate the transition between the two buttons? I think animation in this case is not that obvious to the user. In most cases the button is updated when the user clicks on it, and during this action the finger hides the underlying animation. https://codereview.chromium.org/1291083004/diff/20001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/customtabs/CustomTabIntentDataProvider.java (right): https://codereview.chromium.org/1291083004/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/customtabs/CustomTabIntentDataProvider.java:80: Bitmap mIcon; On 2015/08/19 09:08:19, Benoit L wrote: > Shouldn't these members be final? Yes they should be final. Done. https://codereview.chromium.org/1291083004/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/customtabs/CustomTabIntentDataProvider.java:87: public ActionButtonParams(Bitmap icon, String description, PendingIntent pendingIntent) { On 2015/08/19 09:08:19, Benoit L wrote: > nit: You could make this constructor private, and have a public static method > "fromBundle" with the same content as parseActionButtonBundle(). This way you > are sure that no invalid ActionButtonParams can be built (with a null > description for instance). Brilliant idea! Done. https://codereview.chromium.org/1291083004/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/customtabs/CustomTabIntentDataProvider.java:333: assert mActionButtonParams != null; On 2015/08/19 09:08:19, Benoit L wrote: > nit: assert shouldShowActionButton() ? Done. https://codereview.chromium.org/1291083004/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/customtabs/CustomTabIntentDataProvider.java:343: private static boolean checkCustomButtonIconWithinBounds(Context context, Bitmap bitmap) { On 2015/08/19 09:08:19, Benoit L wrote: > Should this move to ActionButtonParams? You can do all the checks you have to at > creation time, and then not bother with it later, since everything is final in > this class. Awesome idea! Done! https://codereview.chromium.org/1291083004/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/customtabs/CustomTabIntentDataProvider.java:363: static ActionButtonParams parseActionButtonBundle(Context context, Bundle bundle) { On 2015/08/19 09:08:19, Benoit L wrote: > See above comment about moving this to ActionButtonParams#fromBundle(). Done. https://codereview.chromium.org/1291083004/diff/20001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/customtabs/CustomTabsConnection.java (right): https://codereview.chromium.org/1291083004/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/customtabs/CustomTabsConnection.java:311: public boolean updateToolbarUI(ICustomTabsCallback callback, Bundle bundle) { On 2015/08/19 09:08:19, Benoit L wrote: > This function can be called on *any* thread, since we are using an AIDL-based > service. This means that you probably want to post a task to the UI thread to do > the UI update. Parsing the Bundle and constructing the ActionButtonParams > instance here is fine though. > > And since you have to return a boolean, this seems to be an additional reason > why you want to separate the parsing and error-checking in ActionButtonParams > from the actual UI manipulation. Good catch. Done.
Thank you, looks good. Only one real comment, I think that the service call should return a boolean, and only return when the UI has been updated. See the comment inline. https://codereview.chromium.org/1291083004/diff/40001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/customtabs/ActionButtonParams.java (right): https://codereview.chromium.org/1291083004/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/customtabs/ActionButtonParams.java:27: * Convenient constructor for {@link ActionButtonParams}. nit: Is this javadoc really needed? https://codereview.chromium.org/1291083004/diff/40001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/customtabs/CustomTabsConnection.java (right): https://codereview.chromium.org/1291083004/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/customtabs/CustomTabsConnection.java:327: public void updateToolbarUI(final ICustomTabsCallback callback, Bundle bundle) { You have two solutions to return something meaningful from here: 1. Use ThreadUtils.runOnUiThreadBlocking(): may be a bit ugly since it can introduce a longish wait for the client, but at least the client knows that the button has been updated when the call returns. 2. Return false if ActionButtonParams.fromBundle() returns false. I would actually suggest to do both :-) - Return false if the bundle cannot be parsed, on whatever thread this call executes. - Use ThreadUtils.runOnUiThreadBlocking() to do the actual UI update. I think that there is a value for the client to know that this call doesn't return until the UI is updated. Otherwise you could have a nasty (and very unlikely) case where the user clicks twice on the button for instance, and the second click appears to the application to be on the updated button, whereas it wasn't. For that you would need: 1a. User clicks on the first button 1b. The user clicks again, but the UI thread is busy 2a. The client application updates the button 2b. updateToolbarUI returns 3a. The click is dispatched to the client app 3b. The UI is actually updated. https://codereview.chromium.org/1291083004/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/customtabs/CustomTabsConnection.java:330: if (actionButtonBundle == null) return; return false; https://codereview.chromium.org/1291083004/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/customtabs/CustomTabsConnection.java:334: if (params == null) return; return false; https://codereview.chromium.org/1291083004/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/customtabs/CustomTabsConnection.java:335: ThreadUtils.postOnUiThread(new Runnable() { final boolean[] didSucceed = {true}; and in the runnable: didSucceed[0] = result;
https://codereview.chromium.org/1291083004/diff/40001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/customtabs/ActionButtonParams.java (right): https://codereview.chromium.org/1291083004/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/customtabs/ActionButtonParams.java:27: * Convenient constructor for {@link ActionButtonParams}. On 2015/08/20 09:28:49, Benoit L wrote: > nit: Is this javadoc really needed? Removed. https://codereview.chromium.org/1291083004/diff/40001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/customtabs/CustomTabsConnection.java (right): https://codereview.chromium.org/1291083004/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/customtabs/CustomTabsConnection.java:327: public void updateToolbarUI(final ICustomTabsCallback callback, Bundle bundle) { On 2015/08/20 09:28:49, Benoit L wrote: > You have two solutions to return something meaningful from here: > 1. Use ThreadUtils.runOnUiThreadBlocking(): may be a bit ugly since it can > introduce a longish wait for the client, but at least the client knows that the > button has been updated when the call returns. > 2. Return false if ActionButtonParams.fromBundle() returns false. > > I would actually suggest to do both :-) > - Return false if the bundle cannot be parsed, on whatever thread this call > executes. > - Use ThreadUtils.runOnUiThreadBlocking() to do the actual UI update. > > I think that there is a value for the client to know that this call doesn't > return until the UI is updated. Otherwise you could have a nasty (and very > unlikely) case where the user clicks twice on the button for instance, and the > second click appears to the application to be on the updated button, whereas it > wasn't. > > For that you would need: > 1a. User clicks on the first button > 1b. The user clicks again, but the UI thread is busy > 2a. The client application updates the button > 2b. updateToolbarUI returns > 3a. The click is dispatched to the client app > 3b. The UI is actually updated. Great idea. I parse the data here and return false if any error happens. Also I wait for the ui thread to update. Done. https://codereview.chromium.org/1291083004/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/customtabs/CustomTabsConnection.java:330: if (actionButtonBundle == null) return; On 2015/08/20 09:28:49, Benoit L wrote: > return false; Done. https://codereview.chromium.org/1291083004/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/customtabs/CustomTabsConnection.java:334: if (params == null) return; On 2015/08/20 09:28:49, Benoit L wrote: > return false; Done. https://codereview.chromium.org/1291083004/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/customtabs/CustomTabsConnection.java:335: ThreadUtils.postOnUiThread(new Runnable() { On 2015/08/20 09:28:49, Benoit L wrote: > final boolean[] didSucceed = {true}; > > and in the runnable: > didSucceed[0] = result; Done.
https://codereview.chromium.org/1291083004/diff/60001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/customtabs/ActionButtonParams.java (right): https://codereview.chromium.org/1291083004/diff/60001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/customtabs/ActionButtonParams.java:22: final Bitmap mIcon; see below. I think we should use a single static way of constructing this and also make this not final. Sp lets add getters for mIcon and mDescription. https://codereview.chromium.org/1291083004/diff/60001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/customtabs/ActionButtonParams.java:26: public ActionButtonParams(@NonNull Bitmap icon, @NonNull String description, why not have a class method with update(Bitmap, String) for this? The static call below makes it look like that is the only constructor. Then we have this, which I think is a bit confusing . https://codereview.chromium.org/1291083004/diff/60001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/customtabs/ActionButtonParams.java:35: * @param bundle The action button button specified by action button bundle https://codereview.chromium.org/1291083004/diff/60001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/customtabs/CustomTabActivity.java (right): https://codereview.chromium.org/1291083004/diff/60001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/customtabs/CustomTabActivity.java:229: mIntentDataProvider.setActionButtonParams(params); add a updateActionButtonParams(Bitmap,String) here instead and just update the current one. https://codereview.chromium.org/1291083004/diff/60001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/customtabs/CustomTabIntentDataProvider.java (right): https://codereview.chromium.org/1291083004/diff/60001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/customtabs/CustomTabIntentDataProvider.java:221: public Drawable getActionButtonIcon(Resources res) { have a getter for action button params instead. THen that class should have the related getters for individual items https://codereview.chromium.org/1291083004/diff/60001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/customtabs/CustomTabIntentDataProvider.java:223: if (mShouldTintActionButton) { move this to action button params https://codereview.chromium.org/1291083004/diff/60001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/customtabs/CustomTabIntentDataProvider.java:224: drawable = TintedDrawable.constructTintedDrawable(res, mActionButtonParams.mIcon); move this to ActionButtonParams https://codereview.chromium.org/1291083004/diff/60001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/customtabs/CustomTabsConnection.java (right): https://codereview.chromium.org/1291083004/diff/60001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/customtabs/CustomTabsConnection.java:27: import android.support.customtabs.CustomTabsCallback; why is this one needed? https://codereview.chromium.org/1291083004/diff/60001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/customtabs/CustomTabsConnection.java:339: final boolean[] didSucceed = {true}; use a Callable<Boolean> instead of this.
https://codereview.chromium.org/1291083004/diff/60001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/customtabs/ActionButtonParams.java (right): https://codereview.chromium.org/1291083004/diff/60001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/customtabs/ActionButtonParams.java:61: static Bitmap tryParseBitmapFromBundle(Context context, Bundle bundle) { It seems that you are not using the tryParse* methods outside of a context where you could have used fromBundle(), which already does the checks, and so you can assume that whatever has been returned by fromBundle() is corrent, right? If it's the case, why don't you make these methods private? https://codereview.chromium.org/1291083004/diff/60001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/customtabs/CustomTabActivity.java (right): https://codereview.chromium.org/1291083004/diff/60001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/customtabs/CustomTabActivity.java:224: public boolean updateActionButton(Bitmap bitmap, String description) { Alternatively to yusufo's suggestion, why don't you take an ActionButtonParams here? This way you don't need the public constructor in ActionButtonParams and can keep everything final. Whatever you prefer. https://codereview.chromium.org/1291083004/diff/60001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/customtabs/CustomTabsConnection.java (right): https://codereview.chromium.org/1291083004/diff/60001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/customtabs/CustomTabsConnection.java:333: final Bitmap bitmap = ActionButtonParams.tryParseBitmapFromBundle(mApplication, Can't you do: final ActionButtonParams actionButtonParams = ActionButtonParams.fromBundle(mApplication, actionButtonBundle); if (actionButtonParams == null) return false; and in the Callable: CustomTabActivity.updateActionButton(callback.asBinder(), actionButtonParams); https://codereview.chromium.org/1291083004/diff/60001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/customtabs/CustomTabsConnection.java:339: final boolean[] didSucceed = {true}; On 2015/08/21 00:07:37, Yusuf wrote: > use a Callable<Boolean> instead of this. Better suggestion than mine, indeed. Sorry about that. :-)
ptal. I followed yusufo@'s suggestion and made ActionButtonParams mutable, which made the implementation cleaner. https://codereview.chromium.org/1291083004/diff/60001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/customtabs/ActionButtonParams.java (right): https://codereview.chromium.org/1291083004/diff/60001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/customtabs/ActionButtonParams.java:22: final Bitmap mIcon; On 2015/08/21 00:07:36, Yusuf wrote: > see below. I think we should use a single static way of constructing this and > also make this not final. Sp lets add getters for mIcon and mDescription. Done. https://codereview.chromium.org/1291083004/diff/60001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/customtabs/ActionButtonParams.java:26: public ActionButtonParams(@NonNull Bitmap icon, @NonNull String description, On 2015/08/21 00:07:36, Yusuf wrote: > why not have a class method with update(Bitmap, String) for this? The static > call below makes it look like that is the only constructor. Then we have this, > which I think is a bit confusing > . Good idea. I was constrained by keeping them final. Done. https://codereview.chromium.org/1291083004/diff/60001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/customtabs/ActionButtonParams.java:35: * @param bundle The action button button specified by On 2015/08/21 00:07:36, Yusuf wrote: > action button bundle Done. https://codereview.chromium.org/1291083004/diff/60001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/customtabs/ActionButtonParams.java:61: static Bitmap tryParseBitmapFromBundle(Context context, Bundle bundle) { On 2015/08/21 11:31:14, Benoit L wrote: > It seems that you are not using the tryParse* methods outside of a context where > you could have used fromBundle(), which already does the checks, and so you can > assume that whatever has been returned by fromBundle() is corrent, right? > > If it's the case, why don't you make these methods private? I wish I could do what you suggested (had the same initial idea myself). Yet in CustomTabConnection I can't construct a valid params because there will be no PendingIntent. Also I don't want to just write a isValid() method that creates a bitmap for verification purpose and discard it later (not memory friendly)... After some thinking I decided to take Yusuf's suggestion, which is to have an update() method in this class. https://codereview.chromium.org/1291083004/diff/60001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/customtabs/CustomTabActivity.java (right): https://codereview.chromium.org/1291083004/diff/60001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/customtabs/CustomTabActivity.java:224: public boolean updateActionButton(Bitmap bitmap, String description) { On 2015/08/21 11:31:15, Benoit L wrote: > Alternatively to yusufo's suggestion, why don't you take an ActionButtonParams > here? This way you don't need the public constructor in ActionButtonParams and > can keep everything final. > > Whatever you prefer. Acknowledged. I grabbed the old params and update it with new bitmap and description. https://codereview.chromium.org/1291083004/diff/60001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/customtabs/CustomTabActivity.java:229: mIntentDataProvider.setActionButtonParams(params); On 2015/08/21 00:07:36, Yusuf wrote: > add a updateActionButtonParams(Bitmap,String) here instead and just update the > current one. Done. Called the member function update() directly. https://codereview.chromium.org/1291083004/diff/60001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/customtabs/CustomTabIntentDataProvider.java (right): https://codereview.chromium.org/1291083004/diff/60001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/customtabs/CustomTabIntentDataProvider.java:221: public Drawable getActionButtonIcon(Resources res) { On 2015/08/21 00:07:36, Yusuf wrote: > have a getter for action button params instead. THen that class should have the > related getters for individual items Done. https://codereview.chromium.org/1291083004/diff/60001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/customtabs/CustomTabIntentDataProvider.java:223: if (mShouldTintActionButton) { On 2015/08/21 00:07:36, Yusuf wrote: > move this to action button params Done. https://codereview.chromium.org/1291083004/diff/60001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/customtabs/CustomTabIntentDataProvider.java:224: drawable = TintedDrawable.constructTintedDrawable(res, mActionButtonParams.mIcon); On 2015/08/21 00:07:36, Yusuf wrote: > move this to ActionButtonParams Done. https://codereview.chromium.org/1291083004/diff/60001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/customtabs/CustomTabsConnection.java (right): https://codereview.chromium.org/1291083004/diff/60001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/customtabs/CustomTabsConnection.java:27: import android.support.customtabs.CustomTabsCallback; On 2015/08/21 00:07:37, Yusuf wrote: > why is this one needed? Removed. https://codereview.chromium.org/1291083004/diff/60001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/customtabs/CustomTabsConnection.java:333: final Bitmap bitmap = ActionButtonParams.tryParseBitmapFromBundle(mApplication, On 2015/08/21 11:31:15, Benoit L wrote: > Can't you do: > > final ActionButtonParams actionButtonParams = > ActionButtonParams.fromBundle(mApplication, actionButtonBundle); > if (actionButtonParams == null) return false; > > > and in the Callable: > > CustomTabActivity.updateActionButton(callback.asBinder(), actionButtonParams); > The reason that I cannot construct an actionButtonParams is that PendingIntent should not be updated and we don't parse PendingIntent from the bundle. Since we want to reuse the previous PendingIntent, I have to wait till we get to the activity and then construct an ActionButtonParams object. https://codereview.chromium.org/1291083004/diff/60001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/customtabs/CustomTabsConnection.java:339: final boolean[] didSucceed = {true}; On 2015/08/21 00:07:37, Yusuf wrote: > use a Callable<Boolean> instead of this. Done.
LGTM, thank you. https://codereview.chromium.org/1291083004/diff/60001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/customtabs/ActionButtonParams.java (right): https://codereview.chromium.org/1291083004/diff/60001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/customtabs/ActionButtonParams.java:61: static Bitmap tryParseBitmapFromBundle(Context context, Bundle bundle) { On 2015/08/21 17:01:23, Ian Wen wrote: > On 2015/08/21 11:31:14, Benoit L wrote: > > It seems that you are not using the tryParse* methods outside of a context > where > > you could have used fromBundle(), which already does the checks, and so you > can > > assume that whatever has been returned by fromBundle() is corrent, right? > > > > If it's the case, why don't you make these methods private? > > I wish I could do what you suggested (had the same initial idea myself). > > Yet in CustomTabConnection I can't construct a valid params because there will > be no PendingIntent. Also I don't want to just write a isValid() method that > creates a bitmap for verification purpose and discard it later (not memory > friendly)... After some thinking I decided to take Yusuf's suggestion, which is > to have an update() method in this class. Alright, that's fine. Thank you for explaining the reasoning.
yusufo@, could you take a final look at the CL to see whether there are any things to be adjusted? I know one thing that needs to change is the signature of the aidl function needs to be "udpateVisual()". Anything else?
yusufo@, could you take a final look at the CL to see whether there are any things to be adjusted? I know one thing that needs to change is the signature of the aidl function needs to be "udpateVisual()". Anything else?
On 2015/09/08 17:01:01, Ian Wen wrote: > yusufo@, could you take a final look at the CL to see whether there are any > things to be adjusted? > > I know one thing that needs to change is the signature of the aidl function > needs to be "udpateVisual()". Anything else? Nothing for now. I am waiting for the support lib review to give the l-g-t-m on this
FYI, We can land this now after updating the signatures.
ianwen@chromium.org changed reviewers: + mariakhomenko@chromium.org
ianwen@chromium.org changed reviewers: + mariakhomenko@chromium.org
Maria, could you ptal the change in IntentUtils?
Maria, could you ptal the change in IntentUtils?
The CQ bit was checked by ianwen@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1291083004/100001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1291083004/100001
lgtm lgtm for IntentUtils, didn't review the rest https://codereview.chromium.org/1291083004/diff/100001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/util/IntentUtils.java (right): https://codereview.chromium.org/1291083004/diff/100001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/util/IntentUtils.java:28: private static final String TAG = "cr.IntentUtils"; I think there's a new rule that says to use underscore instead of a dot for separation -- there was an email to clank-team@
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: android_arm64_dbg_recipe on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/android_arm64_d...) android_compile_dbg on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/android_compile...)
The CQ bit was checked by ianwen@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1291083004/120001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1291083004/120001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by ianwen@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from lizeb@chromium.org, mariakhomenko@chromium.org Link to the patchset: https://codereview.chromium.org/1291083004/#ps140001 (title: "cr.->cr_")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1291083004/140001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1291083004/140001
Message was sent while issue was closed.
Committed patchset #8 (id:140001)
Message was sent while issue was closed.
Patchset 8 (id:??) landed as https://crrev.com/773b9523d10279efa4a83642fbc82ae4eebe1219 Cr-Commit-Position: refs/heads/master@{#350047} |
