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

Proposed by Justin McPherson
Status: Merged
Merged at revision: 915
Proposed branch: lp://staging/~justinmcp/oxide/mediahub
Merge into: lp://staging/~oxide-developers/oxide/oxide.trunk
Diff against target: 3896 lines (+3622/-4) (has conflicts)
30 files modified
CMakeLists.txt (+7/-0)
patches/mediahub-support.patch (+55/-0)
patches/series (+1/-0)
shared/browser/media/oxide_browser_media_player_manager.cc (+205/-0)
shared/browser/media/oxide_browser_media_player_manager.h (+95/-0)
shared/browser/media/oxide_media_player.cc (+26/-0)
shared/browser/media/oxide_media_player.h (+75/-0)
shared/browser/media/oxide_media_web_contents_observer.cc (+77/-0)
shared/browser/media/oxide_media_web_contents_observer.h (+47/-0)
shared/browser/media/oxide_player_media_hub.cc (+213/-0)
shared/browser/media/oxide_player_media_hub.h (+112/-0)
shared/browser/oxide_browser_process_main.cc (+13/-0)
shared/browser/oxide_content_browser_client.cc (+3/-1)
shared/browser/oxide_web_view.cc (+61/-3)
shared/common/oxide_constants.cc (+2/-0)
shared/common/oxide_constants.h (+2/-0)
shared/common/oxide_message_enums.h (+6/-0)
shared/common/oxide_messages.h (+130/-0)
shared/media/mediahub.gyp (+30/-0)
shared/media/mediahub_player_shim.cc (+342/-0)
shared/media/mediahub_player_shim.h (+143/-0)
shared/renderer/media/oxide_media_info_loader.cc (+192/-0)
shared/renderer/media/oxide_media_info_loader.h (+126/-0)
shared/renderer/media/oxide_renderer_media_player_manager.cc (+233/-0)
shared/renderer/media/oxide_renderer_media_player_manager.h (+105/-0)
shared/renderer/media/oxide_web_media_player.cc (+929/-0)
shared/renderer/media/oxide_web_media_player.h (+326/-0)
shared/renderer/oxide_content_renderer_client.cc (+29/-0)
shared/renderer/oxide_content_renderer_client.h (+13/-0)
shared/shared.gyp (+24/-0)
Text conflict in shared/browser/oxide_web_view.cc
To merge this branch: bzr merge lp://staging/~justinmcp/oxide/mediahub
Reviewer Review Type Date Requested Status
Chris Coulson Approve
Review via email: mp+240673@code.staging.launchpad.net

Commit message

Support for Audio playback through media hub.

To post a comment you must log in.
Revision history for this message
Justin McPherson (justinmcp) wrote :
Revision history for this message
Chris Coulson (chrisccoulson) wrote :
Download full text (3.5 KiB)

Thanks for working on this. Before I review this properly, I want to offer some suggestions that will hopefully make it a bit easier to review (and easier to maintain too):

- Please try to keep patches to Chromium as small as possible. There's already a couple that are too big (although not quite as big as this). I rebase the canary branch of Oxide on Chromium daily which normally only takes half an hour or so, but it will almost be a full time job with this patch ;)

- If you need to compile new code in to an existing Chromium target rather than Oxide, please place the code in to an appropriately named directory shared/port (I use something that vaguely matches the Chromium target name) and then just use the patch to modify the gyp file to add the new source files.

- That said, you should only need to compile code in to an existing Chromium component if it requires access to API's that aren't exported (using API's that aren't exported from a component will cause link failures in component builds) or your patch makes other changes to existing code in Chromium that adds a dependency on the new code. There are examples of this in shared/port and qt/core/port.

- MediaWebContentsObserver is just a content::WebContentsObserver - there's no reason to compile this in to patch this in to Chromium at all - it can just be implemented in shared/browser and created by WebView (or maybe WebViewContentsHelper - I need to think about that). content::WebContentsObserver is there so we can get events from RenderViews and RenderFrames without having to modify Chromium. This would avoid making any changes to content::RenderViewHostImpl.

