Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: enable picture-in-picture mode for video tags #17686

Merged
merged 6 commits into from Aug 22, 2019

Conversation

@brenca
Copy link
Member

commented Apr 3, 2019

Description of Change

This PR enables the picture-in-picture mode already available in chrome. This is partly a feat:, partly a fix: - without these changes, the DOM reports that PiP mode is available, but no actual window is created for it, and the DOM APIs return a 0x0 phantom PiP window. With these changes if PiP is disabled with the build flag, the DOM APIs correctly report that support for this feature is unavailable.

The added //chrome dependencies are self contained, but there are a couple of extra icons baked into the code when this feature is enabled. I'm happy to explore ways to only include the necessary icons, but we might we might end up with two folders with a small BUILD.gn in each that way, and I thought the easier maintainability/readability vs small size increase was worth it.

Resolves #17369 and #11582.

Checklist

Release Notes

Notes: Added support for picture-in-picture mode for video elements.

@brenca brenca requested review from nornagon and deepak1556 Apr 3, 2019

@brenca brenca requested a review from electron/wg-upgrades as a code owner Apr 3, 2019

@brenca brenca changed the title feat: enable picture-in-picture mode for video tags feat: enable picture-in-picture mode for video tags [WIP] Apr 3, 2019

@electron-cation electron-cation bot removed the new-pr 🌱 label Apr 4, 2019

@brenca brenca force-pushed the brenca/pip-video branch from ef2577c to 1b65f1d Apr 11, 2019

@brenca brenca changed the title feat: enable picture-in-picture mode for video tags [WIP] feat: enable picture-in-picture mode for video tags Apr 11, 2019


#include "chrome/browser/ui/views/overlay/skip_ad_label_button.h"

-#include "chrome/grit/generated_resources.h"

This comment has been minimized.

Copy link
@MarshallOfSound

MarshallOfSound Apr 16, 2019

Member

Is there anyway we can pull some compiler/ninja voodoo to make this work without a patch

cc @nornagon @deepak1556

This comment has been minimized.

Copy link
@nornagon

nornagon Apr 16, 2019

Member

Perhaps we could consider refactoring upstream to make these resources more self-contained?

@MarshallOfSound
Copy link
Member

left a comment

Rebased it and tested locally, approving and requesting a 6-0-x backport as without it requesting a PiP view crashes

@deepak1556

This comment has been minimized.

Copy link
Member

commented Apr 29, 2019

requesting a 6-0-x backport as without it requesting a PiP view crashes

For 6-0-x I would suggest disabling PIP web apis through the embedder hooks https://groups.google.com/a/chromium.org/d/msg/embedder-dev/A4zqjsmibXc/Osy_LLZ0DwAJ. I don't see the reason to ship this feature.

@MarshallOfSound

This comment has been minimized.

Copy link
Member

commented Apr 29, 2019

@deepak1556 I want to do that for 5-0-x (disable it) but I'd like to make it possible to use this in 6-0-x

@deepak1556

This comment has been minimized.

Copy link
Member

commented May 1, 2019

LGTM, Can you fix the conflicts.

@ckerr

This comment has been minimized.

Copy link
Member

commented May 14, 2019

@brenca ping, can you fix the conflicts

@mohamedghan

This comment has been minimized.

Copy link

commented Jul 28, 2019

guys, i'm having a bad time trying to add pip in my project. i'm using electron 6.x.x beta15

@alirezavalizade

This comment has been minimized.

Copy link

commented Jul 28, 2019

Any update on this pull request?

@Ajeey

This comment has been minimized.

Copy link
Contributor

commented Jul 31, 2019

Guys, any updates on this? Would love to see this ported to Electron!

@jkleinsc jkleinsc force-pushed the brenca/pip-video branch from 6229174 to c04c073 Aug 1, 2019

@brenca brenca force-pushed the brenca/pip-video branch from d8d9b31 to 9c408bd Aug 9, 2019

@brenca brenca removed the target/6-0-x label Aug 9, 2019

@ctreadw6 ctreadw6 referenced this pull request Aug 17, 2019

@brenca brenca force-pushed the brenca/pip-video branch from af2b350 to a1d08a8 Aug 21, 2019

@brenca brenca force-pushed the brenca/pip-video branch from a1d08a8 to 7aa14d4 Aug 21, 2019

@zcbenz
zcbenz approved these changes Aug 22, 2019

@zcbenz zcbenz merged commit 9ccd6aa into master Aug 22, 2019

13 checks passed

Artifact Comparison No Changes
Details
Semantic Pull Request ready to be squashed
Details
WIP Ready for review
Details
appveyor: win-ia32-testing AppVeyor build succeeded
Details
appveyor: win-ia32-testing-pr AppVeyor build succeeded
Details
appveyor: win-x64-testing AppVeyor build succeeded
Details
appveyor: win-x64-testing-pr AppVeyor build succeeded
Details
build-linux Workflow: build-linux
Details
build-mac Workflow: build-mac
Details
electron-arm-testing Build #20190821.31 succeeded
Details
electron-arm64-testing Build #20190821.31 succeeded
Details
lint Workflow: lint
Details
release-notes Release notes found

@zcbenz zcbenz deleted the brenca/pip-video branch Aug 22, 2019

@release-clerk

This comment has been minimized.

Copy link

commented Aug 22, 2019

Release Notes Persisted

Added support for picture-in-picture mode for video elements.

@codebytere

This comment has been minimized.

Copy link
Member

commented Aug 23, 2019

/trop run backport-to 7-0-x

@trop

This comment has been minimized.

Copy link
Contributor

commented Aug 23, 2019

The backport process for this PR has been manually initiated,
sending your 1's and 0's to "7-0-x" here we go! :D

@trop

This comment has been minimized.

Copy link
Contributor

commented Aug 23, 2019

I was unable to backport this PR to "7-0-x" cleanly;
you will need to perform this backport manually.

@trop

This comment has been minimized.

Copy link
Contributor

commented Aug 23, 2019

A maintainer has manually backported this PR to "7-0-x", please check out #19914

@sofianguy sofianguy added this to Fixed in 7.0.0-beta.4 in 7.0.x Sep 3, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.