Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2860633002:
[Cast,Android] Add origin to the Cast notification (Closed)
Description[Cast,Android] Add origin to the Cast notification
We get the current tab's URL when the notification is shown.
BUG=691577
TEST=existing tests+manual
Review-Url: https://codereview.chromium.org/2860633002
Cr-Commit-Position: refs/heads/master@{#469150}
Committed: https://chromium.googlesource.com/chromium/src/+/201ab5f17e23e13c6797cfe3215dca8808bb6313
Patch Set 1 #
Total comments: 4
Patch Set 2 : Use the tab's URL #
Total comments: 1
Messages
Total messages: 25 (14 generated)
The CQ bit was checked by avayvod@chromium.org to run a CQ dry run
avayvod@chromium.org changed reviewers: + dgn@chromium.org
PTaL
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/2860633002/diff/1/chrome/android/java/src/org... File chrome/android/java/src/org/chromium/chrome/browser/media/remote/CastNotificationControl.java (right): https://codereview.chromium.org/2860633002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/media/remote/CastNotificationControl.java:148: new URI(listener.getFrameUrl()), true); not familiar with that, I assume that frameUrl is the one for the embedding website, not the video itself (that would be sourceUrl), right? Would that show the website itself or youtube in the case of an embedded youtube video?
https://codereview.chromium.org/2860633002/diff/1/chrome/android/java/src/org... File chrome/android/java/src/org/chromium/chrome/browser/media/remote/CastNotificationControl.java (right): https://codereview.chromium.org/2860633002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/media/remote/CastNotificationControl.java:148: new URI(listener.getFrameUrl()), true); On 2017/05/03 at 11:07:40, dgn wrote: > not familiar with that, I assume that frameUrl is the one for the embedding website, not the video itself (that would be sourceUrl), right? Would that show the website itself or youtube in the case of an embedded youtube video? Yeah, frameUrl is the URL of the frame containing the video so it would be m.youtube.com in the embedded video case. I think the latest policy is to show the main frame's origin for all these things, so maybe I could somehow obtain the currently focused tab at the moment of casting and get its URL instead? WDYT? Either way I should look into testing this too :)
https://codereview.chromium.org/2860633002/diff/1/chrome/android/java/src/org... File chrome/android/java/src/org/chromium/chrome/browser/media/remote/CastNotificationControl.java (right): https://codereview.chromium.org/2860633002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/media/remote/CastNotificationControl.java:148: new URI(listener.getFrameUrl()), true); On 2017/05/03 14:53:10, whywhat wrote: > On 2017/05/03 at 11:07:40, dgn wrote: > > not familiar with that, I assume that frameUrl is the one for the embedding > website, not the video itself (that would be sourceUrl), right? Would that show > the website itself or youtube in the case of an embedded youtube video? > > Yeah, frameUrl is the URL of the frame containing the video so it would be > http://m.youtube.com in the embedded video case. > I think the latest policy is to show the main frame's origin for all these > things, so maybe I could somehow obtain the currently focused tab at the moment > of casting and get its URL instead? WDYT? > > Either way I should look into testing this too :) Sounds good yes, I imagine users would look at tab names/urls to match the notification anyway.
Use the tab's URL
PTAL https://codereview.chromium.org/2860633002/diff/1/chrome/android/java/src/org... File chrome/android/java/src/org/chromium/chrome/browser/media/remote/CastNotificationControl.java (right): https://codereview.chromium.org/2860633002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/media/remote/CastNotificationControl.java:148: new URI(listener.getFrameUrl()), true); On 2017/05/03 at 14:56:11, dgn wrote: > On 2017/05/03 14:53:10, whywhat wrote: > > On 2017/05/03 at 11:07:40, dgn wrote: > > > not familiar with that, I assume that frameUrl is the one for the embedding > > website, not the video itself (that would be sourceUrl), right? Would that show > > the website itself or youtube in the case of an embedded youtube video? > > > > Yeah, frameUrl is the URL of the frame containing the video so it would be > > http://m.youtube.com in the embedded video case. > > I think the latest policy is to show the main frame's origin for all these > > things, so maybe I could somehow obtain the currently focused tab at the moment > > of casting and get its URL instead? WDYT? > > > > Either way I should look into testing this too :) > > Sounds good yes, I imagine users would look at tab names/urls to match the notification anyway. Done.
The CQ bit was checked by avayvod@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.
Description was changed from ========== [Cast,Android] Add origin to the Cast notification We have the frame url stored in the MediaStateListener. BUG=691577 TEST=existing tests+manual ========== to ========== [Cast,Android] Add origin to the Cast notification We get the current tab's URL when the notification is shown. BUG=691577 TEST=existing tests+manual ==========
The CQ bit was checked by avayvod@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from dgn@chromium.org Link to the patchset: https://codereview.chromium.org/2860633002/#ps20001 (title: "Use the tab's URL")
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": 1493848235845320,
"parent_rev": "f4063d939f75d7858172f7a0558b3873697d9b1e", "commit_rev":
"201ab5f17e23e13c6797cfe3215dca8808bb6313"}
Message was sent while issue was closed.
Description was changed from ========== [Cast,Android] Add origin to the Cast notification We get the current tab's URL when the notification is shown. BUG=691577 TEST=existing tests+manual ========== to ========== [Cast,Android] Add origin to the Cast notification We get the current tab's URL when the notification is shown. BUG=691577 TEST=existing tests+manual Review-Url: https://codereview.chromium.org/2860633002 Cr-Commit-Position: refs/heads/master@{#469150} Committed: https://chromium.googlesource.com/chromium/src/+/201ab5f17e23e13c6797cfe3215d... ==========
Message was sent while issue was closed.
Committed patchset #2 (id:20001) as https://chromium.googlesource.com/chromium/src/+/201ab5f17e23e13c6797cfe3215d...
Message was sent while issue was closed.
https://codereview.chromium.org/2860633002/diff/20001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/media/remote/CastNotificationControl.java (right): https://codereview.chromium.org/2860633002/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/media/remote/CastNotificationControl.java:268: Activity activity = ApplicationStatus.getLastTrackedFocusedActivity(); post-review nit: Is it possible that content starts playing in a tab after you switched to another one? like load a page, interact with it, js delays playing media, so in the meantime you switch to another tab. In that case wouldn't this get the wrong tab to get the URL?
Message was sent while issue was closed.
On 2017/05/04 at 09:56:03, dgn wrote: > https://codereview.chromium.org/2860633002/diff/20001/chrome/android/java/src... > File chrome/android/java/src/org/chromium/chrome/browser/media/remote/CastNotificationControl.java (right): > > https://codereview.chromium.org/2860633002/diff/20001/chrome/android/java/src... > chrome/android/java/src/org/chromium/chrome/browser/media/remote/CastNotificationControl.java:268: Activity activity = ApplicationStatus.getLastTrackedFocusedActivity(); > post-review nit: Is it possible that content starts playing in a tab after you switched to another one? like load a page, interact with it, js delays playing media, so in the meantime you switch to another tab. > > In that case wouldn't this get the wrong tab to get the URL? Good point. Sent a follow up here: https://codereview.chromium.org/2862153002 |
