Merge lp://staging/~justinmcp/oxide/mediahub into lp://staging/~oxide-developers/oxide/oxide.trunk
- mediahub
- Merge into oxide.trunk
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 |
Related bugs: |
Reviewer | Review Type | Date Requested | Status |
---|---|---|---|
Chris Coulson | Approve | ||
Review via email:
|
Commit message
Support for Audio playback through media hub.
Description of the change

Justin McPherson (justinmcp) wrote : | # |

Chris Coulson (chrisccoulson) wrote : | # |
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.
- MediaWebContent
- It doesn't look like BrowserMediaPla
- Does BrowserMediaPla
- Similar comments with RendererMediaPl
- With RendererMediaPl
- Add content:
- Modify content:
We could then implement this in our ContentRenderer
- 833. By Chris Coulson
-
WebPreferences is leaked if we delete a WebView without ever accessing the preferences

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?

Chris Coulson (chrisccoulson) wrote : | # |
If you're on the UI thread, you can use BrowserContext:
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 BrowserContextI
- 834. By Chris Coulson
-
Get rid of ContentBrowserC
lient:: CreateWebPrefer ences() , 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 OverrideSystemL
ocationProvider through PlatformIntegration - 836. By Chris Coulson
-
Get rid of the ContentMainDelegate and ContentBrowserC
lient implementations from qt/ - 837. By Chris Coulson
-
Rename PlatformIntegration to BrowserPlatform
Integration (we'll almost certainly have a RendererPlatfor mIntegration 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 GetBrowserProce
ssMainInstance - 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 PowerSaveBlocke
rImpl, 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

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 :)

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).

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-rasterizati on 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.

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:/

Justin McPherson (justinmcp) : | # |

Justin McPherson (justinmcp) wrote : | # |
Not finished yet, have not made recommended adjustments to oxide_content_
- 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 QDesktopService
s::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 CompositorSoftw
areOutputDevice ::current_ frame_ to CompositorSoftw areOutputDevice ::backing_ frame_ - 889. By Chris Coulson
-
Collapse CompositorThrea
dProxy and CompositorThrea dProxyBase - 890. By Chris Coulson
-
Drop the refcount check from CompositorThrea
dProxy: :DidSwapComposi torFrame - 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 CompositorFrame
Handle is a RefCountedThrea dSafe - 893. By Chris Coulson
-
Actually, add back the refcount check dropped from r890. As CompositorFrame
Handle 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 CompositorThrea
dProxy - 895. By Chris Coulson
-
Stop using non-const references in CompositorThrea
dProxy - 896. By Chris Coulson
-
Move the CompositorThrea
dProxy check from the compositor thread to the UI thread, as it is already for ReclaimResource sForFrame - 897. By Chris Coulson
-
Stack allocate CompositorFrameAck on the compositor thread in the ReclaimResource
sForFrame 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 LocationBarCont
roller 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

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!

Justin McPherson (justinmcp) : | # |
- 904. By Chris Coulson
-
Make CertificateErro
r.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

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 :(

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

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

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

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:/
> Your team Oxide Developers is subscribed to branch lp:oxide.
>

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:/
> > Your team Oxide Developers is subscribed to branch lp:oxide.
> >
>
> --
> https:/
> Your team Oxide Developers is subscribed to branch lp:oxide.
>

Alexandre Abreu (abreu-alexandre) wrote : | # |
+1 !
Requires https:/ /code.launchpad .net/~phablet- team/media- hub/oxide- support, to be merged.