Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2627753006:
[Media>UI] Show artist instead of origin on Wearable (Closed)
|
Created:
3 years, 11 months ago by Zhiqiang Zhang (Slow) Modified:
3 years, 11 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] Show artist instead of origin on Wearable
Wearable has limited space for showing media metadata. The origin of the
page takes too much space and is less important than artist to the user.
If the page specifies artwork, we show it on Wearable instead of the
origin.
BUG=679358
Review-Url: https://codereview.chromium.org/2627753006
Cr-Commit-Position: refs/heads/master@{#443802}
Committed: https://chromium.googlesource.com/chromium/src/+/7c0e0135f94dbac938d6f8eec018df26609741ea
Patch Set 1 #
Total comments: 2
Patch Set 2 : rebased #Messages
Total messages: 18 (10 generated)
Description was changed from ========== [Media>UI] Show artist instead of origin on Wearable Wearable has limited space for showing media metadata. The origin of the page takes too much space and is less important than artist to the user. If the page specifies artwork, we show it on Wearable instead of the origin. BUG=677000 ========== to ========== [Media>UI] Show artist instead of origin on Wearable Wearable has limited space for showing media metadata. The origin of the page takes too much space and is less important than artist to the user. If the page specifies artwork, we show it on Wearable instead of the origin. BUG=679358 ==========
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...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Everytime we change something showing the origin, I think about asking security first. Do push notifications show origin on Wear (if enabled there)? https://codereview.chromium.org/2627753006/diff/1/chrome/android/java/src/org... File chrome/android/java/src/org/chromium/chrome/browser/media/ui/MediaNotificationManager.java (left): https://codereview.chromium.org/2627753006/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/media/ui/MediaNotificationManager.java:695: if (Build.VERSION.SDK_INT >= Build.VERSION_CODES.LOLLIPOP) { why removing this code?
> Everytime we change something showing the origin, I think about asking security > first. Sorry I forgot to mention that we need to wait for security approval on the bug before landing this CL. Currently just make it ready :) > Do push notifications show origin on Wear (if enabled there)? No, the push notifications doesn't :( https://codereview.chromium.org/2627753006/diff/1/chrome/android/java/src/org... File chrome/android/java/src/org/chromium/chrome/browser/media/ui/MediaNotificationManager.java (left): https://codereview.chromium.org/2627753006/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/media/ui/MediaNotificationManager.java:695: if (Build.VERSION.SDK_INT >= Build.VERSION_CODES.LOLLIPOP) { On 2017/01/12 22:13:28, whywhat wrote: > why removing this code? The logic is that on newer Android versions, SUBTITLE can be specified, so we use metadata.artist as KEY_ARTIST and origin as SUBTITLE. Unfortunately, due to the limit of UI space, wear will show TITLE and SUBTITLE, and ARTIST will never be shown. I think we can just simplify the logic since there's no different on what is observed.
lgtm pending security review then :)
On 2017/01/12 at 23:44:03, whywhat wrote: > lgtm pending security review then :) I also prefixed the title with DO NOT SUBMIT - don't forget to remove when ready.
On 2017/01/13 00:07:24, whywhat wrote: > On 2017/01/12 at 23:44:03, whywhat wrote: > > lgtm pending security review then :) > > I also prefixed the title with DO NOT SUBMIT - don't forget to remove when > ready. I'm landing this as we got approval via email :)
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/2627753006/#ps20001 (title: "rebased")
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": 1484421368048800,
"parent_rev": "c9c5ea2a110bd0721860bf2899f1dfecc150fc06", "commit_rev":
"7c0e0135f94dbac938d6f8eec018df26609741ea"}
Message was sent while issue was closed.
Description was changed from ========== [Media>UI] Show artist instead of origin on Wearable Wearable has limited space for showing media metadata. The origin of the page takes too much space and is less important than artist to the user. If the page specifies artwork, we show it on Wearable instead of the origin. BUG=679358 ========== to ========== [Media>UI] Show artist instead of origin on Wearable Wearable has limited space for showing media metadata. The origin of the page takes too much space and is less important than artist to the user. If the page specifies artwork, we show it on Wearable instead of the origin. BUG=679358 Review-Url: https://codereview.chromium.org/2627753006 Cr-Commit-Position: refs/heads/master@{#443802} Committed: https://chromium.googlesource.com/chromium/src/+/7c0e0135f94dbac938d6f8eec018... ==========
Message was sent while issue was closed.
Committed patchset #2 (id:20001) as https://chromium.googlesource.com/chromium/src/+/7c0e0135f94dbac938d6f8eec018... |