- It doesn't look like BrowserMediaPlayerManager needs to be patched in to Chromium either (nor could it be if MediaWebContentsObserver is implemented in Oxide).

- Does BrowserMediaPlayerManager need to use the media::MediaPlayerManager interface? It looks like that is there to allow media::MediaPlayerAndroid (and media::MediaPlayerBridge) in media/ to talk to the Android implementation of media::MediaPlayerManager in content/, but as we aren't using media::MediaPlayerAndroid it seems unnecessary to implement this interface.

- Similar comments with RendererMediaPlayerManager - it's just a content::RenderFrameObserver. This could be implemented in shared/renderer, and you can use ContentRendererClient::RenderFrameCreated() to create it. This would avoid making changes to content::RenderFrameImpl to add the media_player_manager_ member. You can implement a static method to map from content::RenderFrame to RendererMediaPlayerManager in Oxide that the WebMediaPlayer implementation would use.

- With RendererMediaPlayerManager implemented as a content::RenderFrameObserver in Oxide, WebMediaPlayerOxide would need to move there too. For this, you'd definitely need a small patch which would:
  - Add content::ContentRendererClient::OverrideWebMediaPlayer() that returns a blink::WebMediaPlayer.
  - Modify content::RenderFrameImpl::createMediaPlayer() to call content::ContentRendererClient::OverrideWebMediaPlayer(), falling back to the default if Oxide returns nothing.
We could then implement this in our ContentRendererClient impl in...

Read more...

review: Needs Fixing
833. By Chris Coulson

WebPreferences is leaked if we delete a WebView without ever accessing the preferences

Revision history for this message
Justin McPherson (justinmcp) wrote :

Thank you for the awesome review!

The only thing I am not sure on with these changes, is how I can grab the cookies from oxide to pass to the media request. Any ideas?

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

If you're on the UI thread, you can use BrowserContext::GetCookieStore()

http://bazaar.launchpad.net/~oxide-developers/oxide/oxide.trunk/view/head:/shared/browser/oxide_browser_context.h#L214

As BrowserContext is a UI thread only object, if you're on the IO thread you'll need to find a way to map to BrowserContextIOData and retrieve the CookieStore from there. As an example, see BrowserContextIOData::FromResourceContext(), which maps a content:ResourceContext associated with network requests to a BrowserContextIOData.

834. By Chris Coulson

