Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1328663003:
[Custom tab]Fix a bug that clicking media notification opens blank tab (Closed)
|
Created:
5 years, 3 months ago by Ian Wen Modified:
5 years, 3 months ago CC:
chromium-reviews, feature-media-reviews_chromium.org, mcasas+watch_chromium.org, posciak+watch_chromium.org, wjia+watch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
Description[Custom tab]Fix a bug that clicking media notification opens blank tab
This CL lets Tab#createBringTabToFrontIntent() return null if the given
tab id belongs to a custom tab. A "clickable" notion is introduced here
to describe notifications that do not carry a content intent.
BUG=524855
Committed: https://crrev.com/85d35ae8d57db58d38d31dce1f1b8a9d3b3e005e
Cr-Commit-Position: refs/heads/master@{#347768}
Patch Set 1 #Patch Set 2 : self review #
Total comments: 8
Patch Set 3 : respond to comments #
Total comments: 4
Patch Set 4 : Let notifications do nothing on click #
Total comments: 8
Patch Set 5 : respond to dfalcantara's comments #
Total comments: 6
Patch Set 6 : respond to Mounir's comments #
Total comments: 2
Patch Set 7 : comments + rebase #
Total comments: 1
Patch Set 8 : final touch #
Depends on Patchset: Messages
Total messages: 29 (4 generated)
ianwen@chromium.org changed reviewers: + dfalcantara@chromium.org, mlamouri@chromium.org, yusufo@chromium.org
ptal. I will create another patch for Web RTC.
https://codereview.chromium.org/1328663003/diff/20001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/media/ui/MediaNotificationInfo.java (right): https://codereview.chromium.org/1328663003/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/media/ui/MediaNotificationInfo.java:44: * Whether the media belongs to a custom tab. media notification https://codereview.chromium.org/1328663003/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/media/ui/MediaNotificationInfo.java:46: public final boolean isCustomTab; isForCustomTab? https://codereview.chromium.org/1328663003/diff/20001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/media/ui/NotificationMediaPlaybackControls.java (right): https://codereview.chromium.org/1328663003/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/media/ui/NotificationMediaPlaybackControls.java:348: * @return An {@link PendingIntent} to bring chrome back to forground. If the tab id belongs to nit: typos and incorrect description /** * @return Generally returns {@link PendingIntent} to bring Chrome back to the * foreground. If the Tab ID belongs to a Custom Tab, however, the * PendingIntent stops media playback. */ https://codereview.chromium.org/1328663003/diff/20001/chrome/android/java/str... File chrome/android/java/strings/android_chrome_strings.grd (right): https://codereview.chromium.org/1328663003/diff/20001/chrome/android/java/str... chrome/android/java/strings/android_chrome_strings.grd:2150: Touch to stop <ph name="URL_OF_THE_CURRENT_TAB">%1$s<ex>https://apprtc.appspot.com</ex></ph> You're not stopping a website. "Touch to stop playback on <site>"?
All done. Thanks for the quick review! https://codereview.chromium.org/1328663003/diff/20001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/media/ui/MediaNotificationInfo.java (right): https://codereview.chromium.org/1328663003/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/media/ui/MediaNotificationInfo.java:44: * Whether the media belongs to a custom tab. On 2015/09/02 21:55:17, dfalcantara wrote: > media notification Done. https://codereview.chromium.org/1328663003/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/media/ui/MediaNotificationInfo.java:46: public final boolean isCustomTab; On 2015/09/02 21:55:17, dfalcantara wrote: > isForCustomTab? Done. https://codereview.chromium.org/1328663003/diff/20001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/media/ui/NotificationMediaPlaybackControls.java (right): https://codereview.chromium.org/1328663003/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/media/ui/NotificationMediaPlaybackControls.java:348: * @return An {@link PendingIntent} to bring chrome back to forground. If the tab id belongs to On 2015/09/02 21:55:17, dfalcantara wrote: > nit: typos and incorrect description > > /** > * @return Generally returns {@link PendingIntent} to bring Chrome back to the > * foreground. If the Tab ID belongs to a Custom Tab, however, the > * PendingIntent stops media playback. > */ Done. https://codereview.chromium.org/1328663003/diff/20001/chrome/android/java/str... File chrome/android/java/strings/android_chrome_strings.grd (right): https://codereview.chromium.org/1328663003/diff/20001/chrome/android/java/str... chrome/android/java/strings/android_chrome_strings.grd:2150: Touch to stop <ph name="URL_OF_THE_CURRENT_TAB">%1$s<ex>https://apprtc.appspot.com</ex></ph> On 2015/09/02 21:55:17, dfalcantara wrote: > You're not stopping a website. "Touch to stop playback on <site>"? Done.
dfalcantara@chromium.org changed reviewers: + avayvod@chromium.org
Re-nitting myself. Going to defer to Mounir or avayvod@ to give you the l-g-t-m. https://codereview.chromium.org/1328663003/diff/40001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/media/ui/MediaNotificationInfo.java (right): https://codereview.chromium.org/1328663003/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/media/ui/MediaNotificationInfo.java:44: * Whether the media notification belongs to a custom tab. nit: {@link CustomTab}, now that I think about it. That's the name of your class, right? https://codereview.chromium.org/1328663003/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/media/ui/MediaNotificationInfo.java:62: boolean isCustomTab, nit: isForCustomTab https://codereview.chromium.org/1328663003/diff/40001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/media/ui/NotificationMediaPlaybackControls.java (right): https://codereview.chromium.org/1328663003/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/media/ui/NotificationMediaPlaybackControls.java:338: boolean isCustomTab = mMediaNotificationInfo.isForCustomTab; nit: isForCustomTab https://codereview.chromium.org/1328663003/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/media/ui/NotificationMediaPlaybackControls.java:349: * tab id belongs to a Custom Tab, however, the {@link PendingIntent} stops media nit: id -> ID, Custom Tab -> {@link CustomTab}
This doesn't seem to be what is suggested in the bug as I understand? We do have a way to stop playback, maybe we should just remove the intent and string for Custom Tabs completely?
The decision made in last custom tab's meeting was that we first try to bring the tab back; if we cannot, we should stop the playback and video recording. Killing the tab is not friendly to the client and will make the notification useless.
On 2015/09/03 at 01:25:30, ianwen wrote: > The decision made in last custom tab's meeting was that we first try to bring the tab back; if we cannot, we should stop the playback and video recording. Killing the tab is not friendly to the client and will make the notification useless. With the current implementation, what happens if the user clicks on the notification?
With the current implementation, a blank chrome ntp will be shown when the user clicks the notification. Why? Because the given tabid belongs to a custom tab and ChromeLauncherActivity is confused, so it treats the intent as a main intent.
On 2015/09/03 at 17:00:54, ianwen wrote: > With the current implementation, a blank chrome ntp will be shown when the user clicks the notification. Why? Because the given tabid belongs to a custom tab and ChromeLauncherActivity is confused, so it treats the intent as a main intent. Can you override createBringTabToFrontIntent() for CustomTab so it returns null? Then, it might just work (if Android does the right thing...). Otherwise, we could probably have some code checking that the returned intent isn't null and not set it in that case.
On 2015/09/03 at 17:54:43, Mounir Lamouri wrote: > On 2015/09/03 at 17:00:54, ianwen wrote: > > With the current implementation, a blank chrome ntp will be shown when the user clicks the notification. Why? Because the given tabid belongs to a custom tab and ChromeLauncherActivity is confused, so it treats the intent as a main intent. > > Can you override createBringTabToFrontIntent() for CustomTab so it returns null? Then, it might just work (if Android does the right thing...). Otherwise, we could probably have some code checking that the returned intent isn't null and not set it in that case. Alternatively, if possible, we can return a no-op intent instead of null so we prevent callers for having to worry about createBringTabToFrontIntent to be null. I'm fine either way.
On 2015/09/03 18:01:54, Mounir Lamouri wrote: > On 2015/09/03 at 17:54:43, Mounir Lamouri wrote: > > On 2015/09/03 at 17:00:54, ianwen wrote: > > > With the current implementation, a blank chrome ntp will be shown when the > user clicks the notification. Why? Because the given tabid belongs to a custom > tab and ChromeLauncherActivity is confused, so it treats the intent as a main > intent. > > > > Can you override createBringTabToFrontIntent() for CustomTab so it returns > null? Then, it might just work (if Android does the right thing...). Otherwise, > we could probably have some code checking that the returned intent isn't null > and not set it in that case. > > Alternatively, if possible, we can return a no-op intent instead of null so we > prevent callers for having to worry about createBringTabToFrontIntent to be > null. I'm fine either way. Frankly, I'd rather have it return null and have callers check. Firing an Intent to Android when we have a clear signal that nothing should be done is a waste.
On 2015/09/03 at 18:20:09, dfalcantara wrote: > On 2015/09/03 18:01:54, Mounir Lamouri wrote: > > On 2015/09/03 at 17:54:43, Mounir Lamouri wrote: > > > On 2015/09/03 at 17:00:54, ianwen wrote: > > > > With the current implementation, a blank chrome ntp will be shown when the > > user clicks the notification. Why? Because the given tabid belongs to a custom > > tab and ChromeLauncherActivity is confused, so it treats the intent as a main > > intent. > > > > > > Can you override createBringTabToFrontIntent() for CustomTab so it returns > > null? Then, it might just work (if Android does the right thing...). Otherwise, > > we could probably have some code checking that the returned intent isn't null > > and not set it in that case. > > > > Alternatively, if possible, we can return a no-op intent instead of null so we > > prevent callers for having to worry about createBringTabToFrontIntent to be > > null. I'm fine either way. > > Frankly, I'd rather have it return null and have callers check. Firing an Intent > to Android when we have a clear signal that nothing should be done is a waste. As I said, I don't mind either way but I think using that createBringTabToFrontIntent() method is the way to go instead of having all callers who want to bring a tab back to front have to worry about whether it is a CustomTab.
I'm fine with what Mounir proposed (it's actually my original plan). A new patchset is on its way!
ptal the new patchset. :)
https://codereview.chromium.org/1328663003/diff/60001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/media/ui/NotificationMediaPlaybackControls.java (right): https://codereview.chromium.org/1328663003/diff/60001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/media/ui/NotificationMediaPlaybackControls.java:339: * intent. nit :capitalize Intent and URL everywhere. How about "Clickable notifications do something when clicked." because a "content intent" doesn't make sense without looking up PendingIntent's javadoc. https://codereview.chromium.org/1328663003/diff/60001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/media/ui/NotificationMediaPlaybackControls.java:355: * , this method does nothing. dangling comma. Since the comment doesn't say anything about what the function actually does with the content Intent, how about: If possible, prepares the notification so that clicking it brings back its associated Tab to the foreground. You need a better name for the function, too, but I don't have a good one off the top of my head. https://codereview.chromium.org/1328663003/diff/60001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/tab/Tab.java (right): https://codereview.chromium.org/1328663003/diff/60001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/tab/Tab.java:2844: * @return An Intent that tells Chrome to open a Tab with a particular ID. If the ID belongs @return Intent that tells Chrome to bring an Activity for a particular Tab back to the foreground, or null if this isn't possible. https://codereview.chromium.org/1328663003/diff/60001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/tab/Tab.java:2848: // Iterate through all active custom tabs and check whether the given tabId belongs to a {@link CustomTab}.
https://codereview.chromium.org/1328663003/diff/60001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/media/ui/NotificationMediaPlaybackControls.java (right): https://codereview.chromium.org/1328663003/diff/60001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/media/ui/NotificationMediaPlaybackControls.java:339: * intent. On 2015/09/04 00:03:27, dfalcantara wrote: > nit :capitalize Intent and URL everywhere. > > How about "Clickable notifications do something when clicked." because a > "content intent" doesn't make sense without looking up PendingIntent's javadoc. Done. https://codereview.chromium.org/1328663003/diff/60001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/media/ui/NotificationMediaPlaybackControls.java:355: * , this method does nothing. On 2015/09/04 00:03:27, dfalcantara wrote: > dangling comma. > > Since the comment doesn't say anything about what the function actually does > with the content Intent, how about: > If possible, prepares the notification so that clicking it brings back its > associated Tab to the foreground. > > You need a better name for the function, too, but I don't have a good one off > the top of my head. Done. https://codereview.chromium.org/1328663003/diff/60001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/tab/Tab.java (right): https://codereview.chromium.org/1328663003/diff/60001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/tab/Tab.java:2844: * @return An Intent that tells Chrome to open a Tab with a particular ID. If the ID belongs On 2015/09/04 00:03:27, dfalcantara wrote: > @return Intent that tells Chrome to bring an Activity for a particular Tab back > to the foreground, or null if this isn't possible. Done. https://codereview.chromium.org/1328663003/diff/60001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/tab/Tab.java:2848: // Iterate through all active custom tabs and check whether the given tabId belongs to a On 2015/09/04 00:03:27, dfalcantara wrote: > {@link CustomTab}. Done.
https://codereview.chromium.org/1328663003/diff/80001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/media/ui/NotificationMediaPlaybackControls.java (right): https://codereview.chromium.org/1328663003/diff/80001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/media/ui/NotificationMediaPlaybackControls.java:339: private String getStatusText(boolean isClickable) { Hmm, I actually wanted to entirely remove that. Given that it's orthogonal to your change, I sent a CL that does it. Hopefully it will have landed by the time you are back at the office: https://codereview.chromium.org/1326163002 https://codereview.chromium.org/1328663003/diff/80001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/media/ui/NotificationMediaPlaybackControls.java:413: boolean isClickable = prepareClickingIntent(mNotificationBuilder); Instead of that, feel free to remove the method and have the same pattern as the other class: // Add an intent to bring the tab to front when possible. Intent tabIntent = Tab.createBringTabToFrontIntent(tabId); if (tabIntent) { mNotificationBuild.setContentIntent(PendingIntent.getActivity(mContext, mMediaNotificationInfo.tabId, tabIntent, 0); } https://codereview.chromium.org/1328663003/diff/80001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/tab/Tab.java (right): https://codereview.chromium.org/1328663003/diff/80001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/tab/Tab.java:2850: for (WeakReference<Activity> ref : list) { I wonder if it would be worth having this as a Tab method to not have to do crazy hacks like that to know the Tab's implementation. Not needed for that CL but maybe a proof that some design needs to be changed.
Thanks for the comments! https://codereview.chromium.org/1328663003/diff/80001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/media/ui/NotificationMediaPlaybackControls.java (right): https://codereview.chromium.org/1328663003/diff/80001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/media/ui/NotificationMediaPlaybackControls.java:339: private String getStatusText(boolean isClickable) { On 2015/09/04 10:36:44, Mounir Lamouri wrote: > Hmm, I actually wanted to entirely remove that. Given that it's orthogonal to > your change, I sent a CL that does it. Hopefully it will have landed by the time > you are back at the office: > https://codereview.chromium.org/1326163002 Okay glad you made the change. :) https://codereview.chromium.org/1328663003/diff/80001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/media/ui/NotificationMediaPlaybackControls.java:413: boolean isClickable = prepareClickingIntent(mNotificationBuilder); On 2015/09/04 10:36:44, Mounir Lamouri wrote: > Instead of that, feel free to remove the method and have the same pattern as the > other class: > > // Add an intent to bring the tab to front when possible. > Intent tabIntent = Tab.createBringTabToFrontIntent(tabId); > if (tabIntent) { > mNotificationBuild.setContentIntent(PendingIntent.getActivity(mContext, > mMediaNotificationInfo.tabId, tabIntent, 0); > } Done. https://codereview.chromium.org/1328663003/diff/80001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/tab/Tab.java (right): https://codereview.chromium.org/1328663003/diff/80001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/tab/Tab.java:2850: for (WeakReference<Activity> ref : list) { On 2015/09/04 10:36:44, Mounir Lamouri wrote: > I wonder if it would be worth having this as a Tab method to not have to do > crazy hacks like that to know the Tab's implementation. Not needed for that CL > but maybe a proof that some design needs to be changed. Agreed that letting Tab.java know about CustomTabActivity is not very elegant. I wish I could let a subclass override this method. Yet it is static... I once thought creating a TabUtil class, but it seems to be an overkill to create one class and only contain one method. Luckily getRunningActivities() is not a hack, as it is used widely in our codebase. @See ActivityDelegate and DocumentTabModelImpl.
lgtm with comments applied. Thanks for taking the time to do the right thing here! :) https://codereview.chromium.org/1328663003/diff/100001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/media/ui/NotificationMediaPlaybackControls.java (right): https://codereview.chromium.org/1328663003/diff/100001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/media/ui/NotificationMediaPlaybackControls.java:397: Intent chromeIntent = Tab.createBringTabToFrontIntent(tabId); nit: s/chromeIntent/tabIntent/ maybe?
Ping dfalcantara@? :) https://codereview.chromium.org/1328663003/diff/100001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/media/ui/NotificationMediaPlaybackControls.java (right): https://codereview.chromium.org/1328663003/diff/100001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/media/ui/NotificationMediaPlaybackControls.java:397: Intent chromeIntent = Tab.createBringTabToFrontIntent(tabId); On 2015/09/06 12:45:18, Mounir Lamouri wrote: > nit: s/chromeIntent/tabIntent/ maybe? Done.
https://codereview.chromium.org/1328663003/diff/120001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/tab/Tab.java (right): https://codereview.chromium.org/1328663003/diff/120001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/tab/Tab.java:2851: // Iterate through all active custom tabs and check whether the given tabId belongs to a Need to keep using {@link CustomTab} everywhere because "custom tab" can be ambiguous.
(but lgtm otherwise)
The CQ bit was checked by ianwen@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from mlamouri@chromium.org, dfalcantara@chromium.org Link to the patchset: https://codereview.chromium.org/1328663003/#ps140001 (title: "final touch")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1328663003/140001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1328663003/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/85d35ae8d57db58d38d31dce1f1b8a9d3b3e005e Cr-Commit-Position: refs/heads/master@{#347768} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
