New issue
Advanced search Search tips
Note: Color blocks (like or ) mean that a user may not be available. Tooltip shows the reason.

Issue 851502 link

Starred by 1 user

Issue metadata

Status: Verified
Owner:
Closed: Jun 2018
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: Linux , Windows , Chrome , Mac
Pri: 3
Type: Bug



Sign in to add a comment

Media Context Menu does not work for Loop/Controls/Picture-in-Picture with OOPIF

Project Member Reported by mlamouri@chromium.org, Jun 11 2018

Issue description

STR:
 1. Open https://mounirlamouri.github.io/sandbox/autoplay/unmuted-cross-origin-iframe.html
 2. Attempt to toggle "Loop", "Show controls" or "Picture-in-Picture"

Expected result: work as if the video was in the main frame.
Actual result: does not work.

Cast, copy and open actions seem to work fine.

+apacible@ and fbeaufort@ as they changed some Picture-in-Picture context menu recently and this could be a good follow-up. I'm particularly worried that we may end up with the item not working on embeds.
 
Cc: wjmaclean@chromium.org
After some investigation, I've noticed the issue lives in WebViewImpl::PerformMediaPlayerAction where the node resulting from HitTestResult is not a video or audio element. I believe this is due to bug 776807.
Owner: wjmaclean@chromium.org
Issue 776807 is primarily about making sure the coordinates passed to the plugin are correct, but in this case the context menu commands fail because WebViewImpl doesn't know about the elements in the OOPIF subframe, and the check at

https://cs.chromium.org/chromium/src/third_party/blink/renderer/core/exported/web_view_impl.cc?rcl=8b6e2bfbde8c38653901c6dcfe5baa4e78c245f9&l=2946

fails and early outs.

The appropriate fix here (I think) is to move PerformMediaPlayerAction to a location (WebFrameWidgetImpl?) where it can serve both main frames and OOPIFs.

I can look into this, though maybe not in the next few days.
Owner: mlamouri@chromium.org
Status: Started (was: Available)
I got annoyed enough by this while dogfooding the feature that I wrote a fix.
Project Member

Comment 4 by bugdroid1@chromium.org, Jun 27 2018

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/fdff8bf75117f760384d5f8ec775fa3827335423

commit fdff8bf75117f760384d5f8ec775fa3827335423
Author: Mounir Lamouri <mlamouri@chromium.org>
Date: Wed Jun 27 21:49:53 2018

OOPIF: enable context menu media actions on OOPIF.

This will allow actions such as "loop", "show controls" or the new
"Picture-in-Picture" to work on OOPIF.

Bug:  851502 
Change-Id: Ie2937e908866511c5dbdd0bbf31cf90b3704b859
Reviewed-on: https://chromium-review.googlesource.com/1115582
Commit-Queue: Mounir Lamouri <mlamouri@chromium.org>
Reviewed-by: Daniel Cheng <dcheng@chromium.org>
Reviewed-by: Jochen Eisinger <jochen@chromium.org>
Cr-Commit-Position: refs/heads/master@{#570911}
[modify] https://crrev.com/fdff8bf75117f760384d5f8ec775fa3827335423/chrome/browser/renderer_context_menu/render_view_context_menu.cc
[modify] https://crrev.com/fdff8bf75117f760384d5f8ec775fa3827335423/content/browser/frame_host/render_frame_host_impl.cc
[modify] https://crrev.com/fdff8bf75117f760384d5f8ec775fa3827335423/content/browser/frame_host/render_frame_host_impl.h
[modify] https://crrev.com/fdff8bf75117f760384d5f8ec775fa3827335423/content/browser/renderer_host/render_view_host_impl.cc
[modify] https://crrev.com/fdff8bf75117f760384d5f8ec775fa3827335423/content/browser/renderer_host/render_view_host_impl.h
[modify] https://crrev.com/fdff8bf75117f760384d5f8ec775fa3827335423/content/common/frame_messages.h
[modify] https://crrev.com/fdff8bf75117f760384d5f8ec775fa3827335423/content/common/view_messages.h
[modify] https://crrev.com/fdff8bf75117f760384d5f8ec775fa3827335423/content/public/browser/render_frame_host.h
[modify] https://crrev.com/fdff8bf75117f760384d5f8ec775fa3827335423/content/public/browser/render_view_host.h
[modify] https://crrev.com/fdff8bf75117f760384d5f8ec775fa3827335423/content/renderer/render_frame_impl.cc
[modify] https://crrev.com/fdff8bf75117f760384d5f8ec775fa3827335423/content/renderer/render_frame_impl.h
[modify] https://crrev.com/fdff8bf75117f760384d5f8ec775fa3827335423/content/renderer/render_view_impl.cc
[modify] https://crrev.com/fdff8bf75117f760384d5f8ec775fa3827335423/content/renderer/render_view_impl.h
[modify] https://crrev.com/fdff8bf75117f760384d5f8ec775fa3827335423/third_party/blink/public/web/web_local_frame.h
[modify] https://crrev.com/fdff8bf75117f760384d5f8ec775fa3827335423/third_party/blink/public/web/web_view.h
[modify] https://crrev.com/fdff8bf75117f760384d5f8ec775fa3827335423/third_party/blink/renderer/core/exported/web_view_impl.cc
[modify] https://crrev.com/fdff8bf75117f760384d5f8ec775fa3827335423/third_party/blink/renderer/core/exported/web_view_impl.h
[modify] https://crrev.com/fdff8bf75117f760384d5f8ec775fa3827335423/third_party/blink/renderer/core/frame/web_local_frame_impl.cc
[modify] https://crrev.com/fdff8bf75117f760384d5f8ec775fa3827335423/third_party/blink/renderer/core/frame/web_local_frame_impl.h

Status: Fixed (was: Started)
Cc: krajshree@chromium.org
Labels: Needs-Feedback
Able to reproduce the issue on Mac 10.13.3 using chrome build without fix on enabling the #site-per-process flag.

Tested the fix on Mac 10.13.3, Win-10 and Ubuntu 17.10 using latest chrome version #69.0.3475.0 as per the comment #0.
Attaching screen cast for reference.
Observed that on toggling the "Show controls" from context menu worked as expected but toggling the "loop" option from context menu did nothing in the video.

mlamouri@ - Could you please check the attached screen cast and please help us in confirming the fix.

Thanks...!!
851502@buildwithfix.mp4
1.1 MB View Download
Status: Verified (was: Fixed)
Loop would only apply when the video reaches the end. The fact that loop is checked means that it worked.

Thanks for verifying and for the recording! :)

Sign in to add a comment