Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2675693004:
[Media>UI] Reset media metadata when navigating away (Closed)
|
Created:
3 years, 10 months ago by Zhiqiang Zhang (Slow) Modified:
3 years, 10 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] Reset media metadata when navigating away
There is a bug caused by media metadata being not properly reset when
navigating away, so that the notification title and artist stays the
same when the new page starts to play media. This CL fixes the issue.
BUG=687973
Review-Url: https://codereview.chromium.org/2675693004
Cr-Commit-Position: refs/heads/master@{#448231}
Committed: https://chromium.googlesource.com/chromium/src/+/b0a34cb69bdca47cfe3dd3d1cead96446391dcbf
Patch Set 1 #Patch Set 2 : . #
Total comments: 6
Patch Set 3 : listen to the correct event & new tests #
Total comments: 1
Patch Set 4 : added comments #
Messages
Total messages: 20 (13 generated)
The CQ bit was checked by zqzhang@chromium.org to run a CQ dry run
zqzhang@chromium.org changed reviewers: + avayvod@chromium.org
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 https://codereview.chromium.org/2675693004/diff/20001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/media/ui/MediaSessionTabHelper.java (right): https://codereview.chromium.org/2675693004/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/media/ui/MediaSessionTabHelper.java:303: mCurrentMetadata = getMetadata(); nit: why is this needed - looks like a workaround against us not keeping mCurrentMetadata up-to-date which contradicts its name :) https://codereview.chromium.org/2675693004/diff/20001/chrome/android/javatest... File chrome/android/javatests/src/org/chromium/chrome/browser/media/ui/NotificationTitleUpdatedTest.java (right): https://codereview.chromium.org/2675693004/diff/20001/chrome/android/javatest... chrome/android/javatests/src/org/chromium/chrome/browser/media/ui/NotificationTitleUpdatedTest.java:53: public void testSessionStatePlaying() { nit: what's up with these changes?
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...
FYI I just realised onUrlUpdated() is not a proper signal for detecting navigating away. We should listen to onDidNavigateMainFrame() instead. Also, in-page navigation (scrolling to an anchor) is also handled. The new changes are trivial so doesn't need another look. New test (for in-page navigation) is added. https://codereview.chromium.org/2675693004/diff/20001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/media/ui/MediaSessionTabHelper.java (right): https://codereview.chromium.org/2675693004/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/media/ui/MediaSessionTabHelper.java:303: mCurrentMetadata = getMetadata(); On 2017/02/02 19:46:43, whywhat_slow_PST_till_Feb_6 wrote: > nit: why is this needed - looks like a workaround against us not keeping > mCurrentMetadata up-to-date which contradicts its name :) mCurrentMetadata selects either mPageMetadata or mFallbackMetadata. In case of navigating away, I'm not confident that onTitleUpdated() is called after this, so need to update mCurrentMetadata to be safe. https://codereview.chromium.org/2675693004/diff/20001/chrome/android/javatest... File chrome/android/javatests/src/org/chromium/chrome/browser/media/ui/NotificationTitleUpdatedTest.java (right): https://codereview.chromium.org/2675693004/diff/20001/chrome/android/javatest... chrome/android/javatests/src/org/chromium/chrome/browser/media/ui/NotificationTitleUpdatedTest.java:53: public void testSessionStatePlaying() { On 2017/02/02 19:46:43, whywhat_slow_PST_till_Feb_6 wrote: > nit: what's up with these changes? These arethe leftovers when we removed the custom media notification.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2675693004/diff/20001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/media/ui/MediaSessionTabHelper.java (right): https://codereview.chromium.org/2675693004/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/media/ui/MediaSessionTabHelper.java:303: mCurrentMetadata = getMetadata(); On 2017/02/03 at 15:50:29, Zhiqiang Zhang wrote: > On 2017/02/02 19:46:43, whywhat_slow_PST_till_Feb_6 wrote: > > nit: why is this needed - looks like a workaround against us not keeping > > mCurrentMetadata up-to-date which contradicts its name :) > > mCurrentMetadata selects either mPageMetadata or mFallbackMetadata. In case of navigating away, I'm not confident that onTitleUpdated() is called after this, so need to update mCurrentMetadata to be safe. That sounds like a source of future errors, could you confirm if it's called or not and if not, add a comment why you need to update mCurrentMetadata. If it's hard/impossible to confirm/guarantee, you can probably more or less paste your reply above in a comment above updating the mCurrentMetadata. https://codereview.chromium.org/2675693004/diff/40001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/media/ui/MediaSessionTabHelper.java (right): https://codereview.chromium.org/2675693004/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/media/ui/MediaSessionTabHelper.java:306: mCurrentMetadata = getMetadata(); I feel like a comment here is justified at least :)
The CQ bit was checked by zqzhang@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from avayvod@chromium.org Link to the patchset: https://codereview.chromium.org/2675693004/#ps60001 (title: "added comments")
https://codereview.chromium.org/2675693004/diff/20001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/media/ui/MediaSessionTabHelper.java (right): https://codereview.chromium.org/2675693004/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/media/ui/MediaSessionTabHelper.java:303: mCurrentMetadata = getMetadata(); On 2017/02/03 17:58:56, whywhat_slow_PST_till_Feb_6 wrote: > On 2017/02/03 at 15:50:29, Zhiqiang Zhang wrote: > > On 2017/02/02 19:46:43, whywhat_slow_PST_till_Feb_6 wrote: > > > nit: why is this needed - looks like a workaround against us not keeping > > > mCurrentMetadata up-to-date which contradicts its name :) > > > > mCurrentMetadata selects either mPageMetadata or mFallbackMetadata. In case of > navigating away, I'm not confident that onTitleUpdated() is called after this, > so need to update mCurrentMetadata to be safe. > > That sounds like a source of future errors, could you confirm if it's called or > not and if not, add a comment why you need to update mCurrentMetadata. If it's > hard/impossible to confirm/guarantee, you can probably more or less paste your > reply above in a comment above updating the mCurrentMetadata. Done.
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": 60001, "attempt_start_ts": 1486378483317370,
"parent_rev": "b9c4a13594bb46a2b2ef77c32cc4a0b4e57cdbd7", "commit_rev":
"b0a34cb69bdca47cfe3dd3d1cead96446391dcbf"}
Message was sent while issue was closed.
Description was changed from ========== [Media>UI] Reset media metadata when navigating away There is a bug caused by media metadata being not properly reset when navigating away, so that the notification title and artist stays the same when the new page starts to play media. This CL fixes the issue. BUG=687973 ========== to ========== [Media>UI] Reset media metadata when navigating away There is a bug caused by media metadata being not properly reset when navigating away, so that the notification title and artist stays the same when the new page starts to play media. This CL fixes the issue. BUG=687973 Review-Url: https://codereview.chromium.org/2675693004 Cr-Commit-Position: refs/heads/master@{#448231} Committed: https://chromium.googlesource.com/chromium/src/+/b0a34cb69bdca47cfe3dd3d1cead... ==========
Message was sent while issue was closed.
Committed patchset #4 (id:60001) as https://chromium.googlesource.com/chromium/src/+/b0a34cb69bdca47cfe3dd3d1cead... |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
