Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2876863002:
[Android] Show notification for opening new tab in background of Browser Actions (Closed)
Description[Android] Show notification for opening new tab in background of Browser Actions
This CL implements the logic of display notification when selecting
"Open in new Chrome tab" from the Browser Actions context menu. Clicking
the notification will open Chrome. If the notification is for single
link, Chrome shows the current active tab, otherwise Chrome shows the
tab switcher. Opening Chrome will dismiss the notification.
BUG=719080
Review-Url: https://codereview.chromium.org/2876863002
Cr-Commit-Position: refs/heads/master@{#475789}
Committed: https://chromium.googlesource.com/chromium/src/+/bc1417c0c8f45b206f2835a0f2bdc0fa4dcad8d7
Patch Set 1 #
Total comments: 2
Patch Set 2 : Update based on David's comment and rebase #
Total comments: 10
Patch Set 3 : Update based on David's comments. #
Total comments: 6
Patch Set 4 : Update based on David's comments. #
Total comments: 2
Patch Set 5 : Update based on David's comment. #
Total comments: 6
Patch Set 6 : Update based on Yusuf's comments. #Patch Set 7 : Rebase. #Patch Set 8 : Fix trybots errors. #
Dependent Patchsets: Messages
Total messages: 35 (16 generated)
ltian@chromium.org changed reviewers: + dtrainor@chromium.org
The original CL is a little big so I split it into several small ones. Can you take a look of this CL whether the notification code is reasonable? The CL has updated the code based on your comments in the original CL. Thanks!
initial comment! will do a larger review later today! https://codereview.chromium.org/2876863002/diff/1/chrome/android/java/src/org... File chrome/android/java/src/org/chromium/chrome/browser/notifications/NotificationUmaTracker.java (right): https://codereview.chromium.org/2876863002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/notifications/NotificationUmaTracker.java:32: PHYSICAL_WEB, MEDIA, SITES, SYNC, BROWSER_ACTIONS, SYSTEM_NOTIFICATION_TYPE_BOUNDARY}) Needto update histograms.xml as well. See the comment over "public static final int"'s below.
https://codereview.chromium.org/2876863002/diff/1/chrome/android/java/src/org... File chrome/android/java/src/org/chromium/chrome/browser/notifications/NotificationUmaTracker.java (right): https://codereview.chromium.org/2876863002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/notifications/NotificationUmaTracker.java:32: PHYSICAL_WEB, MEDIA, SITES, SYNC, BROWSER_ACTIONS, SYSTEM_NOTIFICATION_TYPE_BOUNDARY}) On 2017/05/11 20:26:52, David Trainor-ping if over 24h wrote: > Needto update histograms.xml as well. See the comment over "public static final > int"'s below. Done.
https://codereview.chromium.org/2876863002/diff/20001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/ChromeTabbedActivity.java (right): https://codereview.chromium.org/2876863002/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ChromeTabbedActivity.java:571: if (getActivityTab() == null) mLayoutManager.showOverview(false); Can you add {} around this now? https://codereview.chromium.org/2876863002/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ChromeTabbedActivity.java:575: cancelBrowserActionsNotification(); Can this hit a static method on that delegate or something so we can put all BrowserAction notification logic in one place? https://codereview.chromium.org/2876863002/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ChromeTabbedActivity.java:643: private boolean shouldToggleOverview() { Can we just call this setInitialOverviewState() and roll the initial check "getActivityTab() == null" into this too? Then just have this call showOverview() or hideOverview() as necessary. https://codereview.chromium.org/2876863002/diff/20001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/browseractions/BrowserActionsContextMenuItemDelegate.java (right): https://codereview.chromium.org/2876863002/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/browseractions/BrowserActionsContextMenuItemDelegate.java:58: Intent intent = new Intent(mContext, ChromeTabbedActivity.class); ChromeLauncherActivity? https://codereview.chromium.org/2876863002/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/browseractions/BrowserActionsContextMenuItemDelegate.java:60: intent.putExtra(EXTRA_IS_SINGLE_URL, true); Can we just pull isSingleUrl() out to a helper that checks if we have browser actions and the sdk version? Then we can just rely on that to set these and pick the right content title. https://codereview.chromium.org/2876863002/diff/20001/tools/metrics/histogram... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/2876863002/diff/20001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:116422: + <int value="9" label="Browser Actions"/> This will have to move to enums.xml when you rebase.
https://codereview.chromium.org/2876863002/diff/20001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/ChromeTabbedActivity.java (right): https://codereview.chromium.org/2876863002/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ChromeTabbedActivity.java:575: cancelBrowserActionsNotification(); Can this hit a static method on that delegate or something so we can put all BrowserAction notification logic in one place? https://codereview.chromium.org/2876863002/diff/20001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/browseractions/BrowserActionsContextMenuItemDelegate.java (right): https://codereview.chromium.org/2876863002/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/browseractions/BrowserActionsContextMenuItemDelegate.java:58: Intent intent = new Intent(mContext, ChromeTabbedActivity.class); ChromeLauncherActivity? https://codereview.chromium.org/2876863002/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/browseractions/BrowserActionsContextMenuItemDelegate.java:60: intent.putExtra(EXTRA_IS_SINGLE_URL, true); Can we just pull isSingleUrl() out to a helper that checks if we have browser actions and the sdk version? Then we can just rely on that to set these and pick the right content title. https://codereview.chromium.org/2876863002/diff/20001/tools/metrics/histogram... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/2876863002/diff/20001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:116422: + <int value="9" label="Browser Actions"/> This will have to move to enums.xml when you rebase.
https://codereview.chromium.org/2876863002/diff/20001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/ChromeTabbedActivity.java (right): https://codereview.chromium.org/2876863002/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ChromeTabbedActivity.java:571: if (getActivityTab() == null) mLayoutManager.showOverview(false); On 2017/05/17 16:48:01, David Trainor-ping if over 24h wrote: > Can you add {} around this now? Done. https://codereview.chromium.org/2876863002/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ChromeTabbedActivity.java:575: cancelBrowserActionsNotification(); On 2017/05/17 16:48:02, David Trainor-ping if over 24h wrote: > Can this hit a static method on that delegate or something so we can put all > BrowserAction notification logic in one place? Done. https://codereview.chromium.org/2876863002/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ChromeTabbedActivity.java:643: private boolean shouldToggleOverview() { On 2017/05/17 16:48:01, David Trainor-ping if over 24h wrote: > Can we just call this setInitialOverviewState() and roll the initial check > "getActivityTab() == null" into this too? Then just have this call > showOverview() or hideOverview() as necessary. Done. https://codereview.chromium.org/2876863002/diff/20001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/browseractions/BrowserActionsContextMenuItemDelegate.java (right): https://codereview.chromium.org/2876863002/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/browseractions/BrowserActionsContextMenuItemDelegate.java:58: Intent intent = new Intent(mContext, ChromeTabbedActivity.class); On 2017/05/17 16:48:02, David Trainor-ping if over 24h wrote: > ChromeLauncherActivity? Done. https://codereview.chromium.org/2876863002/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/browseractions/BrowserActionsContextMenuItemDelegate.java:60: intent.putExtra(EXTRA_IS_SINGLE_URL, true); On 2017/05/17 16:48:02, David Trainor-ping if over 24h wrote: > Can we just pull isSingleUrl() out to a helper that checks if we have browser > actions and the sdk version? Then we can just rely on that to set these and > pick the right content title. Talk with Sam and he is ok to display notification for multiple links if single link notification is swiped away. So using SharedPreference should be enough for it. I refactor these whole piece of code creating the notification. https://codereview.chromium.org/2876863002/diff/20001/tools/metrics/histogram... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/2876863002/diff/20001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:116422: + <int value="9" label="Browser Actions"/> On 2017/05/17 16:48:02, David Trainor-ping if over 24h wrote: > This will have to move to enums.xml when you rebase. Done.
https://codereview.chromium.org/2876863002/diff/40001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/ChromeTabbedActivity.java (right): https://codereview.chromium.org/2876863002/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ChromeTabbedActivity.java:593: if (setInitialOverviewState()) { set -> Should just set. No need to return anything :). https://codereview.chromium.org/2876863002/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ChromeTabbedActivity.java:1814: mLayoutManager.showOverview(false); Do we need this? If so, should we just return after this line? https://codereview.chromium.org/2876863002/diff/40001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/browseractions/BrowserActionsContextMenuItemDelegate.java (right): https://codereview.chromium.org/2876863002/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/browseractions/BrowserActionsContextMenuItemDelegate.java:78: R.string.browser_actions_multi_links_open_notification_title)); Just pick the title resource based on hasBrowserActionsNotification(). Then call builder.setContentTitle() with that resource id.
https://codereview.chromium.org/2876863002/diff/40001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/ChromeTabbedActivity.java (right): https://codereview.chromium.org/2876863002/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ChromeTabbedActivity.java:593: if (setInitialOverviewState()) { On 2017/05/23 06:16:43, David Trainor-ping if over 24h wrote: > set -> Should just set. No need to return anything :). Done. https://codereview.chromium.org/2876863002/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ChromeTabbedActivity.java:1814: mLayoutManager.showOverview(false); On 2017/05/23 06:16:43, David Trainor-ping if over 24h wrote: > Do we need this? If so, should we just return after this line? Originally, this is a separate check outside the togglerOverview and if true it will directly call showOverview without animation. So to integrate this into togglerOverview, I add this for a separate check. And yes, it should just return, thanks for pointing this out. https://codereview.chromium.org/2876863002/diff/40001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/browseractions/BrowserActionsContextMenuItemDelegate.java (right): https://codereview.chromium.org/2876863002/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/browseractions/BrowserActionsContextMenuItemDelegate.java:78: R.string.browser_actions_multi_links_open_notification_title)); On 2017/05/23 06:16:43, David Trainor-ping if over 24h wrote: > Just pick the title resource based on hasBrowserActionsNotification(). Then > call builder.setContentTitle() with that resource id. Done.
lgtm lgtm % nit https://codereview.chromium.org/2876863002/diff/60001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/ChromeTabbedActivity.java (right): https://codereview.chromium.org/2876863002/diff/60001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ChromeTabbedActivity.java:665: if ((isSingleUrl && isOverviewVisible) || (!isSingleUrl && !isOverviewVisible)) { Is this the same as "if (isSingleUrl == isOverviewVisible)"?
ltian@chromium.org changed reviewers: + yusufo@chromium.org
yusufo@chromium.org: could you take a look of the changes in BrowserActionsContextMenuItemDelegate? Thanks!
https://codereview.chromium.org/2876863002/diff/60001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/ChromeTabbedActivity.java (right): https://codereview.chromium.org/2876863002/diff/60001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ChromeTabbedActivity.java:665: if ((isSingleUrl && isOverviewVisible) || (!isSingleUrl && !isOverviewVisible)) { On 2017/05/25 17:23:28, David Trainor-ping if over 24h wrote: > Is this the same as "if (isSingleUrl == isOverviewVisible)"? Done.
lgtm with nits https://codereview.chromium.org/2876863002/diff/80001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/ChromeTabbedActivity.java (right): https://codereview.chromium.org/2876863002/diff/80001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ChromeTabbedActivity.java:647: private boolean isStartedByBrowserActions() { would look much nicer if this was a static in BrowserActions package somewhere. maybe in the BrowserActionsActivity? or the menu item delegate itself. https://codereview.chromium.org/2876863002/diff/80001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ChromeTabbedActivity.java:665: if (isSingleUrl == isOverviewVisible) { all this logic is specific to BrowserActions and should live there as static. https://codereview.chromium.org/2876863002/diff/80001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/browseractions/BrowserActionsContextMenuItemDelegate.java (right): https://codereview.chromium.org/2876863002/diff/80001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/browseractions/BrowserActionsContextMenuItemDelegate.java:47: * Extra that indicates whether to show a Tab for single url or the tab swictcher for switcher
https://codereview.chromium.org/2876863002/diff/80001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/ChromeTabbedActivity.java (right): https://codereview.chromium.org/2876863002/diff/80001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ChromeTabbedActivity.java:647: private boolean isStartedByBrowserActions() { On 2017/05/30 17:57:15, Yusuf wrote: > would look much nicer if this was a static in BrowserActions package somewhere. > maybe in the BrowserActionsActivity? or the menu item delegate itself. Done. https://codereview.chromium.org/2876863002/diff/80001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ChromeTabbedActivity.java:665: if (isSingleUrl == isOverviewVisible) { On 2017/05/30 17:57:15, Yusuf wrote: > all this logic is specific to BrowserActions and should live there as static. Done. https://codereview.chromium.org/2876863002/diff/80001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/browseractions/BrowserActionsContextMenuItemDelegate.java (right): https://codereview.chromium.org/2876863002/diff/80001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/browseractions/BrowserActionsContextMenuItemDelegate.java:47: * Extra that indicates whether to show a Tab for single url or the tab swictcher for On 2017/05/30 17:57:15, Yusuf wrote: > switcher Done.
The CQ bit was checked by ltian@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from dtrainor@chromium.org, yusufo@chromium.org Link to the patchset: https://codereview.chromium.org/2876863002/#ps100001 (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_compile_dbg on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_comp...) android_cronet on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_cron...) cast_shell_android on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/cast_shell_a...) cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...) chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...) ios-simulator on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
The CQ bit was checked by ltian@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from dtrainor@chromium.org, yusufo@chromium.org Link to the patchset: https://codereview.chromium.org/2876863002/#ps120001 (title: "Rebase.")
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: chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
The CQ bit was checked by ltian@chromium.org to run a CQ dry run
Dry run: 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
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by ltian@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from dtrainor@chromium.org, yusufo@chromium.org Link to the patchset: https://codereview.chromium.org/2876863002/#ps140001 (title: "Fix trybots errors.")
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": 140001, "attempt_start_ts": 1496203428479060,
"parent_rev": "4345e38cd114d781b630039d2cc0f18c7514f626", "commit_rev":
"bc1417c0c8f45b206f2835a0f2bdc0fa4dcad8d7"}
Message was sent while issue was closed.
Description was changed from ========== [Android] Show notification for opening new tab in background of Browser Actions This CL implements the logic of display notification when selecting "Open in new Chrome tab" from the Browser Actions context menu. Clicking the notification will open Chrome. If the notification is for single link, Chrome shows the current active tab, otherwise Chrome shows the tab switcher. Opening Chrome will dismiss the notification. BUG=719080 ========== to ========== [Android] Show notification for opening new tab in background of Browser Actions This CL implements the logic of display notification when selecting "Open in new Chrome tab" from the Browser Actions context menu. Clicking the notification will open Chrome. If the notification is for single link, Chrome shows the current active tab, otherwise Chrome shows the tab switcher. Opening Chrome will dismiss the notification. BUG=719080 Review-Url: https://codereview.chromium.org/2876863002 Cr-Commit-Position: refs/heads/master@{#475789} Committed: https://chromium.googlesource.com/chromium/src/+/bc1417c0c8f45b206f2835a0f2bd... ==========
Message was sent while issue was closed.
Committed patchset #8 (id:140001) as https://chromium.googlesource.com/chromium/src/+/bc1417c0c8f45b206f2835a0f2bd... |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
