Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2862153002:
[Android, Cast] Get the notification origin from the correct tab. (Closed)
Description[Android, Cast] Get the notification origin from the correct tab.
BUG=691577
TEST=existing tests
Review-Url: https://codereview.chromium.org/2862153002
Cr-Commit-Position: refs/heads/master@{#470192}
Committed: https://chromium.googlesource.com/chromium/src/+/21e42d8f013f59f7764b466f05c7433640eae1b2
Patch Set 1 #
Total comments: 2
Patch Set 2 : Set the origin when the notification is updated. #Patch Set 3 : Set the origin when a route is selected #
Total comments: 1
Patch Set 4 : Marked mTabOrigin as nullable #Messages
Total messages: 23 (13 generated)
The CQ bit was checked by avayvod@chromium.org to run a CQ dry run
avayvod@chromium.org changed reviewers: + dgn@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.
https://codereview.chromium.org/2862153002/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/2862153002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/media/remote/CastNotificationControl.java:74: sInstance.mOrigin = getCurrentTabOrigin(activity); Hum... here the origin is set when the singleton CastNotificationControl is created, and never updated later. So if you cast one thing and then cast another one, wouldn't the origin still be the first one? Can we pass to show() or through PlayerState the activity, or frameid (or whatever we use to identify frames/tab in the native code) ?
https://codereview.chromium.org/2862153002/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/2862153002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/media/remote/CastNotificationControl.java:74: sInstance.mOrigin = getCurrentTabOrigin(activity); On 2017/05/05 at 10:39:38, dgn wrote: > Hum... here the origin is set when the singleton CastNotificationControl is created, and never updated later. So if you cast one thing and then cast another one, wouldn't the origin still be the first one? > > Can we pass to show() or through PlayerState the activity, or frameid (or whatever we use to identify frames/tab in the native code) ? The origin is set every time getOrCreate() is called. It is called from onStateReset() which is called everytime before we show the picker dialog, so it may still be too early (e.g. if I'm casting one video and try to cast another - it will reset the origin. So yeah, I'm going to move it to show() somehow.
Set the origin when the notification is updated.
Set the origin when a route is selected
PTAL I now save the origin of the current tab as soon as the device is picked for the current video. It's done via modal cast dialog so the tab/activity can't change between the user opening the dialog and choosing the route.
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.
Thanks! lgtm https://codereview.chromium.org/2862153002/diff/40001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/media/remote/CastNotificationControl.java (right): https://codereview.chromium.org/2862153002/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/media/remote/CastNotificationControl.java:51: private String mTabOrigin = null; nit: Don't initialise to default values in java (source: https://groups.google.com/a/chromium.org/forum/#!msg/chromium-dev/ylbLOvLs0bs...) annotate with @Nullable instead?
Marked mTabOrigin as nullable
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/2862153002/#ps60001 (title: "Marked mTabOrigin as nullable")
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": 1494276612493720,
"parent_rev": "463a9eb80e6934a1eb3055aba2112e1bd3d4e6b6", "commit_rev":
"21e42d8f013f59f7764b466f05c7433640eae1b2"}
Message was sent while issue was closed.
Description was changed from ========== [Android, Cast] Get the notification origin from the correct tab. BUG=691577 TEST=existing tests ========== to ========== [Android, Cast] Get the notification origin from the correct tab. BUG=691577 TEST=existing tests Review-Url: https://codereview.chromium.org/2862153002 Cr-Commit-Position: refs/heads/master@{#470192} Committed: https://chromium.googlesource.com/chromium/src/+/21e42d8f013f59f7764b466f05c7... ==========
Message was sent while issue was closed.
Committed patchset #4 (id:60001) as https://chromium.googlesource.com/chromium/src/+/21e42d8f013f59f7764b466f05c7... |
