Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2963903002:
Send a callback to notify which default menu item is chosen in Browser Actions (Closed)
DescriptionSend a callback to notify which default menu item is chosen in Browser Actions
This CL parses a new PendingIntent from BrowserActionsIntent as a
callback if any default menu item is chosen in Browser Actions. Once a
default menu item is chosen, Chrome adds its menuId to the PendigIntent
and then sends it out to notify the 3rd party app.
BUG=737814
Review-Url: https://codereview.chromium.org/2963903002
Cr-Commit-Position: refs/heads/master@{#490143}
Committed: https://chromium.googlesource.com/chromium/src/+/9a650335461f853e79328d0d173361f3731b7547
Patch Set 1 #Patch Set 2 : Update extra name. #Patch Set 3 : Send chosen id through the data of callback intent. #
Total comments: 17
Patch Set 4 : Update based on Yusuf and Ted's comments. #Patch Set 5 : Sync with client changes. #
Total comments: 4
Patch Set 6 : Update based on Ted's comments. #
Total comments: 2
Patch Set 7 : Update based on Yusuf's comments. #Patch Set 8 : Rebase. #Patch Set 9 : gclient sync #
Dependent Patchsets: Messages
Total messages: 26 (12 generated)
ltian@chromium.org changed reviewers: + tedchoc@chromium.org, yusufo@chromium.org
Can you take a look of the changes in this CL? Thanks!
https://codereview.chromium.org/2963903002/diff/40001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/browseractions/BrowserActionActivity.java (right): https://codereview.chromium.org/2963903002/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/browseractions/BrowserActionActivity.java:38: private PendingIntent mDefaultActionIntent; I would call this like something mOnDefaultActionSelectedCallback. We want to distinguish this vs a pendingintent that should be triggered to take the default action. https://codereview.chromium.org/2963903002/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/browseractions/BrowserActionActivity.java:138: public String getCreatorPackageName() { Remove this. I left a comments in this CL: https://codereview.chromium.org/2903563003 to remove it, but it didn't happen, so please do that now. https://codereview.chromium.org/2963903002/diff/40001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/browseractions/BrowserActionsContextMenuHelper.java (right): https://codereview.chromium.org/2963903002/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/browseractions/BrowserActionsContextMenuHelper.java:44: private static final String TAG = "BrowserActions"; cr_BrowserActions https://codereview.chromium.org/2963903002/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/browseractions/BrowserActionsContextMenuHelper.java:65: private final PendingIntent mDefaultAction; same naming comment as the other file. https://codereview.chromium.org/2963903002/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/browseractions/BrowserActionsContextMenuHelper.java:164: if (mDefaultAction != null) { I would do: if (mDefaultAction == null) return; Then you can reduce the indent of the rest.
https://codereview.chromium.org/2963903002/diff/40001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/browseractions/BrowserActionActivity.java (right): https://codereview.chromium.org/2963903002/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/browseractions/BrowserActionActivity.java:38: private PendingIntent mDefaultActionIntent; On 2017/06/30 17:48:33, Ted C wrote: > I would call this like something mOnDefaultActionSelectedCallback. We want to > distinguish this vs a pendingintent that should be triggered to take the default > action. I would probably not use the word "default" for this. It is fairly easy to misinterpret what that means in this context. mOnActionSelectedCallback is informative enough, tbh, maybe mOnBrowserActionSelectedCallback if we want to get across that they are not client customized? https://codereview.chromium.org/2963903002/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/browseractions/BrowserActionActivity.java:60: intent, BrowserActionsIntent.EXTRA_DEFULT_ACTION); DEFAULT https://codereview.chromium.org/2963903002/diff/40001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/browseractions/BrowserActionsContextMenuHelper.java (right): https://codereview.chromium.org/2963903002/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/browseractions/BrowserActionsContextMenuHelper.java:157: } else if (mCustomItemActionMap.indexOfKey(itemId) >= 0) { maybe add an early return for this and just call the notify once?
https://codereview.chromium.org/2963903002/diff/40001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/browseractions/BrowserActionActivity.java (right): https://codereview.chromium.org/2963903002/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/browseractions/BrowserActionActivity.java:38: private PendingIntent mDefaultActionIntent; On 2017/06/30 17:55:56, Yusuf wrote: > On 2017/06/30 17:48:33, Ted C wrote: > > I would call this like something mOnDefaultActionSelectedCallback. We want to > > distinguish this vs a pendingintent that should be triggered to take the > default > > action. > > I would probably not use the word "default" for this. It is fairly easy to > misinterpret what that means in this context. mOnActionSelectedCallback is > informative enough, tbh, maybe mOnBrowserActionSelectedCallback if we want to > get across that they are not client customized? Done. https://codereview.chromium.org/2963903002/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/browseractions/BrowserActionActivity.java:38: private PendingIntent mDefaultActionIntent; On 2017/06/30 17:48:33, Ted C wrote: > I would call this like something mOnDefaultActionSelectedCallback. We want to > distinguish this vs a pendingintent that should be triggered to take the default > action. Done. https://codereview.chromium.org/2963903002/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/browseractions/BrowserActionActivity.java:60: intent, BrowserActionsIntent.EXTRA_DEFULT_ACTION); On 2017/06/30 17:55:56, Yusuf wrote: > DEFAULT Done. https://codereview.chromium.org/2963903002/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/browseractions/BrowserActionActivity.java:138: public String getCreatorPackageName() { On 2017/06/30 17:48:34, Ted C wrote: > Remove this. I left a comments in this CL: > https://codereview.chromium.org/2903563003 to remove it, but it didn't happen, > so please do that now. Ops, there must be I thought I deleted it and marked it as done but somehow I forgot. Sorry for that! https://codereview.chromium.org/2963903002/diff/40001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/browseractions/BrowserActionsContextMenuHelper.java (right): https://codereview.chromium.org/2963903002/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/browseractions/BrowserActionsContextMenuHelper.java:44: private static final String TAG = "BrowserActions"; On 2017/06/30 17:48:34, Ted C wrote: > cr_BrowserActions I thought "cr_" will be automatically added by org.chromium.base.Log, isn't it? https://codereview.chromium.org/2963903002/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/browseractions/BrowserActionsContextMenuHelper.java:65: private final PendingIntent mDefaultAction; On 2017/06/30 17:48:34, Ted C wrote: > same naming comment as the other file. Done. https://codereview.chromium.org/2963903002/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/browseractions/BrowserActionsContextMenuHelper.java:157: } else if (mCustomItemActionMap.indexOfKey(itemId) >= 0) { On 2017/06/30 17:55:56, Yusuf wrote: > maybe add an early return for this and just call the notify once? Every case calls the notify with different params (menuId), even we put the notify at the end, we still need each case has a line to set the param, that's why I don't make the call at the end. https://codereview.chromium.org/2963903002/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/browseractions/BrowserActionsContextMenuHelper.java:164: if (mDefaultAction != null) { On 2017/06/30 17:48:34, Ted C wrote: > I would do: > > if (mDefaultAction == null) return; > > Then you can reduce the indent of the rest. Done.
lgtm w/ nits https://codereview.chromium.org/2963903002/diff/40001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/browseractions/BrowserActionsContextMenuHelper.java (right): https://codereview.chromium.org/2963903002/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/browseractions/BrowserActionsContextMenuHelper.java:44: private static final String TAG = "BrowserActions"; On 2017/06/30 22:14:05, ltian wrote: > On 2017/06/30 17:48:34, Ted C wrote: > > cr_BrowserActions > > I thought "cr_" will be automatically added by org.chromium.base.Log, isn't it? It will, but it also causes a separate string to be created. I think we should inline cr_ here and elsewhere. https://codereview.chromium.org/2963903002/diff/80001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/browseractions/BrowserActionsContextMenuHelper.java (right): https://codereview.chromium.org/2963903002/diff/80001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/browseractions/BrowserActionsContextMenuHelper.java:165: Intent addedIntent = new Intent(); I would call this additionalData instead of addedIntent. https://codereview.chromium.org/2963903002/diff/80001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/browseractions/BrowserActionsContextMenuHelper.java:166: addedIntent.setData(Uri.parse(String.valueOf(menuId))); I guess I'd expect to pass this as an extra instead of data, but I'm not sure. Yusuf should have a better idea of whether this is the standard practice.
https://codereview.chromium.org/2963903002/diff/80001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/browseractions/BrowserActionsContextMenuHelper.java (right): https://codereview.chromium.org/2963903002/diff/80001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/browseractions/BrowserActionsContextMenuHelper.java:165: Intent addedIntent = new Intent(); On 2017/07/17 17:57:00, Ted C wrote: > I would call this additionalData instead of addedIntent. Done. https://codereview.chromium.org/2963903002/diff/80001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/browseractions/BrowserActionsContextMenuHelper.java:166: addedIntent.setData(Uri.parse(String.valueOf(menuId))); On 2017/07/17 17:57:00, Ted C wrote: > I guess I'd expect to pass this as an extra instead of data, but I'm not sure. > Yusuf should have a better idea of whether this is the standard practice. Yes, Yusuf and Marcel suggest to pass this info through data instead of an extra.
lgtm https://codereview.chromium.org/2963903002/diff/100001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/browseractions/BrowserActionsContextMenuHelper.java (right): https://codereview.chromium.org/2963903002/diff/100001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/browseractions/BrowserActionsContextMenuHelper.java:163: private void notifyDefaultSelection(@BrowserActionsItemId int menuId) { notifyBrowserActionSelected for having shared names.
https://codereview.chromium.org/2963903002/diff/100001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/browseractions/BrowserActionsContextMenuHelper.java (right): https://codereview.chromium.org/2963903002/diff/100001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/browseractions/BrowserActionsContextMenuHelper.java:163: private void notifyDefaultSelection(@BrowserActionsItemId int menuId) { On 2017/07/20 23:53:52, Yusuf wrote: > notifyBrowserActionSelected for having shared names. Done.
The CQ bit was checked by ltian@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from tedchoc@chromium.org, yusufo@chromium.org Link to the patchset: https://codereview.chromium.org/2963903002/#ps120001 (title: "Update based on Yusuf's comments.")
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 commit-bot@chromium.org
Try jobs failed on following builders: android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)
The CQ bit was checked by ltian@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from tedchoc@chromium.org, yusufo@chromium.org Link to the patchset: https://codereview.chromium.org/2963903002/#ps160001 (title: "gclient sync")
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 commit-bot@chromium.org
CQ cannot get SignCLA result. Please try later.
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...
CQ is committing da patch.
Bot data: {"patchset_id": 160001, "attempt_start_ts": 1501188112103300,
"parent_rev": "d351d8aec1d311b4f04b3cc32fbfa33a739ff859", "commit_rev":
"ab7dc5d5dad6e41b2d5f178de7b027646b02fbec"}
CQ is committing da patch.
Bot data: {"patchset_id": 160001, "attempt_start_ts": 1501188112103300,
"parent_rev": "4a91b02366609335d90356ae1d83a3e32a751c7d", "commit_rev":
"338225fd83e5bdf7dfb408cd0a397c21525c55d1"}
CQ is committing da patch.
Bot data: {"patchset_id": 160001, "attempt_start_ts": 1501188112103300,
"parent_rev": "dc2f4b07c7440115d330ddf28f6595a9cc9a4fc8", "commit_rev":
"9a650335461f853e79328d0d173361f3731b7547"}
Message was sent while issue was closed.
Description was changed from ========== Send a callback to notify which default menu item is chosen in Browser Actions This CL parses a new PendingIntent from BrowserActionsIntent as a callback if any default menu item is chosen in Browser Actions. Once a default menu item is chosen, Chrome adds its menuId to the PendigIntent and then sends it out to notify the 3rd party app. BUG=737814 ========== to ========== Send a callback to notify which default menu item is chosen in Browser Actions This CL parses a new PendingIntent from BrowserActionsIntent as a callback if any default menu item is chosen in Browser Actions. Once a default menu item is chosen, Chrome adds its menuId to the PendigIntent and then sends it out to notify the 3rd party app. BUG=737814 Review-Url: https://codereview.chromium.org/2963903002 Cr-Commit-Position: refs/heads/master@{#490143} Committed: https://chromium.googlesource.com/chromium/src/+/9a650335461f853e79328d0d1733... ==========
Message was sent while issue was closed.
Committed patchset #9 (id:160001) as https://chromium.googlesource.com/chromium/src/+/9a650335461f853e79328d0d1733... |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
