Merge lp://staging/~justinmcp/oxide/media-arbitration into lp://staging/~oxide-developers/oxide/oxide.trunk

Proposed by Justin McPherson
Status: Needs review
Proposed branch: lp://staging/~justinmcp/oxide/media-arbitration
Merge into: lp://staging/~oxide-developers/oxide/oxide.trunk
Diff against target: 866 lines (+645/-4)
18 files modified
CMakeLists.txt (+5/-0)
shared/browser/media/mediahub_player_shim.cc (+48/-0)
shared/browser/media/mediahub_player_shim.h (+9/-0)
shared/browser/media/oxide_browser_media_arbitrator.cc (+204/-0)
shared/browser/media/oxide_browser_media_arbitrator.h (+89/-0)
shared/browser/media/oxide_media_web_contents_observer.cc (+2/-2)
shared/browser/oxide_browser_process_main.cc (+4/-0)
shared/browser/oxide_content_browser_client.cc (+2/-1)
shared/browser/oxide_web_view.cc (+7/-0)
shared/common/oxide_constants.cc (+1/-0)
shared/common/oxide_constants.h (+1/-0)
shared/common/oxide_messages.h (+29/-0)
shared/renderer/media/oxide_renderer_media_arbitrator.cc (+148/-0)
shared/renderer/media/oxide_renderer_media_arbitrator.h (+61/-0)
shared/renderer/media/oxide_renderer_media_player_manager.cc (+1/-1)
shared/renderer/oxide_content_renderer_client.cc (+20/-0)
shared/renderer/oxide_content_renderer_client.h (+3/-0)
shared/shared.gyp (+11/-0)
To merge this branch: bzr merge lp://staging/~justinmcp/oxide/media-arbitration
Reviewer Review Type Date Requested Status
Chris Coulson Pending
Review via email: mp+266517@code.staging.launchpad.net
To post a comment you must log in.
Revision history for this message
Justin McPherson (justinmcp) wrote :

The nature of this change makes it pretty intrusive. I've tried to cut it down, but we may need to chat about the details.

Originally I made a MediaArbitrator delegate class, but concerned about adding to much code in the patches, I pulled that and it mostly rests in RenderFrameImpl.

This is something that really belongs upstream.

Since support for the MH side of the arbitration is not available yet. At the time of this comment, this branch should not yet be merged.

Revision history for this message
Chris Coulson (chrisccoulson) wrote :

Hi,

So there's a couple of things I'm a bit concerned about with this:

- It's quite a big patch. It might be better to constrain the patch to just adding new content API's and then adding the actual additional IPC and other bits in Oxide instead. This is something I've been meaning to do with support-native-popup-menus.patch (I just want to shrink this down to a new method on ContentRendererClient which allows Oxide to create a blink::WebExternalPopupMenu and a small patch in RenderFrameImpl::createExternalPopupMenu to call it, rather than the gigantic patch to enable the Android + OSX bits).
- As it's implemented now, arbitration leaves blink in an inconsistent state. When blink calls play() on the blink::WebMediaPlayer implementation, it expects it to play. If it doesn't play, you need to ensure blink ends up in a consistent state (ie, not thinking that it's playing). I'm not sure how best to do that without even more intrusive changes. It looks like the Android player ends up in a paused state.

1179. By Justin McPherson

Better handling of process level arbitration permission

1180. By Justin McPherson

Merge from trunk.

1181. By Justin McPherson

Finish merging to trunk.

Unmerged revisions

1181. By Justin McPherson

Finish merging to trunk.

1180. By Justin McPherson

Merge from trunk.

1179. By Justin McPherson

Better handling of process level arbitration permission

1178. By Justin McPherson

Merge from trunk.

1177. By Justin McPherson

Cleanup

1176. By Justin McPherson

WIP

1175. By Justin McPherson

Fixup whitespace changes

1174. By Justin McPherson

WIP

1173. By Justin McPherson

WIP

1172. By Justin McPherson

WIP

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
The diff is not available at this time. You can reload the page or download it.

Subscribers

People subscribed via source and target branches