Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2714993002:
[Media>UI] Fix a media notification update issue when audio focus changes between tabs (Closed)
|
Created:
3 years, 10 months ago by Zhiqiang Zhang (Slow) Modified:
3 years, 9 months ago Reviewers:
whywhat CC:
agrieve+watch_chromium.org, chromium-reviews, feature-media-reviews_chromium.org Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
Description[Media>UI] Fix a media notification update issue when audio focus changes between tabs
There's a issue which is caused by the asynchronousness of audio focus.
When tab B gains focus from tab A, we will first receive the event that
A becomes playing, and then B becomes paused. We should ignore the
notification updates when the tab id mismatches the current notification
and the tab is paused.
BUG=695967
Review-Url: https://codereview.chromium.org/2714993002
Cr-Commit-Position: refs/heads/master@{#453206}
Committed: https://chromium.googlesource.com/chromium/src/+/1ee77687069628ebd0bba3ff791a5ee6c8d17b41
Patch Set 1 #
Total comments: 8
Patch Set 2 : fixed test and addressed nits #
Messages
Total messages: 21 (13 generated)
zqzhang@chromium.org changed reviewers: + avayvod@chromium.org
PTAL
The CQ bit was checked by zqzhang@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...
https://codereview.chromium.org/2714993002/diff/1/chrome/android/java/src/org... File chrome/android/java/src/org/chromium/chrome/browser/media/ui/MediaNotificationManager.java (right): https://codereview.chromium.org/2714993002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/media/ui/MediaNotificationManager.java:659: return; nit: you should update the comment for the public show() method that it might not do anything if already showing for a different tab and trying to show a paused state? In fact, should MediaSessionTabHelper be aware of this and not update its state if showing the notification failed? Or destroy/cleanup the media session, for example? https://codereview.chromium.org/2714993002/diff/1/chrome/android/javatests/sr... File chrome/android/javatests/src/org/chromium/chrome/browser/media/ui/NotificationTitleUpdatedTest.java (right): https://codereview.chromium.org/2714993002/diff/1/chrome/android/javatests/sr... chrome/android/javatests/src/org/chromium/chrome/browser/media/ui/NotificationTitleUpdatedTest.java:131: * 2. create newTab, set the title of mTab, start the media session of mTab, nit: s/mTab/newTab? https://codereview.chromium.org/2714993002/diff/1/chrome/android/javatests/sr... chrome/android/javatests/src/org/chromium/chrome/browser/media/ui/NotificationTitleUpdatedTest.java:133: * 3. stop the media session of mTab, the notification should not change. nit: the important bit is probably updating the title of mTab after it's suspended?
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
Description was changed from ========== [Media>UI] Fix an media notification update issue when audio focus changes between tabs There's a issue which is caused by the asynchronousness of audio focus. When tab B gains focus from tab A, we will first receive the event that A becomes playing, and then B becomes paused. We should ignore the notification updates when the tab id mismatches the current notification and the tab is paused. BUG=695967 ========== to ========== [Media>UI] Fix a media notification update issue when audio focus changes between tabs There's a issue which is caused by the asynchronousness of audio focus. When tab B gains focus from tab A, we will first receive the event that A becomes playing, and then B becomes paused. We should ignore the notification updates when the tab id mismatches the current notification and the tab is paused. BUG=695967 ==========
The CQ bit was checked by zqzhang@chromium.org to run a CQ dry run
PTAL https://codereview.chromium.org/2714993002/diff/1/chrome/android/java/src/org... File chrome/android/java/src/org/chromium/chrome/browser/media/ui/MediaNotificationManager.java (right): https://codereview.chromium.org/2714993002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/media/ui/MediaNotificationManager.java:659: return; On 2017/02/24 at 21:13:09, whywhat wrote: > nit: you should update the comment for the public show() method that it might not do anything if already showing for a different tab and trying to show a paused state? Done. > In fact, should MediaSessionTabHelper be aware of this and not update its state if showing the notification failed? Or destroy/cleanup the media session, for example? I guess not. That's more like AudioFocus and should be handled by AudioFocusManager if we want to. IMHO, MSTH should be per tab and does not need to care about other tabs. https://codereview.chromium.org/2714993002/diff/1/chrome/android/javatests/sr... File chrome/android/javatests/src/org/chromium/chrome/browser/media/ui/NotificationTitleUpdatedTest.java (right): https://codereview.chromium.org/2714993002/diff/1/chrome/android/javatests/sr... chrome/android/javatests/src/org/chromium/chrome/browser/media/ui/NotificationTitleUpdatedTest.java:131: * 2. create newTab, set the title of mTab, start the media session of mTab, On 2017/02/24 at 21:13:09, whywhat wrote: > nit: s/mTab/newTab? Done. https://codereview.chromium.org/2714993002/diff/1/chrome/android/javatests/sr... chrome/android/javatests/src/org/chromium/chrome/browser/media/ui/NotificationTitleUpdatedTest.java:133: * 3. stop the media session of mTab, the notification should not change. On 2017/02/24 at 21:13:09, whywhat wrote: > nit: the important bit is probably updating the title of mTab after it's suspended? Done. Updated the comments.
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.
lgtm pending a satisfactory answer to my question. Satisfactory could be one of the two: - no, it doesn't matter what MSTH does when the notification is not shown - yes, and it's fixed now (although this might require extra review) https://codereview.chromium.org/2714993002/diff/1/chrome/android/java/src/org... File chrome/android/java/src/org/chromium/chrome/browser/media/ui/MediaNotificationManager.java (right): https://codereview.chromium.org/2714993002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/media/ui/MediaNotificationManager.java:659: return; On 2017/02/25 at 15:09:25, Zhiqiang Zhang wrote: > On 2017/02/24 at 21:13:09, whywhat wrote: > > nit: you should update the comment for the public show() method that it might not do anything if already showing for a different tab and trying to show a paused state? > > Done. > > > In fact, should MediaSessionTabHelper be aware of this and not update its state if showing the notification failed? Or destroy/cleanup the media session, for example? > > I guess not. That's more like AudioFocus and should be handled by AudioFocusManager if we want to. IMHO, MSTH should be per tab and does not need to care about other tabs. My concern is with the mediaSessionStateChanged in MSTH - it will update mCurrentMediaImage and mCurrentMetadata and will also set the volume control stream of the activity to music - will that work as expected when the notification is not shown? Any undesired effects when the user switches back to that tab?
https://codereview.chromium.org/2714993002/diff/1/chrome/android/java/src/org... File chrome/android/java/src/org/chromium/chrome/browser/media/ui/MediaNotificationManager.java (right): https://codereview.chromium.org/2714993002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/media/ui/MediaNotificationManager.java:659: return; On 2017/02/26 at 22:13:25, whywhat wrote: > On 2017/02/25 at 15:09:25, Zhiqiang Zhang wrote: > > On 2017/02/24 at 21:13:09, whywhat wrote: > > > nit: you should update the comment for the public show() method that it might not do anything if already showing for a different tab and trying to show a paused state? > > > > Done. > > > > > In fact, should MediaSessionTabHelper be aware of this and not update its state if showing the notification failed? Or destroy/cleanup the media session, for example? > > > > I guess not. That's more like AudioFocus and should be handled by AudioFocusManager if we want to. IMHO, MSTH should be per tab and does not need to care about other tabs. > > My concern is with the mediaSessionStateChanged in MSTH - it will update mCurrentMediaImage and mCurrentMetadata and will also set the volume control stream of the activity to music - will that work as expected when the notification is not shown? Any undesired effects when the user switches back to that tab? For mCurrentMediaImage and mCurrentMetadata, they are kept per MSTH so they should be fine. We always route the MediaSession for the tab having audio focus, so we just need to keep the notification always show for the focused tab. As for setting the volume control stream, currently we set it per tab, i.e. to STREAM_MUSIC when the tab starts playback and to STREAM_DEFAULT when the tab stops playback. There's definitely a race condition and could be problematic. I think it should be moved to some centralized place such as AudioFocusManager. However I think it should be orthogonal to this CL.
On 2017/02/26 at 22:13:26, avayvod wrote: > lgtm pending a satisfactory answer to my question. Satisfactory could be one of the two: > - no, it doesn't matter what MSTH does when the notification is not shown > - yes, and it's fixed now (although this might require extra review) > Sorry, the short answer is no.
The CQ bit was checked by zqzhang@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": 20001, "attempt_start_ts": 1488203420486320,
"parent_rev": "f823ed66c7c348e46f84b95e6c550fc6e827f14e", "commit_rev":
"1ee77687069628ebd0bba3ff791a5ee6c8d17b41"}
Message was sent while issue was closed.
Description was changed from ========== [Media>UI] Fix a media notification update issue when audio focus changes between tabs There's a issue which is caused by the asynchronousness of audio focus. When tab B gains focus from tab A, we will first receive the event that A becomes playing, and then B becomes paused. We should ignore the notification updates when the tab id mismatches the current notification and the tab is paused. BUG=695967 ========== to ========== [Media>UI] Fix a media notification update issue when audio focus changes between tabs There's a issue which is caused by the asynchronousness of audio focus. When tab B gains focus from tab A, we will first receive the event that A becomes playing, and then B becomes paused. We should ignore the notification updates when the tab id mismatches the current notification and the tab is paused. BUG=695967 Review-Url: https://codereview.chromium.org/2714993002 Cr-Commit-Position: refs/heads/master@{#453206} Committed: https://chromium.googlesource.com/chromium/src/+/1ee77687069628ebd0bba3ff791a... ==========
Message was sent while issue was closed.
Committed patchset #2 (id:20001) as https://chromium.googlesource.com/chromium/src/+/1ee77687069628ebd0bba3ff791a... |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