Get rid of ContentBrowserClient::CreateWebPreferences(), clean up preferences ownership a bit and add some more tests to check the behaviour of WebView.preferences (including testing that WebViews inherit their opener's preferences)

835. By Chris Coulson

Proxy OverrideSystemLocationProvider through PlatformIntegration

836. By Chris Coulson

Get rid of the ContentMainDelegate and ContentBrowserClient implementations from qt/

837. By Chris Coulson

Rename PlatformIntegration to BrowserPlatformIntegration (we'll almost certainly have a RendererPlatformIntegration at some point), and introduce PlatformDelegate

838. By Chris Coulson

Get rid of some uses of ContentClient::GetInstance from Oxide code

839. By Chris Coulson

Remove the last user of ContentClient::GetInstance()

840. By Chris Coulson

Bump Chromium rev to 40.0.2209.0

841. By Chris Coulson

Fix return type for flags propertes on SecurityStatus (doesn't affect the QML API)

842. By Chris Coulson

Add an experimental API for changing the process model (currently untested - it only exists for unbreaking single process mode. Please don't use it yet)

843. By Chris Coulson

Allow other Chromium process models to be selected, add an environment variable (OXIDE_PROCESS_MODEL), and add a warning for unsupported process models

844. By Chris Coulson

Add missing enums

845. By Chris Coulson

Fix arm build

846. By Chris Coulson

Fix a shutdown hang introduced by r838

847. By Chris Coulson

Make the UserScriptSlave destructor private

848. By Chris Coulson

In multi-process mode, a RenderProcessHost is deleted when there are no more objects in the browser process using it. However, in single-process mode there is a single RPH representing the in-process render-thread, which lives until the process dies. This means that we need to keep the BrowserContext alive too, to avoid use-after-free crashes

849. By Chris Coulson

Fix a nullptr deref on mobile

850. By Chris Coulson

Fix a crash when typing

851. By Chris Coulson

Get rid of a reinterpret_cast

852. By Chris Coulson

Make Oxide.processModel authoritative (ie, OXIDE_PROCESS_MODEL only sets the default)

853. By Chris Coulson

Get rid of GetBrowserProcessMainInstance

854. By Chris Coulson

Consolidate the startup-related state in to one class

855. By Chris Coulson

Make WebView.context read-only in single-process mode. In single-process mode, it is not possible to have mutliple WebContexts, so all WebViews will use the default context. However, if single-process is selected from the OXIDE_PROCESS_MODEL environment variable, allow the first application created WebContext to become the default context (so that "WebView { context: WebContext {} }" would still work

856. By Chris Coulson

Add revision metadata for recently added APIs

857. By Chris Coulson

Bump Chromium rev to 40.0.2214.5

858. By Chris Coulson

Disable WebView.incognito in single-process mode - it won't work anyway

859. By Chris Coulson

Don't create an OTR context unnecessarily

860. By Chris Coulson

OxideQQuickGlobal shouldn't be a real singleton - each QQmlEngine assumes ownership of it

861. By Chris Coulson

Don't run the API tests twice (with/without a profile directory). Instead, add a "persistent" property to TestWebContext and allow individual tests to opt out of persistent storage

862. By Chris Coulson

Don't subclass PowerSaveBlockerImpl, as our implementation is never created

863. By Olivier Tilloy

Monitor application state changes, and update PowerSaveBlocker state accordingly
(remove the screen dim lock when the app goes into the background, and restore it when it comes to the foreground).

864. By Chris Coulson

Bump Chromium rev to 40.0.2214.6

865. By Chris Coulson

Don't require cups-config

866. By Chris Coulson

Limit the maximum decoded image size on mobile

867. By Chris Coulson

Revert r866, as it had some other unfinished changes

868. By Chris Coulson

Limit the maximum decoded image size on mobile

869. By Chris Coulson

Gesture detector settings are in DIP units, so don't apply a scale to them. Also, tweak the gesture fling curve (probably needs further refinement)

870. By Chris Coulson

Bump Chromium rev to 40.0.2214.10

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

I've not finished reviewing all of this yet - it's taking me quite a while. But I'll leave my comments for what I have reviewed so far (which is basically everything on the browser side).

Some general comments on coding style:

- Please try to keep lines to 80 characters where possible. It's ok to go over when it really isn't possible to split up a line, but regularly going over this makes it difficult for me to read the code with a vertical split in vim
- All single-statement if blocks should have braces.
- Please use "NULL" rather than "0" for the null value (note, at some point we'll be following Chromium and switching to nullptr).
- The OVERRIDE and FINAL macros have been replaced by the override and final keywords everywhere else now. I thought those macros had actually been removed from Chromium.

I've left other comments inline :)

review: Needs Fixing
Revision history for this message
Justin McPherson (justinmcp) wrote :

NULL is 0 :), I'd rather use nullptr, when is the switch?

871. By Olivier Tilloy

Do not assume the cookie returned by com.canonical.Unity.Screen.keepDisplayOn is > 0
(the very first call actually returns 0, and each subsequent call increments the cookie by 1).

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

I'm aware that they're the same. But NULL is easier to grep than 0.

I plan to switch after we've branched 1.4, which will be when this and bug 1370366 have landed - hopefully by the start of next week ;)

872. By Olivier Tilloy

Expose an API to save/restore the current state of a webview.

873. By Alexandre Abreu

Fix issue with devtools target type

874. By Alexandre Abreu

Fix pt-BR.po and oxide mo inconsistent naming

875. By Chris Coulson

Default to pinch virtual-viewport on mobile (same as Android now)

876. By Chris Coulson

Drop the explicit --disable-gpu-rasterization flag - the blacklist takes care of disabling this globally on linux. Add "OXIDE_IGNORE_GPU_BLACKLIST" environment variable for disabling the blacklist - useful for testing GPU rasterization on the phone, which is totally awesome

877. By Olivier Tilloy

Do not notify the IM extension if the current input method type is none.

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

Thanks for the updates - it's looking better :)

Sorry I've not had a chance to fully review the bits on the renderer side so far. I have had a quick look though and left some comments inline for the standout issues (and the general comments in https://code.launchpad.net/~justinmcp/oxide/mediahub/+merge/240673/comments/596543 apply to the renderer side too).

Revision history for this message
Justin McPherson (justinmcp) :
Revision history for this message
Justin McPherson (justinmcp) wrote :

Not finished yet, have not made recommended adjustments to oxide_content_renderer_client.cc. Feel free to keep looking though :)

878. By Chris Coulson

Update qt version to 1.5.0

879. By Chris Coulson

Bump Chromium rev to 41.0.2224.3

880. By Chris Coulson

Bump Chromium rev to 41.0.2236.0

881. By Chris Coulson

Fix the condition for setting WEBRTC_DETECT_ARM_NEON

882. By Chris Coulson

Remove fuzz from power_save_blocker.patch

883. By Chris Coulson

Ship v8 natives data files

884. By Chris Coulson

Tidy up LoadEvent's for failed loads

885. By Chris Coulson

Call QDesktopServices::openUrl on the UI thread. Even though QDesktopServices is thread safe, it uses QMetaObject::invokeMethod in direct mode making it quite difficult to interact with correctly when using setUrlHandler. Also, fix up tests to not provide dummy handlers in order to force load failures

886. By Chris Coulson

Version LoadEvent.isError

887. By Chris Coulson

Bump Chromium rev to 41.0.2243.0

888. By Chris Coulson

Rename CompositorSoftwareOutputDevice::current_frame_ to CompositorSoftwareOutputDevice::backing_frame_

889. By Chris Coulson

Collapse CompositorThreadProxy and CompositorThreadProxyBase

890. By Chris Coulson

Drop the refcount check from CompositorThreadProxy::DidSwapCompositorFrame - always reclaim the resources for returned frames. It should be a bug for consumer to return frames that are still in use

891. By Chris Coulson

Handle the frontbuffer being deleted on the compositor thread when posting the swap frame to the main thread. This can happen when, eg, hiding a webview (when we delete the LayerTreeHost)

892. By Chris Coulson

r890 requires that CompositorFrameHandle is a RefCountedThreadSafe

893. By Chris Coulson

Actually, add back the refcount check dropped from r890. As CompositorFrameHandle isn't meant to be thread safe, we should ensure that the compositor owns the object at this point before passing it to the compositor thread

894. By Chris Coulson

Add a typedef for the frame handle vector in CompositorThreadProxy

895. By Chris Coulson

Stop using non-const references in CompositorThreadProxy

896. By Chris Coulson

Move the CompositorThreadProxy check from the compositor thread to the UI thread, as it is already for ReclaimResourcesForFrame

897. By Chris Coulson

Stack allocate CompositorFrameAck on the compositor thread in the ReclaimResourcesForFrame path

898. By Chris Coulson

Clean up the GPU thread shim

899. By Chris Coulson

Add a UA stylesheet that sets a minimum viewport width (active on non-desktop form factors), set to the same width as Chrome for Android

900. By Chris Coulson

Only inject the viewport UA override if there is no viewport meta tag, else we end up setting min-width: 980px everywhere (which breaks sites that behave correctly). There is still an edge case that is broken here - if the viewport meta tag is added or removed dynamically then the layout will break

901. By Chris Coulson

Add LocationBarController API, which allows embedders to opt in to having the renderer compositor calculate the UI position. This will be used by the browser to animate the locationbar when scrolling

902. By Chris Coulson

Various fixes in CompositorUtils:
- Don't pass TextureRef between threads, as it's not thread safe. In particular, don't add a reference to it on the GPU thread and then post it to the UI thread with a task that may not ever run (eg, at shutdown).
- Keep all TextureRef's in a map in CompositorUtils, as this allows us to release them all at shutdown in the case where we create a new one after the UI thread has stopped processing tasks. This and the above fix should fix bug 1337506.
- The client ID passed to TextureRef is not the ID of the client process, but the client-side resource ID of the texture (which we don't have, because we don't use the client API). Just pass 0 to this

903. By Chris Coulson

Bump Chromium rev to 41.0.2267.0

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

Finally getting back to reviewing this - sorry for the delay.

I've added some comments inline for recent changes and bits that were missed. I've also added some replies to previous comments (not sure if you get notified of those).

One other general comment - there's still a lot of single-line if() blocks without braces - I'd really like to see those fixed :)

But, it's getting there. Thanks for all the work on this!

review: Needs Fixing
Revision history for this message
Justin McPherson (justinmcp) :
904. By Chris Coulson

Make CertificateError.cancelled work correctly

905. By Chris Coulson

Don't fire TypeCommitted LoadEvents for subframes

906. By Chris Coulson

Fix build with Qt >= 5.4

907. By Chris Coulson

Revert r906, as it's incomplete

908. By Chris Coulson

Fix build with Qt >= 5.4

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

I did add some inline replies to your responses to the original review, but it seems Launchpad didn't commit them all when I left my last comment :(

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

If we can fix the player ID issue, then I'll merge this in and we can fix up remaining issues later :)

909. By Chris Coulson

Fix a build failure on new checkouts

Revision history for this message
Justin McPherson (justinmcp) wrote :

See comments inline

910. By Chris Coulson

Bump Chromium rev to 41.0.2272.2

911. By Chris Coulson

Bump Chromium rev to 41.0.2272.3

912. By Chris Coulson

Refactor WebFrame around RenderFrameHost rather than FrameTreeNode, although we can't get rid of our use of FrameTreeNode just yet. Plus other fixes

913. By Chris Coulson

Drop the NULL checks on root_frame_

914. By Chris Coulson

Ensure V8 snapshot is loaded in the browser process, so single-process mode works

915. By Chris Coulson

Add experimental support for playing audio via mediahub on mobile

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

I've merged this in to trunk now. Thanks for working on it!

I'll report individual bugs next week for any outstanding issues

review: Approve
Revision history for this message
David Barth (dbarth) wrote :

\o/

It is a great milestone !

On Fri, Jan 16, 2015 at 9:53 PM, Chris Coulson <email address hidden>
wrote:

> Review: Approve
>
> I've merged this in to trunk now. Thanks for working on it!
>
> I'll report individual bugs next week for any outstanding issues
> --
> https://code.launchpad.net/~justinmcp/oxide/mediahub/+merge/240673
> Your team Oxide Developers is subscribed to branch lp:oxide.
>

Revision history for this message
Jim Hodapp (jhodapp) wrote :

Yes indeed, well done guys!

Jim

On Mon, Jan 19, 2015 at 4:07 AM, David Barth <email address hidden>
wrote:

> \o/
>
> It is a great milestone !
>
> On Fri, Jan 16, 2015 at 9:53 PM, Chris Coulson <
> <email address hidden>>
> wrote:
>
> > Review: Approve
> >
> > I've merged this in to trunk now. Thanks for working on it!
> >
> > I'll report individual bugs next week for any outstanding issues
> > --
> > https://code.launchpad.net/~justinmcp/oxide/mediahub/+merge/240673
> > Your team Oxide Developers is subscribed to branch lp:oxide.
> >
>
> --
> https://code.launchpad.net/~justinmcp/oxide/mediahub/+merge/240673
> Your team Oxide Developers is subscribed to branch lp:oxide.
>

Revision history for this message
Alexandre Abreu (abreu-alexandre) wrote :

+1 !

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