[PIP] When video ends we should show a play button |
|||||||||||||
Issue descriptionRight now, when a PIP'd video ends, the PIP window shows the last frame without providing a way to replay video. Instead, we want to show a play button on top of the last frame visible (like what <video> elements currently do inline).
,
Dec 12
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/28e9c18c8acd371da97a72cf14c70eb0a1702fb7 commit 28e9c18c8acd371da97a72cf14c70eb0a1702fb7 Author: François Beaufort <beaufort.francois@gmail.com> Date: Wed Dec 12 10:39:49 2018 Add replay button to Picture-in-Picture window when video ends This CL makes sure there's a new replay button visible in the PiP window when video ends. This icon comes from Material Icons library at https://material.io/tools/icons/?icon=replay. https://i.imgur.com/I1n90tM.png Bug: 911620 Change-Id: Ibfcb96b9c69228ad74afe96ec00eb0e28d335ad8 Reviewed-on: https://chromium-review.googlesource.com/c/1360852 Commit-Queue: François Beaufort <beaufort.francois@gmail.com> Reviewed-by: Mounir Lamouri <mlamouri@chromium.org> Reviewed-by: Evan Stade <estade@chromium.org> Cr-Commit-Position: refs/heads/master@{#615846} [modify] https://crrev.com/28e9c18c8acd371da97a72cf14c70eb0a1702fb7/chrome/app/generated_resources.grd [modify] https://crrev.com/28e9c18c8acd371da97a72cf14c70eb0a1702fb7/chrome/browser/picture_in_picture/picture_in_picture_window_controller_browsertest.cc [modify] https://crrev.com/28e9c18c8acd371da97a72cf14c70eb0a1702fb7/chrome/browser/ui/BUILD.gn [modify] https://crrev.com/28e9c18c8acd371da97a72cf14c70eb0a1702fb7/chrome/browser/ui/views/overlay/overlay_window_views.cc [modify] https://crrev.com/28e9c18c8acd371da97a72cf14c70eb0a1702fb7/chrome/browser/ui/views/overlay/overlay_window_views.h [add] https://crrev.com/28e9c18c8acd371da97a72cf14c70eb0a1702fb7/chrome/browser/ui/views/overlay/playback_image_button.cc [add] https://crrev.com/28e9c18c8acd371da97a72cf14c70eb0a1702fb7/chrome/browser/ui/views/overlay/playback_image_button.h [modify] https://crrev.com/28e9c18c8acd371da97a72cf14c70eb0a1702fb7/components/vector_icons/BUILD.gn [add] https://crrev.com/28e9c18c8acd371da97a72cf14c70eb0a1702fb7/components/vector_icons/replay.icon
,
Dec 12
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/50a63f77f9f7fd2a3138cf71b034268a334f9e00 commit 50a63f77f9f7fd2a3138cf71b034268a334f9e00 Author: Friedrich Horschig [CET] <fhorschig@chromium.org> Date: Wed Dec 12 13:37:22 2018 Revert "Add replay button to Picture-in-Picture window when video ends" This reverts commit 28e9c18c8acd371da97a72cf14c70eb0a1702fb7. Reason for revert: Broke numerous browser_tests and viz_browser_tests on Linux ChromiumOS MSan Tests: https://ci.chromium.org/p/chromium/builders/luci.chromium.ci/Linux%20ChromiumOS%20MSan%20Tests/10120 Relevant log snippet: ==23389==WARNING: MemorySanitizer: use-of-uninitialized-value #0 0x55d8b905b0bd in views::PlaybackImageButton::UpdateImageAndTooltipText() ./../../chrome/browser/ui/views/overlay/playback_image_button.cc:65:3 #1 0x55d8b905ac31 in views::PlaybackImageButton::SetButtonSize(gfx::Size const&) ./../../chrome/browser/ui/views/overlay/playback_image_button.cc:52:3 ==> 37 broken *PictureInPictureWindowControllerBrowserTest* Original change's description: > Add replay button to Picture-in-Picture window when video ends > > This CL makes sure there's a new replay button visible in the PiP window > when video ends. This icon comes from Material Icons library at > https://material.io/tools/icons/?icon=replay. > > https://i.imgur.com/I1n90tM.png > > Bug: 911620 > > Change-Id: Ibfcb96b9c69228ad74afe96ec00eb0e28d335ad8 > Reviewed-on: https://chromium-review.googlesource.com/c/1360852 > Commit-Queue: François Beaufort <beaufort.francois@gmail.com> > Reviewed-by: Mounir Lamouri <mlamouri@chromium.org> > Reviewed-by: Evan Stade <estade@chromium.org> > Cr-Commit-Position: refs/heads/master@{#615846} TBR=beaufort.francois@gmail.com,estade@chromium.org,mlamouri@chromium.org Change-Id: I655a32a2e5033500392cd44c921442f066297190 No-Presubmit: true No-Tree-Checks: true No-Try: true Bug: 911620 Reviewed-on: https://chromium-review.googlesource.com/c/1373758 Reviewed-by: Friedrich Horschig [CET] <fhorschig@chromium.org> Commit-Queue: Friedrich Horschig [CET] <fhorschig@chromium.org> Cr-Commit-Position: refs/heads/master@{#615872} [modify] https://crrev.com/50a63f77f9f7fd2a3138cf71b034268a334f9e00/chrome/app/generated_resources.grd [modify] https://crrev.com/50a63f77f9f7fd2a3138cf71b034268a334f9e00/chrome/browser/picture_in_picture/picture_in_picture_window_controller_browsertest.cc [modify] https://crrev.com/50a63f77f9f7fd2a3138cf71b034268a334f9e00/chrome/browser/ui/BUILD.gn [modify] https://crrev.com/50a63f77f9f7fd2a3138cf71b034268a334f9e00/chrome/browser/ui/views/overlay/overlay_window_views.cc [modify] https://crrev.com/50a63f77f9f7fd2a3138cf71b034268a334f9e00/chrome/browser/ui/views/overlay/overlay_window_views.h [delete] https://crrev.com/b89bd2e0ab359b607692eb3ee3e6f9032268a134/chrome/browser/ui/views/overlay/playback_image_button.cc [delete] https://crrev.com/b89bd2e0ab359b607692eb3ee3e6f9032268a134/chrome/browser/ui/views/overlay/playback_image_button.h [modify] https://crrev.com/50a63f77f9f7fd2a3138cf71b034268a334f9e00/components/vector_icons/BUILD.gn [delete] https://crrev.com/b89bd2e0ab359b607692eb3ee3e6f9032268a134/components/vector_icons/replay.icon
,
Dec 12
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/5659c937dda2cad54577291c6188275bd3b833d1 commit 5659c937dda2cad54577291c6188275bd3b833d1 Author: François Beaufort <beaufort.francois@gmail.com> Date: Wed Dec 12 18:10:10 2018 Reland "Add replay button to Picture-in-Picture window when video ends" This is a reland of 28e9c18c8acd371da97a72cf14c70eb0a1702fb7 Original change's description: > Add replay button to Picture-in-Picture window when video ends > > This CL makes sure there's a new replay button visible in the PiP window > when video ends. This icon comes from Material Icons library at > https://material.io/tools/icons/?icon=replay. > > https://i.imgur.com/I1n90tM.png > > Bug: 911620 > > Change-Id: Ibfcb96b9c69228ad74afe96ec00eb0e28d335ad8 > Reviewed-on: https://chromium-review.googlesource.com/c/1360852 > Commit-Queue: François Beaufort <beaufort.francois@gmail.com> > Reviewed-by: Mounir Lamouri <mlamouri@chromium.org> > Reviewed-by: Evan Stade <estade@chromium.org> > Cr-Commit-Position: refs/heads/master@{#615846} Bug: 911620 Change-Id: I8d994d2feb32dd6b83185d9ec3fb44af0be5f977 Reviewed-on: https://chromium-review.googlesource.com/c/1374229 Reviewed-by: Friedrich Horschig [CET] <fhorschig@chromium.org> Reviewed-by: Evan Stade <estade@chromium.org> Commit-Queue: François Beaufort <beaufort.francois@gmail.com> Cr-Commit-Position: refs/heads/master@{#615959} [modify] https://crrev.com/5659c937dda2cad54577291c6188275bd3b833d1/chrome/app/generated_resources.grd [modify] https://crrev.com/5659c937dda2cad54577291c6188275bd3b833d1/chrome/browser/picture_in_picture/picture_in_picture_window_controller_browsertest.cc [modify] https://crrev.com/5659c937dda2cad54577291c6188275bd3b833d1/chrome/browser/ui/BUILD.gn [modify] https://crrev.com/5659c937dda2cad54577291c6188275bd3b833d1/chrome/browser/ui/views/overlay/overlay_window_views.cc [modify] https://crrev.com/5659c937dda2cad54577291c6188275bd3b833d1/chrome/browser/ui/views/overlay/overlay_window_views.h [add] https://crrev.com/5659c937dda2cad54577291c6188275bd3b833d1/chrome/browser/ui/views/overlay/playback_image_button.cc [add] https://crrev.com/5659c937dda2cad54577291c6188275bd3b833d1/chrome/browser/ui/views/overlay/playback_image_button.h [modify] https://crrev.com/5659c937dda2cad54577291c6188275bd3b833d1/components/vector_icons/BUILD.gn [add] https://crrev.com/5659c937dda2cad54577291c6188275bd3b833d1/components/vector_icons/replay.icon
,
Dec 12
Verified in Chromium 73.0.3639.0.
,
Dec 12
This bug requires manual review: There is .grd file changes and we are only 47 days from stable. Please contact the milestone owner if you have questions. Owners: govind@(Android), kariahda@(iOS), djmm@(ChromeOS), abdulsyed@(Desktop) For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot
,
Dec 12
fbeaufort@ how safe is this change for a merge ? I see you just landed it today on trunk, can you verify this in tonight's canary and share the results tomorrow
,
Dec 12
I've downloaded official chromium build from https://download-chromium.appspot.com/?platform=Linux_x64&type=snapshots and tried it there. It is pretty safe to merge as the Picture-in-Picture feature is all controllable via Finch so if anything goes wrong, we can shut it down.
,
Dec 12
fbeaufort@ re-looking this is P3, feel this does not warrant a merge for M72 Lets roll this out in M73
,
Dec 12
P3 is I believe the default priority. I don't think this really was a P3. It's not a blocker but given that we are launching the feature on Chrome OS in 72, this level of polish would be welcome.
,
Dec 12
,
Dec 12
This bug requires manual review: There is .grd file changes and we are only 47 days from stable. Please contact the milestone owner if you have questions. Owners: govind@(Android), kariahda@(iOS), djmm@(ChromeOS), abdulsyed@(Desktop) For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot
,
Dec 12
Approving the merge per comment #10, to M72 branch: 3626 @govind, we need translation run to kick off once this is checked in.
,
Dec 12
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/c7ae78da26ab274f7dfeab351db7ef719e7d7fe0 commit c7ae78da26ab274f7dfeab351db7ef719e7d7fe0 Author: François Beaufort <beaufort.francois@gmail.com> Date: Wed Dec 12 23:06:11 2018 Reland "Add replay button to Picture-in-Picture window when video ends" This is a reland of 28e9c18c8acd371da97a72cf14c70eb0a1702fb7 Original change's description: > Add replay button to Picture-in-Picture window when video ends > > This CL makes sure there's a new replay button visible in the PiP window > when video ends. This icon comes from Material Icons library at > https://material.io/tools/icons/?icon=replay. > > https://i.imgur.com/I1n90tM.png > > Bug: 911620 > > Change-Id: Ibfcb96b9c69228ad74afe96ec00eb0e28d335ad8 > Reviewed-on: https://chromium-review.googlesource.com/c/1360852 > Commit-Queue: François Beaufort <beaufort.francois@gmail.com> > Reviewed-by: Mounir Lamouri <mlamouri@chromium.org> > Reviewed-by: Evan Stade <estade@chromium.org> > Cr-Commit-Position: refs/heads/master@{#615846} Bug: 911620 Change-Id: I8d994d2feb32dd6b83185d9ec3fb44af0be5f977 Reviewed-on: https://chromium-review.googlesource.com/c/1374229 Reviewed-by: Friedrich Horschig [CET] <fhorschig@chromium.org> Reviewed-by: Evan Stade <estade@chromium.org> Commit-Queue: François Beaufort <beaufort.francois@gmail.com> Cr-Original-Commit-Position: refs/heads/master@{#615959}(cherry picked from commit 5659c937dda2cad54577291c6188275bd3b833d1) Reviewed-on: https://chromium-review.googlesource.com/c/1374829 Reviewed-by: Mounir Lamouri <mlamouri@chromium.org> Cr-Commit-Position: refs/branch-heads/3626@{#314} Cr-Branched-From: d897fb137fbaaa9355c0c93124cc048824eb1e65-refs/heads/master@{#612437} [modify] https://crrev.com/c7ae78da26ab274f7dfeab351db7ef719e7d7fe0/chrome/app/generated_resources.grd [modify] https://crrev.com/c7ae78da26ab274f7dfeab351db7ef719e7d7fe0/chrome/browser/picture_in_picture/picture_in_picture_window_controller_browsertest.cc [modify] https://crrev.com/c7ae78da26ab274f7dfeab351db7ef719e7d7fe0/chrome/browser/ui/BUILD.gn [modify] https://crrev.com/c7ae78da26ab274f7dfeab351db7ef719e7d7fe0/chrome/browser/ui/views/overlay/overlay_window_views.cc [modify] https://crrev.com/c7ae78da26ab274f7dfeab351db7ef719e7d7fe0/chrome/browser/ui/views/overlay/overlay_window_views.h [add] https://crrev.com/c7ae78da26ab274f7dfeab351db7ef719e7d7fe0/chrome/browser/ui/views/overlay/playback_image_button.cc [add] https://crrev.com/c7ae78da26ab274f7dfeab351db7ef719e7d7fe0/chrome/browser/ui/views/overlay/playback_image_button.h [modify] https://crrev.com/c7ae78da26ab274f7dfeab351db7ef719e7d7fe0/components/vector_icons/BUILD.gn [add] https://crrev.com/c7ae78da26ab274f7dfeab351db7ef719e7d7fe0/components/vector_icons/replay.icon
,
Dec 13
The NextAction date has arrived: 2018-12-13
,
Dec 19
Here's a summary of the rules that were executed: - OnlyMergeApprovedChange: Rule Failed -- Revision c7ae78da26ab274f7dfeab351db7ef719e7d7fe0 was merged to refs/branch-heads/3626 branch with no merge approval from a TPM! Please explain why this change was merged to the branch! - AcknowledgeMerge: Notification Required --
,
Dec 19
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/c7ae78da26ab274f7dfeab351db7ef719e7d7fe0 Commit: c7ae78da26ab274f7dfeab351db7ef719e7d7fe0 Author: beaufort.francois@gmail.com Commiter: mlamouri@chromium.org Date: 2018-12-12 23:06:11 +0000 UTC Reland "Add replay button to Picture-in-Picture window when video ends" This is a reland of 28e9c18c8acd371da97a72cf14c70eb0a1702fb7 Original change's description: > Add replay button to Picture-in-Picture window when video ends > > This CL makes sure there's a new replay button visible in the PiP window > when video ends. This icon comes from Material Icons library at > https://material.io/tools/icons/?icon=replay. > > https://i.imgur.com/I1n90tM.png > > Bug: 911620 > > Change-Id: Ibfcb96b9c69228ad74afe96ec00eb0e28d335ad8 > Reviewed-on: https://chromium-review.googlesource.com/c/1360852 > Commit-Queue: François Beaufort <beaufort.francois@gmail.com> > Reviewed-by: Mounir Lamouri <mlamouri@chromium.org> > Reviewed-by: Evan Stade <estade@chromium.org> > Cr-Commit-Position: refs/heads/master@{#615846} Bug: 911620 Change-Id: I8d994d2feb32dd6b83185d9ec3fb44af0be5f977 Reviewed-on: https://chromium-review.googlesource.com/c/1374229 Reviewed-by: Friedrich Horschig [CET] <fhorschig@chromium.org> Reviewed-by: Evan Stade <estade@chromium.org> Commit-Queue: François Beaufort <beaufort.francois@gmail.com> Cr-Original-Commit-Position: refs/heads/master@{#615959}(cherry picked from commit 5659c937dda2cad54577291c6188275bd3b833d1) Reviewed-on: https://chromium-review.googlesource.com/c/1374829 Reviewed-by: Mounir Lamouri <mlamouri@chromium.org> Cr-Commit-Position: refs/branch-heads/3626@{#314} Cr-Branched-From: d897fb137fbaaa9355c0c93124cc048824eb1e65-refs/heads/master@{#612437}
,
Dec 20
This merge was approved at #13. |
|||||||||||||
►
Sign in to add a comment |
|||||||||||||
Comment 1 by fbeaufort@chromium.org
, Dec 4